Re: [PATCH RFC] Proposition of a --auto-nvram option for grub-install

[Lukasz Zemczak]

Hey Daniel!

Thank you for your feedback! I can drop the warning message, sure, I
added it only for verbosity anyway so there's no real reason for
keeping it around.

As for your second comment: generally this is what I want to achieve
by checking the return value of the efibootmgr command. Whenever
efibootmgr exits with an error code of 2 this basically means "EFI
variables are not supported on this system." - i.e. that the system
we're running on has no NVRAM (at least in efibootmgr's mind). In all
other cases, if there is any other error returned by efiboomgr, fail
the grub_install_register_efi() as we did before. Therefore if
something fails on a platform with NVRAM, we fail hard as previously.

Cheers,


On 17 April 2018 at 20:23, Daniel Kiper <address@hidden> wrote:
> On Thu, Apr 12, 2018 at 07:09:58PM +0200, Lukasz Zemczak wrote:
>> Hello everyone,
>>
>> I'm writing to this list since I would like to get some feedback on an
>> additional option to the grub-install tool we would find very
>> convenient to have. The diff is attached to the e-mail (also available
>> as a pastebin [1]).
>>
>> The idea of the --auto-nvram (the name is just a proposition) flag is
>> a bit similar to what --no-nvram does. After providing the option
>> during grub-install, the tool will attempt to guess if there is access
>> to NVRAM variables for EFI and/or IEEE1275 and, if yes, perform the
>> usual variable updates. If no access to the NVRAM is available the
>> whole thing is handled somewhat similar to --no-nvram + a warning
>
> Make sense for me but I am not sure about "a warning". Hmmm...
> Is it really needed?
>
>> message displayed. Rationale:
>>
>> We would like to use this in Ubuntu for cases of dual BIOS/EFI
>> bootloaders installed (at the same time), helpful for the situation of
>> calling grub-install --target=x86_64-efi from the shim-efi package on
>> a BIOS legacy-mode booted machine. For this legacy-mode case when
>> running on a EFI-enabled device, currently this causes grub-install to
>> fail as obviously there is no access to the NVRAM and no --no-nvram is
>> given. We don't want to unconditionally append --no-nvram as this is
>> not what we want to happen for the case of a system that is actually
>> booted in EFI-mode. With this flag, we would be simply performing a
>> grub-install --target=x86_64-efi --auto-nvram unconditionally which
>> would do the right thing in both cases, allowing for a much simpler
>> handling of this dual-bootloader case in Ubuntu. Having it being done
>> inside grub-installer makes everything much cleaner.
>>
>> This is of course just a proposition about which I wanted to get some
>> feedback from people that know the codebase the most. It's my first
>> time working on the grub project so apologies for any flukes or
>> silliness in the code or the idea itself.
>>
>> Thank you!
>>
>> Best regards,
>>
>> [1] http://paste.ubuntu.com/p/cWR3k3NZgF/
>
> Next time please use git format-patch/send-email to send the patches.
>
>> diff --git a/grub-core/osdep/basic/no_platform.c 
>> b/grub-core/osdep/basic/no_platform.c
>> index d76c34c14..b39e97f48 100644
>> --- a/grub-core/osdep/basic/no_platform.c
>> +++ b/grub-core/osdep/basic/no_platform.c
>> @@ -25,7 +25,7 @@
>>
>>  void
>>  grub_install_register_ieee1275 (int is_prep, const char *install_device,
>> -                             int partno, const char *relpath)
>> +                             int partno, const char *relpath, int 
>> detect_nvram)
>>  {
>>    grub_util_error ("%s", _("no IEEE1275 routines are available for your 
>> platform"));
>>  }
>> @@ -33,7 +33,8 @@ grub_install_register_ieee1275 (int is_prep, const char 
>> *install_device,
>>  void
>>  grub_install_register_efi (grub_device_t efidir_grub_dev,
>>                          const char *efifile_path,
>> -                        const char *efi_distributor)
>> +                        const char *efi_distributor,
>> +                        int detect_nvram)
>>  {
>>    grub_util_error ("%s", _("no EFI routines are available for your 
>> platform"));
>>  }
>> diff --git a/grub-core/osdep/unix/platform.c 
>> b/grub-core/osdep/unix/platform.c
>> index ca448bc11..4eb2c11c9 100644
>> --- a/grub-core/osdep/unix/platform.c
>> +++ b/grub-core/osdep/unix/platform.c
>> @@ -134,7 +134,8 @@ grub_install_remove_efi_entries_by_distributor (const 
>> char *efi_distributor)
>>  int
>>  grub_install_register_efi (grub_device_t efidir_grub_dev,
>>                          const char *efifile_path,
>> -                        const char *efi_distributor)
>> +                        const char *efi_distributor,
>> +                        int detect_nvram)
>>  {
>>    const char * efidir_disk;
>>    int efidir_part;
>> @@ -153,6 +154,21 @@ grub_install_register_efi (grub_device_t 
>> efidir_grub_dev,
>>  #ifdef __linux__
>>    grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
>>  #endif
>> +
>> +  /* If requested, we try to detect if NVRAM access is available and if not,
>> +     warn the user and resume normal operation.  */
>> +  if (detect_nvram)
>> +    {
>> +      error = grub_util_exec_redirect_null ((const char * []){ 
>> "efibootmgr", NULL });
>> +      if (error == 2)
>> +     {
>> +       grub_util_warn ("%s", _("Auto-NVRAM selected and no EFI variable 
>> support detected on the system."));
>
> Wait, I have a feeling that you should fail hard here. In general
> I think that if somebody passed --auto-nvram on platforms with NVRAM
> and something fails then everything should fail. If somebody passed
> --auto-nvram on platforms without NVRAM then any attempt to access
> NVRAM should be skipped (silently?). Does it make sense?
>
> Daniel



-- 
Łukasz 'sil2100' Zemczak
 Foundations Team
 address@hidden
 www.canonical.com