Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Robbie Harwood]

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

Philip Müller <philm@manjaro.org> writes:

>> Hello Robbie, hello Daniel,
>>
>> with the commit 26031d3b101648352e4e427f04bf69d320088e77
>> 30_uefi-firmware will always call `fwsetup --is-supported' to check
>> if the system supports EFI or not. However most installed grub
>> versions on MBR don't support the '--is-supported' flag and crash the
>> menu creation routine.
>>
>> Arch Linux is tracking the issue here: https://bugs.archlinux.org/task/75701
>>
>> What would be the best approach with older installations of grub via
>> grub-install not having to reinstall grub to MBR as some users don't
>> even know on how to install grub properly as that job a graphical
>> installer does.
>
> Hi all,

Adding grub-devel to CC.

> I looked into the '30_uefi-firmware' changes a little more and also 
> commented my findings at the Arch-Bugtracker: 
> https://bugs.archlinux.org/task/75701#comment210684
>
> Before Menu changes we simply had this:
>
> ### BEGIN /etc/grub.d/30_uefi-firmware ###
> menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
>    fwsetup
> }
> ### END /etc/grub.d/30_uefi-firmware ###
>
> After Menu changes we went to that:
>
> ### BEGIN /etc/grub.d/30_uefi-firmware ###
> fwsetup --is-supported
> if [ "$grub_platform" = "efi" -a "$?" = 0 ]; then
>    menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
>      fwsetup
>    }
> fi
> ### END /etc/grub.d/30_uefi-firmware ###
>
> So 0eb684e8bfb0a9d2d42017a354740be25947babe I get and 
> 26031d3b101648352e4e427f04bf69d320088e77 tries to improve things, 
> however doesn't count in what will happen if 'fwsetup' is not supporting 
> the flag. It may either crash due to 
> 1e79bbfbda24a08cb856ff30f8b1bec460779b91 or start UEFI firmware instead.

Why doesn't grub on the MBR get updated when you install a new grub
package?  That seems like the real issue here - what am I missing?

Regardless, if we can't count on fwsetup being updated, then I think we
need to go back to the original version of my patch which doesn't have
--is-supported.

> Simply don't break user-space when older grub got installed to MBR. 
> Somehow this idea needs some more thinking and a solution for that
> case too.

Sure, what do you suggest?

Be well,
--Robbie

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

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Mike Gilbert]

On Tue, Aug 30, 2022 at 4:16 PM Robbie Harwood <rharwood@redhat.com> wrote:
> Why doesn't grub on the MBR get updated when you install a new grub
> package?  That seems like the real issue here - what am I missing?

I think the grub project tries to maintain config file compatibility
with older versions so that users can use new config files even if an
older grub version is still installed in /boot/grub.

> Regardless, if we can't count on fwsetup being updated, then I think we
> need to go back to the original version of my patch which doesn't have
> --is-supported.

You could add a feature flag, which causes grub-core to set an
environment variable when a new feature is supported. See the features
array in grub-core/normal/main.c.

You would then check for this feature flag in the grub.d snippet
before calling fwsetup --is-supported.

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Javier Martinez Canillas]

On 8/30/22 23:14, Mike Gilbert wrote:> On Tue, Aug 30, 2022 at 4:16 PM Robbie Harwood <rharwood@redhat.com> wrote:
>> Why doesn't grub on the MBR get updated when you install a new grub
>> package?  That seems like the real issue here - what am I missing?
> 
> I think the grub project tries to maintain config file compatibility
> with older versions so that users can use new config files even if an
> older grub version is still installed in /boot/grub.
>

In my opinion the grub configuration scheme is already too complex (a script
that executes a set of scripts that generate sections of a script executed
on boot) to also throw backward compatibility in the mix.

I agree with Robbie here, either don't re-generate your GRUB config file in
a installed system or update the GRUB core image in the embedding gap too.

Anything but that feels like an uphill battle to me and would just add much
more complexity.
 
>> Regardless, if we can't count on fwsetup being updated, then I think we
>> need to go back to the original version of my patch which doesn't have
>> --is-supported.
>

Indeed. The original patch didn't use that option, it was added to address
some feedback that Robbie had on an earlier version.
 
> You could add a feature flag, which causes grub-core to set an
> environment variable when a new feature is supported. See the features
> array in grub-core/normal/main.c.
>
> You would then check for this feature flag in the grub.d snippet
> before calling fwsetup --is-supported.
>

Please don't. As mentioned, I think we should aim to simplify the grub.cfg
instead of making it more complicated.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Philip Müller]

[-- Attachment #1.1: Type: text/plain, Size: 1299 bytes --]

On 30.08.22 23:34, Javier Martinez Canillas wrote:
>> You could add a feature flag, which causes grub-core to set an
>> environment variable when a new feature is supported. See the features
>> array in grub-core/normal/main.c.
>>
>> You would then check for this feature flag in the grub.d snippet
>> before calling fwsetup --is-supported.
>>
> Please don't. As mentioned, I think we should aim to simplify the grub.cfg
> instead of making it more complicated.

Well I think it would be the best approach to add backward compatibility 
as most users don't even know on how to install grub via grub-install. 
That is done via the graphical installer Calamares on most Arch-based 
Distributions. Updating the grub menu is common if you install multiple 
kernels or use snapshots via BTRFS.

Simply calling 'fwsetup' is a big NO-NO to me and others. The old 
version runs into the EFI firware or simply turn off the PC during boot, 
which creates boot loops for some or unbootable systems.

I checked on my end with an older grub in /boot and the updated menu.cfg 
script. Only when removing the snippet of 30_uefi-firmware the system is 
bootable again.

Also social media is slowly picking up that issue:
https://www.youtube.com/watch?v=b_KHtK2b5cA

-- 
Best, Philip


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Javier Martinez Canillas]

On 8/31/22 00:07, Philip Müller wrote:
> On 30.08.22 23:34, Javier Martinez Canillas wrote:
>>> You could add a feature flag, which causes grub-core to set an
>>> environment variable when a new feature is supported. See the features
>>> array in grub-core/normal/main.c.
>>>
>>> You would then check for this feature flag in the grub.d snippet
>>> before calling fwsetup --is-supported.
>>>
>> Please don't. As mentioned, I think we should aim to simplify the grub.cfg
>> instead of making it more complicated.
> 
> Well I think it would be the best approach to add backward compatibility 
> as most users don't even know on how to install grub via grub-install.
> That is done via the graphical installer Calamares on most Arch-based 
> Distributions. Updating the grub menu is common if you install multiple 
> kernels or use snapshots via BTRFS.
> 
> Simply calling 'fwsetup' is a big NO-NO to me and others. The old 
> version runs into the EFI firware or simply turn off the PC during boot, 
> which creates boot loops for some or unbootable systems.
>

I'm OK to not call fwsetup at all, that's what we originally had in the
series posted by Robbie, for example in v2:

https://lists.gnu.org/archive/html/grub-devel/2022-08/msg00169.html

Then they added the fwsetup --is-supported option as mentioned because
other developer asked for that. That patch was included in v3 onward:

https://lists.gnu.org/archive/html/grub-devel/2022-08/msg00196.html
 
> I checked on my end with an older grub in /boot and the updated menu.cfg 
> script. Only when removing the snippet of 30_uefi-firmware the system is 
> bootable again.
>

That's fair. Then probably what we should do is to partially revert
commit 26031d3b1016 ("efi: Don't display a uefi-firmware entry if
it's not supported"). Something like the following patch perhaps ?

From c3edc64687686d9a5a54f769ec03101b2c4cdef1 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Wed, 31 Aug 2022 00:30:31 +0200
Subject: [PATCH] templates: Don't execute fwsetup unconditionally
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 26031d3b1016 ("efi: Don't display a uefi-firmware entry if it's not
supported") added a new `fwsetup --is-supported` option that could be used
to check if the firmware allows to enter into a firmware user interface.

That option was then used by /etc/grub.d/30_uefi-firmware script to figure
out whether a `fwsetup` entry should be included or not in the boot menu.

But unfortunately, old GRUB images will fail to boot if are booted with an
updated GRUB config file. Since the `fwsetup --is-supported` call would be
taken as a plan `fwsetup` with no options.

This could either lead to a crash (i.e: on legacy BIOS systems where that
is not supported) or to the machine entering into the firmware settings.

To prevent that, let's partially revert the mentioned commit and keep the
old logic that didn't execute the `fwsetup` command and just included an
entry for it if GRUB is executed in an EFI platform.

Fixes: 26031d3b1016 ("efi: Don't display a uefi-firmware entry if it's not supported")
Reported-by: Philip Müller <philm@manjaro.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 util/grub.d/30_uefi-firmware.in | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
index c1731e5bbbb3..b6041b55e2a0 100644
--- a/util/grub.d/30_uefi-firmware.in
+++ b/util/grub.d/30_uefi-firmware.in
@@ -31,8 +31,7 @@ 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
+if [ "\$grub_platform" = "efi" ]; then
 	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
 		fwsetup
 	}
-- 
2.37.1

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Philip Müller]

[-- Attachment #1.1: Type: text/plain, Size: 360 bytes --]

On 31.08.22 00:43, Javier Martinez Canillas wrote:
> To prevent that, let's partially revert the mentioned commit and keep the
> old logic that didn't execute the `fwsetup` command and just included an
> entry for it if GRUB is executed in an EFI platform.

I was about to do the same on my end when we discussed it internally ...

-- 
Best, Philip


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Luna Jernberg]

Had this problem on my Arch Linux and EOS setup had to chroot in and
fiddle with this last week: https://youtu.be/b_KHtK2b5cA

On 8/31/22, Philip Müller via Grub-devel <grub-devel@gnu.org> wrote:
> On 31.08.22 00:43, Javier Martinez Canillas wrote:
>> To prevent that, let's partially revert the mentioned commit and keep the
>> old logic that didn't execute the `fwsetup` command and just included an
>> entry for it if GRUB is executed in an EFI platform.
>
> I was about to do the same on my end when we discussed it internally ...
>
> --
> Best, Philip
>
>

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Morten Linderud]

On Wed, Aug 31, 2022 at 12:07:33AM +0200, Philip Müller wrote:
> On 30.08.22 23:34, Javier Martinez Canillas wrote:
> > > You could add a feature flag, which causes grub-core to set an
> > > environment variable when a new feature is supported. See the features
> > > array in grub-core/normal/main.c.
> > > 
> > > You would then check for this feature flag in the grub.d snippet
> > > before calling fwsetup --is-supported.
> > > 
> > Please don't. As mentioned, I think we should aim to simplify the grub.cfg
> > instead of making it more complicated.
> 
> Well I think it would be the best approach to add backward compatibility as
> most users don't even know on how to install grub via grub-install. That is
> done via the graphical installer Calamares on most Arch-based Distributions.
> Updating the grub menu is common if you install multiple kernels or use
> snapshots via BTRFS.

Lets make a clear distinction between how Arch, the package upstream in this
case, and how derivative distros like Manjaro, the downstream user of this
package, uses grub though. The intent behind the packaging on our end does not
match up with what you the expectations you are conveying here.

Arch doesn't make assumptions about the users system and since `grub-install` is
a complicated command we can't automate this for users. If they use Secure Boot
they need to include modules, where is the ESP located and so on.

Arch requires the users to know how this command works, and it's a fair
assumption as it's documented in our install instructions. The fact that
derivative distros relies on this decision from Arch is their problem and should
not necessarily be a grub upstream issue.

Other distros build and ship monolithic grub.efi binaries for Secure Boot and
shim setups. From what I understand this entails that the configuration and the
binary stays in sync. This is not the case for Arch currently but might change
in the future.


If grub needs the configuration and grub.efi binaries to stay in sync, then it
would be much better for `grub-install` to support a config file like how people
have wound up using `grub-mkconfig`. Then shipping a hook that simply runs
`grub-install` would be simpler.

I think this is a good future solution as it would remove the need for distros
to ship `grub-install` wrappers anyway.

-- 
Morten Linderud
PGP: 9C02FF419FECBE16

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Mihai Moldovan]

[-- Attachment #1.1: Type: text/plain, Size: 2915 bytes --]

* On 8/30/22 22:16, Robbie Harwood wrote:
> Philip Müller <philm@manjaro.org> writes:
>
>>> What would be the best approach with older installations of grub via
>>> grub-install not having to reinstall grub to MBR as some users don't
>>> even know on how to install grub properly as that job a graphical
>>> installer does.
> 
> Why doesn't grub on the MBR get updated when you install a new grub
> package?  That seems like the real issue here - what am I missing?
> 
> Regardless, if we can't count on fwsetup being updated, then I think we
> need to go back to the original version of my patch which doesn't have
> --is-supported.

I am not denying that implementing backwards-compatibility is in order as part
of this report, but I would also like to point out Robbie's initial, very valid
point, which has so far been mostly ignored or put aside with a weird "that's a
job a graphical installer does" argument.

It is not. It should not be that way.

Archlinux and Gentoo are the only two Linux-based distributions I know of that
do not re-install grub (both to the boot directory and MBR, ESP or whatever
other thing systems are using) on package upgrades.

For Gentoo, at least, this is on purpose. It is *expected* for users to do that
manually when they see an upgrade to grub. Unfortunately, the documentation is
only mentioning the initial deployment and forgets about upgrade paths, so my
initial assumption that it was properly documented is incorrect. I probably
should make a note to fix this in the wiki pages and handbook.

All other Linux-based distributions and derivatives I know of - may that be
Debian-based, RH-based or *SuSE-based (though I lack experience with more exotic
ones, like Slackware, Mandriva, NixOS, ...) go a long way to make sure that grub
is automatically updated on every package upgrade by virtue of additional,
rather complex scripts that try to detect where it was installed to previously
and, importantly, to synchronize that up - both in the boot partition as well as
its core image loader, where ever that might be. Otherwise, upgrading the grub
package serves no useful purpose, since users will never see any changes.

A boot loader is not meant to be installed once "by a graphical installer" and
then stuck on that version. I can already smell "never change a running system"
and "I don't care for new features", but you should care for bug fixes,
especially security-related ones. Archlinux users seem to be missing out on
that, unless they take manual steps, and from the report it seems like most of
them are not even aware that they should be updating grub manually, which either
indicates improper education and documentation in case it is really MEANT to be
a manual-only process, or a bug in Archlinux' binary grub package.

I would call that an Archlinux bug, especially with all that echo.



Mihai

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Dimitri John Ledkov]

On Tue, 30 Aug 2022 at 21:22, Robbie Harwood <rharwood@redhat.com> wrote:
>
> Philip Müller <philm@manjaro.org> writes:
>
> >> Hello Robbie, hello Daniel,
> >>
> >> with the commit 26031d3b101648352e4e427f04bf69d320088e77
> >> 30_uefi-firmware will always call `fwsetup --is-supported' to check
> >> if the system supports EFI or not. However most installed grub
> >> versions on MBR don't support the '--is-supported' flag and crash the
> >> menu creation routine.
> >>
> >> Arch Linux is tracking the issue here: https://bugs.archlinux.org/task/75701
> >>
> >> What would be the best approach with older installations of grub via
> >> grub-install not having to reinstall grub to MBR as some users don't
> >> even know on how to install grub properly as that job a graphical
> >> installer does.
> >
> > Hi all,
>
> Adding grub-devel to CC.
>
> > I looked into the '30_uefi-firmware' changes a little more and also
> > commented my findings at the Arch-Bugtracker:
> > https://bugs.archlinux.org/task/75701#comment210684
> >
> > Before Menu changes we simply had this:
> >
> > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> >    fwsetup
> > }
> > ### END /etc/grub.d/30_uefi-firmware ###
> >
> > After Menu changes we went to that:
> >
> > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > fwsetup --is-supported
> > if [ "$grub_platform" = "efi" -a "$?" = 0 ]; then
> >    menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> >      fwsetup
> >    }
> > fi

above code looks buggy to me. As far as I understand fwsetup only
works on EFI grub_platform, and thus originally the menu entry and
fwsetup call was scoped under efi platform. thus there should be two
if conditions, not one:

if [ "$grub_platform" = "efi" ]; then
fwsetup --is-supported
if [ "$?" = 0 ]; then
menuetry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
fwsetup
}
fi
fi

This way the code continues to work correctly on bios platforms
(skipped completely), and on EFI like platforms fwsetup menu entry is
only shown if fwsetup --is-supported executes successfully.

the optimisation of having two conditionals evaluated together, whilst
efficient, is wrong given cross grub platform compatibilities desires.

This fwsetup has never before been executed on bios platform, and we
should not be introducing this.

> > ### END /etc/grub.d/30_uefi-firmware ###
> >
> > So 0eb684e8bfb0a9d2d42017a354740be25947babe I get and
> > 26031d3b101648352e4e427f04bf69d320088e77 tries to improve things,
> > however doesn't count in what will happen if 'fwsetup' is not supporting
> > the flag. It may either crash due to
> > 1e79bbfbda24a08cb856ff30f8b1bec460779b91 or start UEFI firmware instead.
>
> Why doesn't grub on the MBR get updated when you install a new grub
> package?  That seems like the real issue here - what am I missing?
>
> Regardless, if we can't count on fwsetup being updated, then I think we
> need to go back to the original version of my patch which doesn't have
> --is-supported.
>
> > Simply don't break user-space when older grub got installed to MBR.
> > Somehow this idea needs some more thinking and a solution for that
> > case too.
>
> Sure, what do you suggest?
>
> Be well,
> --Robbie
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
okurrr,

Dimitri

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Robbie Harwood]

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

Javier Martinez Canillas <javierm@redhat.com> writes:

> On 8/31/22 00:07, Philip Müller wrote:
>> On 30.08.22 23:34, Javier Martinez Canillas wrote:
>>>> You could add a feature flag, which causes grub-core to set an
>>>> environment variable when a new feature is supported. See the features
>>>> array in grub-core/normal/main.c.
>>>>
>>>> You would then check for this feature flag in the grub.d snippet
>>>> before calling fwsetup --is-supported.
>>>>
>>> Please don't. As mentioned, I think we should aim to simplify the grub.cfg
>>> instead of making it more complicated.
>> 
>> Well I think it would be the best approach to add backward compatibility 
>> as most users don't even know on how to install grub via grub-install.
>> That is done via the graphical installer Calamares on most Arch-based 
>> Distributions. Updating the grub menu is common if you install multiple 
>> kernels or use snapshots via BTRFS.
>> 
>> Simply calling 'fwsetup' is a big NO-NO to me and others. The old 
>> version runs into the EFI firware or simply turn off the PC during boot, 
>> which creates boot loops for some or unbootable systems.
>
> I'm OK to not call fwsetup at all, that's what we originally had in the
> series posted by Robbie, for example in v2:
>
> https://lists.gnu.org/archive/html/grub-devel/2022-08/msg00169.html
>
> Then they added the fwsetup --is-supported option as mentioned because
> other developer asked for that. That patch was included in v3 onward:
>
> https://lists.gnu.org/archive/html/grub-devel/2022-08/msg00196.html
>  
>> I checked on my end with an older grub in /boot and the updated menu.cfg 
>> script. Only when removing the snippet of 30_uefi-firmware the system is 
>> bootable again.
>>
>
> That's fair. Then probably what we should do is to partially revert
> commit 26031d3b1016 ("efi: Don't display a uefi-firmware entry if
> it's not supported"). Something like the following patch perhaps ?
>
> From c3edc64687686d9a5a54f769ec03101b2c4cdef1 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> Date: Wed, 31 Aug 2022 00:30:31 +0200
> Subject: [PATCH] templates: Don't execute fwsetup unconditionally
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Commit 26031d3b1016 ("efi: Don't display a uefi-firmware entry if it's not
> supported") added a new `fwsetup --is-supported` option that could be used
> to check if the firmware allows to enter into a firmware user interface.
>
> That option was then used by /etc/grub.d/30_uefi-firmware script to figure
> out whether a `fwsetup` entry should be included or not in the boot menu.
>
> But unfortunately, old GRUB images will fail to boot if are booted with an
> updated GRUB config file. Since the `fwsetup --is-supported` call would be
> taken as a plan `fwsetup` with no options.
>
> This could either lead to a crash (i.e: on legacy BIOS systems where that
> is not supported) or to the machine entering into the firmware settings.
>
> To prevent that, let's partially revert the mentioned commit and keep the
> old logic that didn't execute the `fwsetup` command and just included an
> entry for it if GRUB is executed in an EFI platform.
>
> Fixes: 26031d3b1016 ("efi: Don't display a uefi-firmware entry if it's not supported")
> Reported-by: Philip Müller <philm@manjaro.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  util/grub.d/30_uefi-firmware.in | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
> index c1731e5bbbb3..b6041b55e2a0 100644
> --- a/util/grub.d/30_uefi-firmware.in
> +++ b/util/grub.d/30_uefi-firmware.in
> @@ -31,8 +31,7 @@ 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
> +if [ "\$grub_platform" = "efi" ]; then
>  	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
>  		fwsetup
>  	}
> -- 
> 2.37.1

While we could revert the entire --is-supported logic as well, since
this is upstream pre-release code, it's probably easier for the
downstreams that pulled this change if we don't, so:

Reviewed-by: Robbie Harwood <rharwood@redhat.com>.

Be well,
--Robbie

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

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Philip Müller]

[-- Attachment #1.1: Type: text/plain, Size: 621 bytes --]

On 31.08.22 20:44, Robbie Harwood wrote:
> While we could revert the entire --is-supported logic as well, since
> this is upstream pre-release code, it's probably easier for the
> downstreams that pulled this change if we don't, so:
> 
> Reviewed-by: Robbie Harwood<rharwood@redhat.com>.

Yep, that would also work. For example Arch did it at some point but 
removed the revert for some reason later on:

https://github.com/archlinux/svntogit-packages/commit/1cd6ce212cb456340beba6093b89e2bd196f908b

Also the call of 'fwsetup' on legacy BIOS systems makes not much sense, 
isn't it?

-- 
Best, Philip


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Glenn Washburn]

On Wed, 31 Aug 2022 16:14:42 +0100
Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:

> On Tue, 30 Aug 2022 at 21:22, Robbie Harwood <rharwood@redhat.com> wrote:
> >
> > Philip Müller <philm@manjaro.org> writes:
> >
> > >> Hello Robbie, hello Daniel,
> > >>
> > >> with the commit 26031d3b101648352e4e427f04bf69d320088e77
> > >> 30_uefi-firmware will always call `fwsetup --is-supported' to check
> > >> if the system supports EFI or not. However most installed grub
> > >> versions on MBR don't support the '--is-supported' flag and crash the
> > >> menu creation routine.
> > >>
> > >> Arch Linux is tracking the issue here: https://bugs.archlinux.org/task/75701
> > >>
> > >> What would be the best approach with older installations of grub via
> > >> grub-install not having to reinstall grub to MBR as some users don't
> > >> even know on how to install grub properly as that job a graphical
> > >> installer does.
> > >
> > > Hi all,
> >
> > Adding grub-devel to CC.
> >
> > > I looked into the '30_uefi-firmware' changes a little more and also
> > > commented my findings at the Arch-Bugtracker:
> > > https://bugs.archlinux.org/task/75701#comment210684
> > >
> > > Before Menu changes we simply had this:
> > >
> > > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > > menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> > >    fwsetup
> > > }
> > > ### END /etc/grub.d/30_uefi-firmware ###
> > >
> > > After Menu changes we went to that:
> > >
> > > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > > fwsetup --is-supported
> > > if [ "$grub_platform" = "efi" -a "$?" = 0 ]; then
> > >    menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> > >      fwsetup
> > >    }
> > > fi
> 
> above code looks buggy to me. As far as I understand fwsetup only
> works on EFI grub_platform, and thus originally the menu entry and
> fwsetup call was scoped under efi platform. thus there should be two
> if conditions, not one:
> 
> if [ "$grub_platform" = "efi" ]; then
> fwsetup --is-supported
> if [ "$?" = 0 ]; then
> menuetry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> fwsetup
> }
> fi
> fi

I agree there's a bug, but I don't think its what is mentioned. The bug
is that $? is not set to 1 when a command does not exist. So in
i386-pc, where there is no fwsetup, executing "fwsetup --is-supported"
will fail because there is no command fwsetup. And $? should be set to
1 in this case, which would allow the existing code to work as expected.

Even if we fix the bug I mention, I support the above suggested change
because its easier to read.

> 
> This way the code continues to work correctly on bios platforms
> (skipped completely), and on EFI like platforms fwsetup menu entry is
> only shown if fwsetup --is-supported executes successfully.
> 
> the optimisation of having two conditionals evaluated together, whilst
> efficient, is wrong given cross grub platform compatibilities desires.
> 
> This fwsetup has never before been executed on bios platform, and we
> should not be introducing this.

And it never will be because its never built for the BIOS platform.
GRUB will error with "error: [16] can't find command `fwsetup'.".

Glenn

> 
> > > ### END /etc/grub.d/30_uefi-firmware ###
> > >
> > > So 0eb684e8bfb0a9d2d42017a354740be25947babe I get and
> > > 26031d3b101648352e4e427f04bf69d320088e77 tries to improve things,
> > > however doesn't count in what will happen if 'fwsetup' is not supporting
> > > the flag. It may either crash due to
> > > 1e79bbfbda24a08cb856ff30f8b1bec460779b91 or start UEFI firmware instead.
> >
> > Why doesn't grub on the MBR get updated when you install a new grub
> > package?  That seems like the real issue here - what am I missing?
> >
> > Regardless, if we can't count on fwsetup being updated, then I think we
> > need to go back to the original version of my patch which doesn't have
> > --is-supported.
> >
> > > Simply don't break user-space when older grub got installed to MBR.
> > > Somehow this idea needs some more thinking and a solution for that
> > > case too.
> >
> > Sure, what do you suggest?
> >
> > Be well,
> > --Robbie
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> 

Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Daniel Kiper]

On Mon, Sep 05, 2022 at 05:49:47PM -0500, Glenn Washburn wrote:
> On Wed, 31 Aug 2022 16:14:42 +0100
> Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
> > On Tue, 30 Aug 2022 at 21:22, Robbie Harwood <rharwood@redhat.com> wrote:
> > > Philip Müller <philm@manjaro.org> writes:
> > >
> > > >> Hello Robbie, hello Daniel,
> > > >>
> > > >> with the commit 26031d3b101648352e4e427f04bf69d320088e77
> > > >> 30_uefi-firmware will always call `fwsetup --is-supported' to check
> > > >> if the system supports EFI or not. However most installed grub
> > > >> versions on MBR don't support the '--is-supported' flag and crash the
> > > >> menu creation routine.
> > > >>
> > > >> Arch Linux is tracking the issue here: https://bugs.archlinux.org/task/75701
> > > >>
> > > >> What would be the best approach with older installations of grub via
> > > >> grub-install not having to reinstall grub to MBR as some users don't
> > > >> even know on how to install grub properly as that job a graphical
> > > >> installer does.
> > > >
> > > > Hi all,
> > >
> > > Adding grub-devel to CC.
> > >
> > > > I looked into the '30_uefi-firmware' changes a little more and also
> > > > commented my findings at the Arch-Bugtracker:
> > > > https://bugs.archlinux.org/task/75701#comment210684
> > > >
> > > > Before Menu changes we simply had this:
> > > >
> > > > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > > > menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> > > >    fwsetup
> > > > }
> > > > ### END /etc/grub.d/30_uefi-firmware ###
> > > >
> > > > After Menu changes we went to that:
> > > >
> > > > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > > > fwsetup --is-supported
> > > > if [ "$grub_platform" = "efi" -a "$?" = 0 ]; then
> > > >    menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> > > >      fwsetup
> > > >    }
> > > > fi
> >
> > above code looks buggy to me. As far as I understand fwsetup only
> > works on EFI grub_platform, and thus originally the menu entry and
> > fwsetup call was scoped under efi platform. thus there should be two
> > if conditions, not one:
> >
> > if [ "$grub_platform" = "efi" ]; then
> > fwsetup --is-supported
> > if [ "$?" = 0 ]; then
> > menuetry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> > fwsetup
> > }
> > fi
> > fi
>
> I agree there's a bug, but I don't think its what is mentioned. The bug
> is that $? is not set to 1 when a command does not exist. So in
> i386-pc, where there is no fwsetup, executing "fwsetup --is-supported"
> will fail because there is no command fwsetup. And $? should be set to
> 1 in this case, which would allow the existing code to work as expected.
>
> Even if we fix the bug I mention, I support the above suggested change
> because its easier to read.

I concur.

Dimitri, may I ask you to send a patch with fix as you suggested above?

Daniel