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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 17 Mar 2021 at 07:36, Shawn Guo <shawn.guo@xxxxxxxxxx> wrote:
>
> On Tue, Mar 16, 2021 at 10:33:17AM +0100, Ard Biesheuvel wrote:
> > On Tue, 16 Mar 2021 at 10:06, Shawn Guo <shawn.guo@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> 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.
>
> I'm not arguing against ideal of checking EFI_UNSUPPORTED.  Instead, I
> agree with that.  What I'm arguing is that this should be done by kernel
> rather than efibootmgr.  The efi_variables_supported() of efibootmgr
> checks EFIVARFS_MAGIC on /sys/firmware/efi/efivars.  So if we have kernel
> function efivar_init() check and respect EFI_UNSUPPORTED return and stop
> right there, we are all good then.  Could you take a look at the patch
> attached and see if it's acceptable?
>
> Shawn
>
> ------8<---------------
>
> From a30a9a03ed254e0f893b831618b30eaffe7f2da7 Mon Sep 17 00:00:00 2001
> From: Shawn Guo <shawn.guo@xxxxxxxxxx>
> Date: Wed, 17 Mar 2021 11:57:58 +0800
> Subject: [PATCH] efivars: respect EFI_UNSUPPORTED return from firmware
>
> As per UEFI spec 2.8B section 8.2, EFI_UNSUPPORTED may be returned by
> EFI variable runtime services if no variable storage is supported by
> firmware.  In this case, there is no point for kernel to continue
> efivars initialization.  That said, efivar_init() should fail by
> returning an error code, so that efivarfs will not be mounted on
> /sys/firmware/efi/efivars at all.  Otherwise, user space like efibootmgr
> will be confused by the EFIVARFS_MAGIC seen there, while EFI variable
> calls cannot be made successfully.
>
> Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>

Yes, this makes sense.

Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx>

I'll queue this up


> ---
>  drivers/firmware/efi/vars.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 41c1d00bf933..abdc8a6a3963 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -484,6 +484,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>                                 }
>                         }
>
> +                       break;
> +               case EFI_UNSUPPORTED:
> +                       err = -EOPNOTSUPP;
> +                       status = EFI_NOT_FOUND;
>                         break;
>                 case EFI_NOT_FOUND:
>                         break;
> --
> 2.17.1
>



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux