Hi Harsimran, On Tue, 5 May 2026 at 08:30, Harsimran Singh Tungal <[email protected]> wrote: > > On 2026-04-28 12:16 -0600, Simon Glass wrote: > > Hi Harsimran, > > > > On 2026-04-24T17:31:50, Harsimran Singh Tungal > > <[email protected]> wrote: > > > efi_loader: enable EFI runtime SetVariable()/GetVariable() using FF-A > > > transport > > > > > > Route EFI runtime variable APIs through FF-A MM communication > > > > > > This patch implements full EFI Runtime Variable Services (GetVariable, > > > SetVariable, GetNextVariableName, and QueryVariableInfo) using the > > > FF-A/MM communication backend. Once ExitBootServices() has been invoked, > > > all variable operations now use the runtime-safe FF-A transport instead > > > of the boot-time communication paths. > > > > > > Key changes: > > > ============ > > > > > > - Add runtime-safe variable property handling via FF-A MM SP. > > > - Add runtime versions of variable access and property operations. > > > - Replace all standard memory operations with EFI-runtime-safe variants. > > > > > > Signed-off-by: Harsimran Singh Tungal <[email protected]> > > > > > > lib/charset.c | 2 +- > > > lib/efi_loader/efi_variable_tee.c | 322 > > > +++++++++++++++++++++++++++++++++++++- > > > 2 files changed, 318 insertions(+), 6 deletions(-) > > > > > diff --git a/lib/efi_loader/efi_variable_tee.c > > > b/lib/efi_loader/efi_variable_tee.c > > > @@ -991,6 +1066,92 @@ out: > > > + /* > > > + * UEFI > 2.7 needs the attributes set even if the buffer is > > > + * smaller > > > + */ > > > + if (attributes) { > > > + tmp = get_property_int_runtime(variable_name, name_size, > > > vendor, > > > + &var_property); > > > + if (tmp != EFI_SUCCESS) { > > > + ret = tmp; > > > + return ret; > > > + } > > > + *attributes = var_acc->attr; > > > + if (var_property.property & > > > + VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY) > > > + *attributes |= EFI_VARIABLE_READ_ONLY; > > > + } > > > > get_property_int_runtime() ends up calling setup_mm_hdr() -> > > get_comm_buf(), and at runtime get_comm_buf() does > > efi_memset_runtime(ffa_shared_buf, 0, CONFIG_FFA_SHARED_MM_BUF_SIZE) > > on the same shared buffer that var_acc points into. > > > > By the time you read var_acc->attr it has been zeroed, so > > '*attributes' will only ever come back as 0 (optionally OR'd with > > EFI_VARIABLE_READ_ONLY). The boot-time path gets away with this > > because get_property_int() calloc()s a fresh buffer; the runtime path > > cannot. Please cache var_acc->attr in a local before the call, or move > > the get_property_int_runtime() call ahead of the SP exchange. > > > Agreed, you're right. > > `get_property_int_runtime()` reuses the same runtime FF-A shared buffer, so > the second call can clear the buffer backing `var_acc` before `var_acc->attr` > is copied out. > > For v2, I will cache `var_acc->attr` in a local before calling > `get_property_int_runtime()` and then used that cached value when updating > `*attributes`. > > Thanks > > > > diff --git a/lib/efi_loader/efi_variable_tee.c > > > b/lib/efi_loader/efi_variable_tee.c > > > @@ -1228,7 +1477,70 @@ efi_set_variable_runtime(u16 *variable_name, const > > > efi_guid_t *guid, > > > + ro = !!(attributes & EFI_VARIABLE_READ_ONLY); > > > + attributes &= EFI_VARIABLE_MASK; > > > + > > > + ret = get_property_int_runtime(variable_name, name_size, guid, > > > + &var_property); > > > + if (ret != EFI_SUCCESS) > > > + return ret; > > > + > > > + /* > > > + * Allocate the buffer early, before switching to RW (if needed) > > > + * so we won't need to account for any failures in reading/setting > > > + * the properties, if the allocation fails > > > + */ > > > + comm_buf = setup_mm_hdr((void **)&var_acc, payload_size, > > > + SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret); > > > + if (!comm_buf) > > > + return ret; > > > + > > > + if (var_property.property & VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY) > > > + return EFI_WRITE_PROTECTED; > > > > The comment is copied from efi_set_variable_int() and no longer > > matches the code - there is no RW switch in the runtime path (no > > ro_check argument), and the allocation happens after the property > > read, the opposite of what the comment claims. Either drop the comment > > or, better, do the RO check before calling setup_mm_hdr() so the > > early-return is quicker. > > > > Agreed. > > That comment was copied from the boot-time path. > For v2, I will drop it and move the `VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY` > check ahead of `setup_mm_hdr()`, so the runtime path returns > `EFI_WRITE_PROTECTED` before allocating the MM communication buffer. > > > > diff --git a/lib/efi_loader/efi_variable_tee.c > > > b/lib/efi_loader/efi_variable_tee.c > > > @@ -859,6 +859,38 @@ out: > > > +static efi_status_t __efi_runtime set_property_int_runtime(const u16 > > > *variable_name, > > > + efi_uintn_t > > > name_size, > > > + const efi_guid_t > > > *vendor, > > > + struct > > > var_check_property *var_property) > > > > As mentioned elsewhere, the four new functions > > (set_property_int_runtime, get_property_int_runtime, > > efi_get_variable_runtime, efi_get_next_variable_name_runtime) are > > near-line-for-line copies of their boot-time counterparts with > > memcpy/guidcpy/memset swapped for the efi_*_runtime helpers and a > > different mm_communicate(). Since setup_mm_hdr() was made runtime-safe > > in patch 3, can the boot-time helpers be reused directly (with > > mm_communicate() picking the right transport based on > > efi_is_runtime_enabled())? It would also avoid the special-case free() > > path. > > > > I kept the separate runtime wrappers intentionally so the > post-ExitBootServices call graph stays explicit and auditable: only > runtime-safe helpers, no driver-model assumptions, and no boot-time heap > cleanup semantics. > > I also tried to match the existing EFI variable structure: `efi_variable.c` > (Done by Heinrich Schuchardt) already has separate boot-time and runtime > service entry points, so I kept the same split here instead of introducing a > different pattern only for the TEE/FF-A backend. > > Apart from these runtime variants, I will collapse the other `_runtime` > helpers > into the common implementations in v2 patchset. `get_mm_comms()`, > `mm_communicate()`, > `ffa_mm_communicate()`, and `ffa_notify_mm_sp()` will handle both boot and > runtime, > with only a small `efi_at_runtime()` branch for the phase-specific parts. > > Does this explanation sound reasonable to you?
Yes that makes sense, thanks. > > Thanks > > > > diff --git a/lib/charset.c b/lib/charset.c > > > @@ -407,7 +407,7 @@ size_t __efi_runtime u16_strnlen(const u16 *in, > > > size_t count) > > > -size_t u16_strsize(const void *in) > > > +size_t __efi_runtime u16_strsize(const void *in) > > > > This change should really be in its own patch with a one-line note > > that it's needed by the new runtime callers. It is unrelated to the > > SetVariable/GetVariable wiring and is easy to lose in this 320-line > > diff. > > > > For v2, I'll split the `u16_strsize()` `__efi_runtime` annotation into a > separate small patch with a short note that it is needed by > the new runtime variable callers. That should keep this patch focused on > the SetVariable/GetVariable runtime wiring. > > > > Key changes: > > > ============ > > > > > > > > - Add runtime-safe variable property handling via FF-A MM SP. > > > - Add runtime versions of variable access and property operations. > > > - Replace all standard memory operations with EFI-runtime-safe variants. > > > > The'============' underline is more for rST documentation than commit > > messages - please drop it. The third bullet is also misleading: only > > the new runtime functions use the runtime-safe variants; the existing > > boot-time helpers are unchanged. It is generally better to use prose > > than bullet points in a commit message. > > > > Sure, I will update this in v2. Regards, Simon
