* Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT [not found] <alpine.DEB.2.11.1507211017590.18576 () nanos> @ 2015-07-30 17:43 ` Lukasz Anaczkowski 2015-07-30 17:43 ` [PATCH] x86, acpi: Handle lapic/x2apic " Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-07-30 17:43 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi > Please add proper explanations why the array parser is required and > why the parse order needs to be reverse. Thanks for comments. I'm sending patch with reworked commit message. I hope that at least some of the doubts are cleaner now. Cheers, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-07-30 17:43 ` [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT Lukasz Anaczkowski @ 2015-07-30 17:43 ` Lukasz Anaczkowski 2015-08-02 9:57 ` Thomas Gleixner 2015-08-02 12:40 ` Marc Zyngier 0 siblings, 2 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-07-30 17:43 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski, Yinghai Lu >From the ACPI spec: "Logical processors with APIC ID values less than 0xFF (whether in XAPIC or X2APIC mode) must use the Processor LAPIC structure [...]. Logical processors with APIC ID values 0xFF and greater must use the Processor Local x2APIC structure." Because of above, BIOS is first enumerating cores with HT with LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF). With current kernel code, where enumeration is in order: BSP, X2APIC, LAPIC enumeration on machine with more than 255 CPUs (each core with 4 HT) first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs get higher logical CPU IDs (50..y), as in example below: Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId 0 0 0000 97 0001 145 0002 193 0003 1 50 0004 98 0005 146 0006 194 0007 2 51 0010 99 0011 147 0012 195 0013 3 52 0014 100 0015 148 0016 196 0017 4 53 0018 101 0019 149 001a 197 001b 5 54 001c 102 001d 150 001e 198 001f ... 62 95 00f8 143 00f9 191 00fa 239 00fb 63 37 00ff 96 00fc 144 00fd 192 00fe 64 1 0100 13 0101 25 0102 38 0103 65 2 0104 14 0105 26 0106 39 0107 ... (Core - physical core, LCpu - logical CPU, ApicId - ID assigned by BIOS). This is wrong for the following reasons: () it's hard to predict how cores and threads will be enumerated () when it's hard to predict, s/w threads cannot be properly affinitized causing significant performance impact due to e.g. inproper cache sharing () enumeration is inconsistent with how threads are enumerated on other Intel Xeon processors To fix this, each LAPIC/X2APIC entry from MADT table needs to be handled at the same time when processing it, thus adding acpi_subtable_proc structure which stores () ACPI table id () handler that processes table () counter how many items has been processed and passing it to acpi_table_parse_entries(). Also, order in which MADT LAPIC/X2APIC handlers are passed is reversed to achieve correct CPU enumeration. In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', in result less cores may be booted, since some of the CPUs the kernel will try to use will have APIC ID >= 0xFF. In such case, one should not pass 'nox2apic'. Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since 2009, when X2APIC support was initially added. I do not know why MADT parsing code was added in the reversed order in the first place. I guess it didn't matter at that time since nobody cared about cores with APIC IDs >= 0xFF, right? This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563, thus putting Yinghai Lu as 'Signed-off-by', as well. Signed-off-by: Yinghai Lu <yinghai@kernel.org> Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- drivers/acpi/numa.c | 28 ++++++++++++----- drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- drivers/irqchip/irq-gic.c | 15 ++++++--- include/linux/acpi.h | 13 ++++++-- 5 files changed, 111 insertions(+), 49 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..fb4a9d6 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + count = madt_proc[0].count; + x2count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) return count; } - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, - acpi_parse_x2apic_nmi, 0); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, - acpi_parse_lapic_nmi, 0); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; + madt_proc[0].handler = acpi_parse_x2apic_nmi; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; + madt_proc[1].handler = acpi_parse_lapic_nmi; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), 0); + count = madt_proc[0].count; + x2count = madt_proc[1].count; if (count < 0 || x2count < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); /* TBD: Cleanup to allow fallback to MPS */ diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c index acaa3b4..a000195 100644 --- a/drivers/acpi/numa.c +++ b/drivers/acpi/numa.c @@ -314,9 +314,15 @@ static int __init acpi_table_parse_srat(enum acpi_srat_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_SRAT, - sizeof(struct acpi_table_srat), id, - handler, max_entries); + struct acpi_subtable_proc srat_proc; + + memset(&srat_proc, 0, sizeof(srat_proc)); + srat_proc.id = id; + srat_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + &srat_proc, 1, max_entries); } int __init acpi_numa_init(void) @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) /* SRAT: Static Resource Affinity Table */ if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, - acpi_parse_x2apic_affinity, 0); - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, - acpi_parse_processor_affinity, 0); + struct acpi_subtable_proc srat_proc[2]; + + memset(srat_proc, 0, sizeof(srat_proc)); + srat_proc[0].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; + srat_proc[0].handler = acpi_parse_x2apic_affinity; + srat_proc[1].id = ACPI_SRAT_TYPE_CPU_AFFINITY; + srat_proc[1].handler = acpi_parse_processor_affinity; + + acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + srat_proc, ARRAY_SIZE(srat_proc), 0); + cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..1217e41 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -216,25 +216,27 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; int count = 0; unsigned long table_end; + int i; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; - - if (!table_size) + } + if (!table_size) { + proc[0].count = -EINVAL; return -EINVAL; + } if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } @@ -247,12 +249,17 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (max_entries && count++ >= max_entries) + continue; + if (proc[i].handler(entry, table_end)) { + proc[i].count = -EINVAL; return -EINVAL; - - count++; + } + proc[i].count++; + break; } /* @@ -260,7 +267,11 @@ acpi_parse_entries(char *id, unsigned long table_size, * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] Invalid zero length\n"); + proc[0].count = -EINVAL; return -EINVAL; } @@ -269,18 +280,20 @@ acpi_parse_entries(char *id, unsigned long table_size, } if (max_entries && count > max_entries) { - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] ignored %i entries of %i found\n", + count-max_entries, count); } return count; } int __init -acpi_table_parse_entries(char *id, +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -288,11 +301,10 @@ acpi_table_parse_entries(char *id, int count; u32 instance = 0; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; + } if (!strncmp(id, ACPI_SIG_MADT, 4)) instance = acpi_apic_instance; @@ -300,11 +312,12 @@ acpi_table_parse_entries(char *id, acpi_get_table_with_size(id, instance, &table_header, &tbl_size); if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; @@ -314,9 +327,15 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), id, - handler, max_entries); + struct acpi_subtable_proc madt_proc; + + memset(&madt_proc, 0, sizeof(madt_proc)); + madt_proc.id = id; + madt_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + &madt_proc, 1, max_entries); } /** diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4dd8826..d004a32 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) { void __iomem *cpu_base, *dist_base; int count; + struct acpi_subtable_proc gic_proc; + + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; + gic_proc.handler = gic_acpi_parse_madt_cpu; /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_cpu, table, - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); + table, gic_proc, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) * Find distributor base address. We expect one distributor entry since * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. */ + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; + gic_proc.handler = gic_acpi_parse_madt_distributor; + count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_distributor, table, - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); + table, gic_proc, 0); if (count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..59b17e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -145,10 +151,13 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); +int acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries); + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-07-30 17:43 ` [PATCH] x86, acpi: Handle lapic/x2apic " Lukasz Anaczkowski @ 2015-08-02 9:57 ` Thomas Gleixner 2015-08-02 12:40 ` Marc Zyngier 1 sibling, 0 replies; 52+ messages in thread From: Thomas Gleixner @ 2015-08-02 9:57 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: mingo, H. Peter Anvin, x86, Jason Cooper, rjw, len.brown, pavel, linux-pm, LKML, linux-acpi, Yinghai Lu, Marc Zyngier On Thu, 30 Jul 2015, Lukasz Anaczkowski wrote: > Also, order in which MADT LAPIC/X2APIC handlers are passed is > reversed to achieve correct CPU enumeration. Ok. That is an understandable changelog and the patch itself looks about correct, but I leave that judgement to the ACPI experts. This also wants an ack/review from Marc for the GIC part. Acked-by: Thomas Gleixner <tglx@linutronix.de> ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-07-30 17:43 ` [PATCH] x86, acpi: Handle lapic/x2apic " Lukasz Anaczkowski 2015-08-02 9:57 ` Thomas Gleixner @ 2015-08-02 12:40 ` Marc Zyngier 2015-08-03 18:26 ` Lukasz Anaczkowski 1 sibling, 1 reply; 52+ messages in thread From: Marc Zyngier @ 2015-08-02 12:40 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: tglx, mingo, hpa, x86, jason, rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On Thu, 30 Jul 2015 19:43:39 +0200 Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote: > From the ACPI spec: > "Logical processors with APIC ID values less than 0xFF > (whether in XAPIC or X2APIC mode) must use the Processor LAPIC > structure [...]. Logical processors with APIC ID values 0xFF and > greater must use the Processor Local x2APIC structure." > > Because of above, BIOS is first enumerating cores with HT with > LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF). > > With current kernel code, where enumeration is in order: > BSP, X2APIC, LAPIC > enumeration on machine with more than 255 CPUs (each core with 4 HT) > first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs > get higher logical CPU IDs (50..y), as in example below: > > Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId > 0 0 0000 97 0001 145 0002 193 0003 > 1 50 0004 98 0005 146 0006 194 0007 > 2 51 0010 99 0011 147 0012 195 0013 > 3 52 0014 100 0015 148 0016 196 0017 > 4 53 0018 101 0019 149 001a 197 001b > 5 54 001c 102 001d 150 001e 198 001f > ... > 62 95 00f8 143 00f9 191 00fa 239 00fb > 63 37 00ff 96 00fc 144 00fd 192 00fe > 64 1 0100 13 0101 25 0102 38 0103 > 65 2 0104 14 0105 26 0106 39 0107 > ... > > (Core - physical core, LCpu - logical CPU, ApicId - ID assigned > by BIOS). > > This is wrong for the following reasons: > () it's hard to predict how cores and threads will be enumerated > () when it's hard to predict, s/w threads cannot be properly affinitized > causing significant performance impact due to e.g. inproper cache > sharing > () enumeration is inconsistent with how threads are enumerated on > other Intel Xeon processors > > To fix this, each LAPIC/X2APIC entry from MADT table needs to be > handled at the same time when processing it, thus adding > acpi_subtable_proc structure which stores > () ACPI table id > () handler that processes table > () counter how many items has been processed > and passing it to acpi_table_parse_entries(). > > Also, order in which MADT LAPIC/X2APIC handlers are passed is > reversed to achieve correct CPU enumeration. > > In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', > in result less cores may be booted, since some of the CPUs the kernel > will try to use will have APIC ID >= 0xFF. In such case, one > should not pass 'nox2apic'. > > Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since 2009, > when X2APIC support was initially added. I do not know why MADT parsing > code was added in the reversed order in the first place. > I guess it didn't matter at that time since nobody cared about cores > with APIC IDs >= 0xFF, right? > > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563, > thus putting Yinghai Lu as 'Signed-off-by', as well. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> > --- > arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- > drivers/acpi/numa.c | 28 ++++++++++++----- > drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- > drivers/irqchip/irq-gic.c | 15 ++++++--- > include/linux/acpi.h | 13 ++++++-- > 5 files changed, 111 insertions(+), 49 deletions(-) > Hi Lukasz, [...] > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 4dd8826..d004a32 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) > { > void __iomem *cpu_base, *dist_base; > int count; > + struct acpi_subtable_proc gic_proc; > + > + memset(gic_proc, 0, sizeof(gic_proc)); You haven't ever tried compiling this, have you? > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; > + gic_proc.handler = gic_acpi_parse_madt_cpu; > > /* Collect CPU base addresses */ > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_cpu, table, > - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); > + table, gic_proc, 0); This doesn't match the prototype below. > if (count <= 0) { > pr_err("No valid GICC entries exist\n"); > return -EINVAL; > @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) > * Find distributor base address. We expect one distributor entry since > * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. > */ > + memset(gic_proc, 0, sizeof(gic_proc)); > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; > + gic_proc.handler = gic_acpi_parse_madt_distributor; > + > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_distributor, table, > - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); > + table, gic_proc, 0); > if (count <= 0) { > pr_err("No valid GICD entries exist\n"); > return -EINVAL; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index d2445fa..59b17e8 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) > (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) > > +struct acpi_subtable_proc { > + int id; > + acpi_tbl_entry_handler handler; > + int count; > +}; > + > char * __acpi_map_table (unsigned long phys_addr, unsigned long size); > void __acpi_unmap_table(char *map, unsigned long size); > int early_acpi_boot_init(void); > @@ -145,10 +151,13 @@ int acpi_numa_init (void); > > int acpi_table_init (void); > int acpi_table_parse(char *id, acpi_tbl_table_handler handler); > +int acpi_table_parse_entries_array(char *id, unsigned long table_size, > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int __init acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries); > + struct acpi_subtable_proc *proc, int proc_num, Could you please check that it actually compiles when you enable ACPI on arm64? Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-02 12:40 ` Marc Zyngier @ 2015-08-03 18:26 ` Lukasz Anaczkowski 2015-08-03 18:26 ` Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-08-03 18:26 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi Hi Marc, > You haven't ever tried compiling this, have you? I *thought* I tried, but obviously I did it wrong. This time I made sure it compiles. Thanks for poiting this out. Sending v2 of the patch with fixed arm64 compilation. Cheers, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-03 18:26 ` Lukasz Anaczkowski @ 2015-08-03 18:26 ` Lukasz Anaczkowski 2015-08-26 7:04 ` Anaczkowski, Lukasz ` (2 more replies) 0 siblings, 3 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-08-03 18:26 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski, Yinghai Lu v2: Fixed ARM64 syntax error >From the ACPI spec: "Logical processors with APIC ID values less than 0xFF (whether in XAPIC or X2APIC mode) must use the Processor LAPIC structure [...]. Logical processors with APIC ID values 0xFF and greater must use the Processor Local x2APIC structure." Because of above, BIOS is first enumerating cores with HT with LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF). With current kernel code, where enumeration is in order: BSP, X2APIC, LAPIC enumeration on machine with more than 255 CPUs (each core with 4 HT) first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs get higher logical CPU IDs (50..y), as in example below: Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId 0 0 0000 97 0001 145 0002 193 0003 1 50 0004 98 0005 146 0006 194 0007 2 51 0010 99 0011 147 0012 195 0013 3 52 0014 100 0015 148 0016 196 0017 4 53 0018 101 0019 149 001a 197 001b 5 54 001c 102 001d 150 001e 198 001f ... 62 95 00f8 143 00f9 191 00fa 239 00fb 63 37 00ff 96 00fc 144 00fd 192 00fe 64 1 0100 13 0101 25 0102 38 0103 65 2 0104 14 0105 26 0106 39 0107 ... (Core - physical core, LCpu - logical CPU, ApicId - ID assigned by BIOS). This is wrong for the following reasons: () it's hard to predict how cores and threads will be enumerated () when it's hard to predict, s/w threads cannot be properly affinitized causing significant performance impact due to e.g. inproper cache sharing () enumeration is inconsistent with how threads are enumerated on other Intel Xeon processors To fix this, each LAPIC/X2APIC entry from MADT table needs to be handled at the same time when processing it, thus adding acpi_subtable_proc structure which stores () ACPI table id () handler that processes table () counter how many items has been processed and passing it to acpi_table_parse_entries(). Also, order in which MADT LAPIC/X2APIC handlers are passed is reversed to achieve correct CPU enumeration. In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', in result less cores may be booted, since some of the CPUs the kernel will try to use will have APIC ID >= 0xFF. In such case, one should not pass 'nox2apic'. Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since 2009, when X2APIC support was initially added. I do not know why MADT parsing code was added in the reversed order in the first place. I guess it didn't matter at that time since nobody cared about cores with APIC IDs >= 0xFF, right? This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563, thus putting Yinghai Lu as 'Signed-off-by', as well. Signed-off-by: Yinghai Lu <yinghai@kernel.org> Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- drivers/acpi/numa.c | 28 ++++++++++++----- drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- drivers/irqchip/irq-gic.c | 15 ++++++--- include/linux/acpi.h | 13 ++++++-- 5 files changed, 111 insertions(+), 49 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..fb4a9d6 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + count = madt_proc[0].count; + x2count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) return count; } - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, - acpi_parse_x2apic_nmi, 0); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, - acpi_parse_lapic_nmi, 0); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; + madt_proc[0].handler = acpi_parse_x2apic_nmi; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; + madt_proc[1].handler = acpi_parse_lapic_nmi; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), 0); + count = madt_proc[0].count; + x2count = madt_proc[1].count; if (count < 0 || x2count < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); /* TBD: Cleanup to allow fallback to MPS */ diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c index acaa3b4..a000195 100644 --- a/drivers/acpi/numa.c +++ b/drivers/acpi/numa.c @@ -314,9 +314,15 @@ static int __init acpi_table_parse_srat(enum acpi_srat_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_SRAT, - sizeof(struct acpi_table_srat), id, - handler, max_entries); + struct acpi_subtable_proc srat_proc; + + memset(&srat_proc, 0, sizeof(srat_proc)); + srat_proc.id = id; + srat_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + &srat_proc, 1, max_entries); } int __init acpi_numa_init(void) @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) /* SRAT: Static Resource Affinity Table */ if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, - acpi_parse_x2apic_affinity, 0); - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, - acpi_parse_processor_affinity, 0); + struct acpi_subtable_proc srat_proc[2]; + + memset(srat_proc, 0, sizeof(srat_proc)); + srat_proc[0].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; + srat_proc[0].handler = acpi_parse_x2apic_affinity; + srat_proc[1].id = ACPI_SRAT_TYPE_CPU_AFFINITY; + srat_proc[1].handler = acpi_parse_processor_affinity; + + acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + srat_proc, ARRAY_SIZE(srat_proc), 0); + cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..1217e41 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -216,25 +216,27 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; int count = 0; unsigned long table_end; + int i; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; - - if (!table_size) + } + if (!table_size) { + proc[0].count = -EINVAL; return -EINVAL; + } if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } @@ -247,12 +249,17 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (max_entries && count++ >= max_entries) + continue; + if (proc[i].handler(entry, table_end)) { + proc[i].count = -EINVAL; return -EINVAL; - - count++; + } + proc[i].count++; + break; } /* @@ -260,7 +267,11 @@ acpi_parse_entries(char *id, unsigned long table_size, * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] Invalid zero length\n"); + proc[0].count = -EINVAL; return -EINVAL; } @@ -269,18 +280,20 @@ acpi_parse_entries(char *id, unsigned long table_size, } if (max_entries && count > max_entries) { - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] ignored %i entries of %i found\n", + count-max_entries, count); } return count; } int __init -acpi_table_parse_entries(char *id, +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -288,11 +301,10 @@ acpi_table_parse_entries(char *id, int count; u32 instance = 0; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; + } if (!strncmp(id, ACPI_SIG_MADT, 4)) instance = acpi_apic_instance; @@ -300,11 +312,12 @@ acpi_table_parse_entries(char *id, acpi_get_table_with_size(id, instance, &table_header, &tbl_size); if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; @@ -314,9 +327,15 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), id, - handler, max_entries); + struct acpi_subtable_proc madt_proc; + + memset(&madt_proc, 0, sizeof(madt_proc)); + madt_proc.id = id; + madt_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + &madt_proc, 1, max_entries); } /** diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4dd8826..d98b866 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) { void __iomem *cpu_base, *dist_base; int count; + struct acpi_subtable_proc gic_proc; + + memset(&gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; + gic_proc.handler = gic_acpi_parse_madt_cpu; /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_cpu, table, - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); + table, &gic_proc, 1, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) * Find distributor base address. We expect one distributor entry since * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. */ + memset(&gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; + gic_proc.handler = gic_acpi_parse_madt_distributor; + count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_distributor, table, - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); + table, &gic_proc, 1, 0); if (count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..59b17e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -145,10 +151,13 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); +int acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries); + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* RE: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-03 18:26 ` Lukasz Anaczkowski @ 2015-08-26 7:04 ` Anaczkowski, Lukasz 2015-08-26 10:43 ` Marc Zyngier 2015-08-26 11:03 ` [PATCH] x86, " Marc Zyngier 2015-08-26 12:56 ` Tomasz Nowicki 2 siblings, 1 reply; 52+ messages in thread From: Anaczkowski, Lukasz @ 2015-08-26 7:04 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason, marc.zyngier Cc: rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On Monday, August 3, 2015 8:26 PM Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote: > v2: Fixed ARM64 syntax error Hi Marc, Does this patch look ok now? Thanks, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 7:04 ` Anaczkowski, Lukasz @ 2015-08-26 10:43 ` Marc Zyngier 2015-08-26 11:42 ` Lorenzo Pieralisi 0 siblings, 1 reply; 52+ messages in thread From: Marc Zyngier @ 2015-08-26 10:43 UTC (permalink / raw) To: Anaczkowski, Lukasz, tglx, mingo, hpa, x86, jason Cc: rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu Hi Lukasz, On 26/08/15 08:04, Anaczkowski, Lukasz wrote: > On Monday, August 3, 2015 8:26 PM > Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote: > >> v2: Fixed ARM64 syntax error > > Hi Marc, > > Does this patch look ok now? Contrarily to popular belief, I do not read every email on LKML. Crazy, I know. How about cc-ing me next time? Even your reply to my original rant wasn't addressed to me (how did you manage that?)... I'll have a look now. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 10:43 ` Marc Zyngier @ 2015-08-26 11:42 ` Lorenzo Pieralisi 2015-08-26 12:43 ` Marc Zyngier 0 siblings, 1 reply; 52+ messages in thread From: Lorenzo Pieralisi @ 2015-08-26 11:42 UTC (permalink / raw) To: Marc Zyngier Cc: Anaczkowski, Lukasz, tglx, mingo, hpa, x86, jason, rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu Hi Lukasz, On Wed, Aug 26, 2015 at 11:43:04AM +0100, Marc Zyngier wrote: > Hi Lukasz, > > On 26/08/15 08:04, Anaczkowski, Lukasz wrote: > > On Monday, August 3, 2015 8:26 PM > > Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote: > > > >> v2: Fixed ARM64 syntax error > > > > Hi Marc, > > > > Does this patch look ok now? No it does not, it seems to break arm64, I put together a fix below. I do not think the way you handle the count increment in acpi_parse_entries() is correct anyway, since you increment it only if max_entries != 0, which changes mainline behaviour. Thanks, Lorenzo -- >8 -- diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index d98b866..ca5591d 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1101,7 +1101,7 @@ gic_v2_acpi_init(struct acpi_table_header *table) count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), table, &gic_proc, 1, 0); - if (count <= 0) { + if (gic_proc.count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; } @@ -1117,7 +1117,7 @@ gic_v2_acpi_init(struct acpi_table_header *table) count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), table, &gic_proc, 1, 0); - if (count <= 0) { + if (gic_proc.count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; } else if (count > 1) { -- 2.2.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 11:42 ` Lorenzo Pieralisi @ 2015-08-26 12:43 ` Marc Zyngier 2015-08-26 17:49 ` Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Marc Zyngier @ 2015-08-26 12:43 UTC (permalink / raw) To: Lorenzo Pieralisi, Anaczkowski, Lukasz Cc: tglx, mingo, hpa, x86, jason, rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On 26/08/15 12:42, Lorenzo Pieralisi wrote: > Hi Lukasz, > > On Wed, Aug 26, 2015 at 11:43:04AM +0100, Marc Zyngier wrote: >> Hi Lukasz, >> >> On 26/08/15 08:04, Anaczkowski, Lukasz wrote: >>> On Monday, August 3, 2015 8:26 PM >>> Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote: >>> >>>> v2: Fixed ARM64 syntax error >>> >>> Hi Marc, >>> >>> Does this patch look ok now? > > No it does not, it seems to break arm64, I put together a fix > below. I do not think the way you handle the count increment > in acpi_parse_entries() is correct anyway, since you increment > it only if max_entries != 0, which changes mainline behaviour. Yeah, this is fundamentally flawed: - count is only incremented when max_entries != 0, as you noticed - With max_entries != 0, count now represent the sum of all matches Is that expected? - The proc iteration stops after the first match. Why? - The test for max_entries is done inside the proc loop. Why? I came up with the following patch that restores arm64 to a booting state. If the intention was to change the meaning of the acpi_parse_entries return value, then this should be documented and agreed upon. diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 1217e41..f06327f 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -249,19 +249,24 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { + bool match = false; + + if (max_entries && count >= max_entries) + break; for (i = 0; i < proc_num; i++) { if (entry->type != proc[i].id) continue; - if (max_entries && count++ >= max_entries) - continue; if (proc[i].handler(entry, table_end)) { proc[i].count = -EINVAL; return -EINVAL; } proc[i].count++; - break; + match = true; } + if (match) + count++; + /* * If entry->length is 0, break from this loop to avoid * infinite loop. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 12:43 ` Marc Zyngier @ 2015-08-26 17:49 ` Lukasz Anaczkowski 2015-08-26 17:49 ` [PATCH] x86, arm64, " Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-08-26 17:49 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi Marc nad Lorenzo, First of all appologies for breaking arm64 (again) and thank you for debugging effort. I own you. > - count is only incremented when max_entries != 0, as you noticed You are right, sorry for that, it's fixed in v3. > - With max_entries != 0, count now represent the sum of all matches > Is that expected? I have no strong opinion on that one. All of the x86 ACPI entries handling only checks for count < 0, or uses count from the acpi_subtable_proc structure (and that's why I didn't noticed the mainline breakage). If you think it's not correct or less usable than other approach, let me know. > - The proc iteration stops after the first match. Why? So, the initial implementation of the acpi_parse_entries accepted single handler for the ACPI table. Now, with this change, assumption is that different handlers for different tables/subtables are passed, meaning only one can meet entry->type == proc[i].id condition. mainline breakage). This approach saves one local varaible, but I don't think this is ultimate argument :) > - The test for max_entries is done inside the proc loop. Why? That's obviously wrong in context of the overall wrong counting. > [...] this should be documented and agreed upon. I've added description with assumptions. Again, if you think it's not correct, let me know. Tomasz Nowicki wrote: > should acpi_table_parse_entries suppose to be removed above? Thanks for pointing this out. I've missed implementation of acpi_table_parse_entries when was backporting initial patch. I've added it back. Cheers, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 17:49 ` Lukasz Anaczkowski @ 2015-08-26 17:49 ` Lukasz Anaczkowski 2015-08-27 9:37 ` Lorenzo Pieralisi ` (2 more replies) 0 siblings, 3 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-08-26 17:49 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski, Yinghai Lu v3: () Fixed entries counting () Added missing acpi_table_parse_entries definition () acpi_parse_entries() now returns sum of all matching entries v2: Fixed ARM64 syntax error >From the ACPI spec: "Logical processors with APIC ID values less than 0xFF (whether in XAPIC or X2APIC mode) must use the Processor LAPIC structure [...]. Logical processors with APIC ID values 0xFF and greater must use the Processor Local x2APIC structure." Because of above, BIOS is first enumerating cores with HT with LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF). With current kernel code, where enumeration is in order: BSP, X2APIC, LAPIC enumeration on machine with more than 255 CPUs (each core with 4 HT) first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs get higher logical CPU IDs (50..y), as in example below: Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId 0 0 0000 97 0001 145 0002 193 0003 1 50 0004 98 0005 146 0006 194 0007 2 51 0010 99 0011 147 0012 195 0013 3 52 0014 100 0015 148 0016 196 0017 4 53 0018 101 0019 149 001a 197 001b 5 54 001c 102 001d 150 001e 198 001f ... 62 95 00f8 143 00f9 191 00fa 239 00fb 63 37 00ff 96 00fc 144 00fd 192 00fe 64 1 0100 13 0101 25 0102 38 0103 65 2 0104 14 0105 26 0106 39 0107 ... (Core - physical core, LCpu - logical CPU, ApicId - ID assigned by BIOS). This is wrong for the following reasons: () it's hard to predict how cores and threads will be enumerated () when it's hard to predict, s/w threads cannot be properly affinitized causing significant performance impact due to e.g. inproper cache sharing () enumeration is inconsistent with how threads are enumerated on other Intel Xeon processors To fix this, each LAPIC/X2APIC entry from MADT table needs to be handled at the same time when processing it, thus adding acpi_subtable_proc structure which stores () ACPI table id () handler that processes table () counter how many items has been processed and passing it to acpi_table_parse_entries(). Also, order in which MADT LAPIC/X2APIC handlers are passed is reversed to achieve correct CPU enumeration. In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', in result less cores may be booted, since some of the CPUs the kernel will try to use will have APIC ID >= 0xFF. In such case, one should not pass 'nox2apic'. Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since 2009, when X2APIC support was initially added. I do not know why MADT parsing code was added in the reversed order in the first place. I guess it didn't matter at that time since nobody cared about cores with APIC IDs >= 0xFF, right? This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563, thus putting Yinghai Lu as 'Signed-off-by', as well. Signed-off-by: Yinghai Lu <yinghai@kernel.org> Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> Acked-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/acpi/boot.c | 29 ++++++++---- drivers/acpi/numa.c | 28 ++++++++--- drivers/acpi/tables.c | 113 +++++++++++++++++++++++++++++++++----------- drivers/irqchip/irq-gic.c | 15 ++++-- include/linux/acpi.h | 13 ++++- 5 files changed, 149 insertions(+), 49 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..fb4a9d6 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + count = madt_proc[0].count; + x2count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) return count; } - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, - acpi_parse_x2apic_nmi, 0); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, - acpi_parse_lapic_nmi, 0); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; + madt_proc[0].handler = acpi_parse_x2apic_nmi; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; + madt_proc[1].handler = acpi_parse_lapic_nmi; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), 0); + count = madt_proc[0].count; + x2count = madt_proc[1].count; if (count < 0 || x2count < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); /* TBD: Cleanup to allow fallback to MPS */ diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c index acaa3b4..a000195 100644 --- a/drivers/acpi/numa.c +++ b/drivers/acpi/numa.c @@ -314,9 +314,15 @@ static int __init acpi_table_parse_srat(enum acpi_srat_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_SRAT, - sizeof(struct acpi_table_srat), id, - handler, max_entries); + struct acpi_subtable_proc srat_proc; + + memset(&srat_proc, 0, sizeof(srat_proc)); + srat_proc.id = id; + srat_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + &srat_proc, 1, max_entries); } int __init acpi_numa_init(void) @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) /* SRAT: Static Resource Affinity Table */ if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, - acpi_parse_x2apic_affinity, 0); - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, - acpi_parse_processor_affinity, 0); + struct acpi_subtable_proc srat_proc[2]; + + memset(srat_proc, 0, sizeof(srat_proc)); + srat_proc[0].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; + srat_proc[0].handler = acpi_parse_x2apic_affinity; + srat_proc[1].id = ACPI_SRAT_TYPE_CPU_AFFINITY; + srat_proc[1].handler = acpi_parse_processor_affinity; + + acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + srat_proc, ARRAY_SIZE(srat_proc), 0); + cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..d5c9a1b 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -214,27 +214,45 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } } +/** + * acpi_table_parse - for each proc_num find a subtable with proc->id + * and run proc->handler on it. Assumption is that there's only + * single handler for particular id. + * + * @id: table id (for debugging purposes) + * @table_size: single entry size + * @table_header: where does the table start? + * @proc: array of acpi_subtable_proc struct containing subtable id + * and associated handler with it + * @proc_num: how big proc is? + * @max_entries: how many entries can we process? + * + * On success returns sum of all matching entries for all proc handlers. + * Oterwise, -ENODEV or -EINVAL is returned. + */ int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; int count = 0; unsigned long table_end; + int i; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; - - if (!table_size) + } + if (!table_size) { + proc[0].count = -EINVAL; return -EINVAL; + } if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } @@ -247,20 +265,31 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + if (max_entries && count >= max_entries) + continue; + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (proc[i].handler(entry, table_end)) { + proc[i].count = -EINVAL; return -EINVAL; - - count++; + } + proc[i].count++; + break; } + if (i != proc_num) + count++; /* * If entry->length is 0, break from this loop to avoid * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] Invalid zero length\n"); + proc[0].count = -EINVAL; return -EINVAL; } @@ -269,18 +298,20 @@ acpi_parse_entries(char *id, unsigned long table_size, } if (max_entries && count > max_entries) { - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] ignored %i entries of %i found\n", + count-max_entries, count); } return count; } int __init -acpi_table_parse_entries(char *id, +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -288,11 +319,10 @@ acpi_table_parse_entries(char *id, int count; u32 instance = 0; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; + } if (!strncmp(id, ACPI_SIG_MADT, 4)) instance = acpi_apic_instance; @@ -300,23 +330,50 @@ acpi_table_parse_entries(char *id, acpi_get_table_with_size(id, instance, &table_header, &tbl_size); if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; } int __init +acpi_table_parse_entries(char *id, + unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries) +{ + struct acpi_subtable_proc proc[1]; + + if (!handler) + return -EINVAL; + + memset(proc, 0, sizeof(proc)); + proc[0].id = entry_id; + proc[0].handler = handler; + + return acpi_table_parse_entries_array(id, table_size, proc, 1, + max_entries); +} + +int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), id, - handler, max_entries); + struct acpi_subtable_proc madt_proc; + + memset(&madt_proc, 0, sizeof(madt_proc)); + madt_proc.id = id; + madt_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + &madt_proc, 1, max_entries); } /** diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4dd8826..d98b866 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) { void __iomem *cpu_base, *dist_base; int count; + struct acpi_subtable_proc gic_proc; + + memset(&gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; + gic_proc.handler = gic_acpi_parse_madt_cpu; /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_cpu, table, - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); + table, &gic_proc, 1, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) * Find distributor base address. We expect one distributor entry since * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. */ + memset(&gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; + gic_proc.handler = gic_acpi_parse_madt_distributor; + count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_distributor, table, - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); + table, &gic_proc, 1, 0); if (count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..59b17e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -145,10 +151,13 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); +int acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries); + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 17:49 ` [PATCH] x86, arm64, " Lukasz Anaczkowski @ 2015-08-27 9:37 ` Lorenzo Pieralisi 2015-09-08 11:07 ` Lukasz Anaczkowski 2015-08-28 8:30 ` [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Ingo Molnar 2015-09-01 8:02 ` Tomasz Nowicki 2 siblings, 1 reply; 52+ messages in thread From: Lorenzo Pieralisi @ 2015-08-27 9:37 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: Marc Zyngier, tomasz.nowicki, tglx, mingo, hpa, x86, jason, rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On Wed, Aug 26, 2015 at 06:49:29PM +0100, Lukasz Anaczkowski wrote: [...] > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 2e19189..d5c9a1b 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -214,27 +214,45 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > } > } > > +/** > + * acpi_table_parse - for each proc_num find a subtable with proc->id Name of function does not correspond to the function you are actually commenting. > + * and run proc->handler on it. Assumption is that there's only > + * single handler for particular id. > + * > + * @id: table id (for debugging purposes) > + * @table_size: single entry size > + * @table_header: where does the table start? > + * @proc: array of acpi_subtable_proc struct containing subtable id > + * and associated handler with it > + * @proc_num: how big proc is? > + * @max_entries: how many entries can we process? > + * > + * On success returns sum of all matching entries for all proc handlers. > + * Oterwise, -ENODEV or -EINVAL is returned. s/Oterwise/Otherwise > + */ > int __init > acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries) > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries) > { > struct acpi_subtable_header *entry; > int count = 0; > unsigned long table_end; > + int i; > > - if (acpi_disabled) > + if (acpi_disabled) { > + proc[0].count = -ENODEV; > return -ENODEV; > - > - if (!id || !handler) > - return -EINVAL; > - > - if (!table_size) > + } Add a space please > + if (!table_size) { > + proc[0].count = -EINVAL; This is misleading. Why do we want to return error only in the first entry ? If I get the function logic right, if this function returns an error somehow the parsing failed, so the proc array content must be considered invalid. Ergo, just returning an error code should be enough and that's what you should check first in eg acpi_parse_madt_lapic_entries(). Comment is valid for the whole patch. > return -EINVAL; > + } > > if (!table_header) { > pr_warn("%4.4s not present\n", id); > + proc[0].count = -ENODEV; > return -ENODEV; > } > > @@ -247,20 +265,31 @@ acpi_parse_entries(char *id, unsigned long table_size, > > while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < > table_end) { > - if (entry->type == entry_id > - && (!max_entries || count < max_entries)) { > - if (handler(entry, table_end)) > + if (max_entries && count >= max_entries) > + continue; > + for (i = 0; i < proc_num; i++) { > + if (entry->type != proc[i].id) > + continue; > + if (proc[i].handler(entry, table_end)) { > + proc[i].count = -EINVAL; > return -EINVAL; > - > - count++; > + } > + proc[i].count++; > + break; > } > + if (i != proc_num) > + count++; > > /* > * If entry->length is 0, break from this loop to avoid > * infinite loop. > */ > if (entry->length == 0) { > - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); > + pr_err("[%4.4s:0x%02x ", id, proc[0].id); > + for (i = 1; i < proc_num; i++) > + pr_cont(" 0x%02x", proc[i].id); > + pr_cont("] Invalid zero length\n"); > + proc[0].count = -EINVAL; > return -EINVAL; > } > > @@ -269,18 +298,20 @@ acpi_parse_entries(char *id, unsigned long table_size, > } > > if (max_entries && count > max_entries) { > - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", > - id, entry_id, count - max_entries, count); > + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); > + for (i = 1; i < proc_num; i++) > + pr_cont(" 0x%02x", proc[i].id); > + pr_cont("] ignored %i entries of %i found\n", > + count-max_entries, count); > } > > return count; > } > > int __init > -acpi_table_parse_entries(char *id, > +acpi_table_parse_entries_array(char *id, > unsigned long table_size, > - int entry_id, > - acpi_tbl_entry_handler handler, > + struct acpi_subtable_proc *proc, int proc_num, > unsigned int max_entries) > { > struct acpi_table_header *table_header = NULL; > @@ -288,11 +319,10 @@ acpi_table_parse_entries(char *id, > int count; > u32 instance = 0; > > - if (acpi_disabled) > + if (acpi_disabled) { > + proc[0].count = -ENODEV; > return -ENODEV; > - > - if (!id || !handler) > - return -EINVAL; > + } > > if (!strncmp(id, ACPI_SIG_MADT, 4)) > instance = acpi_apic_instance; > @@ -300,23 +330,50 @@ acpi_table_parse_entries(char *id, > acpi_get_table_with_size(id, instance, &table_header, &tbl_size); > if (!table_header) { > pr_warn("%4.4s not present\n", id); > + proc[0].count = -ENODEV; > return -ENODEV; > } > > - count = acpi_parse_entries(id, table_size, handler, table_header, > - entry_id, max_entries); > + count = acpi_parse_entries(id, table_size, table_header, > + proc, proc_num, max_entries); > > early_acpi_os_unmap_memory((char *)table_header, tbl_size); > return count; > } > > int __init > +acpi_table_parse_entries(char *id, > + unsigned long table_size, > + int entry_id, > + acpi_tbl_entry_handler handler, > + unsigned int max_entries) > +{ > + struct acpi_subtable_proc proc[1]; > + > + if (!handler) > + return -EINVAL; > + > + memset(proc, 0, sizeof(proc)); > + proc[0].id = entry_id; > + proc[0].handler = handler; > + > + return acpi_table_parse_entries_array(id, table_size, proc, 1, > + max_entries); > +} > + > +int __init > acpi_table_parse_madt(enum acpi_madt_type id, > acpi_tbl_entry_handler handler, unsigned int max_entries) > { > - return acpi_table_parse_entries(ACPI_SIG_MADT, > - sizeof(struct acpi_table_madt), id, > - handler, max_entries); Why can't you leave this call as-is ? > + struct acpi_subtable_proc madt_proc; Is there a reason why you use a struct here and a size 1 array in acpi_table_parse_entries() ? Keep them consistent. Actually, I do not think you need to parse acpi_table_parse_madt() at all unless I am missing something. Lorenzo > + > + memset(&madt_proc, 0, sizeof(madt_proc)); > + madt_proc.id = id; > + madt_proc.handler = handler; > + > + return acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + &madt_proc, 1, max_entries); > } > > /** > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 4dd8826..d98b866 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) > { > void __iomem *cpu_base, *dist_base; > int count; > + struct acpi_subtable_proc gic_proc; > + > + memset(&gic_proc, 0, sizeof(gic_proc)); > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; > + gic_proc.handler = gic_acpi_parse_madt_cpu; > > /* Collect CPU base addresses */ > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_cpu, table, > - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); > + table, &gic_proc, 1, 0); > if (count <= 0) { > pr_err("No valid GICC entries exist\n"); > return -EINVAL; > @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) > * Find distributor base address. We expect one distributor entry since > * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. > */ > + memset(&gic_proc, 0, sizeof(gic_proc)); > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; > + gic_proc.handler = gic_acpi_parse_madt_distributor; > + > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_distributor, table, > - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); > + table, &gic_proc, 1, 0); > if (count <= 0) { > pr_err("No valid GICD entries exist\n"); > return -EINVAL; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index d2445fa..59b17e8 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) > (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) > > +struct acpi_subtable_proc { > + int id; > + acpi_tbl_entry_handler handler; > + int count; > +}; > + > char * __acpi_map_table (unsigned long phys_addr, unsigned long size); > void __acpi_unmap_table(char *map, unsigned long size); > int early_acpi_boot_init(void); > @@ -145,10 +151,13 @@ int acpi_numa_init (void); > > int acpi_table_init (void); > int acpi_table_parse(char *id, acpi_tbl_table_handler handler); > +int acpi_table_parse_entries_array(char *id, unsigned long table_size, > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int __init acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries); > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int __init acpi_table_parse_entries(char *id, unsigned long table_size, > int entry_id, > acpi_tbl_entry_handler handler, > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-08-27 9:37 ` Lorenzo Pieralisi @ 2015-09-08 11:07 ` Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-08 11:07 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi Hi Lorenzo, Ingo and Tomasz, I'm sending revisited set of patches with all your comments addressed (I hope), thus I'll skip replying to single each of them. Thanks in advance for comments. Cheers, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-08 11:07 ` Lukasz Anaczkowski @ 2015-09-08 11:07 ` Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 1/4] acpi: rename acpi_table_parse_entries Lukasz Anaczkowski 2015-09-08 16:27 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Marc Zyngier 0 siblings, 2 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-08 11:07 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski This series of patches attempts to fix how CPUs are enumerated by kernel when there's more than 255 of them on single processor. In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements specified in ACPI spec. Without this patches, kernel then would first enumerate BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence of that could be performance penalties due to wrong L2 cache sharing. More details in patch 4/4. Also, simpler approach has been considered, which did not required ACPI parsing interface changes, however it failed to meet requirements. More details can be found here: https://lkml.org/lkml/2015/9/7/285 I've compiled this code successfully for x86/arm64 and verified it on x86. I'd really appreciate if someone could give it a try on arm64. Although interface has changed, semantics stayed the same, thus runtime issues should not appear. Please verify, thanks! Lukasz Anaczkowski (4): acpi: rename acpi_table_parse_entries x86, arm64, acpi: Added acpi_subtable_proc acpi: multi proc support x86, acpi: Handle apic/x2apic entries in MADT in correct order arch/x86/kernel/acpi/boot.c | 39 +++++++++++++++++------- drivers/acpi/numa.c | 13 ++++++-- drivers/acpi/tables.c | 73 +++++++++++++++++++++++++++++++++++---------- drivers/irqchip/irq-gic.c | 15 +++++++--- include/linux/acpi.h | 13 ++++++-- 5 files changed, 119 insertions(+), 34 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/4] acpi: rename acpi_table_parse_entries 2015-09-08 11:07 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski @ 2015-09-08 11:07 ` Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 2/4] x86, arm64, acpi: Added acpi_subtable_proc Lukasz Anaczkowski 2015-09-08 16:27 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Marc Zyngier 1 sibling, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-08 11:07 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski ACPI subtable parsing needs to be extended to allow two or more handlers to be run in the same ACPI table walk. This is needed to fix CPU enumeration when APIC/X2APIC entries are interleaved. Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- drivers/acpi/tables.c | 13 ++++++++++++- include/linux/acpi.h | 4 ++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..aa8bcc6 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -277,7 +277,7 @@ acpi_parse_entries(char *id, unsigned long table_size, } int __init -acpi_table_parse_entries(char *id, +acpi_table_parse_entries_array(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, @@ -311,6 +311,17 @@ acpi_table_parse_entries(char *id, } int __init +acpi_table_parse_entries(char *id, + unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries) +{ + return acpi_table_parse_entries_array(id, table_size, entry_id, + handler, max_entries); +} + +int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..07fd1d1 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -153,6 +153,10 @@ int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, unsigned int max_entries); +int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries); int acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 2/4] x86, arm64, acpi: Added acpi_subtable_proc 2015-09-08 11:07 ` [PATCH 1/4] acpi: rename acpi_table_parse_entries Lukasz Anaczkowski @ 2015-09-08 11:07 ` Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 3/4] acpi: multi proc support Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-08 11:07 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski Single item array is only supported to verify that original logic is not changed. Update for many item array support is to be added in next patch. Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- arch/x86/kernel/acpi/boot.c | 36 ++++++++++++++++++++++++++++-------- drivers/acpi/numa.c | 13 ++++++++++--- drivers/acpi/tables.c | 40 +++++++++++++++++++++++++--------------- drivers/irqchip/irq-gic.c | 15 +++++++++++---- include/linux/acpi.h | 13 +++++++++---- 5 files changed, 83 insertions(+), 34 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..21cb7a0 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + struct acpi_subtable_proc madt_proc[1]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1005,19 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[0].handler = acpi_parse_x2apic; + x2count = acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + count = acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); @@ -1019,10 +1029,20 @@ static int __init acpi_parse_madt_lapic_entries(void) return count; } - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, - acpi_parse_x2apic_nmi, 0); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, - acpi_parse_lapic_nmi, 0); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; + madt_proc[0].handler = acpi_parse_x2apic_nmi; + count = acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), 0); + + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; + madt_proc[0].handler = acpi_parse_lapic_nmi; + x2count = acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), 0); + if (count < 0 || x2count < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); /* TBD: Cleanup to allow fallback to MPS */ diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c index acaa3b4..f3ccc68 100644 --- a/drivers/acpi/numa.c +++ b/drivers/acpi/numa.c @@ -314,9 +314,15 @@ static int __init acpi_table_parse_srat(enum acpi_srat_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_SRAT, - sizeof(struct acpi_table_srat), id, - handler, max_entries); + struct acpi_subtable_proc srat_proc; + + memset(&srat_proc, 0, sizeof(srat_proc)); + srat_proc.id = id; + srat_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + &srat_proc, 1, max_entries); } int __init acpi_numa_init(void) @@ -335,6 +341,7 @@ int __init acpi_numa_init(void) acpi_parse_x2apic_affinity, 0); acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, acpi_parse_processor_affinity, 0); + cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index aa8bcc6..33539ee 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -216,9 +216,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; int count = 0; @@ -227,7 +227,7 @@ acpi_parse_entries(char *id, unsigned long table_size, if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!table_size) @@ -247,12 +247,12 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id + if (entry->type == proc->id && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + if (!proc->handler || proc->handler(entry, table_end)) return -EINVAL; - count++; + proc->count++; } /* @@ -260,7 +260,7 @@ acpi_parse_entries(char *id, unsigned long table_size, * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); return -EINVAL; } @@ -268,9 +268,11 @@ acpi_parse_entries(char *id, unsigned long table_size, ((unsigned long)entry + entry->length); } + count = proc->count; + if (max_entries && count > max_entries) { pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + id, proc->id, count - max_entries, count); } return count; @@ -279,8 +281,7 @@ acpi_parse_entries(char *id, unsigned long table_size, int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -291,7 +292,7 @@ acpi_table_parse_entries_array(char *id, if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!strncmp(id, ACPI_SIG_MADT, 4)) @@ -303,8 +304,8 @@ acpi_table_parse_entries_array(char *id, return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; @@ -317,8 +318,17 @@ acpi_table_parse_entries(char *id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries_array(id, table_size, entry_id, - handler, max_entries); + struct acpi_subtable_proc proc[1]; + + if (!handler) + return -EINVAL; + + memset(proc, 0, sizeof(proc)); + proc[0].id = entry_id; + proc[0].handler = handler; + + return acpi_table_parse_entries_array(id, table_size, proc, 1, + max_entries); } int __init diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4dd8826..581336c 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) { void __iomem *cpu_base, *dist_base; int count; + struct acpi_subtable_proc gic_proc[1]; + + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; + gic_proc[0].handler = gic_acpi_parse_madt_cpu; /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_cpu, table, - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); + table, gic_proc, 1, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) * Find distributor base address. We expect one distributor entry since * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. */ + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; + gic_proc[0].handler = gic_acpi_parse_madt_distributor; + count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_distributor, table, - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); + table, gic_proc, 1, 0); if (count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 07fd1d1..52c8d20 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -146,16 +152,15 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries); + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, unsigned int max_entries); int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries); int acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 3/4] acpi: multi proc support 2015-09-08 11:07 ` [PATCH 2/4] x86, arm64, acpi: Added acpi_subtable_proc Lukasz Anaczkowski @ 2015-09-08 11:07 ` Lukasz Anaczkowski 2015-09-08 11:08 ` [PATCH 4/4] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-08 11:07 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski Now, it is possible to pass two or more handlers. Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- drivers/acpi/tables.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 33539ee..ececeac 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -214,6 +214,22 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } } +/** + * acpi_parse_entries - for each proc_num find a subtable with proc->id + * and run proc->handler on it. Assumption is that there's only + * single handler for particular entry id. + * + * @id: table id (for debugging purposes) + * @table_size: single entry size + * @table_header: where does the table start? + * @proc: array of acpi_subtable_proc struct containing entry id + * and associated handler with it + * @proc_num: how big proc is? + * @max_entries: how many entries can we process? + * + * On success returns sum of all matching entries for all proc handlers. + * Oterwise, -ENODEV or -EINVAL is returned. + */ int __init acpi_parse_entries(char *id, unsigned long table_size, struct acpi_table_header *table_header, @@ -247,13 +263,20 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == proc->id - && (!max_entries || count < max_entries)) { - if (!proc->handler || proc->handler(entry, table_end)) + if (max_entries && count >= max_entries) + break; + + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (!proc->handler || proc[i].handler(entry, table_end)) { return -EINVAL; proc->count++; + break; } + if (i != proc_num) + count++; /* * If entry->length is 0, break from this loop to avoid @@ -268,8 +291,6 @@ acpi_parse_entries(char *id, unsigned long table_size, ((unsigned long)entry + entry->length); } - count = proc->count; - if (max_entries && count > max_entries) { pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", id, proc->id, count - max_entries, count); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 4/4] x86, acpi: Handle apic/x2apic entries in MADT in correct order 2015-09-08 11:07 ` [PATCH 3/4] acpi: multi proc support Lukasz Anaczkowski @ 2015-09-08 11:08 ` Lukasz Anaczkowski 2015-09-08 15:22 ` Marc Zyngier 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-08 11:08 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski, Yinghai Lu ACPI specifies the following rules when listing APIC IDs: (1) Boot processor is listed first (2) For multi-threaded processors, BIOS should list the first logical processor of each of the individual multi-threaded processors in MADT before listing any of the second logical processors. (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF should be listed in X2APIC subtable Because of above, when there's more than 0xFF logical CPUs, BIOS interleaves APIC/X2APIC subtables. Assuming, there's 72 cores, 72 hyper-threads each, 288 CPUs total, listing is like this: APIC (0,4,8, .., 252) X2APIC (258,260,264, .. 284) APIC (1,5,9,...,253) X2APIC (259,261,265,...,285) APIC (2,6,10,...,254) X2APIC (260,262,266,..,286) APIC (3,7,11,...,251) X2APIC (255,261,262,266,..,287) Now, before this patch, due to how ACPI MADT subtables were parsed (BSP then X2APIC then APIC), kernel enumerated CPUs in reverted order (i.e. high APIC IDs were getting low logical IDs, and low APIC IDs were getting high logical IDs). This is wrong for the following reasons: () it's hard to predict how cores and threads are enumerated () when it's hard to predict, s/w threads cannot be properly affinitized causing significant performance impact due to e.g. inproper cache sharing () enumeration is inconsistent with how threads are enumerated on other Intel Xeon processors To fix this, each APIC/X2APIC entry from MADT table needs to be handled at the same time when processing them. Thus, adding acpi_subtable_proc structure which stores () ACPI table id () handler that processes table () counter how many items has been processed and passing it to acpi_table_parse_entries(). Also, order in which MADT APIC/X2APIC handlers are passed is reversed to achieve correct CPU enumeration. In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', in result less cores may be booted, since some of the CPUs the kernel will try to use will have APIC ID >= 0xFF. In such case, one should not pass 'nox2apic'. Disclimer: code parsing MADT APIC/X2APIC has not been touched since 2009, when X2APIC support was initially added. I do not know why MADT parsing code was added in the reversed order in the first place. I guess it didn't matter at that time since nobody cared about cores with APIC IDs >= 0xFF, right? This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563, thus putting Yinghai Lu as 'Signed-off-by', as well. Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> Signed-off-by: Yinghai Lu <yinghai@kernel.org> Acked-by: Thomas Gleixner <tglx@linutronix.de> (commit message) --- arch/x86/kernel/acpi/boot.c | 37 ++++++++++++++++++------------------- drivers/acpi/tables.c | 3 ++- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index 21cb7a0..8e014d4 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,7 +981,8 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; - struct acpi_subtable_proc madt_proc[1]; + int ret; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1008,16 +1009,19 @@ static int __init acpi_parse_madt_lapic_entries(void) memset(madt_proc, 0, sizeof(madt_proc)); madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC; madt_proc[0].handler = acpi_parse_x2apic; - x2count = acpi_table_parse_entries_array(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), - madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); - - memset(madt_proc, 0, sizeof(madt_proc)); - madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; - madt_proc[0].handler = acpi_parse_lapic; - count = acpi_table_parse_entries_array(ACPI_SIG_MADT, + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[1].handler = acpi_parse_lapic; + ret = acpi_table_parse_entries_array(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + if (ret < 0) { + printk(KERN_ERR PREFIX + "Error parsing LAPIC/X2APIC entries\n"); + return ret; + } + + x2count = madt_proc[0].count; + count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); @@ -1032,21 +1036,16 @@ static int __init acpi_parse_madt_lapic_entries(void) memset(madt_proc, 0, sizeof(madt_proc)); madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; madt_proc[0].handler = acpi_parse_x2apic_nmi; - count = acpi_table_parse_entries_array(ACPI_SIG_MADT, + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; + madt_proc[1].handler = acpi_parse_lapic_nmi; + ret = acpi_table_parse_entries_array(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), madt_proc, ARRAY_SIZE(madt_proc), 0); - memset(madt_proc, 0, sizeof(madt_proc)); - madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; - madt_proc[0].handler = acpi_parse_lapic_nmi; - x2count = acpi_table_parse_entries_array(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), - madt_proc, ARRAY_SIZE(madt_proc), 0); - - if (count < 0 || x2count < 0) { + if (ret < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); /* TBD: Cleanup to allow fallback to MPS */ - return count; + return ret; } return 0; } diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index ececeac..bc23220 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -239,6 +239,7 @@ acpi_parse_entries(char *id, unsigned long table_size, struct acpi_subtable_header *entry; int count = 0; unsigned long table_end; + int i; if (acpi_disabled) return -ENODEV; @@ -269,7 +270,7 @@ acpi_parse_entries(char *id, unsigned long table_size, for (i = 0; i < proc_num; i++) { if (entry->type != proc[i].id) continue; - if (!proc->handler || proc[i].handler(entry, table_end)) { + if (!proc->handler || proc[i].handler(entry, table_end)) return -EINVAL; proc->count++; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] x86, acpi: Handle apic/x2apic entries in MADT in correct order 2015-09-08 11:08 ` [PATCH 4/4] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski @ 2015-09-08 15:22 ` Marc Zyngier 0 siblings, 0 replies; 52+ messages in thread From: Marc Zyngier @ 2015-09-08 15:22 UTC (permalink / raw) To: Lukasz Anaczkowski, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On 08/09/15 12:08, Lukasz Anaczkowski wrote: > ACPI specifies the following rules when listing APIC IDs: > (1) Boot processor is listed first > (2) For multi-threaded processors, BIOS should list the first logical > processor of each of the individual multi-threaded processors in MADT > before listing any of the second logical processors. > (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF > should be listed in X2APIC subtable [snip] > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index ececeac..bc23220 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -239,6 +239,7 @@ acpi_parse_entries(char *id, unsigned long table_size, > struct acpi_subtable_header *entry; > int count = 0; > unsigned long table_end; > + int i; > > if (acpi_disabled) > return -ENODEV; > @@ -269,7 +270,7 @@ acpi_parse_entries(char *id, unsigned long table_size, > for (i = 0; i < proc_num; i++) { > if (entry->type != proc[i].id) > continue; > - if (!proc->handler || proc[i].handler(entry, table_end)) { > + if (!proc->handler || proc[i].handler(entry, table_end)) > return -EINVAL; This needs to be moved into the right patch, because this is otherwise not bisectable... M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-08 11:07 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 1/4] acpi: rename acpi_table_parse_entries Lukasz Anaczkowski @ 2015-09-08 16:27 ` Marc Zyngier 2015-09-08 22:45 ` Al Stone ` (2 more replies) 1 sibling, 3 replies; 52+ messages in thread From: Marc Zyngier @ 2015-09-08 16:27 UTC (permalink / raw) To: Lukasz Anaczkowski, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On 08/09/15 12:07, Lukasz Anaczkowski wrote: > This series of patches attempts to fix how CPUs are enumerated by kernel when > there's more than 255 of them on single processor. > In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements > specified in ACPI spec. Without this patches, kernel then would first enumerate > BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high > logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence > of that could be performance penalties due to wrong L2 cache sharing. > More details in patch 4/4. > > Also, simpler approach has been considered, which did not required ACPI parsing > interface changes, however it failed to meet requirements. More details can be > found here: https://lkml.org/lkml/2015/9/7/285 > > I've compiled this code successfully for x86/arm64 and verified it on x86. I'd > really appreciate if someone could give it a try on arm64. > Although interface has changed, semantics stayed the same, thus runtime issues > should not appear. Please verify, thanks! I really don't see why you cannot provide simple helpers that avoids the churn on code that doesn't require this new feature. It just makes the code hard(er) to read, and unnecessarily convoluted. ACPI doesn't need more of that, really. I've come up with this (on top of your series): diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c index f3ccc68..acaa3b4 100644 --- a/drivers/acpi/numa.c +++ b/drivers/acpi/numa.c @@ -314,15 +314,9 @@ static int __init acpi_table_parse_srat(enum acpi_srat_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - struct acpi_subtable_proc srat_proc; - - memset(&srat_proc, 0, sizeof(srat_proc)); - srat_proc.id = id; - srat_proc.handler = handler; - - return acpi_table_parse_entries_array(ACPI_SIG_SRAT, - sizeof(struct acpi_table_srat), - &srat_proc, 1, max_entries); + return acpi_table_parse_entries(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), id, + handler, max_entries); } int __init acpi_numa_init(void) @@ -341,7 +335,6 @@ int __init acpi_numa_init(void) acpi_parse_x2apic_affinity, 0); acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, acpi_parse_processor_affinity, 0); - cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index bc23220..758b334 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -215,7 +215,7 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } /** - * acpi_parse_entries - for each proc_num find a subtable with proc->id + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id * and run proc->handler on it. Assumption is that there's only * single handler for particular entry id. * @@ -230,8 +230,8 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) * On success returns sum of all matching entries for all proc handlers. * Oterwise, -ENODEV or -EINVAL is returned. */ -int __init -acpi_parse_entries(char *id, unsigned long table_size, +static int __init +acpi_parse_entries_array(char *id, unsigned long table_size, struct acpi_table_header *table_header, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) @@ -300,6 +300,20 @@ acpi_parse_entries(char *id, unsigned long table_size, return count; } +int __init acpi_parse_entries(char *id, unsigned long table_size, + acpi_tbl_entry_handler handler, + struct acpi_table_header *table_header, + int entry_id, unsigned int max_entries) +{ + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + }; + + return acpi_parse_entries_array(id, table_size, table_header, + &proc, 1, max_entries); +} + int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, @@ -326,8 +340,8 @@ acpi_table_parse_entries_array(char *id, return -ENODEV; } - count = acpi_parse_entries(id, table_size, table_header, - proc, proc_num, max_entries); + count = acpi_parse_entries_array(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; @@ -340,17 +354,13 @@ acpi_table_parse_entries(char *id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - struct acpi_subtable_proc proc[1]; - - if (!handler) - return -EINVAL; - - memset(proc, 0, sizeof(proc)); - proc[0].id = entry_id; - proc[0].handler = handler; + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + }; - return acpi_table_parse_entries_array(id, table_size, proc, 1, - max_entries); + return acpi_table_parse_entries_array(id, table_size, &proc, 1, + max_entries); } int __init diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 581336c..4dd8826 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1091,16 +1091,12 @@ gic_v2_acpi_init(struct acpi_table_header *table) { void __iomem *cpu_base, *dist_base; int count; - struct acpi_subtable_proc gic_proc[1]; - - memset(gic_proc, 0, sizeof(gic_proc)); - gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; - gic_proc[0].handler = gic_acpi_parse_madt_cpu; /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - table, gic_proc, 1, 0); + gic_acpi_parse_madt_cpu, table, + ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; @@ -1110,13 +1106,10 @@ gic_v2_acpi_init(struct acpi_table_header *table) * Find distributor base address. We expect one distributor entry since * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. */ - memset(gic_proc, 0, sizeof(gic_proc)); - gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; - gic_proc[0].handler = gic_acpi_parse_madt_distributor; - count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - table, gic_proc, 1, 0); + gic_acpi_parse_madt_distributor, table, + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); if (count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 52c8d20..0f6381d 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -152,9 +152,9 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); int __init acpi_parse_entries(char *id, unsigned long table_size, - struct acpi_table_header *table_header, - struct acpi_subtable_proc *proc, int proc_num, - unsigned int max_entries); + acpi_tbl_entry_handler handler, + struct acpi_table_header *table_header, + int entry_id, unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, In my view, this makes the change a lot more palatable, and it can fit in exactly two patches: 1) add the acpi_subtable_proc stuff with the compatibility helpers 2) change arch/x86/kernel/acpi/boot.c to do whatever you want it to do It will be a lot easier to review and to verify. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-08 16:27 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Marc Zyngier @ 2015-09-08 22:45 ` Al Stone 2015-09-09 7:01 ` Anaczkowski, Lukasz 2015-09-09 9:30 ` [PATCH 0/2] " Lukasz Anaczkowski 2 siblings, 0 replies; 52+ messages in thread From: Al Stone @ 2015-09-08 22:45 UTC (permalink / raw) To: Marc Zyngier, Lukasz Anaczkowski, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On 09/08/2015 10:27 AM, Marc Zyngier wrote: > On 08/09/15 12:07, Lukasz Anaczkowski wrote: >> This series of patches attempts to fix how CPUs are enumerated by kernel when >> there's more than 255 of them on single processor. >> In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements >> specified in ACPI spec. Without this patches, kernel then would first enumerate >> BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high >> logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence >> of that could be performance penalties due to wrong L2 cache sharing. >> More details in patch 4/4. >> >> Also, simpler approach has been considered, which did not required ACPI parsing >> interface changes, however it failed to meet requirements. More details can be >> found here: https://lkml.org/lkml/2015/9/7/285 >> >> I've compiled this code successfully for x86/arm64 and verified it on x86. I'd >> really appreciate if someone could give it a try on arm64. >> Although interface has changed, semantics stayed the same, thus runtime issues >> should not appear. Please verify, thanks! > > I really don't see why you cannot provide simple helpers that avoids > the churn on code that doesn't require this new feature. It just makes > the code hard(er) to read, and unnecessarily convoluted. ACPI doesn't > need more of that, really. > > I've come up with this (on top of your series): > > diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c > index f3ccc68..acaa3b4 100644 > --- a/drivers/acpi/numa.c > +++ b/drivers/acpi/numa.c > @@ -314,15 +314,9 @@ static int __init > acpi_table_parse_srat(enum acpi_srat_type id, > acpi_tbl_entry_handler handler, unsigned int max_entries) > { > - struct acpi_subtable_proc srat_proc; > - > - memset(&srat_proc, 0, sizeof(srat_proc)); > - srat_proc.id = id; > - srat_proc.handler = handler; > - > - return acpi_table_parse_entries_array(ACPI_SIG_SRAT, > - sizeof(struct acpi_table_srat), > - &srat_proc, 1, max_entries); > + return acpi_table_parse_entries(ACPI_SIG_SRAT, > + sizeof(struct acpi_table_srat), id, > + handler, max_entries); > } > > int __init acpi_numa_init(void) > @@ -341,7 +335,6 @@ int __init acpi_numa_init(void) > acpi_parse_x2apic_affinity, 0); > acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, > acpi_parse_processor_affinity, 0); > - > cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, > acpi_parse_memory_affinity, > NR_NODE_MEMBLKS); > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index bc23220..758b334 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -215,7 +215,7 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > } > > /** > - * acpi_parse_entries - for each proc_num find a subtable with proc->id > + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id > * and run proc->handler on it. Assumption is that there's only > * single handler for particular entry id. > * > @@ -230,8 +230,8 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > * On success returns sum of all matching entries for all proc handlers. > * Oterwise, -ENODEV or -EINVAL is returned. > */ > -int __init > -acpi_parse_entries(char *id, unsigned long table_size, > +static int __init > +acpi_parse_entries_array(char *id, unsigned long table_size, > struct acpi_table_header *table_header, > struct acpi_subtable_proc *proc, int proc_num, > unsigned int max_entries) > @@ -300,6 +300,20 @@ acpi_parse_entries(char *id, unsigned long table_size, > return count; > } > > +int __init acpi_parse_entries(char *id, unsigned long table_size, > + acpi_tbl_entry_handler handler, > + struct acpi_table_header *table_header, > + int entry_id, unsigned int max_entries) > +{ > + struct acpi_subtable_proc proc = { > + .id = entry_id, > + .handler = handler, > + }; > + > + return acpi_parse_entries_array(id, table_size, table_header, > + &proc, 1, max_entries); > +} > + > int __init > acpi_table_parse_entries_array(char *id, > unsigned long table_size, > @@ -326,8 +340,8 @@ acpi_table_parse_entries_array(char *id, > return -ENODEV; > } > > - count = acpi_parse_entries(id, table_size, table_header, > - proc, proc_num, max_entries); > + count = acpi_parse_entries_array(id, table_size, table_header, > + proc, proc_num, max_entries); > > early_acpi_os_unmap_memory((char *)table_header, tbl_size); > return count; > @@ -340,17 +354,13 @@ acpi_table_parse_entries(char *id, > acpi_tbl_entry_handler handler, > unsigned int max_entries) > { > - struct acpi_subtable_proc proc[1]; > - > - if (!handler) > - return -EINVAL; > - > - memset(proc, 0, sizeof(proc)); > - proc[0].id = entry_id; > - proc[0].handler = handler; > + struct acpi_subtable_proc proc = { > + .id = entry_id, > + .handler = handler, > + }; > > - return acpi_table_parse_entries_array(id, table_size, proc, 1, > - max_entries); > + return acpi_table_parse_entries_array(id, table_size, &proc, 1, > + max_entries); > } > > int __init > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 581336c..4dd8826 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -1091,16 +1091,12 @@ gic_v2_acpi_init(struct acpi_table_header *table) > { > void __iomem *cpu_base, *dist_base; > int count; > - struct acpi_subtable_proc gic_proc[1]; > - > - memset(gic_proc, 0, sizeof(gic_proc)); > - gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; > - gic_proc[0].handler = gic_acpi_parse_madt_cpu; > > /* Collect CPU base addresses */ > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - table, gic_proc, 1, 0); > + gic_acpi_parse_madt_cpu, table, > + ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); > if (count <= 0) { > pr_err("No valid GICC entries exist\n"); > return -EINVAL; > @@ -1110,13 +1106,10 @@ gic_v2_acpi_init(struct acpi_table_header *table) > * Find distributor base address. We expect one distributor entry since > * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. > */ > - memset(gic_proc, 0, sizeof(gic_proc)); > - gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; > - gic_proc[0].handler = gic_acpi_parse_madt_distributor; > - > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - table, gic_proc, 1, 0); > + gic_acpi_parse_madt_distributor, table, > + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); > if (count <= 0) { > pr_err("No valid GICD entries exist\n"); > return -EINVAL; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 52c8d20..0f6381d 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -152,9 +152,9 @@ int acpi_numa_init (void); > int acpi_table_init (void); > int acpi_table_parse(char *id, acpi_tbl_table_handler handler); > int __init acpi_parse_entries(char *id, unsigned long table_size, > - struct acpi_table_header *table_header, > - struct acpi_subtable_proc *proc, int proc_num, > - unsigned int max_entries); > + acpi_tbl_entry_handler handler, > + struct acpi_table_header *table_header, > + int entry_id, unsigned int max_entries); > int __init acpi_table_parse_entries(char *id, unsigned long table_size, > int entry_id, > acpi_tbl_entry_handler handler, > > In my view, this makes the change a lot more palatable, and it > can fit in exactly two patches: > > 1) add the acpi_subtable_proc stuff with the compatibility helpers > 2) change arch/x86/kernel/acpi/boot.c to do whatever you want it > to do > > It will be a lot easier to review and to verify. > > Thanks, > > M. > I have to agree with Marc here. His changes on top of the prior patches make this much cleaner. The MADT parsing is messy enough as it is without adding changes needed by only the APIC/X2APIC subtables into the interface for all MADT parsing. I'm also still not convinced we have to have the ability to call multiple callbacks for each entry as we cycle through the MADT entries. I've looked through the previous comments on these patches and I'm still not getting it; maybe I'm just being dense (it wouldn't be the first time). Logically, I see no difference between making two passes through the MADT subtables, once for each subtable type and noting the info I need, versus making two callbacks for each subtable, and noting the info I need between callbacks. If nothing else, my personal opinion is that the former would be easier to maintain over time since it would be specific to the users of APIC/X2APIC and not affect other users parsing MADT subtables. Obviously, though, I'm not the final arbiter of all things ACPI... this is just my $0.02 worth... -- ciao, al ----------------------------------- Al Stone Software Engineer Red Hat, Inc. ahs3@redhat.com ----------------------------------- ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-08 16:27 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Marc Zyngier 2015-09-08 22:45 ` Al Stone @ 2015-09-09 7:01 ` Anaczkowski, Lukasz 2015-09-09 9:30 ` [PATCH 0/2] " Lukasz Anaczkowski 2 siblings, 0 replies; 52+ messages in thread From: Anaczkowski, Lukasz @ 2015-09-09 7:01 UTC (permalink / raw) To: Marc Zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi From: Marc Zyngier [mailto:marc.zyngier@arm.com] Sent: Tuesday, September 8, 2015 6:28 PM > In my view, this makes the change a lot more palatable, and it can fit in exactly two patches: > > 1) add the acpi_subtable_proc stuff with the compatibility helpers > 2) change arch/x86/kernel/acpi/boot.c to do whatever you want it > to do > It will be a lot easier to review and to verify. Thanks, Marc. Sounds you're right, I'll try to follow KISS rule better next time. Will send updated patches. Cheers, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-08 16:27 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Marc Zyngier 2015-09-08 22:45 ` Al Stone 2015-09-09 7:01 ` Anaczkowski, Lukasz @ 2015-09-09 9:30 ` Lukasz Anaczkowski 2015-09-09 9:30 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski 2 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-09 9:30 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski This series of patches attempts to fix how CPUs are enumerated by kernel when there's more than 255 of them on single processor. In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements specified in ACPI spec. Without this patches, kernel then would first enumerate BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence of that could be performance penalties due to wrong L2 cache sharing. More details in patch 2/2. Also, simpler approach has been considered, which did not required ACPI parsing interface changes, however it failed to meet requirements. More details can be found here: https://lkml.org/lkml/2015/9/7/285 Lukasz Anaczkowski (2): acpi: Added acpi_subtable_proc to ACPI table parsers x86, acpi: Handle apic/x2apic entries in MADT in correct order arch/x86/kernel/acpi/boot.c | 22 +++++++++-- drivers/acpi/tables.c | 89 ++++++++++++++++++++++++++++++++++++--------- include/linux/acpi.h | 13 +++++++ 3 files changed, 103 insertions(+), 21 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers 2015-09-09 9:30 ` [PATCH 0/2] " Lukasz Anaczkowski @ 2015-09-09 9:30 ` Lukasz Anaczkowski 2015-09-09 9:30 ` [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski 2015-09-09 10:47 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Marc Zyngier 0 siblings, 2 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-09 9:30 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski ACPI subtable parsing needs to be extended to allow two or more handlers to be run in the same ACPI table walk, thus adding acpi_subtable_proc structure which stores () ACPI table id () handler that processes table () counter how many items has been processed and passing it to acpi_parse_entries_array() and acpi_table_parse_entries_array(). This is needed to fix CPU enumeration when APIC/X2APIC entries are interleaved. Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- drivers/acpi/tables.c | 89 +++++++++++++++++++++++++++++++++++++++++---------- include/linux/acpi.h | 13 ++++++++ 2 files changed, 85 insertions(+), 17 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..13e5089 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -214,20 +214,37 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } } +/** + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id + * and run proc->handler on it. Assumption is that there's only + * single handler for particular entry id. + * + * @id: table id (for debugging purposes) + * @table_size: single entry size + * @table_header: where does the table start? + * @proc: array of acpi_subtable_proc struct containing entry id + * and associated handler with it + * @proc_num: how big proc is? + * @max_entries: how many entries can we process? + * + * On success returns sum of all matching entries for all proc handlers. + * Oterwise, -ENODEV or -EINVAL is returned. + */ int __init -acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, +acpi_parse_entries_array(char *id, unsigned long table_size, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; - int count = 0; unsigned long table_end; + int count = 0; + int i; if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!table_size) @@ -247,20 +264,27 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + if (max_entries && count >= max_entries) + break; + + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (!proc->handler || proc[i].handler(entry, table_end)) return -EINVAL; - count++; + proc->count++; + break; } + if (i != proc_num) + count++; /* * If entry->length is 0, break from this loop to avoid * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); return -EINVAL; } @@ -270,17 +294,31 @@ acpi_parse_entries(char *id, unsigned long table_size, if (max_entries && count > max_entries) { pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + id, proc->id, count - max_entries, count); } return count; } +int __init acpi_parse_entries(char *id, unsigned long table_size, + acpi_tbl_entry_handler handler, + struct acpi_table_header *table_header, + int entry_id, unsigned int max_entries) +{ + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + .count = 0, + }; + + return acpi_parse_entries_array(id, table_size, table_header, + &proc, 1, max_entries); +} + int __init -acpi_table_parse_entries(char *id, +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -291,7 +329,7 @@ acpi_table_parse_entries(char *id, if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!strncmp(id, ACPI_SIG_MADT, 4)) @@ -303,14 +341,31 @@ acpi_table_parse_entries(char *id, return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries_array(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; } int __init +acpi_table_parse_entries(char *id, + unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries) +{ + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + .count = 0, + }; + + return acpi_table_parse_entries_array(id, table_size, &proc, 1, + max_entries); +} + +int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..7a25ef9 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -149,10 +155,17 @@ int __init acpi_parse_entries(char *id, unsigned long table_size, acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, int entry_id, unsigned int max_entries); +int __init acpi_parse_entries_array(char *id, unsigned long table_size, + struct acpi_table_header *table_header, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, unsigned int max_entries); +int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order 2015-09-09 9:30 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski @ 2015-09-09 9:30 ` Lukasz Anaczkowski 2015-09-09 13:56 ` Lorenzo Pieralisi 2015-09-09 10:47 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Marc Zyngier 1 sibling, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-09 9:30 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski ACPI specifies the following rules when listing APIC IDs: (1) Boot processor is listed first (2) For multi-threaded processors, BIOS should list the first logical processor of each of the individual multi-threaded processors in MADT before listing any of the second logical processors. (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF should be listed in X2APIC subtable Because of above, when there's more than 0xFF logical CPUs, BIOS interleaves APIC/X2APIC subtables. Assuming, there's 72 cores, 72 hyper-threads each, 288 CPUs total, listing is like this: APIC (0,4,8, .., 252) X2APIC (258,260,264, .. 284) APIC (1,5,9,...,253) X2APIC (259,261,265,...,285) APIC (2,6,10,...,254) X2APIC (260,262,266,..,286) APIC (3,7,11,...,251) X2APIC (255,261,262,266,..,287) Now, before this patch, due to how ACPI MADT subtables were parsed (BSP then X2APIC then APIC), kernel enumerated CPUs in reverted order (i.e. high APIC IDs were getting low logical IDs, and low APIC IDs were getting high logical IDs). This is wrong for the following reasons: () it's hard to predict how cores and threads are enumerated () when it's hard to predict, s/w threads cannot be properly affinitized causing significant performance impact due to e.g. inproper cache sharing () enumeration is inconsistent with how threads are enumerated on other Intel Xeon processors So, order in which MADT APIC/X2APIC handlers are passed is reverse and both handlers are passed to be called during same MADT table to walk to achieve correct CPU enumeration. In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', in result less cores may be booted, since some of the CPUs the kernel will try to use will have APIC ID >= 0xFF. In such case, one should not pass 'nox2apic'. Disclimer: code parsing MADT APIC/X2APIC has not been touched since 2009, when X2APIC support was initially added. I do not know why MADT parsing code was added in the reversed order in the first place. I guess it didn't matter at that time since nobody cared about cores with APIC IDs >= 0xFF, right? This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563 Here's the explanation why parsing interface needs to be changed and why simpler approach will not work https://lkml.org/lkml/2015/9/7/285 Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> Acked-by: Thomas Gleixner <tglx@linutronix.de> (commit message) --- arch/x86/kernel/acpi/boot.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..116e911 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,8 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + int ret; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1006,22 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + ret = acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + if (ret < 0) { + printk(KERN_ERR PREFIX + "Error parsing LAPIC/X2APIC entries\n"); + return ret; + } + + x2count = madt_proc[0].count; + count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order 2015-09-09 9:30 ` [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski @ 2015-09-09 13:56 ` Lorenzo Pieralisi 2015-09-09 14:27 ` Anaczkowski, Lukasz 0 siblings, 1 reply; 52+ messages in thread From: Lorenzo Pieralisi @ 2015-09-09 13:56 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: Marc Zyngier, tomasz.nowicki, tglx, mingo, hpa, x86, jason, rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On Wed, Sep 09, 2015 at 10:30:18AM +0100, Lukasz Anaczkowski wrote: > ACPI specifies the following rules when listing APIC IDs: > (1) Boot processor is listed first > (2) For multi-threaded processors, BIOS should list the first logical > processor of each of the individual multi-threaded processors in MADT > before listing any of the second logical processors. > (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF > should be listed in X2APIC subtable > > Because of above, when there's more than 0xFF logical CPUs, BIOS > interleaves APIC/X2APIC subtables. > > Assuming, there's 72 cores, 72 hyper-threads each, 288 CPUs total, > listing is like this: > > APIC (0,4,8, .., 252) > X2APIC (258,260,264, .. 284) > APIC (1,5,9,...,253) > X2APIC (259,261,265,...,285) > APIC (2,6,10,...,254) > X2APIC (260,262,266,..,286) > APIC (3,7,11,...,251) > X2APIC (255,261,262,266,..,287) > > Now, before this patch, due to how ACPI MADT subtables were parsed (BSP > then X2APIC then APIC), kernel enumerated CPUs in reverted order (i.e. > high APIC IDs were getting low logical IDs, and low APIC IDs were > getting high logical IDs). > This is wrong for the following reasons: > () it's hard to predict how cores and threads are enumerated So ? Why would the logical cpus order matters at all ? I guessed there are probeable properties that allows the kernel to build the affinity (ie topology list, shared caches, smt siblings, etc). Please explain, since I am confused, to me all you need is a list of existing physical ids, in whatever order they come, that's at least what we need on ARM. > () when it's hard to predict, s/w threads cannot be properly affinitized > causing significant performance impact due to e.g. inproper cache > sharing See above. Why cpus logical ordering would matter here ? > () enumeration is inconsistent with how threads are enumerated on > other Intel Xeon processors And why does that matter ? Is it because userspace is making assumptions on the logical cpu enumeration scheme ? I am just asking, I would like to understand. > So, order in which MADT APIC/X2APIC handlers are passed is > reverse and both handlers are passed to be called during same MADT > table to walk to achieve correct CPU enumeration. Define "correct" please, you define the logical ordering you want to achieve, you do not define why that's more "correct" than the current implementation. Thank you, Lorenzo > In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', > in result less cores may be booted, since some of the CPUs the kernel > will try to use will have APIC ID >= 0xFF. In such case, one > should not pass 'nox2apic'. > > Disclimer: code parsing MADT APIC/X2APIC has not been touched since 2009, > when X2APIC support was initially added. I do not know why MADT parsing > code was added in the reversed order in the first place. > I guess it didn't matter at that time since nobody cared about cores > with APIC IDs >= 0xFF, right? > > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563 > > Here's the explanation why parsing interface needs to be changed > and why simpler approach will not work https://lkml.org/lkml/2015/9/7/285 > > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> > Acked-by: Thomas Gleixner <tglx@linutronix.de> (commit message) > --- > arch/x86/kernel/acpi/boot.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index e49ee24..116e911 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -981,6 +981,8 @@ static int __init acpi_parse_madt_lapic_entries(void) > { > int count; > int x2count = 0; > + int ret; > + struct acpi_subtable_proc madt_proc[2]; > > if (!cpu_has_apic) > return -ENODEV; > @@ -1004,10 +1006,22 @@ static int __init acpi_parse_madt_lapic_entries(void) > acpi_parse_sapic, MAX_LOCAL_APIC); > > if (!count) { > - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, > - acpi_parse_x2apic, MAX_LOCAL_APIC); > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, > - acpi_parse_lapic, MAX_LOCAL_APIC); > + memset(madt_proc, 0, sizeof(madt_proc)); > + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; > + madt_proc[0].handler = acpi_parse_lapic; > + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; > + madt_proc[1].handler = acpi_parse_x2apic; > + ret = acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); > + if (ret < 0) { > + printk(KERN_ERR PREFIX > + "Error parsing LAPIC/X2APIC entries\n"); > + return ret; > + } > + > + x2count = madt_proc[0].count; > + count = madt_proc[1].count; > } > if (!count && !x2count) { > printk(KERN_ERR PREFIX "No LAPIC entries present\n"); > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order 2015-09-09 13:56 ` Lorenzo Pieralisi @ 2015-09-09 14:27 ` Anaczkowski, Lukasz 2015-09-09 15:43 ` Lorenzo Pieralisi 0 siblings, 1 reply; 52+ messages in thread From: Anaczkowski, Lukasz @ 2015-09-09 14:27 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marc Zyngier, tomasz.nowicki, tglx, mingo, hpa, x86, jason, rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Craft, Clayton A -----Original Message----- From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] Sent: Wednesday, September 9, 2015 3:56 PM > On Wed, Sep 09, 2015 at 10:30:18AM +0100, Lukasz Anaczkowski wrote: > > () it's hard to predict how cores and threads are enumerated > So ? Why would the logical cpus order matters at all ? I guessed > there are probeable properties that allows the kernel to build > the affinity (ie topology list, shared caches, smt siblings, etc). > Please explain, since I am confused, to me all you need is a list > of existing physical ids, in whatever order they come, that's at least > what we need on ARM. Hi Lorenzo, Sure, let me try to explain this better. Proper (i.e. predictable way of CPU enumeration) matters for HPC software, (this is where I come from) as there are workloads that have some assumptions on CPU enumeration in order to keep cache hit ratio as high as possible. E.g. in KNL cores share L2 caches, and if during enumeration logical cores do not reflect physical cores, S/W can start affinitize threads to the same physical cores causing great performance impact exactly due to L2 cache misses. (e.g. s/w assumes that HT CPUs are separated by core count). Now, those changes would not be required if someone who have written APIC spec had reserved more than just 1 byte for CPU id :) Unfortunately, it's the case for x86 APIC ID and once it turns out there's a need to enumerate more than that, they added X2APIC spec which has 4 bytes for ID. Even that would be also fine if there were just physical cores, but with HT, ACPI clearly says, that first must be listed physical cores and only after that HT CPUs (and that's why APIC/X2APIC subtables are interleaved). When GIC spec was added, someone was smart enough to put 4 bytes from the begging, so you don't need to care about it on ARM :) > > () enumeration is inconsistent with how threads are enumerated on > > other Intel Xeon processors > And why does that matter ? Is it because userspace is making assumptions > on the logical cpu enumeration scheme ? I am just asking, I would > like to understand. Yes, HPC software makes some assumptions about CPU enumeration (as mentioned above) and having inconsistent enumeration between different x86 CPUs (Xeon vs Xeon Phi) make such s/w basically not portable. > > So, order in which MADT APIC/X2APIC handlers are passed is > > reverse and both handlers are passed to be called during same MADT > > table to walk to achieve correct CPU enumeration. > Define "correct" please, you define the logical ordering you > want to achieve, you do not define why that's more "correct" > than the current implementation. Ok, probably 'correct' word is not the best here :) Does 'compatible' sound better? Thanks, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order 2015-09-09 14:27 ` Anaczkowski, Lukasz @ 2015-09-09 15:43 ` Lorenzo Pieralisi 0 siblings, 0 replies; 52+ messages in thread From: Lorenzo Pieralisi @ 2015-09-09 15:43 UTC (permalink / raw) To: Anaczkowski, Lukasz Cc: Marc Zyngier, tomasz.nowicki, tglx, mingo, hpa, x86, jason, rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Craft, Clayton A On Wed, Sep 09, 2015 at 03:27:47PM +0100, Anaczkowski, Lukasz wrote: > -----Original Message----- > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] > Sent: Wednesday, September 9, 2015 3:56 PM > > > On Wed, Sep 09, 2015 at 10:30:18AM +0100, Lukasz Anaczkowski wrote: > > > () it's hard to predict how cores and threads are enumerated > > > So ? Why would the logical cpus order matters at all ? I guessed > > there are probeable properties that allows the kernel to build > > the affinity (ie topology list, shared caches, smt siblings, etc). > > Please explain, since I am confused, to me all you need is a list > > of existing physical ids, in whatever order they come, that's at least > > what we need on ARM. > > Hi Lorenzo, > > Sure, let me try to explain this better. > > Proper (i.e. predictable way of CPU enumeration) matters for HPC software, > (this is where I come from) as there are workloads that have some assumptions > on CPU enumeration in order to keep cache hit ratio as high as possible. > E.g. in KNL cores share L2 caches, and if during enumeration logical cores do not > reflect physical cores, S/W can start affinitize threads to the same physical cores > causing great performance impact exactly due to L2 cache misses. > (e.g. s/w assumes that HT CPUs are separated by core count). > > Now, those changes would not be required if someone who have written > APIC spec had reserved more than just 1 byte for CPU id :) > Unfortunately, it's the case for x86 APIC ID and once it turns out there's a need > to enumerate more than that, they added X2APIC spec which has 4 bytes for ID. > Even that would be also fine if there were just physical cores, but with HT, ACPI > clearly says, that first must be listed physical cores and only after that HT CPUs > (and that's why APIC/X2APIC subtables are interleaved). > > When GIC spec was added, someone was smart enough to put 4 bytes from > the begging, so you don't need to care about it on ARM :) > > > > () enumeration is inconsistent with how threads are enumerated on > > > other Intel Xeon processors > > > And why does that matter ? Is it because userspace is making assumptions > > on the logical cpu enumeration scheme ? I am just asking, I would > > like to understand. > > Yes, HPC software makes some assumptions about CPU enumeration (as mentioned > above) and having inconsistent enumeration between different x86 CPUs (Xeon vs Xeon Phi) > make such s/w basically not portable. Eh, what about "other s/w" (since MADT APIC/X2APIC parsing is unchanged since 2009 as you mentioned) that relies on the way current enumeration is implemented ? I will leave that to you. /me going back to commenting the code :) > > > So, order in which MADT APIC/X2APIC handlers are passed is > > > reverse and both handlers are passed to be called during same MADT > > > table to walk to achieve correct CPU enumeration. > > > Define "correct" please, you define the logical ordering you > > want to achieve, you do not define why that's more "correct" > > than the current implementation. > > Ok, probably 'correct' word is not the best here :) > Does 'compatible' sound better? No, see above :) Thanks, Lorenzo ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers 2015-09-09 9:30 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski 2015-09-09 9:30 ` [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski @ 2015-09-09 10:47 ` Marc Zyngier 2015-09-09 13:47 ` Lukasz Anaczkowski 1 sibling, 1 reply; 52+ messages in thread From: Marc Zyngier @ 2015-09-09 10:47 UTC (permalink / raw) To: Lukasz Anaczkowski, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi Hi Lucasz, Nit: Please add a version number to the patches you send - this is at least the 4th revision of this series, and it is harder to keep track of what I'm reviewing. git send-email --subject-prefix "PATCH v4" is your friend. On 09/09/15 10:30, Lukasz Anaczkowski wrote: > ACPI subtable parsing needs to be extended to allow two or more > handlers to be run in the same ACPI table walk, thus adding > acpi_subtable_proc structure which stores > () ACPI table id > () handler that processes table > () counter how many items has been processed > and passing it to acpi_parse_entries_array() and > acpi_table_parse_entries_array(). > > This is needed to fix CPU enumeration when APIC/X2APIC entries > are interleaved. > > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> > --- > drivers/acpi/tables.c | 89 +++++++++++++++++++++++++++++++++++++++++---------- > include/linux/acpi.h | 13 ++++++++ > 2 files changed, 85 insertions(+), 17 deletions(-) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 2e19189..13e5089 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -214,20 +214,37 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > } > } > > +/** > + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id > + * and run proc->handler on it. Assumption is that there's only > + * single handler for particular entry id. > + * > + * @id: table id (for debugging purposes) > + * @table_size: single entry size > + * @table_header: where does the table start? > + * @proc: array of acpi_subtable_proc struct containing entry id > + * and associated handler with it > + * @proc_num: how big proc is? > + * @max_entries: how many entries can we process? > + * > + * On success returns sum of all matching entries for all proc handlers. > + * Oterwise, -ENODEV or -EINVAL is returned. s/Oterwise/Otherwise/ > + */ > int __init > -acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > +acpi_parse_entries_array(char *id, unsigned long table_size, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries) > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries) It seems that there is no user of this function outside of this file, so it can be made static. > { > struct acpi_subtable_header *entry; > - int count = 0; > unsigned long table_end; > + int count = 0; > + int i; > > if (acpi_disabled) > return -ENODEV; > > - if (!id || !handler) > + if (!id) > return -EINVAL; > > if (!table_size) > @@ -247,20 +264,27 @@ acpi_parse_entries(char *id, unsigned long table_size, > > while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < > table_end) { > - if (entry->type == entry_id > - && (!max_entries || count < max_entries)) { > - if (handler(entry, table_end)) > + if (max_entries && count >= max_entries) > + break; > + > + for (i = 0; i < proc_num; i++) { > + if (entry->type != proc[i].id) > + continue; > + if (!proc->handler || proc[i].handler(entry, table_end)) > return -EINVAL; > > - count++; > + proc->count++; > + break; > } > + if (i != proc_num) > + count++; > > /* > * If entry->length is 0, break from this loop to avoid > * infinite loop. > */ > if (entry->length == 0) { > - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); > + pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); > return -EINVAL; > } > > @@ -270,17 +294,31 @@ acpi_parse_entries(char *id, unsigned long table_size, > > if (max_entries && count > max_entries) { > pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", > - id, entry_id, count - max_entries, count); > + id, proc->id, count - max_entries, count); > } > > return count; > } > > +int __init acpi_parse_entries(char *id, unsigned long table_size, > + acpi_tbl_entry_handler handler, > + struct acpi_table_header *table_header, > + int entry_id, unsigned int max_entries) > +{ > + struct acpi_subtable_proc proc = { > + .id = entry_id, > + .handler = handler, > + .count = 0, count is implicitly initialized to zero when you have a partial initializer. > + }; > + > + return acpi_parse_entries_array(id, table_size, table_header, > + &proc, 1, max_entries); > +} > + > int __init > -acpi_table_parse_entries(char *id, > +acpi_table_parse_entries_array(char *id, > unsigned long table_size, > - int entry_id, > - acpi_tbl_entry_handler handler, > + struct acpi_subtable_proc *proc, int proc_num, > unsigned int max_entries) > { > struct acpi_table_header *table_header = NULL; > @@ -291,7 +329,7 @@ acpi_table_parse_entries(char *id, > if (acpi_disabled) > return -ENODEV; > > - if (!id || !handler) > + if (!id) > return -EINVAL; > > if (!strncmp(id, ACPI_SIG_MADT, 4)) > @@ -303,14 +341,31 @@ acpi_table_parse_entries(char *id, > return -ENODEV; > } > > - count = acpi_parse_entries(id, table_size, handler, table_header, > - entry_id, max_entries); > + count = acpi_parse_entries_array(id, table_size, table_header, > + proc, proc_num, max_entries); > > early_acpi_os_unmap_memory((char *)table_header, tbl_size); > return count; > } > > int __init > +acpi_table_parse_entries(char *id, > + unsigned long table_size, > + int entry_id, > + acpi_tbl_entry_handler handler, > + unsigned int max_entries) > +{ > + struct acpi_subtable_proc proc = { > + .id = entry_id, > + .handler = handler, > + .count = 0, Same here. > + }; > + > + return acpi_table_parse_entries_array(id, table_size, &proc, 1, > + max_entries); > +} > + > +int __init > acpi_table_parse_madt(enum acpi_madt_type id, > acpi_tbl_entry_handler handler, unsigned int max_entries) > { > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index d2445fa..7a25ef9 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) > (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) > > +struct acpi_subtable_proc { > + int id; > + acpi_tbl_entry_handler handler; > + int count; > +}; > + > char * __acpi_map_table (unsigned long phys_addr, unsigned long size); > void __acpi_unmap_table(char *map, unsigned long size); > int early_acpi_boot_init(void); > @@ -149,10 +155,17 @@ int __init acpi_parse_entries(char *id, unsigned long table_size, > acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > int entry_id, unsigned int max_entries); > +int __init acpi_parse_entries_array(char *id, unsigned long table_size, > + struct acpi_table_header *table_header, > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); and you can drop this one if you make it static in tables.c > int __init acpi_table_parse_entries(char *id, unsigned long table_size, > int entry_id, > acpi_tbl_entry_handler handler, > unsigned int max_entries); > +int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int acpi_table_parse_madt(enum acpi_madt_type id, > acpi_tbl_entry_handler handler, > unsigned int max_entries); > Other than that, I gave it a quick run on arm64, and it did boot without any observable issue. If you respin it to address the above minor comments, you can add my: Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> Tested-by: Marc Zyngier <marc.zyngier@arm.com> Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers 2015-09-09 10:47 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Marc Zyngier @ 2015-09-09 13:47 ` Lukasz Anaczkowski 2015-09-09 13:47 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-09 13:47 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi From: Marc Zyngier [mailto:marc.zyngier@arm.com] Sent: Wednesday, September 9, 2015 12:48 PM > Nit: Please add a version number to the patches you send - this is at > least the 4th revision of this series, and it is harder to keep track of > what I'm reviewing. git send-email --subject-prefix "PATCH v4" is your > friend. Oh... so this is how you do that! Thanks! Let me put 'v4' on this series and I'll start counting from that. > If you respin it to address the above minor > comments, you can add my: > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Tested-by: Marc Zyngier <marc.zyngier@arm.com> I've incorporated your comments and putting tags into 1/2 patch. Thanks, Marc, for all your support! Cheers, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-09 13:47 ` Lukasz Anaczkowski @ 2015-09-09 13:47 ` Lukasz Anaczkowski 2015-09-09 13:47 ` [PATCH v4 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski 2015-09-09 20:45 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Rafael J. Wysocki 0 siblings, 2 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-09 13:47 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski This series of patches attempts to fix how CPUs are enumerated by kernel when there's more than 255 of them on single processor. In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements specified in ACPI spec. Without this patches, kernel then would first enumerate BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence of that could be performance penalties due to wrong L2 cache sharing. More details in patch 2/2. Also, simpler approach has been considered, which did not required ACPI parsing interface changes, however it failed to meet requirements. More details can be found here: https://lkml.org/lkml/2015/9/7/285 Lukasz Anaczkowski (2): acpi: Added acpi_subtable_proc to ACPI table parsers x86, acpi: Handle apic/x2apic entries in MADT in correct order arch/x86/kernel/acpi/boot.c | 22 +++++++++-- drivers/acpi/tables.c | 91 ++++++++++++++++++++++++++++++++++++--------- include/linux/acpi.h | 19 ++++++++-- 3 files changed, 107 insertions(+), 25 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers 2015-09-09 13:47 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski @ 2015-09-09 13:47 ` Lukasz Anaczkowski 2015-09-09 13:47 ` [PATCH v4 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski 2015-09-09 20:45 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Rafael J. Wysocki 1 sibling, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-09 13:47 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski ACPI subtable parsing needs to be extended to allow two or more handlers to be run in the same ACPI table walk, thus adding acpi_subtable_proc structure which stores () ACPI table id () handler that processes table () counter how many items has been processed and passing it to acpi_parse_entries_array() and acpi_table_parse_entries_array(). This is needed to fix CPU enumeration when APIC/X2APIC entries are interleaved. Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> Tested-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/acpi/tables.c | 91 +++++++++++++++++++++++++++++++++++++++++---------- include/linux/acpi.h | 19 +++++++++-- 2 files changed, 89 insertions(+), 21 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..b2608fe 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -214,20 +214,37 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } } -int __init -acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, +/** + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id + * and run proc->handler on it. Assumption is that there's only + * single handler for particular entry id. + * + * @id: table id (for debugging purposes) + * @table_size: single entry size + * @table_header: where does the table start? + * @proc: array of acpi_subtable_proc struct containing entry id + * and associated handler with it + * @proc_num: how big proc is? + * @max_entries: how many entries can we process? + * + * On success returns sum of all matching entries for all proc handlers. + * Otherwise, -ENODEV or -EINVAL is returned. + */ +static int __init +acpi_parse_entries_array(char *id, unsigned long table_size, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; - int count = 0; unsigned long table_end; + int count = 0; + int i; if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!table_size) @@ -247,20 +264,27 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + if (max_entries && count >= max_entries) + break; + + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (!proc->handler || proc[i].handler(entry, table_end)) return -EINVAL; - count++; + proc->count++; + break; } + if (i != proc_num) + count++; /* * If entry->length is 0, break from this loop to avoid * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); return -EINVAL; } @@ -270,17 +294,32 @@ acpi_parse_entries(char *id, unsigned long table_size, if (max_entries && count > max_entries) { pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + id, proc->id, count - max_entries, count); } return count; } int __init -acpi_table_parse_entries(char *id, +acpi_parse_entries(char *id, + unsigned long table_size, + acpi_tbl_entry_handler handler, + struct acpi_table_header *table_header, + int entry_id, unsigned int max_entries) +{ + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + }; + + return acpi_parse_entries_array(id, table_size, table_header, + &proc, 1, max_entries); +} + +int __init +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -291,7 +330,7 @@ acpi_table_parse_entries(char *id, if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!strncmp(id, ACPI_SIG_MADT, 4)) @@ -303,14 +342,30 @@ acpi_table_parse_entries(char *id, return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries_array(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; } int __init +acpi_table_parse_entries(char *id, + unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries) +{ + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + }; + + return acpi_table_parse_entries_array(id, table_size, &proc, 1, + max_entries); +} + +int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..3dd741f 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -150,9 +156,16 @@ int __init acpi_parse_entries(char *id, unsigned long table_size, struct acpi_table_header *table_header, int entry_id, unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, - unsigned int max_entries); + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries); +int __init acpi_table_parse_entries(char *id, unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries); +int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v4 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order 2015-09-09 13:47 ` [PATCH v4 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski @ 2015-09-09 13:47 ` Lukasz Anaczkowski 0 siblings, 0 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-09-09 13:47 UTC (permalink / raw) To: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski ACPI specifies the following rules when listing APIC IDs: (1) Boot processor is listed first (2) For multi-threaded processors, BIOS should list the first logical processor of each of the individual multi-threaded processors in MADT before listing any of the second logical processors. (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF should be listed in X2APIC subtable Because of above, when there's more than 0xFF logical CPUs, BIOS interleaves APIC/X2APIC subtables. Assuming, there's 72 cores, 72 hyper-threads each, 288 CPUs total, listing is like this: APIC (0,4,8, .., 252) X2APIC (258,260,264, .. 284) APIC (1,5,9,...,253) X2APIC (259,261,265,...,285) APIC (2,6,10,...,254) X2APIC (260,262,266,..,286) APIC (3,7,11,...,251) X2APIC (255,261,262,266,..,287) Now, before this patch, due to how ACPI MADT subtables were parsed (BSP then X2APIC then APIC), kernel enumerated CPUs in reverted order (i.e. high APIC IDs were getting low logical IDs, and low APIC IDs were getting high logical IDs). This is wrong for the following reasons: () it's hard to predict how cores and threads are enumerated () when it's hard to predict, s/w threads cannot be properly affinitized causing significant performance impact due to e.g. inproper cache sharing () enumeration is inconsistent with how threads are enumerated on other Intel Xeon processors So, order in which MADT APIC/X2APIC handlers are passed is reverse and both handlers are passed to be called during same MADT table to walk to achieve correct CPU enumeration. In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', in result less cores may be booted, since some of the CPUs the kernel will try to use will have APIC ID >= 0xFF. In such case, one should not pass 'nox2apic'. Disclimer: code parsing MADT APIC/X2APIC has not been touched since 2009, when X2APIC support was initially added. I do not know why MADT parsing code was added in the reversed order in the first place. I guess it didn't matter at that time since nobody cared about cores with APIC IDs >= 0xFF, right? This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563 Here's the explanation why parsing interface needs to be changed and why simpler approach will not work https://lkml.org/lkml/2015/9/7/285 Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> Acked-by: Thomas Gleixner <tglx@linutronix.de> (commit message) --- arch/x86/kernel/acpi/boot.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..116e911 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,8 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + int ret; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1006,22 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + ret = acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + if (ret < 0) { + printk(KERN_ERR PREFIX + "Error parsing LAPIC/X2APIC entries\n"); + return ret; + } + + x2count = madt_proc[0].count; + count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-09 13:47 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski 2015-09-09 13:47 ` [PATCH v4 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski @ 2015-09-09 20:45 ` Rafael J. Wysocki 2015-09-18 22:38 ` Rafael J. Wysocki 1 sibling, 1 reply; 52+ messages in thread From: Rafael J. Wysocki @ 2015-09-09 20:45 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On Wednesday, September 09, 2015 03:47:27 PM Lukasz Anaczkowski wrote: > This series of patches attempts to fix how CPUs are enumerated by kernel when > there's more than 255 of them on single processor. > In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements > specified in ACPI spec. Without this patches, kernel then would first enumerate > BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high > logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence > of that could be performance penalties due to wrong L2 cache sharing. > More details in patch 2/2. > > Also, simpler approach has been considered, which did not required ACPI parsing > interface changes, however it failed to meet requirements. More details can be > found here: https://lkml.org/lkml/2015/9/7/285 > > Lukasz Anaczkowski (2): > acpi: Added acpi_subtable_proc to ACPI table parsers > x86, acpi: Handle apic/x2apic entries in MADT in correct order > > arch/x86/kernel/acpi/boot.c | 22 +++++++++-- > drivers/acpi/tables.c | 91 ++++++++++++++++++++++++++++++++++++--------- > include/linux/acpi.h | 19 ++++++++-- > 3 files changed, 107 insertions(+), 25 deletions(-) OK Does anyone in the CC have any objections against merging this series? If not, I'll queue it up as 4.3-rc material. Thanks, Rafael ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs 2015-09-09 20:45 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Rafael J. Wysocki @ 2015-09-18 22:38 ` Rafael J. Wysocki 0 siblings, 0 replies; 52+ messages in thread From: Rafael J. Wysocki @ 2015-09-18 22:38 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On Wednesday, September 09, 2015 10:45:48 PM Rafael J. Wysocki wrote: > On Wednesday, September 09, 2015 03:47:27 PM Lukasz Anaczkowski wrote: > > This series of patches attempts to fix how CPUs are enumerated by kernel when > > there's more than 255 of them on single processor. > > In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements > > specified in ACPI spec. Without this patches, kernel then would first enumerate > > BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high > > logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence > > of that could be performance penalties due to wrong L2 cache sharing. > > More details in patch 2/2. > > > > Also, simpler approach has been considered, which did not required ACPI parsing > > interface changes, however it failed to meet requirements. More details can be > > found here: https://lkml.org/lkml/2015/9/7/285 > > > > Lukasz Anaczkowski (2): > > acpi: Added acpi_subtable_proc to ACPI table parsers > > x86, acpi: Handle apic/x2apic entries in MADT in correct order > > > > arch/x86/kernel/acpi/boot.c | 22 +++++++++-- > > drivers/acpi/tables.c | 91 ++++++++++++++++++++++++++++++++++++--------- > > include/linux/acpi.h | 19 ++++++++-- > > 3 files changed, 107 insertions(+), 25 deletions(-) > > OK > > Does anyone in the CC have any objections against merging this series? > > If not, I'll queue it up as 4.3-rc material. Well, I had a plan to push this for 4.3-rc2, but then I looked at it again and realized that I'd like it to stay in linux-next for a couple of weeks at least as I'm really unsure that it's not going to uncover some nastiness somewhere. Which practically means it's now scheduled for v4.4. Thanks, Rafael ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 17:49 ` [PATCH] x86, arm64, " Lukasz Anaczkowski 2015-08-27 9:37 ` Lorenzo Pieralisi @ 2015-08-28 8:30 ` Ingo Molnar 2015-09-01 8:02 ` Tomasz Nowicki 2 siblings, 0 replies; 52+ messages in thread From: Ingo Molnar @ 2015-08-28 8:30 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: marc.zyngier, lorenzo.pieralisi, tomasz.nowicki, tglx, mingo, hpa, x86, jason, rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu * Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote: > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563, > thus putting Yinghai Lu as 'Signed-off-by', as well. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> > Acked-by: Thomas Gleixner <tglx@linutronix.de> > --- > arch/x86/kernel/acpi/boot.c | 29 ++++++++---- > drivers/acpi/numa.c | 28 ++++++++--- > drivers/acpi/tables.c | 113 +++++++++++++++++++++++++++++++++----------- > drivers/irqchip/irq-gic.c | 15 ++++-- > include/linux/acpi.h | 13 ++++- > 5 files changed, 149 insertions(+), 49 deletions(-) Yeah, so this patch is way too large, it should be split into 3-4 parts that first introduce separate changes (such as renames), then add any extra functions that might be needed, and finally does the minimal changes that actually change behavior. Thanks, Ingo ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-08-26 17:49 ` [PATCH] x86, arm64, " Lukasz Anaczkowski 2015-08-27 9:37 ` Lorenzo Pieralisi 2015-08-28 8:30 ` [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Ingo Molnar @ 2015-09-01 8:02 ` Tomasz Nowicki 2015-09-01 12:07 ` Anaczkowski, Lukasz 2 siblings, 1 reply; 52+ messages in thread From: Tomasz Nowicki @ 2015-09-01 8:02 UTC (permalink / raw) To: Lukasz Anaczkowski, marc.zyngier, lorenzo.pieralisi, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On 26.08.2015 19:49, Lukasz Anaczkowski wrote: > v3: > () Fixed entries counting > () Added missing acpi_table_parse_entries definition > () acpi_parse_entries() now returns sum of all matching > entries > > v2: Fixed ARM64 syntax error > > From the ACPI spec: > "Logical processors with APIC ID values less than 0xFF > (whether in XAPIC or X2APIC mode) must use the Processor LAPIC > structure [...]. Logical processors with APIC ID values 0xFF and > greater must use the Processor Local x2APIC structure." > > Because of above, BIOS is first enumerating cores with HT with > LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF). > > With current kernel code, where enumeration is in order: > BSP, X2APIC, LAPIC > enumeration on machine with more than 255 CPUs (each core with 4 HT) > first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs > get higher logical CPU IDs (50..y), as in example below: > > Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId > 0 0 0000 97 0001 145 0002 193 0003 > 1 50 0004 98 0005 146 0006 194 0007 > 2 51 0010 99 0011 147 0012 195 0013 > 3 52 0014 100 0015 148 0016 196 0017 > 4 53 0018 101 0019 149 001a 197 001b > 5 54 001c 102 001d 150 001e 198 001f > ... > 62 95 00f8 143 00f9 191 00fa 239 00fb > 63 37 00ff 96 00fc 144 00fd 192 00fe > 64 1 0100 13 0101 25 0102 38 0103 > 65 2 0104 14 0105 26 0106 39 0107 > ... > > (Core - physical core, LCpu - logical CPU, ApicId - ID assigned > by BIOS). > > This is wrong for the following reasons: > () it's hard to predict how cores and threads will be enumerated > () when it's hard to predict, s/w threads cannot be properly affinitized > causing significant performance impact due to e.g. inproper cache > sharing > () enumeration is inconsistent with how threads are enumerated on > other Intel Xeon processors > > To fix this, each LAPIC/X2APIC entry from MADT table needs to be > handled at the same time when processing it, thus adding > acpi_subtable_proc structure which stores > () ACPI table id > () handler that processes table > () counter how many items has been processed > and passing it to acpi_table_parse_entries(). Why can't you leave the parsing code as is and create ApicId sorted list while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after all... Do I miss something ? Tomasz > > Also, order in which MADT LAPIC/X2APIC handlers are passed is > reversed to achieve correct CPU enumeration. > > In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', > in result less cores may be booted, since some of the CPUs the kernel > will try to use will have APIC ID >= 0xFF. In such case, one > should not pass 'nox2apic'. > > Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since 2009, > when X2APIC support was initially added. I do not know why MADT parsing > code was added in the reversed order in the first place. > I guess it didn't matter at that time since nobody cared about cores > with APIC IDs >= 0xFF, right? > > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563, > thus putting Yinghai Lu as 'Signed-off-by', as well. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> > Acked-by: Thomas Gleixner <tglx@linutronix.de> > --- > arch/x86/kernel/acpi/boot.c | 29 ++++++++---- > drivers/acpi/numa.c | 28 ++++++++--- > drivers/acpi/tables.c | 113 +++++++++++++++++++++++++++++++++----------- > drivers/irqchip/irq-gic.c | 15 ++++-- > include/linux/acpi.h | 13 ++++- > 5 files changed, 149 insertions(+), 49 deletions(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index e49ee24..fb4a9d6 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) > { > int count; > int x2count = 0; > + struct acpi_subtable_proc madt_proc[2]; > > if (!cpu_has_apic) > return -ENODEV; > @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void) > acpi_parse_sapic, MAX_LOCAL_APIC); > > if (!count) { > - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, > - acpi_parse_x2apic, MAX_LOCAL_APIC); > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, > - acpi_parse_lapic, MAX_LOCAL_APIC); > + memset(madt_proc, 0, sizeof(madt_proc)); > + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; > + madt_proc[0].handler = acpi_parse_lapic; > + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; > + madt_proc[1].handler = acpi_parse_x2apic; > + acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); > + count = madt_proc[0].count; > + x2count = madt_proc[1].count; > } > if (!count && !x2count) { > printk(KERN_ERR PREFIX "No LAPIC entries present\n"); > @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) > return count; > } > > - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, > - acpi_parse_x2apic_nmi, 0); > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, > - acpi_parse_lapic_nmi, 0); > + memset(madt_proc, 0, sizeof(madt_proc)); > + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; > + madt_proc[0].handler = acpi_parse_x2apic_nmi; > + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; > + madt_proc[1].handler = acpi_parse_lapic_nmi; > + acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + madt_proc, ARRAY_SIZE(madt_proc), 0); > + count = madt_proc[0].count; > + x2count = madt_proc[1].count; > if (count < 0 || x2count < 0) { > printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); > /* TBD: Cleanup to allow fallback to MPS */ > diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c > index acaa3b4..a000195 100644 > --- a/drivers/acpi/numa.c > +++ b/drivers/acpi/numa.c > @@ -314,9 +314,15 @@ static int __init > acpi_table_parse_srat(enum acpi_srat_type id, > acpi_tbl_entry_handler handler, unsigned int max_entries) > { > - return acpi_table_parse_entries(ACPI_SIG_SRAT, > - sizeof(struct acpi_table_srat), id, > - handler, max_entries); > + struct acpi_subtable_proc srat_proc; > + > + memset(&srat_proc, 0, sizeof(srat_proc)); > + srat_proc.id = id; > + srat_proc.handler = handler; > + > + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, > + sizeof(struct acpi_table_srat), > + &srat_proc, 1, max_entries); > } > > int __init acpi_numa_init(void) > @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) > > /* SRAT: Static Resource Affinity Table */ > if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { > - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, > - acpi_parse_x2apic_affinity, 0); > - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, > - acpi_parse_processor_affinity, 0); > + struct acpi_subtable_proc srat_proc[2]; > + > + memset(srat_proc, 0, sizeof(srat_proc)); > + srat_proc[0].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; > + srat_proc[0].handler = acpi_parse_x2apic_affinity; > + srat_proc[1].id = ACPI_SRAT_TYPE_CPU_AFFINITY; > + srat_proc[1].handler = acpi_parse_processor_affinity; > + > + acpi_table_parse_entries_array(ACPI_SIG_SRAT, > + sizeof(struct acpi_table_srat), > + srat_proc, ARRAY_SIZE(srat_proc), 0); > + > cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, > acpi_parse_memory_affinity, > NR_NODE_MEMBLKS); > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 2e19189..d5c9a1b 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -214,27 +214,45 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > } > } > > +/** > + * acpi_table_parse - for each proc_num find a subtable with proc->id > + * and run proc->handler on it. Assumption is that there's only > + * single handler for particular id. > + * > + * @id: table id (for debugging purposes) > + * @table_size: single entry size > + * @table_header: where does the table start? > + * @proc: array of acpi_subtable_proc struct containing subtable id > + * and associated handler with it > + * @proc_num: how big proc is? > + * @max_entries: how many entries can we process? > + * > + * On success returns sum of all matching entries for all proc handlers. > + * Oterwise, -ENODEV or -EINVAL is returned. > + */ > int __init > acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries) > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries) > { > struct acpi_subtable_header *entry; > int count = 0; > unsigned long table_end; > + int i; > > - if (acpi_disabled) > + if (acpi_disabled) { > + proc[0].count = -ENODEV; > return -ENODEV; > - > - if (!id || !handler) > - return -EINVAL; > - > - if (!table_size) > + } > + if (!table_size) { > + proc[0].count = -EINVAL; > return -EINVAL; > + } > > if (!table_header) { > pr_warn("%4.4s not present\n", id); > + proc[0].count = -ENODEV; > return -ENODEV; > } > > @@ -247,20 +265,31 @@ acpi_parse_entries(char *id, unsigned long table_size, > > while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < > table_end) { > - if (entry->type == entry_id > - && (!max_entries || count < max_entries)) { > - if (handler(entry, table_end)) > + if (max_entries && count >= max_entries) > + continue; > + for (i = 0; i < proc_num; i++) { > + if (entry->type != proc[i].id) > + continue; > + if (proc[i].handler(entry, table_end)) { > + proc[i].count = -EINVAL; > return -EINVAL; > - > - count++; > + } > + proc[i].count++; > + break; > } > + if (i != proc_num) > + count++; > > /* > * If entry->length is 0, break from this loop to avoid > * infinite loop. > */ > if (entry->length == 0) { > - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); > + pr_err("[%4.4s:0x%02x ", id, proc[0].id); > + for (i = 1; i < proc_num; i++) > + pr_cont(" 0x%02x", proc[i].id); > + pr_cont("] Invalid zero length\n"); > + proc[0].count = -EINVAL; > return -EINVAL; > } > > @@ -269,18 +298,20 @@ acpi_parse_entries(char *id, unsigned long table_size, > } > > if (max_entries && count > max_entries) { > - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", > - id, entry_id, count - max_entries, count); > + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); > + for (i = 1; i < proc_num; i++) > + pr_cont(" 0x%02x", proc[i].id); > + pr_cont("] ignored %i entries of %i found\n", > + count-max_entries, count); > } > > return count; > } > > int __init > -acpi_table_parse_entries(char *id, > +acpi_table_parse_entries_array(char *id, > unsigned long table_size, > - int entry_id, > - acpi_tbl_entry_handler handler, > + struct acpi_subtable_proc *proc, int proc_num, > unsigned int max_entries) > { > struct acpi_table_header *table_header = NULL; > @@ -288,11 +319,10 @@ acpi_table_parse_entries(char *id, > int count; > u32 instance = 0; > > - if (acpi_disabled) > + if (acpi_disabled) { > + proc[0].count = -ENODEV; > return -ENODEV; > - > - if (!id || !handler) > - return -EINVAL; > + } > > if (!strncmp(id, ACPI_SIG_MADT, 4)) > instance = acpi_apic_instance; > @@ -300,23 +330,50 @@ acpi_table_parse_entries(char *id, > acpi_get_table_with_size(id, instance, &table_header, &tbl_size); > if (!table_header) { > pr_warn("%4.4s not present\n", id); > + proc[0].count = -ENODEV; > return -ENODEV; > } > > - count = acpi_parse_entries(id, table_size, handler, table_header, > - entry_id, max_entries); > + count = acpi_parse_entries(id, table_size, table_header, > + proc, proc_num, max_entries); > > early_acpi_os_unmap_memory((char *)table_header, tbl_size); > return count; > } > > int __init > +acpi_table_parse_entries(char *id, > + unsigned long table_size, > + int entry_id, > + acpi_tbl_entry_handler handler, > + unsigned int max_entries) > +{ > + struct acpi_subtable_proc proc[1]; > + > + if (!handler) > + return -EINVAL; > + > + memset(proc, 0, sizeof(proc)); > + proc[0].id = entry_id; > + proc[0].handler = handler; > + > + return acpi_table_parse_entries_array(id, table_size, proc, 1, > + max_entries); > +} > + > +int __init > acpi_table_parse_madt(enum acpi_madt_type id, > acpi_tbl_entry_handler handler, unsigned int max_entries) > { > - return acpi_table_parse_entries(ACPI_SIG_MADT, > - sizeof(struct acpi_table_madt), id, > - handler, max_entries); > + struct acpi_subtable_proc madt_proc; > + > + memset(&madt_proc, 0, sizeof(madt_proc)); > + madt_proc.id = id; > + madt_proc.handler = handler; > + > + return acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + &madt_proc, 1, max_entries); > } > > /** > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 4dd8826..d98b866 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) > { > void __iomem *cpu_base, *dist_base; > int count; > + struct acpi_subtable_proc gic_proc; > + > + memset(&gic_proc, 0, sizeof(gic_proc)); > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; > + gic_proc.handler = gic_acpi_parse_madt_cpu; > > /* Collect CPU base addresses */ > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_cpu, table, > - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); > + table, &gic_proc, 1, 0); > if (count <= 0) { > pr_err("No valid GICC entries exist\n"); > return -EINVAL; > @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) > * Find distributor base address. We expect one distributor entry since > * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. > */ > + memset(&gic_proc, 0, sizeof(gic_proc)); > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; > + gic_proc.handler = gic_acpi_parse_madt_distributor; > + > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_distributor, table, > - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); > + table, &gic_proc, 1, 0); > if (count <= 0) { > pr_err("No valid GICD entries exist\n"); > return -EINVAL; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index d2445fa..59b17e8 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) > (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) > > +struct acpi_subtable_proc { > + int id; > + acpi_tbl_entry_handler handler; > + int count; > +}; > + > char * __acpi_map_table (unsigned long phys_addr, unsigned long size); > void __acpi_unmap_table(char *map, unsigned long size); > int early_acpi_boot_init(void); > @@ -145,10 +151,13 @@ int acpi_numa_init (void); > > int acpi_table_init (void); > int acpi_table_parse(char *id, acpi_tbl_table_handler handler); > +int acpi_table_parse_entries_array(char *id, unsigned long table_size, > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int __init acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries); > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int __init acpi_table_parse_entries(char *id, unsigned long table_size, > int entry_id, > acpi_tbl_entry_handler handler, > ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-09-01 8:02 ` Tomasz Nowicki @ 2015-09-01 12:07 ` Anaczkowski, Lukasz 2015-09-01 13:36 ` Tomasz Nowicki 0 siblings, 1 reply; 52+ messages in thread From: Anaczkowski, Lukasz @ 2015-09-01 12:07 UTC (permalink / raw) To: Tomasz Nowicki, marc.zyngier, lorenzo.pieralisi, tglx, mingo, hpa, x86, jason Cc: rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org] Sent: Tuesday, September 1, 2015 10:03 AM >> To fix this, each LAPIC/X2APIC entry from MADT table needs to be >> handled at the same time when processing it, thus adding >> acpi_subtable_proc structure which stores >> () ACPI table id >> () handler that processes table >> () counter how many items has been processed and passing it to >> acpi_table_parse_entries(). > Why can't you leave the parsing code as is and create ApicId sorted list while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after all... Do I miss something ? Just to make sure I understand correctly - you suggest to replace calls to acpi_register_lapic() with a code that builds an APIC ID list while parsing LAPIC/X2APIC, and after parsing is done, go thru the list and call acpi_register_lapic() on each APIC ID, correct? Thanks, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-09-01 12:07 ` Anaczkowski, Lukasz @ 2015-09-01 13:36 ` Tomasz Nowicki 2015-09-07 14:04 ` Anaczkowski, Lukasz 0 siblings, 1 reply; 52+ messages in thread From: Tomasz Nowicki @ 2015-09-01 13:36 UTC (permalink / raw) To: Anaczkowski, Lukasz, marc.zyngier, lorenzo.pieralisi, tglx, mingo, hpa, x86, jason Cc: rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On 01.09.2015 14:07, Anaczkowski, Lukasz wrote: > From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org] > Sent: Tuesday, September 1, 2015 10:03 AM > >>> To fix this, each LAPIC/X2APIC entry from MADT table needs to be >>> handled at the same time when processing it, thus adding >>> acpi_subtable_proc structure which stores >>> () ACPI table id >>> () handler that processes table >>> () counter how many items has been processed and passing it to >>> acpi_table_parse_entries(). > >> Why can't you leave the parsing code as is and create ApicId sorted list while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after all... Do I miss something ? > > Just to make sure I understand correctly - you suggest to replace calls to acpi_register_lapic() with a code that builds an APIC ID list while parsing LAPIC/X2APIC, and after parsing is done, > go thru the list and call acpi_register_lapic() on each APIC ID, correct? > Yes, does it work for you? Tomasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-09-01 13:36 ` Tomasz Nowicki @ 2015-09-07 14:04 ` Anaczkowski, Lukasz 2015-09-08 14:44 ` Tomasz Nowicki 0 siblings, 1 reply; 52+ messages in thread From: Anaczkowski, Lukasz @ 2015-09-07 14:04 UTC (permalink / raw) To: Tomasz Nowicki, marc.zyngier, lorenzo.pieralisi, tglx, mingo, hpa, x86, jason Cc: rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org] Sent: Tuesday, September 1, 2015 3:37 PM > On 01.09.2015 14:07, Anaczkowski, Lukasz wrote: >> From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org] >> Sent: Tuesday, September 1, 2015 10:03 AM >>> >>>> To fix this, each LAPIC/X2APIC entry from MADT table needs to be >>>> handled at the same time when processing it, thus adding >>>> acpi_subtable_proc structure which stores >>>> () ACPI table id >>>> () handler that processes table >>>> () counter how many items has been processed and passing it to >>>> acpi_table_parse_entries(). >> >>> Why can't you leave the parsing code as is and create ApicId sorted list while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after all... Do I miss something ? >> >> Just to make sure I understand correctly - you suggest to replace >> calls to acpi_register_lapic() with a code that builds an APIC ID list while parsing LAPIC/X2APIC, and after parsing is done, go thru the list and call acpi_register_lapic() on each APIC ID, correct? >> > > Yes, does it work for you? Hi Tomasz, sorry for late response, I was distracted by other things. So, I see two options to build the list: (a) use APIC ID as the table index (b) always append APIC ID to the end of table, in the order that BIOS lists them Also, my goal is to end up with enumeration like this (assuming there's 72 cores, 4 hyper threads each, total 288 logical CPUs): APIC ID -> Logical ID 0 -> 0 1 -> 72 2 -> 144 3 -> 216 4 -> 1 5 -> 73 6 -> 145 7 -> 217 8 -> 2 ... 284 -> 71 285 -> 143 286 -> 215 287 -> 287 Note that n,n+1,n+2,n+3 APIC IDs share same physical core, while being separated by core count in logical listing (e.g. 0,72,144,216 share same physical core). Now, ACPI spec specifies how APIC IDs should be listed: (1) Boot processor is listed first (2) For multi-threaded processors, BIOS should list the first logical processor of each of the individual multi-threaded processors in MADT before listing any of the second logical processors. (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF should be listed in X2APIC subtable Keeping in mind above, BIOS lists APIC IDs as: APIC (0,4,8, .., 252) X2APIC (256,260,264, .. 284) APIC (1,5,9,...,253) X2API (257,261,265, 285) etc so in the end (2) rule is followed. So, when (a) indexing is selected, mapping will be like: APIC ID -> Logical ID 0 -> 0 1 -> 1 2 -> 2 3 -> 3 ... 287 -> 287 If (b) indexing is selected, mapping is like: APIC ID -> Logical ID 0 -> 0 1 -> 63 2 -> 126 3 -> 189 4 -> 1 5 -> 64 ... 284 -> 256 285 -> 266 286 -> 276 287 -> 286 Why? Because first APIC entries will be in the list (since this is the first parser to run), and just then X2APIC entries will be in the list. So, long story short, unless you see other simple option on how to build the list of APIC IDs, I'm sorry to say that this approach will not work. Cheers, Lukasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT 2015-09-07 14:04 ` Anaczkowski, Lukasz @ 2015-09-08 14:44 ` Tomasz Nowicki 0 siblings, 0 replies; 52+ messages in thread From: Tomasz Nowicki @ 2015-09-08 14:44 UTC (permalink / raw) To: Anaczkowski, Lukasz Cc: Tomasz Nowicki, marc.zyngier, lorenzo.pieralisi, tglx, mingo, hpa, x86, jason, rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On 07.09.2015 16:04, Anaczkowski, Lukasz wrote: > > From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org] > Sent: Tuesday, September 1, 2015 3:37 PM >> On 01.09.2015 14:07, Anaczkowski, Lukasz wrote: >>> From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org] >>> Sent: Tuesday, September 1, 2015 10:03 AM >>>> >>>>> To fix this, each LAPIC/X2APIC entry from MADT table needs to be >>>>> handled at the same time when processing it, thus adding >>>>> acpi_subtable_proc structure which stores >>>>> () ACPI table id >>>>> () handler that processes table >>>>> () counter how many items has been processed and passing it to >>>>> acpi_table_parse_entries(). >>> >>>> Why can't you leave the parsing code as is and create ApicId sorted list while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after all... Do I miss something ? >>> >>> Just to make sure I understand correctly - you suggest to replace >>> calls to acpi_register_lapic() with a code that builds an APIC ID list while parsing LAPIC/X2APIC, and after parsing is done, go thru the list and call acpi_register_lapic() on each APIC ID, correct? >>> >> >> Yes, does it work for you? > > Hi Tomasz, sorry for late response, I was distracted by other things. > > So, I see two options to build the list: > > (a) use APIC ID as the table index > (b) always append APIC ID to the end of table, in the order that BIOS lists them > > Also, my goal is to end up with enumeration like this (assuming there's 72 cores, 4 hyper threads each, total 288 logical CPUs): > > APIC ID -> Logical ID > 0 -> 0 > 1 -> 72 > 2 -> 144 > 3 -> 216 > 4 -> 1 > 5 -> 73 > 6 -> 145 > 7 -> 217 > 8 -> 2 > ... > 284 -> 71 > 285 -> 143 > 286 -> 215 > 287 -> 287 > > Note that n,n+1,n+2,n+3 APIC IDs share same physical core, while being separated by core count in logical listing (e.g. 0,72,144,216 share same physical core). > > Now, ACPI spec specifies how APIC IDs should be listed: > (1) Boot processor is listed first > (2) For multi-threaded processors, BIOS should list the first logical processor of each of the individual multi-threaded processors in MADT before listing any of the second logical processors. > (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF should be listed in X2APIC subtable > > Keeping in mind above, BIOS lists APIC IDs as: > > APIC (0,4,8, .., 252) > X2APIC (256,260,264, .. 284) > APIC (1,5,9,...,253) > X2API (257,261,265, 285) > etc OK got it, your patches make sense then. Tomasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-03 18:26 ` Lukasz Anaczkowski 2015-08-26 7:04 ` Anaczkowski, Lukasz @ 2015-08-26 11:03 ` Marc Zyngier 2015-08-26 12:56 ` Tomasz Nowicki 2 siblings, 0 replies; 52+ messages in thread From: Marc Zyngier @ 2015-08-26 11:03 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: tglx, mingo, hpa, x86, jason, rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On 2015-08-03 19:26, Lukasz Anaczkowski wrote: > v2: Fixed ARM64 syntax error > > From the ACPI spec: > "Logical processors with APIC ID values less than 0xFF > (whether in XAPIC or X2APIC mode) must use the Processor LAPIC > structure [...]. Logical processors with APIC ID values 0xFF and > greater must use the Processor Local x2APIC structure." > > Because of above, BIOS is first enumerating cores with HT with > LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF). > > With current kernel code, where enumeration is in order: > BSP, X2APIC, LAPIC > enumeration on machine with more than 255 CPUs (each core with 4 HT) > first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs > get higher logical CPU IDs (50..y), as in example below: > > Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId > 0 0 0000 97 0001 145 0002 193 0003 > 1 50 0004 98 0005 146 0006 194 0007 > 2 51 0010 99 0011 147 0012 195 0013 > 3 52 0014 100 0015 148 0016 196 0017 > 4 53 0018 101 0019 149 001a 197 001b > 5 54 001c 102 001d 150 001e 198 001f > ... > 62 95 00f8 143 00f9 191 00fa 239 00fb > 63 37 00ff 96 00fc 144 00fd 192 00fe > 64 1 0100 13 0101 25 0102 38 0103 > 65 2 0104 14 0105 26 0106 39 0107 > ... > > (Core - physical core, LCpu - logical CPU, ApicId - ID assigned > by BIOS). > > This is wrong for the following reasons: > () it's hard to predict how cores and threads will be enumerated > () when it's hard to predict, s/w threads cannot be properly > affinitized > causing significant performance impact due to e.g. inproper cache > sharing > () enumeration is inconsistent with how threads are enumerated on > other Intel Xeon processors > > To fix this, each LAPIC/X2APIC entry from MADT table needs to be > handled at the same time when processing it, thus adding > acpi_subtable_proc structure which stores > () ACPI table id > () handler that processes table > () counter how many items has been processed > and passing it to acpi_table_parse_entries(). > > Also, order in which MADT LAPIC/X2APIC handlers are passed is > reversed to achieve correct CPU enumeration. > > In scenario when someone boots kernel with options 'maxcpus=72 > nox2apic', > in result less cores may be booted, since some of the CPUs the kernel > will try to use will have APIC ID >= 0xFF. In such case, one > should not pass 'nox2apic'. > > Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since > 2009, > when X2APIC support was initially added. I do not know why MADT > parsing > code was added in the reversed order in the first place. > I guess it didn't matter at that time since nobody cared about cores > with APIC IDs >= 0xFF, right? > > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563, > thus putting Yinghai Lu as 'Signed-off-by', as well. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> It looked good, but I then made the mistake to actually try it. Bad idea: [...] RCU: Adjusting geometry for rcu_fanout_leaf=64, nr_cpu_ids=6 NR_IRQS:64 nr_irqs:64 0 No valid GICC entries exist ACPI: Failed to initialize GIC IRQ controller Kernel panic - not syncing: No interrupt controller found. CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.2.0-rc8+ #4623 Hardware name: ARM Juno development board (r1) (DT) Call trace: [<ffffffc000089960>] dump_backtrace+0x0/0x124 [<ffffffc000089a94>] show_stack+0x10/0x1c [<ffffffc000658968>] dump_stack+0x84/0xc8 [<ffffffc0006577d8>] panic+0xe0/0x220 [<ffffffc0008cd2f4>] init_IRQ+0x20/0x2c [<ffffffc0008cb850>] start_kernel+0x260/0x3b8 ---[ end Kernel panic - not syncing: No interrupt controller found. Reverting the patch makes the machine boot again. As I presume you don't have access to such a box, I'll try to investigate it shortly. Thanks, M. -- Fast, cheap, reliable. Pick two. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT 2015-08-03 18:26 ` Lukasz Anaczkowski 2015-08-26 7:04 ` Anaczkowski, Lukasz 2015-08-26 11:03 ` [PATCH] x86, " Marc Zyngier @ 2015-08-26 12:56 ` Tomasz Nowicki 2 siblings, 0 replies; 52+ messages in thread From: Tomasz Nowicki @ 2015-08-26 12:56 UTC (permalink / raw) To: Lukasz Anaczkowski, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Yinghai Lu On 03.08.2015 20:26, Lukasz Anaczkowski wrote: > v2: Fixed ARM64 syntax error > > From the ACPI spec: > "Logical processors with APIC ID values less than 0xFF > (whether in XAPIC or X2APIC mode) must use the Processor LAPIC > structure [...]. Logical processors with APIC ID values 0xFF and > greater must use the Processor Local x2APIC structure." > > Because of above, BIOS is first enumerating cores with HT with > LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF). > > With current kernel code, where enumeration is in order: > BSP, X2APIC, LAPIC > enumeration on machine with more than 255 CPUs (each core with 4 HT) > first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs > get higher logical CPU IDs (50..y), as in example below: > > Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId > 0 0 0000 97 0001 145 0002 193 0003 > 1 50 0004 98 0005 146 0006 194 0007 > 2 51 0010 99 0011 147 0012 195 0013 > 3 52 0014 100 0015 148 0016 196 0017 > 4 53 0018 101 0019 149 001a 197 001b > 5 54 001c 102 001d 150 001e 198 001f > ... > 62 95 00f8 143 00f9 191 00fa 239 00fb > 63 37 00ff 96 00fc 144 00fd 192 00fe > 64 1 0100 13 0101 25 0102 38 0103 > 65 2 0104 14 0105 26 0106 39 0107 > ... > > (Core - physical core, LCpu - logical CPU, ApicId - ID assigned > by BIOS). > > This is wrong for the following reasons: > () it's hard to predict how cores and threads will be enumerated > () when it's hard to predict, s/w threads cannot be properly affinitized > causing significant performance impact due to e.g. inproper cache > sharing > () enumeration is inconsistent with how threads are enumerated on > other Intel Xeon processors > > To fix this, each LAPIC/X2APIC entry from MADT table needs to be > handled at the same time when processing it, thus adding > acpi_subtable_proc structure which stores > () ACPI table id > () handler that processes table > () counter how many items has been processed > and passing it to acpi_table_parse_entries(). > > Also, order in which MADT LAPIC/X2APIC handlers are passed is > reversed to achieve correct CPU enumeration. > > In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', > in result less cores may be booted, since some of the CPUs the kernel > will try to use will have APIC ID >= 0xFF. In such case, one > should not pass 'nox2apic'. > > Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since 2009, > when X2APIC support was initially added. I do not know why MADT parsing > code was added in the reversed order in the first place. > I guess it didn't matter at that time since nobody cared about cores > with APIC IDs >= 0xFF, right? > > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563, > thus putting Yinghai Lu as 'Signed-off-by', as well. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> > --- > arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- > drivers/acpi/numa.c | 28 ++++++++++++----- > drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- > drivers/irqchip/irq-gic.c | 15 ++++++--- > include/linux/acpi.h | 13 ++++++-- > 5 files changed, 111 insertions(+), 49 deletions(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index e49ee24..fb4a9d6 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) > { > int count; > int x2count = 0; > + struct acpi_subtable_proc madt_proc[2]; > > if (!cpu_has_apic) > return -ENODEV; > @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void) > acpi_parse_sapic, MAX_LOCAL_APIC); > > if (!count) { > - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, > - acpi_parse_x2apic, MAX_LOCAL_APIC); > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, > - acpi_parse_lapic, MAX_LOCAL_APIC); > + memset(madt_proc, 0, sizeof(madt_proc)); > + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; > + madt_proc[0].handler = acpi_parse_lapic; > + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; > + madt_proc[1].handler = acpi_parse_x2apic; > + acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); > + count = madt_proc[0].count; > + x2count = madt_proc[1].count; > } > if (!count && !x2count) { > printk(KERN_ERR PREFIX "No LAPIC entries present\n"); > @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) > return count; > } > > - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, > - acpi_parse_x2apic_nmi, 0); > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, > - acpi_parse_lapic_nmi, 0); > + memset(madt_proc, 0, sizeof(madt_proc)); > + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; > + madt_proc[0].handler = acpi_parse_x2apic_nmi; > + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; > + madt_proc[1].handler = acpi_parse_lapic_nmi; > + acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + madt_proc, ARRAY_SIZE(madt_proc), 0); > + count = madt_proc[0].count; > + x2count = madt_proc[1].count; > if (count < 0 || x2count < 0) { > printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); > /* TBD: Cleanup to allow fallback to MPS */ > diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c > index acaa3b4..a000195 100644 > --- a/drivers/acpi/numa.c > +++ b/drivers/acpi/numa.c > @@ -314,9 +314,15 @@ static int __init > acpi_table_parse_srat(enum acpi_srat_type id, > acpi_tbl_entry_handler handler, unsigned int max_entries) > { > - return acpi_table_parse_entries(ACPI_SIG_SRAT, > - sizeof(struct acpi_table_srat), id, > - handler, max_entries); > + struct acpi_subtable_proc srat_proc; > + > + memset(&srat_proc, 0, sizeof(srat_proc)); > + srat_proc.id = id; > + srat_proc.handler = handler; > + > + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, > + sizeof(struct acpi_table_srat), > + &srat_proc, 1, max_entries); > } > > int __init acpi_numa_init(void) > @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) > > /* SRAT: Static Resource Affinity Table */ > if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { > - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, > - acpi_parse_x2apic_affinity, 0); > - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, > - acpi_parse_processor_affinity, 0); > + struct acpi_subtable_proc srat_proc[2]; > + > + memset(srat_proc, 0, sizeof(srat_proc)); > + srat_proc[0].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; > + srat_proc[0].handler = acpi_parse_x2apic_affinity; > + srat_proc[1].id = ACPI_SRAT_TYPE_CPU_AFFINITY; > + srat_proc[1].handler = acpi_parse_processor_affinity; > + > + acpi_table_parse_entries_array(ACPI_SIG_SRAT, > + sizeof(struct acpi_table_srat), > + srat_proc, ARRAY_SIZE(srat_proc), 0); > + > cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, > acpi_parse_memory_affinity, > NR_NODE_MEMBLKS); > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 2e19189..1217e41 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -216,25 +216,27 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > > int __init > acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries) > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries) > { > struct acpi_subtable_header *entry; > int count = 0; > unsigned long table_end; > + int i; > > - if (acpi_disabled) > + if (acpi_disabled) { > + proc[0].count = -ENODEV; > return -ENODEV; > - > - if (!id || !handler) > - return -EINVAL; > - > - if (!table_size) > + } > + if (!table_size) { > + proc[0].count = -EINVAL; > return -EINVAL; > + } > > if (!table_header) { > pr_warn("%4.4s not present\n", id); > + proc[0].count = -ENODEV; > return -ENODEV; > } > > @@ -247,12 +249,17 @@ acpi_parse_entries(char *id, unsigned long table_size, > > while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < > table_end) { > - if (entry->type == entry_id > - && (!max_entries || count < max_entries)) { > - if (handler(entry, table_end)) > + for (i = 0; i < proc_num; i++) { > + if (entry->type != proc[i].id) > + continue; > + if (max_entries && count++ >= max_entries) > + continue; > + if (proc[i].handler(entry, table_end)) { > + proc[i].count = -EINVAL; > return -EINVAL; > - > - count++; > + } > + proc[i].count++; > + break; > } > > /* > @@ -260,7 +267,11 @@ acpi_parse_entries(char *id, unsigned long table_size, > * infinite loop. > */ > if (entry->length == 0) { > - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); > + pr_err("[%4.4s:0x%02x ", id, proc[0].id); > + for (i = 1; i < proc_num; i++) > + pr_cont(" 0x%02x", proc[i].id); > + pr_cont("] Invalid zero length\n"); > + proc[0].count = -EINVAL; > return -EINVAL; > } > > @@ -269,18 +280,20 @@ acpi_parse_entries(char *id, unsigned long table_size, > } > > if (max_entries && count > max_entries) { > - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", > - id, entry_id, count - max_entries, count); > + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); > + for (i = 1; i < proc_num; i++) > + pr_cont(" 0x%02x", proc[i].id); > + pr_cont("] ignored %i entries of %i found\n", > + count-max_entries, count); > } > > return count; > } > > int __init > -acpi_table_parse_entries(char *id, > +acpi_table_parse_entries_array(char *id, > unsigned long table_size, > - int entry_id, > - acpi_tbl_entry_handler handler, > + struct acpi_subtable_proc *proc, int proc_num, > unsigned int max_entries) > { > struct acpi_table_header *table_header = NULL; > @@ -288,11 +301,10 @@ acpi_table_parse_entries(char *id, > int count; > u32 instance = 0; > > - if (acpi_disabled) > + if (acpi_disabled) { > + proc[0].count = -ENODEV; > return -ENODEV; > - > - if (!id || !handler) > - return -EINVAL; > + } > > if (!strncmp(id, ACPI_SIG_MADT, 4)) > instance = acpi_apic_instance; > @@ -300,11 +312,12 @@ acpi_table_parse_entries(char *id, > acpi_get_table_with_size(id, instance, &table_header, &tbl_size); > if (!table_header) { > pr_warn("%4.4s not present\n", id); > + proc[0].count = -ENODEV; > return -ENODEV; > } > > - count = acpi_parse_entries(id, table_size, handler, table_header, > - entry_id, max_entries); > + count = acpi_parse_entries(id, table_size, table_header, > + proc, proc_num, max_entries); > > early_acpi_os_unmap_memory((char *)table_header, tbl_size); > return count; > @@ -314,9 +327,15 @@ int __init > acpi_table_parse_madt(enum acpi_madt_type id, > acpi_tbl_entry_handler handler, unsigned int max_entries) > { > - return acpi_table_parse_entries(ACPI_SIG_MADT, > - sizeof(struct acpi_table_madt), id, > - handler, max_entries); > + struct acpi_subtable_proc madt_proc; > + > + memset(&madt_proc, 0, sizeof(madt_proc)); > + madt_proc.id = id; > + madt_proc.handler = handler; > + > + return acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + &madt_proc, 1, max_entries); > } > > /** > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 4dd8826..d98b866 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) > { > void __iomem *cpu_base, *dist_base; > int count; > + struct acpi_subtable_proc gic_proc; > + > + memset(&gic_proc, 0, sizeof(gic_proc)); > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; > + gic_proc.handler = gic_acpi_parse_madt_cpu; > > /* Collect CPU base addresses */ > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_cpu, table, > - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); > + table, &gic_proc, 1, 0); > if (count <= 0) { > pr_err("No valid GICC entries exist\n"); > return -EINVAL; > @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) > * Find distributor base address. We expect one distributor entry since > * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. > */ > + memset(&gic_proc, 0, sizeof(gic_proc)); > + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; > + gic_proc.handler = gic_acpi_parse_madt_distributor; > + > count = acpi_parse_entries(ACPI_SIG_MADT, > sizeof(struct acpi_table_madt), > - gic_acpi_parse_madt_distributor, table, > - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); > + table, &gic_proc, 1, 0); > if (count <= 0) { > pr_err("No valid GICD entries exist\n"); > return -EINVAL; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index d2445fa..59b17e8 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) > (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ > ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) > > +struct acpi_subtable_proc { > + int id; > + acpi_tbl_entry_handler handler; > + int count; > +}; > + > char * __acpi_map_table (unsigned long phys_addr, unsigned long size); > void __acpi_unmap_table(char *map, unsigned long size); > int early_acpi_boot_init(void); > @@ -145,10 +151,13 @@ int acpi_numa_init (void); > > int acpi_table_init (void); > int acpi_table_parse(char *id, acpi_tbl_table_handler handler); > +int acpi_table_parse_entries_array(char *id, unsigned long table_size, > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int __init acpi_parse_entries(char *id, unsigned long table_size, > - acpi_tbl_entry_handler handler, > struct acpi_table_header *table_header, > - int entry_id, unsigned int max_entries); > + struct acpi_subtable_proc *proc, int proc_num, > + unsigned int max_entries); > int __init acpi_table_parse_entries(char *id, unsigned long table_size, > int entry_id, > acpi_tbl_entry_handler handler, > You rename acpi_table_parse_entries -> acpi_table_parse_entries_array, should acpi_table_parse_entries suppose to be removed above? Tomasz ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT @ 2015-07-13 13:54 Lukasz Anaczkowski 2015-07-13 15:22 ` Hanjun Guo 0 siblings, 1 reply; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-07-13 13:54 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563. In case when BIOS is populating MADT wiht both x2apic and local apic entries (as per ACPI spec), kernel builds it's processor table in the following order: BSP, X2APIC, local APIC, resulting in processors on the same core are not separated by core count. This patch fixes this behavior and resulting assignment is correct. Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- drivers/acpi/numa.c | 28 ++++++++++++----- drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- drivers/irqchip/irq-gic.c | 15 ++++++--- include/linux/acpi.h | 13 ++++++-- 5 files changed, 111 insertions(+), 49 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..1bb79d5 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + count = madt_proc[0].count; + x2count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) return count; } - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, - acpi_parse_x2apic_nmi, 0); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, - acpi_parse_lapic_nmi, 0); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; + madt_proc[0].handler = acpi_parse_lapic_nmi; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; + madt_proc[1].handler = acpi_parse_x2apic_nmi; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), 0); + count = madt_proc[0].count; + x2count = madt_proc[1].count; if (count < 0 || x2count < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); /* TBD: Cleanup to allow fallback to MPS */ diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c index acaa3b4..2bdff0c 100644 --- a/drivers/acpi/numa.c +++ b/drivers/acpi/numa.c @@ -314,9 +314,15 @@ static int __init acpi_table_parse_srat(enum acpi_srat_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_SRAT, - sizeof(struct acpi_table_srat), id, - handler, max_entries); + struct acpi_subtable_proc srat_proc; + + memset(&srat_proc, 0, sizeof(srat_proc)); + srat_proc.id = id; + srat_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + &srat_proc, 1, max_entries); } int __init acpi_numa_init(void) @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) /* SRAT: Static Resource Affinity Table */ if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, - acpi_parse_x2apic_affinity, 0); - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, - acpi_parse_processor_affinity, 0); + struct acpi_subtable_proc srat_proc[2]; + + memset(srat_proc, 0, sizeof(srat_proc)); + srat_proc[0].id = ACPI_SRAT_TYPE_CPU_AFFINITY; + srat_proc[0].handler = acpi_parse_processor_affinity; + srat_proc[1].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; + srat_proc[1].handler = acpi_parse_x2apic_affinity; + + acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + srat_proc, ARRAY_SIZE(srat_proc), 0); + cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..1217e41 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -216,25 +216,27 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; int count = 0; unsigned long table_end; + int i; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; - - if (!table_size) + } + if (!table_size) { + proc[0].count = -EINVAL; return -EINVAL; + } if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } @@ -247,12 +249,17 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (max_entries && count++ >= max_entries) + continue; + if (proc[i].handler(entry, table_end)) { + proc[i].count = -EINVAL; return -EINVAL; - - count++; + } + proc[i].count++; + break; } /* @@ -260,7 +267,11 @@ acpi_parse_entries(char *id, unsigned long table_size, * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] Invalid zero length\n"); + proc[0].count = -EINVAL; return -EINVAL; } @@ -269,18 +280,20 @@ acpi_parse_entries(char *id, unsigned long table_size, } if (max_entries && count > max_entries) { - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] ignored %i entries of %i found\n", + count-max_entries, count); } return count; } int __init -acpi_table_parse_entries(char *id, +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -288,11 +301,10 @@ acpi_table_parse_entries(char *id, int count; u32 instance = 0; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; + } if (!strncmp(id, ACPI_SIG_MADT, 4)) instance = acpi_apic_instance; @@ -300,11 +312,12 @@ acpi_table_parse_entries(char *id, acpi_get_table_with_size(id, instance, &table_header, &tbl_size); if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; @@ -314,9 +327,15 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), id, - handler, max_entries); + struct acpi_subtable_proc madt_proc; + + memset(&madt_proc, 0, sizeof(madt_proc)); + madt_proc.id = id; + madt_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + &madt_proc, 1, max_entries); } /** diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4dd8826..d004a32 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) { void __iomem *cpu_base, *dist_base; int count; + struct acpi_subtable_proc gic_proc; + + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; + gic_proc.handler = gic_acpi_parse_madt_cpu; /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_cpu, table, - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); + table, gic_proc, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) * Find distributor base address. We expect one distributor entry since * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. */ + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; + gic_proc.handler = gic_acpi_parse_madt_distributor; + count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_distributor, table, - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); + table, gic_proc, 0); if (count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..59b17e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -145,10 +151,13 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); +int acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries); + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, -- 1.8.3.1 -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited. ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT 2015-07-13 13:54 [PATCH] x86, acpi: Handle xapic/x2apic " Lukasz Anaczkowski @ 2015-07-13 15:22 ` Hanjun Guo 2015-07-14 8:06 ` Lukasz Anaczkowski 0 siblings, 1 reply; 52+ messages in thread From: Hanjun Guo @ 2015-07-13 15:22 UTC (permalink / raw) To: Lukasz Anaczkowski, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi Hi Lukasz, On 07/13/2015 09:54 PM, Lukasz Anaczkowski wrote: > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563. > > In case when BIOS is populating MADT wiht both x2apic and local apic > entries (as per ACPI spec), kernel builds it's processor table > in the following order: BSP, X2APIC, local APIC, resulting in > processors on the same core are not separated by core count. I'm confused here, I can't figure out where is the problem, could you explain it in detail? For me, logical CPU IDs allocated by OS will have mappings to physical CPU IDs, what you are saying seems CPU topology problem to me. > > This patch fixes this behavior and resulting assignment is > correct. > > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> > --- > arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- > drivers/acpi/numa.c | 28 ++++++++++++----- > drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- > drivers/irqchip/irq-gic.c | 15 ++++++--- Anyway, You also miss the SMP boot for ARM64 in ACPI way, which will cause compile error on ARM64. Thanks Hanjun ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT 2015-07-13 15:22 ` Hanjun Guo @ 2015-07-14 8:06 ` Lukasz Anaczkowski 2015-07-14 8:06 ` Lukasz Anaczkowski 2015-07-14 14:24 ` Hanjun Guo 0 siblings, 2 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-07-14 8:06 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On 07/13/2015 23:22 PM, Hanjun Guo wrote: >> In case when BIOS is populating MADT wiht both x2apic and local apic >> entries (as per ACPI spec), kernel builds it's processor table >> in the following order: BSP, X2APIC, local APIC, resulting in >> processors on the same core are not separated by core count. > > I'm confused here, I can't figure out where is the problem, could you > explain it in detail? > > For me, logical CPU IDs allocated by OS will have mappings to physical > CPU IDs, what you are saying seems CPU topology problem to me. Sorry for confusion, I'll send another patch with updated commit message where I'll try to put some more details and explanation. Hope that one will be less confusing. >> arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- >> drivers/acpi/numa.c | 28 ++++++++++++----- >> drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- >> drivers/irqchip/irq-gic.c | 15 ++++++--- > > Anyway, You also miss the SMP boot for ARM64 in ACPI way, which will > cause compile error on ARM64. The only symbol that's changed is acpi_parse_entries and I don't see any calls to it in ARM64. I also tried compilation of arm64/defconfig and it compiles fine. Could yoy point me to place where it might failing? Thanks, Lukasz -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited. ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT 2015-07-14 8:06 ` Lukasz Anaczkowski @ 2015-07-14 8:06 ` Lukasz Anaczkowski 2015-07-14 8:26 ` Jiang Liu 2015-07-21 8:27 ` Thomas Gleixner 2015-07-14 14:24 ` Hanjun Guo 1 sibling, 2 replies; 52+ messages in thread From: Lukasz Anaczkowski @ 2015-07-14 8:06 UTC (permalink / raw) To: tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi, Lukasz Anaczkowski This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" previously published at https://lkml.org/lkml/2013/1/21/563. In case when BIOS is populating MADT wiht both x2apic and local apic entries (as per ACPI spec), e.g. for Xeon Phi Knights Landing, kernel builds it's processor table in the following order: BSP, X2APIC, local APIC, resulting in processors on the same core are not separated by core count, i.e. Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId 0 0 ( 0 [0000]), 97 ( 1 [0001]), 145 ( 2 [0002]), 193 ( 3 [0003]) 1 50 ( 4 [0004]), 98 ( 5 [0005]), 146 ( 6 [0006]), 194 ( 7 [0007]) 2 51 ( 16 [0010]), 99 ( 17 [0011]), 147 ( 18 [0012]), 195 ( 19 [0013]) 3 52 ( 20 [0014]), 100 ( 21 [0015]), 148 ( 22 [0016]), 196 ( 23 [0017]) 4 53 ( 24 [0018]), 101 ( 25 [0019]), 149 ( 26 [001a]), 197 ( 27 [001b]) 5 54 ( 28 [001c]), 102 ( 29 [001d]), 150 ( 30 [001e]), 198 ( 31 [001f]) ... Please note, how LCpu are mixed for physical cores (Core). This patch fixes this behavior and resulting assignment is consistent with other Xeon processors, i.e. Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId 0 0 ( 0 [0000]), 72 ( 1 [0001]), 144 ( 2 [0002]), 216 ( 3 [0003]) 1 1 ( 4 [0004]), 73 ( 5 [0005]), 145 ( 6 [0006]), 217 ( 7 [0007]) 2 2 ( 8 [0008]), 74 ( 9 [0009]), 146 ( 10 [000a]), 218 ( 11 [000b]) 3 3 ( 12 [000c]), 75 ( 13 [000d]), 147 ( 14 [000e]), 219 ( 15 [000f]) 4 4 ( 16 [0010]), 76 ( 17 [0011]), 148 ( 18 [0012]), 220 ( 19 [0013]) 5 5 ( 20 [0014]), 77 ( 21 [0015]), 149 ( 22 [0016]), 221 ( 23 [0017]) ... Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> --- arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- drivers/acpi/numa.c | 28 ++++++++++++----- drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- drivers/irqchip/irq-gic.c | 15 ++++++--- include/linux/acpi.h | 13 ++++++-- 5 files changed, 111 insertions(+), 49 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..1bb79d5 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + count = madt_proc[0].count; + x2count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) return count; } - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, - acpi_parse_x2apic_nmi, 0); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, - acpi_parse_lapic_nmi, 0); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; + madt_proc[0].handler = acpi_parse_lapic_nmi; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; + madt_proc[1].handler = acpi_parse_x2apic_nmi; + acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), 0); + count = madt_proc[0].count; + x2count = madt_proc[1].count; if (count < 0 || x2count < 0) { printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n"); /* TBD: Cleanup to allow fallback to MPS */ diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c index acaa3b4..2bdff0c 100644 --- a/drivers/acpi/numa.c +++ b/drivers/acpi/numa.c @@ -314,9 +314,15 @@ static int __init acpi_table_parse_srat(enum acpi_srat_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_SRAT, - sizeof(struct acpi_table_srat), id, - handler, max_entries); + struct acpi_subtable_proc srat_proc; + + memset(&srat_proc, 0, sizeof(srat_proc)); + srat_proc.id = id; + srat_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + &srat_proc, 1, max_entries); } int __init acpi_numa_init(void) @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) /* SRAT: Static Resource Affinity Table */ if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, - acpi_parse_x2apic_affinity, 0); - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, - acpi_parse_processor_affinity, 0); + struct acpi_subtable_proc srat_proc[2]; + + memset(srat_proc, 0, sizeof(srat_proc)); + srat_proc[0].id = ACPI_SRAT_TYPE_CPU_AFFINITY; + srat_proc[0].handler = acpi_parse_processor_affinity; + srat_proc[1].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; + srat_proc[1].handler = acpi_parse_x2apic_affinity; + + acpi_table_parse_entries_array(ACPI_SIG_SRAT, + sizeof(struct acpi_table_srat), + srat_proc, ARRAY_SIZE(srat_proc), 0); + cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 2e19189..1217e41 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -216,25 +216,27 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; int count = 0; unsigned long table_end; + int i; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; - - if (!table_size) + } + if (!table_size) { + proc[0].count = -EINVAL; return -EINVAL; + } if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } @@ -247,12 +249,17 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (max_entries && count++ >= max_entries) + continue; + if (proc[i].handler(entry, table_end)) { + proc[i].count = -EINVAL; return -EINVAL; - - count++; + } + proc[i].count++; + break; } /* @@ -260,7 +267,11 @@ acpi_parse_entries(char *id, unsigned long table_size, * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] Invalid zero length\n"); + proc[0].count = -EINVAL; return -EINVAL; } @@ -269,18 +280,20 @@ acpi_parse_entries(char *id, unsigned long table_size, } if (max_entries && count > max_entries) { - pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + pr_warn("[%4.4s:0x%02x ", id, proc[0].id); + for (i = 1; i < proc_num; i++) + pr_cont(" 0x%02x", proc[i].id); + pr_cont("] ignored %i entries of %i found\n", + count-max_entries, count); } return count; } int __init -acpi_table_parse_entries(char *id, +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -288,11 +301,10 @@ acpi_table_parse_entries(char *id, int count; u32 instance = 0; - if (acpi_disabled) + if (acpi_disabled) { + proc[0].count = -ENODEV; return -ENODEV; - - if (!id || !handler) - return -EINVAL; + } if (!strncmp(id, ACPI_SIG_MADT, 4)) instance = acpi_apic_instance; @@ -300,11 +312,12 @@ acpi_table_parse_entries(char *id, acpi_get_table_with_size(id, instance, &table_header, &tbl_size); if (!table_header) { pr_warn("%4.4s not present\n", id); + proc[0].count = -ENODEV; return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; @@ -314,9 +327,15 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) { - return acpi_table_parse_entries(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), id, - handler, max_entries); + struct acpi_subtable_proc madt_proc; + + memset(&madt_proc, 0, sizeof(madt_proc)); + madt_proc.id = id; + madt_proc.handler = handler; + + return acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + &madt_proc, 1, max_entries); } /** diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4dd8826..d004a32 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table) { void __iomem *cpu_base, *dist_base; int count; + struct acpi_subtable_proc gic_proc; + + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT; + gic_proc.handler = gic_acpi_parse_madt_cpu; /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_cpu, table, - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); + table, gic_proc, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table) * Find distributor base address. We expect one distributor entry since * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. */ + memset(gic_proc, 0, sizeof(gic_proc)); + gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR; + gic_proc.handler = gic_acpi_parse_madt_distributor; + count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_distributor, table, - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); + table, gic_proc, 0); if (count <= 0) { pr_err("No valid GICD entries exist\n"); return -EINVAL; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d2445fa..59b17e8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -145,10 +151,13 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); +int acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries); + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, int entry_id, acpi_tbl_entry_handler handler, -- 1.8.3.1 -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited. ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT 2015-07-14 8:06 ` Lukasz Anaczkowski @ 2015-07-14 8:26 ` Jiang Liu 2015-07-14 11:17 ` Anaczkowski, Lukasz 2015-07-21 8:27 ` Thomas Gleixner 1 sibling, 1 reply; 52+ messages in thread From: Jiang Liu @ 2015-07-14 8:26 UTC (permalink / raw) To: Lukasz Anaczkowski, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On 2015/7/14 16:06, Lukasz Anaczkowski wrote: > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563. > > In case when BIOS is populating MADT wiht both x2apic and local apic > entries (as per ACPI spec), e.g. for Xeon Phi Knights Landing, > kernel builds it's processor table in the following order: > BSP, X2APIC, local APIC, resulting in processors on the same core > are not separated by core count, i.e. > > Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId > 0 0 ( 0 [0000]), 97 ( 1 [0001]), 145 ( 2 [0002]), 193 ( 3 [0003]) > 1 50 ( 4 [0004]), 98 ( 5 [0005]), 146 ( 6 [0006]), 194 ( 7 [0007]) > 2 51 ( 16 [0010]), 99 ( 17 [0011]), 147 ( 18 [0012]), 195 ( 19 [0013]) > 3 52 ( 20 [0014]), 100 ( 21 [0015]), 148 ( 22 [0016]), 196 ( 23 [0017]) > 4 53 ( 24 [0018]), 101 ( 25 [0019]), 149 ( 26 [001a]), 197 ( 27 [001b]) > 5 54 ( 28 [001c]), 102 ( 29 [001d]), 150 ( 30 [001e]), 198 ( 31 [001f]) > ... > > Please note, how LCpu are mixed for physical cores (Core). > > This patch fixes this behavior and resulting assignment is > consistent with other Xeon processors, i.e. > > Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId > 0 0 ( 0 [0000]), 72 ( 1 [0001]), 144 ( 2 [0002]), 216 ( 3 [0003]) > 1 1 ( 4 [0004]), 73 ( 5 [0005]), 145 ( 6 [0006]), 217 ( 7 [0007]) > 2 2 ( 8 [0008]), 74 ( 9 [0009]), 146 ( 10 [000a]), 218 ( 11 [000b]) > 3 3 ( 12 [000c]), 75 ( 13 [000d]), 147 ( 14 [000e]), 219 ( 15 [000f]) > 4 4 ( 16 [0010]), 76 ( 17 [0011]), 148 ( 18 [0012]), 220 ( 19 [0013]) > 5 5 ( 20 [0014]), 77 ( 21 [0015]), 149 ( 22 [0016]), 221 ( 23 [0017]) > ... > > Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> Hi Lukasz, I have some concerns here about "maxcpus" and "nox2apic" kernel parameters. Say "maxcpus=72 nox2apic" is specified, user may get less than 72 CPUs with you patch applied. Original code will try to only all xapic CPUs before trying x2apic CPUs, so "maxcpus" doesn't conflict with "nox2apic". Thanks! Gerry ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT 2015-07-14 8:26 ` Jiang Liu @ 2015-07-14 11:17 ` Anaczkowski, Lukasz 0 siblings, 0 replies; 52+ messages in thread From: Anaczkowski, Lukasz @ 2015-07-14 11:17 UTC (permalink / raw) To: Jiang Liu, tglx, mingo, hpa, x86, jason Cc: rjw, Brown, Len, pavel, linux-pm, linux-kernel, linux-acpi > I have some concerns here about "maxcpus" and "nox2apic" kernel parameters. Say "maxcpus=72 nox2apic" is specified, user may get less than 72 CPUs with you patch applied. Original code will try to only all xapic CPUs before trying x2apic CPUs, so "maxcpus" doesn't conflict with "nox2apic". HI Gerry, 'nox2apic' is causing kernel panics with and w/o my patch. Here's the stack trace: #0 panic (fmt=0xffffffff818043c8 "timer doesn't work through Interrupt-remapped IO-APIC") at kernel/panic.c:72 #1 0xffffffff813df48c in panic_if_irq_remap (msg=<optimized out>) at drivers/iommu/irq_remapping.c:152 #2 0xffffffff81b5a705 in check_timer () at arch/x86/kernel/apic/io_apic.c:2124 #3 0xffffffff81b5afa3 in setup_IO_APIC () at arch/x86/kernel/apic/io_apic.c:2278 #4 0xffffffff81b59655 in apic_bsp_setup (upmode=<optimized out>) at arch/x86/kernel/apic/apic.c:2184 #5 0xffffffff81b57370 in native_smp_prepare_cpus (max_cpus=<optimized out>) at arch/x86/kernel/smpboot.c:1201 #6 0xffffffff81b49182 in smp_prepare_cpus (max_cpus=<optimized out>) at ./arch/x86/include/asm/smp.h:99 #7 kernel_init_freeable () at init/main.c:999 #8 0xffffffff8155c64e in kernel_init (unused=<optimized out>) at init/main.c:937 #9 0xffffffff8156f51f in ret_from_fork () at arch/x86/entry/entry_64.S:526 #10 0x0000000000000000 in ?? () On HSW i7-4770, I'm getting 8 CPUs booted, regardless of the 'nox2apic' setting w/ my patch. Cheers, Lukasz -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT 2015-07-14 8:06 ` Lukasz Anaczkowski 2015-07-14 8:26 ` Jiang Liu @ 2015-07-21 8:27 ` Thomas Gleixner 1 sibling, 0 replies; 52+ messages in thread From: Thomas Gleixner @ 2015-07-21 8:27 UTC (permalink / raw) To: Lukasz Anaczkowski Cc: mingo, hpa, x86, jason, rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On Tue, 14 Jul 2015, Lukasz Anaczkowski wrote: > This patch is based on work of "Yinghai Lu <yinghai@kernel.org>" > previously published at https://lkml.org/lkml/2013/1/21/563. > > In case when BIOS is populating MADT wiht both x2apic and local apic > entries (as per ACPI spec), e.g. for Xeon Phi Knights Landing, > kernel builds it's processor table in the following order: > BSP, X2APIC, local APIC, resulting in processors on the same core > are not separated by core count, i.e. You are missing to explain WHY this is the wrong ordering. > Core LCpu ApicId LCpu ApicId LCpu ApicId LCpu ApicId > 0 0 ( 0 [0000]), 97 ( 1 [0001]), 145 ( 2 [0002]), 193 ( 3 [0003]) > 1 50 ( 4 [0004]), 98 ( 5 [0005]), 146 ( 6 [0006]), 194 ( 7 [0007]) > 2 51 ( 16 [0010]), 99 ( 17 [0011]), 147 ( 18 [0012]), 195 ( 19 [0013]) > 3 52 ( 20 [0014]), 100 ( 21 [0015]), 148 ( 22 [0016]), 196 ( 23 [0017]) > 4 53 ( 24 [0018]), 101 ( 25 [0019]), 149 ( 26 [001a]), 197 ( 27 [001b]) > 5 54 ( 28 [001c]), 102 ( 29 [001d]), 150 ( 30 [001e]), 198 ( 31 [001f]) > ... > > Please note, how LCpu are mixed for physical cores (Core). > > This patch fixes this behavior and resulting assignment is > consistent with other Xeon processors, i.e. You are missing to explain HOW you fix it. It's completely non obvious why the conversion to an parse array makes it work. > if (!count) { > - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, > - acpi_parse_x2apic, MAX_LOCAL_APIC); > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, > - acpi_parse_lapic, MAX_LOCAL_APIC); > + memset(madt_proc, 0, sizeof(madt_proc)); > + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; > + madt_proc[0].handler = acpi_parse_lapic; > + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; > + madt_proc[1].handler = acpi_parse_x2apic; Here you revert the parse order. > + acpi_table_parse_entries_array(ACPI_SIG_MADT, > + sizeof(struct acpi_table_madt), > + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); > + count = madt_proc[0].count; > + x2count = madt_proc[1].count; > } > if (!count && !x2count) { > printk(KERN_ERR PREFIX "No LAPIC entries present\n"); > @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void) > return count; > } > > - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI, > - acpi_parse_x2apic_nmi, 0); > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI, > - acpi_parse_lapic_nmi, 0); > + memset(madt_proc, 0, sizeof(madt_proc)); > + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI; > + madt_proc[0].handler = acpi_parse_lapic_nmi; > + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI; > + madt_proc[1].handler = acpi_parse_x2apic_nmi; Ditto > int __init acpi_numa_init(void) > @@ -331,10 +337,18 @@ int __init acpi_numa_init(void) > > /* SRAT: Static Resource Affinity Table */ > if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { > - acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY, > - acpi_parse_x2apic_affinity, 0); > - acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY, > - acpi_parse_processor_affinity, 0); > + struct acpi_subtable_proc srat_proc[2]; > + > + memset(srat_proc, 0, sizeof(srat_proc)); > + srat_proc[0].id = ACPI_SRAT_TYPE_CPU_AFFINITY; > + srat_proc[0].handler = acpi_parse_processor_affinity; > + srat_proc[1].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY; > + srat_proc[1].handler = acpi_parse_x2apic_affinity; Once more. Please add proper explanations why the array parser is required and why the parse order needs to be reverse. Thanks, tglx ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT 2015-07-14 8:06 ` Lukasz Anaczkowski 2015-07-14 8:06 ` Lukasz Anaczkowski @ 2015-07-14 14:24 ` Hanjun Guo 1 sibling, 0 replies; 52+ messages in thread From: Hanjun Guo @ 2015-07-14 14:24 UTC (permalink / raw) To: Lukasz Anaczkowski, tglx, mingo, hpa, x86, jason Cc: rjw, len.brown, pavel, linux-pm, linux-kernel, linux-acpi On 07/14/2015 04:06 PM, Lukasz Anaczkowski wrote: > On 07/13/2015 23:22 PM, Hanjun Guo wrote: >>> In case when BIOS is populating MADT wiht both x2apic and local apic >>> entries (as per ACPI spec), kernel builds it's processor table >>> in the following order: BSP, X2APIC, local APIC, resulting in >>> processors on the same core are not separated by core count. >> >> I'm confused here, I can't figure out where is the problem, could you >> explain it in detail? >> >> For me, logical CPU IDs allocated by OS will have mappings to physical >> CPU IDs, what you are saying seems CPU topology problem to me. > > Sorry for confusion, I'll send another patch with updated commit message > where I'll try to put some more details and explanation. Hope that one will > be less confusing. > >>> arch/x86/kernel/acpi/boot.c | 29 +++++++++++++----- >>> drivers/acpi/numa.c | 28 ++++++++++++----- >>> drivers/acpi/tables.c | 75 ++++++++++++++++++++++++++++----------------- >>> drivers/irqchip/irq-gic.c | 15 ++++++--- >> >> Anyway, You also miss the SMP boot for ARM64 in ACPI way, which will >> cause compile error on ARM64. > > The only symbol that's changed is acpi_parse_entries and I don't see > any calls to it in ARM64. I also tried compilation of arm64/defconfig > and it compiles fine. Could yoy point me to place where it might failing? Sorry, I misread your patch that you modified acpi_table_parse_madt() also, but it turns out that you keep it as before, sorry for the noise. Thanks Hanjun ^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2015-09-18 22:10 UTC | newest] Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <alpine.DEB.2.11.1507211017590.18576 () nanos> 2015-07-30 17:43 ` [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT Lukasz Anaczkowski 2015-07-30 17:43 ` [PATCH] x86, acpi: Handle lapic/x2apic " Lukasz Anaczkowski 2015-08-02 9:57 ` Thomas Gleixner 2015-08-02 12:40 ` Marc Zyngier 2015-08-03 18:26 ` Lukasz Anaczkowski 2015-08-03 18:26 ` Lukasz Anaczkowski 2015-08-26 7:04 ` Anaczkowski, Lukasz 2015-08-26 10:43 ` Marc Zyngier 2015-08-26 11:42 ` Lorenzo Pieralisi 2015-08-26 12:43 ` Marc Zyngier 2015-08-26 17:49 ` Lukasz Anaczkowski 2015-08-26 17:49 ` [PATCH] x86, arm64, " Lukasz Anaczkowski 2015-08-27 9:37 ` Lorenzo Pieralisi 2015-09-08 11:07 ` Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 1/4] acpi: rename acpi_table_parse_entries Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 2/4] x86, arm64, acpi: Added acpi_subtable_proc Lukasz Anaczkowski 2015-09-08 11:07 ` [PATCH 3/4] acpi: multi proc support Lukasz Anaczkowski 2015-09-08 11:08 ` [PATCH 4/4] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski 2015-09-08 15:22 ` Marc Zyngier 2015-09-08 16:27 ` [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs Marc Zyngier 2015-09-08 22:45 ` Al Stone 2015-09-09 7:01 ` Anaczkowski, Lukasz 2015-09-09 9:30 ` [PATCH 0/2] " Lukasz Anaczkowski 2015-09-09 9:30 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski 2015-09-09 9:30 ` [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski 2015-09-09 13:56 ` Lorenzo Pieralisi 2015-09-09 14:27 ` Anaczkowski, Lukasz 2015-09-09 15:43 ` Lorenzo Pieralisi 2015-09-09 10:47 ` [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Marc Zyngier 2015-09-09 13:47 ` Lukasz Anaczkowski 2015-09-09 13:47 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Lukasz Anaczkowski 2015-09-09 13:47 ` [PATCH v4 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers Lukasz Anaczkowski 2015-09-09 13:47 ` [PATCH v4 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order Lukasz Anaczkowski 2015-09-09 20:45 ` [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs Rafael J. Wysocki 2015-09-18 22:38 ` Rafael J. Wysocki 2015-08-28 8:30 ` [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Ingo Molnar 2015-09-01 8:02 ` Tomasz Nowicki 2015-09-01 12:07 ` Anaczkowski, Lukasz 2015-09-01 13:36 ` Tomasz Nowicki 2015-09-07 14:04 ` Anaczkowski, Lukasz 2015-09-08 14:44 ` Tomasz Nowicki 2015-08-26 11:03 ` [PATCH] x86, " Marc Zyngier 2015-08-26 12:56 ` Tomasz Nowicki 2015-07-13 13:54 [PATCH] x86, acpi: Handle xapic/x2apic " Lukasz Anaczkowski 2015-07-13 15:22 ` Hanjun Guo 2015-07-14 8:06 ` Lukasz Anaczkowski 2015-07-14 8:06 ` Lukasz Anaczkowski 2015-07-14 8:26 ` Jiang Liu 2015-07-14 11:17 ` Anaczkowski, Lukasz 2015-07-21 8:27 ` Thomas Gleixner 2015-07-14 14:24 ` Hanjun Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).