Re: [PATCH v10 05/10] arm_ffa: introduce armffa command

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

On Sun, Apr 02, 2023 at 02:40:34PM +1200, Simon Glass wrote:
> Hi Abdellatif,
> 
> On Wed, 29 Mar 2023 at 05:12, Abdellatif El Khlifi <
> abdellatif.elkhlifi@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@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:
> >
> > * 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                     | 238 +++++++++++++++++++++++++++++++
> >  doc/arch/arm64.ffa.rst           |   7 +
> >  doc/usage/cmd/armffa.rst         | 107 ++++++++++++++
> >  doc/usage/index.rst              |   1 +
> >  drivers/firmware/arm-ffa/Kconfig |   1 +
> >  8 files changed, 368 insertions(+)
> >  create mode 100644 cmd/armffa.c
> >  create mode 100644 doc/usage/cmd/armffa.rst
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 62c30184bb..add208e4ef 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@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 ba5ec69293..b814a20d8a 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 d95833b2de..a1eb45f881 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -12,6 +12,8 @@ obj-y += panic.o
> >  obj-y += version.o
> >
> >  # command
> > +
> > +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..d983a23bbc
> > --- /dev/null
> > +++ b/cmd/armffa.c
> > @@ -0,0 +1,238 @@
> > +// 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 <command.h>
> > +#include <dm.h>
> > +#include <mapmem.h>
> > +#include <stdlib.h>
> > +#include <asm/io.h>
> > +
> > +/**
> > + * do_ffa_getpart() - implementation of the getpart subcommand
> > + * @cmdtp:             Command Table
> > + * @flag:              flags
> > + * @argc:              number of arguments
> > + * @argv:              arguments
> > + *
> > + * This function queries the secure partition information which the UUID
> is provided
> 
> s/This function queries/Query/
> 
> We know it is a function so try to be brief and use the imperative mood
> like you do in commit messages.
> 
> > + * 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 info_idx;
> > +       struct udevice *dev = NULL;
> > +
> > +       if (argc != 1)
> > +               return -EINVAL;
> 
> Since this is a command handler, you must return CMD_RET_USAGE here. Please
> fix globally.
> 
> 
> > +
> > +       uclass_first_device(UCLASS_FFA, &dev);
> > +       if (!dev) {
> 
> ret = uclass_first_device_err(...)
> if (ret) {
> 
> (please fix globally)
> 
> > +               log_err("[FFA][CMD] Cannot find FF-A bus device\n");
> > +               return -ENODEV;
> 
> CMD_RET_FAILURE - please fix throughout. Often it is easier to put all this
> code (after arg checking) in a separate function which returns an error
> code, then have the do_... function check that and either return 0 or print
> a message and return CMD_RET_FAILURE.
> 
> > +       }
> > +
> > +       /* Mode 1: getting the number of secure partitions */
> > +       ret = ffa_partition_info_get(dev, argv[0], &count, NULL);
> > +       if (ret) {
> > +               log_err("[FFA][CMD] Failure in querying partitions count
> (error code: %d)\n", ret);
> 
> %dE gives you a nice error name if you want it.
> 
> Do you think you need the [FFA][CMD] stuff? Just the message should be
> enough.
> 
> > +               return ret;
> > +       }
> > +
> > +       if (!count) {
> > +               log_info("[FFA][CMD] No secure partition found\n");
> > +               return ret;
> > +       }
> > +
> > +       /*
> > +        * Pre-allocate a buffer to be filled by the driver
> > +        * with ffa_partition_info structs
> > +        */
> > +
> > +       log_info("[FFA][CMD] Pre-allocating %d partition(s) info
> structures\n", count);
> > +
> > +       parts_info = calloc(count, sizeof(struct ffa_partition_info));
> 
> Just use a local variable and avoid the allocation.

The number of partitions can't be known in advance. A dynamic allocation is needed in this case

> 
> > +       if (!parts_info)
> > +               return -EINVAL;
> > +
> > +       /* Ask the driver to fill the buffer with the SPs info */
> > +
> > +       ret = ffa_partition_info_get(dev, argv[0], &count, parts_info);
> > +       if (ret) {
> > +               log_err("[FFA][CMD] Failure in querying partition(s) info
> (error code: %d)\n", ret);
> > +               free(parts_info);
> > +               return ret;
> > +       }
> > +
> > +       /* SPs found , show the partition information */
> > +       for (info_idx = 0; info_idx < count ; info_idx++) {
> 
> We generally use 'i' for loops as it is shorter.
> 
> > +               log_info("[FFA][CMD] Partition: id = 0x%x , exec_ctxt
> 0x%x , properties 0x%x\n",
> 
> You don't need the 0x. Generally hex is used for everything in U-Boot. If
> you feel there is ambiguity, use %#x
> 
> > +                        parts_info[info_idx].id,
> > +                        parts_info[info_idx].exec_ctxt,
> > +                        parts_info[info_idx].properties);
> > +       }
> > +
> > +       free(parts_info);
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * do_ffa_ping() - implementation of the ping subcommand
> > + * @cmdtp:             Command Table
> > + * @flag:              flags
> > + * @argc:              number of arguments
> > + * @argv:              arguments
> > + *
> > + * This function sends data to the secure partition which the ID is
> provided
> 
> partition for which ?

The secure partition (aka SP) resides in the secure world and is discovered at runtime.
The purpose of the partition is providing secure services (e.g: firmware update, reading EFI variables, ...)

Cheers,
Abdellatif

> 
> > + * as an argument. The function uses the arm_ffa driver
> sync_send_receive operation
> > + * which implements FFA_MSG_SEND_DIRECT_{REQ,RESP} ABIs to send/receive
> data.
> > + *
> > + * Return:
> > + *
> > + * CMD_RET_SUCCESS: on success, otherwise failure
> > + */
> > +int do_ffa_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> argv[])
> > +{
> > +       struct ffa_send_direct_data msg = {
> > +                       .data0 = 0xaaaaaaaa,
> > +                       .data1 = 0xbbbbbbbb,
> > +                       .data2 = 0xcccccccc,
> > +                       .data3 = 0xdddddddd,
> > +                       .data4 = 0xeeeeeeee,
> > +       };
> > +       u16 part_id;
> > +       int ret;
> > +       struct udevice *dev = NULL;
> > +
> > +       if (argc != 1)
> > +               return -EINVAL;
> > +
> > +       errno = 0;
> > +       part_id = strtoul(argv[0], NULL, 16);
> > +
> > +       if (errno) {
> > +               log_err("[FFA][CMD] Invalid partition ID\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       uclass_first_device(UCLASS_FFA, &dev);
> > +       if (!dev) {
> > +               log_err("[FFA][CMD] Cannot find FF-A bus device\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       ret = ffa_sync_send_receive(dev, part_id, &msg, 1);
> > +       if (!ret) {
> > +               u8 cnt;
> > +
> > +               log_info("[FFA][CMD] SP response:\n[LSB]\n");
> > +               for (cnt = 0;
> > +                    cnt < sizeof(struct ffa_send_direct_data) /
> sizeof(u64);
> > +                    cnt++)
> > +                       log_info("[FFA][CMD] 0x%llx\n", ((u64
> *)&msg)[cnt]);
> > +       } else {
> > +               log_err("[FFA][CMD] Sending direct request error (%d)\n",
> ret);
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + *do_ffa_devlist() - implementation of the devlist subcommand
> > + * @cmdtp: [in]                Command Table
> > + * @flag:              flags
> > + * @argc:              number of arguments
> > + * @argv:              arguments
> > + *
> > + * This function queries the device belonging to the UCLASS_FFA
> > + * class.
> > + *
> > + * Return:
> > + *
> > + * CMD_RET_SUCCESS: on success, otherwise failure
> > + */
> > +int do_ffa_devlist(struct cmd_tbl *cmdtp, int flag, int argc, char
> *const argv[])
> > +{
> > +       struct udevice *dev = NULL;
> > +
> > +       uclass_first_device(UCLASS_FFA, &dev);
> > +       if (!dev) {
> > +               log_err("[FFA][CMD] Cannot find FF-A bus device\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       log_info("[FFA][CMD] device name %s, dev %08x, driver name %s,
> ops %08x\n",
> > +                dev->name,
> > +                (u32)map_to_sysmem(dev),
> 
> use %p as this is a pointer
> 
> > +                dev->driver->name,
> > +                (u32)map_to_sysmem(dev->driver->ops));
> 
> Use %p as this is a pointer
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static struct cmd_tbl armffa_commands[] = {
> > +       U_BOOT_CMD_MKENT(getpart, 1, 1, do_ffa_getpart, "", ""),
> > +       U_BOOT_CMD_MKENT(ping, 1, 1, do_ffa_ping, "", ""),
> > +       U_BOOT_CMD_MKENT(devlist, 0, 1, do_ffa_devlist, "", ""),
> > +};
> > +
> > +/**
> > + * do_armffa() - the armffa command main function
> > + * @cmdtp:     Command Table
> > + * @flag:              flags
> > + * @argc:              number of arguments
> > + * @argv:              arguments
> > + *
> > + * This function identifies which armffa subcommand to run.
> > + * Then, it makes sure the arm_ffa device is probed and
> > + * ready for use.
> > + * Then, it runs the subcommand.
> > + *
> > + * Return:
> > + *
> > + * CMD_RET_SUCCESS: on success, otherwise failure
> > + */
> > +static int do_armffa(struct cmd_tbl *cmdtp, int flag, int argc, char
> *const argv[])
> 
> You can use U_BOOT_CMD_WITH_SUBCMDS and drop this function
> 
> > +{
> > +       struct cmd_tbl *armffa_cmd;
> > +       int ret;
> > +
> > +       if (argc < 2)
> > +               return CMD_RET_USAGE;
> > +
> > +       armffa_cmd = find_cmd_tbl(argv[1], armffa_commands,
> ARRAY_SIZE(armffa_commands));
> > +
> > +       argc -= 2;
> > +       argv += 2;
> > +
> > +       if (!armffa_cmd || argc > armffa_cmd->maxargs)
> > +               return CMD_RET_USAGE;
> > +
> > +       ret = armffa_cmd->cmd(armffa_cmd, flag, argc, argv);
> > +
> > +       return cmd_process_error(armffa_cmd, ret);
> > +}
> > +
> > +U_BOOT_CMD(armffa, 4, 1, do_armffa,
> > +          "Arm FF-A operations test command",
> > +          "getpart <partition UUID>\n"
> > +          "     - lists the partition(s) info\n"
> > +          "ping <partition ID>\n"
> > +          "     - sends a data pattern to the specified partition\n"
> > +          "devlist\n"
> > +          "     - displays information about the FF-A device/driver\n");
> > diff --git a/doc/arch/arm64.ffa.rst b/doc/arch/arm64.ffa.rst
> > index ddf6435402..5fedb0c255 100644
> > --- a/doc/arch/arm64.ffa.rst
> > +++ b/doc/arch/arm64.ffa.rst
> > @@ -218,6 +218,13 @@ The following features are provided:
> >
> >  - FF-A bus can be compiled and used without EFI
> >
> > +The armffa command
> > +-----------------------------------
> > +
> > +armffa is an implementation defined command showcasing how to use the
> FF-A bus and how to invoke the driver operations.
> > +
> > +Please refer the command documentation at doc/usage/cmd/armffa.rst
> > +
> >  Example of boot logs with FF-A enabled
> >  --------------------------------------
> >
> > diff --git a/doc/usage/cmd/armffa.rst b/doc/usage/cmd/armffa.rst
> > new file mode 100644
> > index 0000000000..9bf59e393b
> > --- /dev/null
> > +++ b/doc/usage/cmd/armffa.rst
> > @@ -0,0 +1,107 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
> > +
> > +armffa command
> > +==============
> > +
> > +Synopsis
> > +--------
> > +
> > +::
> > +
> > +   armffa [sub-command] [arguments]
> > +
> > +   sub-commands:
> > +
> > +        getpart [partition UUID]
> > +
> > +            lists the partition(s) info
> > +
> > +        ping [partition ID]
> > +
> > +            sends a data pattern to the specified partition
> > +
> > +        devlist
> > +
> > +            displays information about the FF-A device/driver
> > +
> > +Description
> > +-----------
> > +
> > +armffa is a command showcasing how to use the FF-A bus and how to invoke
> its 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.
> > +
> > +Example
> > +-------
> > +
> > +The following examples are run on Corstone-1000 platform with debug logs
> enabled.
> > +
> > +* ping
> > +
> > +::
> > +
> > +   corstone1000# armffa ping 0x8003
> > +   [FFA][CMD] SP response:
> > +   [LSB]
> > +   [FFA][CMD] 0xfffffffe
> > +   [FFA][CMD] 0x0
> > +   [FFA][CMD] 0x0
> > +   [FFA][CMD] 0x0
> > +   [FFA][CMD] 0x0
> > +
> > +* ping (failure case)
> > +
> > +::
> > +
> > +   corstone1000# armffa ping 0x8009
> > +   [FFA][CMD] Sending direct request error (-22)
> > +   Command 'ping' failed: Error -22
> > +
> > +* getpart
> > +
> > +::
> > +
> > +   corstone1000# armffa getpart 33d532ed-e699-0942-c09c-a798d9cd722d
> > +   [FFA] Preparing for checking partitions count
> > +   [FFA] Searching partitions using the provided UUID
> > +   [FFA] No partition found. Querying framework ...
> > +   [FFA] Reading partitions data from the RX buffer
> > +   [FFA] Number of partition(s) matching the UUID: 1
> > +   [FFA][CMD] Pre-allocating 1 partition(s) info structures
> > +   [FFA] Preparing for filling partitions info
> > +   [FFA] Searching partitions using the provided UUID
> > +   [FFA] Partition ID 8003 matches the provided UUID
> > +   [FFA][CMD] Partition: id = 0x8003 , exec_ctxt 0x1 , properties 0x3
> 
> To me this [FFA] stuff is redundant and it looks awful. Please drop it.
> 
> > +
> > +* getpart (failure case)
> > +
> > +::
> > +
> > +   corstone1000# armffa getpart 33d532ed-e699-0942-c09c-a798d9cd7228
> > +   [FFA] Preparing for checking partitions count
> > +   [FFA] Searching partitions using the provided UUID
> > +   [FFA] No partition found. Querying framework ...
> > +   [FFA] INVALID_PARAMETERS: Unrecognized UUID
> > +   [FFA][CMD] Failure in querying partitions count (error code: -22)
> > +   Command 'getpart' failed: Error -22
> > +
> > +* devlist
> > +
> > +::
> > +
> > +   corstone1000# armffa devlist
> > +   [FFA][CMD] device name arm_ffa, dev fdf41c30, driver name arm_ffa,
> ops fffc0fc8
> > +
> > +Configuration
> > +-------------
> > +
> > +The command is available if CONFIG_CMD_ARMFFA=y and
> CONFIG_ARM_FFA_TRANSPORT=y.
> > +
> > +Return value
> > +------------
> > +
> > +The return value $? is 0 (true) on success and a negative error code on
> failure.
> > diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> > index bc85e1d49a..df107fb710 100644
> > --- a/doc/usage/index.rst
> > +++ b/doc/usage/index.rst
> > @@ -21,6 +21,7 @@ Shell commands
> >
> >     cmd/acpi
> >     cmd/addrmap
> > +   cmd/armffa
> >     cmd/askenv
> >     cmd/base
> >     cmd/bdinfo
> > diff --git a/drivers/firmware/arm-ffa/Kconfig
> b/drivers/firmware/arm-ffa/Kconfig
> > index 9200c8028b..a7d5392859 100644
> > --- a/drivers/firmware/arm-ffa/Kconfig
> > +++ b/drivers/firmware/arm-ffa/Kconfig
> > @@ -5,6 +5,7 @@ config ARM_FFA_TRANSPORT
> >         depends on DM && ARM64
> >         select ARM_SMCCC
> >         select ARM_SMCCC_FEATURES
> > +       imply CMD_ARMFFA
> >         select LIB_UUID
> >         select DEVRES
> >         help
> > --
> > 2.25.1
> >
> 
> Regards,
> SImon