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
