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

[Ard Biesheuvel <ardb@kernel.org>]

On Tue, 16 Mar 2021 at 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.