[PATCH 07/12] efi: selftest: add runtime variable tests with non-volatile storage
Harsimran Singh Tungal
harsimransingh.tungal at arm.com
Wed May 6 17:14:25 CEST 2026
On 2026-04-28 12:04 -0600, Simon Glass wrote:
> Hi Harsimran,
>
> On 2026-04-24T17:31:50, Harsimran Singh Tungal
> <harsimransingh.tungal at arm.com> wrote:
> > efi: selftest: add runtime variable tests with non-volatile storage
> >
> > Extend runtime variable tests for persistent storage
> >
> > Previously, EFI selftesting of runtime variables was only supported
> > under CONFIG_EFI_RT_VOLATILE_STORE, which uses VarToFile to simulate
> > non-volatile variable storage. This commit adds new test cases that
> > exercise runtime variable operations for persistent storage.
> >
> > Features tested:
> > - Creation of runtime-accessible variables (set)
> > - Retrieval of runtime variables (get)
> > - Deletion using SetVariable() with size = 0
> > - Append operation using EFI_VARIABLE_APPEND_WRITE
> >
> > This improves EFI compliance validation for non-volatile runtime storage
> > scenarios and ensures proper attribute enforcement and variable
> > management.
> >
> > Signed-off-by: Harsimran Singh Tungal <harsimransingh.tungal at arm.com>
> >
> > lib/efi_selftest/efi_selftest_variables_runtime.c | 106 +++++++++++++++++++++-
> > 1 file changed, 104 insertions(+), 2 deletions(-)
>
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -39,6 +46,80 @@ static int execute(void)
> > + /* Compare the value of EFI variable if it already exists in non volatile storage */
> > + if (!IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> > + len = sizeof(v) / 2;
> > + ret = st_runtime->get_variable(u'efi_st_var0', &guid_vendor0,
> > + &attr, &len, data);
> > + if (ret == EFI_SUCCESS) {
>
> The two-phase split worries me. As written, phase 1 creates a
> non-volatile variable and immediately returns EFI_ST_SUCCESS, so a
> single 'bootefi selftest' run reports a green test even though none of
> the new assertions (append, delete, re-get) actually ran. It also
> leaves 'efi_st_var0' behind in NV storage if the user never reboots
> (or if phase 2 fails on an early assertion before the final delete).
> Could you split this into two distinct EFI_UNIT_TEST entries - say
> 'variables at runtime (setup)' and 'variables at runtime (verify)'.
> Then the user (and CI) explicitly select each phase, and the
> persistent state across the two is part of the contract rather than a
> hidden side-effect. At minimum, phase 1 needs to print a clear 'test
> incomplete, please reboot and rerun' rather than EFI_ST_SUCCESS.
>
Hi Simon, Heinrich,
I would like to confirm the design direction for this selftest change before I send v2.
The proposal is:
- Keep the existing automatic `variables at runtime` test for `CONFIG_EFI_RT_VOLATILE_STORE=y`, so the volatile-store runtime coverage stays in the current single-run form.
- For the non-volatile case, replace the previous implicit two-phase flow with two explicit on-request tests:
`variables at runtime setup`
EFI_UNIT_TEST(variables_run_setup) = {
.name = "variables at runtime setup",
.phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,
.setup = setup,
.execute = execute_setup,
.on_request = true,
};
`variables at runtime verify`
EFI_UNIT_TEST(variables_run_verify) = {
.name = "variables at runtime verify",
.phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,
.setup = setup,
.execute = execute_verify,
.on_request = true,
};
- `execute_setup` would:
run the runtime `QueryVariableInfo()` checks,
exercise create/delete of a runtime variable,
create `efi_st_var0` in non-volatile storage with the first half of the payload,
print that the system should be rebooted and `variables at runtime verify` should be run next.
- `execute_verify` would:
validate the prepared variable from `setup`,
append the remaining payload,
validate the full contents,
delete the variable,
confirm that it is gone.
- If `setup` finds an existing `efi_st_var0` already containing the expected half-payload, it would report that setup is already complete and ask the user to run `verify`.
If the existing variable does not match, it would delete the stale variable and recreate the expected setup state.
- The failure logs would also be made step-specific, so local runs and CI logs show exactly which runtime operation fails, and `verify` would print an explicit success line.
The intention is to make the reboot-dependent non-volatile flow explicit, while keeping the current volatile-store runtime test structure unchanged.
Does this design look reasonable to both of you for v2?
Regards
Harsimran Singh Tungal
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -39,6 +46,80 @@ static int execute(void)
> > + return EFI_ST_SUCCESS;
> > + } else {
> > + if (ret == EFI_NOT_FOUND) {
> > + efi_st_printf("EFI Variable efi_st_var0 not found. "
> > + "Executing First Phase\n");
> > + }
> > + }
> > + }
>
> Any return value other than EFI_SUCCESS or EFI_NOT_FOUND
> (EFI_DEVICE_ERROR, EFI_UNSUPPORTED on a platform without an FF-A
> backend, etc.) silently falls through to the volatile-store path below
> and mis-diagnoses the failure. Please fail explicitly when ret is not
> one of the two expected values.
>
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -279,7 +360,28 @@ static int execute(void)
> > } else {
> > - if (ret != EFI_UNSUPPORTED) {
> > + if (ret != EFI_SUCCESS) {
>
> This flips the contract: previously the !CONFIG_EFI_RT_VOLATILE_STORE
> branch asserted that runtime SetVariable() returns EFI_UNSUPPORTED,
> which is what every board without a runtime variable backend still
> does. After this patch any such board fails 'variables at runtime'.
> Please gate the new behaviour on something like CONFIG_EFI_MM_COMM_TEE
> (or whatever indicates a working runtime backend) and keep the
> EFI_UNSUPPORTED expectation otherwise.
>
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -39,6 +46,80 @@ static int execute(void)
> > + /* Append an existing variable */
> > + append_len = sizeof(v) - len;
> > + ret = st_runtime->set_variable(u'efi_st_var0', &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_APPEND_WRITE |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + append_len, (v + len));
>
> Not sure if it is possible to align these lines properly? Not related
> to this patch, but the EFI_VARIABLE_... prefix is really too long.
>
> Drop redundant parens around v + len
>
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -39,6 +46,80 @@ static int execute(void)
> > + if (memcmp(data, v, len)) {
> > + efi_st_error("GetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
>
> Quite a lot of separate sites in this hunk print 'GetVariable failed'
> or 'SetVariable failed' with no further context - when one trips in CI
> it's impossible to tell which assertion fired without rebuilding with
> prints. Please make each message identify the step (e.g. 'phase 2:
> append SetVariable failed', 'phase 2: post-delete GetVariable did not
> return EFI_NOT_FOUND') so a log line is enough to localise the
> failure.
>
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -3,6 +3,7 @@
> > + * Copyright (c) 2026 Arm Limited and/or its affiliates <open-source-office at arm.com>
> > + *
> > * This unit test checks the runtime services for variables after
> > * ExitBootServices():
> > @@ -22,7 +23,13 @@ static const efi_guid_t efi_rt_var_guid = U_BOOT_EFI_RT_VAR_FILE_GUID;
> > + * For EFI variables in non-volatile storage, these tests have to be executed in two phases
>
> Please use 'non-volatile' (hyphenated) consistently in the doc comment
> and in the inline comment a few lines down ('non volatile storage');
> the rest of the file already spells it that way. Same nit for the
> commit message body. The commit message also opens with 'Extend
> runtime variable tests for persistent storage' that just paraphrases
> the subject - please just drop it. 'Previously, EFI selftesting … was
> only supported …' should be present tense per U-Boot conventions ('EFI
> selftesting of runtime variables is currently only supported …').
>
> Regards,
> Simon
>
More information about the U-Boot
mailing list