Re: [PATCH v5 2/3] x86/acpi: Integrate BGRT preservation with status reporting

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2026 16:57, Marek Marczykowski-Górecki wrote:
> On Wed, Mar 25, 2026 at 04:44:15PM +0100, Jan Beulich wrote:
>> On 25.03.2026 16:32, Marek Marczykowski-Górecki wrote:
>>> On Wed, Mar 25, 2026 at 04:16:25PM +0100, Jan Beulich wrote:
>>>> On 24.03.2026 13:33, Soumyajyotii Ssarkar wrote:
>>>>> @@ -327,6 +328,11 @@ static int __init cf_check acpi_parse_hpet(struct 
>>>>> acpi_table_header *table)
>>>>>   return 0;
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * Invalidate BGRT if image is in conventional RAM (preservation failed).
>>>>> + * If preservation succeeded, image is in EfiACPIReclaimMemory, which
>>>>> + * won't match RAM_TYPE_CONVENTIONAL check, so table remains valid.
>>>>> + */
>>>>>  static int __init cf_check acpi_invalidate_bgrt(struct acpi_table_header 
>>>>> *table)
>>>>>  {
>>>>>   struct acpi_table_bgrt *bgrt_tbl =
>>>>> @@ -754,5 +760,7 @@ int __init acpi_boot_init(void)
>>>>>
>>>>>   acpi_table_parse(ACPI_SIG_BGRT, acpi_invalidate_bgrt);
>>>>>
>>>>> + efi_bgrt_status_info();
>>>>> +
>>>>>   return 0;
>>>>>  }
>>>>
>>>> Does this really need doing from here? If you called it ...
>>>>
>>>>> --- a/xen/common/efi/boot.c
>>>>> +++ b/xen/common/efi/boot.c
>>>>> @@ -1911,6 +1911,22 @@ static bool __init cf_check 
>>>>> rt_range_valid(unsigned long smfn, unsigned long emf
>>>>>      return true;
>>>>>  }
>>>>>
>>>>> +void __init efi_bgrt_status_info(void)
>>>>> +{
>>>>> +    if ( !efi_enabled(EFI_BOOT) )
>>>>> +        return;
>>>>> +
>>>>> +    if ( bgrt_info.preserved )
>>>>> +    {
>>>>> +        printk(XENLOG_INFO "EFI: BGRT image preserved: %lu KB\n",
>>>>> +               bgrt_info.size / 1024);
>>>>> +        printk(XENLOG_INFO "EFI: BGRT relocated from %p to %p\n",
>>>>> +               bgrt_info.old_addr, bgrt_info.new_addr);
>>>>> +    }
>>>>> +    else if ( bgrt_info.failure_reason[0] )
>>>>> +        printk(XENLOG_WARNING "EFI: BGRT preservation failed: %s\n",
>>>>> +               bgrt_info.failure_reason);
>>>>> +}
>>>>>
>>>>>  void __init efi_init_memory(void)
>>>>>  {
>>>>
>>>> ... out of this function, it could be static and no stub (misplaced in
>>>> the earlier patch) would be needed either.
>>>
>>> It was here before, and I complained about it, because it printed the
>>> invalidation reason way later than the actual invalidation.
>>
>> Sadly now I complain about this call out of acpi_boot_init(). What's wrong
>> with logging the BGRT stuff together with the memory map?
> 
> If you try to diagnose what went wrong with BGRT, that's not very
> intuitive to find - for example on my system it's 32 messages later.

Simply grep the log for BGRT?

> It's even worse if system happens to crash between those two points.

Hmm, perhaps.

> IMO it makes sense to log reason for BGRT invalidation together with
> the actual invalidation (message). I would be okay with moving it before
> the actual invalidation, but I don't think there is a place like this in
> xen/common/efi/boot.c (at a point where normal printk can be used already).

I guess what you really mean is printk() output actually going out (i.e.
not just to the ring buffer).

While still requiring the function to be extern (and there to be a stub),
how about adding the call much earlier in __start_xen, in here:

    else if ( efi_enabled(EFI_BOOT) )
        memmap_type = "EFI";

? Or alternatively anywhere between setting system_state to SYS_STATE_boot
and the call to acpi_boot_init()? Or re-using the other EFI_BOOT check that
we have in __start_xen()?

Jan