On Wed, Apr 01, 2020 at 08:26:24AM +0200, Heinrich Schuchardt wrote:
> On 4/1/20 3:41 AM, AKASHI Takahiro wrote:
> > On Tue, Mar 31, 2020 at 08:05:40AM +0200, Heinrich Schuchardt wrote:
> >> Enable UEFI variables at runtime.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>
> >> ---
> >> lib/efi_loader/efi_runtime.c | 6 +++++-
> >> lib/efi_loader/efi_variable.c | 23 +++++++++++++++++------
> >> 2 files changed, 22 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >> index 664a0422e2..acd644202d 100644
> >> --- a/lib/efi_loader/efi_runtime.c
> >> +++ b/lib/efi_loader/efi_runtime.c
> >> @@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
> >> rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
> >> rt_table->length = sizeof(struct efi_rt_properties_table);
> >> rt_table->runtime_services_supported =
> >> + EFI_RT_SUPPORTED_GET_VARIABLE |
> >> + EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
> >> + EFI_RT_SUPPORTED_SET_VARIABLE |
> >> EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
> >> - EFI_RT_SUPPORTED_CONVERT_POINTER;
> >> + EFI_RT_SUPPORTED_CONVERT_POINTER |
> >> + EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
> >>
> >> /*
> >> * This value must be synced with efi_runtime_detach_list
> >> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >> index 7c39542968..cf8b44c535 100644
> >> --- a/lib/efi_loader/efi_variable.c
> >> +++ b/lib/efi_loader/efi_variable.c
> >> @@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI
> >> efi_query_variable_info(
> >> u64 *remaining_variable_storage_size,
> >> u64 *maximum_variable_size)
> >> {
> >> - /*
> >> *maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
> >> sizeof(struct efi_var_file);
> >> *remaining_variable_storage_size = efi_var_mem_free();
> >> *maximum_variable_size = EFI_VAR_BUF_SIZE -
> >> sizeof(struct efi_var_file) -
> >> sizeof(struct efi_var_entry);
> >> - */
> >> - return EFI_UNSUPPORTED;
> >> + return EFI_SUCCESS;
> >> }
> >>
> >> /**
> >> @@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
> >> efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
> >> u32 *attributes, efi_uintn_t *data_size, void *data)
> >> {
> >> - return EFI_UNSUPPORTED;
> >> + return efi_get_variable_int(variable_name, vendor, attributes,
> >> + data_size, data);
> >> }
> >>
> >> /**
> >> @@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
> >> efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >> u16 *variable_name, efi_guid_t *vendor)
> >> {
> >> - return EFI_UNSUPPORTED;
> >> + return efi_get_next_variable_name_int(variable_name_size, variable_name,
> >> + vendor);
> >> }
> >>
> >> /**
> >> @@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const
> >> efi_guid_t *vendor,
> >> u32 attributes, efi_uintn_t data_size,
> >> const void *data)
> >> {
> >> - return EFI_UNSUPPORTED;
> >> + const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
> >> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> + EFI_VARIABLE_RUNTIME_ACCESS;
> >> +
> >
> > Why not support APPEND_WRITE?
>
> This is the list of attributes that have to be set as a *minimum* in a
> valid call to SetVariable. APPEND_WRITE is not required but optional.
>
> >
> >
> >> + if (attributes &&
> >> + (attributes & required_attributes) != required_attributes)
> >> + return EFI_INVALID_PARAMETER;
> >
> > UEFI specification says,
> > ===8<====
> > Variables that have runtime access but that are not nonvolatile are read-
> > only data variables once ExitBootServices() is performed.
> > [snip]
> > EFI_WRITE_PROTECTED The variable in question is read-only.
> > ===>8===
> >
> > So in this case, we should return WRITE_PROTECTED instead of
> > INVALID_PARAMETER.
>
> Here I check the parameters of the call which may create a new variable.
If !NON_VOLATILE,
> >> + if (attributes &&
> >> + (attributes & required_attributes) != required_attributes)
> >> + return EFI_INVALID_PARAMETER;
this check will return INVALID_PARAMETER whether the variable exists
or not.
> We should later in the code check if the variable already exists and if
> it is volatile return EFI_WRITE_PROTECTED.
>
> Best regards
>
> Heinrich
>
> >
> >> + if ((attributes & ~(u32)EFI_VARIABLE_MASK))
> >> + return EFI_INVALID_PARAMETER;
> >> +
> >> + return efi_set_variable_rt_int(variable_name, vendor, attributes,
> >> + data_size, data);
> >
> > I didn't follow detailed logic of this function, but
> > does it really return UNSUPPORTED if !FAT_WRITE?
?
-Takahiro Akashi
> > -Takahiro Akashi
> >
> >> }
> >>
> >> /**
> >> --
> >> 2.25.1
> >>
>
