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
