Hi Abdellatif, On Wed, 29 Mar 2023 at 05:12, Abdellatif El Khlifi < abdellatif.elkhl...@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.elkhl...@arm.com> > Cc: Tom Rini <tr...@konsulko.com> > Cc: Simon Glass <s...@chromium.org> > Cc: Ilias Apalodimas <ilias.apalodi...@linaro.org> > Cc: Jens Wiklander <jens.wiklan...@linaro.org> > Cc: Heinrich Schuchardt <xypron.g...@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.elkhl...@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-off...@arm.com> > + * > + * Authors: > + * Abdellatif El Khlifi <abdellatif.elkhl...@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. > + 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 ? > + * 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