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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew