Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi tglx, At 10/07/2016 09:00 PM, Thomas Gleixner wrote: On Fri, 7 Oct 2016, Thomas Gleixner wrote: On Fri, 7 Oct 2016, Dou Liyang wrote: Is it possible that the "-1/ox" could appear in the MADT which is one of the ACPI tables? According to the SDM the x2apic id is a 32bit ID, so 0x is a legitimate value. Yes, I see. The ACPI spec says that bit 0 of the x2apic flags field tells whether the logical processor is present or not. So the proper check for x2apic is that flag. The lapic structure has the same flag, but the kernel ignores the flags for both lapic and x2apic. It seems the kernel uses the flags in this sentence: enabled = processor->lapic_flags & ACPI_MADT_ENABLED; I'm going to apply the minimal fix of checking for id == 0xff in acpi_lapic_parse() for now, but this needs to be revisited and fixed proper. Yes, I will do it. Thanks Dou.
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi tglx, At 10/07/2016 09:00 PM, Thomas Gleixner wrote: On Fri, 7 Oct 2016, Thomas Gleixner wrote: On Fri, 7 Oct 2016, Dou Liyang wrote: Is it possible that the "-1/ox" could appear in the MADT which is one of the ACPI tables? According to the SDM the x2apic id is a 32bit ID, so 0x is a legitimate value. Yes, I see. The ACPI spec says that bit 0 of the x2apic flags field tells whether the logical processor is present or not. So the proper check for x2apic is that flag. The lapic structure has the same flag, but the kernel ignores the flags for both lapic and x2apic. It seems the kernel uses the flags in this sentence: enabled = processor->lapic_flags & ACPI_MADT_ENABLED; I'm going to apply the minimal fix of checking for id == 0xff in acpi_lapic_parse() for now, but this needs to be revisited and fixed proper. Yes, I will do it. Thanks Dou.
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi tglx, At 10/07/2016 09:07 PM, Thomas Gleixner wrote: On Thu, 6 Oct 2016, Dou Liyang wrote: + if (processor->id >= 255) { + ++disabled_cpus; Incrementing disabled_cpus here is simply wrong because 0xff is an invalid local APIC id. So we can simply return -EINVAL and be done with it. Yes, It is. + return -EINVAL; Thanks, tglx Thanks, Dou
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi tglx, At 10/07/2016 09:07 PM, Thomas Gleixner wrote: On Thu, 6 Oct 2016, Dou Liyang wrote: + if (processor->id >= 255) { + ++disabled_cpus; Incrementing disabled_cpus here is simply wrong because 0xff is an invalid local APIC id. So we can simply return -EINVAL and be done with it. Yes, It is. + return -EINVAL; Thanks, tglx Thanks, Dou
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Fri, Oct 7, 2016 at 6:00 AM, Thomas Gleixnerwrote: > On Fri, 7 Oct 2016, Thomas Gleixner wrote: >> On Fri, 7 Oct 2016, Dou Liyang wrote: >> > Is it possible that the "-1/ox" could appear in the MADT which is >> > one >> > of the ACPI tables? >> >> According to the SDM the x2apic id is a 32bit ID, so 0x is a >> legitimate value. > > The ACPI spec says that bit 0 of the x2apic flags field tells whether the > logical processor is present or not. So the proper check for x2apic is that > flag. > > The lapic structure has the same flag, but the kernel ignores the flags for > both lapic and x2apic. > > I'm going to apply the minimal fix of checking for id == 0xff in > acpi_lapic_parse() for now, but this needs to be revisited and fixed > proper. Good to me. Thanks for fixing it. Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Fri, Oct 7, 2016 at 6:00 AM, Thomas Gleixner wrote: > On Fri, 7 Oct 2016, Thomas Gleixner wrote: >> On Fri, 7 Oct 2016, Dou Liyang wrote: >> > Is it possible that the "-1/ox" could appear in the MADT which is >> > one >> > of the ACPI tables? >> >> According to the SDM the x2apic id is a 32bit ID, so 0x is a >> legitimate value. > > The ACPI spec says that bit 0 of the x2apic flags field tells whether the > logical processor is present or not. So the proper check for x2apic is that > flag. > > The lapic structure has the same flag, but the kernel ignores the flags for > both lapic and x2apic. > > I'm going to apply the minimal fix of checking for id == 0xff in > acpi_lapic_parse() for now, but this needs to be revisited and fixed > proper. Good to me. Thanks for fixing it. Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, 6 Oct 2016, Dou Liyang wrote: > > + if (processor->id >= 255) { > + ++disabled_cpus; Incrementing disabled_cpus here is simply wrong because 0xff is an invalid local APIC id. So we can simply return -EINVAL and be done with it. > + return -EINVAL; Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, 6 Oct 2016, Dou Liyang wrote: > > + if (processor->id >= 255) { > + ++disabled_cpus; Incrementing disabled_cpus here is simply wrong because 0xff is an invalid local APIC id. So we can simply return -EINVAL and be done with it. > + return -EINVAL; Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Fri, 7 Oct 2016, Thomas Gleixner wrote: > On Fri, 7 Oct 2016, Dou Liyang wrote: > > Is it possible that the "-1/ox" could appear in the MADT which is > > one > > of the ACPI tables? > > According to the SDM the x2apic id is a 32bit ID, so 0x is a > legitimate value. The ACPI spec says that bit 0 of the x2apic flags field tells whether the logical processor is present or not. So the proper check for x2apic is that flag. The lapic structure has the same flag, but the kernel ignores the flags for both lapic and x2apic. I'm going to apply the minimal fix of checking for id == 0xff in acpi_lapic_parse() for now, but this needs to be revisited and fixed proper. Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Fri, 7 Oct 2016, Thomas Gleixner wrote: > On Fri, 7 Oct 2016, Dou Liyang wrote: > > Is it possible that the "-1/ox" could appear in the MADT which is > > one > > of the ACPI tables? > > According to the SDM the x2apic id is a 32bit ID, so 0x is a > legitimate value. The ACPI spec says that bit 0 of the x2apic flags field tells whether the logical processor is present or not. So the proper check for x2apic is that flag. The lapic structure has the same flag, but the kernel ignores the flags for both lapic and x2apic. I'm going to apply the minimal fix of checking for id == 0xff in acpi_lapic_parse() for now, but this needs to be revisited and fixed proper. Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Fri, 7 Oct 2016, Dou Liyang wrote: > Is it possible that the "-1/ox" could appear in the MADT which is one > of the ACPI tables? According to the SDM the x2apic id is a 32bit ID, so 0x is a legitimate value. Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Fri, 7 Oct 2016, Dou Liyang wrote: > Is it possible that the "-1/ox" could appear in the MADT which is one > of the ACPI tables? According to the SDM the x2apic id is a 32bit ID, so 0x is a legitimate value. Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, 6 Oct 2016, Yinghai Lu wrote: > 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; > > + } So that means, that x2apic_apic_id_valid() is wrong as well
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, 6 Oct 2016, Yinghai Lu wrote: > 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; > > + } So that means, that x2apic_apic_id_valid() is wrong as well
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, 6 Oct 2016, Yinghai Lu wrote: > On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyangwrote: > 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 0x) > so could save some lines. That just makes the code harder to follow and is not really worth the trouble. Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, 6 Oct 2016, Yinghai Lu wrote: > On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyang wrote: > 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 0x) > so could save some lines. That just makes the code harder to follow and is not really worth the trouble. Thanks, tglx
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi Yinghai At 10/07/2016 05:20 AM, Yinghai Lu wrote: On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyangwrote: 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 0x) 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/ox" could appear in the MADT which is one of the ACPI tables? Thanks Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi Yinghai At 10/07/2016 05:20 AM, Yinghai Lu wrote: On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyang 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 0x) 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/ox" could appear in the MADT which is one of the ACPI tables? Thanks Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyangwrote: > 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 0x) so could save some lines. Thanks Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyang 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 0x) so could save some lines. Thanks Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi Yinghai, At 10/06/2016 12:53 PM, Yinghai Lu wrote: On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixnerwrote: @@ -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 _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 = _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
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
Hi Yinghai, At 10/06/2016 12:53 PM, Yinghai Lu wrote: On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner 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 _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 = _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
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixnerwrote: >> @@ -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 _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 = _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; return 1; } Thanks Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner 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 _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 = _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; return 1; } Thanks Yinghai
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Mon, 3 Oct 2016, Yinghai Lu wrote: > init_cpu_node become: > [ 55.477160] init_cpu_to_node: > [ 55.483280] cpu 0 -> apicid 0x0 -> node 0 > [ 55.491558] cpu 1 -> apicid 0xff -> node 1 > [ 55.500017] cpu 2 -> apicid 0x2 -> node 0 > [ 55.508296] cpu 3 -> apicid 0x4 -> node 0 > [ 55.516575] cpu 4 -> apicid 0x6 -> node 0 > ... > > looks like problem is > > acpi_parse_lapic==>acpi_register_lapic==>__generic_processor_info==>allocate_logical_cpuid > > it will take lapic_id[0xff] take cpu index 1. > > Then will have not /dev/cpu/1/msr, that will make the MLC not happy. That's pretty irrelevant whether that MLC thing - whatever it is - is unhappy or not. If it cannot deal with something missing, it's surely not the kernels problem. Unplug the cpu and the file will be missing as well. > -static int acpi_register_lapic(int id, u32 acpiid, u8 enabled) > +static int acpi_register_lapic(int id, u32 acpiid, u8 enabled, int > disabled_id) > { > unsigned int ver = 0; > int cpu; This is whitespace damaged. The patch does not apply. Can you finaly after doing 10 years of kernel development start being more careful? > @@ -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? Thanks, tglx 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
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Mon, 3 Oct 2016, Yinghai Lu wrote: > init_cpu_node become: > [ 55.477160] init_cpu_to_node: > [ 55.483280] cpu 0 -> apicid 0x0 -> node 0 > [ 55.491558] cpu 1 -> apicid 0xff -> node 1 > [ 55.500017] cpu 2 -> apicid 0x2 -> node 0 > [ 55.508296] cpu 3 -> apicid 0x4 -> node 0 > [ 55.516575] cpu 4 -> apicid 0x6 -> node 0 > ... > > looks like problem is > > acpi_parse_lapic==>acpi_register_lapic==>__generic_processor_info==>allocate_logical_cpuid > > it will take lapic_id[0xff] take cpu index 1. > > Then will have not /dev/cpu/1/msr, that will make the MLC not happy. That's pretty irrelevant whether that MLC thing - whatever it is - is unhappy or not. If it cannot deal with something missing, it's surely not the kernels problem. Unplug the cpu and the file will be missing as well. > -static int acpi_register_lapic(int id, u32 acpiid, u8 enabled) > +static int acpi_register_lapic(int id, u32 acpiid, u8 enabled, int > disabled_id) > { > unsigned int ver = 0; > int cpu; This is whitespace damaged. The patch does not apply. Can you finaly after doing 10 years of kernel development start being more careful? > @@ -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? Thanks, tglx 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
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, Sep 22, 2016 at 12:10 PM, tip-bot for Gu Zhengwrote: > > x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping > > The whole patch-set aims at making cpuid <-> nodeid mapping persistent. So > that, > when node online/offline happens, cache based on cpuid <-> nodeid mapping > such as > wq_numa_possible_cpumask will not cause any problem. > It contains 4 steps: > 1. Enable apic registeration flow to handle both enabled and disabled cpus. > 2. Introduce a new array storing all possible cpuid <-> apicid mapping. > 3. Enable _MAT and MADT relative apis to return non-present or disabled cpus' > apicid. > 4. Establish all possible cpuid <-> nodeid mapping. > > This patch finishes step 2. > > In this patch, we introduce a new static array named cpuid_to_apicid[], > which is large enough to store info for all possible cpus. > > And then, we modify the cpuid calculation. In generic_processor_info(), > it simply finds the next unused cpuid. And it is also why the cpuid <-> nodeid > mapping changes with node hotplug. > > After this patch, we find the next unused cpuid, map it to an apicid, > and store the mapping in cpuid_to_apicid[], so that cpuid <-> apicid > mapping will be persistent. > > And finally we will use this array to make cpuid <-> nodeid persistent. > > cpuid <-> apicid mapping is established at local apic registeration time. > But non-present or disabled cpus are ignored. > > In this patch, we establish all possible cpuid <-> apicid mapping when > registering local apic. Hi, This one cause one regression on 8 sockets system: MLC from intel does not run anymore. the root cause is : cpu index used to be 0-447. with this patch, cpu index change to 0, 2-448. The MADT from system is 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) [ 42.144598] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 42.156836] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) ... [ 47.552852] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 47.565088] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 47.577322] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 47.589561] ACPI: X2APIC (uid[0x00] apic_id[0x00] enabled) [ 47.600899] ACPI: X2APIC (uid[0x02] apic_id[0x02] enabled) [ 47.612234] ACPI: X2APIC (uid[0x04] apic_id[0x04] enabled) ... init_cpu_node become: [ 55.477160] init_cpu_to_node: [ 55.483280] cpu 0 -> apicid 0x0 -> node 0 [ 55.491558] cpu 1 -> apicid 0xff -> node 1 [ 55.500017] cpu 2 -> apicid 0x2 -> node 0 [ 55.508296] cpu 3 -> apicid 0x4 -> node 0 [ 55.516575] cpu 4 -> apicid 0x6 -> node 0 ... looks like problem is acpi_parse_lapic==>acpi_register_lapic==>__generic_processor_info==>allocate_logical_cpuid it will take lapic_id[0xff] take cpu index 1. Then will have not /dev/cpu/1/msr, that will make the MLC not happy. Following change could workaround the problem at this point. Index: linux-2.6/arch/x86/kernel/acpi/boot.c === --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c +++ linux-2.6/arch/x86/kernel/acpi/boot.c @@ -163,10 +163,11 @@ static int __init acpi_parse_madt(struct * @id: local apic id to register * @acpiid: ACPI id to register * @enabled: this cpu is enabled or not + * @disabled_id: not used apic id * * Returns the logic cpu number which maps to the local apic */ -static int acpi_register_lapic(int id, u32 acpiid, u8 enabled) +static int acpi_register_lapic(int id, u32 acpiid, u8 enabled, int disabled_id) { unsigned int ver = 0; int cpu; @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u return -EINVAL; } +if (!enabled && (id == disabled_id)) { +++disabled_cpus; +return -EINVAL; +} + if (boot_cpu_physical_apicid != -1U) ver = boot_cpu_apic_version; @@ -213,7 +219,7 @@ acpi_parse_x2apic(struct acpi_subtable_h if (!apic->apic_id_valid(apic_id) && enabled) printk(KERN_WARNING PREFIX "x2apic entry ignored\n"); else -acpi_register_lapic(apic_id, processor->uid, enabled); +acpi_register_lapic(apic_id, processor->uid, enabled, -1); #else printk(KERN_WARNING PREFIX "x2apic entry ignored\n"); #endif @@ -242,7 +248,7 @@ acpi_parse_lapic(struct acpi_subtable_he */ acpi_register_lapic(processor->id,/* APIC ID */ processor->processor_id, /* ACPI ID */ -processor->lapic_flags & ACPI_MADT_ENABLED); +processor->lapic_flags & ACPI_MADT_ENABLED, 0xff); return 0; } @@ -261,7 +267,7 @@ acpi_parse_sapic(struct acpi_subtable_he acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */ processor->processor_id, /* ACPI ID */ -processor->lapic_flags &
Re: [tip:x86/apic] x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
On Thu, Sep 22, 2016 at 12:10 PM, tip-bot for Gu Zheng wrote: > > x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping > > The whole patch-set aims at making cpuid <-> nodeid mapping persistent. So > that, > when node online/offline happens, cache based on cpuid <-> nodeid mapping > such as > wq_numa_possible_cpumask will not cause any problem. > It contains 4 steps: > 1. Enable apic registeration flow to handle both enabled and disabled cpus. > 2. Introduce a new array storing all possible cpuid <-> apicid mapping. > 3. Enable _MAT and MADT relative apis to return non-present or disabled cpus' > apicid. > 4. Establish all possible cpuid <-> nodeid mapping. > > This patch finishes step 2. > > In this patch, we introduce a new static array named cpuid_to_apicid[], > which is large enough to store info for all possible cpus. > > And then, we modify the cpuid calculation. In generic_processor_info(), > it simply finds the next unused cpuid. And it is also why the cpuid <-> nodeid > mapping changes with node hotplug. > > After this patch, we find the next unused cpuid, map it to an apicid, > and store the mapping in cpuid_to_apicid[], so that cpuid <-> apicid > mapping will be persistent. > > And finally we will use this array to make cpuid <-> nodeid persistent. > > cpuid <-> apicid mapping is established at local apic registeration time. > But non-present or disabled cpus are ignored. > > In this patch, we establish all possible cpuid <-> apicid mapping when > registering local apic. Hi, This one cause one regression on 8 sockets system: MLC from intel does not run anymore. the root cause is : cpu index used to be 0-447. with this patch, cpu index change to 0, 2-448. The MADT from system is 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) [ 42.144598] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 42.156836] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) ... [ 47.552852] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 47.565088] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 47.577322] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled) [ 47.589561] ACPI: X2APIC (uid[0x00] apic_id[0x00] enabled) [ 47.600899] ACPI: X2APIC (uid[0x02] apic_id[0x02] enabled) [ 47.612234] ACPI: X2APIC (uid[0x04] apic_id[0x04] enabled) ... init_cpu_node become: [ 55.477160] init_cpu_to_node: [ 55.483280] cpu 0 -> apicid 0x0 -> node 0 [ 55.491558] cpu 1 -> apicid 0xff -> node 1 [ 55.500017] cpu 2 -> apicid 0x2 -> node 0 [ 55.508296] cpu 3 -> apicid 0x4 -> node 0 [ 55.516575] cpu 4 -> apicid 0x6 -> node 0 ... looks like problem is acpi_parse_lapic==>acpi_register_lapic==>__generic_processor_info==>allocate_logical_cpuid it will take lapic_id[0xff] take cpu index 1. Then will have not /dev/cpu/1/msr, that will make the MLC not happy. Following change could workaround the problem at this point. Index: linux-2.6/arch/x86/kernel/acpi/boot.c === --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c +++ linux-2.6/arch/x86/kernel/acpi/boot.c @@ -163,10 +163,11 @@ static int __init acpi_parse_madt(struct * @id: local apic id to register * @acpiid: ACPI id to register * @enabled: this cpu is enabled or not + * @disabled_id: not used apic id * * Returns the logic cpu number which maps to the local apic */ -static int acpi_register_lapic(int id, u32 acpiid, u8 enabled) +static int acpi_register_lapic(int id, u32 acpiid, u8 enabled, int disabled_id) { unsigned int ver = 0; int cpu; @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u return -EINVAL; } +if (!enabled && (id == disabled_id)) { +++disabled_cpus; +return -EINVAL; +} + if (boot_cpu_physical_apicid != -1U) ver = boot_cpu_apic_version; @@ -213,7 +219,7 @@ acpi_parse_x2apic(struct acpi_subtable_h if (!apic->apic_id_valid(apic_id) && enabled) printk(KERN_WARNING PREFIX "x2apic entry ignored\n"); else -acpi_register_lapic(apic_id, processor->uid, enabled); +acpi_register_lapic(apic_id, processor->uid, enabled, -1); #else printk(KERN_WARNING PREFIX "x2apic entry ignored\n"); #endif @@ -242,7 +248,7 @@ acpi_parse_lapic(struct acpi_subtable_he */ acpi_register_lapic(processor->id,/* APIC ID */ processor->processor_id, /* ACPI ID */ -processor->lapic_flags & ACPI_MADT_ENABLED); +processor->lapic_flags & ACPI_MADT_ENABLED, 0xff); return 0; } @@ -261,7 +267,7 @@ acpi_parse_sapic(struct acpi_subtable_he acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */ processor->processor_id, /* ACPI ID */ -processor->lapic_flags & ACPI_MADT_ENABLED); +