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

[Ard Biesheuvel <ardb@kernel.org>]

On Tue, 16 Mar 2021 at 10:06, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> 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.
>

I disagree.

My Yoga returns

efivars: get_next_variable: status=8000000000000003

which is documented in the UEFI spec 2.8B section 8.2 as

"""
EFI_UNSUPPORTED
After ExitBootServices() has been called, this return code may be
returned if no variable storage is supported. The platform should
describe this runtime service as unsupported at runtime via an
EFI_RT_PROPERTIES_TABLE configuration table.
"""

No other condition is documented under which GetNextVariable() can
return EFI_UNSUPPORTED, so it is perfectly suitable to decide whether
the platform in question supports variable services at runtime at all.

Ideally, the platform should describe this in the RT_PROP table, but
the spec is very clear that the runtime services themselves should
still be callable, but return EFI_UNSUPPORTED in that case.