[PATCH] efi: add feature_efifwsetup_check

[PATCH] efi: add feature_efifwsetup_check

[Felipe Contreras]

In commit 26031d3b1 (efi: Don't display a uefi-firmware entry if it's
not supported, 2022-08-18) an unconditional call to `fwsetup
--is-supported` was added which causes the system to reboot if
`grub-install` hasn't been run because `fwsetup` doesn't have the new
option.

This is a huge regression.

To make sure `fwsetup --is-supported` works we add a new feature that
only newer GRUB installations have: feature_efifwsetup_check.

If the feature is available the current behavior is maintained (`fwsetup
--is-supported`) but if it's not available the behavior before 26031d3b1
is maintained (unconditionally show the menu entry on EFI platforms).

Also do this only on the EFI platform as Dimitri John Ledkov suggested.

Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Robbie Harwood <rharwood@redhat.com>
Cc: Dimitri John Ledkov <xnox@ubuntu.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 grub-core/normal/main.c         |  2 +-
 util/grub.d/30_uefi-firmware.in | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
index cb0e8e7fd..072e8e681 100644
--- a/grub-core/normal/main.c
+++ b/grub-core/normal/main.c
@@ -506,7 +506,7 @@ static const char *features[] = {
   "feature_chainloader_bpb", "feature_ntldr", "feature_platform_search_hint",
   "feature_default_font_path", "feature_all_video_module",
   "feature_menuentry_id", "feature_menuentry_options", "feature_200_final",
-  "feature_nativedisk_cmd", "feature_timeout_style"
+  "feature_nativedisk_cmd", "feature_timeout_style", "feature_efifwsetup_check"
 };
 
 GRUB_MOD_INIT(normal)
diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
index c1731e5bb..c23d84741 100644
--- a/util/grub.d/30_uefi-firmware.in
+++ b/util/grub.d/30_uefi-firmware.in
@@ -31,10 +31,16 @@ LABEL="UEFI Firmware Settings"
 gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
 
 cat << EOF
-fwsetup --is-supported
-if [ "\$grub_platform" = "efi" -a "\$?" = 0 ]; then
-	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
-		fwsetup
-	}
+if [ "\$grub_platform" = "efi" ]; then
+  if [ "\$feature_efifwsetup_check" = "y" ] ; then
+    fwsetup --is-supported
+  else
+    true
+  fi
+  if [ "\$?" = 0 ]; then
+	  menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
+		  fwsetup
+	  }
+  fi
 fi
 EOF
-- 
2.37.3

Re: [PATCH] efi: add feature_efifwsetup_check

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 2767 bytes --]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> In commit 26031d3b1 (efi: Don't display a uefi-firmware entry if it's
> not supported, 2022-08-18) an unconditional call to `fwsetup
> --is-supported` was added which causes the system to reboot if
> `grub-install` hasn't been run because `fwsetup` doesn't have the new
> option.
>
> This is a huge regression.
>
> To make sure `fwsetup --is-supported` works we add a new feature that
> only newer GRUB installations have: feature_efifwsetup_check.
>
> If the feature is available the current behavior is maintained (`fwsetup
> --is-supported`) but if it's not available the behavior before 26031d3b1
> is maintained (unconditionally show the menu entry on EFI platforms).
>
> Also do this only on the EFI platform as Dimitri John Ledkov suggested.
>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Robbie Harwood <rharwood@redhat.com>
> Cc: Dimitri John Ledkov <xnox@ubuntu.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  grub-core/normal/main.c         |  2 +-
>  util/grub.d/30_uefi-firmware.in | 16 +++++++++++-----
>  2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
> index cb0e8e7fd..072e8e681 100644
> --- a/grub-core/normal/main.c
> +++ b/grub-core/normal/main.c
> @@ -506,7 +506,7 @@ static const char *features[] = {
>    "feature_chainloader_bpb", "feature_ntldr", "feature_platform_search_hint",
>    "feature_default_font_path", "feature_all_video_module",
>    "feature_menuentry_id", "feature_menuentry_options", "feature_200_final",
> -  "feature_nativedisk_cmd", "feature_timeout_style"
> +  "feature_nativedisk_cmd", "feature_timeout_style", "feature_efifwsetup_check"
>  };
>  
>  GRUB_MOD_INIT(normal)
> diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
> index c1731e5bb..c23d84741 100644
> --- a/util/grub.d/30_uefi-firmware.in
> +++ b/util/grub.d/30_uefi-firmware.in
> @@ -31,10 +31,16 @@ LABEL="UEFI Firmware Settings"
>  gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
>  
>  cat << EOF
> -fwsetup --is-supported
> -if [ "\$grub_platform" = "efi" -a "\$?" = 0 ]; then
> -	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> -		fwsetup
> -	}
> +if [ "\$grub_platform" = "efi" ]; then
> +  if [ "\$feature_efifwsetup_check" = "y" ] ; then
> +    fwsetup --is-supported
> +  else
> +    true
> +  fi
> +  if [ "\$?" = 0 ]; then
> +	  menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> +		  fwsetup
> +	  }
> +  fi
>  fi
>  EOF
> -- 
> 2.37.3

Thanks for your interest, but I prefer Javier's proposed approach.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH] efi: add feature_efifwsetup_check

[Felipe Contreras]

On Mon, Sep 12, 2022 at 5:27 AM Robbie Harwood <rharwood@redhat.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > In commit 26031d3b1 (efi: Don't display a uefi-firmware entry if it's
> > not supported, 2022-08-18) an unconditional call to `fwsetup
> > --is-supported` was added which causes the system to reboot if
> > `grub-install` hasn't been run because `fwsetup` doesn't have the new
> > option.
> >
> > This is a huge regression.
> >
> > To make sure `fwsetup --is-supported` works we add a new feature that
> > only newer GRUB installations have: feature_efifwsetup_check.
> >
> > If the feature is available the current behavior is maintained (`fwsetup
> > --is-supported`) but if it's not available the behavior before 26031d3b1
> > is maintained (unconditionally show the menu entry on EFI platforms).
> >
> > Also do this only on the EFI platform as Dimitri John Ledkov suggested.
> >
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Cc: Robbie Harwood <rharwood@redhat.com>
> > Cc: Dimitri John Ledkov <xnox@ubuntu.com>
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  grub-core/normal/main.c         |  2 +-
> >  util/grub.d/30_uefi-firmware.in | 16 +++++++++++-----
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
> > index cb0e8e7fd..072e8e681 100644
> > --- a/grub-core/normal/main.c
> > +++ b/grub-core/normal/main.c
> > @@ -506,7 +506,7 @@ static const char *features[] = {
> >    "feature_chainloader_bpb", "feature_ntldr", "feature_platform_search_hint",
> >    "feature_default_font_path", "feature_all_video_module",
> >    "feature_menuentry_id", "feature_menuentry_options", "feature_200_final",
> > -  "feature_nativedisk_cmd", "feature_timeout_style"
> > +  "feature_nativedisk_cmd", "feature_timeout_style", "feature_efifwsetup_check"
> >  };
> >
> >  GRUB_MOD_INIT(normal)
> > diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
> > index c1731e5bb..c23d84741 100644
> > --- a/util/grub.d/30_uefi-firmware.in
> > +++ b/util/grub.d/30_uefi-firmware.in
> > @@ -31,10 +31,16 @@ LABEL="UEFI Firmware Settings"
> >  gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
> >
> >  cat << EOF
> > -fwsetup --is-supported
> > -if [ "\$grub_platform" = "efi" -a "\$?" = 0 ]; then
> > -     menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> > -             fwsetup
> > -     }
> > +if [ "\$grub_platform" = "efi" ]; then
> > +  if [ "\$feature_efifwsetup_check" = "y" ] ; then
> > +    fwsetup --is-supported
> > +  else
> > +    true
> > +  fi
> > +  if [ "\$?" = 0 ]; then
> > +       menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> > +               fwsetup
> > +       }
> > +  fi
> >  fi
> >  EOF
> > --
> > 2.37.3
>
> Thanks for your interest, but I prefer Javier's proposed approach.

Just to be clear: Javier's patch will *always* show a menu entry for
UEFI firmware UI, even if the machine doesn't support it, so the user
will click the entry, and an error will appear.

And also will make it so nothing uses `fwsetup --is-supported`. Should
the option be removed then?

-- 
Felipe Contreras