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

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

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.
It's even worse if system happens to crash between those two points.
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).

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

Attachment: signature.asc
Description: PGP signature