Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Demi Marie Obenour <demi@invisiblethingslab.com>]

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

On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> This means that some sanity checks we would like to perform on
> configuration tables or other data structures in memory are not
> currently possible. Xen does, however, expose EFI memory descriptor info
> via a Xen hypercall, so let's wire that up instead.
> 
> Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/efi.c |  5 ++-
>  drivers/xen/efi.c          | 34 ++++++++++++++++++++
>  include/linux/efi.h        |  1 +
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 55bd3f4aab28..2c12b1a06481 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
>   * and if so, populate the supplied memory descriptor with the appropriate
>   * data.
>   */
> -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  {
>  	efi_memory_desc_t *md;
>  
> @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  	return -ENOENT;
>  }
>  
> +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> +	 __weak __alias(__efi_mem_desc_lookup);
> +
>  /*
>   * Calculate the highest address of an efi memory descriptor.
>   */
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> index d1ff2186ebb4..74f3f6d8cdc8 100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -26,6 +26,7 @@
>  
>  #include <xen/interface/xen.h>
>  #include <xen/interface/platform.h>
> +#include <xen/page.h>
>  #include <xen/xen.h>
>  #include <xen/xen-ops.h>
>  
> @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
>  	efi.get_next_high_mono_count	= xen_efi_get_next_high_mono_count;
>  	efi.reset_system		= xen_efi_reset_system;
>  }
> +
> +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> +{
> +	static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> +		      "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> +	struct xen_platform_op op = {
> +		.cmd = XENPF_firmware_info,
> +		.u.firmware_info = {
> +			.type = XEN_FW_EFI_INFO,
> +			.index = XEN_FW_EFI_MEM_INFO,
> +			.u.efi_info.mem.addr = phys_addr,
> +			.u.efi_info.mem.size = U64_MAX - phys_addr,
> +		}
> +	};
> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +	int rc;
> +
> +	if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> +		return __efi_mem_desc_lookup(phys_addr, out_md);
> +
> +	rc = HYPERVISOR_platform_op(&op);
> +	if (rc) {
> +		pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> +			phys_addr, rc);
> +	}
> +
> +	out_md->phys_addr	= info->mem.addr;

This will be equal to phys_addr, not the actual start of the memory
region.

> +	out_md->num_pages	= info->mem.size >> EFI_PAGE_SHIFT;

Similarly, this will be the number of bytes in the memory region
after phys_addr, not the total number of bytes in the region.  These two
differences mean that this function is not strictly equivalent to the
original efi_mem_desc_lookup().

I am not sure if this matters in practice, but I thought you would want
to be aware of it.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]