drivers/firmware/efi/esrt.c | 34 ++++++++++++++++++++++++---------- drivers/xen/efi.c | 33 +++++++++++++++++++++++++++++++++ include/linux/efi.h | 10 ++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-)
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(-)
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 */
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things LabOn 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?
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
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.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
© 2016 - 2024 Red Hat, Inc.