On Fri, Mar 06, 2026 at 12:48:08AM +0530, Soumyajyotii Ssarkar wrote: > The ACPI BGRT (Boot Graphics Resource Table) contains a pointer to a > boot logo image stored in BootServicesData memory. When Xen reclaims > this memory during boot, the image is lost and the BGRT table becomes > invalid, causing Linux dom0 to report ACPI checksum errors. > > Add preservation logic similar to ESRT table handling: > - Locate BGRT table via XSDT during EFI boot services phase > - Validate BMP image signature and size (max 16 MB) > - Copy image to EfiACPIReclaimMemory (safe from reclamation) > - Update BGRT table with new image address > - Recalculate ACPI table checksum > > The preservation runs automatically during efi_exit_boot() before > Boot Services are terminated. This ensures the image remains > accessible to dom0. > > Open coded ACPI parsing is used because Xen's ACPI subsystem is not > available during the EFI boot phase. The RSDP is obtained from the > EFI System Table, and the XSDT is walked manually to find BGRT.
Thanks, this overall looks good, and it works :) See few remarks below. > Signed-off-by: Soumyajyotii Ssarkar <[email protected]> > --- > xen/arch/x86/efi/efi-boot.h | 2 + > xen/common/efi/boot.c | 187 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 189 insertions(+) > > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 42a2c46b5e..27792a56ff 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(SystemTable); > + This covers only multiboot path, but not xen.efi. It needs adding also in efi_start(). > efi_exit_boot(ImageHandle, SystemTable); > } > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 967094994d..1e3489e902 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -7,6 +7,7 @@ > #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> > @@ -173,6 +174,14 @@ static struct file __initdata ramdisk; > static struct file __initdata xsm; > static const CHAR16 __initconst newline[] = L"\r\n"; > > +static __initdata struct { > + bool preserved; > + uint64_t old_addr; > + uint64_t new_addr; > + uint32_t size; > + const char *failure_reason; > +} bgrt_debug_info; > + > static void __init PrintStr(const CHAR16 *s) > { > StdOut->OutputString(StdOut, (CHAR16 *)s ); > @@ -747,6 +756,171 @@ static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE > *SystemTable) > efi_bs->FreePool(memory_map); > } > > +struct bmp_header { > + uint16_t signature; > + uint32_t file_size; > + uint16_t reserved_1; > + uint16_t reserved_2; > + uint32_t data_offset; > +} __attribute__((packed)); > + > +/* > + * ACPI Structures - defined locally, > + * since we cannot include acpi headers > + * in EFI Context. > + */ > + > +struct acpi_rsdp { > + char signature[8]; > + uint8_t checksum; > + char oem_id[6]; > + uint8_t revision; > + uint32_t rsdt_physical_address; > + uint32_t length; > + uint64_t xsdt_physical_address; > + uint8_t extended_checksum; > + uint8_t reserved[3]; > +} __attribute__((packed)); > + > +struct acpi_table_header { > + char signature[4]; > + uint32_t length; > + uint8_t revision; > + uint8_t checksum; > + char oem_id[6]; > + char oem_table_id[8]; > + uint32_t oem_revision; > + uint32_t asl_compiler_id; > + uint32_t asl_compiler_revision; > +} __attribute__((packed)); > + > +struct acpi_xsdt { > + struct acpi_table_header header; > + uint64_t table_offset_entry[1]; /* Variable array length */ uint64_t table_offset_entry[]; BTW, do we have some canonical place with list of files imported (and kept in sync) with other projects? xen/include/acpi/actbl.h doesn't exactly follow Xen coding style, but it's unclear to me if it needs to stay this way. > +} __attribute__((packed)); > + > +struct acpi_bgrt { > + struct acpi_table_header header; > + uint16_t version; > + uint8_t status; > + uint8_t image_type; > + uint64_t image_address; > + uint32_t image_offset_x; > + uint32_t image_offset_y; > +} __attribute__((packed)); > + > +static struct acpi_bgrt* __init find_bgrt_table(EFI_SYSTEM_TABLE > *SystemTable) > +{ > + EFI_GUID acpi2_guid = ACPI_20_TABLE_GUID; > + struct acpi_rsdp *rsdp = NULL; > + struct acpi_xsdt *xsdt; > + struct acpi_bgrt *bgrt; > + uint32_t entry_count, actual_size; > + unsigned int i; > + > + for ( i = 0; i < SystemTable->NumberOfTableEntries; i++ ) > + { > + if ( match_guid(&acpi2_guid, > &SystemTable->ConfigurationTable[i].VendorGuid) ) > + { > + rsdp = SystemTable->ConfigurationTable[i].VendorTable; > + break; > + } > + } > + > + if ( !rsdp || !rsdp->xsdt_physical_address ) > + return NULL; > + > + xsdt = (struct acpi_xsdt *)rsdp->xsdt_physical_address; > + if ( !xsdt ) > + return NULL; > + > + actual_size = (xsdt->header.length - sizeof(struct acpi_table_header)); > + entry_count = (actual_size / sizeof(uint64_t)); > + > + for ( i = 0; i < entry_count; i++ ) > + { > + struct acpi_table_header *header = (struct acpi_table_header > *)xsdt->table_offset_entry[i]; > + > + if ( header->signature[0] == 'B' > + && header->signature[1] == 'G' > + && header->signature[2] == 'R' > + && header->signature[3] == 'T' ) strncmp? > + { > + bgrt = (struct acpi_bgrt *)header; You can just return it here, avoiding the extra variable. > + return bgrt; > + } > + } > + return NULL; > +} > + > +#define MAX_IMAGE_SIZE (16 * 1024 * 1024) /* Sanity check: reject if > bigger */ > + > +static void __init efi_preserve_bgrt_img(EFI_SYSTEM_TABLE *SystemTable) > +{ > + struct acpi_bgrt *bgrt; > + struct bmp_header *bmp; > + void *old_image, *new_image; > + uint32_t image_size; > + EFI_STATUS status; > + uint8_t checksum; > + unsigned int i; > + > + bgrt_debug_info.preserved = false; > + bgrt_debug_info.failure_reason = NULL; > + > + bgrt = find_bgrt_table(SystemTable); > + if ( !bgrt ) > + { > + bgrt_debug_info.failure_reason = "BGRT table not found in XSDT"; > + return; > + } > + > + if ( !bgrt->image_address ) > + { > + bgrt_debug_info.failure_reason = "BGRT image_address is NULL"; > + return; > + } > + > + old_image = (void *)bgrt->image_address; > + bmp = (struct bmp_header *)old_image; > + > + if ( bmp->signature != 0x4D42 ) > + { > + bgrt_debug_info.failure_reason = "Invalid BMP signature"; > + return; > + } > + > + image_size = bmp->file_size; > + if ( !image_size || image_size > MAX_IMAGE_SIZE ) > + { > + bgrt_debug_info.failure_reason = "Invalid image size"; > + return; > + } > + > + status = efi_bs->AllocatePool(EfiACPIReclaimMemory, image_size, > &new_image); > + if ( status != EFI_SUCCESS || !new_image ) > + { > + bgrt_debug_info.failure_reason = "Memory allocation failed"; > + return; > + } > + > + memcpy(new_image, old_image, image_size); > + > + bgrt->image_address = (uint64_t)new_image; > + bgrt->status |= 0x01; Why forcing the "displayed" bit here? > + bgrt->header.checksum = 0; > + checksum = 0; > + for ( i = 0; i < bgrt->header.length; i++ ) > + checksum += ((uint8_t *)bgrt)[i]; > + bgrt->header.checksum = (uint8_t)(0 - checksum); > + > + bgrt_debug_info.preserved = true; > + bgrt_debug_info.old_addr = (uint64_t)old_image; > + bgrt_debug_info.new_addr = (uint64_t)new_image; > + bgrt_debug_info.size = image_size; > +} > + This is quite a bit of code, maybe move to a separate file? But I'd like to hear what others think. > /* > * Include architecture specific implementation here, which references the > * static globals defined above. > @@ -1794,6 +1968,19 @@ void __init efi_init_memory(void) > if ( !efi_enabled(EFI_BOOT) ) > return; > > + if ( bgrt_debug_info.preserved ) > + { > + printk(XENLOG_INFO "EFI: BGRT image preserved: %u KB\n", > + bgrt_debug_info.size / 1024); > + printk(XENLOG_INFO "EFI: BGRT relocated from %#" PRIx64 " to %#" > PRIx64 "\n", > + bgrt_debug_info.old_addr, bgrt_debug_info.new_addr); > + } > + else if ( bgrt_debug_info.failure_reason ) > + { > + printk(XENLOG_WARNING "EFI: BGRT preservation failed: %s\n", > + bgrt_debug_info.failure_reason); > + } > + This is a bit unfortunate place to print this info, because it happens _after_ possibly printing the "invalidating image" message. Maybe you can wrap it in another function and call it next to the invalidating code? > printk(XENLOG_DEBUG "EFI memory map:%s\n", > map_bs ? " (mapping BootServices)" : ""); > for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) > -- > 2.53.0 > -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab
signature.asc
Description: PGP signature
