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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.03.2026 14:29, Soumyajyotii Ssarkar wrote:
> @@ -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;

No need for fixed-width types here, I expect (see ./CODING_STYLE).

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

Why would this be needed, when efi_tables() has already run?

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

Pleas prefer sizeof(<expression>) over sizeof(<type>), such that as a reader
one can know what is actually meant.

> +    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' )

Nit (style): If this was to not be replaced by a suitable function call, the
operators belong at the end of the earlier line (again see ./CODING_STYLE).

> +        {
> +            bgrt = (struct acpi_bgrt *)header;
> +            return bgrt;
> +        }
> +    }
> +    return NULL;
> +}

Nit (style): Blank line please ahead of the main return statement of a
function.

> +#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";

Why "invalid"? The cap is arbitrary, isn't it?

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

Seeing how you need to cast here, imo using pointer type fields and ...

> +    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",

... %p here would be preferable. With any casts between uint64_t / UINT64 and
pointer types you need to be aware that these will cause issues the moment we
gain a 32-bit use of this EFI interfacing code. Hence the fewer such casts,
the better.

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

Did you verify this actually works? efi_preserve_bgrt_img() runs when we're
still in physical mode, and hence the pointers stored will be physical
addresses. Whereas here you need virtual ones. A trick to use may be to
initialize the field with a pointer to a string literal (perhaps simply an
empty string). That'll cause a relocation to be emitted for the field, and
hence the pointer will then be relocated together with the rest of the Xen
image.

Jan