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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Oct 03, 2022 at 01:57:14PM -0400, Demi Marie Obenour wrote:
> On Mon, Oct 03, 2022 at 07:04:02PM +0200, Marek Marczykowski-Górecki wrote:
> > On Mon, Oct 03, 2022 at 06:37:19PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
> > > <demi@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > > > > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> > > > > <demi@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > 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@xxxxxxxxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > > > > > ---
> > > > > > >  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.
> > > > >
> > > > > This is a bit disappointing. Is there no way to obtain this
> > > > > information via a Xen hypercall?
> > > >
> > > > It is possible, but doing so is very complex (it essentially requires a
> > > > binary search).  This really should be fixed on the Xen side.
> > > >
> > > > > In any case, it means we'll need to round down phys_addr to page size
> > > > > at the very least.
> > > >
> > > > That makes sense.  Are there any callers that will be broken even with
> > > > this rounding?
> > > 
> > > As far as I can tell, it should work fine. The only thing to double
> > > check is whether we are not creating spurious error messages from
> > > efi_arch_mem_reserve() this way, but as far as I can tell, that should
> > > be fine too.
> > > 
> > > Is there anyone at your end that can give this a spin on an actual
> > > Xen/x86 system?
> > 
> > Demi, if you open a PR with this at
> > https://github.com/QubesOS/qubes-linux-kernel/pulls, I can run it
> > through our CI - (at least) one of the machines has ESRT table. AFAIR
> > your test laptop has it too.
> 
> Just this patch or the whole series?

Whole series.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature