Re: [PATCH] efi: stub: override RT_PROP table supported mask based on EFI variable

[Shawn Guo <shawn.guo@linaro.org>]

On Tue, Mar 16, 2021 at 08:57:19AM +0100, Ard Biesheuvel wrote:
> On Tue, 16 Mar 2021 at 08:52, Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > On Mon, Mar 15, 2021 at 02:07:01PM +0100, Ard Biesheuvel wrote:
> > > On Mon, 15 Mar 2021 at 04:11, Shawn Guo <shawn.guo@linaro.org> wrote:
> > > >
> > > > On Tue, Mar 09, 2021 at 07:47:25PM +0100, Ard Biesheuvel wrote:
> > > > > On Tue, 9 Mar 2021 at 19:10, Rob Clark <robdclark@gmail.com> wrote:
> > > > > >
> > > ...
> > > > > > fwiw, the valid use-case for ACPI boot on these things is for distro
> > > > > > installer.. it might not be the shiny accelerated experience, but you
> > > > > > want to be able to get thru the installer and then install updates to
> > > > > > get latest kernel/dtb/etc
> > > > > >
> > > > > > it is a small use-case, but kinda an important step ;-)
> > > > > >
> > > > >
> > > > > That is a fair point. However, as I understand it, we need this to work around
> > > > > - the need to pass efi=novamap
> > > > > - broken poweroff on Flex5g
> > > >
> > > > One more: broken EFI variable runtime services on all Snapdragon laptops
> > > >
> > > > It's been another pain of running debian-installer (d-i) on these
> > > > laptops, where EFI NV variables are just stored on UFS disk.  So after
> > > > Linux takes over the control of UFS, EFI NV variable runtime services
> > > > then become out of service.  Currently, we have to apply a hack [1] on
> > > > d-i grub-installer to get around the issue,  and it's been the only
> > > > remaining out-of-tree patch we have to carry for d-i.  With this nice
> > > > `OverrideSupported` support, we will be able to drop that hack
> > > > completely.
> > > >
> > > > >
> > > > > So an installer either needs to set the EFI variable, or pass
> > > > > efi=novamap on the first boot. Note that there are no arm64 EFI
> > > > > systems known where efi=novamap causes problems. In fact, I would
> > > > > prefer to stop using SetVirtualAddressMap() altogether, as it does not
> > > > > provide any benefit whatsoever. So perhaps we should make efi=novamap
> > > > > the default and be done with it.
> > > > >
> > > > > Broken poweroff is hardly a showstopper for an installer, given that
> > > > > we cannot even install GRUB correctly.
> > > > >
> > > > > In summary, I am more than happy to collaborate constructively on this
> > > > > (which is why I wrote the patch), but I don't think we're at a point
> > > > > yet where this is the only thing standing in our way when it comes to
> > > > > a smooth out-of-the-box Linux installation experience.
> > > >
> > > > There might be more to be done for getting a smooth Linux installation
> > > > experience.  But IMHO, this `OverrideSupported` thing is definitely
> > > > a big step to that.
> > > >
> > >
> > > So the problem here seems to be that grub-install (or efibootmgr)
> > > tolerates efivarfs being absent entirely, but bails out if it exists
> > > but gives an error when trying to access it, right?
> >
> > Yes, with EFI variables runtime service marked as unsupported,
> > efibootmgr will just exit on efi_variables_supported() check [1] in
> > a way that its parent process, i.e. grub-install, doesn't take as an
> > error.  But otherwise, efibootmgr will go much further and exit with
> > a real error when trying to access efivars.
> >
> 
> OK, so I suggest we fix efibootmgr, by extending the
> efi_variables_supported() check to perform a GetVariable() call on an
> arbitrary variable (e.g., BootOrder),

Hmm, I'm not sure we should ask more from user space, as it's already
been doing the right thing, and efi_variables_supported() is proved to
work properly with any sane low-level software (kernel + firmware),
either EFI variables service is supported or not.  That said, IMHO,
right now the low-level software on Snapdragon laptops is insane, i.e.
the unsupported/broken EFI runtime services are not communicated to
user space properly in established way.

Shawn

> and treat EFI_UNSUPPORTED as a
> special case that means that carrying on is pointless.
> 
> (but someone please confirm that the snapdragon efi firmware does
> return EFI_UNSUPPORTED and not some other return value when calling
> GetVariable() from under the OS)