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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

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

On Mon, Oct 03, 2022 at 08:01:03PM +0200, Marek Marczykowski-Górecki wrote:
> 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@invisiblethingslab.com> 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@invisiblethingslab.com> 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@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.
> > > > > >
> > > > > > 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.

I have tested the series as in
https://github.com/QubesOS/qubes-linux-kernel/pull/681 and it seems to
work great.
Note the series there differs from this thread, and is marked as "v3" - I
assume (but haven't verified) it has changes requested in this thread
applied. Demi, can you confirm? If so, you can probably send this v3,
and feel free to include my Tested-by (unless you make significant
changes, ofc).

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

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