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

[Dou Liyang]

Hi Yinghai

At 10/07/2016 05:20 AM, Yinghai Lu wrote:
> On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyang <douly.fnst@xxxxxxxxxxxxxx> wrote:
>
> > I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or
> > greater.
>
> Good to know. Maybe later when one package have more cores like 30 cores etc.
>
> > 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
>
> Yes, that should work. but should do the same thing for x2apic
>
> in acpi_parse_x2apic should have
>
> > +       if (processor->local_apic_id == -1) {
> > +               ++disabled_cpus;
> > +               return -EINVAL;
> > +       }
>
> that is the reason why i want to extend acpi_register_lapic()
> to take extra disabled_id (one is 0xff and another is 0xffffffff)
> so could save some lines.

Yes, I understood.
But I think adding an extra disabled_id is not a good way for
validating the apic_id. If the disabled_id is not just one id(-1 or
255), may be two or more, even be a range. what should we do for
extending our code?

Firstly, I am not sure that the "-1" could appear in the MADT, even if
the ACPI tables is unreasonable.

Seondly, I guess if we need the check, there are some reserved methods
in the kernel, such as "default_apic_id_valid", "x2apic_apic_id_valid"
and so on. we should extend all of them and use them for check.


CC'ed: Rafael and Lv

May I ask a question?

Is it possible that the "-1/oxffffffff" could appear in the MADT which 
is one of the ACPI tables?


> Thanks
>
> Yinghai