Re: [PATCH v4 5/5] optee: add FF-A support

[Jens Wiklander]

On Wed, Aug 25, 2021 at 05:12:45PM +0530, Sumit Garg wrote:
> On Thu, 19 Aug 2021 at 16:37, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Adds support for using FF-A [1] as transport to the OP-TEE driver.
> >
> > Introduces struct optee_msg_param_fmem which carries all information
> > needed when OP-TEE is calling FFA_MEM_RETRIEVE_REQ to get the shared
> > memory reference mapped by the hypervisor in S-EL2. Register usage is
> > also updated to include the information needed.
> >
> > The FF-A part of this driver is enabled if CONFIG_ARM_FFA_TRANSPORT is
> > enabled.
> >
> > [1] https://developer.arm.com/documentation/den0077/latest
> > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> > ---
> >  drivers/tee/optee/Makefile        |   3 +-
> >  drivers/tee/optee/call.c          |  13 +-
> >  drivers/tee/optee/core.c          |  16 +-
> >  drivers/tee/optee/ffa_abi.c       | 907 ++++++++++++++++++++++++++++++
> >  drivers/tee/optee/optee_ffa.h     | 153 +++++
> >  drivers/tee/optee/optee_msg.h     |  27 +-
> >  drivers/tee/optee/optee_private.h |  43 +-
> >  7 files changed, 1148 insertions(+), 14 deletions(-)
> >  create mode 100644 drivers/tee/optee/ffa_abi.c
> >  create mode 100644 drivers/tee/optee/optee_ffa.h
> >
[snip]
> > --- /dev/null
> > +++ b/drivers/tee/optee/ffa_abi.c
> > @@ -0,0 +1,907 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/arm_ffa.h>
> > +#include <linux/errno.h>
> > +#include <linux/scatterlist.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/tee_drv.h>
> > +#include <linux/types.h>
> > +#include "optee_private.h"
> > +#include "optee_ffa.h"
> > +#include "optee_rpc_cmd.h"
> > +
> > +/*
> > + * This file implement the FF-A ABI used when communicating with secure world
> > + * OP-TEE OS via FF-A.
> > + * This file is divided into the follow sections:
> 
> s/follow/following/

Thanks, I'll fix.

[snip]
> > +static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > +                                   const struct ffa_dev_ops *ops,
> > +                                   unsigned int *rpc_arg_count)
> > +{
> > +       struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > +       int rc;
> > +
> > +       rc = ops->sync_send_receive(ffa_dev, &data);
> > +       if (rc) {
> > +               pr_err("Unexpected error %d", rc);
> > +               return false;
> > +       }
> > +       if (data.data0) {
> > +               pr_err("Unexpected exchange error %lu", data.data0);
> > +               return false;
> > +       }
> > +
> > +       *rpc_arg_count = (u8)data.data1;
> 
> Why is this special capability required in case of FF-A? Is it true
> that RPC arguments count will be fixed for all RPC commands?

It's to allow this driver to preallocate the argument struct used when
doing RPC. That way we can avoid the chicken and egg problem of allocating
an RPC argumet struct just before doing the real RPC.

This is the maximum number of arguments needed by secure world. In case
a larger value ever is needed, secure world will be able to supply the
needed value.

I plan to update the SMC based ABI with this also, but not in the patch
set.

> 
> > +
> > +       return true;
> > +}

[snip]
> > +static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > +{
> > +       const struct ffa_dev_ops *ffa_ops;
> > +       unsigned int rpc_arg_count;
> > +       struct tee_device *teedev;
> > +       struct optee *optee;
> > +       int rc;
> > +
> > +       ffa_ops = ffa_dev_ops_get(ffa_dev);
> > +       if (!ffa_ops) {
> > +               pr_warn("failed \"method\" init: ffa\n");
> > +               return -ENOENT;
> > +       }
> > +
> > +       if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> > +               return -EINVAL;
> > +
> > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_arg_count))
> > +               return -EINVAL;
> => +
> > +       optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > +       if (!optee) {
> > +               rc = -ENOMEM;
> > +               goto err;
> > +       }
> > +       optee->pool = optee_ffa_config_dyn_shm();
> > +       if (IS_ERR(optee->pool)) {
> > +               rc = PTR_ERR(optee->pool);
> > +               optee->pool = NULL;
> > +               goto err;
> > +       }
> 
> IIUC, with FF-A we will only be supporting dynamic shared memory. So
> CFG_CORE_DYN_SHM=y should be enforced in OP-TEE OS when
> CFG_CORE_FFA=y, but I don't see that currently. Am I missing
> something?

You mean in optee_os.git? With FF-A dynamic shared memory is always
handled, so that flag in irrelevant in that case. However, feel free to
start a discussion on that topic at github.

Thanks,
Jens