| Message ID | 20221003112625.972646-6-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | efi/x86: Avoid corrupted config tables under Xen | expand |
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.
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? In any case, it means we'll need to round down phys_addr to page size at the very least.
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: > > > + 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? I don't think so, unfortunately. That said, with the below adjustment, I think that's okay for the _current_ users of efi_mem_desc_lookup(). > In any case, it means we'll need to round down phys_addr to page size > at the very least.
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?
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?
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.
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?
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.
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. Done: https://github.com/QubesOS/qubes-linux-kernel/pull/681 > AFAIR your test laptop has it too. It does; I plan to test a version that has the needed rounding.
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).
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; + out_md->num_pages = info->mem.size >> EFI_PAGE_SHIFT; + out_md->type = info->mem.type; + out_md->attribute = info->mem.attr; + + return 0; +} diff --git a/include/linux/efi.h b/include/linux/efi.h index 256e70e42114..e0ee6f6da4b4 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -731,6 +731,7 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size); extern int __init efi_uart_console_only (void); extern u64 efi_mem_desc_end(efi_memory_desc_t *md); extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md); +extern int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md); extern void efi_mem_reserve(phys_addr_t addr, u64 size); extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size); extern void efi_initialize_iomem_resources(struct resource *code_resource,