[PATCH 0/3] Fixing ACPI BGRT (Boot Graphics Resource Table) corruption

[PATCH 0/3] Fixing ACPI BGRT (Boot Graphics Resource Table) corruption

[Soumyajyotii Ssarkar]

This series fixes ACPI BGRT (Boot Graphics Resource Table) corruption,
that occured when Xen reuses the memory containing the boot logo
image before dom0 is able to parse ACPI tables.

The BGRT table contains a pointer to a BMP image stored in
BootServicesData memory. When Xen reclaims this memory early in boot,
the pointer becomes invalid, causing Linux dom0 to report:
Xen: `(XEN) ACPI: BGRT: invalidating v1 image at 0x47cc2018`
Linux (dom0): `ACPI BIOS Warning (bug): Incorrect checksum
in table [BGRT] - 0xF9, should be 0xB4 (20250807/utcksum-58)`

This series:
1. Adds BGRT image preservation during EFI boot (similar to ESRT)
2. Removes the now-unnecessary invalidation code
3. Provides an opt-out mechanism (efi=no-bgrt) for servers

The preservation is enabled by default to fix the corruption for all
users, with minimal overhead (~1MB). Also, servers that don't need boot
graphics can disable it using the "efi=no-bgrt" option.

Testing:
- Verified on Intel UEFI system with Fedora 43 dom0
- Before: ACPI checksum errors in dom0
- After: Clean boot, no ACPI warnings
- Memory overhead: ~972 KB (preserved image size)

Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>

Soumyajyotii Ssarkar (3):
  x86/efi: Add BGRT image preservation during boot
  x86/acpi: Remove BGRT invalidation code
  x86/efi: Add opt-out mechanism for BGRT preservation

 xen/arch/x86/acpi/boot.c    |  23 -----
 xen/arch/x86/efi/efi-boot.h |   2 +
 xen/common/efi/boot.c       | 199 ++++++++++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+), 23 deletions(-)

--
2.53.0

[PATCH 1/3] x86/efi: Add BGRT image preservation during boot

[Soumyajyotii Ssarkar]

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.

Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>
---
 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);
+
     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 */
+} __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' )
+        {
+            bgrt = (struct acpi_bgrt *)header;
+            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;
+
+    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;
+}
+
 /*
  * 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);
+    }
+
     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

[PATCH 2/3] x86/acpi: Remove BGRT invalidation code

[Soumyajyotii Ssarkar]

Now that BGRT images are preserved during EFI boot (via
EfiACPIReclaimMemory allocation), the invalidation code in
acpi_parse_bgrt() is no longer needed. The BGRT table remains
valid throughout boot.

This removes the code that was marking BGRT invalid when the
image memory was detected as unavailable, which was causing
ACPI warnings in Linux dom0.

Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>
---
 xen/arch/x86/acpi/boot.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index 1ca2360e00..fc88b559e6 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -327,27 +327,6 @@ static int __init cf_check acpi_parse_hpet(struct acpi_table_header *table)
 	return 0;
 }

-static int __init cf_check acpi_invalidate_bgrt(struct acpi_table_header *table)
-{
-	struct acpi_table_bgrt *bgrt_tbl =
-		container_of(table, struct acpi_table_bgrt, header);
-
-	if (table->length < sizeof(*bgrt_tbl))
-		return -1;
-
-	if (bgrt_tbl->version == 1 && bgrt_tbl->image_address
-	    && !page_is_ram_type(PFN_DOWN(bgrt_tbl->image_address),
-				 RAM_TYPE_CONVENTIONAL))
-		return 0;
-
-	printk(KERN_INFO PREFIX "BGRT: invalidating v%d image at %#"PRIx64"\n",
-	       bgrt_tbl->version, bgrt_tbl->image_address);
-	bgrt_tbl->image_address = 0;
-	bgrt_tbl->status &= ~1;
-
-	return 0;
-}
-
 #define acpi_fadt_copy_address(dst, src, len) do {			\
 	if (fadt->header.revision >= FADT2_REVISION_ID &&		\
 	    fadt->header.length >= ACPI_FADT_V2_SIZE)			\
@@ -752,7 +731,5 @@ int __init acpi_boot_init(void)

 	acpi_hest_init();

-	acpi_table_parse(ACPI_SIG_BGRT, acpi_invalidate_bgrt);
-
 	return 0;
 }
--
2.53.0

[PATCH 3/3] x86/efi: Add opt-out mechanism for BGRT preservation

[Soumyajyotii Ssarkar]

As described in the task, BGRT preservation is now enabled by default to fix ACPI corruption
for desktop/workstation systems (similar to ESRT).

Add an opt-out parameter 'efi=no-bgrt' to allow disabling BGRT
preservation on systems where the ~1MB memory overhead is not
desired.

The parameter is parsed during normal Xen boot (not during EFI
phase), so it only affects diagnostic logging. The opt-out flag
is checked at the start of efi_preserve_bgrt_img().

Usage:
  Default: BGRT preserved automatically
  Opt-out: Add 'efi=no-bgrt' in Xen command line

Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>
---
 xen/common/efi/boot.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 1e3489e902..b735eac6b2 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -167,6 +167,7 @@ static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;

 static UINT32 __initdata mdesc_ver;
 static bool __initdata map_bs;
+static bool __initdata opt_bgrt_disabled = false;

 static struct file __initdata cfg;
 static struct file __initdata kernel;
@@ -868,6 +869,9 @@ static void __init efi_preserve_bgrt_img(EFI_SYSTEM_TABLE *SystemTable)
     bgrt_debug_info.preserved = false;
     bgrt_debug_info.failure_reason = NULL;

+    if ( opt_bgrt_disabled )
+        return;
+
     bgrt = find_bgrt_table(SystemTable);
     if ( !bgrt )
     {
@@ -1873,6 +1877,10 @@ static int __init cf_check parse_efi_param(const char *s)
             else
                 __clear_bit(EFI_RS, &efi_flags);
         }
+        else if ( (ss - s) == 7 && !memcmp(s, "no-bgrt", 7) )
+        {
+            opt_bgrt_disabled = true;
+        }
         else if ( (ss - s) > 5 && !memcmp(s, "attr=", 5) )
         {
             if ( !cmdline_strcmp(s + 5, "uc") )
@@ -1968,7 +1976,11 @@ void __init efi_init_memory(void)
     if ( !efi_enabled(EFI_BOOT) )
         return;

-    if ( bgrt_debug_info.preserved )
+    if ( opt_bgrt_disabled )
+    {
+        printk(XENLOG_INFO "EFI: BGRT preservation disabled\n");
+    }
+    else if ( bgrt_debug_info.preserved )
     {
         printk(XENLOG_INFO "EFI: BGRT image preserved: %u KB\n",
                bgrt_debug_info.size / 1024);
--
2.53.0

Re: [PATCH 2/3] x86/acpi: Remove BGRT invalidation code

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

On Fri, Mar 06, 2026 at 12:48:09AM +0530, Soumyajyotii Ssarkar wrote:
> Now that BGRT images are preserved during EFI boot (via
> EfiACPIReclaimMemory allocation), the invalidation code in
> acpi_parse_bgrt() is no longer needed. The BGRT table remains
> valid throughout boot.
> 
> This removes the code that was marking BGRT invalid when the
> image memory was detected as unavailable, which was causing
> ACPI warnings in Linux dom0.

When preserving failed for any reason, or when it was disabled (the next
patch), the entry still should be invalidated. In fact, the check here
for RAM_TYPE_CONVENTIONAL may already disable invalidation when it got
preserved?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] x86/acpi: Remove BGRT invalidation code

[Soumyajyotii Ssarkar]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

On Fri, Mar 6, 2026 at 7:23 AM Marek Marczykowski-Górecki <
marmarek@invisiblethingslab.com> wrote:

> On Fri, Mar 06, 2026 at 12:48:09AM +0530, Soumyajyotii Ssarkar wrote:
> > Now that BGRT images are preserved during EFI boot (via
> > EfiACPIReclaimMemory allocation), the invalidation code in
> > acpi_parse_bgrt() is no longer needed. The BGRT table remains
> > valid throughout boot.
> >
> > This removes the code that was marking BGRT invalid when the
> > image memory was detected as unavailable, which was causing
> > ACPI warnings in Linux dom0.
>
> When preserving failed for any reason, or when it was disabled (the next
> patch), the entry still should be invalidated. In fact, the check here
> for RAM_TYPE_CONVENTIONAL may already disable invalidation when it got
> preserved?
>
> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab
>


I could move forward with dropping the [PATCH 2/3]. And adapt the other
patches accordingly.
This would serve as a safety net in case the preservation fails for any
reason. Would that be a valid approach?

Thank You,
Soumyajyotii Ssarkar

[-- Attachment #2: Type: text/html, Size: 1563 bytes --]

Re: [PATCH 1/3] x86/efi: Add BGRT image preservation during boot

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 9625 bytes --]

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 <soumyajyotisarkar23@gmail.com>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] x86/acpi: Remove BGRT invalidation code

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Fri, Mar 06, 2026 at 08:59:07AM +0530, Soumyajyotii Ssarkar wrote:
> On Fri, Mar 6, 2026 at 7:23 AM Marek Marczykowski-Górecki <
> marmarek@invisiblethingslab.com> wrote:
> 
> > On Fri, Mar 06, 2026 at 12:48:09AM +0530, Soumyajyotii Ssarkar wrote:
> > > Now that BGRT images are preserved during EFI boot (via
> > > EfiACPIReclaimMemory allocation), the invalidation code in
> > > acpi_parse_bgrt() is no longer needed. The BGRT table remains
> > > valid throughout boot.
> > >
> > > This removes the code that was marking BGRT invalid when the
> > > image memory was detected as unavailable, which was causing
> > > ACPI warnings in Linux dom0.
> >
> > When preserving failed for any reason, or when it was disabled (the next
> > patch), the entry still should be invalidated. In fact, the check here
> > for RAM_TYPE_CONVENTIONAL may already disable invalidation when it got
> > preserved?
> >
> > --
> > Best Regards,
> > Marek Marczykowski-Górecki
> > Invisible Things Lab
> >
> 
> 
> I could move forward with dropping the [PATCH 2/3]. And adapt the other
> patches accordingly.
> This would serve as a safety net in case the preservation fails for any
> reason. Would that be a valid approach?

Yes, exactly.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] x86/efi: Add opt-out mechanism for BGRT preservation

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]

On Fri, Mar 06, 2026 at 12:48:10AM +0530, Soumyajyotii Ssarkar wrote:
> As described in the task, BGRT preservation is now enabled by default to fix ACPI corruption
> for desktop/workstation systems (similar to ESRT).
> 
> Add an opt-out parameter 'efi=no-bgrt' to allow disabling BGRT
> preservation on systems where the ~1MB memory overhead is not
> desired.
> 
> The parameter is parsed during normal Xen boot (not during EFI
> phase), so it only affects diagnostic logging. The opt-out flag
> is checked at the start of efi_preserve_bgrt_img().
> 
> Usage:
>   Default: BGRT preserved automatically
>   Opt-out: Add 'efi=no-bgrt' in Xen command line
> 
> Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>
> ---
>  xen/common/efi/boot.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 1e3489e902..b735eac6b2 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -167,6 +167,7 @@ static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
> 
>  static UINT32 __initdata mdesc_ver;
>  static bool __initdata map_bs;
> +static bool __initdata opt_bgrt_disabled = false;
> 
>  static struct file __initdata cfg;
>  static struct file __initdata kernel;
> @@ -868,6 +869,9 @@ static void __init efi_preserve_bgrt_img(EFI_SYSTEM_TABLE *SystemTable)
>      bgrt_debug_info.preserved = false;
>      bgrt_debug_info.failure_reason = NULL;
> 
> +    if ( opt_bgrt_disabled )
> +        return;
> +
>      bgrt = find_bgrt_table(SystemTable);
>      if ( !bgrt )
>      {
> @@ -1873,6 +1877,10 @@ static int __init cf_check parse_efi_param(const char *s)
>              else
>                  __clear_bit(EFI_RS, &efi_flags);
>          }
> +        else if ( (ss - s) == 7 && !memcmp(s, "no-bgrt", 7) )
> +        {
> +            opt_bgrt_disabled = true;
> +        }

This is too late - standard param parsing happens after efi_exit_boot()
already. See early cmdline parsing in efi_multiboot2() and also in
efi_start() (but note the latter is about options to xen.efi, which
isn't exactly the same as standard xen cmdline...)

>          else if ( (ss - s) > 5 && !memcmp(s, "attr=", 5) )
>          {
>              if ( !cmdline_strcmp(s + 5, "uc") )
> @@ -1968,7 +1976,11 @@ void __init efi_init_memory(void)
>      if ( !efi_enabled(EFI_BOOT) )
>          return;
> 
> -    if ( bgrt_debug_info.preserved )
> +    if ( opt_bgrt_disabled )
> +    {
> +        printk(XENLOG_INFO "EFI: BGRT preservation disabled\n");
> +    }
> +    else if ( bgrt_debug_info.preserved )
>      {
>          printk(XENLOG_INFO "EFI: BGRT image preserved: %u KB\n",
>                 bgrt_debug_info.size / 1024);
> --
> 2.53.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] x86/efi: Add BGRT image preservation during boot

[Jan Beulich]

On 08.03.2026 19:30, Marek Marczykowski-Górecki wrote:
> On Fri, Mar 06, 2026 at 12:48:08AM +0530, Soumyajyotii Ssarkar wrote:
>> --- 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.

I don't really understand why the headers we've got can't be used. Even
some of the library-like code under xen/acpi/ may be usable here.

While we don't exactly keep xen/include/acpi/ in sync with Linux, when
things are added we preferably add them in the way Linux has them.

>> +} __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)

Nit (style): The first * is misplaced.

>> +{
>> +    EFI_GUID acpi2_guid = ACPI_20_TABLE_GUID;
>> +    struct acpi_rsdp *rsdp = NULL;
>> +    struct acpi_xsdt *xsdt;
>> +    struct acpi_bgrt *bgrt;

Here and ...

>> +    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];

... here and elsewhere - please use pointer-to-const wherever possible.

>> +        if (   header->signature[0] == 'B'
>> +            && header->signature[1] == 'G'
>> +            && header->signature[2] == 'R'
>> +            && header->signature[3] == 'T' )
> 
> strncmp?

Or even memcmp() in this case. Plus there is ACPI_SIG_BGRT.

>> +        {
>> +            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?

And if this is needed, why by way of a literal number rather than a suitable
#define?

>> +    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.

Whether to put in a separate file is only the 2nd question imo. The first is
whether this much code is needed in the first place.

Jan

Re: [PATCH 1/3] x86/efi: Add BGRT image preservation during boot

[Soumyajyotii Ssarkar]

[-- Attachment #1: Type: text/plain, Size: 10518 bytes --]

On Mon, Mar 9, 2026 at 1:01 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 08.03.2026 19:30, Marek Marczykowski-Górecki wrote:
> > On Fri, Mar 06, 2026 at 12:48:08AM +0530, Soumyajyotii Ssarkar wrote:
> >> --- 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.
>
> I don't really understand why the headers we've got can't be used. Even
> some of the library-like code under xen/acpi/ may be usable here.
>
> While we don't exactly keep xen/include/acpi/ in sync with Linux, when
> things are added we preferably add them in the way Linux has them.
>
>
I was trying to avoid including the headers from the xen/include/acpi/
since it was specified in the comment. to not include them.
Specific comment specified below this paragraph.
Also since acpi was using datatypes like "u32" while boot.c had types of
"uint32", so it felt a bit non-standardized.
I checked the rest of the boot.c which followed the same manner. So I went
with this choice.

/* * Keep this arch-specific modified include in the common file, as moving
 * it to the arch specific include file would obscure that special care is
 * taken to include it with __ASSEMBLER__ defined.
*/
#define __ASSEMBLER__ /* avoid pulling in ACPI stuff (conflicts with EFI)
*/
#include <asm/fixmap.h>
#undef __ASSEMBLER__
#endif

The ACPI headers in /xen/include/acpi uses defines such
as ACPI_NAME_SIZE, ACPI_OEM_ID_SIZE
and ACPI_OEM_TABLE_ID_SIZE these require adding additional
<acpi/acconfig.h> header.
Also since their is no acpi headers included in the boot.c file, so i
thought to I avoid it.

Thus to get it fully working with ACPI headers from the xen/include/acpi I
would require these three headers.
#include <acpi/acconfig.h>
#include <acpi/actbl.h>
#include <acpi/actbl3.h>

I thought this would lead to cross contamination, and confusing to further
modifications in future so weighing my options I thought best to redefine
them,
for code clarity.
Can you suggest me best option to move forward, should I redefine them as
is or include the headers?

>> +} __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)
>
> Nit (style): The first * is misplaced.
>
> >> +{
> >> +    EFI_GUID acpi2_guid = ACPI_20_TABLE_GUID;
> >> +    struct acpi_rsdp *rsdp = NULL;
> >> +    struct acpi_xsdt *xsdt;
> >> +    struct acpi_bgrt *bgrt;
>
> Here and ...
>
> >> +    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];
>
> ... here and elsewhere - please use pointer-to-const wherever possible.
>
> >> +        if (   header->signature[0] == 'B'
> >> +            && header->signature[1] == 'G'
> >> +            && header->signature[2] == 'R'
> >> +            && header->signature[3] == 'T' )
> >
> > strncmp?
>
> Or even memcmp() in this case. Plus there is ACPI_SIG_BGRT.


Yeah, my apologies. Since I was going with the whole not including acpi
headers idea,
I thought this would be better stylistic choice.

New patch version with strncmp upcoming.

The headers are in xen/include/acpi, so was trying to work around without
including them.
Perhaps including the headers would be the move forward?

What is your opinion Marek?


>
>> +        {
> >> +            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?
>
> And if this is needed, why by way of a literal number rather than a
> suitable
> #define?
>
> >> +    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.
>
>
I believe it won't be necessary to add a separate file for this since it's
just a 2 functions.
I believe this since most of the ESRT patches, which I based my patches on
didn't use a separate file.
But now since with the whole, header issue and redefining the structs I
think a separate file would also be a viable option too.

Perhaps any 2 options:
Moving with a separate file for cleaner code.
Moving with existing file for non-standardized code.
I would love your opinions on this.

Further, I think I should revise the further patches to RFC for clarity.
Before coming to a conclusion.
And sending as patch to be pulled.

Whether to put in a separate file is only the 2nd question imo. The first is
> whether this much code is needed in the first place.
>


> Jan
>

[-- Attachment #2: Type: text/html, Size: 14397 bytes --]

Re: [PATCH 1/3] x86/efi: Add BGRT image preservation during boot

[Jan Beulich]

On 10.03.2026 14:05, Soumyajyotii Ssarkar wrote:
> On Mon, Mar 9, 2026 at 1:01 PM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> On 08.03.2026 19:30, Marek Marczykowski-Górecki wrote:
>>> On Fri, Mar 06, 2026 at 12:48:08AM +0530, Soumyajyotii Ssarkar wrote:
>>>> --- 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.
>>
>> I don't really understand why the headers we've got can't be used. Even
>> some of the library-like code under xen/acpi/ may be usable here.
>>
>> While we don't exactly keep xen/include/acpi/ in sync with Linux, when
>> things are added we preferably add them in the way Linux has them.
>>
>>
> I was trying to avoid including the headers from the xen/include/acpi/
> since it was specified in the comment. to not include them.
> Specific comment specified below this paragraph.
> Also since acpi was using datatypes like "u32" while boot.c had types of
> "uint32", so it felt a bit non-standardized.
> I checked the rest of the boot.c which followed the same manner. So I went
> with this choice.
> 
> /* * Keep this arch-specific modified include in the common file, as moving
>  * it to the arch specific include file would obscure that special care is
>  * taken to include it with __ASSEMBLER__ defined.
> */
> #define __ASSEMBLER__ /* avoid pulling in ACPI stuff (conflicts with EFI)
> */
> #include <asm/fixmap.h>
> #undef __ASSEMBLER__
> #endif
> 
> The ACPI headers in /xen/include/acpi uses defines such
> as ACPI_NAME_SIZE, ACPI_OEM_ID_SIZE
> and ACPI_OEM_TABLE_ID_SIZE these require adding additional
> <acpi/acconfig.h> header.
> Also since their is no acpi headers included in the boot.c file, so i
> thought to I avoid it.
> 
> Thus to get it fully working with ACPI headers from the xen/include/acpi I
> would require these three headers.
> #include <acpi/acconfig.h>
> #include <acpi/actbl.h>
> #include <acpi/actbl3.h>
> 
> I thought this would lead to cross contamination, and confusing to further
> modifications in future so weighing my options I thought best to redefine
> them,
> for code clarity.
> Can you suggest me best option to move forward, should I redefine them as
> is or include the headers?

First - see about re-using ACPI functions we already have. Then put new ACPI
code you need to add in a file different from the EFI one.

Jan

Re: [PATCH 1/3] x86/efi: Add BGRT image preservation during boot

[Soumyajyotii Ssarkar]

[-- Attachment #1: Type: text/plain, Size: 5839 bytes --]

On Tue, 10 Mar, 2026, 7:44 pm Jan Beulich, <jbeulich@suse.com> wrote:

> On 10.03.2026 14:05, Soumyajyotii Ssarkar wrote:
> > On Mon, Mar 9, 2026 at 1:01 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> >> On 08.03.2026 19:30, Marek Marczykowski-Górecki wrote:
> >>> On Fri, Mar 06, 2026 at 12:48:08AM +0530, Soumyajyotii Ssarkar wrote:
> >>>> --- 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.
> >>
> >> I don't really understand why the headers we've got can't be used. Even
> >> some of the library-like code under xen/acpi/ may be usable here.
> >>
> >> While we don't exactly keep xen/include/acpi/ in sync with Linux, when
> >> things are added we preferably add them in the way Linux has them.
> >>
> >>
> > I was trying to avoid including the headers from the xen/include/acpi/
> > since it was specified in the comment. to not include them.
> > Specific comment specified below this paragraph.
> > Also since acpi was using datatypes like "u32" while boot.c had types of
> > "uint32", so it felt a bit non-standardized.
> > I checked the rest of the boot.c which followed the same manner. So I
> went
> > with this choice.
> >
> > /* * Keep this arch-specific modified include in the common file, as
> moving
> >  * it to the arch specific include file would obscure that special care
> is
> >  * taken to include it with __ASSEMBLER__ defined.
> > */
> > #define __ASSEMBLER__ /* avoid pulling in ACPI stuff (conflicts with EFI)
> > */
> > #include <asm/fixmap.h>
> > #undef __ASSEMBLER__
> > #endif
> >
> > The ACPI headers in /xen/include/acpi uses defines such
> > as ACPI_NAME_SIZE, ACPI_OEM_ID_SIZE
> > and ACPI_OEM_TABLE_ID_SIZE these require adding additional
> > <acpi/acconfig.h> header.
> > Also since their is no acpi headers included in the boot.c file, so i
> > thought to I avoid it.
> >
> > Thus to get it fully working with ACPI headers from the xen/include/acpi
> I
> > would require these three headers.
> > #include <acpi/acconfig.h>
> > #include <acpi/actbl.h>
> > #include <acpi/actbl3.h>
> >
> > I thought this would lead to cross contamination, and confusing to
> further
> > modifications in future so weighing my options I thought best to redefine
> > them,
> > for code clarity.
> > Can you suggest me best option to move forward, should I redefine them as
> > is or include the headers?
>
>
>
> First - see about re-using ACPI functions we already have. Then put new
> ACPI
> code you need to add in a file different from the EFI one.
>
> Jan
>

Hey Jan,
Referring to the latest BGRT RFC patch series (RFC v3): as you had
suggested, I have reused the ACPI headers and made the changes you advised.
I hope this version looks satisfactory.

If that approach seems acceptable, I would proceed with introducing a new
file and moving the related changes there as part of the next patch series.

In that case, it might also make sense to involve the ESRT maintainers?
Since I could lay the foundation for the new file and they could extend it
by migrating the ESRT related code their? This might help keep boot.c
cleaner?
I would be interested to know others think on this.

Thank you,
Soumyajyotii Ssarkar

>

[-- Attachment #2: Type: text/html, Size: 8364 bytes --]