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

[Simon Glass]

Hi Harsimran,

On 2026-04-24T17:31:50, 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
>
> This patch extends lib/efi_loader/efi_variable_tee.c to support FF-A
> communication with the secure world during EFI runtime. It enables EFI
> runtime variable access and MM communication using FF-A transport when
> ExitBootServices() has already been called.
>
> Key changes:
>  ------------
>   - Introduce runtime-safe implementations for MM communication,
>     notification, and variable access using FF-A driver.
>   - Introduce communication-buffer helper (get_comm_buf()) that switches
>     between dynamic allocation (boot phase) and the fixed FF-A shared
>     buffer (runtime phase).
>   - Mark persistent data and code with __efi_runtime and
>     __efi_runtime_data attributes.
>   - Use direct physical address mapping for shared buffers since
>     U-Boot operates with 1:1 physical-to-virtual mapping.
> [...]
>
> lib/efi_loader/efi_variable_tee.c | 331 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 319 insertions(+), 12 deletions(-)

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -225,6 +275,35 @@ static int ffa_notify_mm_sp(void)
> +static int __efi_runtime ffa_notify_mm_sp_runtime(void)
> +{
> +     struct ffa_send_direct_data msg = {0};
> +     int ret;
> +     int sp_event_ret;
> +
> +     msg.data0 = CONFIG_FFA_SHARED_MM_BUF_OFFSET;
> +
> +     ret = ffa_sync_send_receive_runtime(mm_sp_id, &msg, 1);
> +     if (ret)
> +             return ret;
> +
> +     ret = ffa_map_sp_event_runtime(sp_event_ret);
> +     return ret;
> +}

It looks like ''sp_event_ret' is uninitialised. The boot-time twin
sets sp_event_ret = msg.data0 after the FF-A call before mapping it;
that line is missing here, so the runtime path branches on whatever
was on the stack and returns -EACCES (or worse, success) regardless of
the SP reply. Please add the assignment, and ideally a selftest
exercising an SP error return so this would have been caught.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -360,6 +439,116 @@ static efi_status_t ffa_mm_communicate(void *comm_buf, ulong comm_buf_size)
> +static efi_status_t __efi_runtime ffa_mm_communicate_runtime(void *comm_buf,
> +                                                          ulong comm_buf_size)
> +{

...
> +     if (!comm_buf)
> +             return EFI_INVALID_PARAMETER;
> +
> +     /* Discover MM partition ID at boot time */
> +     if (!mm_sp_id)
> +             return EFI_UNSUPPORTED;

The comment is copied verbatim from ffa_mm_communicate() but at
runtime we cannot discover anything - we can only check that boot-time
discovery happened. Please rewrite as e.g. 'MM partition ID must have
been discovered at boot time'.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -360,6 +439,116 @@ static efi_status_t ffa_mm_communicate(void *comm_buf, ulong comm_buf_size)
> +static efi_status_t __efi_runtime ffa_mm_communicate_runtime(void *comm_buf,
> +                                                          ulong comm_buf_size)
> +{

...
> +     switch (ffa_ret) {
> +     case 0: {
...
> +     case -EINVAL:
> +             efi_ret = EFI_DEVICE_ERROR;
> +             break;

Almost the entire body is a copy of ffa_mm_communicate(), including
the long cache-maintenance comment block which patch 12 then
duplicates again into the boot helper. Please factor the shared logic
into one helper that takes a 'runtime' bool (or function pointers for
the FF-A and notify hooks) so we don't carry three near-identical
copies. The current arrangement makes future fixes (like the missing
sp_event_ret assignment above) easy to apply to only one twin.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -386,6 +575,27 @@ static enum mm_comms_select get_mm_comms(void)
> +static enum mm_comms_select __efi_runtime get_mm_comms_runtime(void)
> +{
> +     bool ret;
> +
> +     ret = efi_is_runtime_enabled();
> +     if (!ret)
> +             return MM_COMMS_UNDEFINED;
> +
> +     return MM_COMMS_FFA;
> +}

But get_mm_comms_runtime() is only reached via the runtime services
table, which is only wired up after efi_variables_boot_exit_notify()
sets efi_runtime_enabled=true, so it always returns MM_COMMS_FFA,
right? Either drop the wrapper and call ffa_mm_communicate_runtime()
directly from mm_communicate_runtime(), or keep an explicit runtime
sanity check, but please don't dress it up as transport selection.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -433,9 +643,86 @@ 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)
> +{
> +     u8 *comm_buf;
> +
> +     /* 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_is_runtime_enabled()) {
> +             if (IS_ENABLED(CONFIG_ARM_FFA_RT_MODE)) {
> +                     comm_buf = ffa_shared_buf;
> +                     if (!comm_buf)
> +                             return NULL;
> +                     efi_memset_runtime(comm_buf, 0, CONFIG_FFA_SHARED_MM_BUF_SIZE);
> +             } else {
> +                     return NULL;
> +             }
> +     } else {

At runtime there is no validation that 'payload_size + headers' fits
inside CONFIG_FFA_SHARED_MM_BUF_SIZE - the max_buffer_size check in
setup_mm_hdr() compares against the SP-reported limit, which is not
necessarily the same as the static shared buffer size, so an oversized
payload can silently overflow the FF-A buffer when setup_mm_hdr()
writes the header and the caller fills the data. Please add an
explicit size check here, or assert that max_buffer_size <=
CONFIG_FFA_SHARED_MM_BUF_SIZE at init time.

Also: the multi-line comment uses kernel-doc style ('/*' on its own
then '*' lines) elsewhere in this file, but here the first text line
follows '/*' immediately.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -444,10 +731,9 @@ static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize)
>  /**
>   * setup_mm_hdr() -  Allocate a buffer for StandAloneMM and initialize the
> - *                   header data.
> + *                   header data. It is runtime safe.

The function no longer 'allocates' at runtime. It hands back a slot in
the static shared buffer - so the summary is misleading. Please
rewrite the description and drop the trailing 'It is runtime safe'
(already implied by the new __efi_runtime tag).

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -34,20 +35,47 @@
> +static const efi_guid_t __efi_runtime_rodata mm_var_guid_runtime =
> +     EFI_MM_VARIABLE_GUID;

Why a separate '_runtime' copy of this GUID? The original mm_var_guid
was a function-local in setup_mm_hdr(), so you can just promote it to
file scope with __efi_runtime_rodata and use it for both phases.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -993,6 +1281,25 @@ 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.
> +              */
> +             ffa_shared_buf = (void *)CONFIG_FFA_SHARED_MM_BUF_ADDR;
> +             ret = efi_add_memory_map(CONFIG_FFA_SHARED_MM_BUF_ADDR,
> +                                      CONFIG_FFA_SHARED_MM_BUF_SIZE,
> +                                      EFI_RUNTIME_SERVICES_DATA);

efi_add_memory_map() takes a size in bytes but the underlying
implementation expects a page-count argument in some call sites. Can
you please double-check that CONFIG_FFA_SHARED_MM_BUF_SIZE is
interpreted as bytes here and that the buffer base is page-aligned,
otherwise the OS will see a misdescribed runtime region. Worth a
comment stating the alignment assumption.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -34,20 +35,47 @@
> +static bool __efi_runtime_data efi_runtime_enabled;

We already have a global indicator that boot services have exited:
systab.boottime being set to NULL in efi_exit_boot_services()

Adding a second flag here (and a wrapper efi_is_runtime_enabled() over
it) means we need to keep them in sync. I'd prefer either hooking off
systab.boottime (already __efi_runtime_data) or exposing a single
global helper from efi_runtime.c that everyone uses.

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> @@ -4,7 +4,7 @@
>   *  Copyright (C) 2019 Linaro Ltd. <sughosh.ganu at linaro.org>
>   *  Copyright (C) 2019 Linaro Ltd. <ilias.apalodimas at linaro.org>
> - *  Copyright 2022-2023 Arm Limited and/or its affiliates <open-source-office at arm.com>
> + *  Copyright 2022-2023, 2026 Arm Limited and/or its affiliates <open-source-office at arm.com>

How about 2022-2026 ?

The commit message also opens with 'This patch extends ...'; please
use the imperative ('Extend lib/efi_loader/efi_variable_tee.c to ...')
and drop 'This patch'. Same for 'Key changes:' / 'The change reuses
...' - describe what the code does in present tense.

Regards,
Simon