Re: [PATCH v9] Preserve the EFI System Resource Table for dom0

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jul 11, 2022 at 08:41:59AM +0200, Jan Beulich wrote:
> On 08.07.2022 23:34, Demi Marie Obenour wrote:
> > The EFI System Resource Table (ESRT) is necessary for fwupd to identify
> > firmware updates to install.  According to the UEFI specification §23.4,
> > the ESRT shall be stored in memory of type EfiBootServicesData.  However,
> > memory of type EfiBootServicesData is considered general-purpose memory
> > by Xen, so the ESRT needs to be moved somewhere where Xen will not
> > overwrite it.  Copy the ESRT to memory of type EfiRuntimeServicesData,
> > which Xen will not reuse.  dom0 can use the ESRT if (and only if) it is
> > in memory of type EfiRuntimeServicesData.
> > 
> > Earlier versions of this patch reserved the memory in which the ESRT was
> > located.  This created awkward alignment problems, and required either
> > splitting the E820 table or wasting memory.  It also would have required
> > a new platform op for dom0 to use to indicate if the ESRT is reserved.
> > By copying the ESRT into EfiRuntimeServicesData memory, the E820 table
> > does not need to be modified, and dom0 can just check the type of the
> > memory region containing the ESRT.  The copy is only done if the ESRT is
> > not already in EfiRuntimeServicesData memory, avoiding memory leaks on
> > repeated kexec.
> > 
> > See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
> > for details.  Tested using qemu-system-x86_64 and xen.efi.
> > 
> > Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  xen/common/efi/boot.c | 134 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 134 insertions(+)
> > 
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index a25e1d29f1..d2f66a430d 100644
> 
> As asked for before - please have a brief log of changes between
> versions (at least for the delta from the previous version) somewhere
> above from here.

Sorry about that, will fix in v10.

> The patch looks almost okay to me (see below), but I'd like to ask
> for explicit confirmation that this time round you actually did
> test the change at least on one architecture (presumably x86). Iirc
> one of the Arm folks offered to test it on Arm64 - would have been
> nice if you had included whoever it was in Cc.

I did, though it was in emulated QEMU (in full instruction emulation
mode, no hardware virt).  I had to disable EFI Runtime Services to avoid
a fatal page fault (seemingly in the OVMF firmware), but this is
unrelated as the same problem happens on master.  Also, I killed the VM
after systemd started and seemed to be running normally, on the ground
that any problem caused by this patch would have triggered earlier.

That said, this seems to cause another problem on a rebased patch.  I
will fix it after some sleep.

> > @@ -1051,6 +1110,70 @@ static void __init 
> > efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
> >  #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
> >                                   (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
> >  
> > +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> > +{
> > +    EFI_STATUS status;
> > +    UINTN info_size = 0, map_key, mdesc_size;
> > +    void *memory_map = NULL;
> > +    UINT32 ver;
> > +    unsigned int i;
> > +
> > +    for ( ; ; ) {
> 
> I'm sorry, but I'm now going to be picky and _not_ offer correcting
> this style violation for you anymore.

No apology needed.  Is the problem that the brace should be on a
separate line, that the spaces around the semicolons should be removed,
or both?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature