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

Ilias Apalodimas
2023-06-20T14:26:37Z

[...]

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 43603522fd..0d960731cf 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 365371fb51..86af7bcc9e 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 6c37521b4e..7d20a85a46 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -12,6 +12,7 @@ obj-y += panic.o
> obj-y += version.o
>
> # command
> +obj-$(CONFIG_CMD_ARMFFA) += armffa.o
> obj-$(CONFIG_CMD_2048) += 2048.o
> obj-$(CONFIG_CMD_ACPI) += acpi.o
> obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
> diff --git a/cmd/armffa.c b/cmd/armffa.c
> new file mode 100644
> index 0000000000..fa268e9cb9
> --- /dev/null
> +++ b/cmd/armffa.c
> @@ -0,0 +1,196 @@
> +// 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)

Why isn't this static? If it's used in another file we need to move
it to a library suitable file.

> +{
> + int ret;
> +
> + ret = uclass_first_device_err(UCLASS_FFA, devp);
> + if (ret) {
> + log_err("Cannot find FF-A bus device\n");
> + 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_desc *descs;
> + u32 i;
> + struct udevice *dev;
> +
> + if (argc != 2) {
> + log_err("Missing argument\n");
> + return CMD_RET_USAGE;
> + }
> +
> + ret = ffa_get_dev(&dev);
> + if (ret)
> + return CMD_RET_FAILURE;
> +
> + /* Ask the driver to fill the buffer with the SPs info */
> +
> + ret = ffa_partition_info_get(dev, argv[1], &count, &descs);
> + if (ret) {
> + log_err("Failure in querying partition(s) info (error code: %d)\n", ret);
> + 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",
> + descs[i].info.id,
> + descs[i].info.exec_ctxt,
> + descs[i].info.properties);
> + }
> +
> + 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

s/which/ in which/

> + * 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;
> +
> + if (argc != 2) {
> + log_err("Missing argument\n");
> + return CMD_RET_USAGE;
> + }
> +
> + errno = 0;
> + part_id = strtoul(argv[1], NULL, 16);
> +
> + if (errno) {

Is errno used in strtoul?
Does FF-A have any limits regarding the partition id? If yes, it would
be saner to check against that.

> + 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]);

I am not sure I understand why we print it like this.

> + 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));

Isn't it more useful to print the physical address map_to_sysmem() retuns?

> +
> + return CMD_RET_SUCCESS;
> +}
> +
>

Thanks
/Ilias