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

[Jan Beulich <jbeulich@xxxxxxxx>]

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