| Message ID | 1506621851-6929-8-git-send-email-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 28, 2017 at 09:04:04PM +0300, Volodymyr Babchuk wrote: > From: Volodymyr Babchuk <vlad.babchuk@gmail.com> > > This change adds ops for shm_(un)register functions in tee interface. > Client application can use these functions to (un)register an own shared > buffer in OP-TEE address space. This allows zero copy data sharing between > Normal and Secure Worlds. > > Please note that while those functions were added to optee code, > it does not report to userspace that those functions are available. > OP-TEE code does not set TEE_GEN_CAP_REG_MEM flag. This flag will be > enabled only after all other features of dynamic shared memory will be > implemented in subsequent patches. While it's not adveritsed to the user, AFAICT the user could still invoke these via ioctls, right? Is there a problem if the user were to do so, or is it simply not useful without the other features? [...] > +int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, > + struct page **pages, size_t num_pages) > +{ > + pages_array = optee_allocate_pages_array(num_pages); > + if (!pages_array) > + return -ENOMEM; > + msg_arg->params->u.tmem.buf_ptr = virt_to_phys(pages_array) | > + tee_shm_get_page_offset(shm); This doesn't look right. Why is the shm page offset being orred-in to the pages_array physical address? They're completely separate objects. Thanks, Mark.
On 29.09.17 16:06, Mark Rutland wrote: > On Thu, Sep 28, 2017 at 09:04:04PM +0300, Volodymyr Babchuk wrote: >> From: Volodymyr Babchuk <vlad.babchuk@gmail.com> >> >> This change adds ops for shm_(un)register functions in tee interface. >> Client application can use these functions to (un)register an own shared >> buffer in OP-TEE address space. This allows zero copy data sharing between >> Normal and Secure Worlds. >> >> Please note that while those functions were added to optee code, >> it does not report to userspace that those functions are available. >> OP-TEE code does not set TEE_GEN_CAP_REG_MEM flag. This flag will be >> enabled only after all other features of dynamic shared memory will be >> implemented in subsequent patches. > > While it's not adveritsed to the user, AFAICT the user could still > invoke these via ioctls, right? > Is there a problem if the user were to do so, or is it simply not useful > without the other features? Yes, user can invoke this via ioctl. And this buffer will be registeredin OP-TEE. But user will not be able to use this registered buffer, because optee driver does not know how to handle references to registered buffers. This is a complex feature and I tried to split it into different commits to ease up review. Probably, I can remove + .shm_register = optee_shm_register, + .shm_unregister = optee_shm_unregister, from this patch, so user will be not able to call this functions at all. How do you thing? Should I do this? > >> +int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, >> + struct page **pages, size_t num_pages) >> +{ > >> + pages_array = optee_allocate_pages_array(num_pages); >> + if (!pages_array) >> + return -ENOMEM; > >> + msg_arg->params->u.tmem.buf_ptr = virt_to_phys(pages_array) | >> + tee_shm_get_page_offset(shm); > > This doesn't look right. Why is the shm page offset being orred-in to > the pages_array physical address? They're completely separate objects. They present the same registered shared buffer. You have a list of pages and offset from the first page. Strictly speaking this is not buf_ptr anymore, but it is named so... This is a part of OP-TEE ABI. We considered different approaches at [1]. This fits into general ABI design. [1] https://github.com/OP-TEE/optee_os/pull/1232#issuecomment-301851514
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c index f8e044d..ec53dca 100644 --- a/drivers/tee/optee/call.c +++ b/drivers/tee/optee/call.c @@ -490,3 +490,67 @@ void optee_free_pages_array(void *array, size_t num_entries) free_pages_exact(array, get_pages_array_size(num_entries)); } +int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, + struct page **pages, size_t num_pages) +{ + struct tee_shm *shm_arg = NULL; + struct optee_msg_arg *msg_arg; + u64 *pages_array; + phys_addr_t msg_parg; + int rc = 0; + + if (!num_pages) + return -EINVAL; + + pages_array = optee_allocate_pages_array(num_pages); + if (!pages_array) + return -ENOMEM; + + shm_arg = get_msg_arg(ctx, 1, &msg_arg, &msg_parg); + if (IS_ERR(shm_arg)) { + rc = PTR_ERR(shm_arg); + goto out; + } + + optee_fill_pages_list(pages_array, pages, num_pages); + + msg_arg->cmd = OPTEE_MSG_CMD_REGISTER_SHM; + msg_arg->params->attr = OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT | + OPTEE_MSG_ATTR_NONCONTIG; + msg_arg->params->u.tmem.shm_ref = (unsigned long)shm; + msg_arg->params->u.tmem.size = tee_shm_get_size(shm); + msg_arg->params->u.tmem.buf_ptr = virt_to_phys(pages_array) | + tee_shm_get_page_offset(shm); + + if (optee_do_call_with_arg(ctx, msg_parg) || + msg_arg->ret != TEEC_SUCCESS) + rc = -EINVAL; + + tee_shm_free(shm_arg); +out: + optee_free_pages_array(pages_array, num_pages); + return rc; +} + +int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm) +{ + struct tee_shm *shm_arg; + struct optee_msg_arg *msg_arg; + phys_addr_t msg_parg; + int rc = 0; + + shm_arg = get_msg_arg(ctx, 1, &msg_arg, &msg_parg); + if (IS_ERR(shm_arg)) + return PTR_ERR(shm_arg); + + msg_arg->cmd = OPTEE_MSG_CMD_UNREGISTER_SHM; + + msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; + msg_arg->params[0].u.rmem.shm_ref = (unsigned long)shm; + + if (optee_do_call_with_arg(ctx, msg_parg) || + msg_arg->ret != TEEC_SUCCESS) + rc = -EINVAL; + tee_shm_free(shm_arg); + return rc; +} diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 7952357..4d448bf 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -267,6 +267,8 @@ static const struct tee_driver_ops optee_ops = { .close_session = optee_close_session, .invoke_func = optee_invoke_func, .cancel_req = optee_cancel_req, + .shm_register = optee_shm_register, + .shm_unregister = optee_shm_unregister, }; static const struct tee_desc optee_desc = { diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index caa3c04..3ea7f7a 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -160,6 +160,10 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session); void optee_enable_shm_cache(struct optee *optee); void optee_disable_shm_cache(struct optee *optee); +int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, + struct page **pages, size_t num_pages); +int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm); + int optee_from_msg_param(struct tee_param *params, size_t num_params, const struct optee_msg_param *msg_params); int optee_to_msg_param(struct optee_msg_param *msg_params, size_t num_params,