From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 50213C77B7F for ; Fri, 12 May 2023 12:16:39 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id B9A62862C6; Fri, 12 May 2023 14:15:29 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 5870086249; Fri, 12 May 2023 14:15:12 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by phobos.denx.de (Postfix) with ESMTP id 0EE9C86249 for ; Fri, 12 May 2023 14:15:08 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=abdellatif.elkhlifi@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C9BD1FEC; Fri, 12 May 2023 05:15:51 -0700 (PDT) Received: from e130802.arm.com (unknown [10.57.83.163]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7A7EC3F67D; Fri, 12 May 2023 05:15:06 -0700 (PDT) Date: Fri, 12 May 2023 13:14:58 +0100 From: Abdellatif El Khlifi To: Simon Glass Cc: nd@arm.com, u-boot@lists.denx.de Subject: Re: [PATCH v11 05/10] arm_ffa: introduce armffa command Message-ID: <20230512121458.GA112709@e130802.arm.com> References: <20230328161157.219375-1-abdellatif.elkhlifi@arm.com> <20230412094245.44674-1-abdellatif.elkhlifi@arm.com> <20230412094245.44674-6-abdellatif.elkhlifi@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi Simon, > > On Wed, 12 Apr 2023 at 03:43, 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 > > Cc: Tom Rini > > Cc: Simon Glass > > Cc: Ilias Apalodimas > > Cc: Jens Wiklander > > Cc: Heinrich Schuchardt > > > > --- > > 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 > > 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 > > 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 > > + * > > + * Authors: > > + * Abdellatif El Khlifi > > + */ > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +/** > > + * 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