Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1213596 > unrolled thread

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

Started by"Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com>
First post2015-08-26 09:10 +0200
Last post2015-08-26 19:50 +0200
Articles 11 — 6 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT "Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com> - 2015-08-26 09:10 +0200
    Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT Marc Zyngier <marc.zyngier@arm.com> - 2015-08-26 12:50 +0200
      Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2015-08-26 13:50 +0200
        Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT Marc Zyngier <marc.zyngier@arm.com> - 2015-08-26 14:50 +0200
          [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> - 2015-08-26 19:50 +0200
            Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2015-08-27 11:40 +0200
            Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Ingo Molnar <mingo@kernel.org> - 2015-08-28 10:40 +0200
            Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Tomasz Nowicki <tomasz.nowicki@linaro.org> - 2015-09-01 10:10 +0200
              RE: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT "Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com> - 2015-09-01 14:10 +0200
                Re: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT Tomasz Nowicki <tomasz.nowicki@linaro.org> - 2015-09-01 15:40 +0200
          Re: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> - 2015-08-26 19:50 +0200

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

From"Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com>
Date2015-08-26 09:10 +0200
SubjectRE: [PATCH] x86, acpi: Handle lapic/x2apic entries in MADT
Message-ID<q1D0m-71b-37@gated-at.bofh.it>
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
--
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/

[toc] | [next] | [standalone]

#1213735

FromMarc Zyngier <marc.zyngier@arm.com>
Date2015-08-26 12:50 +0200
Message-ID<q1Grg-3jH-25@gated-at.bofh.it>
In reply to#1213596
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...
--
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/

[toc] | [prev] | [next] | [standalone]

#1213763

FromLorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Date2015-08-26 13:50 +0200
Message-ID<q1Hnk-4Eu-13@gated-at.bofh.it>
In reply to#1213735
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

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

[toc] | [prev] | [next] | [standalone]

#1213827

FromMarc Zyngier <marc.zyngier@arm.com>
Date2015-08-26 14:50 +0200
Message-ID<q1Ijo-602-27@gated-at.bofh.it>
In reply to#1213763
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...
--
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/

[toc] | [prev] | [next] | [standalone]

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

FromLukasz Anaczkowski <lukasz.anaczkowski@intel.com>
Date2015-08-26 19:50 +0200
Subject[PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT
Message-ID<q1MZH-4is-5@gated-at.bofh.it>
In reply to#1213827
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

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

[toc] | [prev] | [next] | [standalone]

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

FromLorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Date2015-08-27 11:40 +0200
SubjectRe: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT
Message-ID<q21P3-v3-3@gated-at.bofh.it>
In reply to#1214074
On Wed, Aug 26, 2015 at 06:49:29PM +0100, Lukasz Anaczkowski wrote:

[...]

> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 2e19189..d5c9a1b 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -214,27 +214,45 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>         }
>  }
> 
> +/**
> + * acpi_table_parse - for each proc_num find a subtable with proc->id

Name of function does not correspond to the function you are actually
commenting.

> + *    and run proc->handler on it. Assumption is that there's only
> + *    single handler for particular id.
> + *
> + * @id: table id (for debugging purposes)
> + * @table_size: single entry size
> + * @table_header: where does the table start?
> + * @proc: array of acpi_subtable_proc struct containing subtable id
> + *        and associated handler with it
> + * @proc_num: how big proc is?
> + * @max_entries: how many entries can we process?
> + *
> + * On success returns sum of all matching entries for all proc handlers.
> + * Oterwise, -ENODEV or -EINVAL is returned.

s/Oterwise/Otherwise

> + */
>  int __init
>  acpi_parse_entries(char *id, unsigned long table_size,
> -               acpi_tbl_entry_handler handler,
>                 struct acpi_table_header *table_header,
> -               int entry_id, unsigned int max_entries)
> +               struct acpi_subtable_proc *proc, int proc_num,
> +               unsigned int max_entries)
>  {
>         struct acpi_subtable_header *entry;
>         int count = 0;
>         unsigned long table_end;
> +       int i;
> 
> -       if (acpi_disabled)
> +       if (acpi_disabled) {
> +               proc[0].count = -ENODEV;
>                 return -ENODEV;
> -
> -       if (!id || !handler)
> -               return -EINVAL;
> -
> -       if (!table_size)
> +       }

Add a space please

> +       if (!table_size) {
> +               proc[0].count = -EINVAL;

This is misleading. Why do we want to return error only in the first entry ?
If I get the function logic right, if this function returns an error
somehow the parsing failed, so the proc array content must be considered
invalid. Ergo, just returning an error code should be enough and
that's what you should check first in eg acpi_parse_madt_lapic_entries().

Comment is valid for the whole patch.

>                 return -EINVAL;
> +       }
> 
>         if (!table_header) {
>                 pr_warn("%4.4s not present\n", id);
> +               proc[0].count = -ENODEV;
>                 return -ENODEV;
>         }
> 
> @@ -247,20 +265,31 @@ acpi_parse_entries(char *id, unsigned long table_size,
> 
>         while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>                table_end) {
> -               if (entry->type == entry_id
> -                   && (!max_entries || count < max_entries)) {
> -                       if (handler(entry, table_end))
> +               if (max_entries && count >= max_entries)
> +                       continue;
> +               for (i = 0; i < proc_num; i++) {
> +                       if (entry->type != proc[i].id)
> +                               continue;
> +                       if (proc[i].handler(entry, table_end)) {
> +                               proc[i].count = -EINVAL;
>                                 return -EINVAL;
> -
> -                       count++;
> +                       }
> +                       proc[i].count++;
> +                       break;
>                 }
> +               if (i != proc_num)
> +                       count++;
> 
>                 /*
>                  * If entry->length is 0, break from this loop to avoid
>                  * infinite loop.
>                  */
>                 if (entry->length == 0) {
> -                       pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
> +                       pr_err("[%4.4s:0x%02x ", id, proc[0].id);
> +                       for (i = 1; i < proc_num; i++)
> +                               pr_cont(" 0x%02x", proc[i].id);
> +                       pr_cont("] Invalid zero length\n");
> +                       proc[0].count = -EINVAL;
>                         return -EINVAL;
>                 }
> 
> @@ -269,18 +298,20 @@ acpi_parse_entries(char *id, unsigned long table_size,
>         }
> 
>         if (max_entries && count > max_entries) {
> -               pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
> -                       id, entry_id, count - max_entries, count);
> +               pr_warn("[%4.4s:0x%02x ", id, proc[0].id);
> +               for (i = 1; i < proc_num; i++)
> +                       pr_cont(" 0x%02x", proc[i].id);
> +               pr_cont("] ignored %i entries of %i found\n",
> +                       count-max_entries, count);
>         }
> 
>         return count;
>  }
> 
>  int __init
> -acpi_table_parse_entries(char *id,
> +acpi_table_parse_entries_array(char *id,
>                          unsigned long table_size,
> -                        int entry_id,
> -                        acpi_tbl_entry_handler handler,
> +                        struct acpi_subtable_proc *proc, int proc_num,
>                          unsigned int max_entries)
>  {
>         struct acpi_table_header *table_header = NULL;
> @@ -288,11 +319,10 @@ acpi_table_parse_entries(char *id,
>         int count;
>         u32 instance = 0;
> 
> -       if (acpi_disabled)
> +       if (acpi_disabled) {
> +               proc[0].count = -ENODEV;
>                 return -ENODEV;
> -
> -       if (!id || !handler)
> -               return -EINVAL;
> +       }
> 
>         if (!strncmp(id, ACPI_SIG_MADT, 4))
>                 instance = acpi_apic_instance;
> @@ -300,23 +330,50 @@ acpi_table_parse_entries(char *id,
>         acpi_get_table_with_size(id, instance, &table_header, &tbl_size);
>         if (!table_header) {
>                 pr_warn("%4.4s not present\n", id);
> +               proc[0].count = -ENODEV;
>                 return -ENODEV;
>         }
> 
> -       count = acpi_parse_entries(id, table_size, handler, table_header,
> -                       entry_id, max_entries);
> +       count = acpi_parse_entries(id, table_size, table_header,
> +                       proc, proc_num, max_entries);
> 
>         early_acpi_os_unmap_memory((char *)table_header, tbl_size);
>         return count;
>  }
> 
>  int __init
> +acpi_table_parse_entries(char *id,
> +                               unsigned long table_size,
> +                               int entry_id,
> +                               acpi_tbl_entry_handler handler,
> +                               unsigned int max_entries)
> +{
> +       struct acpi_subtable_proc proc[1];
> +
> +       if (!handler)
> +               return -EINVAL;
> +
> +       memset(proc, 0, sizeof(proc));
> +       proc[0].id = entry_id;
> +       proc[0].handler = handler;
> +
> +       return acpi_table_parse_entries_array(id, table_size, proc, 1,
> +                                       max_entries);
> +}
> +
> +int __init
>  acpi_table_parse_madt(enum acpi_madt_type id,
>                       acpi_tbl_entry_handler handler, unsigned int max_entries)
>  {
> -       return acpi_table_parse_entries(ACPI_SIG_MADT,
> -                                           sizeof(struct acpi_table_madt), id,
> -                                           handler, max_entries);

Why can't you leave this call as-is ?

> +       struct acpi_subtable_proc madt_proc;

Is there a reason why you use a struct here and a size 1 array in
acpi_table_parse_entries() ? Keep them consistent.

Actually, I do not think you need to parse acpi_table_parse_madt() at
all unless I am missing something.

Lorenzo

> +
> +       memset(&madt_proc, 0, sizeof(madt_proc));
> +       madt_proc.id = id;
> +       madt_proc.handler = handler;
> +
> +       return acpi_table_parse_entries_array(ACPI_SIG_MADT,
> +                                           sizeof(struct acpi_table_madt),
> +                                           &madt_proc, 1, max_entries);
>  }
> 
>  /**
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 4dd8826..d98b866 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table)
>  {
>         void __iomem *cpu_base, *dist_base;
>         int count;
> +       struct acpi_subtable_proc gic_proc;
> +
> +       memset(&gic_proc, 0, sizeof(gic_proc));
> +       gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
> +       gic_proc.handler = gic_acpi_parse_madt_cpu;
> 
>         /* Collect CPU base addresses */
>         count = acpi_parse_entries(ACPI_SIG_MADT,
>                                    sizeof(struct acpi_table_madt),
> -                                  gic_acpi_parse_madt_cpu, table,
> -                                  ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +                                  table, &gic_proc, 1, 0);
>         if (count <= 0) {
>                 pr_err("No valid GICC entries exist\n");
>                 return -EINVAL;
> @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table)
>          * Find distributor base address. We expect one distributor entry since
>          * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade.
>          */
> +       memset(&gic_proc, 0, sizeof(gic_proc));
> +       gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR;
> +       gic_proc.handler = gic_acpi_parse_madt_distributor;
> +
>         count = acpi_parse_entries(ACPI_SIG_MADT,
>                                    sizeof(struct acpi_table_madt),
> -                                  gic_acpi_parse_madt_distributor, table,
> -                                  ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
> +                                  table, &gic_proc, 1, 0);
>         if (count <= 0) {
>                 pr_err("No valid GICD entries exist\n");
>                 return -EINVAL;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d2445fa..59b17e8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size)
>                 (!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>                 ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
> 
> +struct acpi_subtable_proc {
> +       int id;
> +       acpi_tbl_entry_handler handler;
> +       int count;
> +};
> +
>  char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
>  void __acpi_unmap_table(char *map, unsigned long size);
>  int early_acpi_boot_init(void);
> @@ -145,10 +151,13 @@ int acpi_numa_init (void);
> 
>  int acpi_table_init (void);
>  int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
> +int acpi_table_parse_entries_array(char *id, unsigned long table_size,
> +                             struct acpi_subtable_proc *proc, int proc_num,
> +                             unsigned int max_entries);
>  int __init acpi_parse_entries(char *id, unsigned long table_size,
> -                             acpi_tbl_entry_handler handler,
>                               struct acpi_table_header *table_header,
> -                             int entry_id, unsigned int max_entries);
> +                             struct acpi_subtable_proc *proc, int proc_num,
> +                             unsigned int max_entries);
>  int __init acpi_table_parse_entries(char *id, unsigned long table_size,
>                                     int entry_id,
>                                     acpi_tbl_entry_handler handler,
> --
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-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/

[toc] | [prev] | [next] | [standalone]

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

FromIngo Molnar <mingo@kernel.org>
Date2015-08-28 10:40 +0200
SubjectRe: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT
Message-ID<q2nmy-6mv-21@gated-at.bofh.it>
In reply to#1214074
* Lukasz Anaczkowski <lukasz.anaczkowski@intel.com> wrote:

> This patch is based on work of "Yinghai Lu <yinghai@kernel.org>"
> previously published at https://lkml.org/lkml/2013/1/21/563,
> thus putting Yinghai Lu as 'Signed-off-by', as well.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/acpi/boot.c |  29 ++++++++----
>  drivers/acpi/numa.c         |  28 ++++++++---
>  drivers/acpi/tables.c       | 113 +++++++++++++++++++++++++++++++++-----------
>  drivers/irqchip/irq-gic.c   |  15 ++++--
>  include/linux/acpi.h        |  13 ++++-
>  5 files changed, 149 insertions(+), 49 deletions(-)

Yeah, so this patch is way too large, it should be split into 3-4 parts that first 
introduce separate changes (such as renames), then add any extra functions that 
might be needed, and finally does the minimal changes that actually change 
behavior.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-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/

[toc] | [prev] | [next] | [standalone]

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

FromTomasz Nowicki <tomasz.nowicki@linaro.org>
Date2015-09-01 10:10 +0200
SubjectRe: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT
Message-ID<q3ONI-Jq-11@gated-at.bofh.it>
In reply to#1214074
On 26.08.2015 19:49, Lukasz Anaczkowski wrote:
> v3:
>      () Fixed entries counting
>      () Added missing acpi_table_parse_entries definition
>      () acpi_parse_entries() now returns sum of all matching
>         entries
>
> v2: Fixed ARM64 syntax error
>
>  From the ACPI spec:
> "Logical processors with APIC ID values less than 0xFF
> (whether in XAPIC or X2APIC mode) must use the Processor LAPIC
> structure [...]. Logical processors with APIC ID values 0xFF and
> greater must use the Processor Local x2APIC structure."
>
> Because of above, BIOS is first enumerating cores with HT with
> LAPIC values (<0xFF) and then rest with X2APIC values (>=0xFF).
>
> With current kernel code, where enumeration is in order:
> BSP, X2APIC, LAPIC
> enumeration on machine with more than 255 CPUs (each core with 4 HT)
> first X2APIC IDs get low logical CPU IDs (1..x) and then LAPIC IDs
> get higher logical CPU IDs (50..y), as in example below:
>
> Core LCpu  ApicId  LCpu   ApicId   LCpu  ApicId  LCpu  ApicId
> 0    0     0000     97    0001     145   0002    193   0003
> 1   50     0004     98    0005     146   0006    194   0007
> 2   51     0010     99    0011     147   0012    195   0013
> 3   52     0014    100    0015     148   0016    196   0017
> 4   53     0018    101    0019     149   001a    197   001b
> 5   54     001c    102    001d     150   001e    198   001f
> ...
> 62  95     00f8    143    00f9     191   00fa    239   00fb
> 63  37     00ff     96    00fc     144   00fd    192   00fe
> 64   1     0100     13    0101     25    0102     38   0103
> 65   2     0104     14    0105     26    0106     39   0107
> ...
>
> (Core - physical core, LCpu - logical CPU, ApicId - ID assigned
> by BIOS).
>
> This is wrong for the following reasons:
> () it's hard to predict how cores and threads will be enumerated
> () when it's hard to predict, s/w threads cannot be properly affinitized
>     causing significant performance impact due to e.g. inproper cache
>     sharing
> () enumeration is inconsistent with how threads are enumerated on
>     other Intel Xeon processors
>
> To fix this, each LAPIC/X2APIC entry from MADT table needs to be
> handled at the same time when processing it, thus adding
> acpi_subtable_proc structure which stores
> () ACPI table id
> () handler that processes table
> () counter how many items has been processed
> and passing it to acpi_table_parse_entries().

Why can't you leave the parsing code as is and create ApicId sorted list 
while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after 
all... Do I miss something ?

Tomasz

>
> Also, order in which MADT LAPIC/X2APIC handlers are passed is
> reversed to achieve correct CPU enumeration.
>
> In scenario when someone boots kernel with options 'maxcpus=72 nox2apic',
> in result less cores may be booted, since some of the CPUs the kernel
> will try to use will have APIC ID >= 0xFF. In such case, one
> should not pass 'nox2apic'.
>
> Disclimer: code parsing MADT LAPIC/X2APIC has not been touched since 2009,
> when X2APIC support was initially added. I do not know why MADT parsing
> code was added in the reversed order in the first place.
> I guess it didn't matter at that time since nobody cared about cores
> with APIC IDs >= 0xFF, right?
>
> This patch is based on work of "Yinghai Lu <yinghai@kernel.org>"
> previously published at https://lkml.org/lkml/2013/1/21/563,
> thus putting Yinghai Lu as 'Signed-off-by', as well.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   arch/x86/kernel/acpi/boot.c |  29 ++++++++----
>   drivers/acpi/numa.c         |  28 ++++++++---
>   drivers/acpi/tables.c       | 113 +++++++++++++++++++++++++++++++++-----------
>   drivers/irqchip/irq-gic.c   |  15 ++++--
>   include/linux/acpi.h        |  13 ++++-
>   5 files changed, 149 insertions(+), 49 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index e49ee24..fb4a9d6 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -981,6 +981,7 @@ static int __init acpi_parse_madt_lapic_entries(void)
>   {
>   	int count;
>   	int x2count = 0;
> +	struct acpi_subtable_proc madt_proc[2];
>
>   	if (!cpu_has_apic)
>   		return -ENODEV;
> @@ -1004,10 +1005,16 @@ static int __init acpi_parse_madt_lapic_entries(void)
>   				      acpi_parse_sapic, MAX_LOCAL_APIC);
>
>   	if (!count) {
> -		x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
> -					acpi_parse_x2apic, MAX_LOCAL_APIC);
> -		count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
> -					acpi_parse_lapic, MAX_LOCAL_APIC);
> +		memset(madt_proc, 0, sizeof(madt_proc));
> +		madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC;
> +		madt_proc[0].handler = acpi_parse_lapic;
> +		madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC;
> +		madt_proc[1].handler = acpi_parse_x2apic;
> +		acpi_table_parse_entries_array(ACPI_SIG_MADT,
> +			    sizeof(struct acpi_table_madt),
> +			    madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC);
> +		count = madt_proc[0].count;
> +		x2count = madt_proc[1].count;
>   	}
>   	if (!count && !x2count) {
>   		printk(KERN_ERR PREFIX "No LAPIC entries present\n");
> @@ -1019,10 +1026,16 @@ static int __init acpi_parse_madt_lapic_entries(void)
>   		return count;
>   	}
>
> -	x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC_NMI,
> -					acpi_parse_x2apic_nmi, 0);
> -	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_NMI,
> -				      acpi_parse_lapic_nmi, 0);
> +	memset(madt_proc, 0, sizeof(madt_proc));
> +	madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_X2APIC_NMI;
> +	madt_proc[0].handler = acpi_parse_x2apic_nmi;
> +	madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_APIC_NMI;
> +	madt_proc[1].handler = acpi_parse_lapic_nmi;
> +	acpi_table_parse_entries_array(ACPI_SIG_MADT,
> +			    sizeof(struct acpi_table_madt),
> +			    madt_proc, ARRAY_SIZE(madt_proc), 0);
> +	count = madt_proc[0].count;
> +	x2count = madt_proc[1].count;
>   	if (count < 0 || x2count < 0) {
>   		printk(KERN_ERR PREFIX "Error parsing LAPIC NMI entry\n");
>   		/* TBD: Cleanup to allow fallback to MPS */
> diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
> index acaa3b4..a000195 100644
> --- a/drivers/acpi/numa.c
> +++ b/drivers/acpi/numa.c
> @@ -314,9 +314,15 @@ static int __init
>   acpi_table_parse_srat(enum acpi_srat_type id,
>   		      acpi_tbl_entry_handler handler, unsigned int max_entries)
>   {
> -	return acpi_table_parse_entries(ACPI_SIG_SRAT,
> -					    sizeof(struct acpi_table_srat), id,
> -					    handler, max_entries);
> +	struct acpi_subtable_proc srat_proc;
> +
> +	memset(&srat_proc, 0, sizeof(srat_proc));
> +	srat_proc.id = id;
> +	srat_proc.handler = handler;
> +
> +	return acpi_table_parse_entries_array(ACPI_SIG_SRAT,
> +					    sizeof(struct acpi_table_srat),
> +					    &srat_proc, 1, max_entries);
>   }
>
>   int __init acpi_numa_init(void)
> @@ -331,10 +337,18 @@ int __init acpi_numa_init(void)
>
>   	/* SRAT: Static Resource Affinity Table */
>   	if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
> -		acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY,
> -				     acpi_parse_x2apic_affinity, 0);
> -		acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY,
> -				     acpi_parse_processor_affinity, 0);
> +		struct acpi_subtable_proc srat_proc[2];
> +
> +		memset(srat_proc, 0, sizeof(srat_proc));
> +		srat_proc[0].id = ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY;
> +		srat_proc[0].handler = acpi_parse_x2apic_affinity;
> +		srat_proc[1].id = ACPI_SRAT_TYPE_CPU_AFFINITY;
> +		srat_proc[1].handler = acpi_parse_processor_affinity;
> +
> +		acpi_table_parse_entries_array(ACPI_SIG_SRAT,
> +					    sizeof(struct acpi_table_srat),
> +					    srat_proc, ARRAY_SIZE(srat_proc), 0);
> +
>   		cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
>   					    acpi_parse_memory_affinity,
>   					    NR_NODE_MEMBLKS);
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 2e19189..d5c9a1b 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -214,27 +214,45 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>   	}
>   }
>
> +/**
> + * acpi_table_parse - for each proc_num find a subtable with proc->id
> + *    and run proc->handler on it. Assumption is that there's only
> + *    single handler for particular id.
> + *
> + * @id: table id (for debugging purposes)
> + * @table_size: single entry size
> + * @table_header: where does the table start?
> + * @proc: array of acpi_subtable_proc struct containing subtable id
> + *        and associated handler with it
> + * @proc_num: how big proc is?
> + * @max_entries: how many entries can we process?
> + *
> + * On success returns sum of all matching entries for all proc handlers.
> + * Oterwise, -ENODEV or -EINVAL is returned.
> + */
>   int __init
>   acpi_parse_entries(char *id, unsigned long table_size,
> -		acpi_tbl_entry_handler handler,
>   		struct acpi_table_header *table_header,
> -		int entry_id, unsigned int max_entries)
> +		struct acpi_subtable_proc *proc, int proc_num,
> +		unsigned int max_entries)
>   {
>   	struct acpi_subtable_header *entry;
>   	int count = 0;
>   	unsigned long table_end;
> +	int i;
>
> -	if (acpi_disabled)
> +	if (acpi_disabled) {
> +		proc[0].count = -ENODEV;
>   		return -ENODEV;
> -
> -	if (!id || !handler)
> -		return -EINVAL;
> -
> -	if (!table_size)
> +	}
> +	if (!table_size) {
> +		proc[0].count = -EINVAL;
>   		return -EINVAL;
> +	}
>
>   	if (!table_header) {
>   		pr_warn("%4.4s not present\n", id);
> +		proc[0].count = -ENODEV;
>   		return -ENODEV;
>   	}
>
> @@ -247,20 +265,31 @@ acpi_parse_entries(char *id, unsigned long table_size,
>
>   	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>   	       table_end) {
> -		if (entry->type == entry_id
> -		    && (!max_entries || count < max_entries)) {
> -			if (handler(entry, table_end))
> +		if (max_entries && count >= max_entries)
> +			continue;
> +		for (i = 0; i < proc_num; i++) {
> +			if (entry->type != proc[i].id)
> +				continue;
> +			if (proc[i].handler(entry, table_end)) {
> +				proc[i].count = -EINVAL;
>   				return -EINVAL;
> -
> -			count++;
> +			}
> +			proc[i].count++;
> +			break;
>   		}
> +		if (i != proc_num)
> +			count++;
>
>   		/*
>   		 * If entry->length is 0, break from this loop to avoid
>   		 * infinite loop.
>   		 */
>   		if (entry->length == 0) {
> -			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
> +			pr_err("[%4.4s:0x%02x ", id, proc[0].id);
> +			for (i = 1; i < proc_num; i++)
> +				pr_cont(" 0x%02x", proc[i].id);
> +			pr_cont("] Invalid zero length\n");
> +			proc[0].count = -EINVAL;
>   			return -EINVAL;
>   		}
>
> @@ -269,18 +298,20 @@ acpi_parse_entries(char *id, unsigned long table_size,
>   	}
>
>   	if (max_entries && count > max_entries) {
> -		pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
> -			id, entry_id, count - max_entries, count);
> +		pr_warn("[%4.4s:0x%02x ", id, proc[0].id);
> +		for (i = 1; i < proc_num; i++)
> +			pr_cont(" 0x%02x", proc[i].id);
> +		pr_cont("] ignored %i entries of %i found\n",
> +			count-max_entries, count);
>   	}
>
>   	return count;
>   }
>
>   int __init
> -acpi_table_parse_entries(char *id,
> +acpi_table_parse_entries_array(char *id,
>   			 unsigned long table_size,
> -			 int entry_id,
> -			 acpi_tbl_entry_handler handler,
> +			 struct acpi_subtable_proc *proc, int proc_num,
>   			 unsigned int max_entries)
>   {
>   	struct acpi_table_header *table_header = NULL;
> @@ -288,11 +319,10 @@ acpi_table_parse_entries(char *id,
>   	int count;
>   	u32 instance = 0;
>
> -	if (acpi_disabled)
> +	if (acpi_disabled) {
> +		proc[0].count = -ENODEV;
>   		return -ENODEV;
> -
> -	if (!id || !handler)
> -		return -EINVAL;
> +	}
>
>   	if (!strncmp(id, ACPI_SIG_MADT, 4))
>   		instance = acpi_apic_instance;
> @@ -300,23 +330,50 @@ acpi_table_parse_entries(char *id,
>   	acpi_get_table_with_size(id, instance, &table_header, &tbl_size);
>   	if (!table_header) {
>   		pr_warn("%4.4s not present\n", id);
> +		proc[0].count = -ENODEV;
>   		return -ENODEV;
>   	}
>
> -	count = acpi_parse_entries(id, table_size, handler, table_header,
> -			entry_id, max_entries);
> +	count = acpi_parse_entries(id, table_size, table_header,
> +			proc, proc_num, max_entries);
>
>   	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
>   	return count;
>   }
>
>   int __init
> +acpi_table_parse_entries(char *id,
> +				unsigned long table_size,
> +				int entry_id,
> +				acpi_tbl_entry_handler handler,
> +				unsigned int max_entries)
> +{
> +	struct acpi_subtable_proc proc[1];
> +
> +	if (!handler)
> +		return -EINVAL;
> +
> +	memset(proc, 0, sizeof(proc));
> +	proc[0].id = entry_id;
> +	proc[0].handler = handler;
> +
> +	return acpi_table_parse_entries_array(id, table_size, proc, 1,
> +					max_entries);
> +}
> +
> +int __init
>   acpi_table_parse_madt(enum acpi_madt_type id,
>   		      acpi_tbl_entry_handler handler, unsigned int max_entries)
>   {
> -	return acpi_table_parse_entries(ACPI_SIG_MADT,
> -					    sizeof(struct acpi_table_madt), id,
> -					    handler, max_entries);
> +	struct acpi_subtable_proc madt_proc;
> +
> +	memset(&madt_proc, 0, sizeof(madt_proc));
> +	madt_proc.id = id;
> +	madt_proc.handler = handler;
> +
> +	return acpi_table_parse_entries_array(ACPI_SIG_MADT,
> +					    sizeof(struct acpi_table_madt),
> +					    &madt_proc, 1, max_entries);
>   }
>
>   /**
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 4dd8826..d98b866 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1091,12 +1091,16 @@ gic_v2_acpi_init(struct acpi_table_header *table)
>   {
>   	void __iomem *cpu_base, *dist_base;
>   	int count;
> +	struct acpi_subtable_proc gic_proc;
> +
> +	memset(&gic_proc, 0, sizeof(gic_proc));
> +	gic_proc.id = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
> +	gic_proc.handler = gic_acpi_parse_madt_cpu;
>
>   	/* Collect CPU base addresses */
>   	count = acpi_parse_entries(ACPI_SIG_MADT,
>   				   sizeof(struct acpi_table_madt),
> -				   gic_acpi_parse_madt_cpu, table,
> -				   ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +				   table, &gic_proc, 1, 0);
>   	if (count <= 0) {
>   		pr_err("No valid GICC entries exist\n");
>   		return -EINVAL;
> @@ -1106,10 +1110,13 @@ gic_v2_acpi_init(struct acpi_table_header *table)
>   	 * Find distributor base address. We expect one distributor entry since
>   	 * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade.
>   	 */
> +	memset(&gic_proc, 0, sizeof(gic_proc));
> +	gic_proc.id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR;
> +	gic_proc.handler = gic_acpi_parse_madt_distributor;
> +
>   	count = acpi_parse_entries(ACPI_SIG_MADT,
>   				   sizeof(struct acpi_table_madt),
> -				   gic_acpi_parse_madt_distributor, table,
> -				   ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
> +				   table, &gic_proc, 1, 0);
>   	if (count <= 0) {
>   		pr_err("No valid GICD entries exist\n");
>   		return -EINVAL;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d2445fa..59b17e8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size)
>   		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>   		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>
> +struct acpi_subtable_proc {
> +	int id;
> +	acpi_tbl_entry_handler handler;
> +	int count;
> +};
> +
>   char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
>   void __acpi_unmap_table(char *map, unsigned long size);
>   int early_acpi_boot_init(void);
> @@ -145,10 +151,13 @@ int acpi_numa_init (void);
>
>   int acpi_table_init (void);
>   int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
> +int acpi_table_parse_entries_array(char *id, unsigned long table_size,
> +			      struct acpi_subtable_proc *proc, int proc_num,
> +			      unsigned int max_entries);
>   int __init acpi_parse_entries(char *id, unsigned long table_size,
> -			      acpi_tbl_entry_handler handler,
>   			      struct acpi_table_header *table_header,
> -			      int entry_id, unsigned int max_entries);
> +			      struct acpi_subtable_proc *proc, int proc_num,
> +			      unsigned int max_entries);
>   int __init acpi_table_parse_entries(char *id, unsigned long table_size,
>   				    int entry_id,
>   				    acpi_tbl_entry_handler handler,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-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/

[toc] | [prev] | [next] | [standalone]

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

From"Anaczkowski, Lukasz" <lukasz.anaczkowski@intel.com>
Date2015-09-01 14:10 +0200
SubjectRE: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT
Message-ID<q3SxY-65L-17@gated-at.bofh.it>
In reply to#1216660
From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org] 
Sent: Tuesday, September 1, 2015 10:03 AM

>> To fix this, each LAPIC/X2APIC entry from MADT table needs to be 
>> handled at the same time when processing it, thus adding 
>> acpi_subtable_proc structure which stores
>> () ACPI table id
>> () handler that processes table
>> () counter how many items has been processed and passing it to 
>> acpi_table_parse_entries().

> Why can't you leave the parsing code as is and create ApicId sorted list while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after all... Do I miss something ?

Just to make sure I understand correctly - you suggest to replace calls to acpi_register_lapic() with a code that builds an APIC ID list while parsing LAPIC/X2APIC, and after parsing is done,
go thru the list and call acpi_register_lapic() on each APIC ID, correct?

Thanks,
Lukasz
--
To unsubscribe from this list: send the line "unsubscribe linux-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/

[toc] | [prev] | [next] | [standalone]

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

FromTomasz Nowicki <tomasz.nowicki@linaro.org>
Date2015-09-01 15:40 +0200
SubjectRe: [PATCH] x86, arm64, acpi: Handle lapic/x2apic entries in MADT
Message-ID<q3TX5-7YE-33@gated-at.bofh.it>
In reply to#1216804

On 01.09.2015 14:07, Anaczkowski, Lukasz wrote:
> From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org]
> Sent: Tuesday, September 1, 2015 10:03 AM
>
>>> To fix this, each LAPIC/X2APIC entry from MADT table needs to be
>>> handled at the same time when processing it, thus adding
>>> acpi_subtable_proc structure which stores
>>> () ACPI table id
>>> () handler that processes table
>>> () counter how many items has been processed and passing it to
>>> acpi_table_parse_entries().
>
>> Why can't you leave the parsing code as is and create ApicId sorted list while parsing LAPIC/X2APIC? You could call acpi_register_lapic() after all... Do I miss something ?
>
> Just to make sure I understand correctly - you suggest to replace calls to acpi_register_lapic() with a code that builds an APIC ID list while parsing LAPIC/X2APIC, and after parsing is done,
> go thru the list and call acpi_register_lapic() on each APIC ID, correct?
>

Yes, does it work for you?

Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-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/

[toc] | [prev] | [next] | [standalone]

#1214079

FromLukasz Anaczkowski <lukasz.anaczkowski@intel.com>
Date2015-08-26 19:50 +0200
Message-ID<q1MZH-4is-7@gated-at.bofh.it>
In reply to#1213827
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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web