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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

Am 12. April 2023 17:53:23 MESZ schrieb Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>:
>On Wed, Apr 12, 2023 at 04:02:43PM +0200, Heinrich Schuchardt wrote:
>> On 4/12/23 11:42, Abdellatif El Khlifi 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:
>> > ===============
>> > 
>> > 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
>> > 
>> > 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@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
>> > +
>> > +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@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>
>> > +
>> > +/**
>> > + * 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 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));
>> > +	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;
>> > +}
>> > +
>> > +/**
>> > + * do_ffa_ping() - implementation of the ping subcommand
>> > + * @cmdtp:		Command Table
>> > + * @flag:		flags
>> > + * @argc:		number of arguments
>> > + * @argv:		arguments
>> > + *
>> > + * Send data to the secure partition which the ID is provided
>> > + * as an argument. Use 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;
>> > +
>> > +	errno = 0;
>> > +	part_id = strtoul(argv[1], NULL, 16);
>> > +
>> > +	if (errno) {
>> > +		log_err("Invalid partition ID\n");
>> > +		return CMD_RET_USAGE;
>> > +	}
>> > +
>> > +	ret = ffa_get_dev(&dev);
>> > +	if (ret)
>> > +		return CMD_RET_FAILURE;
>> > +
>> > +	ret = ffa_sync_send_receive(dev, part_id, &msg, 1);
>> > +	if (!ret) {
>> > +		u8 cnt;
>> > +
>> > +		log_info("SP response:\n[LSB]\n");
>> > +		for (cnt = 0;
>> > +		     cnt < sizeof(struct ffa_send_direct_data) / sizeof(u64);
>> > +		     cnt++)
>> > +			log_info("%llx\n", ((u64 *)&msg)[cnt]);
>> > +		return CMD_RET_SUCCESS;
>> > +	}
>> > +
>> > +	log_err("Sending direct request error (%d)\n", ret);
>> > +	return CMD_RET_FAILURE;
>> > +}
>> > +
>> > +/**
>> > + *do_ffa_devlist() - implementation of the devlist subcommand
>> > + * @cmdtp: [in]		Command Table
>> > + * @flag:		flags
>> > + * @argc:		number of arguments
>> > + * @argv:		arguments
>> > + *
>> > + * Query 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;
>> > +	int ret;
>> > +
>> > +	ret = ffa_get_dev(&dev);
>> > +	if (ret)
>> > +		return CMD_RET_FAILURE;
>> > +
>> > +	log_info("device name %s, dev %p, driver name %s, ops %p\n",
>> > +		 dev->name,
>> > +		(void *)map_to_sysmem(dev),
>> > +		 dev->driver->name,
>> > +		 (void *)map_to_sysmem(dev->driver->ops));
>> > +
>> > +	return CMD_RET_SUCCESS;
>> > +}
>> > +
>> > +static char armffa_help_text[] =
>> > +	"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";
>> > +
>> > +U_BOOT_CMD_WITH_SUBCMDS(armffa, "Arm FF-A test command", armffa_help_text,
>> > +			U_BOOT_SUBCMD_MKENT(getpart, 2, 1, do_ffa_getpart),
>> > +			U_BOOT_SUBCMD_MKENT(ping, 2, 1, do_ffa_ping),
>> > +			U_BOOT_SUBCMD_MKENT(devlist, 1, 1, do_ffa_devlist));
>> > diff --git a/doc/arch/arm64.ffa.rst b/doc/arch/arm64.ffa.rst
>> > index 466e77e3cd..ed7240aef7 100644
>> > --- a/doc/arch/arm64.ffa.rst
>> > +++ b/doc/arch/arm64.ffa.rst
>> > @@ -216,6 +216,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..e73d03ae51
>> > --- /dev/null
>> > +++ b/doc/usage/cmd/armffa.rst
>> > @@ -0,0 +1,105 @@
>> > +.. 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
>> > +   SP response:
>> > +   [LSB]
>> > +   fffffffe
>> > +   0
>> > +   0
>> > +   0
>> > +   0
>> > +
>> > +* ping (failure case)
>> > +
>> > +::
>> > +
>> > +   corstone1000# armffa ping 0
>> > +   Sending direct request error (-22)
>> > +
>> > +* getpart
>> > +
>> > +::
>> > +
>> > +   corstone1000# armffa getpart 33d532ed-e699-0942-c09c-a798d9cd722d
>> > +   Preparing for checking FF-A partitions count
>> > +   Searching FF-A partitions using the provided UUID
>> > +   No FF-A partition found. Querying framework ...
>> > +   Reading FF-A partitions data from the RX buffer
>> > +   Number of FF-A partition(s) matching the UUID: 1
>> > +   Pre-allocating 1 partition(s) info structures
>> > +   Preparing for filling FF-A partitions info
>> > +   Searching FF-A partitions using the provided UUID
>> > +   FF-A partition ID 8003 matches the provided UUID
>> > +   Partition: id = 8003 , exec_ctxt 1 , properties 3
>> > +
>> > +* getpart (failure case)
>> > +
>> > +::
>> > +
>> > +   corstone1000# armffa getpart 33d532ed-e699-0942-c09c-a798d9cd7221
>> > +   Preparing for checking FF-A partitions count
>> > +   Searching FF-A partitions using the provided UUID
>> > +   No FF-A partition found. Querying framework ...
>> > +   INVALID_PARAMETERS: Unrecognized UUID
>> > +   Failure in querying partitions count (error code: -22)
>> > +
>> > +* devlist
>> > +
>> > +::
>> > +
>> > +   corstone1000# armffa devlist
>> > +   device name arm_ffa, dev 00000000fdf41c30, driver name arm_ffa, ops 00000000fffc0e98
>> > +
>> > +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.
>> 
>> CMD_RET_FAILURE results in $? being 1 (false). Did you actually see a
>> negative value?
>
>For armffa, the possible return codes are: CMD_RET_USAGE (-1), CMD_RET_FAILURE (1) and CMD_RET_SUCCESS (0)
>
>I suggest replacing the sentence with:
>
>The return value $? is 0 (true) on success, -1 on usage error and 1 (false) on FF-A related failures.

CMD_RET_USAGE results in $? =1.

>
>Cheers
>Abdellatif
>
>> 
>> Best regards
>> 
>> Heinrich
>> 
>> 
>> > 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
>>