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

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

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

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
>

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

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

>