Messages in this thread | | | From | Sumit Garg <> | Date | Thu, 29 Jul 2021 12:45:12 +0530 | Subject | Re: [PATCH v3 3/5] optee: refactor driver with internal callbacks |
| |
On Thu, 29 Jul 2021 at 12:26, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > On Wed, Jul 28, 2021 at 03:29:33PM +0530, Sumit Garg wrote: > > On Thu, 22 Jul 2021 at 17:48, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > > > > > The OP-TEE driver is refactored with three internal callbacks replacing > > > direct calls to optee_from_msg_param(), optee_to_msg_param() and > > > optee_do_call_with_arg(). > > > > > > These functions a central to communicating with OP-TEE in secure world > > > by using the SMC Calling Convention directly. > > > > > > This refactoring makes room for using other primitives to communicate > > > with OP-TEE in secure world while being able to reuse as much as > > > possible from the present driver. > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > > > --- > > > drivers/tee/optee/call.c | 86 +++++++++-------- > > > drivers/tee/optee/core.c | 148 ++++++++++++++++++++---------- > > > drivers/tee/optee/optee_private.h | 35 +++++-- > > > drivers/tee/optee/rpc.c | 19 ++-- > > > 4 files changed, 182 insertions(+), 106 deletions(-) > > > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > > > index 9d8f5a95e42f..00ecd794e59a 100644 > > > --- a/drivers/tee/optee/call.c > > > +++ b/drivers/tee/optee/call.c > > > @@ -1,6 +1,6 @@ > > > // SPDX-License-Identifier: GPL-2.0-only > > > /* > > > - * Copyright (c) 2015, Linaro Limited > > > + * Copyright (c) 2015-2021, Linaro Limited > > > */ > > > #include <linux/arm-smccc.h> > > > #include <linux/device.h> > > > @@ -118,20 +118,25 @@ static struct optee_session *find_session(struct optee_context_data *ctxdata, > > > /** > > > * optee_do_call_with_arg() - Do an SMC to OP-TEE in secure world > > > * @ctx: calling context > > > - * @parg: physical address of message to pass to secure world > > > + * @arg: shared memory holding the message to pass to secure world > > > * > > > * Does and SMC to OP-TEE in secure world and handles eventual resulting > > > * Remote Procedure Calls (RPC) from OP-TEE. > > > * > > > * Returns return code from secure world, 0 is OK > > > */ > > > -u32 optee_do_call_with_arg(struct tee_context *ctx, phys_addr_t parg) > > > +int optee_do_call_with_arg(struct tee_context *ctx, struct tee_shm *arg) > > > { > > > struct optee *optee = tee_get_drvdata(ctx->teedev); > > > struct optee_call_waiter w; > > > struct optee_rpc_param param = { }; > > > struct optee_call_ctx call_ctx = { }; > > > - u32 ret; > > > + phys_addr_t parg; > > > + int rc; > > > + > > > + rc = tee_shm_get_pa(arg, 0, &parg); > > > + if (rc) > > > + return rc; > > > > > > param.a0 = OPTEE_SMC_CALL_WITH_ARG; > > > reg_pair_from_64(¶m.a1, ¶m.a2, parg); > > > @@ -160,7 +165,7 @@ u32 optee_do_call_with_arg(struct tee_context *ctx, phys_addr_t parg) > > > param.a3 = res.a3; > > > optee_handle_rpc(ctx, ¶m, &call_ctx); > > > } else { > > > - ret = res.a0; > > > + rc = res.a0; > > > break; > > > } > > > } > > > @@ -172,14 +177,12 @@ u32 optee_do_call_with_arg(struct tee_context *ctx, phys_addr_t parg) > > > */ > > > optee_cq_wait_final(&optee->call_queue, &w); > > > > > > - return ret; > > > + return rc; > > > } > > > > > > static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params, > > > - struct optee_msg_arg **msg_arg, > > > - phys_addr_t *msg_parg) > > > + struct optee_msg_arg **msg_arg) > > > { > > > - int rc; > > > struct tee_shm *shm; > > > struct optee_msg_arg *ma; > > > > > > @@ -190,22 +193,13 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params, > > > > > > ma = tee_shm_get_va(shm, 0); > > > if (IS_ERR(ma)) { > > > - rc = PTR_ERR(ma); > > > - goto out; > > > + tee_shm_free(shm); > > > + return (void *)ma; > > > } > > > > > > - rc = tee_shm_get_pa(shm, 0, msg_parg); > > > - if (rc) > > > - goto out; > > > - > > > memset(ma, 0, OPTEE_MSG_GET_ARG_SIZE(num_params)); > > > ma->num_params = num_params; > > > *msg_arg = ma; > > > -out: > > > - if (rc) { > > > - tee_shm_free(shm); > > > - return ERR_PTR(rc); > > > - } > > > > > > return shm; > > > } > > > @@ -214,16 +208,16 @@ int optee_open_session(struct tee_context *ctx, > > > struct tee_ioctl_open_session_arg *arg, > > > struct tee_param *param) > > > { > > > + struct optee *optee = tee_get_drvdata(ctx->teedev); > > > struct optee_context_data *ctxdata = ctx->data; > > > int rc; > > > struct tee_shm *shm; > > > struct optee_msg_arg *msg_arg; > > > - phys_addr_t msg_parg; > > > struct optee_session *sess = NULL; > > > uuid_t client_uuid; > > > > > > /* +2 for the meta parameters added below */ > > > - shm = get_msg_arg(ctx, arg->num_params + 2, &msg_arg, &msg_parg); > > > + shm = get_msg_arg(ctx, arg->num_params + 2, &msg_arg); > > > if (IS_ERR(shm)) > > > return PTR_ERR(shm); > > > > > > @@ -247,7 +241,8 @@ int optee_open_session(struct tee_context *ctx, > > > goto out; > > > export_uuid(msg_arg->params[1].u.octets, &client_uuid); > > > > > > - rc = optee_to_msg_param(msg_arg->params + 2, arg->num_params, param); > > > + rc = optee->ops->to_msg_param(optee, msg_arg->params + 2, > > > + arg->num_params, param); > > > if (rc) > > > goto out; > > > > > > @@ -257,7 +252,7 @@ int optee_open_session(struct tee_context *ctx, > > > goto out; > > > } > > > > > > - if (optee_do_call_with_arg(ctx, msg_parg)) { > > > + if (optee->ops->do_call_with_arg(ctx, shm)) { > > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > > } > > > @@ -272,7 +267,8 @@ int optee_open_session(struct tee_context *ctx, > > > kfree(sess); > > > } > > > > > > - if (optee_from_msg_param(param, arg->num_params, msg_arg->params + 2)) { > > > + if (optee->ops->from_msg_param(optee, param, arg->num_params, > > > + msg_arg->params + 2)) { > > > arg->ret = TEEC_ERROR_COMMUNICATION; > > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > > /* Close session again to avoid leakage */ > > > @@ -291,16 +287,16 @@ int optee_open_session(struct tee_context *ctx, > > > int optee_close_session_helper(struct tee_context *ctx, u32 session) > > > { > > > struct tee_shm *shm; > > > + struct optee *optee = tee_get_drvdata(ctx->teedev); > > > struct optee_msg_arg *msg_arg; > > > - phys_addr_t msg_parg; > > > > > > - shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg); > > > + shm = get_msg_arg(ctx, 0, &msg_arg); > > > if (IS_ERR(shm)) > > > return PTR_ERR(shm); > > > > > > msg_arg->cmd = OPTEE_MSG_CMD_CLOSE_SESSION; > > > msg_arg->session = session; > > > - optee_do_call_with_arg(ctx, msg_parg); > > > + optee->ops->do_call_with_arg(ctx, shm); > > > > > > tee_shm_free(shm); > > > > > > @@ -328,10 +324,10 @@ int optee_close_session(struct tee_context *ctx, u32 session) > > > int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > struct tee_param *param) > > > { > > > + struct optee *optee = tee_get_drvdata(ctx->teedev); > > > struct optee_context_data *ctxdata = ctx->data; > > > struct tee_shm *shm; > > > struct optee_msg_arg *msg_arg; > > > - phys_addr_t msg_parg; > > > struct optee_session *sess; > > > int rc; > > > > > > @@ -342,7 +338,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > if (!sess) > > > return -EINVAL; > > > > > > - shm = get_msg_arg(ctx, arg->num_params, &msg_arg, &msg_parg); > > > + shm = get_msg_arg(ctx, arg->num_params, &msg_arg); > > > if (IS_ERR(shm)) > > > return PTR_ERR(shm); > > > msg_arg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND; > > > @@ -350,16 +346,18 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > msg_arg->session = arg->session; > > > msg_arg->cancel_id = arg->cancel_id; > > > > > > - rc = optee_to_msg_param(msg_arg->params, arg->num_params, param); > > > + rc = optee->ops->to_msg_param(optee, msg_arg->params, arg->num_params, > > > + param); > > > if (rc) > > > goto out; > > > > > > - if (optee_do_call_with_arg(ctx, msg_parg)) { > > > + if (optee->ops->do_call_with_arg(ctx, shm)) { > > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > > } > > > > > > - if (optee_from_msg_param(param, arg->num_params, msg_arg->params)) { > > > + if (optee->ops->from_msg_param(optee, param, arg->num_params, > > > + msg_arg->params)) { > > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > > } > > > @@ -373,10 +371,10 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > > > > int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session) > > > { > > > + struct optee *optee = tee_get_drvdata(ctx->teedev); > > > struct optee_context_data *ctxdata = ctx->data; > > > struct tee_shm *shm; > > > struct optee_msg_arg *msg_arg; > > > - phys_addr_t msg_parg; > > > struct optee_session *sess; > > > > > > /* Check that the session is valid */ > > > @@ -386,14 +384,14 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session) > > > if (!sess) > > > return -EINVAL; > > > > > > - shm = get_msg_arg(ctx, 0, &msg_arg, &msg_parg); > > > + shm = get_msg_arg(ctx, 0, &msg_arg); > > > if (IS_ERR(shm)) > > > return PTR_ERR(shm); > > > > > > msg_arg->cmd = OPTEE_MSG_CMD_CANCEL; > > > msg_arg->session = session; > > > msg_arg->cancel_id = cancel_id; > > > - optee_do_call_with_arg(ctx, msg_parg); > > > + optee->ops->do_call_with_arg(ctx, shm); > > > > > > tee_shm_free(shm); > > > return 0; > > > @@ -592,10 +590,10 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, > > > struct page **pages, size_t num_pages, > > > unsigned long start) > > > { > > > - struct tee_shm *shm_arg = NULL; > > > + struct optee *optee = tee_get_drvdata(ctx->teedev); > > > struct optee_msg_arg *msg_arg; > > > + struct tee_shm *shm_arg; > > > u64 *pages_list; > > > - phys_addr_t msg_parg; > > > int rc; > > > > > > if (!num_pages) > > > @@ -609,7 +607,7 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, > > > if (!pages_list) > > > return -ENOMEM; > > > > > > - shm_arg = get_msg_arg(ctx, 1, &msg_arg, &msg_parg); > > > + shm_arg = get_msg_arg(ctx, 1, &msg_arg); > > > if (IS_ERR(shm_arg)) { > > > rc = PTR_ERR(shm_arg); > > > goto out; > > > @@ -630,7 +628,7 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, > > > msg_arg->params->u.tmem.buf_ptr = virt_to_phys(pages_list) | > > > (tee_shm_get_page_offset(shm) & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1)); > > > > > > - if (optee_do_call_with_arg(ctx, msg_parg) || > > > + if (optee->ops->do_call_with_arg(ctx, shm_arg) || > > > msg_arg->ret != TEEC_SUCCESS) > > > rc = -EINVAL; > > > > > > @@ -642,12 +640,12 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm, > > > > > > int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm) > > > { > > > - struct tee_shm *shm_arg; > > > + struct optee *optee = tee_get_drvdata(ctx->teedev); > > > struct optee_msg_arg *msg_arg; > > > - phys_addr_t msg_parg; > > > + struct tee_shm *shm_arg; > > > int rc = 0; > > > > > > - shm_arg = get_msg_arg(ctx, 1, &msg_arg, &msg_parg); > > > + shm_arg = get_msg_arg(ctx, 1, &msg_arg); > > > if (IS_ERR(shm_arg)) > > > return PTR_ERR(shm_arg); > > > > > > @@ -656,7 +654,7 @@ int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *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) || > > > + if (optee->ops->do_call_with_arg(ctx, shm_arg) || > > > msg_arg->ret != TEEC_SUCCESS) > > > rc = -EINVAL; > > > tee_shm_free(shm_arg); > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > > index 949223b214c3..f689f171a794 100644 > > > --- a/drivers/tee/optee/core.c > > > +++ b/drivers/tee/optee/core.c > > > @@ -1,6 +1,6 @@ > > > // SPDX-License-Identifier: GPL-2.0-only > > > /* > > > - * Copyright (c) 2015, Linaro Limited > > > + * Copyright (c) 2015-2021, Linaro Limited > > > */ > > > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > @@ -26,21 +26,87 @@ > > > > > > #define OPTEE_SHM_NUM_PRIV_PAGES CONFIG_OPTEE_SHM_NUM_PRIV_PAGES > > > > > > +static void from_msg_param_value(struct tee_param *p, u32 attr, > > > + const struct optee_msg_param *mp) > > > +{ > > > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > > + attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > > + p->u.value.a = mp->u.value.a; > > > + p->u.value.b = mp->u.value.b; > > > + p->u.value.c = mp->u.value.c; > > > +} > > > + > > > +static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > > + const struct optee_msg_param *mp) > > > +{ > > > + struct tee_shm *shm; > > > + phys_addr_t pa; > > > + int rc; > > > + > > > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > > + attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > > > + p->u.memref.size = mp->u.tmem.size; > > > + shm = (struct tee_shm *)(unsigned long)mp->u.tmem.shm_ref; > > > + if (!shm) { > > > + p->u.memref.shm_offs = 0; > > > + p->u.memref.shm = NULL; > > > + return 0; > > > + } > > > + > > > + rc = tee_shm_get_pa(shm, 0, &pa); > > > + if (rc) > > > + return rc; > > > + > > > + p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; > > > + p->u.memref.shm = shm; > > > + > > > + /* Check that the memref is covered by the shm object */ > > > + if (p->u.memref.size) { > > > + size_t o = p->u.memref.shm_offs + > > > + p->u.memref.size - 1; > > > + > > > + rc = tee_shm_get_pa(shm, o, NULL); > > > + if (rc) > > > + return rc; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > > + const struct optee_msg_param *mp) > > > +{ > > > + struct tee_shm *shm; > > > + > > > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > > + attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > > > + p->u.memref.size = mp->u.rmem.size; > > > + shm = (struct tee_shm *)(unsigned long)mp->u.rmem.shm_ref; > > > + > > > + if (shm) { > > > + p->u.memref.shm_offs = mp->u.rmem.offs; > > > + p->u.memref.shm = shm; > > > + } else { > > > + p->u.memref.shm_offs = 0; > > > + p->u.memref.shm = NULL; > > > + } > > > +} > > > + > > > /** > > > * optee_from_msg_param() - convert from OPTEE_MSG parameters to > > > * struct tee_param > > > + * @optee: main service struct > > > > I don't see this new argument being used. Can you throw some light on > > the use-case for this additional argument? > > This function is registered as a callback and the counter part in the > FF-A ABI (optee_ffa_from_msg_param()) will need this parameter. So it's > needed to have the same signature. >
Okay, I see its usage.
> > > > > * @params: subsystem internal parameter representation > > > * @num_params: number of elements in the parameter arrays > > > * @msg_params: OPTEE_MSG parameters > > > * Returns 0 on success or <0 on failure > > > */ > > > -int optee_from_msg_param(struct tee_param *params, size_t num_params, > > > - const struct optee_msg_param *msg_params) > > > +static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > > + size_t num_params, > > > + const struct optee_msg_param *msg_params) > > > { > > > int rc; > > > size_t n; > > > - struct tee_shm *shm; > > > - phys_addr_t pa; > > > > > > for (n = 0; n < num_params; n++) { > > > struct tee_param *p = params + n; > > > @@ -55,48 +121,19 @@ int optee_from_msg_param(struct tee_param *params, size_t num_params, > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > > - attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > > - p->u.value.a = mp->u.value.a; > > > - p->u.value.b = mp->u.value.b; > > > - p->u.value.c = mp->u.value.c; > > > + from_msg_param_value(p, attr, mp); > > > break; > > > case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT: > > > case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: > > > case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: > > > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > > - attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > > > - p->u.memref.size = mp->u.tmem.size; > > > - shm = (struct tee_shm *)(unsigned long) > > > - mp->u.tmem.shm_ref; > > > - if (!shm) { > > > - p->u.memref.shm_offs = 0; > > > - p->u.memref.shm = NULL; > > > - break; > > > - } > > > - rc = tee_shm_get_pa(shm, 0, &pa); > > > + rc = from_msg_param_tmp_mem(p, attr, mp); > > > if (rc) > > > return rc; > > > - p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; > > > - p->u.memref.shm = shm; > > > break; > > > case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: > > > case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: > > > case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: > > > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > > - attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > > > - p->u.memref.size = mp->u.rmem.size; > > > - shm = (struct tee_shm *)(unsigned long) > > > - mp->u.rmem.shm_ref; > > > - > > > - if (!shm) { > > > - p->u.memref.shm_offs = 0; > > > - p->u.memref.shm = NULL; > > > - break; > > > - } > > > - p->u.memref.shm_offs = mp->u.rmem.offs; > > > - p->u.memref.shm = shm; > > > - > > > + from_msg_param_reg_mem(p, attr, mp); > > > break; > > > > > > default: > > > @@ -106,6 +143,16 @@ int optee_from_msg_param(struct tee_param *params, size_t num_params, > > > return 0; > > > } > > > > > > +static void to_msg_param_value(struct optee_msg_param *mp, > > > + const struct tee_param *p) > > > +{ > > > + mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > > + mp->u.value.a = p->u.value.a; > > > + mp->u.value.b = p->u.value.b; > > > + mp->u.value.c = p->u.value.c; > > > +} > > > + > > > static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > > const struct tee_param *p) > > > { > > > @@ -148,13 +195,15 @@ static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > > > > > /** > > > * optee_to_msg_param() - convert from struct tee_params to OPTEE_MSG parameters > > > + * @optee: main service struct > > > > Same here. > > This function is also registered as a callback. Here it's questionable > though, but I added it to let optee->ops->to_msg_param() and > optee->ops->from_msg_param() have better symmetry. >
I am fine with that. FWIW:
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
-Sumit
> Thanks, > Jens
| |