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

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

[Ard Biesheuvel]

Allow EFI systems to override the set of supported runtime services
declared via the RT_PROP table, by checking for the existence of a
'OverrideSupported' EFI variable of the appropriate size under the
RT_PROP table GUID, and if it does, combine the supported mask using
logical AND. (This means the override can only remove support, not
add it back).

Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: linux-arm-msm@vger.kernel.org

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efi-stub.c | 37 ++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 26e69788f27a..a23d95039b2a 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -96,6 +96,41 @@ static void install_memreserve_table(void)
 		efi_err("Failed to install memreserve config table!\n");
 }
 
+static void check_rt_properties_table_override(void)
+{
+	static const efi_guid_t rt_prop_guid = EFI_RT_PROPERTIES_TABLE_GUID;
+	efi_rt_properties_table_t *table;
+	unsigned long size = sizeof(u32);
+	efi_status_t status;
+	u32 override;
+
+	status = get_efi_var(L"OverrideSupported", &rt_prop_guid, NULL, &size, &override);
+	if (status != EFI_SUCCESS || size != sizeof(override))
+		return;
+
+	table = get_efi_config_table(rt_prop_guid);
+	if (!table) {
+		/* no table exists yet - allocate a new one */
+		status = efi_bs_call(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
+				     sizeof(*table), (void **)&table);
+		if (status != EFI_SUCCESS)
+			return;
+		table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
+		table->length = sizeof(*table);
+		table->runtime_services_supported = EFI_RT_SUPPORTED_ALL;
+
+		status = efi_bs_call(install_configuration_table,
+				     (efi_guid_t *)&rt_prop_guid, table);
+		if (status != EFI_SUCCESS) {
+			efi_warn("Failed to install RT_PROP override table\n");
+			return;
+		}
+	}
+
+	efi_info("Applying RT_PROP table override from EFI variable\n");
+	table->runtime_services_supported &= override;
+}
+
 static u32 get_supported_rt_services(void)
 {
 	const efi_rt_properties_table_t *rt_prop_table;
@@ -210,6 +245,8 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 
 	secure_boot = efi_get_secureboot();
 
+	check_rt_properties_table_override();
+
 	/*
 	 * Unauthenticated device tree data is a security hazard, so ignore
 	 * 'dtb=' unless UEFI Secure Boot is disabled.  We assume that secure
-- 
2.30.1

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

[Shawn Guo]

On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> Allow EFI systems to override the set of supported runtime services
> declared via the RT_PROP table, by checking for the existence of a
> 'OverrideSupported' EFI variable of the appropriate size under the
> RT_PROP table GUID, and if it does, combine the supported mask using
> logical AND. (This means the override can only remove support, not
> add it back).
> 
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: linux-arm-msm@vger.kernel.org
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:

Tested-by: Shawn Guo <shawn.guo@linaro.org>

With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
'efi=novamap' kernel cmdline and get around the broken poweroff runtime
services nicely.  Thanks!

Shawn

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

[Ard Biesheuvel]

On Sun, 7 Mar 2021 at 12:02, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> > Allow EFI systems to override the set of supported runtime services
> > declared via the RT_PROP table, by checking for the existence of a
> > 'OverrideSupported' EFI variable of the appropriate size under the
> > RT_PROP table GUID, and if it does, combine the supported mask using
> > logical AND. (This means the override can only remove support, not
> > add it back).
> >
> > Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: linux-arm-msm@vger.kernel.org
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:
>
> Tested-by: Shawn Guo <shawn.guo@linaro.org>
>
> With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
> 'efi=novamap' kernel cmdline and get around the broken poweroff runtime
> services nicely.  Thanks!
>

Thanks for confirming.

However, I am not going to merge this without some justification, and
hopefully some input from other folks (Leif?)

RTPROP already provides what we need on all platforms that use
DtbLoader, and the patch for that is queued up for v5.12-rcX, with a
cc:stable to v5.10. This allows any RT service to be marked as
disabled, including SetVirtualAddressMap().

So afaict, that means that this patch would be a special case for
Flex5G, right? So how are platforms such as this one going to load the
DTB? If some loader will be involved (or even just GRUB), shouldn't it
be that component that sets RTPROP like DtbLoader will, not the kernel
itself.

Btw I don't think ACPI boot is a use case here. I don't see a software
framebuffer with no wifi support as a usage mode that justifies
carrying EFI stub hacks for everyone.

-- 
Ard.

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

[Shawn Guo]

On Mon, Mar 08, 2021 at 02:34:48PM +0100, Ard Biesheuvel wrote:
> On Sun, 7 Mar 2021 at 12:02, Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> > > Allow EFI systems to override the set of supported runtime services
> > > declared via the RT_PROP table, by checking for the existence of a
> > > 'OverrideSupported' EFI variable of the appropriate size under the
> > > RT_PROP table GUID, and if it does, combine the supported mask using
> > > logical AND. (This means the override can only remove support, not
> > > add it back).
> > >
> > > Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > Cc: linux-arm-msm@vger.kernel.org
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:
> >
> > Tested-by: Shawn Guo <shawn.guo@linaro.org>
> >
> > With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
> > 'efi=novamap' kernel cmdline and get around the broken poweroff runtime
> > services nicely.  Thanks!
> >
> 
> Thanks for confirming.
> 
> However, I am not going to merge this without some justification, and
> hopefully some input from other folks (Leif?)
> 
> RTPROP already provides what we need on all platforms that use
> DtbLoader, and the patch for that is queued up for v5.12-rcX, with a
> cc:stable to v5.10. This allows any RT service to be marked as
> disabled, including SetVirtualAddressMap().
> 
> So afaict, that means that this patch would be a special case for
> Flex5G, right?

It's for all Snapdragon based laptops, as we need to disable
SetVirtualAddressMap runtime services on all of them.

> So how are platforms such as this one going to load the
> DTB? If some loader will be involved (or even just GRUB),

Yes, GRUB.

> shouldn't it
> be that component that sets RTPROP like DtbLoader will, not the kernel
> itself.
> 
> Btw I don't think ACPI boot is a use case here. I don't see a software
> framebuffer with no wifi support as a usage mode that justifies
> carrying EFI stub hacks for everyone.

Okay.  I'm fine to carry it as an out-of-tree patch until someday you
consider ACPI boot is useful for everyone.  But I do boot these laptops
with ACPI at daily basis right now as arm64 native build machine, with
USB Ethernet adapter.

Shawn

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

[Ard Biesheuvel]

On Tue, 9 Mar 2021 at 04:22, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Mon, Mar 08, 2021 at 02:34:48PM +0100, Ard Biesheuvel wrote:
> > On Sun, 7 Mar 2021 at 12:02, Shawn Guo <shawn.guo@linaro.org> wrote:
> > >
> > > On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> > > > Allow EFI systems to override the set of supported runtime services
> > > > declared via the RT_PROP table, by checking for the existence of a
> > > > 'OverrideSupported' EFI variable of the appropriate size under the
> > > > RT_PROP table GUID, and if it does, combine the supported mask using
> > > > logical AND. (This means the override can only remove support, not
> > > > add it back).
> > > >
> > > > Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > Cc: linux-arm-msm@vger.kernel.org
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:
> > >
> > > Tested-by: Shawn Guo <shawn.guo@linaro.org>
> > >
> > > With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
> > > 'efi=novamap' kernel cmdline and get around the broken poweroff runtime
> > > services nicely.  Thanks!
> > >
> >
> > Thanks for confirming.
> >
> > However, I am not going to merge this without some justification, and
> > hopefully some input from other folks (Leif?)
> >
> > RTPROP already provides what we need on all platforms that use
> > DtbLoader, and the patch for that is queued up for v5.12-rcX, with a
> > cc:stable to v5.10. This allows any RT service to be marked as
> > disabled, including SetVirtualAddressMap().
> >
> > So afaict, that means that this patch would be a special case for
> > Flex5G, right?
>
> It's for all Snapdragon based laptops, as we need to disable
> SetVirtualAddressMap runtime services on all of them.
>
> > So how are platforms such as this one going to load the
> > DTB? If some loader will be involved (or even just GRUB),
>
> Yes, GRUB.
>
> > shouldn't it
> > be that component that sets RTPROP like DtbLoader will, not the kernel
> > itself.
> >
> > Btw I don't think ACPI boot is a use case here. I don't see a software
> > framebuffer with no wifi support as a usage mode that justifies
> > carrying EFI stub hacks for everyone.
>
> Okay.  I'm fine to carry it as an out-of-tree patch until someday you
> consider ACPI boot is useful for everyone.  But I do boot these laptops
> with ACPI at daily basis right now as arm64 native build machine, with
> USB Ethernet adapter.
>

There may be several reasons why this patch might become worthwhile
for upstream, but until that moment, I'd rather not merge it, as it
will affect all users, including ones that boot with EFI secure boot
enabled. (I haven't quite convinced myself that disabling runtime
services arbitrarily using a EFI variable is not something that can be
abused)

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

[Rob Clark]

On Mon, Mar 8, 2021 at 7:22 PM Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Mon, Mar 08, 2021 at 02:34:48PM +0100, Ard Biesheuvel wrote:
> > On Sun, 7 Mar 2021 at 12:02, Shawn Guo <shawn.guo@linaro.org> wrote:
> > >
> > > On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> > > > Allow EFI systems to override the set of supported runtime services
> > > > declared via the RT_PROP table, by checking for the existence of a
> > > > 'OverrideSupported' EFI variable of the appropriate size under the
> > > > RT_PROP table GUID, and if it does, combine the supported mask using
> > > > logical AND. (This means the override can only remove support, not
> > > > add it back).
> > > >
> > > > Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > Cc: linux-arm-msm@vger.kernel.org
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:
> > >
> > > Tested-by: Shawn Guo <shawn.guo@linaro.org>
> > >
> > > With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
> > > 'efi=novamap' kernel cmdline and get around the broken poweroff runtime
> > > services nicely.  Thanks!
> > >
> >
> > Thanks for confirming.
> >
> > However, I am not going to merge this without some justification, and
> > hopefully some input from other folks (Leif?)
> >
> > RTPROP already provides what we need on all platforms that use
> > DtbLoader, and the patch for that is queued up for v5.12-rcX, with a
> > cc:stable to v5.10. This allows any RT service to be marked as
> > disabled, including SetVirtualAddressMap().
> >
> > So afaict, that means that this patch would be a special case for
> > Flex5G, right?
>
> It's for all Snapdragon based laptops, as we need to disable
> SetVirtualAddressMap runtime services on all of them.
>
> > So how are platforms such as this one going to load the
> > DTB? If some loader will be involved (or even just GRUB),
>
> Yes, GRUB.
>
> > shouldn't it
> > be that component that sets RTPROP like DtbLoader will, not the kernel
> > itself.
> >
> > Btw I don't think ACPI boot is a use case here. I don't see a software
> > framebuffer with no wifi support as a usage mode that justifies
> > carrying EFI stub hacks for everyone.
>
> Okay.  I'm fine to carry it as an out-of-tree patch until someday you
> consider ACPI boot is useful for everyone.  But I do boot these laptops
> with ACPI at daily basis right now as arm64 native build machine, with
> USB Ethernet adapter.

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

BR,
-R

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

[Ard Biesheuvel]

On Tue, 9 Mar 2021 at 19:10, Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Mar 8, 2021 at 7:22 PM Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > On Mon, Mar 08, 2021 at 02:34:48PM +0100, Ard Biesheuvel wrote:
> > > On Sun, 7 Mar 2021 at 12:02, Shawn Guo <shawn.guo@linaro.org> wrote:
> > > >
> > > > On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> > > > > Allow EFI systems to override the set of supported runtime services
> > > > > declared via the RT_PROP table, by checking for the existence of a
> > > > > 'OverrideSupported' EFI variable of the appropriate size under the
> > > > > RT_PROP table GUID, and if it does, combine the supported mask using
> > > > > logical AND. (This means the override can only remove support, not
> > > > > add it back).
> > > > >
> > > > > Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > Cc: linux-arm-msm@vger.kernel.org
> > > > >
> > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > >
> > > > Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:
> > > >
> > > > Tested-by: Shawn Guo <shawn.guo@linaro.org>
> > > >
> > > > With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
> > > > 'efi=novamap' kernel cmdline and get around the broken poweroff runtime
> > > > services nicely.  Thanks!
> > > >
> > >
> > > Thanks for confirming.
> > >
> > > However, I am not going to merge this without some justification, and
> > > hopefully some input from other folks (Leif?)
> > >
> > > RTPROP already provides what we need on all platforms that use
> > > DtbLoader, and the patch for that is queued up for v5.12-rcX, with a
> > > cc:stable to v5.10. This allows any RT service to be marked as
> > > disabled, including SetVirtualAddressMap().
> > >
> > > So afaict, that means that this patch would be a special case for
> > > Flex5G, right?
> >
> > It's for all Snapdragon based laptops, as we need to disable
> > SetVirtualAddressMap runtime services on all of them.
> >
> > > So how are platforms such as this one going to load the
> > > DTB? If some loader will be involved (or even just GRUB),
> >
> > Yes, GRUB.
> >
> > > shouldn't it
> > > be that component that sets RTPROP like DtbLoader will, not the kernel
> > > itself.
> > >
> > > Btw I don't think ACPI boot is a use case here. I don't see a software
> > > framebuffer with no wifi support as a usage mode that justifies
> > > carrying EFI stub hacks for everyone.
> >
> > Okay.  I'm fine to carry it as an out-of-tree patch until someday you
> > consider ACPI boot is useful for everyone.  But I do boot these laptops
> > with ACPI at daily basis right now as arm64 native build machine, with
> > USB Ethernet adapter.
>
> 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

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.

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

[Rob Clark]

On Tue, Mar 9, 2021 at 10:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 9 Mar 2021 at 19:10, Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Mon, Mar 8, 2021 at 7:22 PM Shawn Guo <shawn.guo@linaro.org> wrote:
> > >
> > > On Mon, Mar 08, 2021 at 02:34:48PM +0100, Ard Biesheuvel wrote:
> > > > On Sun, 7 Mar 2021 at 12:02, Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > >
> > > > > On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> > > > > > Allow EFI systems to override the set of supported runtime services
> > > > > > declared via the RT_PROP table, by checking for the existence of a
> > > > > > 'OverrideSupported' EFI variable of the appropriate size under the
> > > > > > RT_PROP table GUID, and if it does, combine the supported mask using
> > > > > > logical AND. (This means the override can only remove support, not
> > > > > > add it back).
> > > > > >
> > > > > > Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > > Cc: linux-arm-msm@vger.kernel.org
> > > > > >
> > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > >
> > > > > Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:
> > > > >
> > > > > Tested-by: Shawn Guo <shawn.guo@linaro.org>
> > > > >
> > > > > With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
> > > > > 'efi=novamap' kernel cmdline and get around the broken poweroff runtime
> > > > > services nicely.  Thanks!
> > > > >
> > > >
> > > > Thanks for confirming.
> > > >
> > > > However, I am not going to merge this without some justification, and
> > > > hopefully some input from other folks (Leif?)
> > > >
> > > > RTPROP already provides what we need on all platforms that use
> > > > DtbLoader, and the patch for that is queued up for v5.12-rcX, with a
> > > > cc:stable to v5.10. This allows any RT service to be marked as
> > > > disabled, including SetVirtualAddressMap().
> > > >
> > > > So afaict, that means that this patch would be a special case for
> > > > Flex5G, right?
> > >
> > > It's for all Snapdragon based laptops, as we need to disable
> > > SetVirtualAddressMap runtime services on all of them.
> > >
> > > > So how are platforms such as this one going to load the
> > > > DTB? If some loader will be involved (or even just GRUB),
> > >
> > > Yes, GRUB.
> > >
> > > > shouldn't it
> > > > be that component that sets RTPROP like DtbLoader will, not the kernel
> > > > itself.
> > > >
> > > > Btw I don't think ACPI boot is a use case here. I don't see a software
> > > > framebuffer with no wifi support as a usage mode that justifies
> > > > carrying EFI stub hacks for everyone.
> > >
> > > Okay.  I'm fine to carry it as an out-of-tree patch until someday you
> > > consider ACPI boot is useful for everyone.  But I do boot these laptops
> > > with ACPI at daily basis right now as arm64 native build machine, with
> > > USB Ethernet adapter.
> >
> > 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
>
> 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.

Fair, it was less of an argument for/against the patch, and more just
reminding folks that there is an ACPI boot use case ;-)

BR,
-R

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

[Shawn Guo]

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:
> >
> > On Mon, Mar 8, 2021 at 7:22 PM Shawn Guo <shawn.guo@linaro.org> wrote:
> > >
> > > On Mon, Mar 08, 2021 at 02:34:48PM +0100, Ard Biesheuvel wrote:
> > > > On Sun, 7 Mar 2021 at 12:02, Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > >
> > > > > On Sat, Mar 06, 2021 at 12:35:19PM +0100, Ard Biesheuvel wrote:
> > > > > > Allow EFI systems to override the set of supported runtime services
> > > > > > declared via the RT_PROP table, by checking for the existence of a
> > > > > > 'OverrideSupported' EFI variable of the appropriate size under the
> > > > > > RT_PROP table GUID, and if it does, combine the supported mask using
> > > > > > logical AND. (This means the override can only remove support, not
> > > > > > add it back).
> > > > > >
> > > > > > Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
> > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > Cc: Shawn Guo <shawn.guo@linaro.org>
> > > > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > > Cc: linux-arm-msm@vger.kernel.org
> > > > > >
> > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > >
> > > > > Awesome, Ard!  On both Lenovo Yoga C630 and Flex 5G latops:
> > > > >
> > > > > Tested-by: Shawn Guo <shawn.guo@linaro.org>
> > > > >
> > > > > With 'OverrideSupported' EFI variable added from UEFI Shell, we can drop
> > > > > 'efi=novamap' kernel cmdline and get around the broken poweroff runtime
> > > > > services nicely.  Thanks!
> > > > >
> > > >
> > > > Thanks for confirming.
> > > >
> > > > However, I am not going to merge this without some justification, and
> > > > hopefully some input from other folks (Leif?)
> > > >
> > > > RTPROP already provides what we need on all platforms that use
> > > > DtbLoader, and the patch for that is queued up for v5.12-rcX, with a
> > > > cc:stable to v5.10. This allows any RT service to be marked as
> > > > disabled, including SetVirtualAddressMap().
> > > >
> > > > So afaict, that means that this patch would be a special case for
> > > > Flex5G, right?
> > >
> > > It's for all Snapdragon based laptops, as we need to disable
> > > SetVirtualAddressMap runtime services on all of them.
> > >
> > > > So how are platforms such as this one going to load the
> > > > DTB? If some loader will be involved (or even just GRUB),
> > >
> > > Yes, GRUB.
> > >
> > > > shouldn't it
> > > > be that component that sets RTPROP like DtbLoader will, not the kernel
> > > > itself.
> > > >
> > > > Btw I don't think ACPI boot is a use case here. I don't see a software
> > > > framebuffer with no wifi support as a usage mode that justifies
> > > > carrying EFI stub hacks for everyone.
> > >
> > > Okay.  I'm fine to carry it as an out-of-tree patch until someday you
> > > consider ACPI boot is useful for everyone.  But I do boot these laptops
> > > with ACPI at daily basis right now as arm64 native build machine, with
> > > USB Ethernet adapter.
> >
> > 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.

Shawn

[1] https://salsa.debian.org/installer-team/grub-installer/-/merge_requests/5

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

[Ard Biesheuvel]

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?

This is not only a problem on Snapdragon systems afaik - most Uboot
based arm64 systems booting via EFI are affected by this as well.

So it would be good if we could align with those folks, and maybe the
ones working on RISC-V as well, to see if we can get some consensus
around taking this approach.

For the folks newly cc'ed on this thread: full version here
https://lore.kernel.org/linux-arm-msm/20210306113519.294287-1-ardb@kernel.org/

Note that I am both the author of the patch, and the maintainer
pushing back on it. Please chime in if you think there are reasons why
we want this, something like this or nothing like this.

-- 
Ard.

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

[Heinrich Schuchardt]

On 3/15/21 2:07 PM, 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?
>
> This is not only a problem on Snapdragon systems afaik - most Uboot
> based arm64 systems booting via EFI are affected by this as well.
>
> So it would be good if we could align with those folks, and maybe the
> ones working on RISC-V as well, to see if we can get some consensus
> around taking this approach.
>
> For the folks newly cc'ed on this thread: full version here
> https://lore.kernel.org/linux-arm-msm/20210306113519.294287-1-ardb@kernel.org/
>
> Note that I am both the author of the patch, and the maintainer
> pushing back on it. Please chime in if you think there are reasons why
> we want this, something like this or nothing like this.
>
Hello Ard,

looking at this thread it is hard to understand why this patch should be
needed.

If an UEFI application does not want to consume a service, it can do so
without having to manipulate the RT properties table.

Which UEFI applications are broken? Why can't they be fixed instead of
patching the kernel?

Can we have complete descriptions of the deficiencies of the involved
applications. I saw GRUB and the Debian installer mentioned in the
thread. Are there others?

Best regards

Heinrich

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

[Shawn Guo]

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.

Shawn

[1] https://github.com/rhboot/efibootmgr/blob/master/src/efibootmgr.c#L1764

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

[Ard Biesheuvel]

On Tue, 16 Mar 2021 at 08:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/15/21 2:07 PM, 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?
> >
> > This is not only a problem on Snapdragon systems afaik - most Uboot
> > based arm64 systems booting via EFI are affected by this as well.
> >
> > So it would be good if we could align with those folks, and maybe the
> > ones working on RISC-V as well, to see if we can get some consensus
> > around taking this approach.
> >
> > For the folks newly cc'ed on this thread: full version here
> > https://lore.kernel.org/linux-arm-msm/20210306113519.294287-1-ardb@kernel.org/
> >
> > Note that I am both the author of the patch, and the maintainer
> > pushing back on it. Please chime in if you think there are reasons why
> > we want this, something like this or nothing like this.
> >
> Hello Ard,
>
> looking at this thread it is hard to understand why this patch should be
> needed.
>
> If an UEFI application does not want to consume a service, it can do so
> without having to manipulate the RT properties table.
>
> Which UEFI applications are broken? Why can't they be fixed instead of
> patching the kernel?
>
> Can we have complete descriptions of the deficiencies of the involved
> applications. I saw GRUB and the Debian installer mentioned in the
> thread. Are there others?
>

The problem is that the proprietary EDK2 / UEFI firmware on Qualcomm
Snapdragon based laptops that were built to run Windows does not
implement get/setvariable after ExitBootServices. Instead, every call
to any of the variable services returns with an EFI_UNSUPPORTED error.

The correct way to address this is a RT_PROP table that encodes this
behavior, and this is what we added in the special DtbLoader driver
that is used to boot Linux in DT mode (as the firmware only implements
ACPI support). So for systems that can/will run DtbLoader, the problem
is solved.

What remains is ACPI boot, or boot modes where DtbLoader does not
work. In those cases, it would be useful to have another way to convey
this information to the OS in a way that does not rely on the kernel
command line.

But thinking about this, perhaps we should be fixing this in
efibootmgr instead. EFI_UNSUPPORTED is a valid and documented return
code that conveys that the operation did not fail with an error, but
that efibootmgr is not supported to begin with on the platform in
question.

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

[Ard Biesheuvel]

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

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

[Ilias Apalodimas]

Hi Ard, 

On Tue, Mar 16, 2021 at 08:52:52AM +0100, Ard Biesheuvel wrote:
> On Tue, 16 Mar 2021 at 08:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 3/15/21 2:07 PM, 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?
> > >
> > > This is not only a problem on Snapdragon systems afaik - most Uboot
> > > based arm64 systems booting via EFI are affected by this as well.
> > >
> > > So it would be good if we could align with those folks, and maybe the
> > > ones working on RISC-V as well, to see if we can get some consensus
> > > around taking this approach.
> > >
> > > For the folks newly cc'ed on this thread: full version here
> > > https://lore.kernel.org/linux-arm-msm/20210306113519.294287-1-ardb@kernel.org/
> > >
> > > Note that I am both the author of the patch, and the maintainer
> > > pushing back on it. Please chime in if you think there are reasons why
> > > we want this, something like this or nothing like this.
> > >
> > Hello Ard,
> >
> > looking at this thread it is hard to understand why this patch should be
> > needed.
> >
> > If an UEFI application does not want to consume a service, it can do so
> > without having to manipulate the RT properties table.
> >
> > Which UEFI applications are broken? Why can't they be fixed instead of
> > patching the kernel?
> >
> > Can we have complete descriptions of the deficiencies of the involved
> > applications. I saw GRUB and the Debian installer mentioned in the
> > thread. Are there others?
> >
> 
> The problem is that the proprietary EDK2 / UEFI firmware on Qualcomm
> Snapdragon based laptops that were built to run Windows does not
> implement get/setvariable after ExitBootServices. Instead, every call
> to any of the variable services returns with an EFI_UNSUPPORTED error.
> 
> The correct way to address this is a RT_PROP table that encodes this
> behavior, and this is what we added in the special DtbLoader driver
> that is used to boot Linux in DT mode (as the firmware only implements
> ACPI support). So for systems that can/will run DtbLoader, the problem
> is solved.
> 
> What remains is ACPI boot, or boot modes where DtbLoader does not
> work. In those cases, it would be useful to have another way to convey
> this information to the OS in a way that does not rely on the kernel
> command line.
> 
> But thinking about this, perhaps we should be fixing this in
> efibootmgr instead. EFI_UNSUPPORTED is a valid and documented return
> code that conveys that the operation did not fail with an error, but
> that efibootmgr is not supported to begin with on the platform in
> question.

It all depends on how smart we want to make the efi stub. In essence
it's an OS loader, that we have complete control over and we can play tricks
on broken/incompatible firmwares, but is that what we want ? And if yes, were
do we draw the line of what we fix or not?

I think the current problem doesn't make a strong case to add such
functionality. U-Boot doesn't expose SetVariable at all, but even if it did
and returned EFI_UNSUPPORTED, I'd expect the consuming applications to handle
the error gracefully.  I mean why should we treat EFI_UNSUPPORTED differently
than EFI_DEVICE_ERROR or EFI_INVALID_PARAMETER (or all the allowed return
codes)?

Cheers
/Ilias

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

[Ard Biesheuvel]

On Tue, 16 Mar 2021 at 09:04, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Ard,
>
> On Tue, Mar 16, 2021 at 08:52:52AM +0100, Ard Biesheuvel wrote:
> > On Tue, 16 Mar 2021 at 08:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
...
> > > looking at this thread it is hard to understand why this patch should be
> > > needed.
> > >
> > > If an UEFI application does not want to consume a service, it can do so
> > > without having to manipulate the RT properties table.
> > >
> > > Which UEFI applications are broken? Why can't they be fixed instead of
> > > patching the kernel?
> > >
> > > Can we have complete descriptions of the deficiencies of the involved
> > > applications. I saw GRUB and the Debian installer mentioned in the
> > > thread. Are there others?
> > >
> >
> > The problem is that the proprietary EDK2 / UEFI firmware on Qualcomm
> > Snapdragon based laptops that were built to run Windows does not
> > implement get/setvariable after ExitBootServices. Instead, every call
> > to any of the variable services returns with an EFI_UNSUPPORTED error.
> >
> > The correct way to address this is a RT_PROP table that encodes this
> > behavior, and this is what we added in the special DtbLoader driver
> > that is used to boot Linux in DT mode (as the firmware only implements
> > ACPI support). So for systems that can/will run DtbLoader, the problem
> > is solved.
> >
> > What remains is ACPI boot, or boot modes where DtbLoader does not
> > work. In those cases, it would be useful to have another way to convey
> > this information to the OS in a way that does not rely on the kernel
> > command line.
> >
> > But thinking about this, perhaps we should be fixing this in
> > efibootmgr instead. EFI_UNSUPPORTED is a valid and documented return
> > code that conveys that the operation did not fail with an error, but
> > that efibootmgr is not supported to begin with on the platform in
> > question.
>
> It all depends on how smart we want to make the efi stub. In essence
> it's an OS loader, that we have complete control over and we can play tricks
> on broken/incompatible firmwares, but is that what we want ? And if yes, were
> do we draw the line of what we fix or not?
>
> I think the current problem doesn't make a strong case to add such
> functionality. U-Boot doesn't expose SetVariable at all, but even if it did
> and returned EFI_UNSUPPORTED, I'd expect the consuming applications to handle
> the error gracefully.  I mean why should we treat EFI_UNSUPPORTED differently
> than EFI_DEVICE_ERROR or EFI_INVALID_PARAMETER (or all the allowed return
> codes)?
>

EFI_DEVICE_ERROR or EFI_INVALID_PARAMETER means that the particular
call resulted in an error, which may be related to the values of the
arguments, the state of the the flash, etc etc

EFI_UNSUPPORTED means that the platform in question does not support
the routine at all at runtime, and the arguments or the context is
irrelevant.

Given that GRUB already tolerates the second condition, but only if it
is communicated explicitly (via --no-nvram) or implicitly when
efivarfs is absent altogether, I am saying that we should classify a
EFI_UNSUPPORTED return value in the same way, and tolerate it rather
than abort the install.

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

[Ilias Apalodimas]

On Tue, Mar 16, 2021 at 09:14:22AM +0100, Ard Biesheuvel wrote:
> On Tue, 16 Mar 2021 at 09:04, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Ard,
> >
> > On Tue, Mar 16, 2021 at 08:52:52AM +0100, Ard Biesheuvel wrote:
> > > On Tue, 16 Mar 2021 at 08:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> ...
> > > > looking at this thread it is hard to understand why this patch should be
> > > > needed.
> > > >
> > > > If an UEFI application does not want to consume a service, it can do so
> > > > without having to manipulate the RT properties table.
> > > >
> > > > Which UEFI applications are broken? Why can't they be fixed instead of
> > > > patching the kernel?
> > > >
> > > > Can we have complete descriptions of the deficiencies of the involved
> > > > applications. I saw GRUB and the Debian installer mentioned in the
> > > > thread. Are there others?
> > > >
> > >
> > > The problem is that the proprietary EDK2 / UEFI firmware on Qualcomm
> > > Snapdragon based laptops that were built to run Windows does not
> > > implement get/setvariable after ExitBootServices. Instead, every call
> > > to any of the variable services returns with an EFI_UNSUPPORTED error.
> > >
> > > The correct way to address this is a RT_PROP table that encodes this
> > > behavior, and this is what we added in the special DtbLoader driver
> > > that is used to boot Linux in DT mode (as the firmware only implements
> > > ACPI support). So for systems that can/will run DtbLoader, the problem
> > > is solved.
> > >
> > > What remains is ACPI boot, or boot modes where DtbLoader does not
> > > work. In those cases, it would be useful to have another way to convey
> > > this information to the OS in a way that does not rely on the kernel
> > > command line.
> > >
> > > But thinking about this, perhaps we should be fixing this in
> > > efibootmgr instead. EFI_UNSUPPORTED is a valid and documented return
> > > code that conveys that the operation did not fail with an error, but
> > > that efibootmgr is not supported to begin with on the platform in
> > > question.
> >
> > It all depends on how smart we want to make the efi stub. In essence
> > it's an OS loader, that we have complete control over and we can play tricks
> > on broken/incompatible firmwares, but is that what we want ? And if yes, were
> > do we draw the line of what we fix or not?
> >
> > I think the current problem doesn't make a strong case to add such
> > functionality. U-Boot doesn't expose SetVariable at all, but even if it did
> > and returned EFI_UNSUPPORTED, I'd expect the consuming applications to handle
> > the error gracefully.  I mean why should we treat EFI_UNSUPPORTED differently
> > than EFI_DEVICE_ERROR or EFI_INVALID_PARAMETER (or all the allowed return
> > codes)?
> >
> 
> EFI_DEVICE_ERROR or EFI_INVALID_PARAMETER means that the particular
> call resulted in an error, which may be related to the values of the
> arguments, the state of the the flash, etc etc
> 
> EFI_UNSUPPORTED means that the platform in question does not support
> the routine at all at runtime, and the arguments or the context is
> irrelevant.

By differently I implied 'not handle the error correctly'.
So my point was that an application must handle all errors that are allowed
from the spec. Not select the ones it prefers in a meaningfull way.
Which brings us to your next point.

> 
> Given that GRUB already tolerates the second condition, but only if it
> is communicated explicitly (via --no-nvram) or implicitly when
> efivarfs is absent altogether, I am saying that we should classify a
> EFI_UNSUPPORTED return value in the same way, and tolerate it rather
> than abort the install.

+1 

Thanks
/Ilias

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

[Shawn Guo]

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)

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

[Ard Biesheuvel]

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.

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

[Ilias Apalodimas]

Hi Shawn,

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

But the EFI_UNSUPPORTED is an error that's allowed from the spec. 
Yes the sane thing to do is not expose it at all, but it's not violating
any spec by doing so.
So why shouldn't a userspace application be able to handle all return codes
explicitly and instead treat them as a single error? And when that happens why
should the kernel mask that error out for it?

Thanks
/Ilias
> 
> 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)

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

[Heinrich Schuchardt]

On 16.03.21 10:33, Ilias Apalodimas wrote:
> Hi Shawn,
>
>>>>>>> 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,

No, there is not one but there are three EFI variable services.

GetVariable() available after ExitBootServices() and SetVariable() not
available() is completely legal according to the current UEFI specification.

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

Please, describe:

* Which UEFI version is reported by your Snapdragon laptop?
* What is the observed behavior?

>
> But the EFI_UNSUPPORTED is an error that's allowed from the spec.
> Yes the sane thing to do is not expose it at all, but it's not violating
> any spec by doing so.
> So why shouldn't a userspace application be able to handle all return codes
> explicitly and instead treat them as a single error? And when that happens why
> should the kernel mask that error out for it?

The runtime services must always be callable even they can only report
EFI_UNSUPPORTED.

Only since UEFI specification 2.8 errata B do we have the
EFI_RT_PROPERTIES_TABLE which tells us which runtime services are
implemented.

UEFI 2.8 B makes it clear that any runtime service may be reported as
unsupported. EFI applications are expected to cope with a service being
unavailable.

For embedded systems we have the EBBR specification which specifies that
the SetVariable() runtime service is not required to be implemented at
runtime
(https://github.com/ARM-software/ebbr/blob/main/source/chapter2-uefi.rst#uefi-runtime-services).
Ony SetVirtualAddressMap() and ConvertPointer() are required by the EBBR.

Software should not mess with EFI variables without explicit consent by
the user. In this respect GRUB is a real nuisance. Given a Windows
computer installing GRUB changes the BootOrder variable. This results in
a system that directly starts into GRUB instead of Windows. GRUB
provides a menu entry for Windows that cannot boot because it conflicts
with Windows measured boot.

Best regards

Heinrich

>
> Thanks
> /Ilias
>>
>> 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)

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

[Ard Biesheuvel]

On Tue, 16 Mar 2021 at 14:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 16.03.21 10:33, Ilias Apalodimas wrote:
> > Hi Shawn,
> >
> >>>>>>> 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,
>
> No, there is not one but there are three EFI variable services.
>
> GetVariable() available after ExitBootServices() and SetVariable() not
> available() is completely legal according to the current UEFI specification.
>

This is a valid point. efibootmgr may be able to read the Boot####
variables, but may be unable to change them.

> >> 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.
>
> Please, describe:
>
> * Which UEFI version is reported by your Snapdragon laptop?
> * What is the observed behavior?
>

These laptops have the EFI varstore in a eMMC partition. This is
basically the same problem that many uboot based platforms have as
well, i.e., that it is not possible for the OS and the firmware to
share the MMC because the ownership of the MMC controller cannot be
shared.

> >
> > But the EFI_UNSUPPORTED is an error that's allowed from the spec.
> > Yes the sane thing to do is not expose it at all, but it's not violating
> > any spec by doing so.
> > So why shouldn't a userspace application be able to handle all return codes
> > explicitly and instead treat them as a single error? And when that happens why
> > shouldut  the kernel mask that error out for it?
>
> The runtime services must always be callable even they can only report
> EFI_UNSUPPORTED.
>
> Only since UEFI specification 2.8 errata B do we have the
> EFI_RT_PROPERTIES_TABLE which tells us which runtime services are
> implemented.
>
> UEFI 2.8 B makes it clear that any runtime service may be reported as
> unsupported. EFI applications are expected to cope with a service being
> unavailable.
>

Indeed. The firmware on these machines predates the UEFI 2.8B
specification, but given the fact that EFI_UNSUPPORTED is a valid
return code now for these services, we should deal with them
gracefully anyway. And apparently, doing so would also remove the need
for special hacks to support installing GRUB on these platforms.

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

[Heinrich Schuchardt]

On 16.03.21 15:06, Ard Biesheuvel wrote:
> On Tue, 16 Mar 2021 at 14:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 16.03.21 10:33, Ilias Apalodimas wrote:
>>> Hi Shawn,
>>>
>>>>>>>>> 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,
>>
>> No, there is not one but there are three EFI variable services.
>>
>> GetVariable() available after ExitBootServices() and SetVariable() not
>> available() is completely legal according to the current UEFI specification.
>>
>
> This is a valid point. efibootmgr may be able to read the Boot####
> variables, but may be unable to change them.
>
>>>> 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.
>>
>> Please, describe:
>>
>> * Which UEFI version is reported by your Snapdragon laptop?
>> * What is the observed behavior?
>>
>
> These laptops have the EFI varstore in a eMMC partition. This is
> basically the same problem that many uboot based platforms have as
> well, i.e., that it is not possible for the OS and the firmware to
> share the MMC because the ownership of the MMC controller cannot be
> shared.
>
>>>
>>> But the EFI_UNSUPPORTED is an error that's allowed from the spec.
>>> Yes the sane thing to do is not expose it at all, but it's not violating
>>> any spec by doing so.
>>> So why shouldn't a userspace application be able to handle all return codes
>>> explicitly and instead treat them as a single error? And when that happens why
>>> shouldut  the kernel mask that error out for it?
>>
>> The runtime services must always be callable even they can only report
>> EFI_UNSUPPORTED.
>>
>> Only since UEFI specification 2.8 errata B do we have the
>> EFI_RT_PROPERTIES_TABLE which tells us which runtime services are
>> implemented.
>>
>> UEFI 2.8 B makes it clear that any runtime service may be reported as
>> unsupported. EFI applications are expected to cope with a service being
>> unavailable.
>>
>
> Indeed. The firmware on these machines predates the UEFI 2.8B
> specification, but given the fact that EFI_UNSUPPORTED is a valid
> return code now for these services, we should deal with them
> gracefully anyway. And apparently, doing so would also remove the need
> for special hacks to support installing GRUB on these platforms.
>

Hello Ard,

it is still unclear to why you would need the patch. Why should a user
provide a UEFI variable telling which service is not working correctly?

Is the firmware correctly returning EFI_UNSUPPORTED for unsupported
services?
For which services?

In which software is the bug that cannot gracefully deal with
unsupported services?

GRUB never gave me a problem on boards with U-Boot which only provides
GetVariable() and not SetVariable(). GRUB spits out warnings but works
as expected.

Best regards

Heinrich

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

[Ard Biesheuvel]

On Tue, 16 Mar 2021 at 15:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 16.03.21 15:06, Ard Biesheuvel wrote:
> > On Tue, 16 Mar 2021 at 14:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 16.03.21 10:33, Ilias Apalodimas wrote:
> >>> Hi Shawn,
> >>>
> >>>>>>>>> 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,
> >>
> >> No, there is not one but there are three EFI variable services.
> >>
> >> GetVariable() available after ExitBootServices() and SetVariable() not
> >> available() is completely legal according to the current UEFI specification.
> >>
> >
> > This is a valid point. efibootmgr may be able to read the Boot####
> > variables, but may be unable to change them.
> >
> >>>> 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.
> >>
> >> Please, describe:
> >>
> >> * Which UEFI version is reported by your Snapdragon laptop?
> >> * What is the observed behavior?
> >>
> >
> > These laptops have the EFI varstore in a eMMC partition. This is
> > basically the same problem that many uboot based platforms have as
> > well, i.e., that it is not possible for the OS and the firmware to
> > share the MMC because the ownership of the MMC controller cannot be
> > shared.
> >
> >>>
> >>> But the EFI_UNSUPPORTED is an error that's allowed from the spec.
> >>> Yes the sane thing to do is not expose it at all, but it's not violating
> >>> any spec by doing so.
> >>> So why shouldn't a userspace application be able to handle all return codes
> >>> explicitly and instead treat them as a single error? And when that happens why
> >>> shouldut  the kernel mask that error out for it?
> >>
> >> The runtime services must always be callable even they can only report
> >> EFI_UNSUPPORTED.
> >>
> >> Only since UEFI specification 2.8 errata B do we have the
> >> EFI_RT_PROPERTIES_TABLE which tells us which runtime services are
> >> implemented.
> >>
> >> UEFI 2.8 B makes it clear that any runtime service may be reported as
> >> unsupported. EFI applications are expected to cope with a service being
> >> unavailable.
> >>
> >
> > Indeed. The firmware on these machines predates the UEFI 2.8B
> > specification, but given the fact that EFI_UNSUPPORTED is a valid
> > return code now for these services, we should deal with them
> > gracefully anyway. And apparently, doing so would also remove the need
> > for special hacks to support installing GRUB on these platforms.
> >
>
> Hello Ard,
>
> it is still unclear to why you would need the patch. Why should a user
> provide a UEFI variable telling which service is not working correctly?
>

Why would it be the user setting this variable?

> Is the firmware correctly returning EFI_UNSUPPORTED for unsupported
> services?

Yes

> For which services?
>

All runtime services except SetVirtualAddressMap() and ResetSystem()

> In which software is the bug that cannot gracefully deal with
> unsupported services?
>

The debian installer gives up if it cannot set the boot path for GRUB.

> GRUB never gave me a problem on boards with U-Boot which only provides
> GetVariable() and not SetVariable(). GRUB spits out warnings but works
> as expected.
>
> Best regards
>
> Heinrich

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

[Heinrich Schuchardt]

On 16.03.21 15:55, Ard Biesheuvel wrote:
> On Tue, 16 Mar 2021 at 15:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 16.03.21 15:06, Ard Biesheuvel wrote:
>>> On Tue, 16 Mar 2021 at 14:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 16.03.21 10:33, Ilias Apalodimas wrote:
>>>>> Hi Shawn,
>>>>>
>>>>>>>>>>> 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,
>>>>
>>>> No, there is not one but there are three EFI variable services.
>>>>
>>>> GetVariable() available after ExitBootServices() and SetVariable() not
>>>> available() is completely legal according to the current UEFI specification.
>>>>
>>>
>>> This is a valid point. efibootmgr may be able to read the Boot####
>>> variables, but may be unable to change them.
>>>
>>>>>> 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.
>>>>
>>>> Please, describe:
>>>>
>>>> * Which UEFI version is reported by your Snapdragon laptop?
>>>> * What is the observed behavior?
>>>>
>>>
>>> These laptops have the EFI varstore in a eMMC partition. This is
>>> basically the same problem that many uboot based platforms have as
>>> well, i.e., that it is not possible for the OS and the firmware to
>>> share the MMC because the ownership of the MMC controller cannot be
>>> shared.
>>>
>>>>>
>>>>> But the EFI_UNSUPPORTED is an error that's allowed from the spec.
>>>>> Yes the sane thing to do is not expose it at all, but it's not violating
>>>>> any spec by doing so.
>>>>> So why shouldn't a userspace application be able to handle all return codes
>>>>> explicitly and instead treat them as a single error? And when that happens why
>>>>> shouldut  the kernel mask that error out for it?
>>>>
>>>> The runtime services must always be callable even they can only report
>>>> EFI_UNSUPPORTED.
>>>>
>>>> Only since UEFI specification 2.8 errata B do we have the
>>>> EFI_RT_PROPERTIES_TABLE which tells us which runtime services are
>>>> implemented.
>>>>
>>>> UEFI 2.8 B makes it clear that any runtime service may be reported as
>>>> unsupported. EFI applications are expected to cope with a service being
>>>> unavailable.
>>>>
>>>
>>> Indeed. The firmware on these machines predates the UEFI 2.8B
>>> specification, but given the fact that EFI_UNSUPPORTED is a valid
>>> return code now for these services, we should deal with them
>>> gracefully anyway. And apparently, doing so would also remove the need
>>> for special hacks to support installing GRUB on these platforms.
>>>
>>
>> Hello Ard,
>>
>> it is still unclear to why you would need the patch. Why should a user
>> provide a UEFI variable telling which service is not working correctly?
>>
>
> Why would it be the user setting this variable?
>
>> Is the firmware correctly returning EFI_UNSUPPORTED for unsupported
>> services?
>
> Yes
>
>> For which services?
>>
>
> All runtime services except SetVirtualAddressMap() and ResetSystem()
>
>> In which software is the bug that cannot gracefully deal with
>> unsupported services?
>>
>
> The debian installer gives up if it cannot set the boot path for GRUB.

The installer should simply open a message box asking the user to set up
a boot option for shimaa64.efi (or grubaa64.efi).

This is nothing that can be fixed in the Linux kernel.

Best regards

Heinrich

>
>> GRUB never gave me a problem on boards with U-Boot which only provides
>> GetVariable() and not SetVariable(). GRUB spits out warnings but works
>> as expected.
>>
>> Best regards
>>
>> Heinrich

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

[Shawn Guo]

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

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@linaro.org>
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@linaro.org>
---
 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

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

[Ard Biesheuvel]

On Wed, 17 Mar 2021 at 07:36, Shawn Guo <shawn.guo@linaro.org> 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@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.
>
> 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@linaro.org>
> 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@linaro.org>

Yes, this makes sense.

Acked-by: Ard Biesheuvel <ardb@kernel.org>

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
>