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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 3673 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]