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

[norbert.kaminski@3mdeb.com]

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