[PATCH v11 05/10] arm_ffa: introduce armffa command

Abdellatif El Khlifi abdellatif.elkhlifi at arm.com
Fri May 12 14:14:58 CEST 2023


Hi Simon,

> 
> On Wed, 12 Apr 2023 at 03:43, Abdellatif El Khlifi
> <abdellatif.elkhlifi at arm.com> wrote:
> >
> > Provide armffa command showcasing the use of the U-Boot FF-A support
> >
> > armffa is a command showcasing how to invoke FF-A operations.
> > This provides a guidance to the client developers on how to
> > call the FF-A bus interfaces. The command also allows to gather secure
> > partitions information and ping these  partitions. The command is also
> > helpful in testing the communication with secure partitions.
> >
> > For more details please refer to the command documentation [1].
> >
> > [1]: doc/usage/cmd/armffa.rst
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> > Cc: Tom Rini <trini at konsulko.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > Cc: Jens Wiklander <jens.wiklander at linaro.org>
> > Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >
> > ---
> > Changelog:
> > ===============
> >
> > v11:
> >
> > * use U_BOOT_CMD_WITH_SUBCMDS
> > * address nits
> >
> > v10:
> >
> > * use the FF-A driver Uclass operations
> > * use uclass_first_device()
> > * address nits
> >
> > v9:
> >
> > * remove manual FF-A discovery and use DM
> > * use DM class APIs to probe and interact with the FF-A bus
> > * add doc/usage/cmd/armffa.rst
> >
> > v8:
> >
> > * update partition_info_get() second argument to be an SP count
> > * pass NULL device pointer to the FF-A bus discovery and operations
> >
> > v7:
> >
> > * adapt do_ffa_dev_list() following the recent update on
> >   uclass_first_device/uclass_next_device functions (they return void now)
> > * set armffa command to use 64-bit direct messaging
> >
> > v4:
> >
> > * remove pattern data in do_ffa_msg_send_direct_req
> >
> > v3:
> >
> > * use the new driver interfaces (partition_info_get, sync_send_receive)
> >   in armffa command
> >
> > v2:
> >
> > * replace use of ffa_helper_init_device function by
> >  ffa_helper_bus_discover
> >
> > v1:
> >
> > * introduce armffa command
> >
> >  MAINTAINERS                      |   2 +
> >  cmd/Kconfig                      |  10 ++
> >  cmd/Makefile                     |   2 +
> >  cmd/armffa.c                     | 212 +++++++++++++++++++++++++++++++
> >  doc/arch/arm64.ffa.rst           |   7 +
> >  doc/usage/cmd/armffa.rst         | 105 +++++++++++++++
> >  doc/usage/index.rst              |   1 +
> >  drivers/firmware/arm-ffa/Kconfig |   1 +
> >  8 files changed, 340 insertions(+)
> >  create mode 100644 cmd/armffa.c
> >  create mode 100644 doc/usage/cmd/armffa.rst
> >
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> with nits below
> 
> For your docs, please use a proper rST link for
> doc/usage/cmd/armffa.rst so people can click on it and view the
> command docs.
> 
> Also:
> 
> armffa is an implementation-defined command
> 
> (add hyphen)
> 
> although I'm not sure how an implementation can define the command?
> 
> The example output in your 'example' docs is very, very verbose.
> Shouldn't it just report problems?

Thanks, addressed in v12.

Cheers,
Abdellatif

> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 76f0f276ce..c64804ca2d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -269,7 +269,9 @@ F:  configs/cortina_presidio-asic-pnand_defconfig
> >  ARM FF-A
> >  M:     Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> >  S:     Maintained
> > +F:     cmd/armffa.c
> >  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
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 8c9b430f99..4cb0b2c167 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -934,6 +934,16 @@ endmenu
> >
> >  menu "Device access commands"
> >
> > +config CMD_ARMFFA
> > +       bool "Arm FF-A test command"
> > +       depends on ARM_FFA_TRANSPORT
> > +       help
> > +         Provides a test command for the FF-A support
> > +         supported options:
> > +               - Listing the partition(s) info
> > +               - Sending a data pattern to the specified partition
> > +               - Displaying the arm_ffa device info
> > +
> >  config CMD_ARMFLASH
> >         #depends on FLASH_CFI_DRIVER
> >         bool "armflash"
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index e032091621..9130b9078d 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -12,6 +12,8 @@ obj-y += panic.o
> >  obj-y += version.o
> >
> >  # command
> > +
> 
> Please drop blank line
> 
> > +obj-$(CONFIG_CMD_ARMFFA) += armffa.o
> >  obj-$(CONFIG_CMD_ACPI) += acpi.o
> >  obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
> >  obj-$(CONFIG_CMD_AES) += aes.o
> > diff --git a/cmd/armffa.c b/cmd/armffa.c
> > new file mode 100644
> > index 0000000000..ab88412c7d
> > --- /dev/null
> > +++ b/cmd/armffa.c
> > @@ -0,0 +1,212 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2022-2023 Arm Limited and/or its affiliates <open-source-office at arm.com>
> > + *
> > + * Authors:
> > + *   Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> > + */
> > +#include <common.h>
> > +#include <arm_ffa.h>
> > +#include <command.h>
> > +#include <dm.h>
> > +#include <mapmem.h>
> > +#include <stdlib.h>
> > +#include <asm/io.h>
> > +
> > +/**
> > + * ffa_get_dev() - Return the FF-A device
> > + * @devp:      pointer to the FF-A device
> > + *
> > + * Search for the FF-A device.
> > + *
> > + * Return:
> > + * 0 on success. Otherwise, failure
> > + */
> > +int ffa_get_dev(struct udevice **devp)
> > +{
> > +       int ret;
> > +
> > +       ret = uclass_first_device_err(UCLASS_FFA, devp);
> > +       if (ret) {
> > +               log_err("Cannot find FF-A bus device\n");
> > +               return -ENODEV;
> 
> return ret
> 
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * do_ffa_getpart() - implementation of the getpart subcommand
> > + * @cmdtp:             Command Table
> > + * @flag:              flags
> > + * @argc:              number of arguments
> > + * @argv:              arguments
> > + *
> > + * Query the secure partition information which the UUID is provided
> > + * as an argument. The function uses the arm_ffa driver
> > + * partition_info_get operation which implements FFA_PARTITION_INFO_GET
> > + * ABI to retrieve the data. The input UUID string is expected to be in big
> > + * endian format.
> > + *
> > + * Return:
> > + *
> > + * CMD_RET_SUCCESS: on success, otherwise failure
> > + */
> > +static int do_ffa_getpart(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                         char *const argv[])
> > +{
> > +       u32 count = 0;
> > +       int ret;
> > +       struct ffa_partition_info *parts_info;
> > +       u32 i;
> > +       struct udevice *dev;
> > +
> > +       ret = ffa_get_dev(&dev);
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +
> > +       /* Mode 1: getting the number of secure partitions */
> > +       ret = ffa_partition_info_get(dev, argv[1], &count, NULL);
> > +       if (ret) {
> > +               log_err("Failure in querying partitions count (error code: %d)\n", ret);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       if (!count) {
> > +               log_info("No secure partition found\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       /*
> > +        * Pre-allocate a buffer to be filled by the driver
> > +        * with ffa_partition_info structs
> > +        */
> > +
> > +       log_info("Pre-allocating %d partition(s) info structures\n", count);
> > +
> > +       parts_info = calloc(count, sizeof(struct ffa_partition_info));
> 
> This should be a local variable:
> 
> struct ffa_partition_info parts_info
> 
> I mentioned this before and you said that the number of things is
> fixed at runtime, but I don't see that here. Also I see is a simple
> struct, so there should be no need to allocate it.
> 
> > +       if (!parts_info)
> > +               return CMD_RET_FAILURE;
> > +
> > +       /* Ask the driver to fill the buffer with the SPs info */
> > +
> > +       ret = ffa_partition_info_get(dev, argv[1], &count, parts_info);
> > +       if (ret) {
> > +               log_err("Failure in querying partition(s) info (error code: %d)\n", ret);
> > +               free(parts_info);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       /* SPs found , show the partition information */
> > +       for (i = 0; i < count ; i++) {
> > +               log_info("Partition: id = %x , exec_ctxt %x , properties %x\n",
> > +                        parts_info[i].id,
> > +                        parts_info[i].exec_ctxt,
> > +                        parts_info[i].properties);
> > +       }
> > +
> > +       free(parts_info);
> > +
> > +       return CMD_RET_SUCCESS;
> > +}
> > +
> [..]
> 
> Regards,
> Simon


More information about the U-Boot mailing list