Re: [PATCH] efi: discover ESRT table on Xen PV too

[Roger Pau Monné]

On Sun, Aug 16, 2020 at 02:19:49AM +0200, Marek Marczykowski-Górecki wrote:
> In case of Xen PV dom0, Xen passes along info about system tables (see
> arch/x86/xen/efi.c), but not the memory map from EFI.

I think that's because the memory map returned by
XENMEM_machine_memory_map is in e820 form, and doesn't contain the
required information about the EFI regions due to the translation done
by efi_arch_process_memory_map in Xen?

> This makes sense
> as it is Xen responsible for managing physical memory address space.
> In this case, it doesn't make sense to condition using ESRT table on
> availability of EFI memory map, as it isn't Linux kernel responsible for
> it.

PV dom0 is kind of special in that regard as it can create mappings to
(almost) any MMIO regions, and hence can change it's memory map
substantially.

> Skip this part on Xen PV (let Xen do the right thing if it deems
> necessary) and use ESRT table normally.

Maybe it would be better to introduce a new hypercall (or add a
parameter to XENMEM_machine_memory_map) in order to be able to fetch
the EFI memory map?

That should allow a PV dom0 to check the ESRT is correct and thus not
diverge from bate metal.

> 
> This is a requirement for using fwupd in PV dom0 to update UEFI using
> capsules.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/firmware/efi/esrt.c | 47 ++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index d5915272141f..5c49f2aaa4b1 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -245,36 +245,38 @@ 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;
>  
> -	rc = efi_mem_desc_lookup(efi.esrt, &md);
> -	if (rc < 0 ||
> -	    (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> -	     md.type != EFI_BOOT_SERVICES_DATA &&
> -	     md.type != EFI_RUNTIME_SERVICES_DATA)) {
> -		pr_warn("ESRT header is not in the memory map.\n");
> -		return;
> -	}
> +	if (efi_enabled(EFI_MEMMAP)) {
> +		rc = efi_mem_desc_lookup(efi.esrt, &md);
> +		if (rc < 0 ||
> +		    (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> +		     md.type != EFI_BOOT_SERVICES_DATA &&
> +		     md.type != EFI_RUNTIME_SERVICES_DATA)) {
> +			pr_warn("ESRT header is not in the memory map.\n");
> +			return;
> +		}

Here you blindly trust the data in the ESRT in the PV case, without
checking it matches the regions on the memory map, which could lead to
errors if ESRT turns to be wrong.

Thanks, Roger.