Hi Harsimran, On 2026-05-14T12:49:13, Harsimran Singh Tungal <[email protected]> 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 <[email protected]> > 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
