Hi Harsimran, On Wed, 6 May 2026 at 09:14, Harsimran Singh Tungal <[email protected]> wrote: > > On 2026-04-28 12:04 -0600, Simon Glass wrote: > > Hi Harsimran, > > > > On 2026-04-24T17:31:50, Harsimran Singh Tungal > > <[email protected]> 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 <[email protected]> > > > > > > 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?
That seems reasonable to me, yes. This is a little tricky :-) I have talked to Heinrich about moving these tests to the normal ut framework, but I have not tried to actually do it yet. In any case it should not affect your work here. [..] Regards, Simon
