| Message ID | 1440611369-26314-2-git-send-email-lukasz.anaczkowski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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/ > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)