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

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

Andrew Cooper:
> On 07/08/2023 11:01 am, Andrew Cooper wrote:
>> On 07/08/2023 10:38 am, 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.
>> I recall there being a discussion over this, ultimately with the version
>> check being removed.  IIRC it was a defacto standard used for a long
>> time prior to actually getting written into the ACPI spec, so does exist
>> in practice in older MADTs.
>>
>> Otherwise LGTM.  I'd suggest dropping this paragraph as it's not related
>> to the change.  It will also help if we do decide to follow up and drop
>> the MADT version check.

It's in so far related as that I know this doesn't cover all cases I
actually wanted to address and I want to mention this. But I can see why
you might not want to have this in this commit message. Feel free to
drop.

>>> 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]
>> https://git.kernel.org/torvalds/c/$SHA
>>
>> Somewhat less verbose. https://korg.docs.kernel.org/git-url-shorteners.html

Ah, handy.

>>> Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>>  xen/arch/x86/acpi/boot.c | 14 ++++++++++----
>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
>>> index 54b72d716b..4a62822fa9 100644
>>> --- a/xen/arch/x86/acpi/boot.c
>>> +++ b/xen/arch/x86/acpi/boot.c
>>> @@ -87,14 +87,17 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, 
>>> const unsigned long end)
>>>     if (BAD_MADT_ENTRY(processor, end))
>>>             return -EINVAL;
>>>  
>>> +   /* Ignore entries with invalid apicid */
>> x2apic ID.
> 
> Oh, and I forgot to say.  I'm happy to fix all of this up on commit if
> you're happy.

Sure, go ahead. Thanks!

Attachment: OpenPGP_signature
Description: OpenPGP digital signature