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

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

Andrew Cooper:
> On 07/08/2023 3:45 pm, Simon Gaiser wrote:
>> 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
> 
> Honestly, the reserved must be zero means there's no need for a version
> check at all.  That bit will not be set even in older MADT revisions.
> 
> That said, it's likely easier to retain the version check than to set up
> a quirks infrastructure for buggy older MADTs.

Yes, that's what I was thinking too. Send a patch:
https://lore.kernel.org/xen-devel/20230911101238.18005-1-simon@xxxxxxxxxxxxxxxxxxxxxx/

Simon

Attachment: OpenPGP_signature
Description: OpenPGP digital signature