Re: [PATCH 1/2] efi: add non-volatile parameter to grub_efi_set_variable

[Glenn Washburn]

Hi Flavio,

I saw this while searching for some old patches and I think something
like this patch series would be a good addition to GRUB.

On Wed, 29 Apr 2020 16:54:09 +0200
Flavio Suligoi <f.suligoi@asem.it> wrote:

> The current version of the function:
> 
> grub_efi_set_variable
> 
> always set an EFI variable as "non-volatile", since the
> variable attribute:
> 
> GRUB_EFI_VARIABLE_NON_VOLATILE
> 
> is fixed in the EFI SetVariable() call.

This is strange line breaking. How about just getting rid of the line
break in this sentence.

> 
> With this change it is possible to decide if the written
> EFI variable is volatile or not.
> 
> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> ---
>  grub-core/commands/efi/efifwsetup.c |  2 +-
>  grub-core/kern/efi/efi.c            | 15 +++++++++------
>  include/grub/efi/efi.h              |  3 ++-
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/grub-core/commands/efi/efifwsetup.c 
> b/grub-core/commands/efi/efifwsetup.c
> index 7a137a7..4ccb578 100644
> --- a/grub-core/commands/efi/efifwsetup.c
> +++ b/grub-core/commands/efi/efifwsetup.c
> @@ -45,7 +45,7 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ 
> ((unused)),
>      os_indications |= *old_os_indications;
>  
>    status = grub_efi_set_variable ("OsIndications", &global, &os_indications,
> -                               sizeof (os_indications));
> +                               sizeof (os_indications), 1);
>    if (status != GRUB_ERR_NONE)
>      return status;
>  
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 3a708ed..dcb09b2 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -193,12 +193,15 @@ grub_efi_set_virtual_address_map (grub_efi_uintn_t 
> memory_map_size,
>  
>  grub_err_t
>  grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,
> -                   void *data, grub_size_t datasize)
> +                   void *data, grub_size_t datasize,
> +                      grub_efi_boolean_t non_volatile)

I think this parameter would be better as "grub_uint64_t attrib". Let
the caller specify what attributes are desired.

>  {
>    grub_efi_status_t status;
>    grub_efi_runtime_services_t *r;
>    grub_efi_char16_t *var16;
>    grub_size_t len, len16;
> +  grub_uint64_t efi_attrib = GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +                             GRUB_EFI_VARIABLE_RUNTIME_ACCESS;
>  
>    len = grub_strlen (var);
>    len16 = len * GRUB_MAX_UTF16_PER_UTF8;
> @@ -210,11 +213,11 @@ grub_efi_set_variable(const char *var, const 
> grub_efi_guid_t *guid,
>  
>    r = grub_efi_system_table->runtime_services;
>  
> -  status = efi_call_5 (r->set_variable, var16, guid, 
> -                    (GRUB_EFI_VARIABLE_NON_VOLATILE
> -                     | GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS
> -                     | GRUB_EFI_VARIABLE_RUNTIME_ACCESS),
> -                    datasize, data);
> +  if (non_volatile)
> +    efi_attrib |= GRUB_EFI_VARIABLE_NON_VOLATILE;
> +
> +  status = efi_call_5 (r->set_variable, var16, guid, efi_attrib,
> +                       datasize, data);
>    grub_free (var16);
>    if (status == GRUB_EFI_SUCCESS)
>      return GRUB_ERR_NONE;
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index e90e00d..9b04150 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -81,7 +81,8 @@ grub_err_t
>  EXPORT_FUNC (grub_efi_set_variable) (const char *var,
>                                    const grub_efi_guid_t *guid,
>                                    void *data,
> -                                  grub_size_t datasize);
> +                                  grub_size_t datasize,
> +                                     grub_efi_boolean_t non_volatile);

This should use tabs like the lines above it.

Glenn

>  int
>  EXPORT_FUNC (grub_efi_compare_device_paths) (const grub_efi_device_path_t 
> *dp1,
>                                            const grub_efi_device_path_t *dp2);