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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/03/2026 2:18 pm, Marek Marczykowski-Górecki wrote:
> 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?

Casting away const is not ok.  The bug is in patch 1, with
efi_get_bgrt() returning a const pointer.

You should never lose the mutable pointer, and it is only objects in
rodata which legitimately lack a mutable pointer in the first place. 
Everything else is strictly mutable from C's point of view.

First fix the build issues, then run Eclair on the result.  There are
other issues in the series, I think.

~Andrew