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