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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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