Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping

[Dou Liyang]

Hi Yinghai,

At 10/06/2016 12:53 PM, Yinghai Lu wrote:
> On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > > @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u
> > >          return -EINVAL;
> > >      }
> > >
> > > +    if (!enabled && (id == disabled_id)) {
> > > +        ++disabled_cpus;
> > > +        return -EINVAL;
> > > +    }
> >
> > Why would you need that disabled_id thing at all? The proper fix is to let
> > the apic driver detect the issue and this boils down to a 5 lines
> > change. Does the patch below fix the issue for you?
> > 8<----------------
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -2076,6 +2076,11 @@ int __generic_processor_info(int apicid,
> >         bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
> >                                 phys_cpu_present_map);
> >
> > +       if (!apic->apic_id_valid(apicid)) {
> > +               disabled_cpus++;
> > +               return -EINVAL;
> > +       }
> > +
> >         /*
> >          * boot_cpu_physical_apicid is designed to have the apicid
> >          * returned by read_apic_id(), i.e, the apicid of the
>
> No, That does not fix the issue.
>
> the system have x2apic pre_enabled from BIOS, so at the time
> apic is set to &apic_x2apic_cluster.
>
> early_acpi_boot_init ==> early_acpi_process_madt ==> acpi_parse_madt
> ==> default_acpi_madt_oem_check
>
> default_acpi_madt_oem_check
>       ==> apic_x2apic_cluster/x2apic_acpi_madt_oem_check ==> x2apic_enabled
>       ==> apic = &apic_x2apic_cluster
>
> and
> static int x2apic_apic_id_valid(int apicid)
> {
>         return 1;
> }
>
> To make your change work, may need to update x2apic_apic_id_valid to
>
> static int x2apic_apic_id_valid(int apicid)
> {
>         if (apicid == 0xff || apicid == -1)
>             return 0;

I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or
greater.
If we do that judgment, it may be affect x2APIC's work in some other places.

I saw the MADT, the main reason may be that we define 0xff to acpi_id
in LAPIC mode.
As you said, it was like:
[   42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[   42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
...

How about doing the acpi_id check when we parse it in
acpi_parse_lapic().

8<----------------

--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -233,6 +233,11 @@ acpi_parse_lapic(struct acpi_subtable_header * 
header, const unsigned long end)

        acpi_table_print_madt_entry(header);

+       if (processor->id >= 255) {
+               ++disabled_cpus;
+               return -EINVAL;
+       }
+
        /*
         * We need to register disabled CPU as well to permit
         * counting disabled CPUs. This allows us to size


Thanks

Dou

>         return 1;
> }
>
>
> Thanks
>
> Yinghai