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

[norbert.kaminski@xxxxxxxxx]

On 29.07.2020 00:16, Marek Marczykowski-Górecki wrote:
> 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<-----
I've built the kernel with your patch. Unfortunately it doesn't bring 
expected
sysfs directories. We still need some changes here.

---
Best Regards,
Norbert Kamiński
Embedded Systems Engineer
GPG key ID: 9E9F90AFE10F466A
3mdeb.com