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

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

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)

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

Simon

[6]: 
https://lore.kernel.org/xen-devel/7f158a54548456daba9f2e105d099d2e5e2c2f38.1691399031.git.simon@xxxxxxxxxxxxxxxxxxxxxx/

Attachment: OpenPGP_signature
Description: OpenPGP digital signature