[PATCH v8 03/10] arm_ffa: introduce Arm FF-A low-level driver
Simon Glass
sjg at chromium.org
Wed Nov 23 03:09:16 CET 2022
should be called 'priov' and should beHi Abdellatif,
On Tue, 22 Nov 2022 at 06:18, Abdellatif El Khlifi
<abdellatif.elkhlifi at 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].
>
> [1]: https://developer.arm.com/documentation/den0077/latest/
> [2]: doc/arch/arm64.ffa.rst
>
> Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Cc: Jens Wiklander <jens.wiklander at 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 at 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 at 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=style#include-files
[..]
> +/*
> + * 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 at 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
More information about the U-Boot
mailing list