[v1] tools: Mark ACPI SDTs as NVS in the PVH build path

[PATCH] tools: Mark ACPI SDTs as NVS in the PVH build path

Posted by Alejandro Vallejo 9 months, 1 week ago

Commit cefeffc7e583 marked ACPI tables as NVS in the hvmloader path
because SeaBIOS may otherwise just mark it as RAM. There is, however,
yet another reason to do it even in the PVH path. Xen's incarnation of
AML relies on having access to some ACPI tables (e.g: _STA of Processor
objects relies on reading the processor online bit in its MADT entry)

This is problematic if the OS tries to reclaim ACPI memory for page
tables as it's needed for runtime and can't be reclaimed after the OSPM
is up and running.

Fixes: de6d188a519f("hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region)"
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
I really, really, really dislike this idea of accessing the MADT from
AML. In time I'll try to implement something to stop doing it, but for
the time being I find it preferable to align libxl to hvmloader rather
than trying to restrict what's reclaimable and what isn't.
---
 tools/firmware/hvmloader/e820.c | 4 ++++
 tools/libs/light/libxl_x86.c    | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index c490a0bc790c..86d39544e887 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -210,6 +210,10 @@ int build_e820_table(struct e820entry *e820,
      * space reuse by an ACPI unaware / buggy bootloader, option ROM, etc.
      * before an ACPI OS takes control. This is possible due to the fact that
      * ACPI NVS memory is explicitly described as non-reclaimable in ACPI spec.
+     *
+     * Furthermore, Xen relies on accessing ACPI tables from within the AML
+     * code exposed to guests. So Xen's ACPI tables are not, in general,
+     * reclaimable.
      */
 
     if ( acpi_enabled )
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index a3164a3077fe..265da8072a59 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -742,7 +742,7 @@ static int domain_construct_memmap(libxl__gc *gc,
             e820[nr].addr = dom->acpi_modules[i].guest_addr_out & ~(page_size - 1);
             e820[nr].size = dom->acpi_modules[i].length +
                 (dom->acpi_modules[i].guest_addr_out & (page_size - 1));
-            e820[nr].type = E820_ACPI;
+            e820[nr].type = E820_NVS;
             nr++;
         }
     }
-- 
2.48.1

Re: [PATCH] tools: Mark ACPI SDTs as NVS in the PVH build path

Posted by Jan Beulich 9 months, 1 week ago

On 10.03.2025 16:25, Alejandro Vallejo wrote:
> Commit cefeffc7e583 marked ACPI tables as NVS in the hvmloader path
> because SeaBIOS may otherwise just mark it as RAM. There is, however,
> yet another reason to do it even in the PVH path. Xen's incarnation of
> AML relies on having access to some ACPI tables (e.g: _STA of Processor
> objects relies on reading the processor online bit in its MADT entry)
> 
> This is problematic if the OS tries to reclaim ACPI memory for page
> tables as it's needed for runtime and can't be reclaimed after the OSPM
> is up and running.
> 
> Fixes: de6d188a519f("hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region)"
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> I really, really, really dislike this idea of accessing the MADT from
> AML.

I think this isn't Xen's invention, but something I've seen in various
systems' AML.

> In time I'll try to implement something to stop doing it, but for
> the time being I find it preferable to align libxl to hvmloader rather
> than trying to restrict what's reclaimable and what isn't.
> ---
>  tools/firmware/hvmloader/e820.c | 4 ++++
>  tools/libs/light/libxl_x86.c    | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> index c490a0bc790c..86d39544e887 100644
> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -210,6 +210,10 @@ int build_e820_table(struct e820entry *e820,
>       * space reuse by an ACPI unaware / buggy bootloader, option ROM, etc.
>       * before an ACPI OS takes control. This is possible due to the fact that
>       * ACPI NVS memory is explicitly described as non-reclaimable in ACPI spec.
> +     *
> +     * Furthermore, Xen relies on accessing ACPI tables from within the AML
> +     * code exposed to guests. So Xen's ACPI tables are not, in general,
> +     * reclaimable.
>       */
>  
>      if ( acpi_enabled )
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index a3164a3077fe..265da8072a59 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -742,7 +742,7 @@ static int domain_construct_memmap(libxl__gc *gc,
>              e820[nr].addr = dom->acpi_modules[i].guest_addr_out & ~(page_size - 1);
>              e820[nr].size = dom->acpi_modules[i].length +
>                  (dom->acpi_modules[i].guest_addr_out & (page_size - 1));
> -            e820[nr].type = E820_ACPI;
> +            e820[nr].type = E820_NVS;

This likely needs a comment then, too.

Jan

Re: [PATCH] tools: Mark ACPI SDTs as NVS in the PVH build path

Posted by Alejandro Vallejo 9 months, 1 week ago

On Tue Mar 11, 2025 at 8:30 AM GMT, Jan Beulich wrote:
> On 10.03.2025 16:25, Alejandro Vallejo wrote:
> > Commit cefeffc7e583 marked ACPI tables as NVS in the hvmloader path
> > because SeaBIOS may otherwise just mark it as RAM. There is, however,
> > yet another reason to do it even in the PVH path. Xen's incarnation of
> > AML relies on having access to some ACPI tables (e.g: _STA of Processor
> > objects relies on reading the processor online bit in its MADT entry)
> > 
> > This is problematic if the OS tries to reclaim ACPI memory for page
> > tables as it's needed for runtime and can't be reclaimed after the OSPM
> > is up and running.
> > 
> > Fixes: de6d188a519f("hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region)"
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> > I really, really, really dislike this idea of accessing the MADT from
> > AML.
>
> I think this isn't Xen's invention, but something I've seen in various
> systems' AML.
>

Do you mean ACPI hotplug? I don't think I've ever seen a real system with
support for it. I don't suppose you remember any specifically? I'd be quite
interested to have a look at their ACPI dumps.

> > In time I'll try to implement something to stop doing it, but for
> > the time being I find it preferable to align libxl to hvmloader rather
> > than trying to restrict what's reclaimable and what isn't.
> > ---
> >  tools/firmware/hvmloader/e820.c | 4 ++++
> >  tools/libs/light/libxl_x86.c    | 2 +-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> > index c490a0bc790c..86d39544e887 100644
> > --- a/tools/firmware/hvmloader/e820.c
> > +++ b/tools/firmware/hvmloader/e820.c
> > @@ -210,6 +210,10 @@ int build_e820_table(struct e820entry *e820,
> >       * space reuse by an ACPI unaware / buggy bootloader, option ROM, etc.
> >       * before an ACPI OS takes control. This is possible due to the fact that
> >       * ACPI NVS memory is explicitly described as non-reclaimable in ACPI spec.
> > +     *
> > +     * Furthermore, Xen relies on accessing ACPI tables from within the AML
> > +     * code exposed to guests. So Xen's ACPI tables are not, in general,
> > +     * reclaimable.
> >       */
> >  
> >      if ( acpi_enabled )
> > diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> > index a3164a3077fe..265da8072a59 100644
> > --- a/tools/libs/light/libxl_x86.c
> > +++ b/tools/libs/light/libxl_x86.c
> > @@ -742,7 +742,7 @@ static int domain_construct_memmap(libxl__gc *gc,
> >              e820[nr].addr = dom->acpi_modules[i].guest_addr_out & ~(page_size - 1);
> >              e820[nr].size = dom->acpi_modules[i].length +
> >                  (dom->acpi_modules[i].guest_addr_out & (page_size - 1));
> > -            e820[nr].type = E820_ACPI;
> > +            e820[nr].type = E820_NVS;
>
> This likely needs a comment then, too.

Fair enough. Let me send a v2 with that.

>
> Jan

Cheers,
Alejandro

Re: [PATCH] tools: Mark ACPI SDTs as NVS in the PVH build path

Posted by Jan Beulich 9 months, 1 week ago

On 11.03.2025 10:17, Alejandro Vallejo wrote:
> On Tue Mar 11, 2025 at 8:30 AM GMT, Jan Beulich wrote:
>> On 10.03.2025 16:25, Alejandro Vallejo wrote:
>>> Commit cefeffc7e583 marked ACPI tables as NVS in the hvmloader path
>>> because SeaBIOS may otherwise just mark it as RAM. There is, however,
>>> yet another reason to do it even in the PVH path. Xen's incarnation of
>>> AML relies on having access to some ACPI tables (e.g: _STA of Processor
>>> objects relies on reading the processor online bit in its MADT entry)
>>>
>>> This is problematic if the OS tries to reclaim ACPI memory for page
>>> tables as it's needed for runtime and can't be reclaimed after the OSPM
>>> is up and running.
>>>
>>> Fixes: de6d188a519f("hvmloader: flip "ACPI data" to "ACPI NVS" type for ACPI table region)"
>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>> ---
>>> I really, really, really dislike this idea of accessing the MADT from
>>> AML.
>>
>> I think this isn't Xen's invention, but something I've seen in various
>> systems' AML.
> 
> Do you mean ACPI hotplug? I don't think I've ever seen a real system with
> support for it. I don't suppose you remember any specifically? I'd be quite
> interested to have a look at their ACPI dumps.

Well, what I've seen were systems with firmware provisions for hotplug,
even if e.g. the electric aspects were missing. There was a time, after
all, during which Intel was trying to get OEMs to support this.

Jan