Re: [PATCH v2 1/6] arm_ffa: introduce Arm FF-A low-level driver

Hi Abdellatif On Fri, Apr 15, 2022 at 01:27:58PM +0100, abdellatif.elkhlifi@arm.com wrote:

From: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com

Add the driver implementing Arm Firmware Framework for Armv8-A v1.0

The Firmware Framework for Arm A-profile processors (FF-A) describes interfaces (ABIs) that standardize communication between the Secure World and Normal World leveraging TrustZone technology. This driver uses SMC32 calling convention.

In u-boot FF-A design, FF-A is considered as a discoverable bus. The Secure World is considered as one entity to communicate with using the FF-A bus. FF-A communication is handled by one device and one instance (the bus). This FF-A driver takes care of all the interactions between Normal world and Secure World.

The driver provides helper FF-A interfaces for user layers. These helper functions allow clients to pass data and select the FF-A function to use for the communication with secure world.

Signed-off-by: Abdellatif El Khlifi abdellatif.elkhlifi@arm.com +++ b/arch/arm/cpu/armv8/smccc-call.S @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0 */ /*

• Copyright (c) 2015, Linaro Limited

• • (C) Copyright 2022 ARM Limited
• • Abdellatif El Khlifi abdellatif.elkhlifi@arm.com
  */

#include <linux/linkage.h> #include <linux/arm-smccc.h> @@ -45,3 +47,28 @@ ENDPROC(__arm_smccc_smc) ENTRY(__arm_smccc_hvc) SMCCC hvc ENDPROC(__arm_smccc_hvc)

+#if (IS_ENABLED(CONFIG_ARM_FFA_TRANSPORT))

• .macro FFASMCCC instr
• .cfi_startproc
• \instr #0
• ldr x9, [sp]
• stp x0, x1, [x9, #ARM_SMCCC_RES_X0_OFFS]
• stp x2, x3, [x9, #ARM_SMCCC_RES_X2_OFFS]
• stp x4, x5, [x9, #ARM_SMCCC_RES_X4_OFFS]
• stp x6, x7, [x9, #ARM_SMCCC_RES_X6_OFFS]
• ret
• .cfi_endproc
• .endm

+/*

• • void arm_ffa_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2,

• •   unsigned long a3, unsigned long a4, unsigned long a5,
    

• •   unsigned long a6, unsigned long a7, struct arm_smccc_res *res)
    

• */

+ENTRY(__arm_ffa_smccc_smc)

• FFASMCCC smc

+ENDPROC(__arm_ffa_smccc_smc)

+#endif diff --git a/arch/arm/lib/asm-offsets.c b/arch/arm/lib/asm-offsets.c index 22fd541f9a..02a4a42fe6 100644 --- a/arch/arm/lib/asm-offsets.c +++ b/arch/arm/lib/asm-offsets.c @@ -9,6 +9,8 @@

• generate asm statements containing #defines,
• compile this file to assembler, and then extract the
• #defines from the assembly-language output.

• • (C) Copyright 2022 ARM Limited
  */

#include <common.h> @@ -115,6 +117,10 @@ int main(void) #ifdef CONFIG_ARM_SMCCC DEFINE(ARM_SMCCC_RES_X0_OFFS, offsetof(struct arm_smccc_res, a0)); DEFINE(ARM_SMCCC_RES_X2_OFFS, offsetof(struct arm_smccc_res, a2)); +#if (IS_ENABLED(CONFIG_ARM_FFA_TRANSPORT))

• DEFINE(ARM_SMCCC_RES_X4_OFFS, offsetof(struct arm_smccc_res, a4));
• DEFINE(ARM_SMCCC_RES_X6_OFFS, offsetof(struct arm_smccc_res, a6));

+#endif DEFINE(ARM_SMCCC_QUIRK_ID_OFFS, offsetof(struct arm_smccc_quirk, id)); DEFINE(ARM_SMCCC_QUIRK_STATE_OFFS, offsetof(struct arm_smccc_quirk, state)); #endif diff --git a/common/board_r.c b/common/board_r.c index b92c1bb0be..bb5f1d0aa6 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -62,6 +62,10 @@ #include <asm-generic/gpio.h> #include <efi_loader.h>

+#ifdef CONFIG_ARM_FFA_TRANSPORT +#include <arm_ffa_helper.h> +#endif

DECLARE_GLOBAL_DATA_PTR;

ulong monitor_flash_len; @@ -771,6 +775,9 @@ static init_fnc_t init_sequence_r[] = { INIT_FUNC_WATCHDOG_RESET initr_net, #endif +#ifdef CONFIG_ARM_FFA_TRANSPORT

• ffa_helper_bus_discover,

+#endif #ifdef CONFIG_POST initr_post, #endif diff --git a/drivers/Kconfig b/drivers/Kconfig index b26ca8cf70..e83c23789d 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -6,6 +6,8 @@ source "drivers/core/Kconfig"

source "drivers/adc/Kconfig"

+source "drivers/arm-ffa/Kconfig"

source "drivers/ata/Kconfig"

source "drivers/axi/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index 4e7cf28440..6671d2a604 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -107,6 +107,7 @@ obj-y += iommu/ obj-y += smem/ obj-y += thermal/ obj-$(CONFIG_TEE) += tee/ +obj-$(CONFIG_ARM_FFA_TRANSPORT) += arm-ffa/ obj-y += axi/ obj-y += ufs/ obj-$(CONFIG_W1) += w1/ diff --git a/drivers/arm-ffa/Kconfig b/drivers/arm-ffa/Kconfig new file mode 100644 index 0000000000..23815534c4 --- /dev/null +++ b/drivers/arm-ffa/Kconfig @@ -0,0 +1,27 @@ +# SPDX-License-Identifier: GPL-2.0

+config ARM_FFA_TRANSPORT

• bool "Enable Arm Firmware Framework for Armv8-A driver"
• depends on DM && ARM64
• select ARM_SMCCC if ARM64
• select LIB_UUID
• select ARM_FFA_TRANSPORT_HELPERS
• help

•  The Firmware Framework for Arm A-profile processors (FF-A)
  

•  describes interfaces (ABIs) that standardize communication
  

•  between the Secure World and Normal World leveraging TrustZone
  

•  technology.
  

•  This driver is based on FF-A specification v1.0 and uses SMC32
  

•  calling convention.
  

•  FF-A specification:
  

•  https://developer.arm.com/documentation/den0077/a/?lang=en
  

•  In u-boot FF-A design, FF-A is considered as a discoverable bus.
  

•  The Secure World is considered as one entity to communicate with
  

•  using the FF-A bus.
  

•  FF-A communication is handled by one device and one instance (the bus).
  

•  This FF-A driver takes care of all the interactions between Normal world
  

•  and Secure World.
  

diff --git a/drivers/arm-ffa/Makefile b/drivers/arm-ffa/Makefile new file mode 100644 index 0000000000..7bc9a336a9 --- /dev/null +++ b/drivers/arm-ffa/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# (C) Copyright 2022 Abdellatif El Khlifi abdellatif.elkhlifi@arm.com +#

+obj-y += arm-ffa-uclass.o core.o diff --git a/drivers/arm-ffa/arm-ffa-uclass.c b/drivers/arm-ffa/arm-ffa-uclass.c new file mode 100644 index 0000000000..2439f87586 --- /dev/null +++ b/drivers/arm-ffa/arm-ffa-uclass.c @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • (C) Copyright 2022 ARM Limited
• • Abdellatif El Khlifi abdellatif.elkhlifi@arm.com
• */

+#include <common.h> +#include <dm.h> +#include <arm_ffa.h> +#include <errno.h> +#include <log.h> +#include <asm/global_data.h>

+DECLARE_GLOBAL_DATA_PTR;

+UCLASS_DRIVER(ffa) = {

• .name = "ffa",
• .id = UCLASS_FFA,

+};

+/**

• • ffa_get_invoke_func - performs a call to the FF-A driver dispatcher
• • @func_id: The FF-A function to be used
• • @func_data: Pointer to the FF-A function arguments

• • 		container structure. This also includes
    

• • 		pointers to the returned data needed by
    

• • 		clients.
    

• • This runtime function passes the FF-A function ID and its arguments to
• • the FF-A driver dispatcher.
• • This function is called by the FF-A helper functions.
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+int __ffa_runtime ffa_get_invoke_func(u32 func_id, struct ffa_interface_data *func_data) +{

• if (!ffa_device_get_ops()->invoke_func)

• return -EINVAL;
  

• return ffa_device_get_ops()->invoke_func(func_id, func_data);

+}

+/**

• • ffa_bus_discover - discover FF-A bus and probe the arm_ffa device
• • This boot time function makes sure the FF-A bus is discoverable.
• • Then, the arm_ffa device is probed and ready to use.
• • This function is called automatically at initcalls
• • level (after u-boot relocation).
• • Arm FF-A transport is implemented through arm_ffa u-boot device managing the FF-A
• • communication.
• • All FF-A clients should use the arm_ffa device to use the FF-A transport.
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+int ffa_bus_discover(void) +{

• return ffa_get_device();

+} diff --git a/drivers/arm-ffa/arm_ffa_prv.h b/drivers/arm-ffa/arm_ffa_prv.h new file mode 100644 index 0000000000..44f258addb --- /dev/null +++ b/drivers/arm-ffa/arm_ffa_prv.h @@ -0,0 +1,193 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • (C) Copyright 2022 ARM Limited
• • Abdellatif El Khlifi abdellatif.elkhlifi@arm.com
• */

+#ifndef __ARM_FFA_PRV_H +#define __ARM_FFA_PRV_H

+#include <arm_ffa.h> +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <dm/read.h>

+/*

• • This header is private. It is exclusively used by the FF-A driver
• */

+/* FF-A core driver name */ +#define FFA_DRV_NAME "arm_ffa"

+/* FF-A driver version definitions */

+#define MAJOR_VERSION_MASK GENMASK(30, 16) +#define MINOR_VERSION_MASK GENMASK(15, 0) +#define GET_FFA_MAJOR_VERSION(x) \

• 		((u16)(FIELD_GET(MAJOR_VERSION_MASK, (x))))
  

+#define GET_FFA_MINOR_VERSION(x) \

• 		((u16)(FIELD_GET(MINOR_VERSION_MASK, (x))))
  

+#define PACK_VERSION_INFO(major, minor) \

• (FIELD_PREP(MAJOR_VERSION_MASK, (major)) | \

• FIELD_PREP(MINOR_VERSION_MASK, (minor)))
  

+#define FFA_MAJOR_VERSION (1) +#define FFA_MINOR_VERSION (0) +#define FFA_VERSION_1_0 \

• 	PACK_VERSION_INFO(FFA_MAJOR_VERSION, FFA_MINOR_VERSION)
  

+/* Endpoint ID mask (u-boot endpoint ID) */

+#define GET_SELF_ENDPOINT_ID_MASK GENMASK(15, 0) +#define GET_SELF_ENDPOINT_ID(x) \

• 	((u16)(FIELD_GET(GET_SELF_ENDPOINT_ID_MASK, (x))))
  

+#define PREP_SELF_ENDPOINT_ID_MASK GENMASK(31, 16) +#define PREP_SELF_ENDPOINT_ID(x) \

• 	(FIELD_PREP(PREP_SELF_ENDPOINT_ID_MASK, (x)))
  

+/* Partition endpoint ID mask (partition with which u-boot communicates with) */

+#define PREP_PART_ENDPOINT_ID_MASK GENMASK(15, 0) +#define PREP_PART_ENDPOINT_ID(x) \

• 	(FIELD_PREP(PREP_PART_ENDPOINT_ID_MASK, (x)))
  

+/* The FF-A SMC function prototype definition */

+typedef void (*invoke_ffa_fn_t)(unsigned long a0, unsigned long a1,

• 	unsigned long a2, unsigned long a3, unsigned long a4,
  

• 	unsigned long a5, unsigned long a6, unsigned long a7,
  

• 	struct arm_smccc_res *res);
  

+/**

• • enum ffa_conduit - Arm FF-A conduits supported by the Arm FF-A driver
• • Currently only SMC32 is supported.
• */

+enum ffa_conduit {

• FFA_CONDUIT_SMC = 0,

+};

+/**

• • FFA_DECLARE_ARGS - FF-A functions local variables
• • @a0-a7: local variables used to set registers x0-x7
• • @res: the structure hosting the FF-A function return data
• • A helper macro for declaring local variables for the FF-A functions arguments.
• • The x0-x7 registers are used to exchange data with the secure world.
• • But, only the bottom 32-bit of thes registers contains the data.
• */

+#define FFA_DECLARE_ARGS \

• unsigned long a0 = 0; \
• unsigned long a1 = 0; \
• unsigned long a2 = 0; \
• unsigned long a3 = 0; \
• unsigned long a4 = 0; \
• unsigned long a5 = 0; \
• unsigned long a6 = 0; \
• unsigned long a7 = 0; \
• struct arm_smccc_res res = {0}

+/* FF-A error codes */ +#define FFA_ERR_STAT_NOT_SUPPORTED (-1) +#define FFA_ERR_STAT_INVALID_PARAMETERS (-2) +#define FFA_ERR_STAT_NO_MEMORY (-3) +#define FFA_ERR_STAT_BUSY (-4) +#define FFA_ERR_STAT_INTERRUPTED (-5) +#define FFA_ERR_STAT_DENIED (-6) +#define FFA_ERR_STAT_RETRY (-7) +#define FFA_ERR_STAT_ABORTED (-8)

+/**

• • struct ffa_features_desc - FF-A functions features
• • @func_id: FF-A function
• • @field1: features read from register w2
• • @field2: features read from register w3
• • Data structure describing the features of the FF-A functions queried by
• • FFA_FEATURES
• */

+struct ffa_features_desc {

• u32 func_id;
• u32 field1;
• u32 field2;

+};

+/**

• • enum ffa_rxtx_buf_sizes - minimum sizes supported
• • for the RX/TX buffers
• */

+enum ffa_rxtx_buf_sizes {

• RXTX_4K,
• RXTX_64K,
• RXTX_16K

+};

+/*

• • Number of the FF-A interfaces features descriptors
• • currently only FFA_RXTX_MAP descriptor is supported
• */

+#define FFA_FEATURE_DESC_CNT (1)

+/**

• • struct ffa_rxtxpair - structure hosting the RX/TX buffers virtual addresses
• • @rxbuf: virtual address of the RX buffer
• • @txbuf: virtual address of the TX buffer
• • Data structure hosting the virtual addresses of the mapped RX/TX buffers
• • These addresses are used by the FF-A functions that use the RX/TX buffers
• */

+struct ffa_rxtxpair {

• u64 rxbuf; /* virtual address */
• u64 txbuf; /* virtual address */

+};

+/**

• • struct ffa_partition_desc - the secure partition descriptor
• • @info: partition information
• • @UUID: UUID
• • Each partition has its descriptor containing the partitions information and the UUID
• */

+struct ffa_partition_desc {

• struct ffa_partition_info info;
• union ffa_partition_uuid UUID;

+};

+/**

• • struct ffa_partitions - descriptors for all secure partitions
• • @count: The number of partitions descriptors
• • @descs The partitions descriptors table
• • This data structure contains the partitions descriptors table
• */

+struct ffa_partitions {

• u32 count;
• struct ffa_partition_desc *descs; /* virtual address */

+};

+/**

• • struct ffa_prvdata - the driver private data structure
• • @dev: The arm_ffa device under u-boot driver model
• • @fwk_version: FF-A framework version
• • @id: u-boot endpoint ID
• • @partitions: The partitions descriptors structure
• • @pair: The RX/TX buffers pair
• • @conduit: The selected conduit
• • @invoke_ffa_fn: The function executing the FF-A function
• • @features: Table of the FF-A functions having features
• • The driver data structure hosting all resident data.
• */

+struct ffa_prvdata {

• struct udevice *dev;
• u32 fwk_version;
• u16 id;
• struct ffa_partitions partitions;
• struct ffa_rxtxpair pair;
• enum ffa_conduit conduit;
• invoke_ffa_fn_t invoke_ffa_fn;
• struct ffa_features_desc features[FFA_FEATURE_DESC_CNT];

+};

+#endif diff --git a/drivers/arm-ffa/core.c b/drivers/arm-ffa/core.c new file mode 100644 index 0000000000..09e4eb753a --- /dev/null +++ b/drivers/arm-ffa/core.c @@ -0,0 +1,1349 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • (C) Copyright 2022 ARM Limited
• • Abdellatif El Khlifi abdellatif.elkhlifi@arm.com
• */

+#include "arm_ffa_prv.h" +#include <asm/global_data.h> +#include <asm/io.h> +#include <common.h> +#include <dm.h> +#include <dm/device-internal.h> +#include <dm/root.h> +#include <linux/errno.h> +#include <linux/sizes.h> +#include <log.h> +#include <malloc.h> +#include <mapmem.h> +#include <string.h>

+DECLARE_GLOBAL_DATA_PTR;

+/**

• • The device private data structure containing all the resident
• • data read from secure world
• */

+struct ffa_prvdata __ffa_runtime_data ffa_priv_data = {0};

+/*

• • Driver functions
• */

+/**

• • ffa_get_device - create, bind and probe the arm_ffa device
• • This boot time function makes sure the arm_ffa device is
• • created, bound to this driver, probed and ready to use.
• • Arm FF-A transport is implemented through a single u-boot
• • device managing the FF-A bus (arm_ffa).
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+int ffa_get_device(void) +{

• int ret;
• if (ffa_priv_data.dev)

• return FFA_ERR_STAT_SUCCESS;
  

• ret = device_bind(dm_root(),

• 	  DM_DRIVER_GET(arm_ffa),
  

• 	  FFA_DRV_NAME,
  

• 	  NULL,
  

• 	  ofnode_null(),
  

• 	  &ffa_priv_data.dev);
  

• if (ret) {

• ffa_priv_data.dev = NULL;
  

• return ret;
  

• }
• /* The FF-A bus discovery succeeds when probing is successful */
• ret = device_probe(ffa_priv_data.dev);
• if (ret) {

• ffa_err("can not probe  the device");
  

• device_unbind(ffa_priv_data.dev);
  

• ffa_priv_data.dev = NULL;
  

• return ret;
  

• }
• return FFA_ERR_STAT_SUCCESS;

+}

+/**

• • ffa_get_version - FFA_VERSION handler function
• • This is the boot time function that implements FFA_VERSION FF-A function
• • to get from the secure world the FF-A framework version
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_get_version(void) +{

• u16 major, minor;
• FFA_DECLARE_ARGS;
• if (!ffa_priv_data.invoke_ffa_fn)

• panic("[FFA] no private data found\n");
  

• a0 = FFA_VERSION;
• a1 = FFA_VERSION_1_0;
• ffa_priv_data.invoke_ffa_fn(a0, a1, a2, a3, a4, a5, a6, a7, &res);
• if (res.a0 == FFA_ERR_STAT_NOT_SUPPORTED) {

• ffa_err("A Firmware Framework implementation does not exist");
  

• return -EOPNOTSUPP;
  

• }
• major = GET_FFA_MAJOR_VERSION(res.a0);
• minor = GET_FFA_MINOR_VERSION(res.a0);
• ffa_info("FF-A driver %d.%d\nFF-A framework %d.%d",

•  FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, minor);
  

• if ((major == FFA_MAJOR_VERSION && minor >= FFA_MINOR_VERSION)) {

• ffa_info("Versions are compatible ");
  

• ffa_priv_data.fwk_version = res.a0;
  

• return FFA_ERR_STAT_SUCCESS;
  

• }
• ffa_info("Versions are incompatible ");
• return -EPROTONOSUPPORT;

+}

+/**

• • ffa_get_endpoint_id - FFA_ID_GET handler function
• • This is the boot time function that implements FFA_ID_GET FF-A function
• • to get from the secure world u-boot endpoint ID
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_get_endpoint_id(void) +{

• FFA_DECLARE_ARGS;
• if (!ffa_priv_data.invoke_ffa_fn)

• panic("[FFA] no private data found\n");
  

• a0 = FFA_ID_GET;
• ffa_priv_data.invoke_ffa_fn(a0, a1, a2, a3, a4, a5, a6, a7, &res);
• switch (res.a0) {
• case FFA_ERROR:
• {

• if (((int)res.a2) == FFA_ERR_STAT_NOT_SUPPORTED) {
  

• 	ffa_err("This function is not implemented at this FF-A instance");
  

• 	return -EOPNOTSUPP;
  

• }
  

• ffa_err("Undefined error code (%d)", ((int)res.a2));
  

• return -EINVAL;
  

• }
• case FFA_SUCCESS:
• {

• ffa_priv_data.id = GET_SELF_ENDPOINT_ID(res.a2);
  

• ffa_info("endpoint ID is %u", ffa_priv_data.id);
  

• return FFA_ERR_STAT_SUCCESS;
  

• }
• default:
• {

• ffa_err("Undefined response function (0x%lx)", res.a0);
  

• return -EINVAL;
  

• }
• }

+}

+/**

• • ffa_get_features_desc - returns the features descriptor of the specified

• • 				FF-A function
    

• • @func_id: the FF-A function which the features are to be retrieved
• • This is a boot time function that searches the features descriptor of the
• • specified FF-A function
• • Return:
• • When found, the address of the features descriptor is returned. Otherwise, NULL.
• */

+static struct ffa_features_desc *ffa_get_features_desc(u32 func_id) +{

• u32 desc_idx;
• /*

• * search for the descriptor of the selected FF-A interface
  

• */
  

• for (desc_idx = 0; desc_idx < FFA_FEATURE_DESC_CNT ; desc_idx++)

• if (ffa_priv_data.features[desc_idx].func_id == func_id)
  

• 	return &ffa_priv_data.features[desc_idx];
  

• return NULL;

+}

+/**

• • ffa_get_rxtx_map_features - FFA_FEATURES handler function with FFA_RXTX_MAP

• • 					argument
    

• • This is the boot time function that implements FFA_FEATURES FF-A function
• • to retrieve the FFA_RXTX_MAP features
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_get_rxtx_map_features(void) +{

• FFA_DECLARE_ARGS;
• if (!ffa_priv_data.invoke_ffa_fn)

• panic("[FFA] no private data found\n");
  

• a0 = FFA_FEATURES;
• a1 = FFA_RXTX_MAP;
• ffa_priv_data.invoke_ffa_fn(a0, a1, a2, a3, a4, a5, a6, a7, &res);
• switch (res.a0) {
• case FFA_ERROR:
• {

• if (((int)res.a2) == FFA_ERR_STAT_NOT_SUPPORTED) {
  

• 	ffa_err("FFA_RXTX_MAP is not implemented at this FF-A instance");
  

• 	return -EOPNOTSUPP;
  

• }
  

• ffa_err("Undefined error code (%d)", ((int)res.a2));
  

• return -EINVAL;
  

This happens quite a few times throughout the code. Why is -EINVAL used here instead of -FFA_ERR_STAT_INVALID_PARAMETERS for example?

We should either use FFA_* errors consistently of get rid of those entirely and stick to standard ones, but let's not mix and match

• }
• case FFA_SUCCESS:
• {

• u32 desc_idx;
  

• /*
  

•  * search for an empty descriptor
  

•  */
  

• for (desc_idx = 0; desc_idx < FFA_FEATURE_DESC_CNT ; desc_idx++)
  

• 	if (!ffa_priv_data.features[desc_idx].func_id) {
  

• 		/*
  

• 		 * populate the descriptor with
  

• 		 * the interface features data
  

• 		 */
  

• 		ffa_priv_data.features[desc_idx].func_id =
  

• 			FFA_RXTX_MAP;
  

• 		ffa_priv_data.features[desc_idx].field1 =
  

• 			res.a2;
  

• 		ffa_info("FFA_RXTX_MAP features data 0x%lx",
  

• 			 res.a2);
  

• 		return FFA_ERR_STAT_SUCCESS;
  

• 	}
  

• ffa_err("Cannot save FFA_RXTX_MAP features data. Descriptors table full");
  

• return -ENOBUFS;
  

Similarly -FFA_ERR_STAT_NO_MEMORY or something?

• }
• default:
• {

• ffa_err("Undefined response function (0x%lx)",
  

• 	res.a0);
  

• return -EINVAL;
  

• }
• }

+}

+/**

• • ffa_get_rxtx_buffers_pages_cnt - reads from the features data descriptors

• • 				the minimum number of pages in each of the RX/TX
    

• • 				buffers
    

• • @buf_4k_pages: Pointer to the minimum number of pages
• • This is the boot time function that returns the minimum number of pages
• • in each of the RX/TX buffers
• • Return:
• • buf_4k_pages points to the returned number of pages
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_get_rxtx_buffers_pages_cnt(size_t *buf_4k_pages) +{

• struct ffa_features_desc *desc = NULL;
• if (!buf_4k_pages)

• return -EINVAL;
  

• desc = ffa_get_features_desc(FFA_RXTX_MAP);
• if (!desc)

• return -EINVAL;
  

• switch (desc->field1) {
• case RXTX_4K:

• *buf_4k_pages = 1;
  

• break;
  

• case RXTX_16K:

• *buf_4k_pages = 4;
  

• break;
  

• case RXTX_64K:

• *buf_4k_pages = 16;
  

• break;
  

• default:

• ffa_err("RX/TX buffer size not supported");
  

• return -EINVAL;
  

• }
• return FFA_ERR_STAT_SUCCESS;

+}

+/**

• • ffa_free_rxtx_buffers - frees the RX/TX buffers
• • @buf_4k_pages: the minimum number of pages in each of the RX/TX

• • 	  buffers
    

• • This is the boot time function used to free the RX/TX buffers
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_free_rxtx_buffers(size_t buf_4k_pages) +{

• efi_status_t free_rxbuf_ret, free_txbuf_ret;
• ffa_info("Freeing RX/TX buffers");
• free_rxbuf_ret = efi_free_pages(ffa_priv_data.pair.rxbuf, buf_4k_pages);
• free_txbuf_ret = efi_free_pages(ffa_priv_data.pair.txbuf, buf_4k_pages);
• if (free_rxbuf_ret != EFI_SUCCESS || free_txbuf_ret != EFI_SUCCESS) {

• ffa_err("Failed to free RX/TX buffers (rx: %lu , tx: %lu)",
  

• 	free_rxbuf_ret,
  

• 	free_txbuf_ret);
  

• return -EINVAL;
  

• }
• ffa_priv_data.pair.rxbuf = 0;
• ffa_priv_data.pair.txbuf = 0;

Should those be set to 0 regardless of the efi_free_pages() outcome? If not then you probably need to handle those one by one. As is it right now one failure to free a buffer means both of those won't be set

• return FFA_ERR_STAT_SUCCESS;

+}

+/**

• • ffa_alloc_rxtx_buffers - allocates the RX/TX buffers
• • @buf_4k_pages: the minimum number of pages in each of the RX/TX

• • 	  buffers
    

• • This is the boot time function used by ffa_map_rxtx_buffers to allocate
• • the RX/TX buffers before mapping them
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_alloc_rxtx_buffers(size_t buf_4k_pages) +{ +#if CONFIG_IS_ENABLED(EFI_LOADER)

• efi_status_t efi_ret;
• void *virt_txbuf;
• void *virt_rxbuf;
• ffa_info("Using %lu 4KB page(s) for RX/TX buffers size",

•  buf_4k_pages);
  

• efi_ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,

• 		     EFI_BOOT_SERVICES_DATA,
  

• 		     buf_4k_pages,
  

• 		     &ffa_priv_data.pair.rxbuf);
  

Why are we specifically limiting this to EFI? I understand using efi allocated memory for potential runtime operations, were we need the memory preserved, but why here?

There's also a side effect with this allocation. The EFI subsystem doesn't usually come up until u-boot is fully up and running. This call will force it to start way earlier

• if (efi_ret != EFI_SUCCESS) {

• ffa_priv_data.pair.rxbuf = 0;
  

• ffa_err("Failure to allocate RX buffer (EFI error: 0x%lx)",
  

• 	efi_ret);
  

• return -ENOBUFS;
  

• }
• ffa_info("RX buffer at virtual address 0x%llx",

•  ffa_priv_data.pair.rxbuf);
  

• virt_rxbuf = (void *)ffa_priv_data.pair.rxbuf;
• /*

• * make sure the buffer is clean before use
  

• */
  

• memset(virt_rxbuf, 0, buf_4k_pages * SZ_4K);
• efi_ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,

• 		     EFI_RUNTIME_SERVICES_DATA,
  

• 		     buf_4k_pages,
  

• 		     &ffa_priv_data.pair.txbuf);
  

• if (efi_ret != EFI_SUCCESS) {

• efi_free_pages(ffa_priv_data.pair.rxbuf, buf_4k_pages);
  

• ffa_priv_data.pair.rxbuf = 0;
  

• ffa_priv_data.pair.txbuf = 0;
  

• ffa_err("Failure to allocate the TX buffer (EFI error: 0x%lx)"
  

• 	, efi_ret);
  

• return -ENOBUFS;
  

• }
• ffa_info("TX buffer at virtual address 0x%llx",

•  ffa_priv_data.pair.txbuf);
  

• virt_txbuf = (void *)ffa_priv_data.pair.txbuf;
• /*

• * make sure the buffer is clean before use
  

• */
  

• memset(virt_txbuf, 0, buf_4k_pages * SZ_4K);
• return FFA_ERR_STAT_SUCCESS;

+#else

• return -ENOBUFS;

+#endif +}

+/**

• • ffa_map_rxtx_buffers - FFA_RXTX_MAP handler function
• • @buf_4k_pages: the minimum number of pages in each of the RX/TX

• • 	  buffers
    

• • This is the boot time function that implements FFA_RXTX_MAP FF-A function
• • to map the RX/TX buffers
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_map_rxtx_buffers(size_t buf_4k_pages) +{

• int ret;
• FFA_DECLARE_ARGS;
• if (!ffa_priv_data.invoke_ffa_fn)

• panic("[FFA] no private data found\n");
  

• ret = ffa_alloc_rxtx_buffers(buf_4k_pages);
• if (ret != FFA_ERR_STAT_SUCCESS)

• return ret;
  

• a0 = FFA_RXTX_MAP;
• a1 = ffa_priv_data.pair.txbuf;
• a2 = ffa_priv_data.pair.rxbuf;
• a3 = buf_4k_pages;
• ffa_priv_data.invoke_ffa_fn(a0, a1, a2, a3, a4, a5, a6, a7, &res);
• switch (res.a0) {
• case FFA_ERROR:
• {

• switch (((int)res.a2)) {
  

• case FFA_ERR_STAT_INVALID_PARAMETERS:
  

• 	ffa_err("One or more fields in input parameters is incorrectly encoded");
  

• 	ret = -EPERM;
  

• 	break;
  

• case FFA_ERR_STAT_NO_MEMORY:
  

• 	ffa_err("Not enough memory");
  

• 	ret = -ENOMEM;
  

• 	break;
  

• case FFA_ERR_STAT_DENIED:
  

• 	ffa_err("Buffer pair already registered");
  

• 	ret = -EACCES;
  

• 	break;
  

• case FFA_ERR_STAT_NOT_SUPPORTED:
  

• 	ffa_err("This function is not implemented at this FF-A instance");
  

• 	ret = -EOPNOTSUPP;
  

• 	break;
  

• default:
  

• 	ffa_err("Undefined error (%d)",
  

• 		((int)res.a2));
  

• 	ret = -EINVAL;
  

• }
  

Can we have an array with string literals for the errors and do a lookup instead of nested case switches?

• break;
  

• }
• case FFA_SUCCESS:

• ffa_info("RX/TX buffers mapped");
  

• return FFA_ERR_STAT_SUCCESS;
  

• default:

• ffa_err("Undefined response function (0x%lx)",
  

• 	res.a0);
  

• ret = -EINVAL;
  

• }
• ffa_free_rxtx_buffers(buf_4k_pages);
• return ret;

+}

+/**

• • ffa_unmap_rxtx_buffers - FFA_RXTX_UNMAP handler function
• • This is the boot time function that implements FFA_RXTX_UNMAP FF-A function
• • to unmap the RX/TX buffers
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_unmap_rxtx_buffers(void) +{

• FFA_DECLARE_ARGS;
• if (!ffa_priv_data.invoke_ffa_fn)

• panic("[FFA] no private data found\n");
  

• a0 = FFA_RXTX_UNMAP;
• a1 = PREP_SELF_ENDPOINT_ID(ffa_priv_data.id);
• ffa_priv_data.invoke_ffa_fn(a0, a1, a2, a3, a4, a5, a6, a7, &res);
• switch (res.a0) {
• case FFA_ERROR:
• {

• if (((int)res.a2) == FFA_ERR_STAT_NOT_SUPPORTED)
  

• 	panic("[FFA] FFA_RXTX_UNMAP is not implemented at this FF-A instance\n");
  

• else if (((int)res.a2) == FFA_ERR_STAT_INVALID_PARAMETERS)
  

• 	panic("[FFA] There is no buffer pair registered on behalf of the caller\n");
  

• else
  

• 	panic("[FFA] Undefined error (%d)\n", ((int)res.a2));
  

There's panics sprinkled around the code. Are all those cases really fatal?

• }
• case FFA_SUCCESS:
• {

• size_t buf_4k_pages = 0;
  

• int ret;
  

• ret = ffa_get_rxtx_buffers_pages_cnt(&buf_4k_pages);
  

• if (ret != FFA_ERR_STAT_SUCCESS)
  

• 	panic("[FFA] RX/TX buffers unmapped but failure in getting pages count\n");
  

• ret = ffa_free_rxtx_buffers(buf_4k_pages);
  

• if (ret != FFA_ERR_STAT_SUCCESS)
  

• 	panic("[FFA] RX/TX buffers unmapped but failure in freeing the memory\n");
  

• ffa_info("RX/TX buffers unmapped and memory freed");
  

• return FFA_ERR_STAT_SUCCESS;
  

• }
• default:

• panic("[FFA] Undefined response function (0x%lx)", res.a0);
  

• }

+}

+/**

• • ffa_release_rx_buffer - FFA_RX_RELEASE handler function
• • This is the boot time function that invokes FFA_RX_RELEASE FF-A function
• • to release the ownership of the RX buffer
• • Return:
• • FFA_ERR_STAT_SUCCESS on success. Otherwise, failure
• */

+static int ffa_release_rx_buffer(void) +{

• FFA_DECLARE_ARGS;
• if (!ffa_priv_data.invoke_ffa_fn)

• panic("[FFA] no private data found\n");
  

• a0 = FFA_RX_RELEASE;
• ffa_priv_data.invoke_ffa_fn(a0, a1, a2, a3, a4, a5, a6, a7, &res);
• switch (res.a0) {
• case FFA_ERROR:
• {

• if (((int)res.a2) == FFA_ERR_STAT_NOT_SUPPORTED)
  

• 	panic("[FFA] FFA_RX_RELEASE is not implemented at this FF-A instance\n");
  

• else if (((int)res.a2) == FFA_ERR_STAT_DENIED)
  

• 	panic("[FFA] Caller did not have ownership of the RX buffer\n");
  

• else
  

• 	panic("[FFA] Undefined error (%d)\n", ((int)res.a2));
  

• }
• case FFA_SUCCESS:

• return FFA_ERR_STAT_SUCCESS;
  

• default:

• panic("[FFA] Undefined response function (0x%lx)\n", res.a0);
  

• }

+}

+/**

• • ffa_uuid_are_identical - checks whether two given UUIDs are identical
• • @uuid1: first UUID
• • @uuid2: second UUID
• • This is a boot time function used by ffa_read_partitions_info to search
• • for a UUID in the partitions descriptors table
• • Return:
• • 1 when UUIDs match. Otherwise, 0
• */

+int ffa_uuid_are_identical(const union ffa_partition_uuid *uuid1,

• 	   const union ffa_partition_uuid *uuid2)
  

+{

• if (!uuid1 || !uuid2)

• return 0;
  

• return (!memcmp(uuid1, uuid2, sizeof(union ffa_partition_uuid)));

+}

+/**

• • ffa_read_partitions_info - reads the data queried by FFA_PARTITION_INFO_GET

• • 					and saves it in the private structure
    

• • @count: The number of partitions queried
• • @part_uuid: Pointer to the partition(s) UUID
• • This is the boot time function that reads the partitions information
• • returned by the FFA_PARTITION_INFO_GET and saves it in the private
• • data structure.
• • Return:
• • The private data structure is updated with the partition(s) information
• • FFA_ERR_STAT_SUCCESS is returned on success. Otherwise, failure
• */

+static int ffa_read_partitions_info(u32 count, union ffa_partition_uuid *part_uuid) +{

• if (!count) {

• ffa_err("No partition detected");
  

• return -ENODATA;
  

• }
• ffa_info("Reading partitions data from the RX buffer");

+#if CONFIG_IS_ENABLED(EFI_LOADER)

• if (!part_uuid) {

• /*
  

•  * querying information of all partitions
  

•  */
  

• u64 data_pages;
  

• u64 data_bytes;
  

• efi_status_t efi_ret;
  

• size_t buf_4k_pages = 0;
  

• u32 desc_idx;
  

• struct ffa_partition_info *parts_info;
  

• int ret;
  

• data_bytes = count * sizeof(struct ffa_partition_desc);
  

• data_pages = efi_size_in_pages(data_bytes);
  

• /*
  

•  * get the RX buffer size in pages
  

•  */
  

• ret = ffa_get_rxtx_buffers_pages_cnt(&buf_4k_pages);
  

• if (ret != FFA_ERR_STAT_SUCCESS) {
  

• 	ffa_err("Can not get the RX buffer size (error %d)", ret);
  

• 	return ret;
  

• }
  

• if (data_pages > buf_4k_pages) {
  

• 	ffa_err("Partitions data size exceeds the RX buffer size:");
  

• 	ffa_err("    Sizes in pages: data %llu , RX buffer %lu ",
  

• 		data_pages,
  

• 		buf_4k_pages);
  

• 	return -ENOMEM;
  

• }
  

• efi_ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
  

• 			     EFI_RUNTIME_SERVICES_DATA,
  

• 			     data_pages,
  

• 			     (u64 *)&ffa_priv_data.partitions.descs);
  

Same questions here. Why are we using the EFI APIs to allocate buffers?

• if (efi_ret != EFI_SUCCESS) {
  

• 	ffa_priv_data.partitions.descs = NULL;
  

• 	ffa_err("Cannot  allocate partitions data buffer (EFI error 0x%lx)",
  

• 		efi_ret);
  

• 	return -ENOBUFS;
  

• }
  

• /*
  

•  * make sure the buffer is clean before use
  

•  */
  

• memset(ffa_priv_data.partitions.descs, 0,
  

•        data_pages * SZ_4K);
  

• parts_info = (struct ffa_partition_info *)ffa_priv_data.pair.rxbuf;
  

• for (desc_idx = 0 ; desc_idx < count ; desc_idx++) {
  

• 	ffa_priv_data.partitions.descs[desc_idx].info =
  

• 		parts_info[desc_idx];
  

• 	ffa_info("Partition ID %x : info cached",
  

• 		 ffa_priv_data.partitions.descs[desc_idx].info.id);
  

• }
  

• ffa_priv_data.partitions.count = count;
  

• ffa_info("%d partition(s) found and cached", count);
  

• } else {

• u32 rx_desc_idx, cached_desc_idx;
  

• struct ffa_partition_info *parts_info;
  

• u8 desc_found;
  

• parts_info = (struct ffa_partition_info *)ffa_priv_data.pair.rxbuf;
  

• /*
  

•  * search for the SP IDs read from the RX buffer
  

•  * in the already cached SPs.
  

•  * Update the UUID when ID found.
  

•  */
  

• for (rx_desc_idx = 0; rx_desc_idx < count ; rx_desc_idx++) {
  

• 	desc_found = 0;
  

• 	/*
  

• 	 * search the current ID in the cached partitions
  

• 	 */
  

• 	for (cached_desc_idx = 0;
  

• 	     cached_desc_idx < ffa_priv_data.partitions.count;
  

• 	     cached_desc_idx++) {
  

• 		/*
  

• 		 * save the UUID
  

• 		 */
  

• 		if (ffa_priv_data.partitions.descs[cached_desc_idx].info.id ==
  

• 		    parts_info[rx_desc_idx].id) {
  

• 			ffa_priv_data.partitions.descs[cached_desc_idx].UUID =
  

• 				*part_uuid;
  

• 			desc_found = 1;
  

• 			break;
  

• 		}
  

• 	}
  

• 	if (!desc_found)
  

• 		return -ENODATA;
  

• }
  

• }

+#else +#warning "arm_ffa: reading FFA_PARTITION_INFO_GET data not implemented" +#endif

• return FFA_ERR_STAT_SUCCESS;

+}

+/**

• • ffa_query_partitions_info - invokes FFA_PARTITION_INFO_GET

• • 					and saves partitions data
    

• • @part_uuid: Pointer to the partition(s) UUID
• • @pcount: Pointer to the number of partitions variable filled when querying
• • This is the boot time function that executes the FFA_PARTITION_INFO_GET
• • to query the partitions data. Then, it calls ffa_read_partitions_info
• • to save the data in the private data structure.
• • After reading the data the RX buffer is released using ffa_release_rx_buffer
• • Return:
• • When part_uuid is NULL, all partitions data are retrieved from secure world
• • When part_uuid is non NULL, data for partitions matching the given UUID are
• • retrieved and the number of partitions is returned
• • FFA_ERR_STAT_SUCCESS is returned on success. Otherwise, failure
• */

+static int ffa_query_partitions_info(union ffa_partition_uuid *part_uuid,

• 		     u32 *pcount)
  

+{

• unsigned long a0 = 0;
• union ffa_partition_uuid query_uuid = {0};
• unsigned long a5 = 0;
• unsigned long a6 = 0;
• unsigned long a7 = 0;
• struct arm_smccc_res res = {0};
• if (!ffa_priv_data.invoke_ffa_fn)

• panic("[FFA] no private data found\n");
  

• a0 = FFA_PARTITION_INFO_GET;
• /*

• * If a UUID is specified. Information for one or more
  

• * partitions in the system is queried. Otherwise, information
  

• * for all installed partitions is queried
  

• */
  

• if (part_uuid) {

• if (!pcount)
  

• 	return -EINVAL;
  

• query_uuid = *part_uuid;
  

• }
• ffa_priv_data.invoke_ffa_fn(a0, query_uuid.words.a1, query_uuid.words.a2,

• 		    query_uuid.words.a3, query_uuid.words.a4,
  

• 		    a5, a6, a7, &res);
  

• switch (res.a0) {
• case FFA_ERROR:
• {

• switch (((int)res.a2)) {
  

• case FFA_ERR_STAT_INVALID_PARAMETERS:
  

• 	ffa_err("Unrecognized UUID");
  

• 	return -EPERM;
  

• case FFA_ERR_STAT_NO_MEMORY:
  

• 	ffa_err("Results cannot fit in RX buffer of the caller");
  

• 	return -ENOMEM;
  

• case FFA_ERR_STAT_DENIED:
  

• 	ffa_err("Callee is not in a state to handle this request");
  

• 	return -EACCES;
  

• case FFA_ERR_STAT_NOT_SUPPORTED:
  

• 	ffa_err("This function is not implemented at this FF-A instance");
  

• 	return -EOPNOTSUPP;
  

• case FFA_ERR_STAT_BUSY:
  

• 	ffa_err("RX buffer of the caller is not free");
  

• 	return -EBUSY;
  

• default:
  

• 	ffa_err("Undefined error (%d)", ((int)res.a2));
  

• 	return -EINVAL;
  

• }
  

• }

Same cases as above. Please map those in an array or something and just do a lookup to print an error

• case FFA_SUCCESS:
• {

• int ret;
  

• /*
  

•  * res.a2 contains the count of partition information descriptors
  

•  * populated in the RX buffer
  

•  */
  

• if (res.a2) {
  

• 	ret = ffa_read_partitions_info(res.a2, part_uuid);
  

• 	if (ret)
  

• 		ffa_err("Failed to read partition(s) data , error (%d)", ret);
  

• }
  

• /*
  

•  * return the SP count
  

•  */
  

• if (part_uuid) {
  

• 	if (!ret)
  

• 		*pcount = res.a2;
  

• 	else
  

• 		*pcount = 0;
  

• }
  

If I am following the code correctly this can be called with (NULL, NULL), which means that the previous !pcount check won't apply. Is there any other check I am missing?

• /*
  

•  * After calling FFA_PARTITION_INFO_GET the buffer ownership
  

•  * is assigned to the consumer (u-boot). So, we need to give
  

•  * the ownership back to the secure world
  

•  */
  

• ret = ffa_release_rx_buffer();
  

• if (!part_uuid && !res.a2) {
  

• 	ffa_err("[FFA] no partition installed in the system");
  

• 	return -ENODEV;
  

• }
  

• return ret;
  

• }
• default:

• ffa_err("Undefined response function (0x%lx)", res.a0);
  

• return  -EINVAL;
  

• }

+}

+/**

[...]

Regards /Ilias