Re: fwupd support under Xen - firmware updates with the UEFI capsule

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Jul 28, 2020 at 10:01:33PM +0100, Andrew Cooper wrote:
> On 28/07/2020 21:00, Jan Beulich wrote:
> > On 28.07.2020 09:41, Norbert Kaminski wrote:
> >> I'm trying to add support for the firmware updates with the UEFI
> >> capsule in
> >> Qubes OS. I've got the troubles with reading ESRT (EFI System
> >> Resource Table)
> >> in the dom0, which is based on the EFI memory map. The EFI_MEMMAP is not
> >> enabled despite the loaded drivers (CONFIG_EFI, CONFIG_EFI_ESRT) and
> >> kernel
> >> cmdline parameters (add_efi_memmap):
> >>
> >> ```
> >> [    3.451249] efi: EFI_MEMMAP is not enabled.
> >> ```
> >
> > It is, according to my understanding, a layering violation to expose
> > the EFI memory map to Dom0. It's not supposed to make use of this
> > information in any way. Hence any functionality depending on its use
> > also needs to be implemented in the hypervisor, with Dom0 making a
> > suitable hypercall to access this functionality. (And I find it
> > quite natural to expect that Xen gets involved in an update of the
> > firmware of a system.)
> 
> ERST is a table (read only by the looks of things) which is a catalogue
> of various bits of firmware in the system, including GUIDs for
> identification, and version information.
> 
> It is the kind of data which the hardware domain should have access to,
> and AFAICT, behaves just like a static ACPI table.
> 
> Presumably it wants to an E820 reserved region so dom0 gets indent
> access, and something in the EFI subsystem needs extending to pass the
> ERST address to dom0.

I think most (if not all) pieces in Xen are already there - there is
XENPF_firmware_info with XEN_EFW_EFI_INFO + XEN_FW_EFI_CONFIG_TABLE
that gives address of the EFI config table. Linux saves it in
efi_systab_xen.tables (arch/x86/xen/efi.c:xen_efi_probe().
I haven't figured out yet if it does anything with that information, but
the content of /sys/firmware/efi/systab suggests it does.

It seems ESRT driver in Linux uses memmap just for some sanity checks
(if the ESRT points at memory with EFI_MEMORY_RUNTIME and appropriate
type). Perhaps the check (if really necessary) can be added to Xen and
in case of dom0 simply skipped in Linux.

Norbert, if you're brave enough ;) I would suggests trying the (Linux)
patch below:

-----8<-----
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index e3d692696583..a2a5ccbb00a8 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -245,13 +245,14 @@ void __init efi_esrt_init(void)
        int rc;
        phys_addr_t end;
 
-       if (!efi_enabled(EFI_MEMMAP))
+       if (!efi_enabled(EFI_MEMMAP) && !efi_enabled(EFI_PARAVIRT))
                return;
 
        pr_debug("esrt-init: loading.\n");
        if (!esrt_table_exists())
                return;
 
+       if (!efi_enabled(EFI_PARAVIRT)) {
        rc = efi_mem_desc_lookup(efi.esrt, &md);
        if (rc < 0 ||
            (!(md.attribute & EFI_MEMORY_RUNTIME) &&
@@ -276,6 +277,7 @@ void __init efi_esrt_init(void)
                       size, max);
                return;
        }
+       }
 
        va = early_memremap(efi.esrt, size);
        if (!va) {
@@ -331,7 +333,8 @@ void __init efi_esrt_init(void)
 
        end = esrt_data + size;
        pr_info("Reserving ESRT space from %pa to %pa.\n", &esrt_data, &end);
-       if (md.type == EFI_BOOT_SERVICES_DATA)
+
+       if (!efi_enabled(EFI_PARAVIRT) && md.type == EFI_BOOT_SERVICES_DATA)
                efi_mem_reserve(esrt_data, esrt_data_size);
 
        pr_debug("esrt-init: loaded.\n");
----8<-----


-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature