Message ID | 20200331060739.4570-1-xypron.glpk@gmx.de |
---|---|
State | Superseded, archived |
Delegated to: | Heinrich Schuchardt |
Headers | show |
Series | efi_loader: non-volatile and runtime variables | expand |
On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote: > As variable services are available at runtime we have to expect EFI_SUCCESS > when calling the services. Why not run variables test *after* ExitBootServices as well as *before* that event? Then we should have more test cases. > Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and > EFI_VARIABLE_NON_VOLATILE set. I'm not sure if this claim(check) is true. At least, you need provide more description about specific test conditions. -Takahiro Akashi > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > .../efi_selftest_variables_runtime.c | 47 +++++++++++++++---- > 1 file changed, 39 insertions(+), 8 deletions(-) > > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c > index b3b40ad2cf..c6005eeeaf 100644 > --- a/lib/efi_selftest/efi_selftest_variables_runtime.c > +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c > @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 = > EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1, > 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6); > > -/* > - * Setup unit test. > +/** > + * setup() - set up unit test. > * > * @handle handle of the loaded image > * @systable system table > @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle, > /** > * execute() - execute unit test > * > - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned. > + * Test variable services at runtime. > */ > static int execute(void) > { > @@ -52,37 +52,68 @@ static int execute(void) > efi_guid_t guid; > u64 max_storage, rem_storage, max_size; > > - ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS, > + ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS | > + EFI_VARIABLE_NON_VOLATILE, > &max_storage, &rem_storage, > &max_size); > - if (ret != EFI_UNSUPPORTED) { > + if (ret != EFI_SUCCESS) { > efi_st_error("QueryVariableInfo failed\n"); > return EFI_ST_FAILURE; > } > > + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_NON_VOLATILE, > + 3, v + 4); > + if (ret != EFI_INVALID_PARAMETER) { > + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n"); > + return EFI_ST_FAILURE; > + } > + > ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, > EFI_VARIABLE_BOOTSERVICE_ACCESS | > EFI_VARIABLE_RUNTIME_ACCESS, > 3, v + 4); > - if (ret != EFI_UNSUPPORTED) { > + if (ret != EFI_INVALID_PARAMETER) { > + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n"); > + return EFI_ST_FAILURE; > + } > + > + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS | > + EFI_VARIABLE_NON_VOLATILE, > + 3, v + 4); > + if (ret != EFI_SUCCESS) { > efi_st_error("SetVariable failed\n"); > return EFI_ST_FAILURE; > } > + > len = 3; > ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0, > &attr, &len, data); > - if (ret != EFI_UNSUPPORTED) { > + if (ret != EFI_SUCCESS) { > efi_st_error("GetVariable failed\n"); > return EFI_ST_FAILURE; > } > + > memset(&guid, 0, 16); > *varname = 0; > + len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16); > ret = runtime->get_next_variable_name(&len, varname, &guid); > - if (ret != EFI_UNSUPPORTED) { > + if (ret != EFI_SUCCESS) { > efi_st_error("GetNextVariableName failed\n"); > return EFI_ST_FAILURE; > } > > + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0, > + 3, v + 4); > + if (ret != EFI_SUCCESS) { > + efi_st_error("Variable deletion failed\n"); > + return EFI_ST_FAILURE; > + } > + > return EFI_ST_SUCCESS; > } > > -- > 2.25.1 >
On 4/1/20 3:05 AM, AKASHI Takahiro wrote: > On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote: >> As variable services are available at runtime we have to expect EFI_SUCCESS >> when calling the services. > > Why not run variables test *after* ExitBootServices as well as > *before* that event? We have a separate test for before ExitBootServices (lib/efi_selftest/efi_selftest_variables.c). > > Then we should have more test cases. Please, feel free to supply some. Unfortunately SCT does not provide runtime testing. I have been running my code successfully against Ubuntu's firmware test suite (https://git.launchpad.net/fwts). > >> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and >> EFI_VARIABLE_NON_VOLATILE set. > > I'm not sure if this claim(check) is true. > At least, you need provide more description about specific test conditions. At runtime non-volatile variables and variables without runtime access cannot be changed according to the UEFI spec. Best regards Heinrich > > -Takahiro Akashi > >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> .../efi_selftest_variables_runtime.c | 47 +++++++++++++++---- >> 1 file changed, 39 insertions(+), 8 deletions(-) >> >> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c >> index b3b40ad2cf..c6005eeeaf 100644 >> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c >> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c >> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 = >> EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1, >> 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6); >> >> -/* >> - * Setup unit test. >> +/** >> + * setup() - set up unit test. >> * >> * @handle handle of the loaded image >> * @systable system table >> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle, >> /** >> * execute() - execute unit test >> * >> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned. >> + * Test variable services at runtime. >> */ >> static int execute(void) >> { >> @@ -52,37 +52,68 @@ static int execute(void) >> efi_guid_t guid; >> u64 max_storage, rem_storage, max_size; >> >> - ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS, >> + ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS | >> + EFI_VARIABLE_RUNTIME_ACCESS | >> + EFI_VARIABLE_NON_VOLATILE, >> &max_storage, &rem_storage, >> &max_size); >> - if (ret != EFI_UNSUPPORTED) { >> + if (ret != EFI_SUCCESS) { >> efi_st_error("QueryVariableInfo failed\n"); >> return EFI_ST_FAILURE; >> } >> >> + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, >> + EFI_VARIABLE_BOOTSERVICE_ACCESS | >> + EFI_VARIABLE_NON_VOLATILE, >> + 3, v + 4); >> + if (ret != EFI_INVALID_PARAMETER) { >> + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n"); >> + return EFI_ST_FAILURE; >> + } >> + >> ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, >> EFI_VARIABLE_BOOTSERVICE_ACCESS | >> EFI_VARIABLE_RUNTIME_ACCESS, >> 3, v + 4); >> - if (ret != EFI_UNSUPPORTED) { >> + if (ret != EFI_INVALID_PARAMETER) { >> + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n"); >> + return EFI_ST_FAILURE; >> + } >> + >> + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, >> + EFI_VARIABLE_BOOTSERVICE_ACCESS | >> + EFI_VARIABLE_RUNTIME_ACCESS | >> + EFI_VARIABLE_NON_VOLATILE, >> + 3, v + 4); >> + if (ret != EFI_SUCCESS) { >> efi_st_error("SetVariable failed\n"); >> return EFI_ST_FAILURE; >> } >> + >> len = 3; >> ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0, >> &attr, &len, data); >> - if (ret != EFI_UNSUPPORTED) { >> + if (ret != EFI_SUCCESS) { >> efi_st_error("GetVariable failed\n"); >> return EFI_ST_FAILURE; >> } >> + >> memset(&guid, 0, 16); >> *varname = 0; >> + len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16); >> ret = runtime->get_next_variable_name(&len, varname, &guid); >> - if (ret != EFI_UNSUPPORTED) { >> + if (ret != EFI_SUCCESS) { >> efi_st_error("GetNextVariableName failed\n"); >> return EFI_ST_FAILURE; >> } >> >> + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0, >> + 3, v + 4); >> + if (ret != EFI_SUCCESS) { >> + efi_st_error("Variable deletion failed\n"); >> + return EFI_ST_FAILURE; >> + } >> + >> return EFI_ST_SUCCESS; >> } >> >> -- >> 2.25.1 >>
On Wed, Apr 01, 2020 at 08:37:38AM +0200, Heinrich Schuchardt wrote: > On 4/1/20 3:05 AM, AKASHI Takahiro wrote: > > On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote: > >> As variable services are available at runtime we have to expect EFI_SUCCESS > >> when calling the services. > > > > Why not run variables test *after* ExitBootServices as well as > > *before* that event? > > We have a separate test for before ExitBootServices > (lib/efi_selftest/efi_selftest_variables.c). Ah, I misundersttood the file to be patched. (EXECUTE_BEFORE_BOOTTIME_EXIT and SETUP_BEFORE_BOOTTIME_EXIT are quite confusing.) > > > > Then we should have more test cases. > > Please, feel free to supply some. ? You re-invented UEFI variable implementation almost from the scratch, then you should cover as many corner cases as possible in general. -Takahiro Akashi > Unfortunately SCT does not provide runtime testing. I have been running > my code successfully against Ubuntu's firmware test suite > (https://git.launchpad.net/fwts). > > > > >> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and > >> EFI_VARIABLE_NON_VOLATILE set. > > > > I'm not sure if this claim(check) is true. > > At least, you need provide more description about specific test conditions. > > At runtime non-volatile variables and variables without runtime access > cannot be changed according to the UEFI spec. > Best regards > > Heinrich > > > > > -Takahiro Akashi > > > >> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >> --- > >> .../efi_selftest_variables_runtime.c | 47 +++++++++++++++---- > >> 1 file changed, 39 insertions(+), 8 deletions(-) > >> > >> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c > >> index b3b40ad2cf..c6005eeeaf 100644 > >> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c > >> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c > >> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 = > >> EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1, > >> 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6); > >> > >> -/* > >> - * Setup unit test. > >> +/** > >> + * setup() - set up unit test. > >> * > >> * @handle handle of the loaded image > >> * @systable system table > >> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle, > >> /** > >> * execute() - execute unit test > >> * > >> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned. > >> + * Test variable services at runtime. > >> */ > >> static int execute(void) > >> { > >> @@ -52,37 +52,68 @@ static int execute(void) > >> efi_guid_t guid; > >> u64 max_storage, rem_storage, max_size; > >> > >> - ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS, > >> + ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS | > >> + EFI_VARIABLE_RUNTIME_ACCESS | > >> + EFI_VARIABLE_NON_VOLATILE, > >> &max_storage, &rem_storage, > >> &max_size); > >> - if (ret != EFI_UNSUPPORTED) { > >> + if (ret != EFI_SUCCESS) { > >> efi_st_error("QueryVariableInfo failed\n"); > >> return EFI_ST_FAILURE; > >> } > >> > >> + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, > >> + EFI_VARIABLE_BOOTSERVICE_ACCESS | > >> + EFI_VARIABLE_NON_VOLATILE, > >> + 3, v + 4); > >> + if (ret != EFI_INVALID_PARAMETER) { > >> + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n"); > >> + return EFI_ST_FAILURE; > >> + } > >> + > >> ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, > >> EFI_VARIABLE_BOOTSERVICE_ACCESS | > >> EFI_VARIABLE_RUNTIME_ACCESS, > >> 3, v + 4); > >> - if (ret != EFI_UNSUPPORTED) { > >> + if (ret != EFI_INVALID_PARAMETER) { > >> + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n"); > >> + return EFI_ST_FAILURE; > >> + } > >> + > >> + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, > >> + EFI_VARIABLE_BOOTSERVICE_ACCESS | > >> + EFI_VARIABLE_RUNTIME_ACCESS | > >> + EFI_VARIABLE_NON_VOLATILE, > >> + 3, v + 4); > >> + if (ret != EFI_SUCCESS) { > >> efi_st_error("SetVariable failed\n"); > >> return EFI_ST_FAILURE; > >> } > >> + > >> len = 3; > >> ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0, > >> &attr, &len, data); > >> - if (ret != EFI_UNSUPPORTED) { > >> + if (ret != EFI_SUCCESS) { > >> efi_st_error("GetVariable failed\n"); > >> return EFI_ST_FAILURE; > >> } > >> + > >> memset(&guid, 0, 16); > >> *varname = 0; > >> + len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16); > >> ret = runtime->get_next_variable_name(&len, varname, &guid); > >> - if (ret != EFI_UNSUPPORTED) { > >> + if (ret != EFI_SUCCESS) { > >> efi_st_error("GetNextVariableName failed\n"); > >> return EFI_ST_FAILURE; > >> } > >> > >> + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0, > >> + 3, v + 4); > >> + if (ret != EFI_SUCCESS) { > >> + efi_st_error("Variable deletion failed\n"); > >> + return EFI_ST_FAILURE; > >> + } > >> + > >> return EFI_ST_SUCCESS; > >> } > >> > >> -- > >> 2.25.1 > >> >
diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c index b3b40ad2cf..c6005eeeaf 100644 --- a/lib/efi_selftest/efi_selftest_variables_runtime.c +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 = EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1, 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6); -/* - * Setup unit test. +/** + * setup() - set up unit test. * * @handle handle of the loaded image * @systable system table @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle, /** * execute() - execute unit test * - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned. + * Test variable services at runtime. */ static int execute(void) { @@ -52,37 +52,68 @@ static int execute(void) efi_guid_t guid; u64 max_storage, rem_storage, max_size; - ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS, + ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS | + EFI_VARIABLE_NON_VOLATILE, &max_storage, &rem_storage, &max_size); - if (ret != EFI_UNSUPPORTED) { + if (ret != EFI_SUCCESS) { efi_st_error("QueryVariableInfo failed\n"); return EFI_ST_FAILURE; } + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_NON_VOLATILE, + 3, v + 4); + if (ret != EFI_INVALID_PARAMETER) { + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n"); + return EFI_ST_FAILURE; + } + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, 3, v + 4); - if (ret != EFI_UNSUPPORTED) { + if (ret != EFI_INVALID_PARAMETER) { + efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n"); + return EFI_ST_FAILURE; + } + + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS | + EFI_VARIABLE_NON_VOLATILE, + 3, v + 4); + if (ret != EFI_SUCCESS) { efi_st_error("SetVariable failed\n"); return EFI_ST_FAILURE; } + len = 3; ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0, &attr, &len, data); - if (ret != EFI_UNSUPPORTED) { + if (ret != EFI_SUCCESS) { efi_st_error("GetVariable failed\n"); return EFI_ST_FAILURE; } + memset(&guid, 0, 16); *varname = 0; + len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16); ret = runtime->get_next_variable_name(&len, varname, &guid); - if (ret != EFI_UNSUPPORTED) { + if (ret != EFI_SUCCESS) { efi_st_error("GetNextVariableName failed\n"); return EFI_ST_FAILURE; } + ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0, + 3, v + 4); + if (ret != EFI_SUCCESS) { + efi_st_error("Variable deletion failed\n"); + return EFI_ST_FAILURE; + } + return EFI_ST_SUCCESS; }
As variable services are available at runtime we have to expect EFI_SUCCESS when calling the services. Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE set. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- .../efi_selftest_variables_runtime.c | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) -- 2.25.1