Re: [PATCH v10 06/10] arm_ffa: introduce sandbox FF-A support

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

On Sun, Apr 02, 2023 at 02:41:01PM +1200, Simon Glass wrote:
> Hi Abdellatif,
> 
> On Wed, 29 Mar 2023 at 05:12, Abdellatif El Khlifi <
> abdellatif.elkhlifi@arm.com> wrote:
> >
> > Emulate Secure World's FF-A ABIs and allow testing U-Boot FF-A support
> >
> > Features of the sandbox FF-A support:
> >
> > - Introduce an FF-A emulator
> > - Introduce an FF-A device driver for FF-A comms with emulated Secure
> World
> > - Provides test methods allowing to read the status of the inspected ABIs
> >
> > The sandbox FF-A emulator supports only 64-bit direct messaging.
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >
> > ---
> > Changelog:
> > ===============
> >
> > v10:
> >
> > * split the FF-A sandbox support into an emulator and a driver
> > * read FFA_VERSION and FFA_PARTITION_INFO_GET state using
> >    sandbox_ffa_query_core_state()
> > * drop CONFIG_SANDBOX_FFA config
> > * address nits
> >
> > v9: align FF-A sandbox driver with FF-A discovery through DM
> >
> > v8: update ffa_bus_prvdata_get() to return a pointer rather than
> >     a pointer address
> >
> > v7: state that sandbox driver supports only 64-bit direct messaging
> >
> > v4: align sandbox driver with the new FF-A driver interfaces
> >     and new way of error handling
> >
> > v1: introduce the sandbox driver
> >
> >  MAINTAINERS                                   |   3 +-
> >  arch/sandbox/dts/sandbox.dtsi                 |   8 +
> >  arch/sandbox/dts/test.dts                     |   8 +
> >  arch/sandbox/include/asm/sandbox_arm_ffa.h    |  68 ++
> >  .../include/asm/sandbox_arm_ffa_priv.h        | 133 ++++
> >  configs/sandbox64_defconfig                   |   1 +
> >  configs/sandbox_defconfig                     |   1 +
> >  doc/arch/arm64.ffa.rst                        |   7 +-
> >  doc/arch/sandbox/sandbox.rst                  |   1 +
> >  drivers/firmware/arm-ffa/Kconfig              |  13 +-
> >  drivers/firmware/arm-ffa/Makefile             |  10 +-
> >  drivers/firmware/arm-ffa/ffa-emul-uclass.c    | 732 ++++++++++++++++++
> >  .../firmware/arm-ffa/sandbox_arm_ffa_priv.h   |  14 -
> >  drivers/firmware/arm-ffa/sandbox_ffa.c        | 108 +++
> >  include/dm/uclass-id.h                        |   1 +
> >  15 files changed, 1087 insertions(+), 21 deletions(-)
> >  create mode 100644 arch/sandbox/include/asm/sandbox_arm_ffa.h
> >  create mode 100644 arch/sandbox/include/asm/sandbox_arm_ffa_priv.h
> >  create mode 100644 drivers/firmware/arm-ffa/ffa-emul-uclass.c
> >  delete mode 100644 drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> >  create mode 100644 drivers/firmware/arm-ffa/sandbox_ffa.c
> >
> 
> [..]
> 
> > +/**
> > + * sandbox_ffa_get_fwk_version() - Return the FFA framework version
> > + * @dev: The sandbox FF-A emulator device
> > + * @func_data:  Pointer to the FF-A function arguments container
> structure
> > + *
> > + * Return the FFA framework version read from the FF-A emulator data.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +static int sandbox_ffa_get_fwk_version(struct udevice *dev, struct
> ffa_sandbox_data *func_data)
> > +{
> > +       struct sandbox_ffa_emul *priv = dev_get_priv(dev);
> > +
> > +       if (!func_data)
> > +               return -EINVAL;
> > +
> > +       if (!func_data->data0 ||
> > +           func_data->data0_size != sizeof(priv->fwk_version))
> > +               return -EINVAL;
> > +
> > +       *((u32 *)func_data->data0) = priv->fwk_version;
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * sandbox_ffa_get_parts() - Return the address of partitions data
> > + * @dev: The sandbox FF-A emulator device
> > + * @func_data:  Pointer to the FF-A function arguments container
> structure
> > + *
> > + * Return the address of partitions data read from the FF-A emulator
> data.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +static int sandbox_ffa_get_parts(struct udevice *dev, struct
> ffa_sandbox_data *func_data)
> > +{
> > +       struct sandbox_ffa_emul *priv = dev_get_priv(dev);
> > +
> > +       if (!func_data)
> > +               return -EINVAL;
> > +
> > +       if (!func_data->data0 ||
> > +           func_data->data0_size != sizeof(struct ffa_partitions *))
> > +               return -EINVAL;
> > +
> > +       *((struct ffa_partitions **)func_data->data0) = &priv->partitions;
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * sandbox_ffa_query_core_state() - Inspect the FF-A ABIs
> > + * @queried_func_id:   The FF-A function to be queried
> > + * @func_data:  Pointer to the FF-A function arguments container
> structure
> > + *
> > + * Queries the status of FF-A ABI specified in the input argument.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +int sandbox_ffa_query_core_state(u32 queried_func_id,
> > +                                struct ffa_sandbox_data *func_data)
> > +{
> > +       struct udevice *dev;
> 
> Please can you use 'emul' for the emulator so it is separate from the
> device?
> 
> > +
> > +       uclass_first_device(UCLASS_FFA_EMUL, &dev);
> > +       if (!dev) {
> > +               log_err("[FFA][SANDBOX][Emul] Cannot find FF-A emulator
> during querying state\n");
> > +               return -ENODEV;
> > +       }
> 
> How about storing the emulator somewhere? You could have:
> 
> struct sandbox_ffa_priv {
>    struct udevice *emul;
> };
> 
> .priv = sizeof(sandbox_ffa_priv)
>

in patchset v11 the sandbox emulator pointer is stored in the FF-A device uc_priv (struct ffa_priv)

Cheers,
Abdellatif

> > +
> > +       switch (queried_func_id) {
> > +       case FFA_RXTX_MAP:
> > +       case FFA_RXTX_UNMAP:
> > +       case FFA_RX_RELEASE:
> > +               return sandbox_ffa_get_rxbuf_flags(dev, queried_func_id,
> func_data);
> > +       case FFA_VERSION:
> > +               return sandbox_ffa_get_fwk_version(dev, func_data);
> > +       case FFA_PARTITION_INFO_GET:
> > +               return sandbox_ffa_get_parts(dev, func_data);
> > +       default:
> > +               log_err("[FFA][Sandbox][Emul] Undefined FF-A interface
> (%d)\n",
> > +                       queried_func_id);
> > +               return -EINVAL;
> > +       }
> > +}
> > +
> > +/**
> > + * sandbox_arm_ffa_smccc_smc() - FF-A SMC call emulation
> > + * @args:      the SMC call arguments
> > + * @res:       the SMC call returned data
> > + *
> > + * Emulates the FF-A ABIs SMC call.
> > + * The emulated FF-A ABI is identified and invoked.
> > + * FF-A emulation is based on the FF-A specification 1.0
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure.
> > + * FF-A protocol error codes are returned using the registers arguments
> as
> > + * described by the specification
> > + */
> > +void sandbox_arm_ffa_smccc_smc(ffa_value_t args, ffa_value_t *res)
> > +{
> > +       int ret = 0;
> > +       struct udevice *dev;
> > +
> > +       uclass_first_device(UCLASS_FFA_EMUL, &dev);
> > +       if (!dev) {
> > +               log_err("[FFA][SANDBOX][Emul] Cannot find FF-A emulator
> during SMC emulation\n");
> > +               return;
> > +       }
> > +
> > +       switch (args.a0) {
> > +       case FFA_SMC_32(FFA_VERSION):
> > +               ret = sandbox_ffa_version(dev, &args, res);
> > +               break;
> > +       case FFA_SMC_32(FFA_PARTITION_INFO_GET):
> > +               ret = sandbox_ffa_partition_info_get(dev, &args, res);
> > +               break;
> > +       case FFA_SMC_32(FFA_RXTX_UNMAP):
> > +               ret = sandbox_ffa_rxtx_unmap(dev, &args, res);
> > +               break;
> > +       case FFA_SMC_64(FFA_MSG_SEND_DIRECT_REQ):
> > +               ret = sandbox_ffa_msg_send_direct_req(dev, &args, res);
> > +               break;
> > +       case FFA_SMC_32(FFA_ID_GET):
> > +               ret = sandbox_ffa_id_get(dev, &args, res);
> > +               break;
> > +       case FFA_SMC_32(FFA_FEATURES):
> > +               ret = sandbox_ffa_features(&args, res);
> > +               break;
> > +       case FFA_SMC_64(FFA_RXTX_MAP):
> > +               ret = sandbox_ffa_rxtx_map(dev, &args, res);
> > +               break;
> > +       case FFA_SMC_32(FFA_RX_RELEASE):
> > +               ret = sandbox_ffa_rx_release(dev, &args, res);
> > +               break;
> > +       default:
> > +               log_err("[FFA][Sandbox][Emul] Undefined FF-A interface
> (0x%lx)\n",
> > +                       args.a0);
> > +       }
> > +
> > +       if (ret != 0)
> > +               log_err("[FFA][Sandbox][Emul] FF-A ABI internal failure
> (%d)\n", ret);
> > +}
> > +
> > +/**
> > + * ffa_emul_find() - Finds the FF-A emulator
> > + * @dev:       the sandbox FF-A device (sandbox-arm-ffa)
> > + * @emulp:     the FF-A emulator device (sandbox-ffa-emul)
> > + *
> > + * Searches for the FF-A emulator and returns its device pointer.
> > + *
> > + * Return:
> > + * 0 on success. Otherwise, failure
> > + */
> > +int ffa_emul_find(struct udevice *dev, struct udevice **emulp)
> > +{
> > +       if (!emulp)
> > +               return -EINVAL;
> > +
> > +       *emulp = NULL;
> > +
> > +       uclass_first_device(UCLASS_FFA_EMUL, emulp);
> > +       if (!(*emulp)) {
> > +               log_err("[FFA][SANDBOX][Emul] Cannot find FF-A
> emulator\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       log_info("[FFA][Sandbox][Emul] FF-A emulator found\n");
> > +
> > +       return 0;
> > +}
> > +
> > +UCLASS_DRIVER(ffa_emul) = {
> > +       .name           = "ffa_emul",
> > +       .id             = UCLASS_FFA_EMUL,
> > +};
> > +
> > +/* Sandbox Arm FF-A emulator operations */
> > +
> > +static const struct ffa_emul_ops sandbox_ffa_emul_ops = {
> > +       .invoke_ffa_fn = sandbox_arm_ffa_smccc_smc,
> > +};
> > +
> > +static const struct udevice_id sandbox_ffa_emul_ids[] = {
> > +       { .compatible = "sandbox,arm-ffa-emul" },
> > +       { }
> > +};
> > +
> > +/* Declaring the sandbox FF-A emulator under UCLASS_FFA_EMUL */
> > +U_BOOT_DRIVER(sandbox_ffa_emul) = {
> > +       .name           = "sandbox_ffa_emul",
> > +       .id             = UCLASS_FFA_EMUL,
> > +       .of_match       = sandbox_ffa_emul_ids,
> > +       .ops            = &sandbox_ffa_emul_ops,
> > +       .priv_auto      = sizeof(struct sandbox_ffa_emul),
> > +};
> > diff --git a/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> b/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> > deleted file mode 100644
> > index 4338f9c9b1..0000000000
> > --- a/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> > +++ /dev/null
> > @@ -1,14 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0+ */
> > -/*
> > - * Copyright 2022-2023 Arm Limited and/or its affiliates <
> open-source-office@arm.com>
> > - *
> > - * Authors:
> > - *   Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > - */
> > -
> > -#ifndef __SANDBOX_ARM_FFA_PRV_H
> > -#define __SANDBOX_ARM_FFA_PRV_H
> > -
> > -/* Future sandbox support private declarations */
> > -
> > -#endif
> > diff --git a/drivers/firmware/arm-ffa/sandbox_ffa.c
> b/drivers/firmware/arm-ffa/sandbox_ffa.c
> > new file mode 100644
> > index 0000000000..bb150fd5cd
> > --- /dev/null
> > +++ b/drivers/firmware/arm-ffa/sandbox_ffa.c
> > @@ -0,0 +1,108 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2022-2023 Arm Limited and/or its affiliates <
> open-source-office@arm.com>
> > + *
> > + * Authors:
> > + *   Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > + */
> > +#include <common.h>
> > +#include <arm_ffa.h>
> > +#include <dm.h>
> > +#include <log.h>
> > +#include <asm/global_data.h>
> > +#include <asm/sandbox_arm_ffa_priv.h>
> > +#include <dm/device-internal.h>
> > +#include <linux/errno.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/**
> > + * ffa_set_smc_conduit() - Set the SMC conduit
> > + *
> > + * Selects the SMC conduit by setting the FF-A ABI invoke function.
> > + * The function emulating the SMC call is provided by the FF-A emulator.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +int ffa_set_smc_conduit(void)
> > +{
> > +       struct udevice *emul;
> > +       int ret;
> > +
> > +       ret = ffa_emul_find(NULL, &emul);
> > +       if (ret) {
> > +               log_err("[FFA][Sandbox] Cannot find FF-A emulator, SMC
> emulation failure\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (!ffa_emul_get_ops(emul)->invoke_ffa_fn) {
> > +               log_err("[FFA][Sandbox] Cannot get FF-A emulator ops, SMC
> emulation failure\n");
> > +               return -ENOSYS;
> > +       }
> > +
> > +       dscvry_info.invoke_ffa_fn = ffa_emul_get_ops(emul)->invoke_ffa_fn;
> > +       log_info("[FFA][Sandbox] Using emulated Arm SMC for FF-A
> conduit\n");
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * sandbox_ffa_probe() - The sandbox FF-A driver probe function
> > + * @dev:       the sandbox Arm FF-A bus device (sandbox-arm-ffa)
> > + *
> > + * Probing is done through ffa_do_probe()
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +static int sandbox_ffa_probe(struct udevice *dev)
> > +{
> > +       return ffa_do_probe(dev);
> 
> Move to a uclass pre_probe()  method.
> 
> > +}
> 
> Drop this. You can add it in the uclass as a child_pre_probe member.
> 
> > +
> > +/**
> > + * sandbox_ffa_bind() - The sandbox FF-A driver bind function
> > + * @dev:       the sandbox-arm-ffa device
> > + * Tries to discover the emulated FF-A bus.
> > + * Return:
> > + *
> > + * 0 on success.
> > + */
> > +static int sandbox_ffa_bind(struct udevice *dev)
> > +{
> > +       bool ret;
> > +
> > +       log_info("[FFA][Sandbox] binding the device\n");
> > +
> > +       ret = ffa_try_discovery();
> 
> The code for this is immediately above. Just do that here, or make the
> above function static and rename it.
> 
> > +       if (ret)
> > +               return 0;
> > +       else
> > +               return -ENODEV;
> 
> if (ret)
>     return ret;
> 
> return 0;
> 
> > +}
> > +
> > +/* Sandbox Arm FF-A emulator operations */
> > +
> > +static const struct ffa_bus_ops sandbox_ffa_ops = {
> > +       .partition_info_get = ffa_get_partitions_info_hdlr,
> > +       .sync_send_receive = ffa_msg_send_direct_req_hdlr,
> > +       .rxtx_unmap = ffa_unmap_rxtx_buffers_hdlr,
> > +};
> > +
> > +static const struct udevice_id sandbox_ffa_id[] = {
> > +       { "sandbox,arm-ffa", 0 },
> > +       { },
> > +};
> > +
> > +/* Declaring the sandbox FF-A driver under UCLASS_FFA */
> > +U_BOOT_DRIVER(sandbox_arm_ffa) = {
> > +       .name           = "sandbox_arm_ffa",
> > +       .of_match = sandbox_ffa_id,
> > +       .id             = UCLASS_FFA,
> > +       .probe          = sandbox_ffa_probe,
> > +       .bind           = sandbox_ffa_bind,
> > +       .ops            = &sandbox_ffa_ops,
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index df77c7da58..4658411935 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -61,6 +61,7 @@ enum uclass_id {
> >         UCLASS_ETH,             /* Ethernet device */
> >         UCLASS_ETH_PHY,         /* Ethernet PHY device */
> >         UCLASS_FFA,             /* Arm Firmware Framework for Armv8-A */
> > +       UCLASS_FFA_EMUL,                /* sandbox FF-A device emulator */
> >         UCLASS_FIRMWARE,        /* Firmware */
> >         UCLASS_FPGA,            /* FPGA device */
> >         UCLASS_FUZZING_ENGINE,  /* Fuzzing engine */
> > --
> > 2.25.1
> >
> 
> Regards,
> Simon