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

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

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.
> 
> I'm afraid it doesn't become clear to me what problem you're trying to
> solve.

I want Xen to not think there are possible CPUs that actually never can
be there.

Without ignoring those dummy entries Xen thinks my NUC has 2 sockets and
that there are 8 logical CPUs that are currently disabled but could be
hotplugged.

I'm moderately sure that soldering in another CPU is not supported, even
less so at runtime ;]

>> --- 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 */
>> +    if (processor->local_apic_id == 0xffffffff)
>> +            return 0;
>> +
>>      /* Don't register processors that cannot be onlined. */
>>      if (madt_revision >= 5 &&
>>          !(processor->lapic_flags & ACPI_MADT_ENABLED) &&
>>          !(processor->lapic_flags & ACPI_MADT_ONLINE_CAPABLE))
>>              return 0;
>>  
>> -    if ((processor->lapic_flags & ACPI_MADT_ENABLED) ||
>> -        processor->local_apic_id != 0xffffffff || opt_cpu_info) {
>> +    if ((processor->lapic_flags & ACPI_MADT_ENABLED) || opt_cpu_info) {
>>              acpi_table_print_madt_entry(header);
>>              log = true;
>>      }
> 
> In particular you're now suppressing log messages which may be relevant.

I intentionally mirrored the behavior of the check directly below.
Unlike the the version in Linux the existing code didn't log ignored
entries. So I did the same for the entries with an invalid ID.

> The one issue that I'm aware of (and that I use a local hack to deal
> with; see bottom) is excess verbosity.
> 
> Jan
> 
> --- unstable.orig/xen/arch/x86/mpparse.c
> +++ unstable/xen/arch/x86/mpparse.c
> @@ -809,8 +809,13 @@ int mp_register_lapic(u32 id, bool enabl
>       };
>       
>       if (MAX_APICS <= id) {
> -             printk(KERN_WARNING "Processor #%d invalid (max %d)\n",
> -                     id, MAX_APICS);
> +             static u32 max_warn = -1;
> +
> +             if (id <= max_warn) {
> +                     printk(KERN_WARNING "Processor #%d invalid (max %d)\n",
> +                            id, MAX_APICS);
> +                     max_warn = id - 1;
> +             }
>               return -EINVAL;
>       }
>

Attachment: OpenPGP_signature
Description: OpenPGP digital signature