[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