Re: [XEN PATCH] x86/ACPI: Ignore entries with invalid APIC IDs when parsing MADT

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

Andrew Cooper:
> On 07/08/2023 10:38 am, Simon Gaiser wrote:
>> It seems some firmwares put dummy entries in the ACPI MADT table for non
>> existing processors. On my NUC11TNHi5 those have the invalid APIC ID
>> 0xff. Linux already has code to handle those cases both in
>> acpi_parse_lapic [1] as well as in acpi_parse_x2apic [2]. So add the
>> same check to Xen.
>>
>> Note that on some older (2nd gen Core i) laptop of mine I also saw dummy
>> entries with a valid APIC ID. Linux would still ignore those because
>> they have !ACPI_MADT_ENABLED && !ACPI_MADT_ONLINE_CAPABLE. But in Xen
>> this check is only active for madt_revision >= 5. But since this version
>> check seems to be intentionally I leave that alone.
> 
> I recall there being a discussion over this, ultimately with the version
> check being removed.  IIRC it was a defacto standard used for a long
> time prior to actually getting written into the ACPI spec, so does exist
> in practice in older MADTs.

So I noticed that the check in Linux is actually slightly different than
I thought. Since [3] it always considers the CPU usable if
ACPI_MADT_ENABLED is set. Otherwise it consider it only usable if
MADT revision >= 5 and ACPI_MADT_ONLINE_CAPABLE is set.

So I checked what the ACPI spec says:

Up to 6.2 Errata B [6] it only defines ACPI_MADT_ENABLE as:

    If zero, this processor is unusable, and the operating system
    support will not attempt to use it.

And the bit that later will be ACPI_MADT_ONLINE_CAPABLE is reserved with
"Must be zero".

6.3 [7] Then adds ACPI_MADT_ONLINE_CAPABLE and changes the meaning of
ACPI_MADT_ENABLE:

    Enabled
        If this bit is set the processor is ready for use. If this bit
        is clear and the Online Capable bit is set, system hardware
        supports enabling this processor during OS runtime. If this bit
        is clear and the Online Capable bit is also clear, this
        processor is unusable, and OSPM shall ignore the contents of the
        Processor Local APIC Structure.

    Online Capbable
        The information conveyed by this bit depends on the value of the
        Enabled bit. If the Enabled bit is set, this bit is reserved and
        must be zero. Otherwise, if this this bit is set, system
        hardware supports enabling this processor during OS runtime.

So with confirming firmwares it should be safe change the simply ignore
the entry if !ACPI_MADT_ENABLED && !ACPI_MADT_ONLINE_CAPABLE

We can also do it like Linux and ignore ACPI_MADT_ONLINE_CAPABLE
completely if revision < 5.

Note that the revision was already increased to 5 before 6.3.

ACPI spec version    MADT revision
                  
6.2 [4]              4
6.2 Errata A [5]     45 (typo I guess)
6.2 Errata B         5
6.3                  5

[3]: https://git.kernel.org/torvalds/c/e2869bd7af608c343988429ceb1c2fe99644a01f
[4]: http://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf
[5]: http://www.uefi.org/sites/default/files/resources/ACPI%206_2_A_Sept29.pdf
[6]: https://uefi.org/sites/default/files/resources/ACPI_6_2_B_final_Jan30.pdf
[7]: https://uefi.org/sites/default/files/resources/ACPI_6_3_May16.pdf

> Otherwise LGTM.  I'd suggest dropping this paragraph as it's not related
> to the change.  It will also help if we do decide to follow up and drop
> the MADT version check.
> 
>>
>> Link: 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f3bf1dbe64b62a2058dd1944c00990df203e8e7a
>>  # [1]
>> Link: 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=10daf10ab154e31237a8c07242be3063fb6a9bf4
>>  # [2]
> 
> https://git.kernel.org/torvalds/c/$SHA
> 
> Somewhat less verbose. https://korg.docs.kernel.org/git-url-shorteners.html
> 
>> Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/x86/acpi/boot.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
>> index 54b72d716b..4a62822fa9 100644
>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -87,14 +87,17 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, 
>> const unsigned long end)
>>      if (BAD_MADT_ENTRY(processor, end))
>>              return -EINVAL;
>>  
>> +    /* Ignore entries with invalid apicid */
> 
> x2apic ID.
> 
> ~Andrew

Attachment: OpenPGP_signature
Description: OpenPGP digital signature