Add status reporting for BGRT preservation and integrate with Xen's
ACPI subsystem:
- efi_bgrt_status_info() prints preservation status (success/failure)
- Called from acpi_boot_init() after ACPI tables are processed
- Clarifying comment explains why invalidation code remains

The invalidation code in acpi_invalidate_bgrt() now acts as a safety
net: if preservation fails, the image remains in conventional RAM
and gets invalidated. If preservation succeeds, the image is in
EfiACPIReclaimMemory which won't match the RAM_TYPE_CONVENTIONAL
check, leaving the table valid.

Signed-off-by: Soumyajyotii Ssarkar <soumyajyotisarkar23@gmail.com>
---
 xen/arch/x86/acpi/boot.c |  8 ++++++++
 xen/common/efi/boot.c    | 16 ++++++++++++++++
 xen/include/xen/efi.h    |  1 +
 3 files changed, 25 insertions(+)

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index 1ca2360e00..20afe79db9 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -29,6 +29,7 @@
 #include <xen/mm.h>
 #include <xen/param.h>
 #include <xen/dmi.h>
+#include <xen/efi.h>
 #include <asm/fixmap.h>
 #include <asm/page.h>
 #include <asm/apic.h>
@@ -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;
 }
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 47d5b9b2a8..e22a42c15b 100644
--- 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)
 {
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index 2e36b01e20..cebda997a5 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -39,6 +39,7 @@ static inline bool efi_enabled(unsigned int feature)
 extern bool efi_secure_boot;

 void efi_init_memory(void);
+void efi_bgrt_status_info(void);
 bool efi_boot_mem_unused(unsigned long *start, unsigned long *end);
 bool efi_rs_using_pgtables(void);
 unsigned long efi_get_time(void);
--
2.53.0

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.

Furthermore, is the EFI_BOOT check really needed? Without taking either
of the EFI boot paths, neither bgrt_info.preserved nor
bgrt_info.failure_reason[0] would have been altered from their initial
values.

Jan

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.

> Furthermore, is the EFI_BOOT check really needed? Without taking either
> of the EFI boot paths, neither bgrt_info.preserved nor
> bgrt_info.failure_reason[0] would have been altered from their initial
> values.
> 
> Jan

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

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?

Jan

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

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

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