Re: [PATCH v2 3/4] templates: Check for EFI at runtime instead of config generation time

[Glenn Washburn]

On Wed, 17 Aug 2022 15:50:33 -0400
Robbie Harwood <rharwood@redhat.com> wrote:

> From: Javier Martinez Canillas <javierm@redhat.com>
> 
> The 30_uefi-firmware template checks if an OsIndicationsSupported UEFI var
> exists and EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set, to decide whether
> a "fwsetup" menu entry would be added or not to the GRUB menu.
> 
> But this has the problem that it will only work if the configuration file
> was created on an UEFI machine that supports booting to a firmware UI.
> 
> This for example doesn't support creating GRUB config files when executing
> on systems that support both UEFI and legacy BIOS booting. Since creating
> the config file from legacy BIOS wouldn't allow to access the firmware UI.
> 
> To prevent this, make the template to unconditionally create the grub.cfg
> snippet but check at runtime if was booted through UEFI to decide if this
> entry should be added. That way it won't be added when booting with BIOS.
> 
> There's no need to check if EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set,
> since that's already done by the "fwsetup" command when is executed.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  util/grub.d/30_uefi-firmware.in | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
> index d344d3883d..b6041b55e2 100644
> --- a/util/grub.d/30_uefi-firmware.in
> +++ b/util/grub.d/30_uefi-firmware.in
> @@ -26,19 +26,14 @@ export TEXTDOMAINDIR="@localedir@"
>  
>  . "$pkgdatadir/grub-mkconfig_lib"
>  
> -EFI_VARS_DIR=/sys/firmware/efi/efivars
> -EFI_GLOBAL_VARIABLE=8be4df61-93ca-11d2-aa0d-00e098032b8c
> -OS_INDICATIONS="$EFI_VARS_DIR/OsIndicationsSupported-$EFI_GLOBAL_VARIABLE"
> +LABEL="UEFI Firmware Settings"
>  
> -if [ -e "$OS_INDICATIONS" ] && \
> -   [ "$(( $(printf 0x%x \'"$(cat $OS_INDICATIONS | cut -b5)"\') & 1 ))" = 1 
> ]; then
> -  LABEL="UEFI Firmware Settings"
> +gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
>  
> -  gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" 
> >&2
> -
> -  cat << EOF
> -menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> -     fwsetup
> -}
> -EOF
> +cat << EOF
> +if [ "\$grub_platform" = "efi" ]; then

I think it would be even better if this was not displayed if
efifwsetup_is_supported () returns false. This could be done with
another condition on the output of "fwsetup --is-supported" and in the
following patch or an additional patch add support for the
"--is-supported" argument which should just return success if
efifwsetup_is_supported () is true and false otherwise. This addition
would preserve the current behavior which this patch otherwise does not
completely preserve.

Glenn

> +     menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> +             fwsetup
> +     }
>  fi
> +EOF