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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 4046 bytes --]

On Thu, Mar 26, 2026 at 09:45:43AM +0100, Jan Beulich wrote:
> 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()?

Yes, either of those would be okay for me. I just want to avoid
potentially loosing important piece of information that Xen already has
at the point of invalidating BGRT.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]