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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sun, Aug 27, 2023 at 05:44:15PM +0200, Thomas Gleixner wrote:
> On Wed, Aug 23 2023 at 14:56, Jan Beulich wrote:
> > On 23.08.2023 11:21, Andrew Cooper wrote:
> >> In the spec, exactly where you'd expect to find them...
> >> 
> >> "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."
> >
> > I don't think this tells us anything about the ID not possibly changing.
> > It merely tells us that OSPM is not expected to parse this table again
> > (IOW firmware updating just this table isn't going to be enough). IDs
> > possibly changing is expressed by (a) the "if the processor information
> > changes", and (b) the next sentence, forbidding them to change while the
> > system is asleep: "While in the sleeping state, logical processors must
> > not be added or removed, nor can their ... ID or ... Flags change."
> >
> >> Which is wordsmithing for "Some firmware was found to be modifying them
> >> and this was deemed to be illegal under the spec".
> >
> > That's your reading of it; I certainly don't infer such from that
> > sentence.
> 
> 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.

> This is also related to SRAT which defines the proximity of memory to
> processors at boot time with a similar set of flags.
> 
> Also 8.4 says:
> 
>   Each processor in the system must be declared in the ACPI namespace in
>   the \_SB scope. .... A Device definition for a processor is declared
>   using the ACPI0007 hardware identifier (HID). Processor configuration
>   information is provided exclusively by objects in the processor
>   device’s object list.
> 
>   When the platform uses the APIC interrupt model, UID object values
>   under a processor device are used to associate processor devices with
>   entries in the MADT.
> 
> 
> MADT is the authoritative table for processor enumeration, whether
> present or not. This is required because that's the only way to size
> resources, which depend on the possible maximum topology.
> 
> 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.

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.  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.

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.  Overall I'm inclined to say we
should take this change, as it avoids flagging a lot of systems as CPU
hotplug capable when they are not.

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I think Andrew had some minor comments.

I wonder whether the check should be done after the entry has been
printed if `opt_cpu_info` is set, but seeing as the ONLINE_CAPABLE is
also done before possibly printing the entry I guess it's fine.

Thanks, Roger.