From: Heinrich Schuchardt <xypron.glpk@gmx.de>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>,
Shawn Guo <shawn.guo@linaro.org>,
Atish Patra <atish.patra@wdc.com>,
Steve McIntyre <steve@einval.com>,
Rob Clark <robdclark@gmail.com>,
linux-efi <linux-efi@vger.kernel.org>,
Jeffrey Hugo <jhugo@codeaurora.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Leif Lindholm <leif@nuviainc.com>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH] efi: stub: override RT_PROP table supported mask based on EFI variable
Date: Tue, 16 Mar 2021 15:45:02 +0100 [thread overview]
Message-ID: <a0383983-9845-24ea-4f78-dec390206c6c@gmx.de> (raw)
In-Reply-To: <CAMj1kXGGdVYVxaz6+P=Ao_ZnzAOQ15eq3USzq3td-hYpbGe+vQ@mail.gmail.com>
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
next prev parent reply other threads:[~2021-03-16 14:46 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-06 11:35 [PATCH] efi: stub: override RT_PROP table supported mask based on EFI variable Ard Biesheuvel
2021-03-07 11:02 ` Shawn Guo
2021-03-08 13:34 ` Ard Biesheuvel
2021-03-09 3:22 ` Shawn Guo
2021-03-09 8:51 ` Ard Biesheuvel
2021-03-09 18:13 ` Rob Clark
2021-03-09 18:47 ` Ard Biesheuvel
2021-03-09 21:19 ` Rob Clark
2021-03-15 3:11 ` Shawn Guo
2021-03-15 13:07 ` Ard Biesheuvel
2021-03-16 7:42 ` Heinrich Schuchardt
2021-03-16 7:52 ` Ard Biesheuvel
2021-03-16 8:04 ` Ilias Apalodimas
2021-03-16 8:14 ` Ard Biesheuvel
2021-03-16 8:27 ` Ilias Apalodimas
2021-03-16 7:52 ` Shawn Guo
2021-03-16 7:57 ` Ard Biesheuvel
2021-03-16 9:06 ` Shawn Guo
2021-03-16 9:33 ` Ard Biesheuvel
2021-03-17 6:36 ` Shawn Guo
2021-03-17 6:58 ` Ard Biesheuvel
2021-03-16 9:33 ` Ilias Apalodimas
2021-03-16 13:25 ` Heinrich Schuchardt
2021-03-16 14:06 ` Ard Biesheuvel
2021-03-16 14:45 ` Heinrich Schuchardt [this message]
2021-03-16 14:55 ` Ard Biesheuvel
2021-03-16 16:06 ` Heinrich Schuchardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a0383983-9845-24ea-4f78-dec390206c6c@gmx.de \
--to=xypron.glpk@gmx.de \
--cc=ardb@kernel.org \
--cc=atish.patra@wdc.com \
--cc=bjorn.andersson@linaro.org \
--cc=ilias.apalodimas@linaro.org \
--cc=jhugo@codeaurora.org \
--cc=leif@nuviainc.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-efi@vger.kernel.org \
--cc=robdclark@gmail.com \
--cc=shawn.guo@linaro.org \
--cc=steve@einval.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).