[PATCH 0/2] libacpi: Fix memory corruption on ACPI CPU hotplug

[PATCH 0/2] libacpi: Fix memory corruption on ACPI CPU hotplug

[Alejandro Vallejo]

Hi,

This series is a follow-up to a prior patch to remove ACPI CPU hotplug AML from
PVH guests. Between now and then the working assumption for the corruption has
been identified to be more general than PVH. This series rewords the commit
message for that patch (which became patch 2 in this series), and includes a
patch 1 that prevents AML overflowing the MADT during execution of _GPE.

More details of the bug in patch1's commit message.

pipeline: https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/2034006083
original patch: https://lore.kernel.org/xen-devel/20250910144921.29048-1-alejandro.garciavallejo@amd.com/

Alejandro Vallejo (2):
  libacpi: Prevent CPU hotplug AML from corrupting memory
  libacpi: Remove CPU hotplug and GPE handling from PVH DSDTs

 tools/libacpi/mk_dsdt.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)


base-commit: aad6ebf0596f7eda6ea709f1c293ef5911ae8938
-- 
2.43.0

[PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Alejandro Vallejo]

CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
by the device model. The GPE handler checks this and compares it against
the "online" flag on each MADT LAPIC entry, setting the flag to its
related bit in the bitmap and adjusting the table's checksum.

The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
reaches 128, even if that overflows the MADT into some other (hopefully
mapped) memory. The reading isn't as problematic as the writing though.

If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
then the bit where the "online" flag would be is flipped, thus
corrupting that memory. And the MADT checksum gets adjusted for a flip
that happened outside its range. It's all terrible.

Note that this corruption happens regardless of the device-model being
present or not, because even if the bitmap holds 0s, the overflowed
memory might not at the bits corresponding to the "online" flag.

This patch adjusts the DSDT so entries >=NCPUS are skipped.

Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
Half RFC. Not thoroughly untested. Pipeline is green, but none of this is tested
there.

v2:
  * New patch with the general fix for HVM too. Turns out the correction
    logic was buggy after all.
---
 tools/libacpi/mk_dsdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index 8ac4f9d0b4..06a229e2e9 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -239,7 +239,8 @@ int main(int argc, char **argv)
         /* Extract current CPU's status: 0=offline; 1=online. */
         stmt("And", "Local1, 1, Local2");
         /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
-        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
+        push_block("If", "And(LLess(%d, NCPU), LNotEqual(Local2, \\_SB.PR%02X.FLG))",
+                   cpu, cpu);
         /* ...If not, update it and the MADT checksum, and notify OSPM. */
         stmt("Store", "Local2, \\_SB.PR%02X.FLG", cpu);
         push_block("If", "LEqual(Local2, 1)");
-- 
2.43.0

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Andrew Cooper]

On 11/09/2025 12:53 pm, Alejandro Vallejo wrote:
> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
> by the device model. The GPE handler checks this and compares it against
> the "online" flag on each MADT LAPIC entry, setting the flag to its
> related bit in the bitmap and adjusting the table's checksum.
>
> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
> reaches 128, even if that overflows the MADT into some other (hopefully
> mapped) memory. The reading isn't as problematic as the writing though.
>
> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
> then the bit where the "online" flag would be is flipped, thus
> corrupting that memory. And the MADT checksum gets adjusted for a flip
> that happened outside its range. It's all terrible.
>
> Note that this corruption happens regardless of the device-model being
> present or not, because even if the bitmap holds 0s, the overflowed
> memory might not at the bits corresponding to the "online" flag.
>
> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>
> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> ---
> Half RFC. Not thoroughly untested. Pipeline is green, but none of this is tested
> there.
>
> v2:
>   * New patch with the general fix for HVM too. Turns out the correction
>     logic was buggy after all.

Hmm, this does sound rather more serious.  I have a nagging feeling that
until recently we always wrote 128 MADT entries.

So, while this looks like a good fix, I think we might want a second
Fixes tag.

~Andrew

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Alejandro Vallejo]

On Thu Sep 11, 2025 at 2:03 PM CEST, Andrew Cooper wrote:
> On 11/09/2025 12:53 pm, Alejandro Vallejo wrote:
>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>> by the device model. The GPE handler checks this and compares it against
>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>> related bit in the bitmap and adjusting the table's checksum.
>>
>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>> reaches 128, even if that overflows the MADT into some other (hopefully
>> mapped) memory. The reading isn't as problematic as the writing though.
>>
>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>> then the bit where the "online" flag would be is flipped, thus
>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>> that happened outside its range. It's all terrible.
>>
>> Note that this corruption happens regardless of the device-model being
>> present or not, because even if the bitmap holds 0s, the overflowed
>> memory might not at the bits corresponding to the "online" flag.
>>
>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>>
>> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
>> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>> ---
>> Half RFC. Not thoroughly untested. Pipeline is green, but none of this is tested
>> there.
>>
>> v2:
>>   * New patch with the general fix for HVM too. Turns out the correction
>>     logic was buggy after all.
>
> Hmm, this does sound rather more serious.  I have a nagging feeling that
> until recently we always wrote 128 MADT entries.

If so, I don't see where. It used to be 16, waaaaaaaaaaaaaaaaaaaaay back when.
Then it got extended to whatever it needed to be.

I have the nagging feeling that rather opaque "some OSs (cough Windows cough)
don't like more than 16 CPUs was actually this bug in action. Making the DSDTs
with exactly 16 CPUs a particular kind of silly.

>
> So, while this looks like a good fix, I think we might want a second
> Fixes tag.

Happy to add it, but I really don't see anything like that in the git log.

Cheers,
Alejandro

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Andrew Cooper]

On 11/09/2025 1:35 pm, Alejandro Vallejo wrote:
> On Thu Sep 11, 2025 at 2:03 PM CEST, Andrew Cooper wrote:
>> On 11/09/2025 12:53 pm, Alejandro Vallejo wrote:
>>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>>> by the device model. The GPE handler checks this and compares it against
>>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>>> related bit in the bitmap and adjusting the table's checksum.
>>>
>>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>>> reaches 128, even if that overflows the MADT into some other (hopefully
>>> mapped) memory. The reading isn't as problematic as the writing though.
>>>
>>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>>> then the bit where the "online" flag would be is flipped, thus
>>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>>> that happened outside its range. It's all terrible.
>>>
>>> Note that this corruption happens regardless of the device-model being
>>> present or not, because even if the bitmap holds 0s, the overflowed
>>> memory might not at the bits corresponding to the "online" flag.
>>>
>>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>>>
>>> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
>>> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
>>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>>> ---
>>> Half RFC. Not thoroughly untested. Pipeline is green, but none of this is tested
>>> there.
>>>
>>> v2:
>>>   * New patch with the general fix for HVM too. Turns out the correction
>>>     logic was buggy after all.
>> Hmm, this does sound rather more serious.  I have a nagging feeling that
>> until recently we always wrote 128 MADT entries.
> If so, I don't see where. It used to be 16, waaaaaaaaaaaaaaaaaaaaay back when.
> Then it got extended to whatever it needed to be.
>
> I have the nagging feeling that rather opaque "some OSs (cough Windows cough)
> don't like more than 16 CPUs was actually this bug in action. Making the DSDTs
> with exactly 16 CPUs a particular kind of silly.
>
>> So, while this looks like a good fix, I think we might want a second
>> Fixes tag.
> Happy to add it, but I really don't see anything like that in the git log.

I can't find it either.  Maybe my memory is getting faulty.

~Andrew

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Jan Beulich]

On 11.09.2025 14:03, Andrew Cooper wrote:
> On 11/09/2025 12:53 pm, Alejandro Vallejo wrote:
>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>> by the device model. The GPE handler checks this and compares it against
>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>> related bit in the bitmap and adjusting the table's checksum.
>>
>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>> reaches 128, even if that overflows the MADT into some other (hopefully
>> mapped) memory. The reading isn't as problematic as the writing though.
>>
>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>> then the bit where the "online" flag would be is flipped, thus
>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>> that happened outside its range. It's all terrible.
>>
>> Note that this corruption happens regardless of the device-model being
>> present or not, because even if the bitmap holds 0s, the overflowed
>> memory might not at the bits corresponding to the "online" flag.
>>
>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>>
>> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
>> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>> ---
>> Half RFC. Not thoroughly untested. Pipeline is green, but none of this is tested
>> there.
>>
>> v2:
>>   * New patch with the general fix for HVM too. Turns out the correction
>>     logic was buggy after all.
> 
> Hmm, this does sound rather more serious.  I have a nagging feeling that
> until recently we always wrote 128 MADT entries.

Not exactly recently, but looks like that's my fault then: 0875433389240
("hvmloader: limit CPUs exposed to guests").

Jan

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Alejandro Vallejo]

On Thu Sep 11, 2025 at 5:04 PM CEST, Jan Beulich wrote:
> On 11.09.2025 14:03, Andrew Cooper wrote:
>> On 11/09/2025 12:53 pm, Alejandro Vallejo wrote:
>>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>>> by the device model. The GPE handler checks this and compares it against
>>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>>> related bit in the bitmap and adjusting the table's checksum.
>>>
>>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>>> reaches 128, even if that overflows the MADT into some other (hopefully
>>> mapped) memory. The reading isn't as problematic as the writing though.
>>>
>>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>>> then the bit where the "online" flag would be is flipped, thus
>>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>>> that happened outside its range. It's all terrible.
>>>
>>> Note that this corruption happens regardless of the device-model being
>>> present or not, because even if the bitmap holds 0s, the overflowed
>>> memory might not at the bits corresponding to the "online" flag.
>>>
>>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>>>
>>> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
>>> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
>>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>>> ---
>>> Half RFC. Not thoroughly untested. Pipeline is green, but none of this is tested
>>> there.
>>>
>>> v2:
>>>   * New patch with the general fix for HVM too. Turns out the correction
>>>     logic was buggy after all.
>> 
>> Hmm, this does sound rather more serious.  I have a nagging feeling that
>> until recently we always wrote 128 MADT entries.
>
> Not exactly recently, but looks like that's my fault then: 0875433389240
> ("hvmloader: limit CPUs exposed to guests").
>
> Jan

Very right. I got to that commit, but thought nr_processor_objects would match 
NCPUS. Wrong assumption.

That sorts out wich fixes tag to attribute this to.

Cheers,
Alejandro

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Jan Beulich]

On 11.09.2025 13:53, Alejandro Vallejo wrote:
> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
> by the device model. The GPE handler checks this and compares it against
> the "online" flag on each MADT LAPIC entry, setting the flag to its
> related bit in the bitmap and adjusting the table's checksum.
> 
> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
> reaches 128, even if that overflows the MADT into some other (hopefully
> mapped) memory. The reading isn't as problematic as the writing though.
> 
> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
> then the bit where the "online" flag would be is flipped, thus
> corrupting that memory. And the MADT checksum gets adjusted for a flip
> that happened outside its range. It's all terrible.
> 
> Note that this corruption happens regardless of the device-model being
> present or not, because even if the bitmap holds 0s, the overflowed
> memory might not at the bits corresponding to the "online" flag.
> 
> This patch adjusts the DSDT so entries >=NCPUS are skipped.
> 
> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")

The code in question originates from e5dc62c4d4f1 ("hvmloader: Fix CPU
hotplug notify handler in ACPI DSDT"), though. Before that there was a
different issue (as mentioned in the description).

> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -239,7 +239,8 @@ int main(int argc, char **argv)
>          /* Extract current CPU's status: 0=offline; 1=online. */
>          stmt("And", "Local1, 1, Local2");
>          /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
> -        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
> +        push_block("If", "And(LLess(%d, NCPU), LNotEqual(Local2, \\_SB.PR%02X.FLG))",
> +                   cpu, cpu);

Don't we need to use \\_SB.NCPU here? From the other two uses it's not
quite clear; it might also be that the one using this form is actually
needlessly doing so. Yet here it may be better if only for consistency's
sake, as the LNotEqual() also operates on an absolute reference.

The other thing is that I'm not fluent in AML operand evaluation rules.
We want to avoid even the read access to FLG, and I'm unconvinced And()
will avoid evaluating its 2nd argument when the first one is 0. IOW this
may need to become a 2nd "If".

I further think that strictly speaking you mean LAnd() here, not And()
(but the above concern remains; all the ASL spec says is "Source1 and
Source2 are evaluated as integers" for both And() and LAnd()).

Jan

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Alejandro Vallejo]

On Thu Sep 11, 2025 at 4:52 PM CEST, Jan Beulich wrote:
> On 11.09.2025 13:53, Alejandro Vallejo wrote:
>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>> by the device model. The GPE handler checks this and compares it against
>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>> related bit in the bitmap and adjusting the table's checksum.
>> 
>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>> reaches 128, even if that overflows the MADT into some other (hopefully
>> mapped) memory. The reading isn't as problematic as the writing though.
>> 
>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>> then the bit where the "online" flag would be is flipped, thus
>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>> that happened outside its range. It's all terrible.
>> 
>> Note that this corruption happens regardless of the device-model being
>> present or not, because even if the bitmap holds 0s, the overflowed
>> memory might not at the bits corresponding to the "online" flag.
>> 
>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>> 
>> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
>
> The code in question originates from e5dc62c4d4f1 ("hvmloader: Fix CPU
> hotplug notify handler in ACPI DSDT"), though. Before that there was a
> different issue (as mentioned in the description).

As you mentioned elsewhere, it probably is 087543338924("hvmloader: limit CPUs
exposed to guests") that matters. Until then the DSDT was correct.

>
>> --- a/tools/libacpi/mk_dsdt.c
>> +++ b/tools/libacpi/mk_dsdt.c
>> @@ -239,7 +239,8 @@ int main(int argc, char **argv)
>>          /* Extract current CPU's status: 0=offline; 1=online. */
>>          stmt("And", "Local1, 1, Local2");
>>          /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
>> -        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
>> +        push_block("If", "And(LLess(%d, NCPU), LNotEqual(Local2, \\_SB.PR%02X.FLG))",
>> +                   cpu, cpu);
>
> Don't we need to use \\_SB.NCPU here? From the other two uses it's not
> quite clear; it might also be that the one using this form is actually
> needlessly doing so. Yet here it may be better if only for consistency's
> sake, as the LNotEqual() also operates on an absolute reference.

\SB.PMAT method does the same thing. I'll just change that too while at it.

> The other thing is that I'm not fluent in AML operand evaluation rules.
> We want to avoid even the read access to FLG, and I'm unconvinced And()
> will avoid evaluating its 2nd argument when the first one is 0. IOW this
> may need to become a 2nd "If".

I don't think there are any rules, it's unspecified. While in practice it
wouldn't matter a lot, it's indeed better not to rely on it not blowing up.

After sending this, I wondered about having a separate if with an early return.

>
> I further think that strictly speaking you mean LAnd() here, not And()
> (but the above concern remains; all the ASL spec says is "Source1 and
> Source2 are evaluated as integers" for both And() and LAnd()).

I very definitely did mean LAnd! Nice catch. As for 

>
> Jan

TL;DR: Will s/And/LAnd/ and move it to a separate If

Cheers,
Alejandro

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Jan Beulich]

On 11.09.2025 17:32, Alejandro Vallejo wrote:
> On Thu Sep 11, 2025 at 4:52 PM CEST, Jan Beulich wrote:
>> On 11.09.2025 13:53, Alejandro Vallejo wrote:
>>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>>> by the device model. The GPE handler checks this and compares it against
>>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>>> related bit in the bitmap and adjusting the table's checksum.
>>>
>>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>>> reaches 128, even if that overflows the MADT into some other (hopefully
>>> mapped) memory. The reading isn't as problematic as the writing though.
>>>
>>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>>> then the bit where the "online" flag would be is flipped, thus
>>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>>> that happened outside its range. It's all terrible.
>>>
>>> Note that this corruption happens regardless of the device-model being
>>> present or not, because even if the bitmap holds 0s, the overflowed
>>> memory might not at the bits corresponding to the "online" flag.
>>>
>>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>>>
>>> Fixes: c70ad37a1f7c("HVM vcpu add/remove: setup dsdt infrastructure...")
>>
>> The code in question originates from e5dc62c4d4f1 ("hvmloader: Fix CPU
>> hotplug notify handler in ACPI DSDT"), though. Before that there was a
>> different issue (as mentioned in the description).
> 
> As you mentioned elsewhere, it probably is 087543338924("hvmloader: limit CPUs
> exposed to guests") that matters. Until then the DSDT was correct.
> 
>>
>>> --- a/tools/libacpi/mk_dsdt.c
>>> +++ b/tools/libacpi/mk_dsdt.c
>>> @@ -239,7 +239,8 @@ int main(int argc, char **argv)
>>>          /* Extract current CPU's status: 0=offline; 1=online. */
>>>          stmt("And", "Local1, 1, Local2");
>>>          /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
>>> -        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
>>> +        push_block("If", "And(LLess(%d, NCPU), LNotEqual(Local2, \\_SB.PR%02X.FLG))",
>>> +                   cpu, cpu);
>>
>> Don't we need to use \\_SB.NCPU here? From the other two uses it's not
>> quite clear; it might also be that the one using this form is actually
>> needlessly doing so. Yet here it may be better if only for consistency's
>> sake, as the LNotEqual() also operates on an absolute reference.
> 
> \SB.PMAT method does the same thing. I'll just change that too while at it.
> 
>> The other thing is that I'm not fluent in AML operand evaluation rules.
>> We want to avoid even the read access to FLG, and I'm unconvinced And()
>> will avoid evaluating its 2nd argument when the first one is 0. IOW this
>> may need to become a 2nd "If".
> 
> I don't think there are any rules, it's unspecified. While in practice it
> wouldn't matter a lot, it's indeed better not to rely on it not blowing up.
> 
> After sending this, I wondered about having a separate if with an early return.
> 
>>
>> I further think that strictly speaking you mean LAnd() here, not And()
>> (but the above concern remains; all the ASL spec says is "Source1 and
>> Source2 are evaluated as integers" for both And() and LAnd()).
> 
> I very definitely did mean LAnd! Nice catch. As for 
> 
>>
>> Jan
> 
> TL;DR: Will s/And/LAnd/ and move it to a separate If

Except that once you use a separate If, no And() or LAnd() will be needed
anymore.

Jan

[PATCH v2 2/2] libacpi: Remove CPU hotplug and GPE handling from PVH DSDTs

[Alejandro Vallejo]

PVH guests have no DM, so this causes the guest to fetch the online CPU
bitmap from an unbacked 0xaf00 PIO port when executing the GPE handler.

Seeing how ACPI CPU hotplug is the only event delivered via GPE, remove
the GPE handler in addition to anything ACPI CPU hotplug related.

This shrinks PVH's DSDT substantially and prevents spuriously executing
a large amount of AML with no purpose at all.

Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
v2:
  * Adjusted commit message
  * All other tags except S-by moved to patch 1.
---
 tools/libacpi/mk_dsdt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index 06a229e2e9..cc4ed3961c 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -218,6 +218,11 @@ int main(int argc, char **argv)
     pop_block();
     /**** Processor end ****/
 #else
+    if (dm_version == QEMU_NONE) {
+        pop_block();
+        pop_block();
+        return 0;
+    }
 
     /* Operation Region 'PRST': bitmask of online CPUs. */
     stmt("OperationRegion", "PRST, SystemIO, %#x, %d",
@@ -265,10 +270,6 @@ int main(int argc, char **argv)
     pop_block();
     pop_block();
 
-    if (dm_version == QEMU_NONE) {
-        pop_block();
-        return 0;
-    }
     /**** Processor end ****/
 
 
-- 
2.43.0

Re: [PATCH v2 2/2] libacpi: Remove CPU hotplug and GPE handling from PVH DSDTs

[Jan Beulich]

On 11.09.2025 13:53, Alejandro Vallejo wrote:
> PVH guests have no DM, so this causes the guest to fetch the online CPU
> bitmap from an unbacked 0xaf00 PIO port when executing the GPE handler.
> 
> Seeing how ACPI CPU hotplug is the only event delivered via GPE, remove
> the GPE handler in addition to anything ACPI CPU hotplug related.
> 
> This shrinks PVH's DSDT substantially and prevents spuriously executing
> a large amount of AML with no purpose at all.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
> v2:
>   * Adjusted commit message
>   * All other tags except S-by moved to patch 1.

This will want backporting; I expect finding a suitable commit for a Fixes:
tag is somewhat difficult.

Jan

Re: [PATCH v2 2/2] libacpi: Remove CPU hotplug and GPE handling from PVH DSDTs

[Alejandro Vallejo]

On Thu Sep 11, 2025 at 5:07 PM CEST, Jan Beulich wrote:
> On 11.09.2025 13:53, Alejandro Vallejo wrote:
>> PVH guests have no DM, so this causes the guest to fetch the online CPU
>> bitmap from an unbacked 0xaf00 PIO port when executing the GPE handler.
>> 
>> Seeing how ACPI CPU hotplug is the only event delivered via GPE, remove
>> the GPE handler in addition to anything ACPI CPU hotplug related.
>> 
>> This shrinks PVH's DSDT substantially and prevents spuriously executing
>> a large amount of AML with no purpose at all.
>> 
>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks

>
>> ---
>> v2:
>>   * Adjusted commit message
>>   * All other tags except S-by moved to patch 1.
>
> This will want backporting; I expect finding a suitable commit for a Fixes:
> tag is somewhat difficult.
>
> Jan

I'd say it's the patch that needlessly enabled GPE handling on PVH.

Fixes: 062975dc9441("acpi: PVH guests need _E02 method")

Cheers,
Alejandro

Re: [PATCH v2 2/2] libacpi: Remove CPU hotplug and GPE handling from PVH DSDTs

[Jan Beulich]

On 11.09.2025 17:36, Alejandro Vallejo wrote:
> On Thu Sep 11, 2025 at 5:07 PM CEST, Jan Beulich wrote:
>> On 11.09.2025 13:53, Alejandro Vallejo wrote:
>>> ---
>>> v2:
>>>   * Adjusted commit message
>>>   * All other tags except S-by moved to patch 1.
>>
>> This will want backporting; I expect finding a suitable commit for a Fixes:
>> tag is somewhat difficult.
> 
> I'd say it's the patch that needlessly enabled GPE handling on PVH.
> 
> Fixes: 062975dc9441("acpi: PVH guests need _E02 method")

Oh, right.

Jan

[PATCH v2 0/2] libacpi: Fix memory corruption on ACPI CPU hotplug

[Alejandro Vallejo]

Hi,

Both patches are independent Patch 2 can very well go in before patch 1.

pipeline:  https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/2034774204
(still running as of now)

      v1: https://lore.kernel.org/xen-devel/20250911115308.16580-1-alejandro.garciavallejo@amd.com/
original: https://lore.kernel.org/xen-devel/20250910144921.29048-1-alejandro.garciavallejo@amd.com/

Cheers,
Alejandro

Alejandro Vallejo (2):
  libacpi: Prevent CPU hotplug AML from corrupting memory
  libacpi: Remove CPU hotplug and GPE handling from PVH DSDTs

 tools/libacpi/mk_dsdt.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)


base-commit: 16fae1561354f35dd524eb8953385d31eac3ce37
-- 
2.43.0

[PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Alejandro Vallejo]

CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
by the device model. The GPE handler checks this and compares it against
the "online" flag on each MADT LAPIC entry, setting the flag to its
related bit in the bitmap and adjusting the table's checksum.

The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
reaches 128, even if that overflows the MADT into some other (hopefully
mapped) memory. The reading isn't as problematic as the writing though.

If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
then the bit where the "online" flag would be is flipped, thus
corrupting that memory. And the MADT checksum gets adjusted for a flip
that happened outside its range. It's all terrible.

Note that this corruption happens regardless of the device-model being
present or not, because even if the bitmap holds 0s, the overflowed
memory might not at the bits corresponding to the "online" flag.

This patch adjusts the DSDT so entries >=NCPUS are skipped.

Fixes: 087543338924("hvmloader: limit CPUs exposed to guests")
Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
v2:
  * Added early returns in AML rather than dubious And() statements
---
 tools/libacpi/mk_dsdt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index 8ac4f9d0b4..aeeb71dfe6 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -231,6 +231,20 @@ int main(int argc, char **argv)
     stmt("Store", "ToBuffer(PRS), Local0");
     for ( cpu = 0; cpu < max_cpus; cpu++ )
     {
+        if ( cpu )
+        {
+            /*
+             * Check if we're still within the MADT bounds
+             *
+             * LLess() takes one byte, but LLessEqual() takes two. Increase
+             * `cpu` by 1, so we can avoid it. It does add up once you do it
+             * 127 times!
+             */
+            push_block("If", "LLess(\\_SB.NCPU, %d)", 1 + cpu);
+            stmt("Return", "One");
+            pop_block();
+        }
+
         /* Read a byte at a time from the PRST online-CPU bitmask. */
         if ( (cpu & 7) == 0 )
             stmt("Store", "DerefOf(Index(Local0, %u)), Local1", cpu/8);
-- 
2.43.0

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Jan Beulich]

On 11.09.2025 18:23, Alejandro Vallejo wrote:
> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
> by the device model. The GPE handler checks this and compares it against
> the "online" flag on each MADT LAPIC entry, setting the flag to its
> related bit in the bitmap and adjusting the table's checksum.
> 
> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
> reaches 128, even if that overflows the MADT into some other (hopefully
> mapped) memory. The reading isn't as problematic as the writing though.
> 
> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
> then the bit where the "online" flag would be is flipped, thus
> corrupting that memory. And the MADT checksum gets adjusted for a flip
> that happened outside its range. It's all terrible.
> 
> Note that this corruption happens regardless of the device-model being
> present or not, because even if the bitmap holds 0s, the overflowed
> memory might not at the bits corresponding to the "online" flag.
> 
> This patch adjusts the DSDT so entries >=NCPUS are skipped.
> 
> Fixes: 087543338924("hvmloader: limit CPUs exposed to guests")
> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>

In principle:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

However, ...

> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -231,6 +231,20 @@ int main(int argc, char **argv)
>      stmt("Store", "ToBuffer(PRS), Local0");
>      for ( cpu = 0; cpu < max_cpus; cpu++ )
>      {
> +        if ( cpu )
> +        {
> +            /*
> +             * Check if we're still within the MADT bounds
> +             *
> +             * LLess() takes one byte, but LLessEqual() takes two. Increase
> +             * `cpu` by 1, so we can avoid it. It does add up once you do it
> +             * 127 times!
> +             */
> +            push_block("If", "LLess(\\_SB.NCPU, %d)", 1 + cpu);
> +            stmt("Return", "One");

... if you already care about size bloat in the conditional, why are the two
bytes per instance that this extra return requires not relevant? They too
add up, and they can be avoided by wrapping the If around the rest of the
code. I didn't count it, but I expect the If encoding to grow by at most one
byte, perhaps none at all.

Jan

> +            pop_block();
> +        }
> +
>          /* Read a byte at a time from the PRST online-CPU bitmask. */
>          if ( (cpu & 7) == 0 )
>              stmt("Store", "DerefOf(Index(Local0, %u)), Local1", cpu/8);

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Alejandro Vallejo]

On Fri Sep 12, 2025 at 8:40 AM CEST, Jan Beulich wrote:
> On 11.09.2025 18:23, Alejandro Vallejo wrote:
>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>> by the device model. The GPE handler checks this and compares it against
>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>> related bit in the bitmap and adjusting the table's checksum.
>> 
>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>> reaches 128, even if that overflows the MADT into some other (hopefully
>> mapped) memory. The reading isn't as problematic as the writing though.
>> 
>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>> then the bit where the "online" flag would be is flipped, thus
>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>> that happened outside its range. It's all terrible.
>> 
>> Note that this corruption happens regardless of the device-model being
>> present or not, because even if the bitmap holds 0s, the overflowed
>> memory might not at the bits corresponding to the "online" flag.
>> 
>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>> 
>> Fixes: 087543338924("hvmloader: limit CPUs exposed to guests")
>> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>
> In principle:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Cheers,

>
> However, ...
>
>> --- a/tools/libacpi/mk_dsdt.c
>> +++ b/tools/libacpi/mk_dsdt.c
>> @@ -231,6 +231,20 @@ int main(int argc, char **argv)
>>      stmt("Store", "ToBuffer(PRS), Local0");
>>      for ( cpu = 0; cpu < max_cpus; cpu++ )
>>      {
>> +        if ( cpu )
>> +        {
>> +            /*
>> +             * Check if we're still within the MADT bounds
>> +             *
>> +             * LLess() takes one byte, but LLessEqual() takes two. Increase
>> +             * `cpu` by 1, so we can avoid it. It does add up once you do it
>> +             * 127 times!
>> +             */
>> +            push_block("If", "LLess(\\_SB.NCPU, %d)", 1 + cpu);
>> +            stmt("Return", "One");
>
> ... if you already care about size bloat in the conditional, why are the two
> bytes per instance that this extra return requires not relevant? They too
> add up, and they can be avoided by wrapping the If around the rest of the
> code. I didn't count it, but I expect the If encoding to grow by at most one
> byte, perhaps none at all.

I don't mind either way. Removing the "return" statement and the pop_block()
would save 254 bytes in tota at most. I don't think the conditional would grow
because the there wouldn't be that much contained within, but regardless the
early return is in the spirit of not going through 127 conditionals on every
GPE handle when you typically only have a handful of CPUs. The sooner we drop
out of AML, the better.

In due time I want to shrink this to be an AML loop in dsdt.asl so it can
be taken out of mk_dsdt.c. That will both shrink the DSDT (a ton) and accelerate
GPE handling, but I don't have time to do it at the moment.

Do you have a preference in table size vs execution-time?

Cheers,
Alejandro

Re: [PATCH v2 1/2] libacpi: Prevent CPU hotplug AML from corrupting memory

[Jan Beulich]

On 12.09.2025 11:32, Alejandro Vallejo wrote:
> On Fri Sep 12, 2025 at 8:40 AM CEST, Jan Beulich wrote:
>> On 11.09.2025 18:23, Alejandro Vallejo wrote:
>>> CPU hotplug relies on the online CPU bitmap being provided on PIO 0xaf00
>>> by the device model. The GPE handler checks this and compares it against
>>> the "online" flag on each MADT LAPIC entry, setting the flag to its
>>> related bit in the bitmap and adjusting the table's checksum.
>>>
>>> The bytecode doesn't, however, stop at NCPUS. It keeps comparing until it
>>> reaches 128, even if that overflows the MADT into some other (hopefully
>>> mapped) memory. The reading isn't as problematic as the writing though.
>>>
>>> If an "entry" outside the MADT is deemed to disagree with the CPU bitmap
>>> then the bit where the "online" flag would be is flipped, thus
>>> corrupting that memory. And the MADT checksum gets adjusted for a flip
>>> that happened outside its range. It's all terrible.
>>>
>>> Note that this corruption happens regardless of the device-model being
>>> present or not, because even if the bitmap holds 0s, the overflowed
>>> memory might not at the bits corresponding to the "online" flag.
>>>
>>> This patch adjusts the DSDT so entries >=NCPUS are skipped.
>>>
>>> Fixes: 087543338924("hvmloader: limit CPUs exposed to guests")
>>> Reported-by: Grygorii Strashko <grygorii_strashko@epam.com>
>>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>>
>> In principle:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Cheers,
> 
>>
>> However, ...
>>
>>> --- a/tools/libacpi/mk_dsdt.c
>>> +++ b/tools/libacpi/mk_dsdt.c
>>> @@ -231,6 +231,20 @@ int main(int argc, char **argv)
>>>      stmt("Store", "ToBuffer(PRS), Local0");
>>>      for ( cpu = 0; cpu < max_cpus; cpu++ )
>>>      {
>>> +        if ( cpu )
>>> +        {
>>> +            /*
>>> +             * Check if we're still within the MADT bounds
>>> +             *
>>> +             * LLess() takes one byte, but LLessEqual() takes two. Increase
>>> +             * `cpu` by 1, so we can avoid it. It does add up once you do it
>>> +             * 127 times!
>>> +             */
>>> +            push_block("If", "LLess(\\_SB.NCPU, %d)", 1 + cpu);
>>> +            stmt("Return", "One");
>>
>> ... if you already care about size bloat in the conditional, why are the two
>> bytes per instance that this extra return requires not relevant? They too
>> add up, and they can be avoided by wrapping the If around the rest of the
>> code. I didn't count it, but I expect the If encoding to grow by at most one
>> byte, perhaps none at all.
> 
> I don't mind either way. Removing the "return" statement and the pop_block()
> would save 254 bytes in tota at most. I don't think the conditional would grow
> because the there wouldn't be that much contained within, but regardless the
> early return is in the spirit of not going through 127 conditionals on every
> GPE handle when you typically only have a handful of CPUs. The sooner we drop
> out of AML, the better.
> 
> In due time I want to shrink this to be an AML loop in dsdt.asl so it can
> be taken out of mk_dsdt.c. That will both shrink the DSDT (a ton) and accelerate
> GPE handling, but I don't have time to do it at the moment.
> 
> Do you have a preference in table size vs execution-time?

Personally I'd favor table size. The AML interpreter is slow anyway.

Jan