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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 14:05, Soumyajyotii Ssarkar wrote:
> On Mon, Mar 9, 2026 at 1:01 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
>> 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?

First - see about re-using ACPI functions we already have. Then put new ACPI
code you need to add in a file different from the EFI one.

Jan