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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.08.2023 00:54, Thomas Gleixner wrote:
> On Tue, Aug 29 2023 at 16:25, Roger Pau Monné wrote:
>> On Sun, Aug 27, 2023 at 05:44:15PM +0200, Thomas Gleixner wrote:
>>> The APIC/X2APIC description of MADT specifies flags:
>>>
>>> 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 Capable      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.
>>
>> Sadly this flag is only present starting with MADT v5.
> 
> Correct. The difference between pre v5 MADT and v5+ is that the latter
> allows the OS to make more informed decisions, but the lack of this flag
> does not make the claim that randomly assigning APIC IDs after the
> initial parsing is a valid approach any more correct. Why?
> 
> Simply because the other relationships vs. processor UIDs and SRAT/SLIT
> are not magically going away due to the lack of that flag.
> 
>>> Otherwise you'd end up with a CPU hotplugged which is outside of the
>>> resource space allocated during init.
>>
>> It's my understating that ACPI UIDs 0xff and 0xfffffff for local ACPI
>> and x2APIC structures respectively are invalid, as that's the
>> broadcast value used by the local (x2)APIC NMI Structures.
> 
> Correct. These IDs are invalid independent of any flag value. 

What we apparently agree on is these special UID values to be invalid,
and that UIDs can't change. But that's not the same for the APIC IDs;
see below. (As a side note, Xen range-checks UID against its
implementation limit MAX_MADT_ENTRIES, so it's more than just the
all-ones values which we'd reject. Not really correct, I know. Looks
like Linux has done away with the simple x86_acpiid_to_apicid[]
translation mechanism. This being a statically sized array requires
this restriction in Xen, for the time being.)

>> I think Jan's point (if I understood correctly) is that Processor or
>> Device objects can have a _MAT method that returns updated MADT
>> entries, and some ACPI implementations might modify the original
>> entries on the MADT and return them from that method when CPU
>> hotplug takes place.

Just to mention it: It's not just "might". I've seen DSDT code doing
so on more than one occasion.

>>  The spec notes that "OSPM does not expect the
>> information provided in this table to be updated if the processor
>> information changes during the lifespan of an OS boot." so that the
>> MADT doesn't need to be updated when CPU hotplug happens, but I don't
>> see that sentence as preventing the MADT to be updated if CPU hotplug
>> takes place, it's just not required.
> 
> Right. But if you read carefully what I wrote then you will figure out
> that any randomly made up APIC ID post MADT enumeration cannot work.

I just went back and re-read your earlier reply. I can't see such
following from what you wrote.

>> I don't see anywhere in the spec that states that APIC IDs 0xff and
>> 0xffffffff are invalid and entries using those IDs should be ignored,
>> but I do think that any system that supports CPU hotplug better has
>> those IDs defined since boot.  Also it seems vendors have started
>> relying on using 0xff and 0xffffffff APIC IDs to signal non-present
>> CPUs, and Linux has been ignoring such entries for quite some time
>> already  without reported issues.
> 
> There is no requirement for the ACPI spec to state this simply because
> these APIC IDs are invalid to address a processor at the architectural
> level. ACPI does not care about architectural restrictions unless really
> required, e.g. like the LAPIC vs. X2APIC exclusiveness.

Correct, active processors can't use such APIC IDs. But placeholder
MADT entries can, so long as valid APIC IDs are put in place by
firmware (and announced via _MAT) at the time a socket is newly
populated. They'll be uniquely identified by their UID, so the OS very
well knows which originally parsed (from MADT) entries are affected.

As stated before, unless putting in place extra restrictions, I can't
even see how firmware would be able to up front determine APIC IDs for
unpopulated sockets: It simply can't know the topology of a package
that's not there yet. Requiring all packages to have identical
topology might be a restriction OSes put in place, but I'd be inclined
to call firmware buggy if it did (short of me being aware of there
being anything in the spec putting in place such a restriction).

Jan