New OP-TEE SHM design.

[lorc]

Those patches are kernel driver part of dynamic shared memory in OP-TEE (refer to https://github.com/OP-TEE/optee_os/pull/1232). Two of those patches are authored by @jenswi-linaro, I just tidied it a bit and rebased on current master.

This patches provide new ioctl to userpace along with support in tee_shm and op-tee kernel part. This ioctl allows userspace process to share own memory with OP-TEE.

Another part - is OP-TEE specific SHM pool that uses kernel memory. With this approach we don't rely on OP-TEE predefined pool.

[lorc]

Addressed @etienne-lms comments, rebased on actual branch.

[lorc]

Rebased, addressed comments. Also added new patch (for privileged calls) and simplified RPC memory allocation.

@jenswi-linaro, own shared memory pool needed because I want to use kernel memory for command buffers.

[jenswi-linaro]

@lorc, kmalloc() should be able to supply the memory needed for command data and vmalloc() for the larger allocation, or am I missing something?

[lorc]

@jenswi-linaro, you are right. Actually, "new shared memory pool" is not a right name for it. But it is the term used in tee code. It is more like an allocator. New "pool" uses kmalloc() to allocate command buffers. And it uses OP-TEE shared memory region to allocate dma_buf regions, so TEE_IOC_SHM_ALLOC will allocate memory from that plain old 2M buffer. It was left for compatibility because new clients will not use TEE_IOC_SHM_ALLOC.

[lorc]

Hi. I just rebased this on actual branch and also added delta patch with new ABI and new RPC cleanup approach.

[lorc]

Addressed comments (and also checkpatch issues). Removed function tee_shm_vmap() with friends (it was used in prev. ABI version).

[lorc]

added v3 delta patch, where get_pages_array_size() was made static function

[lorc]

@jenswi-linaro, thank you for your pull request. I used your patches for tee as is, but I had to squash your patches for shm_pool into mine ones, because I changed patches layout. As you can see, now there are a lot more patches. I tried to separate different parts of this big feature into separate patches.

But now I'm facing the problem. As you suggested, I reverted that part for your patch that changed ctx to teedev. Problem is that I can't unregister shared buffer because it's context is being erased on close:

static void teedev_close_context(struct tee_context *ctx)
{
    struct tee_shm *shm;

    ctx->teedev->desc->ops->release(ctx);
    mutex_lock(&ctx->teedev->mutex);
    list_for_each_entry(shm, &ctx->list_shm, link)
        shm->ctx = NULL; <====== Right there
    mutex_unlock(&ctx->teedev->mutex);
    tee_device_put(ctx->teedev);
    kfree(ctx);
}

So, it all is fails with this backtrace:

[    4.712751] [<ffff000008774f80>] tee_shm_alloc+0x10/0x20
[    4.712860] [<ffff000008775ba4>] get_msg_arg+0x34/0xd0
[    4.712962] [<ffff000008776610>] optee_shm_unregister+0x28/0xb0
[    4.713070] [<ffff0000087744d0>] tee_shm_do_release+0xd8/0xf8
[    4.713174] [<ffff000008774558>] tee_shm_release+0x68/0x80
[    4.713275] [<ffff000008774580>] tee_shm_op_release+0x10/0x18
[    4.713382] [<ffff00000853ab58>] dma_buf_release+0x60/0x1a0
[    4.713508] [<ffff0000081daad4>] __fput+0x8c/0x1d0

Because tee_shm_alloc() receives null context.

I think, this is a reason, why you used teedev instead of ctx in the first place.

Looks like we need to add reference counter to tee_ctx to make it live until last shm will be freed. Is this right approach?

[jenswi-linaro]

Aha, yes I think using a reference counter is more clear.

[lorc]

Hmm, looks like it is harder, than I expected. Problem is that optee_release() also uses shm objects. This increases/decreases reference count in tee_context which leads to recursion during tee_context release. I employed a dirty hack to overcome this issue. You can see it in this patch: https://github.com/lorc/linux/commit/8bea4e4316366e65be2aeb9d9797488c8b021435 So, currently I'm looking for a proper solution. Actually, I can call optee_release() from teedev_close_context() as it was done earlier. Then there will be no recursion. But I'm not sure if this is semantically correct: we are telling TEE driver, that it can release context, but then we are using this context to unregister shared memory buffer.

[lorc]

Okay, I added flag to avoid recursion: https://github.com/lorc/linux/commit/108bb599655dfc244f85ffcb8816590d1603e047 At least this does not look as ugly, as the first approach. Maybe I should return kref back, instead of raw atomics usage.

[lorc]

Ping?

[jenswi-linaro]

I'm just back from vacation, I'll resume reviewing this.

[lorc]

I also pushed new patch that inlines tee_shm_is_registered() and tee_shm_get_id().

I'm not sure if I have to inline another small functions like tee_shm_put() or tee_shm_get_pa()/tee_shm_get_va(). Please let me know, if I should include them in the patch.

[jenswi-linaro]

Further inlining can wait until after this has been merged.

Looks good to me. If @etienne-lms is OK too, please squash and rebase to make it ready for merge.

Keep in mind that this is only the first review. As second review is expected to take place at the kernel mailing lists. During that review we'll keep stuff up to date on https://github.com/linaro-swg/linux/tree/optee

[lorc]

Hello. I have applied last comments by @etienne-lms, squashed, rebased and uploaded patch series. Apart that I added static assert and TODO: comment to the patch https://github.com/linaro-swg/linux/pull/29/commits/0aef7906585cf11daabdc6849646c60d6fa5b66d#diff-f4d6b53ce7cf409265b04a53ae7d121bR467 (tee: optee: add page list manipulation functions):

    /* TODO: add support for RichOS page sizes that != 4096 */
    BUILD_BUG_ON(PAGE_SIZE != OPTEE_MSG_NONCONTIG_PAGE_SIZE);

[lorc]

Hello. So, what is next?

[jbech-linaro]

Hi @lorc , I think we should merge this on our linaro-swg/linux/optee branch, but then as Jens suggested, next thing is to send it to the kernel mailinglist, since that is where we like this to end up in the end. @etienne-lms @jenswi-linaro and other comments left?

[lorc]

Hi @jbech-linaro, sure, I'll submit this patch series to LKML. I just want to be sure that you guys are okay with the patches.

[jenswi-linaro]

1. Merge https://github.com/OP-TEE/optee_os/pull/1631
2. Merge https://github.com/linaro-swg/linux/pull/29 (with a "[NEW SHM]" prefix in the title on all commits to make it easy to identify the commits which are part of this and should be upstreamed)
3. Merge https://github.com/OP-TEE/optee_client/pull/72

Once that's done it's time for @lorc to send the linux patches to LKML and take another round of review there. During the review process we'll keep the testable setup in our development branch (optee) which we'll update with delta patches as needed.

[jforissier]

+1 with what @jenswi-linaro said. Let me add action items:

1. Merge OP-TEE/optee_os#1631

Action @lorc : rebase onto current master, squash some commits together, apply R-b tags. See also my comment about rephrasing some commit subjects (but feel free to ignore them if it turns out I did not understand correctly!). I'll merge the PR as soon as this is done.

2. Merge #29 (with a "[NEW SHM]" prefix in the title on all commits to make it easy to identify the commits which are part of this and should be upstreamed)

Action @lorc : add the "[NEW SHM]" prefix. @jenswi-linaro : should A-b or R-b be added at this point?

3. Merge OP-TEE/optee_client#72

Action @lorc : address pending comments, rebase onto master (resolve conflicts). Then we will gather the A-b/R-b and merge.

This is a cool feature and I'd really like to see it merged soon! So let's finalize this stuff :) Thanks.

[jenswi-linaro]

Action @lorc : add the "[NEW SHM]" prefix. @jenswi-linaro : should A-b or R-b be added at this point?

No, I'd rather take that on LKML. In the end I expect that I'll send the outcome of the review as a separate pull request to arm-soc with my S-o-B added to all commits.

[lorc]

Thank you for review, Etienne. I addressed your comments right in appropriate patches. Also I added [NEW SHM] prefix to whole series.

[lorc]

If there are no other comments on this PR, I'm going to post this patches to LKML later today.

[lorc]

Ah, this is not so easy, as I expected :) It will take some time to resolve conflicts with master branch. @jenswi-linaro, there are your S-o-b tag on your patches, that I'm going to submit as a part of the series. But tee: add register user memory was reworked by me. Do you wish me to preserve your S-o-b tag?

(Honestly, it is the first time, I'm submitting сo-authored patches, I don't quite what is the correct procedure).

[jenswi-linaro]

@lorc, I'm OK either way with tee: add register user memory, I guess I would have kept all S-o-bs.