Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT

Re: [PATCH] x86, acpi: Handle xapic/x2apic entries in MADT

[Lukasz Anaczkowski]

> 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

[PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Lukasz Anaczkowski]

>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

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Thomas Gleixner]

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>

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Marc Zyngier]

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.

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Lukasz Anaczkowski]

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

[PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Lukasz Anaczkowski]

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

RE: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Anaczkowski, Lukasz]

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

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Marc Zyngier]

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...

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Marc Zyngier]

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.

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Lorenzo Pieralisi]

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

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Marc Zyngier]

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...

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Tomasz Nowicki]

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

Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT

[Lukasz Anaczkowski]

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

[PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Lukasz Anaczkowski]

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

Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Lorenzo Pieralisi]

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/
> 

Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Ingo Molnar]

* 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

Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Tomasz Nowicki]

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,
>

RE: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Anaczkowski, Lukasz]

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

Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Tomasz Nowicki]

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

RE: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Anaczkowski, Lukasz]

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

Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Lukasz Anaczkowski]

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

[PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs

[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

[PATCH 1/4] acpi: rename acpi_table_parse_entries

[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

[PATCH 2/4] x86, arm64, acpi: Added acpi_subtable_proc

[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

[PATCH 3/4] acpi: multi proc support

[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

[PATCH 4/4] x86, acpi: Handle apic/x2apic entries in MADT in correct order

[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

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

Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT

[Tomasz Nowicki]

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

Re: [PATCH 4/4] x86, acpi: Handle apic/x2apic entries in MADT in correct order

[Marc Zyngier]

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...

Re: [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs

[Marc Zyngier]

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...

Re: [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs

[Al Stone]

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
-----------------------------------

RE: [PATCH 0/4] Fix how CPUs are enumerated when there's more than 255 CPUs

[Anaczkowski, Lukasz]

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

[PATCH 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs

[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

[PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers

[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

[PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order

[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

Re: [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers

[Marc Zyngier]

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...

Re: [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers

[Lukasz Anaczkowski]

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

[PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs

[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

[PATCH v4 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers

[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

[PATCH v4 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order

[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

Re: [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order

[Lorenzo Pieralisi]

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
> 

RE: [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order

[Anaczkowski, Lukasz]

-----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

Re: [PATCH 2/2] x86, acpi: Handle apic/x2apic entries in MADT in correct order

[Lorenzo Pieralisi]

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

Re: [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs

[Rafael J. Wysocki]

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

Re: [PATCH v4 0/2] Fix how CPUs are enumerated when there's more than 255 CPUs

[Rafael J. Wysocki]

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