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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2023 20:05, Simon Gaiser wrote:
> Jan Beulich:
>> On 07.08.2023 11:38, 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.
>>>
>>> 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]
>>> Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
>>
>> This patch was committed with unaddressed review comments. The normal action
>> in such a case would be to revert, expecting a v2 to arrive. One alternative
>> here would be a timely incremental patch submission. Another alternative,
>> considering in particular Thomas's most recent reply, would be to properly
>> downgrade CPU hotplug support in SUPPORT.md (with a corresponding entry in
>> CHANGELOG.md).
>>
>> I'll give it until the end of next week for either of the alternatives to be
>> carried out. If nothing appears by then, I'll assume I may rightfully revert.
>> (This timeline also allows putting this topic on the Community Call agenda,
>> should anyone think this is necessary.)
> 
> To avoid misunderstandings, since you mentioned the above in your
> response to the related patch I submitted today [3]:
> 
> My understanding is that your main concern is that those entries with
> invalid APIC IDs shouldn't be ignored, since some firmwares later update
> them with valid IDs (on hotplug). This fully conflicts with the
> intention of the patch. Resolving the disagreement that followed is
> outside of my control, as being only the submitter of the patch.

Why would that be outside of your control? Your change broke a feature
officially stated as supported. Now that there is a patch to downgrade
support, that aspect will be resolved as soon as that patch gets ack-ed
and committed.

> You also commented about not logging the ignored entries. Until it's
> clear if the change in general is accepted in the end, I considered it
> pointless to address that detail. If you disagree and want a follow up
> for that, just let me know.

I take a different perspective here: The patch shouldn't have been
committed without this aspect addressed, either verbally or by sending
a v2. I continue to think that an incremental change is warranted to
make sure logging of entries, at least with "cpuinfo" in use, remains
consistent with what we had before. Otherwise debugging of possible
issues becomes yet more difficult.

Jan