Re: [XEN PATCH] x86/ACPI: Ignore entries marked as unusable when parsing MADT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2023 19:51, Simon Gaiser wrote:
> Jan Beulich:
>> On 11.09.2023 12:12, Simon Gaiser wrote:
>>> Up to version 6.2 Errata B [2] the ACPI spec only defines
>>> ACPI_MADT_ENABLE as:
>>>
>>>     If zero, this processor is unusable, and the operating system
>>>     support will not attempt to use it.
>>>
>>> The bit that later will be ACPI_MADT_ONLINE_CAPABLE is reserved with
>>> "Must be zero".
>>>
>>> Version 6.3 [3] 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 conforming firmwares it should be safe to simply ignore the
>>> entry if !ACPI_MADT_ENABLED && !ACPI_MADT_ONLINE_CAPABLE
>>>
>>> As a precaution against buggy firmwares this change, like Linux [4],
>>> ignores ACPI_MADT_ONLINE_CAPABLE completely if MADT revision < 5. Note
>>> that the MADT revision was already increased to 5 with spec version 6.2
>>> Errata A [1], so before introducing the online capable flag. But it
>>> wasn't changed for the new flag, so this is the best we can do here.
>>>
>>> For previous discussion see thread [5].
>>>
>>> Link: 
>>> http://www.uefi.org/sites/default/files/resources/ACPI%206_2_A_Sept29.pdf # 
>>> [1]
>>> Link: 
>>> https://uefi.org/sites/default/files/resources/ACPI_6_2_B_final_Jan30.pdf # 
>>> [2]
>>> Link: https://uefi.org/sites/default/files/resources/ACPI_6_3_May16.pdf # 
>>> [3]
>>> Link: 
>>> https://git.kernel.org/torvalds/c/e2869bd7af608c343988429ceb1c2fe99644a01f 
>>> # [4]
>>> Link: 
>>> https://lore.kernel.org/xen-devel/80bae614-052e-0f90-cf13-0e5e4ed1a5cd@xxxxxxxxxxxxxxxxxxxxxx/
>>>  # [5]
>>> Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
>>
>> Just to avoid misunderstandings: This change addresses a comment from
>> Andrew. I does not attempt to address my feedback on the earlier change,
>> which - as indicated - I intend to revert (timeline extended until mid
>> of the week), unless by then my earlier comments are addressed or the
>> suggested possible alternative is carried out.
>>
>> I think it goes without saying that this patch can't sensibly go in
>> until the earlier one was properly settled upon. In particular, in case
>> of reverting aiui this patch won't even apply anymore.
> 
> It textually conflicts with the other patch [6], and obviously was
> triggered by that discussion, but addresses a slightly different aspect.
> The textual conflict is trivial to resolve. I wasn't sure if it also
> conflicts with the concern you raised there, see below.
> 
> The other patch is about ignoring entries with invalid APIC IDs. As the
> discussion there showed the spec isn't very clear on that and there are
> different opinions how they should be handled. Regarding the flags the
> spec is much more concrete although given the response above I assume
> you also interpret "is unusable" of the old version of the
> ACPI_MADT_ENABLE flag as such that Xen should still allocate resources
> for those processors?
> 
> If I understood your main concern with the other patch correctly you
> have seen firmwares that later update those invalid APIC IDs with valid
> ones. Do those firmwares make use of the online capable flag? (Given
> above response probably not)

Being an older ACPI version, they don't.

> If not, then it indeed doesn't address your concern, and I see no way,
> at least by parsing MADT, how to distinguish those cases from firmwares
> that have dummy entries (the motivation for these patches).

Right. And while this _may_ be kind of acceptable when accompanied by a
downgrade of CPU hotplug support status, I haven't seen any patch to this
effect up to now. Without such a downgrade, my review comments would need
addressing, to avoid a perceived regression. Same goes for the tightening
done in the patch here: It _may_ be kind of acceptable, but only under
that same condition.

Jan