Re: [PATCH v3 3/5] optee: refactor driver with internal callbacks

[Jens Wiklander <jens.wiklander@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 21603 bytes --]

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(&param.a1, &param.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, &param, &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.

> 
> >   * @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.

Thanks,
Jens