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
