Add core EFI boot services code to preserve BGRT (Boot Graphics Resource
Table) images during Xen boot. The BGRT contains a pointer to a boot logo
stored in BootServicesData memory. Without preservation, this memory is
reclaimed causing ACPI checksum errors in dom0.
Implementation:
- Walk XSDT to locate BGRT table (reusing efi.acpi20 from efi_tables())
- Validate BMP image signature and size constraints (max 16MB)
- Allocate EfiACPIReclaimMemory and copy image data
- Update BGRT table with new address and recalculate checksum
The preservation follows the ESRT pattern, running before
ExitBootServices() to ensure image remains accessible.
Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>
---
xen/arch/x86/efi/efi-boot.h | 2 +
xen/common/efi/boot.c | 133 +++++++++++++++++++++++++++++++++++
xen/common/efi/common-stub.c | 1 +
3 files changed, 136 insertions(+)
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 42a2c46b5e..0547d845cd 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -910,6 +910,8 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle,
efi_relocate_esrt(SystemTable);
+ efi_preserve_bgrt_img();
+
efi_exit_boot(ImageHandle, SystemTable);
}
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 967094994d..47d5b9b2a8 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1,12 +1,16 @@
#include "efi.h"
#include <efi/efiprot.h>
#include <efi/efipciio.h>
+#include <acpi/acconfig.h>
+#include <acpi/actbl.h>
+#include <acpi/actbl3.h>
#include <public/xen.h>
#include <xen/bitops.h>
#include <xen/compile.h>
#include <xen/ctype.h>
#include <xen/dmi.h>
#include <xen/domain_page.h>
+#include <xen/errno.h>
#include <xen/init.h>
#include <xen/keyhandler.h>
#include <xen/lib.h>
@@ -747,6 +751,133 @@ static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
efi_bs->FreePool(memory_map);
}
+typedef struct {
+ UINT16 signature;
+ UINT32 file_size;
+ UINT16 reserved[2];
+ UINT32 data_offset;
+} __attribute__((packed)) BMP_HEADER;
+
+static __initdata struct {
+ bool preserved;
+ const void *old_addr;
+ const void *new_addr;
+ UINTN size;
+ const char *failure_reason;
+} bgrt_info = {
+ /* We would prefer the failure_reason to print */
+ .failure_reason = "",
+};
+
+static struct acpi_table_bgrt *__init efi_get_bgrt(void)
+{
+ const struct acpi_table_rsdp *rsdp;
+ const struct acpi_table_xsdt *xsdt;
+ UINTN entry_count;
+ unsigned int i;
+
+ if ( efi.acpi20 == EFI_INVALID_TABLE_ADDR )
+ return NULL;
+
+ rsdp = (const void *)(UINTN)efi.acpi20;
+ if ( !rsdp || !rsdp->xsdt_physical_address )
+ return NULL;
+
+ xsdt = (const void *)rsdp->xsdt_physical_address;
+
+ if ( memcmp(xsdt->header.signature, ACPI_SIG_XSDT, 4) != 0 )
+ return NULL;
+
+ if ( xsdt->header.length < sizeof(xsdt->header) )
+ return NULL;
+ entry_count = (xsdt->header.length - sizeof(xsdt->header)) /
+ sizeof(xsdt->table_offset_entry[0]);
+
+ for ( i = 0; i < entry_count; i++ )
+ {
+ const struct acpi_table_header *hdr;
+
+ hdr = (const void *)xsdt->table_offset_entry[i];
+ if ( !hdr )
+ continue;
+
+ if ( memcmp(hdr->signature, ACPI_SIG_BGRT, 4) == 0 &&
+ hdr->length >= sizeof(struct acpi_table_bgrt) )
+ return (struct acpi_table_bgrt *)hdr;
+ }
+
+ return NULL;
+}
+
+#define BMP_SIGNATURE 0x4D42
+#define MAX_BGRT_IMAGE_SIZE (16 * 1024 * 1024)
+
+static void __init efi_preserve_bgrt_img(void)
+{
+ struct acpi_table_bgrt *bgrt;
+ const BMP_HEADER *bmp;
+ const void *old_image;
+ void *new_image;
+ UINTN image_size;
+ EFI_STATUS status;
+ UINT8 checksum;
+ unsigned int i;
+
+ bgrt_info.preserved = false;
+
+ bgrt = efi_get_bgrt();
+ if ( !bgrt )
+ {
+ bgrt_info.failure_reason = "BGRT table not found";
+ return;
+ }
+
+ if ( !bgrt->image_address )
+ return;
+
+ old_image = (const void *)bgrt->image_address;
+ bmp = old_image;
+
+ if ( bmp->signature != BMP_SIGNATURE )
+ {
+ bgrt_info.failure_reason = "Invalid BMP signature";
+ return;
+ }
+
+ image_size = bmp->file_size;
+ if ( !image_size || image_size > MAX_BGRT_IMAGE_SIZE )
+ {
+ bgrt_info.failure_reason = "Image size exceeds limit";
+ return;
+ }
+
+ /*
+ * Allocate memory of type EfiACPIReclaimMemory so that the image
+ * will remain available for the OS after ExitBootServices().
+ */
+ status = efi_bs->AllocatePool(EfiACPIReclaimMemory, image_size, &new_image);
+ if ( EFI_ERROR(status) )
+ {
+ bgrt_info.failure_reason = "Memory allocation failed";
+ return;
+ }
+ memcpy(new_image, old_image, image_size);
+ bgrt->image_address = (UINTN)new_image;
+ bgrt->header.checksum = 0;
+ checksum = 0;
+
+ for ( i = 0; i < bgrt->header.length; i++ )
+ checksum += ((const UINT8 *)bgrt)[i];
+
+ bgrt->header.checksum = -checksum;
+
+ /* Filling the debug struct for printing later */
+ bgrt_info.preserved = true;
+ bgrt_info.old_addr = old_image;
+ bgrt_info.new_addr = new_image;
+ bgrt_info.size = image_size;
+}
+
/*
* Include architecture specific implementation here, which references the
* static globals defined above.
@@ -1671,6 +1802,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
efi_relocate_esrt(SystemTable);
+ efi_preserve_bgrt_img();
+
efi_exit_boot(ImageHandle, SystemTable);
efi_arch_post_exit_boot(); /* Doesn't return. */
diff --git a/xen/common/efi/common-stub.c b/xen/common/efi/common-stub.c
index 9dc8aa538c..155a973f21 100644
--- a/xen/common/efi/common-stub.c
+++ b/xen/common/efi/common-stub.c
@@ -19,6 +19,7 @@ unsigned long efi_get_time(void)
}
void efi_reset_system(bool warm) { }
+void __init efi_bgrt_status_info(void) { }
int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
{
--
2.53.0On 24.03.2026 13:33, Soumyajyotii Ssarkar wrote:
> @@ -747,6 +751,133 @@ static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> efi_bs->FreePool(memory_map);
> }
>
> +typedef struct {
> + UINT16 signature;
> + UINT32 file_size;
> + UINT16 reserved[2];
> + UINT32 data_offset;
> +} __attribute__((packed)) BMP_HEADER;
> +
> +static __initdata struct {
> + bool preserved;
> + const void *old_addr;
> + const void *new_addr;
> + UINTN size;
> + const char *failure_reason;
> +} bgrt_info = {
> + /* We would prefer the failure_reason to print */
> + .failure_reason = "",
> +};
Noticed only while looking at patch 3: With this initializer, ...
> +static void __init efi_preserve_bgrt_img(void)
> +{
> + struct acpi_table_bgrt *bgrt;
> + const BMP_HEADER *bmp;
> + const void *old_image;
> + void *new_image;
> + UINTN image_size;
> + EFI_STATUS status;
> + UINT8 checksum;
> + unsigned int i;
> +
> + bgrt_info.preserved = false;
... why would this be needed?
Jan
On 24.03.2026 13:33, Soumyajyotii Ssarkar wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1,12 +1,16 @@
> #include "efi.h"
> #include <efi/efiprot.h>
> #include <efi/efipciio.h>
> +#include <acpi/acconfig.h>
> +#include <acpi/actbl.h>
> +#include <acpi/actbl3.h>
I fear this is pretty fragile. acpi/acpi.h is what is supposed to be included
from outside of (the little bit of) ACPICA code that we have got. Which in
turn may conflict with efi/*.h, which is why previously I had suggested to put
the ACPI parsing code elsewhere (if indeed nothing we already have can be
reused).
> #include <public/xen.h>
> #include <xen/bitops.h>
> #include <xen/compile.h>
> #include <xen/ctype.h>
> #include <xen/dmi.h>
> #include <xen/domain_page.h>
> +#include <xen/errno.h>
Is this really needed? Afaict it's included implicitly already.
> @@ -747,6 +751,133 @@ static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> efi_bs->FreePool(memory_map);
> }
>
> +typedef struct {
> + UINT16 signature;
> + UINT32 file_size;
> + UINT16 reserved[2];
> + UINT32 data_offset;
> +} __attribute__((packed)) BMP_HEADER;
All capitals identifiers are by convention #define-s. bmp_header_t perhaps?
> +static __initdata struct {
> + bool preserved;
> + const void *old_addr;
> + const void *new_addr;
> + UINTN size;
> + const char *failure_reason;
> +} bgrt_info = {
> + /* We would prefer the failure_reason to print */
> + .failure_reason = "",
> +};
I don't understand what the comment is supposed to be telling the reader.
Clearly it's not about the relocation subtlety which (imo) absolutely
needs commenting on.
> +static struct acpi_table_bgrt *__init efi_get_bgrt(void)
> +{
> + const struct acpi_table_rsdp *rsdp;
> + const struct acpi_table_xsdt *xsdt;
> + UINTN entry_count;
> + unsigned int i;
> +
> + if ( efi.acpi20 == EFI_INVALID_TABLE_ADDR )
> + return NULL;
> +
> + rsdp = (const void *)(UINTN)efi.acpi20;
Why the intermediate cast to UINTN?
> + if ( !rsdp || !rsdp->xsdt_physical_address )
> + return NULL;
> +
> + xsdt = (const void *)rsdp->xsdt_physical_address;
What if only an RSDT is supplied?
> + if ( memcmp(xsdt->header.signature, ACPI_SIG_XSDT, 4) != 0 )
> + return NULL;
> +
> + if ( xsdt->header.length < sizeof(xsdt->header) )
> + return NULL;
> + entry_count = (xsdt->header.length - sizeof(xsdt->header)) /
> + sizeof(xsdt->table_offset_entry[0]);
> +
> + for ( i = 0; i < entry_count; i++ )
This calls for i and entry_count to have the same type.
> + {
> + const struct acpi_table_header *hdr;
> +
> + hdr = (const void *)xsdt->table_offset_entry[i];
> + if ( !hdr )
> + continue;
> +
> + if ( memcmp(hdr->signature, ACPI_SIG_BGRT, 4) == 0 &&
> + hdr->length >= sizeof(struct acpi_table_bgrt) )
> + return (struct acpi_table_bgrt *)hdr;
Please use container_of() in favor of casts.
> + }
> +
> + return NULL;
> +}
> +
> +#define BMP_SIGNATURE 0x4D42
> +#define MAX_BGRT_IMAGE_SIZE (16 * 1024 * 1024)
Still an uncommented arbitrary constant.
> +static void __init efi_preserve_bgrt_img(void)
> +{
> + struct acpi_table_bgrt *bgrt;
> + const BMP_HEADER *bmp;
> + const void *old_image;
> + void *new_image;
> + UINTN image_size;
> + EFI_STATUS status;
> + UINT8 checksum;
> + unsigned int i;
> +
> + bgrt_info.preserved = false;
> +
> + bgrt = efi_get_bgrt();
> + if ( !bgrt )
> + {
> + bgrt_info.failure_reason = "BGRT table not found";
> + return;
> + }
> +
> + if ( !bgrt->image_address )
> + return;
> +
> + old_image = (const void *)bgrt->image_address;
> + bmp = old_image;
> +
> + if ( bmp->signature != BMP_SIGNATURE )
> + {
> + bgrt_info.failure_reason = "Invalid BMP signature";
> + return;
> + }
> +
> + image_size = bmp->file_size;
> + if ( !image_size || image_size > MAX_BGRT_IMAGE_SIZE )
> + {
> + bgrt_info.failure_reason = "Image size exceeds limit";
Does it, when it's zero?
> + return;
> + }
> +
> + /*
> + * Allocate memory of type EfiACPIReclaimMemory so that the image
> + * will remain available for the OS after ExitBootServices().
> + */
> + status = efi_bs->AllocatePool(EfiACPIReclaimMemory, image_size, &new_image);
> + if ( EFI_ERROR(status) )
> + {
> + bgrt_info.failure_reason = "Memory allocation failed";
> + return;
> + }
> + memcpy(new_image, old_image, image_size);
> + bgrt->image_address = (UINTN)new_image;
This looks like the wrong cast to me, even if in practice this likely is
going to be fine even on 32-bit EFI.
> + bgrt->header.checksum = 0;
> + checksum = 0;
> +
> + for ( i = 0; i < bgrt->header.length; i++ )
> + checksum += ((const UINT8 *)bgrt)[i];
> +
> + bgrt->header.checksum = -checksum;
> +
> + /* Filling the debug struct for printing later */
> + bgrt_info.preserved = true;
> + bgrt_info.old_addr = old_image;
> + bgrt_info.new_addr = new_image;
> + bgrt_info.size = image_size;
I'd suggest to drop "debug" from the comment.
> --- a/xen/common/efi/common-stub.c
> +++ b/xen/common/efi/common-stub.c
> @@ -19,6 +19,7 @@ unsigned long efi_get_time(void)
> }
>
> void efi_reset_system(bool warm) { }
> +void __init efi_bgrt_status_info(void) { }
What is this? Does this belong into a later patch? And did you pay
attention to Marek's earlier comment (as to the use of __init)?
Jan
© 2016 - 2026 Red Hat, Inc.