| Message ID | 20220825215218.1606-1-demi@invisiblethingslab.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for ESRT loading under Xen | expand |
On 25.08.2022 23:52, Demi Marie Obenour wrote: > @@ -40,6 +41,38 @@ > > #define efi_data(op) (op.u.efi_runtime_call) > > +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT, > + "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT"); > + > +bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *md) > +{ > + 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)-1ULL) - phys_addr, > + } > + }; > + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info; > + int rc; > + > + memset(md, 0, sizeof(*md)); /* initialize md even on failure */ > + rc = HYPERVISOR_platform_op(&op); > + if (rc) { > + pr_warn("Could not obtain information on address %llu from Xen: " > + "error %d\n", phys_addr, rc); > + return false; > + } > + > + md->attribute = info->mem.attr; > + md->type = info->mem.type; > + md->num_pages = info->mem.size >> XEN_PAGE_SHIFT; > + md->phys_addr = info->mem.addr; As indicated in reply to your patch changing XEN_FW_EFI_MEM_INFO in the hypervisor: While this may fit the ESRT purpose, the address you return here is not necessarily the start of the region, and hence this function is not a general Xen replacement for the non-Xen function. Therefore I think it also shouldn't give the impression of doing so. Jan
On Fri, Aug 26, 2022 at 09:53:29AM +0200, Jan Beulich wrote: > On 25.08.2022 23:52, Demi Marie Obenour wrote: > > @@ -40,6 +41,38 @@ > > > > #define efi_data(op) (op.u.efi_runtime_call) > > > > +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT, > > + "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT"); > > + > > +bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *md) > > +{ > > + 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)-1ULL) - phys_addr, > > + } > > + }; > > + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info; > > + int rc; > > + > > + memset(md, 0, sizeof(*md)); /* initialize md even on failure */ > > + rc = HYPERVISOR_platform_op(&op); > > + if (rc) { > > + pr_warn("Could not obtain information on address %llu from Xen: " > > + "error %d\n", phys_addr, rc); > > + return false; > > + } > > + > > + md->attribute = info->mem.attr; > > + md->type = info->mem.type; > > + md->num_pages = info->mem.size >> XEN_PAGE_SHIFT; > > + md->phys_addr = info->mem.addr; > > As indicated in reply to your patch changing XEN_FW_EFI_MEM_INFO in > the hypervisor: While this may fit the ESRT purpose, the address you > return here is not necessarily the start of the region, and hence > this function is not a general Xen replacement for the non-Xen > function. Therefore I think it also shouldn't give the impression of > doing so. Is this just a matter of renaming the function? Is it possible to implement the original function with the current hypervisor?
On 26.08.2022 20:01, Demi Marie Obenour wrote: > On Fri, Aug 26, 2022 at 09:53:29AM +0200, Jan Beulich wrote: >> On 25.08.2022 23:52, Demi Marie Obenour wrote: >>> @@ -40,6 +41,38 @@ >>> >>> #define efi_data(op) (op.u.efi_runtime_call) >>> >>> +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT, >>> + "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT"); >>> + >>> +bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *md) >>> +{ >>> + 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)-1ULL) - phys_addr, >>> + } >>> + }; >>> + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info; >>> + int rc; >>> + >>> + memset(md, 0, sizeof(*md)); /* initialize md even on failure */ >>> + rc = HYPERVISOR_platform_op(&op); >>> + if (rc) { >>> + pr_warn("Could not obtain information on address %llu from Xen: " >>> + "error %d\n", phys_addr, rc); >>> + return false; >>> + } >>> + >>> + md->attribute = info->mem.attr; >>> + md->type = info->mem.type; >>> + md->num_pages = info->mem.size >> XEN_PAGE_SHIFT; >>> + md->phys_addr = info->mem.addr; >> >> As indicated in reply to your patch changing XEN_FW_EFI_MEM_INFO in >> the hypervisor: While this may fit the ESRT purpose, the address you >> return here is not necessarily the start of the region, and hence >> this function is not a general Xen replacement for the non-Xen >> function. Therefore I think it also shouldn't give the impression of >> doing so. > > Is this just a matter of renaming the function? Besides renaming the function perhaps it also shouldn't give the impression of being generally usable. I would expect it to be a static helper somewhere, or even be expanded inline. > Is it possible to > implement the original function with the current hypervisor? Yes, but doing so would be ugly: You'd need to "bisect" your way to the start of the region. As an aside (I think I did point this out before): Can you please adjust the way your mail program sends mails? When I respond to your mail (using Thunderbird), I find all the people previously on Cc on the To: list, while your address is lost. As indicated I believe this is a result of the Mail-Followup-To: tag your reply came with (and I further think that TB's treatment of that tag is a reasonable one, albeit perhaps there are other reasonable treatments as well; I am not aware of this tag having any formally specified treatment). Jan
On Tue, Sep 06, 2022 at 08:49:54AM +0200, Jan Beulich wrote: > On 26.08.2022 20:01, Demi Marie Obenour wrote: > > On Fri, Aug 26, 2022 at 09:53:29AM +0200, Jan Beulich wrote: > >> On 25.08.2022 23:52, Demi Marie Obenour wrote: > >>> @@ -40,6 +41,38 @@ > >>> > >>> #define efi_data(op) (op.u.efi_runtime_call) > >>> > >>> +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT, > >>> + "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT"); > >>> + > >>> +bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *md) > >>> +{ > >>> + 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)-1ULL) - phys_addr, > >>> + } > >>> + }; > >>> + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info; > >>> + int rc; > >>> + > >>> + memset(md, 0, sizeof(*md)); /* initialize md even on failure */ > >>> + rc = HYPERVISOR_platform_op(&op); > >>> + if (rc) { > >>> + pr_warn("Could not obtain information on address %llu from Xen: " > >>> + "error %d\n", phys_addr, rc); > >>> + return false; > >>> + } > >>> + > >>> + md->attribute = info->mem.attr; > >>> + md->type = info->mem.type; > >>> + md->num_pages = info->mem.size >> XEN_PAGE_SHIFT; > >>> + md->phys_addr = info->mem.addr; > >> > >> As indicated in reply to your patch changing XEN_FW_EFI_MEM_INFO in > >> the hypervisor: While this may fit the ESRT purpose, the address you > >> return here is not necessarily the start of the region, and hence > >> this function is not a general Xen replacement for the non-Xen > >> function. Therefore I think it also shouldn't give the impression of > >> doing so. > > > > Is this just a matter of renaming the function? > > Besides renaming the function perhaps it also shouldn't give the > impression of being generally usable. I would expect it to be a static > helper somewhere, or even be expanded inline. I would be fine with doing this, but I didn’t want to litter esrt.c with Xen-specific code. IIUC Linux prefers to avoid #ifdef in .c files. > > Is it possible to > > implement the original function with the current hypervisor? > > Yes, but doing so would be ugly: You'd need to "bisect" your way to > the start of the region. > > As an aside (I think I did point this out before): Can you please > adjust the way your mail program sends mails? When I respond to your > mail (using Thunderbird), I find all the people previously on Cc on > the To: list, while your address is lost. As indicated I believe > this is a result of the Mail-Followup-To: tag your reply came with > (and I further think that TB's treatment of that tag is a reasonable > one, albeit perhaps there are other reasonable treatments as well; I > am not aware of this tag having any formally specified treatment). This was a misconfiguration on my end: I marked xen-devel as subscribed in my muttrc. I fixed this and also unset followup_to, so the Mail-Followup-To header should no longer be generated. Please let me know if this is still a problem.
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c index 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..c0fc149a838044cc16bb08a374a0c8ea6b7dcbff 100644 --- a/drivers/firmware/efi/esrt.c +++ b/drivers/firmware/efi/esrt.c @@ -244,22 +244,36 @@ void __init efi_esrt_init(void) struct efi_system_resource_table tmpesrt; size_t size, max, entry_size, entries_size; efi_memory_desc_t md; - int rc; phys_addr_t end; - if (!efi_enabled(EFI_MEMMAP)) - 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"); + if (efi_enabled(EFI_MEMMAP)) { + if (efi_mem_desc_lookup(efi.esrt, &md) < 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; + } + } else if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVIRT)) { + if (!xen_efi_mem_desc_lookup(efi.esrt, &md)) { + pr_warn("Failed to lookup ESRT header in Xen memory map\n"); + return; + } + + /* Recent Xen versions relocate the ESRT to memory of type + * EfiRuntimeServicesData, which Xen will not reuse. If the ESRT + * is not in EfiRuntimeServicesData memory, it has not been reserved + * by Xen and might be allocated to other guests, so it cannot + * safely be used. */ + if (md.type != EFI_RUNTIME_SERVICES_DATA) { + pr_warn("Xen did not reserve ESRT, ignoring it\n"); + return; + } + } else { return; } diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c index d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..b313f213822f0fd5ba6448f6f6f453cfda4c7e23 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> @@ -40,6 +41,38 @@ #define efi_data(op) (op.u.efi_runtime_call) +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT, + "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT"); + +bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *md) +{ + 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)-1ULL) - phys_addr, + } + }; + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info; + int rc; + + memset(md, 0, sizeof(*md)); /* initialize md even on failure */ + rc = HYPERVISOR_platform_op(&op); + if (rc) { + pr_warn("Could not obtain information on address %llu from Xen: " + "error %d\n", phys_addr, rc); + return false; + } + + md->attribute = info->mem.attr; + md->type = info->mem.type; + md->num_pages = info->mem.size >> XEN_PAGE_SHIFT; + md->phys_addr = info->mem.addr; + return true; +} + static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) { struct xen_platform_op op = INIT_EFI_OP(get_time); diff --git a/include/linux/efi.h b/include/linux/efi.h index d2b84c2fec39f0268324d1a38a73ed67786973c9..0598869cdc924aef0e2b9cacc4450b728e1a98c7 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1327,1 +1327,11 @@ struct linux_efi_coco_secret_area { +#if IS_ENABLED(CONFIG_XEN_EFI) +extern bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md); +#else +static inline bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) +{ + BUILD_BUG(); + return false; +} +#endif + #endif /* _LINUX_EFI_H */
This is needed for fwupd to work in Qubes OS. Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> --- drivers/firmware/efi/esrt.c | 34 ++++++++++++++++++++++++---------- drivers/xen/efi.c | 33 +++++++++++++++++++++++++++++++++ include/linux/efi.h | 10 ++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-)