Re: [PATCH v3] Support ESRT in Xen dom0

[Jan Beulich]

On 20.09.2022 18:09, Ard Biesheuvel wrote:
> On Tue, 20 Sept 2022 at 17:54, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 20.09.2022 17:36, Ard Biesheuvel wrote:
>>> On Mon, 19 Sept 2022 at 21:33, Demi Marie Obenour
>>> <demi@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>>>>
>>>> fwupd requires access to the EFI System Resource Table (ESRT) to
>>>> discover which firmware can be updated by the OS.  Currently, Linux does
>>>> not expose the ESRT when running as a Xen dom0.  Therefore, it is not
>>>> possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
>>>> Qubes OS.
>>>>
>>>> Before Xen 4.16, this was not fixable due to hypervisor limitations.
>>>> The UEFI specification requires the ESRT to be in EfiBootServicesData
>>>> memory, which Xen will use for whatever purposes it likes.  Therefore,
>>>> Linux cannot safely access the ESRT, as Xen may have overwritten it.
>>>>
>>>> Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
>>>> or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
>>>> memory, Xen allocates some memory of type EfiRuntimeServicesData, copies
>>>> the ESRT to it, and finally replaces the ESRT pointer with a pointer to
>>>> the copy.  Since Xen will not clobber EfiRuntimeServicesData memory,
>>>> this ensures that the ESRT can safely be accessed by the OS.  It is safe
>>>> to access the ESRT under Xen if, and only if, it is in memory of type
>>>> EfiRuntimeServicesData.
>>>>
>>>
>>> Thanks for the elaborate explanation. This is really helpful.
>>>
>>> So here, you are explaining that the only way for Xen to prevent
>>> itself from potentially clobbering the ESRT is by creating a
>>> completely new allocation?
>>
>> There are surely other ways, e.g. preserving BootServices* regions
>> alongside RuntimeServices* ones. But as the maintainer of the EFI
>> code in Xen I don't view this as a reasonable approach.
> 
> Why not?

Because it's against the intentions the EFI has (or at least had)
for this memory type. Much more than EfiAcpiReclaimMemory this
type is intended for use as ordinary RAM post-boot.

>>> TBH I still don't think this is a scalable approach. There are other
>>> configuration tables that may be passed in EFI boot services memory,
>>> and MS especially were pushing back in the UEFI forum on adding table
>>> types that were passed in anything other the EfiBootServicesData.
>>
>> Within Xen we might abstract the approach currently implemented in
>> case more such pieces of data appear.
>>
>> While I can easily believe MS might be advocating for this model,
>> I view it as problematic not only for Xen. How would you pass on
>> this information across kexec, for example, without introducing
>> further producer-consumer dependencies requiring separate protocols
>> to be followed?
>>
> 
> In this case, I don't think this is unreasonable for configuration
> tables, which only have a GUID and a base address. If the OS knows the
> GUID, and knows how to interpret the contents, it can decide for
> itself whether or not to preserve it. If it doesn't know the GUID, the
> memory is just treated as available memory [after EBS()]
> 
> I personally think reclaimable memory is more suitable for these
> cases, which is why I am willing to consider that as well. Note that
> the EFI spec now also mandates device trees on ARM to be passed via
> EfiAcpiReclaimMemory, simply because it is the memory type suitable
> for firmware tables that only the OS consumes.

We do preserve EfiAcpiReclaimMemory, for the simple reason that with
Xen "the OS" is ambiguous: Is that Xen or Dom0? Most of ACPI is
handled by Dom0, so we can't very well discard the data before Dom0
starts. (This then also matters for what you've said in the earlier
paragraph. In particular the sets of known GUIDs may be dissimilar
for Xen and the Dom0 kernel. Considering your other remark about
fragmentation you might agree that preserving in-place is not very
desirable.)

Especially with DT mandated to use EfiAcpiReclaimMemory I'm willing
to consider using that type for the storing of ESRT (and whatever
else may appear along these lines). Demi, you may want to check for
both types in your Linux side patch ...

Jan