[PATCH v2 03/11] efi_loader: add FF-A runtime support in EFI variable TEE driver

[Simon Glass]

Hi Harsimran,

On 2026-05-14T12:49:13, Harsimran Singh Tungal
<harsimransingh.tungal at arm.com> wrote:
> efi_loader: add FF-A runtime support in EFI variable TEE driver
>
> Enable MM variable services over FF-A after ExitBootServices
>
> Extend lib/efi_loader/efi_variable_tee.c to support FF-A
> communication with the secure world during EFI runtime. Reuse the
> statically reserved FF-A shared buffer after ExitBootServices(),
> make the MM communication path runtime-safe so runtime variable
> operations continue to reach the secure partition.
>
> Share the MM communication and MM SP notification helpers between the
> boot and runtime paths instead of maintaining separate runtime-only
> variants. Select dynamic allocation during boot and the fixed FF-A
> shared buffer at runtime, and reject requests that would exceed the
> shared buffer size.
>
> Mark the required code and data with __efi_runtime and
> __efi_runtime_data, use range-based cache maintenance on the shared
> buffer for the runtime FF-A path, and add the shared buffer to the EFI
> runtime memory map. Document the FF-A shared MM buffer
> [...]
>
> arch/arm/cpu/armv8/cache.S        |   8 +
>  arch/arm/cpu/armv8/cache_v8.c     |  13 +-
>  lib/efi_loader/Kconfig            |   4 +
>  lib/efi_loader/efi_variable_tee.c | 330 +++++++++++++++++++++++++++-----------
>  4 files changed, 260 insertions(+), 95 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -34,20 +36,44 @@
> +static const int __efi_runtime_rodata mm_sp_errmap[] = {
> +     [-MM_NOT_SUPPORTED]      = -EINVAL,
> +     [-MM_INVALID_PARAMETER]  = -EPERM,
> +     [-MM_DENIED]             = -EACCES,
> +     [-MM_NO_MEMORY]          = -EBUSY,
> +};

Hard to read. The MM_* constants are negative, so the designators
silently become positive indices and the array ends up sparse (slot 4
is implicitly zero, since MM_NO_MEMORY is -5). ffa_map_sp_event() then
uses mm_sp_errmap[idx] as a truthy check to distinguish known error
from uninitialised gap, which only works because every assigned value
happens to be non-zero. Please either pack this as a switch (matching
the original) or store {mm_code, errno} pairs; otherwise add a comment
spelling out the convention.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -304,30 +341,61 @@ static efi_status_t ffa_mm_communicate(void *comm_buf, ulong comm_buf_size)
> +     if (IS_ENABLED(CONFIG_ARM64)) {
> +             BUILD_BUG_ON(CONFIG_FFA_SHARED_MM_BUF_ADDR %
> +                          CONFIG_SYS_CACHELINE_SIZE);
> +             BUILD_BUG_ON(CONFIG_FFA_SHARED_MM_BUF_SIZE %
> +                          CONFIG_SYS_CACHELINE_SIZE);
> +             flush_dcache_range((unsigned long)shared_buf,
> +                                (unsigned long)(shared_buf +
> +                                        CONFIG_FFA_SHARED_MM_BUF_SIZE));
> +     }

The commit message says the BUILD_BUG_ON() checks are added "in the
arm64 cache-maintenance path", but they live in ffa_mm_communicate() -
please reword.

Second, both the flush and the matching invalidate below cover the
entire CONFIG_FFA_SHARED_MM_BUF_SIZE regardless of tx_data_size. At
boot the input is only tx_data_size bytes and on the response path we
already trust message_len. Using tx_data_size (request) and
rx_data_size (response) rounded up to a cacheline would avoid flushing
megabytes on every call.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -1010,5 +1137,26 @@ efi_status_t efi_init_variables(void)
> +     if (IS_ENABLED(CONFIG_ARM_FFA_RT_MODE)) {
> +             /*
> +              * The FF-A shared buffer is accessed by EFI runtime services, so it
> +              * must be marked as runtime memory in the EFI memory map.
> +              *
> +              * CONFIG_FFA_SHARED_MM_BUF_ADDR is expected to be EFI-page aligned.
> +              */
> +             ffa_shared_buf = (void *)CONFIG_FFA_SHARED_MM_BUF_ADDR;

This path requires the OS to leave the region identity-mapped, right?
Worth a comment. You could add a BUILD_BUG_ON() to check that it is
EFI-page-aligned.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -434,8 +511,55 @@ static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize)
> +static __efi_runtime u8 *get_comm_buf(efi_uintn_t payload_size)
> +{
> +     efi_uintn_t comm_buf_size;
> +     u8 *comm_buf;
> +
> +     comm_buf_size = MM_COMMUNICATE_HEADER_SIZE +
> +                     MM_VARIABLE_COMMUNICATE_SIZE +
> +                     payload_size;
> +
> +     /*
> +      * After ExitBootServices(), dynamic allocation is no longer permitted.
> +      * Use the predefined FF-A shared buffer at runtime; otherwise allocate
> +      * a fresh buffer during the boot phase.
> +      */
> +     if (efi_at_runtime()) {
> +             if (IS_ENABLED(CONFIG_ARM_FFA_RT_MODE)) {
> +                     if (comm_buf_size > CONFIG_FFA_SHARED_MM_BUF_SIZE)
> +                             return NULL;
> +                     comm_buf = ffa_shared_buf;
> +                     if (!comm_buf)
> +                             return NULL;
> +                     efi_memset_runtime(comm_buf, 0, CONFIG_FFA_SHARED_MM_BUF_SIZE);

Implicit contract here that bites later: at runtime this returns the
static shared buffer, so callers must NOT free() it. The boot-time
setup_mm_hdr() callers all do free(comm_buf), and the runtime variants
in patch 4 carefully don't. Please either document the ownership
contract in the kerneldoc, or expose a paired release helper so
callers don't need to know which phase they are in.

Also, memset'ing the full CONFIG_FFA_SHARED_MM_BUF_SIZE for every
runtime request is wasteful; zeroing comm_buf_size would be enough.

Regards,
Simon