Re: [PATCH v9 06/10] arm_ffa: introduce the FF-A Sandbox driver

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

Hi Simon,

> Hi Abdellatif,
> 
> On Fri, 10 Mar 2023 at 06:10, Abdellatif El Khlifi
> <abdellatif.elkhlifi@arm.com> wrote:
> >
> > Provide a Sandbox driver to emulate the FF-A ABIs
> >
> > The emulated ABIs are those supported by the FF-A core driver
> > and according to FF-A specification v1.0.
> >
> > The Sandbox driver provides operations allowing the test
> > application to read the status of all the inspected ABIs
> > and perform functional tests based on that.
> >
> > sandbox driver 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>
> >
> > ---
> > Changelog:
> > ===============
> >
> > 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                                   |   1 +
> >  arch/sandbox/dts/sandbox.dtsi                 |   4 +
> >  arch/sandbox/dts/test.dts                     |   4 +
> >  configs/sandbox64_defconfig                   |   2 +
> >  configs/sandbox_defconfig                     |   2 +
> >  doc/arch/arm64.ffa.rst                        |   4 +
> >  doc/arch/sandbox/sandbox.rst                  |   1 +
> >  drivers/firmware/arm-ffa/Kconfig              |  11 +-
> >  drivers/firmware/arm-ffa/Makefile             |   1 +
> >  drivers/firmware/arm-ffa/core.c               |  36 +-
> >  drivers/firmware/arm-ffa/sandbox.c            | 610 ++++++++++++++++++
> >  .../firmware/arm-ffa/sandbox_arm_ffa_priv.h   | 129 ++++
> >  include/arm_ffa.h                             |   5 +-
> >  include/sandbox_arm_ffa.h                     | 124 ++++
> >  14 files changed, 928 insertions(+), 6 deletions(-)
> >  create mode 100644 drivers/firmware/arm-ffa/sandbox.c
> >  create mode 100644 drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> >  create mode 100644 include/sandbox_arm_ffa.h
> >
> 
> Could you use 80 columns where possible? There seem to be a lot of
> things that extend beyond that without much of a reason.
> 
> Also you can use 'ulong' instead of 'unsigned long'. It is less
> verbose and a U-Boot standard.
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 18e9c2ce99..2b9d33e964 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -274,6 +274,7 @@ F:  doc/arch/arm64.ffa.rst
> >  F:     doc/usage/cmd/armffa.rst
> >  F:     drivers/firmware/arm-ffa/
> >  F:     include/arm_ffa.h
> > +F:     include/sandbox_arm_ffa.h
> >
> >  ARM FREESCALE IMX
> >  M:     Stefano Babic <sbabic@denx.de>
> > diff --git a/arch/sandbox/dts/sandbox.dtsi b/arch/sandbox/dts/sandbox.dtsi
> > index 30a305c4d2..059c273277 100644
> > --- a/arch/sandbox/dts/sandbox.dtsi
> > +++ b/arch/sandbox/dts/sandbox.dtsi
> > @@ -445,6 +445,10 @@
> >         thermal {
> >                 compatible = "sandbox,thermal";
> >         };
> > +
> > +       sandbox_arm_ffa {
> > +               compatible = "sandbox,arm_ffa";
> > +       };
> >  };
> >
> >  &cros_ec {
> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> > index d72d7a567a..11dc6ed0d9 100644
> > --- a/arch/sandbox/dts/test.dts
> > +++ b/arch/sandbox/dts/test.dts
> > @@ -1802,6 +1802,10 @@
> >                 compatible = "u-boot,fwu-mdata-gpt";
> >                 fwu-mdata-store = <&mmc0>;
> >         };
> > +
> > +       sandbox_arm_ffa {
> > +               compatible = "sandbox,arm_ffa";
> > +       };
> 
> I see that you have this, so the driver should bind automatically.
> 
> Is the problem that you are trying to bind the sandbox emulator?
> Again, you can actually add that to the DT. See how this works for
> i2c, SPI, PCI, for example.
> 
> >  };
> >
> >  #include "sandbox_pmic.dtsi"
> > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > index ccbc18aad0..35d4676cf7 100644
> > --- a/configs/sandbox64_defconfig
> > +++ b/configs/sandbox64_defconfig
> > @@ -259,3 +259,5 @@ CONFIG_FWU_MULTI_BANK_UPDATE=y
> >  CONFIG_UNIT_TEST=y
> >  CONFIG_UT_TIME=y
> >  CONFIG_UT_DM=y
> > +CONFIG_ARM_FFA_TRANSPORT=y
> > +CONFIG_SANDBOX_FFA=y
> > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > index a0fbdad20a..8aab8dda31 100644
> > --- a/configs/sandbox_defconfig
> > +++ b/configs/sandbox_defconfig
> > @@ -336,3 +336,5 @@ CONFIG_TEST_FDTDEC=y
> >  CONFIG_UNIT_TEST=y
> >  CONFIG_UT_TIME=y
> >  CONFIG_UT_DM=y
> > +CONFIG_ARM_FFA_TRANSPORT=y
> > +CONFIG_SANDBOX_FFA=y
> > \ No newline at end of file
> > diff --git a/doc/arch/arm64.ffa.rst b/doc/arch/arm64.ffa.rst
> > index 8fad9ef3d0..555ee9a6ae 100644
> > --- a/doc/arch/arm64.ffa.rst
> > +++ b/doc/arch/arm64.ffa.rst
> > @@ -64,6 +64,10 @@ CONFIG_ARM_FFA_TRANSPORT
> >      Enables the FF-A bus driver. Turn this on if you want to use FF-A
> >      communication.
> >
> > +CONFIG_SANDBOX_FFA
> > +    Enables FF-A Sandbox driver. This emulates the FF-A ABIs handling under
> > +    Sandbox and provides functional tests for FF-A.
> 
> OK that is why I am confused. Please don't call this a driver. It is
> an emulator. When you have an emulator and a driver for the same thing
> it gets confusing.
> 
> > +
> >  FF-A ABIs under the hood
> >  ---------------------------------------
> >
> > diff --git a/doc/arch/sandbox/sandbox.rst b/doc/arch/sandbox/sandbox.rst
> > index cd7f8a2cb0..c5df372e00 100644
> > --- a/doc/arch/sandbox/sandbox.rst
> > +++ b/doc/arch/sandbox/sandbox.rst
> > @@ -200,6 +200,7 @@ Supported Drivers
> >
> >  U-Boot sandbox supports these emulations:
> >
> > +- Arm FF-A
> >  - Block devices
> >  - Chrome OS EC
> >  - GPIO
> > diff --git a/drivers/firmware/arm-ffa/Kconfig b/drivers/firmware/arm-ffa/Kconfig
> > index 2cfd7ef5fc..b5430eb6f4 100644
> > --- a/drivers/firmware/arm-ffa/Kconfig
> > +++ b/drivers/firmware/arm-ffa/Kconfig
> > @@ -2,9 +2,9 @@
> >
> >  config ARM_FFA_TRANSPORT
> >         bool "Enable Arm Firmware Framework for Armv8-A driver"
> > -       depends on DM && ARM64
> > -       select ARM_SMCCC
> > -       select ARM_SMCCC_FEATURES
> > +       depends on DM && (ARM64 || SANDBOX)
> > +       select ARM_SMCCC if !SANDBOX
> > +       select ARM_SMCCC_FEATURES if !SANDBOX
> >         imply CMD_ARMFFA
> >         select LIB_UUID
> >         select DEVRES
> > @@ -32,3 +32,8 @@ config ARM_FFA_TRANSPORT
> >
> >           For more details about the FF-A driver, please refer to doc/arch/arm64.ffa.rst
> >
> > +config SANDBOX_FFA
> > +       bool "FF-A Sandbox driver"
> > +       depends on ARM_FFA_TRANSPORT && SANDBOX
> > +       help
> > +         This emulates the FF-A handling under Sandbox and allows to test the FF-A driver
> > diff --git a/drivers/firmware/arm-ffa/Makefile b/drivers/firmware/arm-ffa/Makefile
> > index c8d83b4bc9..d22c1ba609 100644
> > --- a/drivers/firmware/arm-ffa/Makefile
> > +++ b/drivers/firmware/arm-ffa/Makefile
> > @@ -6,3 +6,4 @@
> >  #   Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> >
> >  obj-y += arm-ffa-uclass.o core.o
> > +obj-$(CONFIG_SANDBOX_FFA) += sandbox.o
> > diff --git a/drivers/firmware/arm-ffa/core.c b/drivers/firmware/arm-ffa/core.c
> > index 2210f5343c..0d2e6ff0d4 100644
> > --- a/drivers/firmware/arm-ffa/core.c
> > +++ b/drivers/firmware/arm-ffa/core.c
> > @@ -1042,6 +1042,7 @@ bool ffa_try_discovery(void)
> >         return true;
> >  }
> >
> > +#if !CONFIG_IS_ENABLED(SANDBOX_FFA)
> 
> We should not need #ifdefs here. The sandbox driver is just another
> driver, just like there is the ARM driver. We select the correct one
> using the devicetree and everything just works.

The FF-A core driver (core.c) has Arm specific code that is not available
when running sandbox. So we still need to use these ifdefs in core.c
to prevent the Arm specific code from building under sandbox.
For example these are Arm specific:

arm_smccc_1_2_smc()
struct arm_smccc_res 
ARM_SMCCC_FEATURE_DRIVER
The SMC conduit. In sandbox mode the core driver should use the emulated conduit implemented by sandbox_arm_ffa_smccc_smc()

> 
> >  /**
> >   * __arm_ffa_fn_smc() - SMC wrapper
> >   * @args: FF-A ABI arguments to be copied to Xn registers
> > @@ -1069,6 +1070,7 @@ void __arm_ffa_fn_smc(ffa_value_t args, ffa_value_t *res)
> >   * The FF-A driver supports the SMCCCv1.2 extended input/output registers.
> >   * So, the legacy SMC invocation is not used.
> >   *
> > + * In Sandbox mode sandbox_arm_ffa is used to test arm_ffa driver.
> >   * Return:
> >   *
> >   * 0 on success. Otherwise, failure
> > @@ -1088,6 +1090,30 @@ ARM_SMCCC_FEATURE_DRIVER(arm_ffa) = {
> >         .driver_name = FFA_DRV_NAME,
> >         .is_supported = ffa_bus_is_supported,
> >  };
> > +#else
> > +/* SANDBOX_FFA */
> > +
> > +/**
> > + * ffa_bind() - The driver bind function
> > + * @dev:       the arm_ffa device
> > + * When using sandbox tries to discover the emulated FF-A bus.
> > + * Return:
> > + *
> > + * 0 on success.
> > + */
> > +static int ffa_bind(struct udevice *dev)
> 
> I don't think this is binding anything. How about ffa_get_dev() ?

In sandbox mode, when the FF-A sandbox driver (emulates the secure world) is probed,  it calls
device_bind_driver() to bind the FF-A core driver (a similar approach to the PSCI way).
ffa_bind() is needed in sandbox mode so the FF-A discovery is setup properly.

In an Arm platform, the PSCI driver calls the FF-A discovery callback
ffa_bus_is_supported() which tries to discover FF-A. When discovery is successful,
PSCI driver binds the FF-A core device by calling device_bind_driver()

The core driver needs to work on both Arm and sandbox.
The sandbox FF-A driver plays the role of a secure world emulator with which
the FF-A core driver exchanges data.

> 
> > +{
> > +       bool ret;
> > +
> > +       log_info("[FFA] binding the device\n");
> > +
> > +       ret = ffa_try_discovery();
> > +       if (ret)
> > +               return 0;
> > +       else
> > +               return -ENODEV;
> > +}
> > +#endif
> >
> >  /**
> >   * ffa_set_smc_conduit() - Set the SMC conduit
> > @@ -1101,7 +1127,12 @@ ARM_SMCCC_FEATURE_DRIVER(arm_ffa) = {
> >   */
> >  static int ffa_set_smc_conduit(void)
> >  {
> > -       dscvry_info.invoke_ffa_fn = __arm_ffa_fn_smc;
> > +#if CONFIG_IS_ENABLED(SANDBOX_FFA)
> > +               dscvry_info.invoke_ffa_fn = sandbox_arm_ffa_smccc_smc;
> > +               log_info("[FFA] Using SMC emulation\n");
> > +#else
> > +               dscvry_info.invoke_ffa_fn = __arm_ffa_fn_smc;
> > +#endif
> 
> This needs to be reworked to go through the uclass to the correct
> driver. Basically you need an ARM driver and a sandbox driver (which
> attaches to the emulator).
> 
> There should not be #idefs in this sort of code...driver model should
> handle it. See other uclasses for examples.

In sandbox we are testing the FF-A core driver. The core driver has some Arm specific code which
explains why we need the ifdefs. In Arm the SMC conduit is implemented by __arm_ffa_fn_smc()
which ends up calling Arm instructions.
When building the core driver with sandbox, the SMC conduit is set to the emulated version of the SMC calls
implemented by sandbox_arm_ffa_smccc_smc()

I'm happy to rename the FF-A sandbox driver to the FF-A sandbox emulator if that clears the doubts.
However, the emulator is still a driver and bound to a sandbox device (the parent of the core).

Cheers,
Abdellatif

> 
> >
> >         log_info("[FFA] Conduit is SMC\n");
> >
> > @@ -1246,4 +1277,7 @@ U_BOOT_DRIVER(arm_ffa) = {
> >         .remove = ffa_remove,
> >         .unbind = ffa_unbind,
> >         .ops            = &ffa_ops,
> > +#if CONFIG_IS_ENABLED(SANDBOX_FFA)
> > +       .bind           = ffa_bind,
> 
> Can drop this
> 
> > +#endif
> >  };
> > diff --git a/drivers/firmware/arm-ffa/sandbox.c b/drivers/firmware/arm-ffa/sandbox.c
> 
> sandbox_emul.c pehaps?
> 
> I am pretty sure this is an emulator
> 
> > new file mode 100644
> > index 0000000000..84c2fc929f
> > --- /dev/null
> > +++ b/drivers/firmware/arm-ffa/sandbox.c
> > @@ -0,0 +1,610 @@
> > +// 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 <dm.h>
> > +#include <mapmem.h>
> > +#include <string.h>
> > +#include <asm/global_data.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/root.h>
> > +#include <linux/errno.h>
> > +#include <linux/sizes.h>
> > +#include "sandbox_arm_ffa_priv.h"
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/* The partitions (SPs) table */
> > +static struct ffa_partition_desc sandbox_partitions[SANDBOX_PARTITIONS_CNT] = {
> > +       {
> > +               .info = { .id = SANDBOX_SP1_ID, .exec_ctxt = 0x5687, .properties = 0x89325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE1_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE1_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE1_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE1_UUID_A4,
> > +               }
> > +       },
> > +       {
> > +               .info = { .id = SANDBOX_SP2_ID, .exec_ctxt = 0x9587, .properties = 0x45325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE2_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE2_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE2_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE2_UUID_A4,
> > +               }
> > +       },
> > +       {
> > +               .info = { .id = SANDBOX_SP3_ID, .exec_ctxt = 0x7687, .properties = 0x23325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE1_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE1_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE1_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE1_UUID_A4,
> > +               }
> > +       },
> > +       {
> > +               .info = { .id = SANDBOX_SP4_ID, .exec_ctxt = 0x1487, .properties = 0x70325621 },
> > +               .sp_uuid = {
> > +                       .a1 = SANDBOX_SERVICE2_UUID_A1,
> > +                       .a2 = SANDBOX_SERVICE2_UUID_A2,
> > +                       .a3 = SANDBOX_SERVICE2_UUID_A3,
> > +                       .a4 = SANDBOX_SERVICE2_UUID_A4,
> > +               }
> > +       }
> > +
> > +};
> > +
> > +/* Driver functions */
> > +
> > +/**
> > + * sandbox_ffa_version() - Emulated FFA_VERSION handler function
> > + * @dev:       the sandbox FF-A device
> > + * @{a0-a7} , res: The SMC call arguments and return structure.
> > + *
> > + * This is the function that emulates FFA_VERSION FF-A function.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +SANDBOX_SMC_FFA_ABI(ffa_version)
> 
> Instead of the macro can you write this out in full? It defeats ctags, etc.
> 
> > +{
> > +       struct sandbox_ffa_priv *priv = dev_get_priv(dev);
> > +
> > +       priv->fwk_version = FFA_VERSION_1_0;
> > +       res->a0 = priv->fwk_version;
> 
> Where is res defined?
> 
> > +
> > +       /* x1-x7 MBZ */
> > +       memset(FFA_X1X7_MBZ_REG_START, 0, FFA_X1X7_MBZ_CNT * sizeof(unsigned long));
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * sandbox_ffa_id_get() - Emulated FFA_ID_GET handler function
> > + * @dev:       the sandbox FF-A device
> > + * @{a0-a7} , res: The SMC call arguments and return structure.
> > + *
> > + * This is the function that emulates FFA_ID_GET FF-A function.
> 
> s/This is the function that e/E/g
> 
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +SANDBOX_SMC_FFA_ABI(ffa_id_get)
> > +{
> > +       struct sandbox_ffa_priv *priv = dev_get_priv(dev);
> > +
> > +       res->a0 = FFA_SMC_32(FFA_SUCCESS);
> > +       res->a1 = 0;
> > +
> > +       priv->id = NS_PHYS_ENDPOINT_ID;
> > +       res->a2 = priv->id;
> > +
> > +       /* x3-x7 MBZ */
> > +       memset(FFA_X3_MBZ_REG_START, 0, FFA_X3X7_MBZ_CNT * sizeof(unsigned long));
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * sandbox_ffa_features() - Emulated FFA_FEATURES handler function
> > + * @dev:       the sandbox FF-A device
> > + * @{a0-a7} , res: The SMC call arguments and return structure.
> > + *
> > + * This is the function that emulates FFA_FEATURES FF-A function.
> > + *
> > + * Return:
> > + *
> > + * 0 on success. Otherwise, failure
> > + */
> > +SANDBOX_SMC_FFA_ABI(ffa_features)
> > +{
> > +       if (pargs->a1 == FFA_SMC_64(FFA_RXTX_MAP)) {
> > +               res->a0 = FFA_SMC_32(FFA_SUCCESS);
> > +               res->a2 = RXTX_BUFFERS_MIN_SIZE;
> > +               res->a3 = 0;
> > +               /* x4-x7 MBZ */
> > +               memset(FFA_X4X7_MBZ_REG_START,
> > +                      0, FFA_X4X7_MBZ_CNT * sizeof(unsigned long));
> > +       } else {
> > +               res->a0 = FFA_SMC_32(FFA_ERROR);
> > +               res->a2 = FFA_ERR_STAT_NOT_SUPPORTED;
> > +               /* x3-x7 MBZ */
> > +               memset(FFA_X3_MBZ_REG_START,
> > +                      0, FFA_X3X7_MBZ_CNT * sizeof(unsigned long));
> > +               log_err("[FFA] [Sandbox] FF-A interface 0x%lx not implemented\n", pargs->a1);
> 
> return -ENOSYS
> 
> > +       }
> > +
> > +       res->a1 = 0;
> > +
> > +       return 0;
> > +}
> > +
> 
> [..]
> 
> > +SANDBOX_SMC_FFA_ABI(ffa_msg_send_direct_req)
> > +{
> > +       u16 part_id;
> > +       struct sandbox_ffa_priv *priv = dev_get_priv(dev);
> > +
> > +       part_id = GET_DST_SP_ID(pargs->a1);
> > +
> > +       if ((GET_NS_PHYS_ENDPOINT_ID(pargs->a1) != priv->id) ||
> 
> drop extra brackets
> 
> > +           !sandbox_ffa_sp_valid(dev, part_id) || pargs->a2) {
> > +               res->a0 = FFA_SMC_32(FFA_ERROR);
> > +               res->a1 = 0;
> > +               res->a2 = FFA_ERR_STAT_INVALID_PARAMETERS;
> > +
> > +               /* x3-x7 MBZ */
> > +               memset(FFA_X3_MBZ_REG_START,
> > +                      0, FFA_X3X7_MBZ_CNT * sizeof(unsigned long));
> > +
> > +               return 0;
> > +       }
> > +
> > +       res->a0 = FFA_SMC_64(FFA_MSG_SEND_DIRECT_RESP);
> > +
> > +       res->a1 = PREP_SRC_SP_ID(part_id) |
> > +               PREP_NS_PHYS_ENDPOINT_ID(priv->id);
> > +
> > +       res->a2 = 0;
> > +
> > +       /* Return 0xff bytes as a response */
> > +       res->a3 = 0xffffffffffffffff;
> > +       res->a4 = 0xffffffffffffffff;
> > +       res->a5 = 0xffffffffffffffff;
> > +       res->a6 = 0xffffffffffffffff;
> > +       res->a7 = 0xffffffffffffffff;
> 
> Would -1UL work?
> 
> > +
> > +       return 0;
> > +}
> > +
> 
> [..]
> 
> > +static int sandbox_ffa_probe(struct udevice *dev)
> > +{
> > +       struct udevice *child_dev = NULL;
> > +       int ret;
> > +
> > +       ret = device_bind_driver(dev, FFA_DRV_NAME, FFA_DRV_NAME, &child_dev);
> 
> Is this binding the emulator? Add it to the DT instead.
> 
> If you put
> 
> .post_bind = dm_scan_fdt_dev()
> 
> in your uclass (like spi-uclass.c does) then it will binding
> child-node devices automatically. Then you can make your emulator a
> child node.
> 
> > +       if (ret) {
> > +               pr_err("%s was not bound: %d, aborting\n", FFA_DRV_NAME, ret);
> > +               return -ENODEV;
> > +       }
> > +
> > +       dev_set_parent_plat(child_dev, dev_get_plat(dev));
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id sandbox_ffa_id[] = {
> > +       { "sandbox,arm_ffa", 0 },
> > +       { },
> > +};
> > +
> > +/* Declaring the sandbox_arm_ffa driver under UCLASS_FFA */
> > +U_BOOT_DRIVER(sandbox_arm_ffa) = {
> > +       .name           = FFA_SANDBOX_DRV_NAME,
> > +       .of_match = sandbox_ffa_id,
> > +       .id             = UCLASS_FFA,
> > +       .probe          = sandbox_ffa_probe,
> > +       .priv_auto      = sizeof(struct sandbox_ffa_priv),
> > +};
> > diff --git a/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h b/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> > new file mode 100644
> > index 0000000000..c35d80de16
> > --- /dev/null
> > +++ b/drivers/firmware/arm-ffa/sandbox_arm_ffa_priv.h
> > @@ -0,0 +1,129 @@
> > +/* 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
> > +
> > +#include <sandbox_arm_ffa.h>
> > +#include "arm_ffa_priv.h"
> > +
> > +/* This header is exclusively used by the Sandbox FF-A driver and sandbox tests */
> > +
> > +/* FF-A core driver name */
> > +#define FFA_SANDBOX_DRV_NAME "sandbox_arm_ffa"
> > +
> > +/* FF-A ABIs internal error codes (as defined by the spec) */
> > +
> > +#define FFA_ERR_STAT_NOT_SUPPORTED     -1
> > +#define FFA_ERR_STAT_INVALID_PARAMETERS        -2
> > +#define FFA_ERR_STAT_NO_MEMORY -3
> > +#define FFA_ERR_STAT_BUSY      -4
> > +#define FFA_ERR_STAT_DENIED    -6
> 
> It's a convention to put negative values in brackets just in case of
> operator-precedence issues. Or you could use an enum.
> 
> > +
> > +/* Non-secure physical FF-A instance */
> > +#define NS_PHYS_ENDPOINT_ID (0)
> > +
> > +#define GET_NS_PHYS_ENDPOINT_ID_MASK           GENMASK(31, 16)
> > +#define GET_NS_PHYS_ENDPOINT_ID(x)             \
> > +                       ((u16)(FIELD_GET(GET_NS_PHYS_ENDPOINT_ID_MASK, (x))))
> > +
> > +/* Helper macro for reading the destination partition ID */
> > +#define GET_DST_SP_ID_MASK             GENMASK(15, 0)
> > +#define GET_DST_SP_ID(x)               \
> > +                       ((u16)(FIELD_GET(GET_DST_SP_ID_MASK, (x))))
> > +
> > +/* Helper macro for setting the source partition ID */
> > +#define PREP_SRC_SP_ID_MASK            GENMASK(31, 16)
> > +#define PREP_SRC_SP_ID(x)              \
> > +                       (FIELD_PREP(PREP_SRC_SP_ID_MASK, (x)))
> > +
> > +/* Helper macro for setting the destination endpoint ID */
> > +#define PREP_NS_PHYS_ENDPOINT_ID_MASK          GENMASK(15, 0)
> > +#define PREP_NS_PHYS_ENDPOINT_ID(x)            \
> > +                       (FIELD_PREP(PREP_NS_PHYS_ENDPOINT_ID_MASK, (x)))
> > +
> > +/*  RX/TX buffers minimum size */
> > +#define RXTX_BUFFERS_MIN_SIZE (RXTX_4K)
> > +#define RXTX_BUFFERS_MIN_PAGES (1)
> > +
> > +/* MBZ registers info */
> > +
> > +/* x1-x7 MBZ */
> > +#define FFA_X1X7_MBZ_CNT (7)
> > +#define FFA_X1X7_MBZ_REG_START (&res->a1)
> > +
> > +/* x4-x7 MBZ */
> > +#define FFA_X4X7_MBZ_CNT (4)
> > +#define FFA_X4X7_MBZ_REG_START (&res->a4)
> > +
> > +/* x3-x7 MBZ */
> > +#define FFA_X3X7_MBZ_CNT (5)
> > +#define FFA_X3_MBZ_REG_START (&res->a3)
> > +
> > +/* number of secure partitions emulated by the FF-A sandbox driver */
> > +#define SANDBOX_PARTITIONS_CNT (4)
> > +
> > +/* Binary data of services UUIDs emulated by the FF-A sandbox driver */
> > +
> > +/* service 1  UUID binary data (little-endian format) */
> > +#define SANDBOX_SERVICE1_UUID_A1       0xed32d533
> > +#define SANDBOX_SERVICE1_UUID_A2       0x99e64209
> > +#define SANDBOX_SERVICE1_UUID_A3       0x9cc02d72
> > +#define SANDBOX_SERVICE1_UUID_A4       0xcdd998a7
> > +
> > +/* service 2  UUID binary data (little-endian format) */
> > +#define SANDBOX_SERVICE2_UUID_A1       0xed32d544
> > +#define SANDBOX_SERVICE2_UUID_A2       0x99e64209
> > +#define SANDBOX_SERVICE2_UUID_A3       0x9cc02d72
> > +#define SANDBOX_SERVICE2_UUID_A4       0xcdd998a7
> > +
> > +/**
> > + * struct ffa_rxtxpair_info - structure hosting the RX/TX buffers flags
> > + * @rxbuf_owned:       RX buffer ownership flag (the owner is non secure world: the consumer)
> > + * @rxbuf_mapped:      RX buffer mapping flag
> > + * @txbuf_owned        TX buffer ownership flag
> > + * @txbuf_mapped:      TX buffer mapping flag
> > + * @rxtx_buf_size:     RX/TX buffers size as set by the FF-A core driver
> > + *
> > + * Data structure hosting the ownership/mapping flags of the RX/TX buffers
> > + * When a buffer is owned/mapped its corresponding flag is set to 1 otherwise 0.
> > + */
> > +struct ffa_rxtxpair_info {
> > +       u8 rxbuf_owned;
> > +       u8 rxbuf_mapped;
> > +       u8 txbuf_owned;
> > +       u8 txbuf_mapped;
> > +       u32 rxtx_buf_size;
> > +};
> > +
> > +/**
> > + * struct sandbox_ffa_priv - the driver private data structure
> > + *
> > + * @dev:       The arm_ffa device under u-boot driver model
> > + * @fwk_version:       FF-A framework version
> > + * @id:        u-boot endpoint ID
> > + * @partitions:        The partitions descriptors structure
> > + * @pair:      The RX/TX buffers pair
> > + * @pair_info: The RX/TX buffers pair flags and size
> > + * @conduit:   The selected conduit
> > + *
> > + * The driver data structure hosting all the emulated secure world data.
> > + */
> > +struct sandbox_ffa_priv {
> > +       struct udevice *dev;
> > +       u32 fwk_version;
> > +       u16 id;
> > +       struct ffa_partitions partitions;
> > +       struct ffa_rxtxpair pair;
> > +       struct ffa_rxtxpair_info pair_info;
> > +};
> > +
> > +#define SANDBOX_SMC_FFA_ABI(ffabi) static int sandbox_##ffabi(struct udevice *dev, \
> > +                                                             ffa_value_t *pargs, ffa_value_t *res)
> 
> drop that
> 
> [..]
> 
> Regards,
> Simon