should be called 'priov' and should beHi Abdellatif,

On Tue, 22 Nov 2022 at 06:18, Abdellatif El Khlifi abdellatif.elkhlifi@arm.com wrote:

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

The Firmware Framework for Arm A-profile processors (FF-A v1.0) [1] describes interfaces (ABIs) that standardize communication between the Secure World and Normal World leveraging TrustZone technology.

This driver uses 64-bit registers as per SMCCCv1.2 spec and comes on top of the SMCCC layer. The driver provides the FF-A ABIs needed for querying the FF-A framework from the secure world.

The driver uses SMC32 calling convention which means using the first 32-bit data of the Xn registers.

All supported ABIs come with their 32-bit version except FFA_RXTX_MAP which has 64-bit version supported.

Both 32-bit and 64-bit direct messaging are supported which allows both 32-bit and 64-bit clients to use the FF-A bus.

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 exports its operations to be used by upper layers.

Exported operations:

• partition_info_get
• sync_send_receive
• rxtx_unmap

For more details please refer to the driver documentation [2].

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

---

Changelog:

v8:

• make ffa_get_partitions_info() second argument to be an SP count in both modes
• update ffa_bus_prvdata_get() to return a pointer rather than a pointer address
• remove packing from ffa_partition_info and ffa_send_direct_data structures
• pass the FF-A bus device to the bus operations

v7:

• add support for 32-bit direct messaging
• rename be_uuid_str_to_le_bin() to uuid_str_to_le_bin()
• improve the declaration of error handling mapping
• stating in doc/arch/arm64.ffa.rst that EFI runtime is not supported

v6:

• drop use of EFI runtime support (We decided with Linaro to add this later)
• drop discovery from initcalls (discovery will be on demand by FF-A users)
• set the alignment of the RX/TX buffers to the larger translation granule size
• move FF-A RX/TX buffers unmapping at ExitBootServices() to a separate commit
• update the documentation and move it to doc/arch/arm64.ffa.rst

v4:

• add doc/README.ffa.drv
• moving the FF-A driver work to drivers/firmware/arm-ffa
• use less #ifdefs in lib/efi_loader/efi_boottime.c and replace #if defined by #if CONFIG_IS_ENABLED
• improving error handling by mapping the FF-A errors to standard errors and logs
• replacing panics with an error log and returning an error code
• improving features discovery in FFA_FEATURES by introducing rxtx_min_pages private data field
• add ffa_remove and ffa_unbind functions
• improve how the driver behaves when bus discovery is done more than once

v3:

• align the interfaces of the U-Boot FF-A driver with those in the linux FF-A driver
• remove the FF-A helper layer
• make the U-Boot FF-A driver independent from EFI
• provide an optional config that enables copying the driver data to EFI runtime section at ExitBootServices service
• use 64-bit version of FFA_RXTX_MAP, FFA_MSG_SEND_DIRECT_{REQ, RESP}

v2:

• make FF-A bus discoverable using device_{bind, probe} APIs
• remove device tree support

v1:

• introduce FF-A bus driver with device tree support

MAINTAINERS | 7 + doc/arch/arm64.ffa.rst | 218 ++++ doc/arch/index.rst | 1 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/firmware/arm-ffa/Kconfig | 30 + drivers/firmware/arm-ffa/Makefile | 6 + drivers/firmware/arm-ffa/arm-ffa-uclass.c | 16 + drivers/firmware/arm-ffa/arm_ffa_prv.h | 200 ++++ drivers/firmware/arm-ffa/core.c | 1315 +++++++++++++++++++++ include/arm_ffa.h | 97 ++ include/dm/uclass-id.h | 4 + 12 files changed, 1897 insertions(+) create mode 100644 doc/arch/arm64.ffa.rst create mode 100644 drivers/firmware/arm-ffa/Kconfig create mode 100644 drivers/firmware/arm-ffa/Makefile create mode 100644 drivers/firmware/arm-ffa/arm-ffa-uclass.c create mode 100644 drivers/firmware/arm-ffa/arm_ffa_prv.h create mode 100644 drivers/firmware/arm-ffa/core.c create mode 100644 include/arm_ffa.h

This looks mostly OK to me. I have a few comments below. [..]

diff --git a/drivers/firmware/arm-ffa/arm_ffa_prv.h b/drivers/firmware/arm-ffa/arm_ffa_prv.h new file mode 100644 index 0000000000..4eea7dc036 --- /dev/null +++ b/drivers/firmware/arm-ffa/arm_ffa_prv.h @@ -0,0 +1,200 @@ +/* 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 <linux/arm-smccc.h>

+/*

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

/* ...*/

is the single-line comment style

+/* 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)))
  

+/*

• • Definitions of the Arm FF-A interfaces supported by the Arm FF-A driver
• */

+#define FFA_SMC(calling_convention, func_num) \

•   ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, (calling_convention),   \
  

•                      ARM_SMCCC_OWNER_STANDARD, (func_num))
  

+#define FFA_SMC_32(func_num) FFA_SMC(ARM_SMCCC_SMC_32, (func_num)) +#define FFA_SMC_64(func_num) FFA_SMC(ARM_SMCCC_SMC_64, (func_num))

+enum ffa_abis {

•   FFA_ERROR                 = 0x60,
  

•   FFA_SUCCESS               = 0x61,
  

•   FFA_INTERRUPT             = 0x62,
  

•   FFA_VERSION               = 0x63,
  

•   FFA_FEATURES              = 0x64,
  

•   FFA_RX_RELEASE            = 0x65,
  

•   FFA_RXTX_MAP              = 0x66,
  

•   FFA_RXTX_UNMAP            = 0x67,
  

•   FFA_PARTITION_INFO_GET    = 0x68,
  

•   FFA_ID_GET                = 0x69,
  

•   FFA_RUN                   = 0x6D,
  

•   FFA_MSG_SEND_DIRECT_REQ   = 0x6F,
  

Can you use lower-case hex consistently?

•   FFA_MSG_SEND_DIRECT_RESP  = 0x70,
  

•   /* to be updated when adding new FFA IDs */
  

•   FFA_FIRST_ID              = FFA_ERROR, /* lowest number ID*/
  

•   FFA_LAST_ID               = FFA_MSG_SEND_DIRECT_RESP, /* highest number ID*/
  

not: spaces before */

+};

+enum ffa_abi_errcode {

•   NOT_SUPPORTED = 1,
  

•   INVALID_PARAMETERS,
  

•   NO_MEMORY,
  

•   BUSY,
  

•   INTERRUPTED,
  

•   DENIED,
  

•   RETRY,
  

•   ABORTED,
  

•   MAX_NUMBER_FFA_ERR
  

+};

+/* container structure and helper macros to map between an FF-A error and relevant error log */ +struct ffa_abi_errmap {

•   char *err_str[MAX_NUMBER_FFA_ERR];
  

+};

+#define FFA_ERRMAP_COUNT (FFA_LAST_ID - FFA_FIRST_ID + 1) +#define FFA_ID_TO_ERRMAP_ID(ffa_id) ((ffa_id) - FFA_FIRST_ID)

+/* The FF-A SMC function definitions */

+typedef struct arm_smccc_1_2_regs ffa_value_t; +typedef void (*invoke_ffa_fn_t)(ffa_value_t args, ffa_value_t *res);

that needs a comment

[..]

+/**

• • 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
• • @rxtx_min_pages: RX/TX buffers minimum size in pages
• • Data structure hosting the virtual addresses of the mapped RX/TX buffers

Just a nit but it catches my eye. We know this is a struct. You can just say "Hosts the ..."

[..]

diff --git a/drivers/firmware/arm-ffa/core.c b/drivers/firmware/arm-ffa/core.c new file mode 100644 index 0000000000..0b1f8e6a07 --- /dev/null +++ b/drivers/firmware/arm-ffa/core.c @@ -0,0 +1,1315 @@ +// 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 <common.h> +#include <dm.h> +#include <dm/device-internal.h> +#include <dm/devres.h> +#include <dm/root.h> +#include <linux/errno.h> +#include <linux/sizes.h> +#include <log.h> +#include <malloc.h> +#include <string.h> +#include <uuid.h>

See this:

https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html?highlight=s...

[..]

+/*

• • Driver core functions
• */

+/**

• • ffa_remove_device - removes the arm_ffa device
• • @dev: the device to be removed
• • This function makes sure the arm_ffa device is removed
• • No need to free the kmalloced data when the device is destroyed.
• • It's automatically done by devm management by
• • device_remove() -> device_free() -> devres_release_probe().
• • Return:
• • 0 on success. Otherwise, failure
• */

+int ffa_remove_device(struct udevice *dev) +{

•   int ret;
  

•   if (!dev) {
  

•           ffa_err("no udevice found");
  

•           return -ENODEV;
  

•   }
  

•   ret = device_remove(dev, DM_REMOVE_NORMAL);
  

•   if (ret) {
  

•           ffa_err("unable to remove. err:%d\n", ret);
  

•           return ret;
  

•   }
  

•   ffa_info("device removed and freed");
  

•   ret = device_unbind(dev);
  

•   if (ret) {
  

•           ffa_err("unable to unbind. err:%d\n", ret);
  

•           return ret;
  

•   }
  

•   ffa_info("device unbound");
  

•   return 0;
  

+}

Do we need this function? We should not be unbinding devices. Even removing them should be done elsewhere if needed. But why?

+/**

• • ffa_device_get - create, bind and probe the arm_ffa device
• • @pdev: the address of a device pointer (to be filled when the arm_ffa bus device is created

• •   successfully)
    

• • This 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:
• • 0 on success. Otherwise, failure
• */

+int ffa_device_get(struct udevice **pdev) +{

•   int ret;
  

•   struct udevice *dev = NULL;
  

•   ret = device_bind(dm_root(), DM_DRIVER_GET(arm_ffa), FFA_DRV_NAME, NULL, ofnode_null(),
  

•                     &dev);
  

Please add a DT binding. Even if only temporary, we need something for this. [..]

+static int ffa_get_version(void) +{

•   u16 major, minor;
  

•   ffa_value_t res = {0};
  

•   int ffa_errno;
  

•   ffa_priv_data->invoke_ffa_fn((ffa_value_t){
  

•                   .a0 = FFA_SMC_32(FFA_VERSION), .a1 = FFA_VERSION_1_0,
  

•                   }, &res);
  

•   ffa_errno = res.a0;
  

•   if (ffa_errno < 0) {
  

•           ffa_print_error_log(FFA_VERSION, ffa_errno);
  

•           return ffa_to_std_errno(ffa_errno);
  

•   }
  

•   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)) {
  

nit: Drop extra brackets

•           ffa_info("Versions are compatible ");
  

•           ffa_priv_data->fwk_version = res.a0;
  

•           return 0;
  

•   }
  

•   ffa_err("versions are incompatible\nExpected: %d.%d , Found: %d.%d\n",
  

•           FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, minor);
  

•   return -EPROTONOSUPPORT;
  

+}

+/**

• • ffa_get_endpoint_id - FFA_ID_GET handler function

I believe these should have () at the end, so:

ffa_get_endpoint_id() - FFA_ID_GET handler function

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

[..]

+/**

• • ffa_free_rxtx_buffers - frees the RX/TX buffers
• • This function frees the RX/TX buffers
• */

+static void ffa_free_rxtx_buffers(void) +{

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

•   if (ffa_priv_data->pair.rxbuf) {
  

•           free((void *)ffa_priv_data->pair.rxbuf);
  

You can't cast an address to a pointer in this way. Should use map_sysmem() or store a pointer.

•           ffa_priv_data->pair.rxbuf = 0;
  

•   }
  

•   if (ffa_priv_data->pair.txbuf) {
  

•           free((void *)ffa_priv_data->pair.txbuf);
  

•           ffa_priv_data->pair.txbuf = 0;
  

•   }
  

+} +[..]

•   /*
  

•    * make sure the buffers are cleared before use
  

•    */
  

•   memset((void *)ffa_priv_data->pair.rxbuf, 0, bytes);
  

•   memset((void *)ffa_priv_data->pair.txbuf, 0, bytes);
  

Yes these should be pointers everywhere, since you are casting all the time.

•   return 0;
  

+}

[..]

+/**

• • ffa_read_partitions_info - reads the data queried by FFA_PARTITION_INFO_GET

• •                                                 and saves it in the private structure
    

Can you fit all these titles on one line, like:

ffa_read_partitions_info() - Read partition information

<insert longer description here?>

• • @count: The number of partitions queried
• • @part_uuid: Pointer to the partition(s) UUID
• • This function 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
• • 0 is returned on success. Otherwise, failure
• */

[..] [..]

+static int ffa_msg_send_direct_req(struct udevice *dev, u16 dst_part_id,

•                              struct ffa_send_direct_data *msg, bool is_smc64)
  

+{

•   ffa_value_t res = {0};
  

•   int ffa_errno;
  

•   u64 req_mode, resp_mode;
  

•   if (!ffa_priv_data || !ffa_priv_data->invoke_ffa_fn)
  

•           return -EINVAL;
  

ffa_priv_data should be called 'priv' and attached to the device with device_get_priv() [..]

+/**

• • ffa_probe - The driver probe function
• • @dev: the arm_ffa device
• • Probing is done at boot time and triggered by the uclass device discovery.
• • At probe level the following actions are done:

• • - setting the conduit
    

• • - querying the FF-A framework version
    

• • - querying from secure world the u-boot endpoint ID
    

• • - querying from secure world the supported features of FFA_RXTX_MAP
    

• • - mapping the RX/TX buffers
    

• • - querying from secure world all the partitions information
    

• • All data queried from secure world is saved in the resident private data structure.
• • The probe will fail if either FF-A framework is not detected or the
• • FF-A requests are not behaving correctly. This ensures that the
• • driver is not installed and its operations are not exported to the clients.
• • Return:
• • 0 on success. Otherwise, failure
• */

+static int ffa_probe(struct udevice *dev) +{

•   int ret;
  

•   ret = ffa_alloc_prvdata(dev);
  

don't do this, see above.

[..]

diff --git a/include/arm_ffa.h b/include/arm_ffa.h new file mode 100644 index 0000000000..74b16174c2 --- /dev/null +++ b/include/arm_ffa.h @@ -0,0 +1,97 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

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

+#ifndef __ARM_FFA_H +#define __ARM_FFA_H

+#include <linux/printk.h>

+/*

• • This header is public. It can be used by clients to access
• • data structures and definitions they need
• */

+/*

• • Macros for displaying logs
• */

+#define ffa_info(fmt, ...) pr_info("[FFA] " fmt "\n", ##__VA_ARGS__) +#define ffa_err(fmt, ...) pr_err("[FFA] " fmt "\n", ##__VA_ARGS__)

You could use log_info(), log_err()

+/*

• • struct ffa_partition_info - Partition information descriptor
• • @id: Partition ID
• • @exec_ctxt: Execution context count
• • @properties: Partition properties
• • Data structure containing information about partitions instantiated in the system
• • This structure is filled with the data queried by FFA_PARTITION_INFO_GET
• */

+struct ffa_partition_info {

•   u16 id;
  

•   u16 exec_ctxt;
  

+/* partition supports receipt of direct requests */ +#define FFA_PARTITION_DIRECT_RECV BIT(0) +/* partition can send direct requests. */ +#define FFA_PARTITION_DIRECT_SEND BIT(1) +/* partition can send and receive indirect messages. */ +#define FFA_PARTITION_INDIRECT_MSG BIT(2)

•   u32 properties;
  

+};

+/*

• • struct ffa_send_direct_data - Data structure hosting the data

• •                                   used by FFA_MSG_SEND_DIRECT_{REQ,RESP}
    

• • @data0-4: Data read/written from/to x3-x7 registers
• • Data structure containing the data to be sent by FFA_MSG_SEND_DIRECT_REQ
• • or read from FFA_MSG_SEND_DIRECT_RESP
• */

+/* For use with FFA_MSG_SEND_DIRECT_{REQ,RESP} which pass data via registers */ +struct ffa_send_direct_data {

•   unsigned long data0; /* w3/x3 */
  

•   unsigned long data1; /* w4/x4 */
  

•   unsigned long data2; /* w5/x5 */
  

•   unsigned long data3; /* w6/x6 */
  

•   unsigned long data4; /* w7/x7 */
  

+};

+struct udevice;

+/**

• • struct ffa_bus_ops - The driver operations structure
• • @partition_info_get: callback for the FFA_PARTITION_INFO_GET
• • @sync_send_receive: callback for the FFA_MSG_SEND_DIRECT_REQ
• • @rxtx_unmap: callback for the FFA_RXTX_UNMAP
• • The data structure providing all the operations supported by the driver.
• • This structure is EFI runtime resident.
• */

+struct ffa_bus_ops {

•   int (*partition_info_get)(struct udevice *dev, const char *uuid_str,
  

•                             u32 *sp_count, struct ffa_partition_info *buffer);
  

•   int (*sync_send_receive)(struct udevice *dev, u16 dst_part_id,
  

•                            struct ffa_send_direct_data *msg,
  

•                            bool is_smc64);
  

•   int (*rxtx_unmap)(struct udevice *dev);
  

+};

Shouldn't this be the .ops member in your driver?

+/**

• • The device driver and the Uclass driver public functions
• */

+/**

• • ffa_bus_ops_get - driver operations getter
• */

+const struct ffa_bus_ops *ffa_bus_ops_get(void);

See how this is done in other uclasses, e.g. spi_get_ops()

Regards, SImon