Re: [RFC PATCH v3 1/3] x86/efi: Add BGRT image preservation infrastructure

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Mar 12, 2026 at 04:44:12PM +0530, Soumyajyotii Ssarkar wrote:
> Add core EFI boot services code to preserve BGRT (Boot Graphics Resource
> Table) images during Xen boot. The BGRT contains a pointer to a boot logo
> stored in BootServicesData memory. Without preservation, this memory is
> reclaimed causing ACPI checksum errors in dom0.
> 
> Implementation:
> - Walk XSDT to locate BGRT table (reusing efi.acpi20 from efi_tables())
> - Validate BMP image signature and size constraints (max 16MB)
> - Allocate EfiACPIReclaimMemory and copy image data
> - Update BGRT table with new address and recalculate checksum
> 
> The preservation follows the ESRT pattern, running before
> ExitBootServices() to ensure image remains accessible.
> 
> Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@xxxxxxxxx>
> ---
>  xen/arch/x86/efi/efi-boot.h |   2 +
>  xen/common/efi/boot.c       | 133 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 135 insertions(+)
> 

...

> +static void __init efi_preserve_bgrt_img(void)
> +{
> +    const struct acpi_table_bgrt *bgrt;
> +    const BMP_HEADER *bmp;
> +    const void *old_image;
> +    void *new_image;
> +    UINTN image_size;
> +    EFI_STATUS status;
> +    UINT8 checksum;
> +    unsigned int i;
> +
> +    bgrt_info.preserved = false;
> +
> +    bgrt = efi_get_bgrt();
> +    if ( !bgrt )
> +    {
> +        bgrt_info.failure_reason = "BGRT table not found";
> +        return;
> +    }
> +
> +    if ( !bgrt->image_address )
> +        return;
> +
> +    old_image = (const void *)bgrt->image_address;
> +    bmp = old_image;
> +
> +    if ( bmp->signature != BMP_SIGNATURE )
> +    {
> +        bgrt_info.failure_reason = "Invalid BMP signature";
> +        return;
> +    }
> +
> +    image_size = bmp->file_size;
> +    if ( !image_size || image_size > MAX_BGRT_IMAGE_SIZE )
> +    {
> +        bgrt_info.failure_reason = "Image size exceeds limit";
> +        return;
> +    }
> +
> +    /*
> +     * Allocate memory of type EfiACPIReclaimMemory so that the image
> +     * will remain available for the OS after ExitBootServices().
> +     */
> +    status = efi_bs->AllocatePool(EfiACPIReclaimMemory, image_size, 
> &new_image);
> +    if ( EFI_ERROR(status) )
> +    {
> +        bgrt_info.failure_reason = "Memory allocation failed";
> +        return;
> +    }
> +    memcpy(new_image, old_image, image_size);
> +    ((struct acpi_table_bgrt *)bgrt)->image_address = (UINTN)new_image;
> +    ((struct acpi_table_bgrt *)bgrt)->header.checksum = 0;

Question to MISRA experts here - is this casting away of const okay
here? Or maybe better be done on the `bgrt` local variable? Or some
other way?

> +    checksum = 0;
> +
> +    for ( i = 0; i < bgrt->header.length; i++ )
> +        checksum += ((const UINT8 *)bgrt)[i];
> +
> +    ((struct acpi_table_bgrt *)bgrt)->header.checksum = -checksum;
> +
> +    /* Filling the debug struct for printing later */
> +    bgrt_info.preserved = true;
> +    bgrt_info.old_addr = old_image;
> +    bgrt_info.new_addr = new_image;
> +    bgrt_info.size = image_size;
> +}
> +
>  /*
>   * Include architecture specific implementation here, which references the
>   * static globals defined above.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature