Messages in this thread | | | From | Sumit Garg <> | Date | Fri, 27 Aug 2021 10:57:00 +0530 | Subject | Re: [PATCH v4 5/5] optee: add FF-A support |
| |
On Thu, 26 Aug 2021 at 20:22, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > 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@linaro.org> 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@linaro.org> > > > --- > > > 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. >
Okay, I see the requirement with FF-A ABI that we need to pass memory reference to "struct thread_rpc_arg" while in case of SMC ABI we directly pass the RPC commands in registers which allows it to work without pre-allocation.
So I am fine with this approach as well given that the pre-allocated memory for RPC arguments may be left unused in some cases but that's fine when compared with the overhead of extra RPC calls with SMC ABI.
> > > > > + > > > + 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.
Ah, I see. You are correct that FF-A enables dynamic shared memory by default.
FWIW, feel free to add:
Acked-by: Sumit Garg <sumit.garg@linaro.org>
-Sumit
> > Thanks, > Jens
| |