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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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