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> --- 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
