x2APIC and many-APIC systems...

x2APIC and many-APIC systems...

[Daniel J Blueman]

Ingo, Suresh,

Commit a35fd28256e7736cc84af8931a16224f0bfaaf6c prevents x2APIC 
structures being used from the ACPI MADT if the cores don't advertise 
the x2APIC feature. Commit c284b42abadbb22083bfde24d308899c08d44ffa 
prevents onlining cores with APIC ID >255 in non-x2APIC mode. Since 
NumaChip/NumaConnect uses x2APIC structures to describe non-x2APIC 
systems (AMD Opteron) with lots of APICs, we thus now can't boot all the 
cores.

We are able to set the x2APIC bit in the processor feature flags, so get 
caught by the second patch. Is there an appropriate approach to use in 
these circumstances? Otherwise, would a patch that separates the APIC ID 
handover and future x2APIC MSR access be appropriate?

Many thanks,
   Daniel
-- 
Daniel J Blueman
Principal Software Engineer, Numascale Asia

Re: x2APIC and many-APIC systems...

[Yinghai Lu]

On Tue, Mar 13, 2012 at 2:29 AM, Daniel J Blueman
<daniel@numascale-asia.com> wrote:
> Ingo, Suresh,
>
> Commit a35fd28256e7736cc84af8931a16224f0bfaaf6c prevents x2APIC structures
> being used from the ACPI MADT if the cores don't advertise the x2APIC
> feature. Commit c284b42abadbb22083bfde24d308899c08d44ffa prevents onlining
> cores with APIC ID >255 in non-x2APIC mode. Since NumaChip/NumaConnect uses
> x2APIC structures to describe non-x2APIC systems (AMD Opteron) with lots of
> APICs, we thus now can't boot all the cores.
>
> We are able to set the x2APIC bit in the processor feature flags, so get
> caught by the second patch. Is there an appropriate approach to use in these
> circumstances? Otherwise, would a patch that separates the APIC ID handover
> and future x2APIC MSR access be appropriate?
>

add is_apicid_valid() in struct apic?

Yinghai

Re: x2APIC and many-APIC systems...

[Suresh Siddha]

On Tue, 2012-03-13 at 15:58 -0700, Yinghai Lu wrote:
> On Tue, Mar 13, 2012 at 2:29 AM, Daniel J Blueman
> <daniel@numascale-asia.com> wrote:
> > Ingo, Suresh,
> >
> > Commit a35fd28256e7736cc84af8931a16224f0bfaaf6c prevents x2APIC structures
> > being used from the ACPI MADT if the cores don't advertise the x2APIC
> > feature. Commit c284b42abadbb22083bfde24d308899c08d44ffa prevents onlining
> > cores with APIC ID >255 in non-x2APIC mode. Since NumaChip/NumaConnect uses
> > x2APIC structures to describe non-x2APIC systems (AMD Opteron) with lots of
> > APICs, we thus now can't boot all the cores.
> >
> > We are able to set the x2APIC bit in the processor feature flags, so get
> > caught by the second patch. Is there an appropriate approach to use in these
> > circumstances? Otherwise, would a patch that separates the APIC ID handover
> > and future x2APIC MSR access be appropriate?
> >
> 
> add is_apicid_valid() in struct apic?

Yes, we can generalize that and move into the apicdriver. Daniel, will
you be able to prepare such a patch?

thanks,
suresh

[PATCH] Move APIC ID validity check into platform APIC code

[Daniel J Blueman]

Move APIC ID validity check into platform APIC code, so it can be
overridden when needed. For NumaChip systems, always trust MADT,
as it's constructed with high APIC IDs.

Behaviour verifies on standard x86 systems and on NumaChip systems
with this, and compile-tested with allyesconfig.

Signed-off-by: Daniel J Blueman <daniel@numascale-asia.com>
Reviewed-by: Steffen Persvold <sp@numascale.com>

---
 arch/x86/include/asm/apic.h           |    6 ++++++
 arch/x86/kernel/apic/apic_flat_64.c   |    2 ++
 arch/x86/kernel/apic/apic_noop.c      |    1 +
 arch/x86/kernel/apic/apic_numachip.c  |   10 +++++++++-
 arch/x86/kernel/apic/bigsmp_32.c      |    1 +
 arch/x86/kernel/apic/es7000_32.c      |    2 ++
 arch/x86/kernel/apic/numaq_32.c       |    1 +
 arch/x86/kernel/apic/probe_32.c       |    1 +
 arch/x86/kernel/apic/summit_32.c      |    1 +
 arch/x86/kernel/apic/x2apic_cluster.c |    1 +
 arch/x86/kernel/apic/x2apic_phys.c    |    1 +
 arch/x86/kernel/apic/x2apic_uv_x.c    |    1 +
 arch/x86/kernel/smpboot.c             |    2 +-
 13 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 3ab9bdd..a9371c9 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -288,6 +288,7 @@ struct apic {
 
 	int (*probe)(void);
 	int (*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
+	int (*apic_id_valid)(int apicid);
 	int (*apic_id_registered)(void);
 
 	u32 irq_delivery_mode;
@@ -532,6 +533,11 @@ static inline unsigned int read_apic_id(void)
 	return apic->get_apic_id(reg);
 }
 
+static inline int default_apic_id_valid(int apicid)
+{
+	return x2apic_mode || (apicid < 255);
+}
+
 extern void default_setup_apic_routing(void);
 
 extern struct apic apic_noop;
diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
index 8c3cdde..359b689 100644
--- a/arch/x86/kernel/apic/apic_flat_64.c
+++ b/arch/x86/kernel/apic/apic_flat_64.c
@@ -180,6 +180,7 @@ static struct apic apic_flat =  {
 	.name				= "flat",
 	.probe				= flat_probe,
 	.acpi_madt_oem_check		= flat_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= flat_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
@@ -337,6 +338,7 @@ static struct apic apic_physflat =  {
 	.name				= "physical flat",
 	.probe				= physflat_probe,
 	.acpi_madt_oem_check		= physflat_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= flat_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/apic_noop.c b/arch/x86/kernel/apic/apic_noop.c
index 775b82b..634ae6c 100644
--- a/arch/x86/kernel/apic/apic_noop.c
+++ b/arch/x86/kernel/apic/apic_noop.c
@@ -124,6 +124,7 @@ struct apic apic_noop = {
 	.probe				= noop_probe,
 	.acpi_madt_oem_check		= NULL,
 
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= noop_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 09d3d8c..d9ea5f3 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -56,6 +56,12 @@ static unsigned int read_xapic_id(void)
 	return get_apic_id(apic_read(APIC_ID));
 }
 
+static int numachip_apic_id_valid(int apicid)
+{
+	/* Trust what bootloader passes in MADT */
+	return 1;
+}
+
 static int numachip_apic_id_registered(void)
 {
 	return physid_isset(read_xapic_id(), phys_cpu_present_map);
@@ -223,10 +229,11 @@ static int __init numachip_system_init(void)
 }
 early_initcall(numachip_system_init);
 
-static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	if (!strncmp(oem_id, "NUMASC", 6)) {
 		numachip_system = 1;
+		setup_force_cpu_cap(X86_FEATURE_X2APIC);
 		return 1;
 	}
 
@@ -238,6 +245,7 @@ static struct apic apic_numachip __refconst = {
 	.name				= "NumaConnect system",
 	.probe				= numachip_probe,
 	.acpi_madt_oem_check		= numachip_acpi_madt_oem_check,
+	.apic_id_valid			= numachip_apic_id_valid,
 	.apic_id_registered		= numachip_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/bigsmp_32.c b/arch/x86/kernel/apic/bigsmp_32.c
index 521bead..0cdec70 100644
--- a/arch/x86/kernel/apic/bigsmp_32.c
+++ b/arch/x86/kernel/apic/bigsmp_32.c
@@ -198,6 +198,7 @@ static struct apic apic_bigsmp = {
 	.name				= "bigsmp",
 	.probe				= probe_bigsmp,
 	.acpi_madt_oem_check		= NULL,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= bigsmp_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/es7000_32.c b/arch/x86/kernel/apic/es7000_32.c
index 5d513bc..e42d1d3b9 100644
--- a/arch/x86/kernel/apic/es7000_32.c
+++ b/arch/x86/kernel/apic/es7000_32.c
@@ -625,6 +625,7 @@ static struct apic __refdata apic_es7000_cluster = {
 	.name				= "es7000",
 	.probe				= probe_es7000,
 	.acpi_madt_oem_check		= es7000_acpi_madt_oem_check_cluster,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= es7000_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
@@ -690,6 +691,7 @@ static struct apic __refdata apic_es7000 = {
 	.name				= "es7000",
 	.probe				= probe_es7000,
 	.acpi_madt_oem_check		= es7000_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= es7000_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/numaq_32.c b/arch/x86/kernel/apic/numaq_32.c
index c4a61ca..00d2422 100644
--- a/arch/x86/kernel/apic/numaq_32.c
+++ b/arch/x86/kernel/apic/numaq_32.c
@@ -478,6 +478,7 @@ static struct apic __refdata apic_numaq = {
 	.name				= "NUMAQ",
 	.probe				= probe_numaq,
 	.acpi_madt_oem_check		= NULL,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= numaq_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/probe_32.c b/arch/x86/kernel/apic/probe_32.c
index 0787bb3..ff2c1b9 100644
--- a/arch/x86/kernel/apic/probe_32.c
+++ b/arch/x86/kernel/apic/probe_32.c
@@ -92,6 +92,7 @@ static struct apic apic_default = {
 	.name				= "default",
 	.probe				= probe_default,
 	.acpi_madt_oem_check		= NULL,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= default_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/summit_32.c b/arch/x86/kernel/apic/summit_32.c
index 1911442..fea000b 100644
--- a/arch/x86/kernel/apic/summit_32.c
+++ b/arch/x86/kernel/apic/summit_32.c
@@ -496,6 +496,7 @@ static struct apic apic_summit = {
 	.name				= "summit",
 	.probe				= probe_summit,
 	.acpi_madt_oem_check		= summit_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= summit_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index 5007958..9193713 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -213,6 +213,7 @@ static struct apic apic_x2apic_cluster = {
 	.name				= "cluster x2apic",
 	.probe				= x2apic_cluster_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index f5373df..bcd1db6 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -119,6 +119,7 @@ static struct apic apic_x2apic_phys = {
 	.name				= "physical x2apic",
 	.probe				= x2apic_phys_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 79b05b8..fc47714 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -351,6 +351,7 @@ static struct apic __refdata apic_x2apic_uv_x = {
 	.name				= "UV large system",
 	.probe				= uv_probe,
 	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= uv_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 66d250c..d279e6e 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -847,7 +847,7 @@ int __cpuinit native_cpu_up(unsigned int cpu)
 
 	if (apicid == BAD_APICID || apicid == boot_cpu_physical_apicid ||
 	    !physid_isset(apicid, phys_cpu_present_map) ||
-	    (!x2apic_mode && apicid >= 255)) {
+	    !apic->apic_id_valid(apicid)) {
 		printk(KERN_ERR "%s: bad cpu %d\n", __func__, cpu);
 		return -EINVAL;
 	}
-- 
1.7.5.4

[tip:x86/platform] x86/platform: Move APIC ID validity check into platform APIC code

[tip-bot for Daniel J Blueman]

Commit-ID:  fa63030e9c79e37b4d4e63b39ffb09cfb7aa0fe4
Gitweb:     http://git.kernel.org/tip/fa63030e9c79e37b4d4e63b39ffb09cfb7aa0fe4
Author:     Daniel J Blueman <daniel@numascale-asia.com>
AuthorDate: Wed, 14 Mar 2012 15:17:34 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 14 Mar 2012 09:49:48 +0100

x86/platform: Move APIC ID validity check into platform APIC code

Move APIC ID validity check into platform APIC code, so it can
be overridden when needed. For NumaChip systems, always trust
MADT, as it's constructed with high APIC IDs.

Behaviour verifies on standard x86 systems and on NumaChip
systems with this, and compile-tested with allyesconfig.

Signed-off-by: Daniel J Blueman <daniel@numascale-asia.com>
Reviewed-by: Steffen Persvold <sp@numascale.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Link: http://lkml.kernel.org/r/1331709454-27966-1-git-send-email-daniel@numascale-asia.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/apic.h           |    6 ++++++
 arch/x86/kernel/apic/apic_flat_64.c   |    2 ++
 arch/x86/kernel/apic/apic_noop.c      |    1 +
 arch/x86/kernel/apic/apic_numachip.c  |   10 +++++++++-
 arch/x86/kernel/apic/bigsmp_32.c      |    1 +
 arch/x86/kernel/apic/es7000_32.c      |    2 ++
 arch/x86/kernel/apic/numaq_32.c       |    1 +
 arch/x86/kernel/apic/probe_32.c       |    1 +
 arch/x86/kernel/apic/summit_32.c      |    1 +
 arch/x86/kernel/apic/x2apic_cluster.c |    1 +
 arch/x86/kernel/apic/x2apic_phys.c    |    1 +
 arch/x86/kernel/apic/x2apic_uv_x.c    |    1 +
 arch/x86/kernel/smpboot.c             |    2 +-
 13 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 3ab9bdd..a9371c9 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -288,6 +288,7 @@ struct apic {
 
 	int (*probe)(void);
 	int (*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
+	int (*apic_id_valid)(int apicid);
 	int (*apic_id_registered)(void);
 
 	u32 irq_delivery_mode;
@@ -532,6 +533,11 @@ static inline unsigned int read_apic_id(void)
 	return apic->get_apic_id(reg);
 }
 
+static inline int default_apic_id_valid(int apicid)
+{
+	return x2apic_mode || (apicid < 255);
+}
+
 extern void default_setup_apic_routing(void);
 
 extern struct apic apic_noop;
diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
index 8c3cdde..359b689 100644
--- a/arch/x86/kernel/apic/apic_flat_64.c
+++ b/arch/x86/kernel/apic/apic_flat_64.c
@@ -180,6 +180,7 @@ static struct apic apic_flat =  {
 	.name				= "flat",
 	.probe				= flat_probe,
 	.acpi_madt_oem_check		= flat_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= flat_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
@@ -337,6 +338,7 @@ static struct apic apic_physflat =  {
 	.name				= "physical flat",
 	.probe				= physflat_probe,
 	.acpi_madt_oem_check		= physflat_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= flat_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/apic_noop.c b/arch/x86/kernel/apic/apic_noop.c
index 775b82b..634ae6c 100644
--- a/arch/x86/kernel/apic/apic_noop.c
+++ b/arch/x86/kernel/apic/apic_noop.c
@@ -124,6 +124,7 @@ struct apic apic_noop = {
 	.probe				= noop_probe,
 	.acpi_madt_oem_check		= NULL,
 
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= noop_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 09d3d8c..d9ea5f3 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -56,6 +56,12 @@ static unsigned int read_xapic_id(void)
 	return get_apic_id(apic_read(APIC_ID));
 }
 
+static int numachip_apic_id_valid(int apicid)
+{
+	/* Trust what bootloader passes in MADT */
+	return 1;
+}
+
 static int numachip_apic_id_registered(void)
 {
 	return physid_isset(read_xapic_id(), phys_cpu_present_map);
@@ -223,10 +229,11 @@ static int __init numachip_system_init(void)
 }
 early_initcall(numachip_system_init);
 
-static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	if (!strncmp(oem_id, "NUMASC", 6)) {
 		numachip_system = 1;
+		setup_force_cpu_cap(X86_FEATURE_X2APIC);
 		return 1;
 	}
 
@@ -238,6 +245,7 @@ static struct apic apic_numachip __refconst = {
 	.name				= "NumaConnect system",
 	.probe				= numachip_probe,
 	.acpi_madt_oem_check		= numachip_acpi_madt_oem_check,
+	.apic_id_valid			= numachip_apic_id_valid,
 	.apic_id_registered		= numachip_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/bigsmp_32.c b/arch/x86/kernel/apic/bigsmp_32.c
index 521bead..0cdec70 100644
--- a/arch/x86/kernel/apic/bigsmp_32.c
+++ b/arch/x86/kernel/apic/bigsmp_32.c
@@ -198,6 +198,7 @@ static struct apic apic_bigsmp = {
 	.name				= "bigsmp",
 	.probe				= probe_bigsmp,
 	.acpi_madt_oem_check		= NULL,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= bigsmp_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/es7000_32.c b/arch/x86/kernel/apic/es7000_32.c
index 5d513bc..e42d1d3b9 100644
--- a/arch/x86/kernel/apic/es7000_32.c
+++ b/arch/x86/kernel/apic/es7000_32.c
@@ -625,6 +625,7 @@ static struct apic __refdata apic_es7000_cluster = {
 	.name				= "es7000",
 	.probe				= probe_es7000,
 	.acpi_madt_oem_check		= es7000_acpi_madt_oem_check_cluster,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= es7000_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
@@ -690,6 +691,7 @@ static struct apic __refdata apic_es7000 = {
 	.name				= "es7000",
 	.probe				= probe_es7000,
 	.acpi_madt_oem_check		= es7000_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= es7000_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/numaq_32.c b/arch/x86/kernel/apic/numaq_32.c
index c4a61ca..00d2422 100644
--- a/arch/x86/kernel/apic/numaq_32.c
+++ b/arch/x86/kernel/apic/numaq_32.c
@@ -478,6 +478,7 @@ static struct apic __refdata apic_numaq = {
 	.name				= "NUMAQ",
 	.probe				= probe_numaq,
 	.acpi_madt_oem_check		= NULL,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= numaq_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/probe_32.c b/arch/x86/kernel/apic/probe_32.c
index 0787bb3..ff2c1b9 100644
--- a/arch/x86/kernel/apic/probe_32.c
+++ b/arch/x86/kernel/apic/probe_32.c
@@ -92,6 +92,7 @@ static struct apic apic_default = {
 	.name				= "default",
 	.probe				= probe_default,
 	.acpi_madt_oem_check		= NULL,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= default_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/summit_32.c b/arch/x86/kernel/apic/summit_32.c
index 1911442..fea000b 100644
--- a/arch/x86/kernel/apic/summit_32.c
+++ b/arch/x86/kernel/apic/summit_32.c
@@ -496,6 +496,7 @@ static struct apic apic_summit = {
 	.name				= "summit",
 	.probe				= probe_summit,
 	.acpi_madt_oem_check		= summit_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= summit_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index 5007958..9193713 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -213,6 +213,7 @@ static struct apic apic_x2apic_cluster = {
 	.name				= "cluster x2apic",
 	.probe				= x2apic_cluster_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index f5373df..bcd1db6 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -119,6 +119,7 @@ static struct apic apic_x2apic_phys = {
 	.name				= "physical x2apic",
 	.probe				= x2apic_phys_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 79b05b8..fc47714 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -351,6 +351,7 @@ static struct apic __refdata apic_x2apic_uv_x = {
 	.name				= "UV large system",
 	.probe				= uv_probe,
 	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
+	.apic_id_valid			= default_apic_id_valid,
 	.apic_id_registered		= uv_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 66d250c..d279e6e 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -847,7 +847,7 @@ int __cpuinit native_cpu_up(unsigned int cpu)
 
 	if (apicid == BAD_APICID || apicid == boot_cpu_physical_apicid ||
 	    !physid_isset(apicid, phys_cpu_present_map) ||
-	    (!x2apic_mode && apicid >= 255)) {
+	    !apic->apic_id_valid(apicid)) {
 		printk(KERN_ERR "%s: bad cpu %d\n", __func__, cpu);
 		return -EINVAL;
 	}

Re: [PATCH] Move APIC ID validity check into platform APIC code

[Yinghai Lu]

On Wed, Mar 14, 2012 at 12:17 AM, Daniel J Blueman
<daniel@numascale-asia.com> wrote:
> Move APIC ID validity check into platform APIC code, so it can be
> overridden when needed. For NumaChip systems, always trust MADT,
> as it's constructed with high APIC IDs.
>
> Behaviour verifies on standard x86 systems and on NumaChip systems
> with this, and compile-tested with allyesconfig.
>
> Signed-off-by: Daniel J Blueman <daniel@numascale-asia.com>
> Reviewed-by: Steffen Persvold <sp@numascale.com>
>
> ---
>  arch/x86/include/asm/apic.h           |    6 ++++++
>  arch/x86/kernel/apic/apic_flat_64.c   |    2 ++
>  arch/x86/kernel/apic/apic_noop.c      |    1 +
>  arch/x86/kernel/apic/apic_numachip.c  |   10 +++++++++-
>  arch/x86/kernel/apic/bigsmp_32.c      |    1 +
>  arch/x86/kernel/apic/es7000_32.c      |    2 ++
>  arch/x86/kernel/apic/numaq_32.c       |    1 +
>  arch/x86/kernel/apic/probe_32.c       |    1 +
>  arch/x86/kernel/apic/summit_32.c      |    1 +
>  arch/x86/kernel/apic/x2apic_cluster.c |    1 +
>  arch/x86/kernel/apic/x2apic_phys.c    |    1 +
>  arch/x86/kernel/apic/x2apic_uv_x.c    |    1 +
>  arch/x86/kernel/smpboot.c             |    2 +-
>  13 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 3ab9bdd..a9371c9 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -288,6 +288,7 @@ struct apic {
>
>        int (*probe)(void);
>        int (*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
> +       int (*apic_id_valid)(int apicid);
>        int (*apic_id_registered)(void);
>
>        u32 irq_delivery_mode;
> @@ -532,6 +533,11 @@ static inline unsigned int read_apic_id(void)
>        return apic->get_apic_id(reg);
>  }
>
> +static inline int default_apic_id_valid(int apicid)
> +{
> +       return x2apic_mode || (apicid < 255);
> +}
> +
>  extern void default_setup_apic_routing(void);
>
>  extern struct apic apic_noop;
> diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
> index 8c3cdde..359b689 100644
> --- a/arch/x86/kernel/apic/apic_flat_64.c
> +++ b/arch/x86/kernel/apic/apic_flat_64.c
> @@ -180,6 +180,7 @@ static struct apic apic_flat =  {
>        .name                           = "flat",
>        .probe                          = flat_probe,
>        .acpi_madt_oem_check            = flat_acpi_madt_oem_check,
> +       .apic_id_valid                  = default_apic_id_valid,
>        .apic_id_registered             = flat_apic_id_registered,
>
>        .irq_delivery_mode              = dest_LowestPrio,
> @@ -337,6 +338,7 @@ static struct apic apic_physflat =  {
>        .name                           = "physical flat",
>        .probe                          = physflat_probe,
>        .acpi_madt_oem_check            = physflat_acpi_madt_oem_check,
> +       .apic_id_valid                  = default_apic_id_valid,
>        .apic_id_registered             = flat_apic_id_registered,
>
>        .irq_delivery_mode              = dest_Fixed,
> diff --git a/arch/x86/kernel/apic/apic_noop.c b/arch/x86/kernel/apic/apic_noop.c
> index 775b82b..634ae6c 100644
> --- a/arch/x86/kernel/apic/apic_noop.c
> +++ b/arch/x86/kernel/apic/apic_noop.c
> @@ -124,6 +124,7 @@ struct apic apic_noop = {
>        .probe                          = noop_probe,
>        .acpi_madt_oem_check            = NULL,
>
> +       .apic_id_valid                  = default_apic_id_valid,
>        .apic_id_registered             = noop_apic_id_registered,
>
>        .irq_delivery_mode              = dest_LowestPrio,
> diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
> index 09d3d8c..d9ea5f3 100644
> --- a/arch/x86/kernel/apic/apic_numachip.c
> +++ b/arch/x86/kernel/apic/apic_numachip.c
> @@ -56,6 +56,12 @@ static unsigned int read_xapic_id(void)
>        return get_apic_id(apic_read(APIC_ID));
>  }
>
> +static int numachip_apic_id_valid(int apicid)
> +{
> +       /* Trust what bootloader passes in MADT */
> +       return 1;
> +}
> +
>  static int numachip_apic_id_registered(void)
>  {
>        return physid_isset(read_xapic_id(), phys_cpu_present_map);
> @@ -223,10 +229,11 @@ static int __init numachip_system_init(void)
>  }
>  early_initcall(numachip_system_init);
>
> -static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
> +static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>  {
>        if (!strncmp(oem_id, "NUMASC", 6)) {
>                numachip_system = 1;
> +               setup_force_cpu_cap(X86_FEATURE_X2APIC);
>                return 1;
>        }

can you check if you can update

!cpu_has_x2apic && (apic_id >= 0xff) && enabled

in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic()

to use some kind of apic_id_valid()

so you could avoid setting that feature bit.

the checking in SRAT could be removed.

Yinghai

Re: [PATCH] Move APIC ID validity check into platform APIC code

[Steffen Persvold]

On 3/14/2012 18:58, Yinghai Lu wrote:
> On Wed, Mar 14, 2012 at 12:17 AM, Daniel J Blueman
[]
>
> can you check if you can update
>
> !cpu_has_x2apic&&  (apic_id>= 0xff)&&  enabled
>
> in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic()
>
> to use some kind of apic_id_valid()
>
> so you could avoid setting that feature bit.
>
> the checking in SRAT could be removed.
>

Yinghai/Team,

One question (as I don't really know *why* this was added to the 
acpi/srat parsing code). In arch/x86/kernel/smpboot.c the check was 
originally :

!x2apic_mode && apicid >= 255

However in arch/x86/kernel/acpi/boot.c and arch/x86/mm/srat.c these 
tests are used :

!cpu_has_x2apic && (apic_id >= 0xff)

Clearly, "cpu_has_x2apic" and "x2apic_mode" are two different things.

Since we can force "cpu_has_x2apic", when Daniel crafted this patch he 
made the following "default" function :

static inline int default_apic_id_valid(int apicid)
{
	return x2apic_mode || (apicid < 255);
}

which, as you can see, checks against "x2apic_mode".

My question is; Is checking for "x2apic_mode" going to do the trick in 
the arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic() ?

If the answer is yes, the patch is going to be very simple. But we can't 
verify that the code in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic() 
actually triggers for the case you wanted it to trigger for because then 
it will check against "x2apic_mode" and not "cpu_has_x2apic".

Cheers,
-- 
Steffen Persvold, Chief Architect NumaChip
Numascale AS - www.numascale.com
Tel: +47 92 49 25 54 Skype: spersvold

Re: [PATCH] Move APIC ID validity check into platform APIC code

[Yinghai Lu]

On Wed, Mar 14, 2012 at 1:18 PM, Steffen Persvold <sp@numascale.com> wrote:
> which, as you can see, checks against "x2apic_mode".
>
> My question is; Is checking for "x2apic_mode" going to do the trick in the
> arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic() ?

no, it is too early.

at that time, only x2apic pre-enabled system have that x2apic_mode set.

>
> If the answer is yes, the patch is going to be very simple. But we can't
> verify that the code in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic()
> actually triggers for the case you wanted it to trigger for because then it
> will check against "x2apic_mode" and not "cpu_has_x2apic".

when kernel, found pre-enabled x2apic, can not use x2apic for some
reason (like x2apic)
it will clear x2apic in cpu feature bit.

for other case like x2apic capable cpus and not preenabled,
but not have dmar table, that feature is not cleared if the not apic id > 255.

So we have default apic_id_valid only check cpu_has_x2apic instead of
x2apic_mode instead.
and use it in boot.c

Yinghai

[PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Steffen Persvold]

Use x2apic_supported() in the default_apic_id_valid() function. If x2apic mode is disabled (via nox2apic for example), x2apic_supported() will return false.

This allows us to substitute the check in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the x2apic cpu feature in the NumaChip apic code.

Signed-off-by: Steffen Persvold <sp@numascale.com>
Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>

---
 arch/x86/include/asm/apic.h          |    2 +-
 arch/x86/kernel/acpi/boot.c          |    2 +-
 arch/x86/kernel/apic/apic_numachip.c |    1 -
 arch/x86/mm/srat.c                   |    7 +------
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index a9371c9..b6d3987 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(void)
 
 static inline int default_apic_id_valid(int apicid)
 {
-	return x2apic_mode || (apicid < 255);
+	return x2apic_supported() || (apicid < 255);
 }
 
 extern void default_setup_apic_routing(void);
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 406ed77..0f42c2f 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -239,7 +239,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	 * to not preallocating memory for all NR_CPUS
 	 * when we use CPU hotplug.
 	 */
-	if (!cpu_has_x2apic && (apic_id >= 0xff) && enabled)
+	if (!apic->apic_id_valid(apic_id) && enabled)
 		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
 	else
 		acpi_register_lapic(apic_id, enabled);
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index d9ea5f3..c13fa9f 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -233,7 +233,6 @@ static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_
 {
 	if (!strncmp(oem_id, "NUMASC", 6)) {
 		numachip_system = 1;
-		setup_force_cpu_cap(X86_FEATURE_X2APIC);
 		return 1;
 	}
 
diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 1c1c4f4..7efd0c6 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -69,12 +69,6 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	if ((pa->flags & ACPI_SRAT_CPU_ENABLED) == 0)
 		return;
 	pxm = pa->proximity_domain;
-	apic_id = pa->apic_id;
-	if (!cpu_has_x2apic && (apic_id >= 0xff)) {
-		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
-			 pxm, apic_id);
-		return;
-	}
 	node = setup_node(pxm);
 	if (node < 0) {
 		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
@@ -82,6 +76,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 		return;
 	}
 
+	apic_id = pa->apic_id;
 	if (apic_id >= MAX_LOCAL_APIC) {
 		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
 		return;
-- 
1.7.4.1

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Yinghai Lu]

On Thu, Mar 15, 2012 at 11:03 AM, Steffen Persvold <sp@numascale.com> wrote:
> Use x2apic_supported() in the default_apic_id_valid() function. If x2apic mode is disabled (via nox2apic for example), x2apic_supported() will return false.
>
> This allows us to substitute the check in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the x2apic cpu feature in the NumaChip apic code.
>
> Signed-off-by: Steffen Persvold <sp@numascale.com>
> Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>

I double checked on system with x2apic preenabled,
nox2apic in boot command line still works well and it
skips starting APs with apic id > 255.

Acked-by: Yinghai Lu <yinghai@kernel.org>

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Suresh Siddha]

On Thu, 2012-03-15 at 13:23 -0700, Yinghai Lu wrote:
> On Thu, Mar 15, 2012 at 11:03 AM, Steffen Persvold <sp@numascale.com> wrote:
> > Use x2apic_supported() in the default_apic_id_valid() function. If x2apic mode is disabled (via nox2apic for example), x2apic_supported() will return false.
> >
> > This allows us to substitute the check in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the x2apic cpu feature in the NumaChip apic code.
> >
> > Signed-off-by: Steffen Persvold <sp@numascale.com>
> > Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>
> 
> I double checked on system with x2apic preenabled,
> nox2apic in boot command line still works well and it
> skips starting APs with apic id > 255.
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>

This breaks the smpboot check if enabling interrupt-remapping/x2apic
fails on a platform. We will be in xapic mode and we don't clear the
x2apic cpufeature bit in this case and as such smpboot check will fail.

So this change breaks the commit
c284b42abadbb22083bfde24d308899c08d44ffa.

I think the right thing is to have two different apid_id_valid checks
one for xapic driver (apic_flat_64.c) and another for x2apic driver
(x2apic_phys/cluster.c) and that way, x2apic MADT entries will be parsed
only if bios has handed over the OS in x2apic mode or if we have
selected the numachip model.

thanks,
suresh

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Steffen Persvold]

On 3/15/12 22:21 , Suresh Siddha wrote:
> On Thu, 2012-03-15 at 13:23 -0700, Yinghai Lu wrote:
>> On Thu, Mar 15, 2012 at 11:03 AM, Steffen Persvold<sp@numascale.com>  wrote:
>>> Use x2apic_supported() in the default_apic_id_valid() function. If x2apic mode is disabled (via nox2apic for example), x2apic_supported() will return false.
>>>
>>> This allows us to substitute the check in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the x2apic cpu feature in the NumaChip apic code.
>>>
>>> Signed-off-by: Steffen Persvold<sp@numascale.com>
>>> Reviewed-by: Daniel J Blueman<daniel@numascale-asia.com>
>>
>> I double checked on system with x2apic preenabled,
>> nox2apic in boot command line still works well and it
>> skips starting APs with apic id>  255.
>>
>> Acked-by: Yinghai Lu<yinghai@kernel.org>
>

Suresh,

> This breaks the smpboot check if enabling interrupt-remapping/x2apic
> fails on a platform. We will be in xapic mode and we don't clear the
> x2apic cpufeature bit in this case and as such smpboot check will fail.
>
> So this change breaks the commit
> c284b42abadbb22083bfde24d308899c08d44ffa.
>

I was afraid of that.

> I think the right thing is to have two different apid_id_valid checks
> one for xapic driver (apic_flat_64.c) and another for x2apic driver
> (x2apic_phys/cluster.c) and that way, x2apic MADT entries will be parsed
> only if bios has handed over the OS in x2apic mode or if we have
> selected the numachip model.
>

Is my understanding of your suggestion correct that in 
x2apic_phys/cluster.c we add the following apic_id_valid() function :

static int x2apic_apic_id_valid(int apicid)
{
  	return x2apic_mode || (apicid < 255);
}

?

Considering that this function (apic->apic_id_valid()) is called already 
in the acpi/boot.c::acpi_parse_x2apic() function is it sufficient enough 
to test for x2apic_mode ? Yinghai indicated that x2apic_mode was not set 
at this point, thus it was testing cpu_has_x2apic instead ?

I must admit that I am not familiar enough with the APIC/ACPI code base 
to determine the sequence of events here (i.e MADT parsing, enabling of 
x2apic mode etc. etc.).

Please advice.

Kind regards,
Steffen

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Steffen Persvold]

On 3/15/12 23:34 , Steffen Persvold wrote:
> On 3/15/12 22:21 , Suresh Siddha wrote:
>> On Thu, 2012-03-15 at 13:23 -0700, Yinghai Lu wrote:
>>> On Thu, Mar 15, 2012 at 11:03 AM, Steffen Persvold<sp@numascale.com>
>>> wrote:
>>>> Use x2apic_supported() in the default_apic_id_valid() function. If
>>>> x2apic mode is disabled (via nox2apic for example),
>>>> x2apic_supported() will return false.
>>>>
>>>> This allows us to substitute the check in
>>>> arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning
>>>> the x2apic cpu feature in the NumaChip apic code.
>>>>
>>>> Signed-off-by: Steffen Persvold<sp@numascale.com>
>>>> Reviewed-by: Daniel J Blueman<daniel@numascale-asia.com>
>>>
>>> I double checked on system with x2apic preenabled,
>>> nox2apic in boot command line still works well and it
>>> skips starting APs with apic id> 255.
>>>
>>> Acked-by: Yinghai Lu<yinghai@kernel.org>
>>
>
> Suresh,
>
>> This breaks the smpboot check if enabling interrupt-remapping/x2apic
>> fails on a platform. We will be in xapic mode and we don't clear the
>> x2apic cpufeature bit in this case and as such smpboot check will fail.
>>
>> So this change breaks the commit
>> c284b42abadbb22083bfde24d308899c08d44ffa.
>>
>
> I was afraid of that.
>
>> I think the right thing is to have two different apid_id_valid checks
>> one for xapic driver (apic_flat_64.c) and another for x2apic driver
>> (x2apic_phys/cluster.c) and that way, x2apic MADT entries will be parsed
>> only if bios has handed over the OS in x2apic mode or if we have
>> selected the numachip model.
>>
>
> Is my understanding of your suggestion correct that in
> x2apic_phys/cluster.c we add the following apic_id_valid() function :
>
> static int x2apic_apic_id_valid(int apicid)
> {
> return x2apic_mode || (apicid < 255);
> }
>
> ?
>
> Considering that this function (apic->apic_id_valid()) is called already
> in the acpi/boot.c::acpi_parse_x2apic() function is it sufficient enough
> to test for x2apic_mode ? Yinghai indicated that x2apic_mode was not set
> at this point, thus it was testing cpu_has_x2apic instead ?
>
> I must admit that I am not familiar enough with the APIC/ACPI code base
> to determine the sequence of events here (i.e MADT parsing, enabling of
> x2apic mode etc. etc.).

After reading the code a bit more it seems that the sequence is as follows :

kernel/setup.c::setup_arch() calls check_x2apic(). check_x2apic() first 
checks the cpu feature flag, then checks the MSR_IA32_APICBASE msr to 
see if bios has enabled x2apic mode. If this is the case, 
x2apic_preenabled and x2apic_mode is set to 1.

Later on in setup_arch(), the ACPI parsing starts.

My assumption is that the approach suggested in my previous email (based 
on Suresh' comment) with separate apic_id_valid() functions would be 
sufficient even for the MADT parsing ?

Kind regards,
Steffen

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Suresh Siddha]

On Thu, 2012-03-15 at 23:34 +0100, Steffen Persvold wrote:
> Is my understanding of your suggestion correct that in 
> x2apic_phys/cluster.c we add the following apic_id_valid() function :
> 
> static int x2apic_apic_id_valid(int apicid)
> {
>   	return x2apic_mode || (apicid < 255);
> }

Steffen, We can have something like:

static int x2apic_apic_id_valid(int apicid)
{
   	return 1;
}

and

static int xapic_apic_id_valid(int apicid)
{
   	return apicid < 255;
}

If we have selected x2apic driver, then we know we are already in x2apic
mode. And also x2apic_uv_x need to use the x2apic version above.

> Considering that this function (apic->apic_id_valid()) is called already 
> in the acpi/boot.c::acpi_parse_x2apic() function is it sufficient enough 
> to test for x2apic_mode ? Yinghai indicated that x2apic_mode was not set 
> at this point, thus it was testing cpu_has_x2apic instead ?

If the bios has handed over to us in x2apic mode (or if it is a numachip
platform), then by this point apic driver is already set to the
corresponding x2apic/numachip driver etc. so we should be fine.

When we are in xapic mode, typically there should be no x2apic MADT
entries. And even if there are any (bios not following x2apic spec), the
above xapic_apic_id_valid() check will consider only those x2apic MADT
entries whose id's are less than 255. xapic mode can go into x2apic mode
later but that flow is not supposed to bring up any cpu with apic id >
255. So parsing only entries with apic id < 255 here should be fine.

Hope this clarifies.

thanks,
suresh

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Steffen Persvold]

On 3/16/2012 00:04, Suresh Siddha wrote:
> On Thu, 2012-03-15 at 23:34 +0100, Steffen Persvold wrote:
>> Is my understanding of your suggestion correct that in
>> x2apic_phys/cluster.c we add the following apic_id_valid() function :
>>
>> static int x2apic_apic_id_valid(int apicid)
>> {
>>    	return x2apic_mode || (apicid<  255);
>> }
>
> Steffen, We can have something like:
>
> static int x2apic_apic_id_valid(int apicid)
> {
>     	return 1;
> }
>
> and
>
> static int xapic_apic_id_valid(int apicid)
> {
>     	return apicid<  255;
> }
>
> If we have selected x2apic driver, then we know we are already in x2apic
> mode. And also x2apic_uv_x need to use the x2apic version above.
>
>> Considering that this function (apic->apic_id_valid()) is called already
>> in the acpi/boot.c::acpi_parse_x2apic() function is it sufficient enough
>> to test for x2apic_mode ? Yinghai indicated that x2apic_mode was not set
>> at this point, thus it was testing cpu_has_x2apic instead ?
>
> If the bios has handed over to us in x2apic mode (or if it is a numachip
> platform), then by this point apic driver is already set to the
> corresponding x2apic/numachip driver etc. so we should be fine.
>
> When we are in xapic mode, typically there should be no x2apic MADT
> entries. And even if there are any (bios not following x2apic spec), the
> above xapic_apic_id_valid() check will consider only those x2apic MADT
> entries whose id's are less than 255. xapic mode can go into x2apic mode
> later but that flow is not supposed to bring up any cpu with apic id>
> 255. So parsing only entries with apic id<  255 here should be fine.
>
> Hope this clarifies.
>

Yes, this is now my understanding aswell. Thank you.

I will resend a reviced patch shortly.

Kind regards,
-- 
Steffen Persvold, Chief Architect NumaChip
Numascale AS - www.numascale.com
Tel: +47 92 49 25 54 Skype: spersvold

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Steffen Persvold]

On 3/16/2012 00:04, Suresh Siddha wrote:
> On Thu, 2012-03-15 at 23:34 +0100, Steffen Persvold wrote:
>> Is my understanding of your suggestion correct that in
>> x2apic_phys/cluster.c we add the following apic_id_valid() function :
>>
>> static int x2apic_apic_id_valid(int apicid)
>> {
>>    	return x2apic_mode || (apicid<  255);
>> }
>
> Steffen, We can have something like:
>
> static int x2apic_apic_id_valid(int apicid)
> {
>     	return 1;
> }
>
> and
>
> static int xapic_apic_id_valid(int apicid)
> {
>     	return apicid<  255;
> }
>
> If we have selected x2apic driver, then we know we are already in x2apic
> mode. And also x2apic_uv_x need to use the x2apic version above.
>

If you specify "nox2apic" option, will it choose the xapic driver 
instead (and early enough) ? Otherwise I think we might break Yinghai's 
commit (a35fd28256e7736cc84af8931a16224f0bfaaf6c).

Cheers,
-- 
Steffen Persvold, Chief Architect NumaChip
Numascale AS - www.numascale.com
Tel: +47 92 49 25 54 Skype: spersvold

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Steffen Persvold]

On 3/16/2012 00:33, Steffen Persvold wrote:
> On 3/16/2012 00:04, Suresh Siddha wrote:
>> On Thu, 2012-03-15 at 23:34 +0100, Steffen Persvold wrote:
>>> Is my understanding of your suggestion correct that in
>>> x2apic_phys/cluster.c we add the following apic_id_valid() function :
>>>
>>> static int x2apic_apic_id_valid(int apicid)
>>> {
>>> return x2apic_mode || (apicid< 255);
>>> }
>>
>> Steffen, We can have something like:
>>
>> static int x2apic_apic_id_valid(int apicid)
>> {
>> return 1;
>> }
>>
>> and
>>
>> static int xapic_apic_id_valid(int apicid)
>> {
>> return apicid< 255;
>> }
>>
>> If we have selected x2apic driver, then we know we are already in x2apic
>> mode. And also x2apic_uv_x need to use the x2apic version above.
>>
>
> If you specify "nox2apic" option, will it choose the xapic driver
> instead (and early enough) ? Otherwise I think we might break Yinghai's
> commit (a35fd28256e7736cc84af8931a16224f0bfaaf6c).
>

Answering myself here, my apologies, but yes it looks like both x2apic 
drivers will be de-activated if the "nox2apic" option is specified 
(x2apic_enabled() will return false).


Cheers,
Steffen

[PATCH] Added separate apic_id_valid() functions for selected apic drivers

[Steffen Persvold]

This allows us to substitute the check in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the x2apic cpu feature in the NumaChip apic code.

The following apic drivers have separate apic_id_valid() functions which will accept x2apic type IDs :

x2apic_phys
x2apic_cluster
x2apic_uv_x
apic_numachip

Signed-off-by: Steffen Persvold <sp@numascale.com>
---
 arch/x86/include/asm/apic.h           |    2 +-
 arch/x86/include/asm/x2apic.h         |    5 +++++
 arch/x86/kernel/acpi/boot.c           |    2 +-
 arch/x86/kernel/apic/apic_numachip.c  |    3 +--
 arch/x86/kernel/apic/x2apic_cluster.c |    2 +-
 arch/x86/kernel/apic/x2apic_phys.c    |    2 +-
 arch/x86/kernel/apic/x2apic_uv_x.c    |    7 ++++++-
 arch/x86/mm/srat.c                    |    7 +------
 8 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index a9371c9..d3eaac4 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(void)
 
 static inline int default_apic_id_valid(int apicid)
 {
-	return x2apic_mode || (apicid < 255);
+	return (apicid < 255);
 }
 
 extern void default_setup_apic_routing(void);
diff --git a/arch/x86/include/asm/x2apic.h b/arch/x86/include/asm/x2apic.h
index 6bf5b8e..92e54ab 100644
--- a/arch/x86/include/asm/x2apic.h
+++ b/arch/x86/include/asm/x2apic.h
@@ -18,6 +18,11 @@ static const struct cpumask *x2apic_target_cpus(void)
 	return cpu_online_mask;
 }
 
+static int x2apic_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int x2apic_apic_id_registered(void)
 {
 	return 1;
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 406ed77..0f42c2f 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -239,7 +239,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	 * to not preallocating memory for all NR_CPUS
 	 * when we use CPU hotplug.
 	 */
-	if (!cpu_has_x2apic && (apic_id >= 0xff) && enabled)
+	if (!apic->apic_id_valid(apic_id) && enabled)
 		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
 	else
 		acpi_register_lapic(apic_id, enabled);
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index d9ea5f3..899803e 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -229,11 +229,10 @@ static int __init numachip_system_init(void)
 }
 early_initcall(numachip_system_init);
 
-static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	if (!strncmp(oem_id, "NUMASC", 6)) {
 		numachip_system = 1;
-		setup_force_cpu_cap(X86_FEATURE_X2APIC);
 		return 1;
 	}
 
diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index 9193713..48f3103 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -213,7 +213,7 @@ static struct apic apic_x2apic_cluster = {
 	.name				= "cluster x2apic",
 	.probe				= x2apic_cluster_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index bcd1db6..8a778db 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -119,7 +119,7 @@ static struct apic apic_x2apic_phys = {
 	.name				= "physical x2apic",
 	.probe				= x2apic_phys_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index fc47714..87bfa69 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -266,6 +266,11 @@ static void uv_send_IPI_all(int vector)
 	uv_send_IPI_mask(cpu_online_mask, vector);
 }
 
+static int uv_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int uv_apic_id_registered(void)
 {
 	return 1;
@@ -351,7 +356,7 @@ static struct apic __refdata apic_x2apic_uv_x = {
 	.name				= "UV large system",
 	.probe				= uv_probe,
 	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= uv_apic_id_valid,
 	.apic_id_registered		= uv_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 1c1c4f4..7efd0c6 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -69,12 +69,6 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	if ((pa->flags & ACPI_SRAT_CPU_ENABLED) == 0)
 		return;
 	pxm = pa->proximity_domain;
-	apic_id = pa->apic_id;
-	if (!cpu_has_x2apic && (apic_id >= 0xff)) {
-		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
-			 pxm, apic_id);
-		return;
-	}
 	node = setup_node(pxm);
 	if (node < 0) {
 		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
@@ -82,6 +76,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 		return;
 	}
 
+	apic_id = pa->apic_id;
 	if (apic_id >= MAX_LOCAL_APIC) {
 		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
 		return;
-- 
1.7.4.1

Re: [PATCH] Added separate apic_id_valid() functions for selected apic drivers

[Suresh Siddha]

On Fri, 2012-03-16 at 01:07 +0100, Steffen Persvold wrote:
> diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
> index 1c1c4f4..7efd0c6 100644
> --- a/arch/x86/mm/srat.c
> +++ b/arch/x86/mm/srat.c
> @@ -69,12 +69,6 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
>  	if ((pa->flags & ACPI_SRAT_CPU_ENABLED) == 0)
>  		return;
>  	pxm = pa->proximity_domain;
> -	apic_id = pa->apic_id;
> -	if (!cpu_has_x2apic && (apic_id >= 0xff)) {

not sure why you removed this. Shouldn't this be replaced with
apic->apic_id_valid() check?

thanks.

> -		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
> -			 pxm, apic_id);
> -		return;
> -	}
>  	node = setup_node(pxm);
>  	if (node < 0) {
>  		printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm);
> @@ -82,6 +76,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
>  		return;
>  	}
>  
> +	apic_id = pa->apic_id;
>  	if (apic_id >= MAX_LOCAL_APIC) {
>  		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
>  		return;

Re: [PATCH] Added separate apic_id_valid() functions for selected apic drivers

[Yinghai Lu]

On Thu, Mar 15, 2012 at 5:13 PM, Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> On Fri, 2012-03-16 at 01:07 +0100, Steffen Persvold wrote:
>> diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
>> index 1c1c4f4..7efd0c6 100644
>> --- a/arch/x86/mm/srat.c
>> +++ b/arch/x86/mm/srat.c
>> @@ -69,12 +69,6 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
>>       if ((pa->flags & ACPI_SRAT_CPU_ENABLED) == 0)
>>               return;
>>       pxm = pa->proximity_domain;
>> -     apic_id = pa->apic_id;
>> -     if (!cpu_has_x2apic && (apic_id >= 0xff)) {
>
> not sure why you removed this. Shouldn't this be replaced with
> apic->apic_id_valid() check?
>

yes,  it should change to apic->apic_id_valid().

in arch/x86/kernel/setup.c,  early_acpi_boot_init() calling will probe
and install right apic.

Yinghai

Re: [PATCH] Added separate apic_id_valid() functions for selected apic drivers

[Steffen Persvold]

On 3/16/2012 01:13, Suresh Siddha wrote:
> On Fri, 2012-03-16 at 01:07 +0100, Steffen Persvold wrote:
>> diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
>> index 1c1c4f4..7efd0c6 100644
>> --- a/arch/x86/mm/srat.c
>> +++ b/arch/x86/mm/srat.c
>> @@ -69,12 +69,6 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
>>   	if ((pa->flags&  ACPI_SRAT_CPU_ENABLED) == 0)
>>   		return;
>>   	pxm = pa->proximity_domain;
>> -	apic_id = pa->apic_id;
>> -	if (!cpu_has_x2apic&&  (apic_id>= 0xff)) {
>
> not sure why you removed this. Shouldn't this be replaced with
> apic->apic_id_valid() check?
>

Well I removed it in both my patches because Yinghai stated :

 >>>the checking in SRAT could be removed.
 >>>
 >>>Yinghai

a couple of emails ago. I could of course use apic->apic_id_valid() here 
too to avoid parsing the PXM.

Cheers,
-- 
Steffen Persvold, Chief Architect NumaChip
Numascale AS - www.numascale.com
Tel: +47 92 49 25 54 Skype: spersvold

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Yinghai Lu]

On Thu, Mar 15, 2012 at 2:21 PM, Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> On Thu, 2012-03-15 at 13:23 -0700, Yinghai Lu wrote:
>> On Thu, Mar 15, 2012 at 11:03 AM, Steffen Persvold <sp@numascale.com> wrote:
>> > Use x2apic_supported() in the default_apic_id_valid() function. If x2apic mode is disabled (via nox2apic for example), x2apic_supported() will return false.
>> >
>> > This allows us to substitute the check in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the x2apic cpu feature in the NumaChip apic code.
>> >
>> > Signed-off-by: Steffen Persvold <sp@numascale.com>
>> > Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>
>>
>> I double checked on system with x2apic preenabled,
>> nox2apic in boot command line still works well and it
>> skips starting APs with apic id > 255.
>>
>> Acked-by: Yinghai Lu <yinghai@kernel.org>
>
> This breaks the smpboot check if enabling interrupt-remapping/x2apic
> fails on a platform. We will be in xapic mode and we don't clear the
> x2apic cpufeature bit in this case and as such smpboot check will fail.

two cases:
A: x2apic is pre-enabled, and some apic id > 255
B: x2apic is not pre-enabled, but x2apic capable, suppose all apic id < 255.

in case A: if nox2apic is passed, or dmar table is not there or
intr-remap can not be enabled.
   that feature bit will be cleared. So check cpu_has_x2apic is enough there.

in case B: if can not enable x2apic, the feature is not cleared, but
apic id already < 255.
   so that check will pass too.

So it should not break the smpboot check.

>
> So this change breaks the commit
> c284b42abadbb22083bfde24d308899c08d44ffa.
>
> I think the right thing is to have two different apid_id_valid checks
> one for xapic driver (apic_flat_64.c) and another for x2apic driver
> (x2apic_phys/cluster.c) and that way, x2apic MADT entries will be parsed
> only if bios has handed over the OS in x2apic mode or if we have
> selected the numachip model.

that looks like more clear.

Thanks

Yinghai

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Yinghai Lu]

On Thu, Mar 15, 2012 at 7:08 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Mar 15, 2012 at 2:21 PM, Suresh Siddha
> <suresh.b.siddha@intel.com> wrote:
>> On Thu, 2012-03-15 at 13:23 -0700, Yinghai Lu wrote:
>>> On Thu, Mar 15, 2012 at 11:03 AM, Steffen Persvold <sp@numascale.com> wrote:
>>> > Use x2apic_supported() in the default_apic_id_valid() function. If x2apic mode is disabled (via nox2apic for example), x2apic_supported() will return false.
>>> >
>>> > This allows us to substitute the check in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the x2apic cpu feature in the NumaChip apic code.
>>> >
>>> > Signed-off-by: Steffen Persvold <sp@numascale.com>
>>> > Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>
>>>
>>> I double checked on system with x2apic preenabled,
>>> nox2apic in boot command line still works well and it
>>> skips starting APs with apic id > 255.
>>>
>>> Acked-by: Yinghai Lu <yinghai@kernel.org>
>>
>> This breaks the smpboot check if enabling interrupt-remapping/x2apic
>> fails on a platform. We will be in xapic mode and we don't clear the
>> x2apic cpufeature bit in this case and as such smpboot check will fail.
>
> two cases:
> A: x2apic is pre-enabled, and some apic id > 255
> B: x2apic is not pre-enabled, but x2apic capable, suppose all apic id < 255.
>
> in case A: if nox2apic is passed, or dmar table is not there or
> intr-remap can not be enabled.
>   that feature bit will be cleared. So check cpu_has_x2apic is enough there.
>
> in case B: if can not enable x2apic, the feature is not cleared, but
> apic id already < 255.
>   so that check will pass too.
>
> So it should not break the smpboot check.
>
>>
>> So this change breaks the commit
>> c284b42abadbb22083bfde24d308899c08d44ffa.

Actually before I sent out acked-by, I did test the patch on my 8
sockets x2apic pre-enabled system with nox2apic.

It booted well while skipping all cpu with apic id > 255.

Thanks

Yinghai

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Yinghai Lu]

On Thu, Mar 15, 2012 at 7:08 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> So this change breaks the commit
>> c284b42abadbb22083bfde24d308899c08d44ffa.
>>
>> I think the right thing is to have two different apid_id_valid checks
>> one for xapic driver (apic_flat_64.c) and another for x2apic driver
>> (x2apic_phys/cluster.c) and that way, x2apic MADT entries will be parsed
>> only if bios has handed over the OS in x2apic mode or if we have
>> selected the numachip model.
>
> that looks like more clear.

after more thinking, I think We should still use cpu_has_x2apic checking.

one maybe invalid case:

System have some cpus apic id < 255, and some cpu apic id > 255.
BSP apic id < 255.
those cpus apic id < 255 will be put into xapic mode, cpus > 255 will
be put into x2apic mode.
and DMAR table intr-remapping will be working.

So if we check x2apic_mode early, will skip cpu with apic id > 255,
even switch to x2apic later.

Thanks

Yinghai Lu

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Steffen Persvold]

On 3/16/2012 05:19, Yinghai Lu wrote:
> On Thu, Mar 15, 2012 at 7:08 PM, Yinghai Lu<yinghai@kernel.org>  wrote:
>>> So this change breaks the commit
>>> c284b42abadbb22083bfde24d308899c08d44ffa.
>>>
>>> I think the right thing is to have two different apid_id_valid checks
>>> one for xapic driver (apic_flat_64.c) and another for x2apic driver
>>> (x2apic_phys/cluster.c) and that way, x2apic MADT entries will be parsed
>>> only if bios has handed over the OS in x2apic mode or if we have
>>> selected the numachip model.
>>
>> that looks like more clear.
>
> after more thinking, I think We should still use cpu_has_x2apic checking.
>
> one maybe invalid case:
>
> System have some cpus apic id<  255, and some cpu apic id>  255.
> BSP apic id<  255.
> those cpus apic id<  255 will be put into xapic mode, cpus>  255 will
> be put into x2apic mode.
> and DMAR table intr-remapping will be working.

Hmm, I didn't know you could have two apic drivers (e.g. apic_flat_64 
and x2apic_*) available at the same time ? Or did I read the above wrong ?

>
> So if we check x2apic_mode early, will skip cpu with apic id>  255,
> even switch to x2apic later.
>

Which one of the two patches I sent, do you (Suresh/Yinghai/others) 
believe is the best/cleanest and works in all cases. I, unfortunately, 
can't test the Intel case as I don't have any available to test on :/

Either patch works fine for NumaChip enabled systems.

If desired I will re-post the patch with the approach you find best, but 
add the apic->apic_id_valid() check in the SRAT code aswell.

Cheers,
-- 
Steffen Persvold, Chief Architect NumaChip
Numascale AS - www.numascale.com
Tel: +47 92 49 25 54 Skype: spersvold

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Yinghai Lu]

On Thu, Mar 15, 2012 at 11:56 PM, Steffen Persvold <sp@numascale.com> wrote:
> Which one of the two patches I sent, do you (Suresh/Yinghai/others) believe
> is the best/cleanest and works in all cases. I, unfortunately, can't test
> the Intel case as I don't have any available to test on :/
>
> Either patch works fine for NumaChip enabled systems.
>
> If desired I will re-post the patch with the approach you find best, but add
> the apic->apic_id_valid() check in the SRAT code aswell.

Ok, let use second way that Suresh proposed, please repost that
without removing checking in srat.

should like:

From: Steffen Persvold <sp@numascale.com>
Subject: [PATCH] Added separate apic_id_valid() functions for selected
apic drivers

This allows us to substitute the check in
arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the
x2apic cpu feature in the NumaChip apic code.

The following apic drivers have separate apic_id_valid() functions
which will accept x2apic type IDs :

x2apic_phys
x2apic_cluster
x2apic_uv_x
apic_numachip

Signed-off-by: Steffen Persvold <sp@numascale.com>
---
 arch/x86/include/asm/apic.h           |    2 +-
 arch/x86/include/asm/x2apic.h         |    5 +++++
 arch/x86/kernel/acpi/boot.c           |    2 +-
 arch/x86/kernel/apic/apic_numachip.c  |    3 +--
 arch/x86/kernel/apic/x2apic_cluster.c |    2 +-
 arch/x86/kernel/apic/x2apic_phys.c    |    2 +-
 arch/x86/kernel/apic/x2apic_uv_x.c    |    7 ++++++-
 arch/x86/mm/srat.c                    |    2 +-
 8 files changed, 17 insertions(+), 8 deletions(-)

Index: linux-2.6/arch/x86/include/asm/apic.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/apic.h
+++ linux-2.6/arch/x86/include/asm/apic.h
@@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(

 static inline int default_apic_id_valid(int apicid)
 {
-	return x2apic_mode || (apicid < 255);
+	return (apicid < 255);
 }

 extern void default_setup_apic_routing(void);
Index: linux-2.6/arch/x86/include/asm/x2apic.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/x2apic.h
+++ linux-2.6/arch/x86/include/asm/x2apic.h
@@ -18,6 +18,11 @@ static const struct cpumask *x2apic_targ
 	return cpu_online_mask;
 }

+static int x2apic_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int x2apic_apic_id_registered(void)
 {
 	return 1;
Index: linux-2.6/arch/x86/kernel/acpi/boot.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/acpi/boot.c
+++ linux-2.6/arch/x86/kernel/acpi/boot.c
@@ -227,7 +227,7 @@ acpi_parse_x2apic(struct acpi_subtable_h
 	 * to not preallocating memory for all NR_CPUS
 	 * when we use CPU hotplug.
 	 */
-	if (!cpu_has_x2apic && (apic_id >= 0xff) && enabled)
+	if (!apic->apic_id_valid(apic_id) && enabled)
 		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
 	else
 		acpi_register_lapic(apic_id, enabled);
Index: linux-2.6/arch/x86/kernel/apic/apic_numachip.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/apic_numachip.c
+++ linux-2.6/arch/x86/kernel/apic/apic_numachip.c
@@ -229,11 +229,10 @@ static int __init numachip_system_init(v
 }
 early_initcall(numachip_system_init);

-static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char
*oem_table_id)
+static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	if (!strncmp(oem_id, "NUMASC", 6)) {
 		numachip_system = 1;
-		setup_force_cpu_cap(X86_FEATURE_X2APIC);
 		return 1;
 	}

Index: linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/x2apic_cluster.c
+++ linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
@@ -218,7 +218,7 @@ static struct apic apic_x2apic_cluster =
 	.name				= "cluster x2apic",
 	.probe				= x2apic_cluster_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,

 	.irq_delivery_mode		= dest_LowestPrio,
Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
+++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
@@ -119,7 +119,7 @@ static struct apic apic_x2apic_phys = {
 	.name				= "physical x2apic",
 	.probe				= x2apic_phys_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,

 	.irq_delivery_mode		= dest_Fixed,
Index: linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/x2apic_uv_x.c
+++ linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -266,6 +266,11 @@ static void uv_send_IPI_all(int vector)
 	uv_send_IPI_mask(cpu_online_mask, vector);
 }

+static int uv_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int uv_apic_id_registered(void)
 {
 	return 1;
@@ -351,7 +356,7 @@ static struct apic __refdata apic_x2apic
 	.name				= "UV large system",
 	.probe				= uv_probe,
 	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= uv_apic_id_valid,
 	.apic_id_registered		= uv_apic_id_registered,

 	.irq_delivery_mode		= dest_Fixed,
Index: linux-2.6/arch/x86/mm/srat.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/srat.c
+++ linux-2.6/arch/x86/mm/srat.c
@@ -70,7 +70,7 @@ acpi_numa_x2apic_affinity_init(struct ac
 		return;
 	pxm = pa->proximity_domain;
 	apic_id = pa->apic_id;
-	if (!cpu_has_x2apic && (apic_id >= 0xff)) {
+	if (!apic->apic_id_valid(apic_id)) {
 		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
 			 pxm, apic_id);
 		return;

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Suresh Siddha]

On Fri, 2012-03-16 at 09:57 -0700, Yinghai Lu wrote:
> On Thu, Mar 15, 2012 at 11:56 PM, Steffen Persvold <sp@numascale.com> wrote:
> > Which one of the two patches I sent, do you (Suresh/Yinghai/others) believe
> > is the best/cleanest and works in all cases. I, unfortunately, can't test
> > the Intel case as I don't have any available to test on :/
> >
> > Either patch works fine for NumaChip enabled systems.
> >
> > If desired I will re-post the patch with the approach you find best, but add
> > the apic->apic_id_valid() check in the SRAT code aswell.
> 
> Ok, let use second way that Suresh proposed, please repost that
> without removing checking in srat.
> 
> should like:
> 
> From: Steffen Persvold <sp@numascale.com>
> Subject: [PATCH] Added separate apic_id_valid() functions for selected
> apic drivers
> 
> This allows us to substitute the check in
> arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the
> x2apic cpu feature in the NumaChip apic code.
> 
> The following apic drivers have separate apic_id_valid() functions
> which will accept x2apic type IDs :
> 
> x2apic_phys
> x2apic_cluster
> x2apic_uv_x
> apic_numachip
> 
> Signed-off-by: Steffen Persvold <sp@numascale.com>

I have no complaints with this version ;)

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

> ---
>  arch/x86/include/asm/apic.h           |    2 +-
>  arch/x86/include/asm/x2apic.h         |    5 +++++
>  arch/x86/kernel/acpi/boot.c           |    2 +-
>  arch/x86/kernel/apic/apic_numachip.c  |    3 +--
>  arch/x86/kernel/apic/x2apic_cluster.c |    2 +-
>  arch/x86/kernel/apic/x2apic_phys.c    |    2 +-
>  arch/x86/kernel/apic/x2apic_uv_x.c    |    7 ++++++-
>  arch/x86/mm/srat.c                    |    2 +-
>  8 files changed, 17 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6/arch/x86/include/asm/apic.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/apic.h
> +++ linux-2.6/arch/x86/include/asm/apic.h
> @@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(
> 
>  static inline int default_apic_id_valid(int apicid)
>  {
> -	return x2apic_mode || (apicid < 255);
> +	return (apicid < 255);
>  }
> 
>  extern void default_setup_apic_routing(void);
> Index: linux-2.6/arch/x86/include/asm/x2apic.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/x2apic.h
> +++ linux-2.6/arch/x86/include/asm/x2apic.h
> @@ -18,6 +18,11 @@ static const struct cpumask *x2apic_targ
>  	return cpu_online_mask;
>  }
> 
> +static int x2apic_apic_id_valid(int apicid)
> +{
> +	return 1;
> +}
> +
>  static int x2apic_apic_id_registered(void)
>  {
>  	return 1;
> Index: linux-2.6/arch/x86/kernel/acpi/boot.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c
> +++ linux-2.6/arch/x86/kernel/acpi/boot.c
> @@ -227,7 +227,7 @@ acpi_parse_x2apic(struct acpi_subtable_h
>  	 * to not preallocating memory for all NR_CPUS
>  	 * when we use CPU hotplug.
>  	 */
> -	if (!cpu_has_x2apic && (apic_id >= 0xff) && enabled)
> +	if (!apic->apic_id_valid(apic_id) && enabled)
>  		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
>  	else
>  		acpi_register_lapic(apic_id, enabled);
> Index: linux-2.6/arch/x86/kernel/apic/apic_numachip.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/apic_numachip.c
> +++ linux-2.6/arch/x86/kernel/apic/apic_numachip.c
> @@ -229,11 +229,10 @@ static int __init numachip_system_init(v
>  }
>  early_initcall(numachip_system_init);
> 
> -static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char
> *oem_table_id)
> +static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>  {
>  	if (!strncmp(oem_id, "NUMASC", 6)) {
>  		numachip_system = 1;
> -		setup_force_cpu_cap(X86_FEATURE_X2APIC);
>  		return 1;
>  	}
> 
> Index: linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_cluster.c
> +++ linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
> @@ -218,7 +218,7 @@ static struct apic apic_x2apic_cluster =
>  	.name				= "cluster x2apic",
>  	.probe				= x2apic_cluster_probe,
>  	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
> -	.apic_id_valid			= default_apic_id_valid,
> +	.apic_id_valid			= x2apic_apic_id_valid,
>  	.apic_id_registered		= x2apic_apic_id_registered,
> 
>  	.irq_delivery_mode		= dest_LowestPrio,
> Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
> +++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
> @@ -119,7 +119,7 @@ static struct apic apic_x2apic_phys = {
>  	.name				= "physical x2apic",
>  	.probe				= x2apic_phys_probe,
>  	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
> -	.apic_id_valid			= default_apic_id_valid,
> +	.apic_id_valid			= x2apic_apic_id_valid,
>  	.apic_id_registered		= x2apic_apic_id_registered,
> 
>  	.irq_delivery_mode		= dest_Fixed,
> Index: linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -266,6 +266,11 @@ static void uv_send_IPI_all(int vector)
>  	uv_send_IPI_mask(cpu_online_mask, vector);
>  }
> 
> +static int uv_apic_id_valid(int apicid)
> +{
> +	return 1;
> +}
> +
>  static int uv_apic_id_registered(void)
>  {
>  	return 1;
> @@ -351,7 +356,7 @@ static struct apic __refdata apic_x2apic
>  	.name				= "UV large system",
>  	.probe				= uv_probe,
>  	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
> -	.apic_id_valid			= default_apic_id_valid,
> +	.apic_id_valid			= uv_apic_id_valid,
>  	.apic_id_registered		= uv_apic_id_registered,
> 
>  	.irq_delivery_mode		= dest_Fixed,
> Index: linux-2.6/arch/x86/mm/srat.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/mm/srat.c
> +++ linux-2.6/arch/x86/mm/srat.c
> @@ -70,7 +70,7 @@ acpi_numa_x2apic_affinity_init(struct ac
>  		return;
>  	pxm = pa->proximity_domain;
>  	apic_id = pa->apic_id;
> -	if (!cpu_has_x2apic && (apic_id >= 0xff)) {
> +	if (!apic->apic_id_valid(apic_id)) {
>  		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
>  			 pxm, apic_id);
>  		return;

Re: [PATCH] Use x2apic_supported() in the default_apic_id_valid() function.

[Yinghai Lu]

On Fri, Mar 16, 2012 at 11:01 AM, Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> On Fri, 2012-03-16 at 09:57 -0700, Yinghai Lu wrote:
>> On Thu, Mar 15, 2012 at 11:56 PM, Steffen Persvold <sp@numascale.com> wrote:
>> > Which one of the two patches I sent, do you (Suresh/Yinghai/others) believe
>> > is the best/cleanest and works in all cases. I, unfortunately, can't test
>> > the Intel case as I don't have any available to test on :/
>> >
>> > Either patch works fine for NumaChip enabled systems.
>> >
>> > If desired I will re-post the patch with the approach you find best, but add
>> > the apic->apic_id_valid() check in the SRAT code aswell.
>>
>> Ok, let use second way that Suresh proposed, please repost that
>> without removing checking in srat.
>>
>> should like:
>>
>> From: Steffen Persvold <sp@numascale.com>
>> Subject: [PATCH] Added separate apic_id_valid() functions for selected
>> apic drivers
>>
>> This allows us to substitute the check in
>> arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic and avoid feigning the
>> x2apic cpu feature in the NumaChip apic code.
>>
>> The following apic drivers have separate apic_id_valid() functions
>> which will accept x2apic type IDs :
>>
>> x2apic_phys
>> x2apic_cluster
>> x2apic_uv_x
>> apic_numachip
>>
>> Signed-off-by: Steffen Persvold <sp@numascale.com>
>
> I have no complaints with this version ;)
>
> Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

good,

also we could add more lines in changelog:

for x2apic pre-enabled system, apic driver is set already early through
  early_acpi_boot_init()/early_acpi_process_madt()/acpi_parse_madt()/default_acpi_madt_oem_check()
so that apic_id_valid() checking will let entry go during MADT and SRAT prasing.

for x2apic not pre-enabled system, all apic id should be less than 255.

Acked-by: Yinghai Lu <yinghai@kernel.org>

>
>> ---
>>  arch/x86/include/asm/apic.h           |    2 +-
>>  arch/x86/include/asm/x2apic.h         |    5 +++++
>>  arch/x86/kernel/acpi/boot.c           |    2 +-
>>  arch/x86/kernel/apic/apic_numachip.c  |    3 +--
>>  arch/x86/kernel/apic/x2apic_cluster.c |    2 +-
>>  arch/x86/kernel/apic/x2apic_phys.c    |    2 +-
>>  arch/x86/kernel/apic/x2apic_uv_x.c    |    7 ++++++-
>>  arch/x86/mm/srat.c                    |    2 +-
>>  8 files changed, 17 insertions(+), 8 deletions(-)
>>
>> Index: linux-2.6/arch/x86/include/asm/apic.h
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/include/asm/apic.h
>> +++ linux-2.6/arch/x86/include/asm/apic.h
>> @@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(
>>
>>  static inline int default_apic_id_valid(int apicid)
>>  {
>> -     return x2apic_mode || (apicid < 255);
>> +     return (apicid < 255);
>>  }
>>
>>  extern void default_setup_apic_routing(void);
>> Index: linux-2.6/arch/x86/include/asm/x2apic.h
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/include/asm/x2apic.h
>> +++ linux-2.6/arch/x86/include/asm/x2apic.h
>> @@ -18,6 +18,11 @@ static const struct cpumask *x2apic_targ
>>       return cpu_online_mask;
>>  }
>>
>> +static int x2apic_apic_id_valid(int apicid)
>> +{
>> +     return 1;
>> +}
>> +
>>  static int x2apic_apic_id_registered(void)
>>  {
>>       return 1;
>> Index: linux-2.6/arch/x86/kernel/acpi/boot.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c
>> +++ linux-2.6/arch/x86/kernel/acpi/boot.c
>> @@ -227,7 +227,7 @@ acpi_parse_x2apic(struct acpi_subtable_h
>>        * to not preallocating memory for all NR_CPUS
>>        * when we use CPU hotplug.
>>        */
>> -     if (!cpu_has_x2apic && (apic_id >= 0xff) && enabled)
>> +     if (!apic->apic_id_valid(apic_id) && enabled)
>>               printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
>>       else
>>               acpi_register_lapic(apic_id, enabled);
>> Index: linux-2.6/arch/x86/kernel/apic/apic_numachip.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/apic/apic_numachip.c
>> +++ linux-2.6/arch/x86/kernel/apic/apic_numachip.c
>> @@ -229,11 +229,10 @@ static int __init numachip_system_init(v
>>  }
>>  early_initcall(numachip_system_init);
>>
>> -static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char
>> *oem_table_id)
>> +static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>>  {
>>       if (!strncmp(oem_id, "NUMASC", 6)) {
>>               numachip_system = 1;
>> -             setup_force_cpu_cap(X86_FEATURE_X2APIC);
>>               return 1;
>>       }
>>
>> Index: linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_cluster.c
>> +++ linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
>> @@ -218,7 +218,7 @@ static struct apic apic_x2apic_cluster =
>>       .name                           = "cluster x2apic",
>>       .probe                          = x2apic_cluster_probe,
>>       .acpi_madt_oem_check            = x2apic_acpi_madt_oem_check,
>> -     .apic_id_valid                  = default_apic_id_valid,
>> +     .apic_id_valid                  = x2apic_apic_id_valid,
>>       .apic_id_registered             = x2apic_apic_id_registered,
>>
>>       .irq_delivery_mode              = dest_LowestPrio,
>> Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
>> +++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
>> @@ -119,7 +119,7 @@ static struct apic apic_x2apic_phys = {
>>       .name                           = "physical x2apic",
>>       .probe                          = x2apic_phys_probe,
>>       .acpi_madt_oem_check            = x2apic_acpi_madt_oem_check,
>> -     .apic_id_valid                  = default_apic_id_valid,
>> +     .apic_id_valid                  = x2apic_apic_id_valid,
>>       .apic_id_registered             = x2apic_apic_id_registered,
>>
>>       .irq_delivery_mode              = dest_Fixed,
>> Index: linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_uv_x.c
>> +++ linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c
>> @@ -266,6 +266,11 @@ static void uv_send_IPI_all(int vector)
>>       uv_send_IPI_mask(cpu_online_mask, vector);
>>  }
>>
>> +static int uv_apic_id_valid(int apicid)
>> +{
>> +     return 1;
>> +}
>> +
>>  static int uv_apic_id_registered(void)
>>  {
>>       return 1;
>> @@ -351,7 +356,7 @@ static struct apic __refdata apic_x2apic
>>       .name                           = "UV large system",
>>       .probe                          = uv_probe,
>>       .acpi_madt_oem_check            = uv_acpi_madt_oem_check,
>> -     .apic_id_valid                  = default_apic_id_valid,
>> +     .apic_id_valid                  = uv_apic_id_valid,
>>       .apic_id_registered             = uv_apic_id_registered,
>>
>>       .irq_delivery_mode              = dest_Fixed,
>> Index: linux-2.6/arch/x86/mm/srat.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/mm/srat.c
>> +++ linux-2.6/arch/x86/mm/srat.c
>> @@ -70,7 +70,7 @@ acpi_numa_x2apic_affinity_init(struct ac
>>               return;
>>       pxm = pa->proximity_domain;
>>       apic_id = pa->apic_id;
>> -     if (!cpu_has_x2apic && (apic_id >= 0xff)) {
>> +     if (!apic->apic_id_valid(apic_id)) {
>>               printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
>>                        pxm, apic_id);
>>               return;
>
>

[PATCH REPOST] Added separate apic_id_valid() functions for selected apic drivers.

[Steffen Persvold]

As suggested by Suresh Siddha <suresh.b.siddha@intel.com> and Yinghai Lu <yinghai@kernel.org> :

For x2apic pre-enabled systems, apic driver is set already early through early_acpi_boot_init()/early_acpi_process_madt()/acpi_parse_madt()/default_acpi_madt_oem_check() so that apic_id_valid() checking will be sufficient during MADT and SRAT prasing. For non- x2apic pre-enabled systems, all apic ids should be less than 255.

This allows us to substitute the checks in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic() and arch/x86/mm/srat.c::acpi_numa_x2apic_affinity_init() with apic->apic_id_valid().

In addition we can avoid feigning the x2apic cpu feature in the NumaChip apic code.

The following apic drivers have separate apic_id_valid() functions which will accept x2apic type IDs :

x2apic_phys
x2apic_cluster
x2apic_uv_x
apic_numachip

Signed-off-by: Steffen Persvold <sp@numascale.com>
---
 arch/x86/include/asm/apic.h           |    2 +-
 arch/x86/include/asm/x2apic.h         |    5 +++++
 arch/x86/kernel/acpi/boot.c           |    2 +-
 arch/x86/kernel/apic/apic_numachip.c  |    3 +--
 arch/x86/kernel/apic/x2apic_cluster.c |    2 +-
 arch/x86/kernel/apic/x2apic_phys.c    |    2 +-
 arch/x86/kernel/apic/x2apic_uv_x.c    |    7 ++++++-
 arch/x86/mm/srat.c                    |    2 +-
 8 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index a9371c9..d3eaac4 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(void)
 
 static inline int default_apic_id_valid(int apicid)
 {
-	return x2apic_mode || (apicid < 255);
+	return (apicid < 255);
 }
 
 extern void default_setup_apic_routing(void);
diff --git a/arch/x86/include/asm/x2apic.h b/arch/x86/include/asm/x2apic.h
index 6bf5b8e..92e54ab 100644
--- a/arch/x86/include/asm/x2apic.h
+++ b/arch/x86/include/asm/x2apic.h
@@ -18,6 +18,11 @@ static const struct cpumask *x2apic_target_cpus(void)
 	return cpu_online_mask;
 }
 
+static int x2apic_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int x2apic_apic_id_registered(void)
 {
 	return 1;
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index ce664f3..4b5059c 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -239,7 +239,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	 * to not preallocating memory for all NR_CPUS
 	 * when we use CPU hotplug.
 	 */
-	if (!cpu_has_x2apic && (apic_id >= 0xff) && enabled)
+	if (!apic->apic_id_valid(apic_id) && enabled)
 		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
 	else
 		acpi_register_lapic(apic_id, enabled);
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index d9ea5f3..899803e 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -229,11 +229,10 @@ static int __init numachip_system_init(void)
 }
 early_initcall(numachip_system_init);
 
-static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	if (!strncmp(oem_id, "NUMASC", 6)) {
 		numachip_system = 1;
-		setup_force_cpu_cap(X86_FEATURE_X2APIC);
 		return 1;
 	}
 
diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index 9193713..48f3103 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -213,7 +213,7 @@ static struct apic apic_x2apic_cluster = {
 	.name				= "cluster x2apic",
 	.probe				= x2apic_cluster_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index bcd1db6..8a778db 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -119,7 +119,7 @@ static struct apic apic_x2apic_phys = {
 	.name				= "physical x2apic",
 	.probe				= x2apic_phys_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index fc47714..87bfa69 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -266,6 +266,11 @@ static void uv_send_IPI_all(int vector)
 	uv_send_IPI_mask(cpu_online_mask, vector);
 }
 
+static int uv_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int uv_apic_id_registered(void)
 {
 	return 1;
@@ -351,7 +356,7 @@ static struct apic __refdata apic_x2apic_uv_x = {
 	.name				= "UV large system",
 	.probe				= uv_probe,
 	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= uv_apic_id_valid,
 	.apic_id_registered		= uv_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 1c1c4f4..efb5b4b 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -70,7 +70,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 		return;
 	pxm = pa->proximity_domain;
 	apic_id = pa->apic_id;
-	if (!cpu_has_x2apic && (apic_id >= 0xff)) {
+	if (!apic->apic_id_valid(apic_id)) {
 		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
 			 pxm, apic_id);
 		return;
-- 
1.7.4.1

Re: [PATCH REPOST] Added separate apic_id_valid() functions for selected apic drivers.

[Steffen Persvold]

On 3/16/2012 20:25, Steffen Persvold wrote:
> As suggested by Suresh Siddha<suresh.b.siddha@intel.com>  and Yinghai Lu<yinghai@kernel.org>  :
>
> For x2apic pre-enabled systems, apic driver is set already early through early_acpi_boot_init()/early_acpi_process_madt()/acpi_parse_madt()/default_acpi_madt_oem_check() so that apic_id_valid() checking will be sufficient during MADT and SRAT prasing. For non- x2apic pre-enabled systems, all apic ids should be less than 255.
>
> This allows us to substitute the checks in arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic() and arch/x86/mm/srat.c::acpi_numa_x2apic_affinity_init() with apic->apic_id_valid().
>
> In addition we can avoid feigning the x2apic cpu feature in the NumaChip apic code.
>
> The following apic drivers have separate apic_id_valid() functions which will accept x2apic type IDs :
>
> x2apic_phys
> x2apic_cluster
> x2apic_uv_x
> apic_numachip
>
> Signed-off-by: Steffen Persvold<sp@numascale.com>

Ingo, any comments on this approach ? It should cover both cases as 
suggested by Suresh and Yinghai.

Kind regards,
-- 
Steffen Persvold, Chief Architect NumaChip
Numascale AS - www.numascale.com
Tel: +47 92 49 25 54 Skype: spersvold


> ---
>   arch/x86/include/asm/apic.h           |    2 +-
>   arch/x86/include/asm/x2apic.h         |    5 +++++
>   arch/x86/kernel/acpi/boot.c           |    2 +-
>   arch/x86/kernel/apic/apic_numachip.c  |    3 +--
>   arch/x86/kernel/apic/x2apic_cluster.c |    2 +-
>   arch/x86/kernel/apic/x2apic_phys.c    |    2 +-
>   arch/x86/kernel/apic/x2apic_uv_x.c    |    7 ++++++-
>   arch/x86/mm/srat.c                    |    2 +-
>   8 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index a9371c9..d3eaac4 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(void)
>
>   static inline int default_apic_id_valid(int apicid)
>   {
> -	return x2apic_mode || (apicid<  255);
> +	return (apicid<  255);
>   }
>
>   extern void default_setup_apic_routing(void);
> diff --git a/arch/x86/include/asm/x2apic.h b/arch/x86/include/asm/x2apic.h
> index 6bf5b8e..92e54ab 100644
> --- a/arch/x86/include/asm/x2apic.h
> +++ b/arch/x86/include/asm/x2apic.h
> @@ -18,6 +18,11 @@ static const struct cpumask *x2apic_target_cpus(void)
>   	return cpu_online_mask;
>   }
>
> +static int x2apic_apic_id_valid(int apicid)
> +{
> +	return 1;
> +}
> +
>   static int x2apic_apic_id_registered(void)
>   {
>   	return 1;
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index ce664f3..4b5059c 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -239,7 +239,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
>   	 * to not preallocating memory for all NR_CPUS
>   	 * when we use CPU hotplug.
>   	 */
> -	if (!cpu_has_x2apic&&  (apic_id>= 0xff)&&  enabled)
> +	if (!apic->apic_id_valid(apic_id)&&  enabled)
>   		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
>   	else
>   		acpi_register_lapic(apic_id, enabled);
> diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
> index d9ea5f3..899803e 100644
> --- a/arch/x86/kernel/apic/apic_numachip.c
> +++ b/arch/x86/kernel/apic/apic_numachip.c
> @@ -229,11 +229,10 @@ static int __init numachip_system_init(void)
>   }
>   early_initcall(numachip_system_init);
>
> -static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
> +static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>   {
>   	if (!strncmp(oem_id, "NUMASC", 6)) {
>   		numachip_system = 1;
> -		setup_force_cpu_cap(X86_FEATURE_X2APIC);
>   		return 1;
>   	}
>
> diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
> index 9193713..48f3103 100644
> --- a/arch/x86/kernel/apic/x2apic_cluster.c
> +++ b/arch/x86/kernel/apic/x2apic_cluster.c
> @@ -213,7 +213,7 @@ static struct apic apic_x2apic_cluster = {
>   	.name				= "cluster x2apic",
>   	.probe				= x2apic_cluster_probe,
>   	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
> -	.apic_id_valid			= default_apic_id_valid,
> +	.apic_id_valid			= x2apic_apic_id_valid,
>   	.apic_id_registered		= x2apic_apic_id_registered,
>
>   	.irq_delivery_mode		= dest_LowestPrio,
> diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
> index bcd1db6..8a778db 100644
> --- a/arch/x86/kernel/apic/x2apic_phys.c
> +++ b/arch/x86/kernel/apic/x2apic_phys.c
> @@ -119,7 +119,7 @@ static struct apic apic_x2apic_phys = {
>   	.name				= "physical x2apic",
>   	.probe				= x2apic_phys_probe,
>   	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
> -	.apic_id_valid			= default_apic_id_valid,
> +	.apic_id_valid			= x2apic_apic_id_valid,
>   	.apic_id_registered		= x2apic_apic_id_registered,
>
>   	.irq_delivery_mode		= dest_Fixed,
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index fc47714..87bfa69 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -266,6 +266,11 @@ static void uv_send_IPI_all(int vector)
>   	uv_send_IPI_mask(cpu_online_mask, vector);
>   }
>
> +static int uv_apic_id_valid(int apicid)
> +{
> +	return 1;
> +}
> +
>   static int uv_apic_id_registered(void)
>   {
>   	return 1;
> @@ -351,7 +356,7 @@ static struct apic __refdata apic_x2apic_uv_x = {
>   	.name				= "UV large system",
>   	.probe				= uv_probe,
>   	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
> -	.apic_id_valid			= default_apic_id_valid,
> +	.apic_id_valid			= uv_apic_id_valid,
>   	.apic_id_registered		= uv_apic_id_registered,
>
>   	.irq_delivery_mode		= dest_Fixed,
> diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
> index 1c1c4f4..efb5b4b 100644
> --- a/arch/x86/mm/srat.c
> +++ b/arch/x86/mm/srat.c
> @@ -70,7 +70,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
>   		return;
>   	pxm = pa->proximity_domain;
>   	apic_id = pa->apic_id;
> -	if (!cpu_has_x2apic&&  (apic_id>= 0xff)) {
> +	if (!apic->apic_id_valid(apic_id)) {
>   		printk(KERN_INFO "SRAT: PXM %u ->  X2APIC 0x%04x ignored\n",
>   			 pxm, apic_id);
>   		return;

Re: [PATCH REPOST] Added separate apic_id_valid() functions for selected apic drivers.

[Ingo Molnar]

* Steffen Persvold <sp@numascale.com> wrote:

> Ingo, any comments on this approach ? It should cover both 
> cases as suggested by Suresh and Yinghai.

Looks good in principle, but we are in the v3.4 merge window 
right now - new patches will have to wait a bit.

Thanks,

	Ingo

[tip:x86/urgent] x86/apic: Add separate apic_id_valid() functions for selected apic drivers

[tip-bot for Steffen Persvold]

Commit-ID:  b7157acf429e6aef690646ba964b9ebd25049ec2
Gitweb:     http://git.kernel.org/tip/b7157acf429e6aef690646ba964b9ebd25049ec2
Author:     Steffen Persvold <sp@numascale.com>
AuthorDate: Fri, 16 Mar 2012 20:25:35 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Mar 2012 13:28:43 +0100

x86/apic: Add separate apic_id_valid() functions for selected apic drivers

As suggested by Suresh Siddha and Yinghai Lu:

For x2apic pre-enabled systems, apic driver is set already early
through early_acpi_boot_init()/early_acpi_process_madt()/
acpi_parse_madt()/default_acpi_madt_oem_check() path so that
apic_id_valid() checking will be sufficient during MADT and SRAT
parsing.

For non-x2apic pre-enabled systems, all apic ids should be less
than 255.

This allows us to substitute the checks in
arch/x86/kernel/acpi/boot.c::acpi_parse_x2apic() and
arch/x86/mm/srat.c::acpi_numa_x2apic_affinity_init() with
apic->apic_id_valid().

In addition we can avoid feigning the x2apic cpu feature in the
NumaChip apic code.

The following apic drivers have separate apic_id_valid()
functions which will accept x2apic type IDs :

 x2apic_phys
 x2apic_cluster
 x2apic_uv_x
 apic_numachip

Signed-off-by: Steffen Persvold <sp@numascale.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Daniel J Blueman <daniel@numascale-asia.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Jack Steiner <steiner@sgi.com>
Link: http://lkml.kernel.org/r/1331925935-13372-1-git-send-email-sp@numascale.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/apic.h           |    2 +-
 arch/x86/include/asm/x2apic.h         |    5 +++++
 arch/x86/kernel/acpi/boot.c           |    2 +-
 arch/x86/kernel/apic/apic_numachip.c  |    3 +--
 arch/x86/kernel/apic/x2apic_cluster.c |    2 +-
 arch/x86/kernel/apic/x2apic_phys.c    |    2 +-
 arch/x86/kernel/apic/x2apic_uv_x.c    |    7 ++++++-
 arch/x86/mm/srat.c                    |    2 +-
 8 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index a9371c9..d3eaac4 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -535,7 +535,7 @@ static inline unsigned int read_apic_id(void)
 
 static inline int default_apic_id_valid(int apicid)
 {
-	return x2apic_mode || (apicid < 255);
+	return (apicid < 255);
 }
 
 extern void default_setup_apic_routing(void);
diff --git a/arch/x86/include/asm/x2apic.h b/arch/x86/include/asm/x2apic.h
index 6bf5b8e..92e54ab 100644
--- a/arch/x86/include/asm/x2apic.h
+++ b/arch/x86/include/asm/x2apic.h
@@ -18,6 +18,11 @@ static const struct cpumask *x2apic_target_cpus(void)
 	return cpu_online_mask;
 }
 
+static int x2apic_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int x2apic_apic_id_registered(void)
 {
 	return 1;
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 406ed77..0f42c2f 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -239,7 +239,7 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 	 * to not preallocating memory for all NR_CPUS
 	 * when we use CPU hotplug.
 	 */
-	if (!cpu_has_x2apic && (apic_id >= 0xff) && enabled)
+	if (!apic->apic_id_valid(apic_id) && enabled)
 		printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
 	else
 		acpi_register_lapic(apic_id, enabled);
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index d9ea5f3..899803e 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -229,11 +229,10 @@ static int __init numachip_system_init(void)
 }
 early_initcall(numachip_system_init);
 
-static int __cpuinit numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+static int numachip_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	if (!strncmp(oem_id, "NUMASC", 6)) {
 		numachip_system = 1;
-		setup_force_cpu_cap(X86_FEATURE_X2APIC);
 		return 1;
 	}
 
diff --git a/arch/x86/kernel/apic/x2apic_cluster.c b/arch/x86/kernel/apic/x2apic_cluster.c
index 9193713..48f3103 100644
--- a/arch/x86/kernel/apic/x2apic_cluster.c
+++ b/arch/x86/kernel/apic/x2apic_cluster.c
@@ -213,7 +213,7 @@ static struct apic apic_x2apic_cluster = {
 	.name				= "cluster x2apic",
 	.probe				= x2apic_cluster_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_LowestPrio,
diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index bcd1db6..8a778db 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -119,7 +119,7 @@ static struct apic apic_x2apic_phys = {
 	.name				= "physical x2apic",
 	.probe				= x2apic_phys_probe,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= x2apic_apic_id_valid,
 	.apic_id_registered		= x2apic_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index fc47714..87bfa69 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -266,6 +266,11 @@ static void uv_send_IPI_all(int vector)
 	uv_send_IPI_mask(cpu_online_mask, vector);
 }
 
+static int uv_apic_id_valid(int apicid)
+{
+	return 1;
+}
+
 static int uv_apic_id_registered(void)
 {
 	return 1;
@@ -351,7 +356,7 @@ static struct apic __refdata apic_x2apic_uv_x = {
 	.name				= "UV large system",
 	.probe				= uv_probe,
 	.acpi_madt_oem_check		= uv_acpi_madt_oem_check,
-	.apic_id_valid			= default_apic_id_valid,
+	.apic_id_valid			= uv_apic_id_valid,
 	.apic_id_registered		= uv_apic_id_registered,
 
 	.irq_delivery_mode		= dest_Fixed,
diff --git a/arch/x86/mm/srat.c b/arch/x86/mm/srat.c
index 1c1c4f4..efb5b4b 100644
--- a/arch/x86/mm/srat.c
+++ b/arch/x86/mm/srat.c
@@ -70,7 +70,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 		return;
 	pxm = pa->proximity_domain;
 	apic_id = pa->apic_id;
-	if (!cpu_has_x2apic && (apic_id >= 0xff)) {
+	if (!apic->apic_id_valid(apic_id)) {
 		printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n",
 			 pxm, apic_id);
 		return;

[PATCH 0/6] Improvements to Yinghai's x86 IOAPIC hotplug work

[Jiang Liu]

This patchset is based on Yinghua's work for IOAPIC hotplug on x86 systems,
please refer to http://lwn.net/Articles/483671/. It applies to:
94738bb PCI: Disable mem in the ioapic removing path
at
git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-x86-irq

Due to resource limitation, (only an Intel Atom system available for building
and testing), only the boot logic has been tested until now, so need more tests
for the hotplug logic when having access to systems with IOAPIC hotplug feature.

BTW, it's really a nightmare to build and test kernel on an Intel Atom system.

Jiang Liu (6):
  x86,IRQ: Fix possible invalid memory access after IOAPIC hot-plugging
  x86,IRQ: Mark unused entries in 'ioapics' array as free at startup
  x86,IRQ: Enhance irq allocation policy for hot-added IOAPICs
  x86,IRQ: split out function ioapic_setup_resource()
  x86,IRQ: Correctly manage MMIO resource used by IOAPIC when
    hot-plugging IOPAICs
  x86,IRQ: Use memory barriers to protect searching side code

 arch/x86/kernel/apic/io_apic.c |  150 +++++++++++++++++++++++++++++-----------
 1 files changed, 110 insertions(+), 40 deletions(-)

-- 
1.7.5.4

[PATCH 0/5] Improvements to Yinghai's IOAPIC hotplug work on x86

[Jiang Liu]

This patchset is based on Yinghua's work for IOAPIC hotplug on x86,
please refer to http://lwn.net/Articles/483671/.
The first two patches are minor bugfixes, the third and fourth are
enhancement to Yinghai's implementation, and the fifth is a RFC patch.

I only have an Atom system at hand to build and test the patchset,
so only the boot logic has been tested until now, need more tests
about the hotplug logic when having access to systems which support
IOAPIC hotplug feature.

BTW, it's really a nightmare to build and test kernel on an Atom system.

Jiang Liu (5):
  x86:IRQ: Fix possible invalid memory access when hot-plugging IOAPICs
  x86:IRQ: Mark unused entries in 'ioapics' array as free at startup
  x86:IRQ: Enhance irq allocation policy for hot-added IOAPICs
  x86:IRQ: Correctly manage MMIO resource used by IOAPIC when
    hot-plugging
  x86:IRQ: Use memory barriers to protect searching side code

 arch/x86/kernel/apic/io_apic.c |  159 ++++++++++++++++++++++++++++------------
 1 files changed, 113 insertions(+), 46 deletions(-)

-- 
1.7.5.4

[PATCH 1/6] x86,IRQ: Fix possible invalid memory access after IOAPIC hot-plugging

[Jiang Liu]

Function free_ioapic_saved_registers() should mark ioapic->saved_regiters
as NULL after the memory has been freed. Otherwise when hot-adding
another IOAPIC, function alloc_ioapic_saved_registers() may reuse the
stale pointer and cause memory corruption.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/kernel/apic/io_apic.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index da02320..7412eb8 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -362,6 +362,7 @@ static void alloc_ioapic_saved_registers(int idx)
 static void free_ioapic_saved_registers(int idx)
 {
 	kfree(ioapics[idx].saved_registers);
+	ioapics[idx].saved_registers = NULL;
 }
 
 int __init arch_early_irq_init(void)
-- 
1.7.5.4

[PATCH 2/6] x86,IRQ: Mark unused entries in 'ioapics' array as free at startup

[Jiang Liu]

Unused entries in ioapics array should be marked as free at startup,
so they could be used by hot-added IOAPICs.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/kernel/apic/io_apic.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 7412eb8..622374f 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -392,6 +392,10 @@ int __init arch_early_irq_init(void)
 		cpumask_set_cpu(0, cfg->domain);
 	}
 
+	/* Mark all left entries as free for IOAPIC hot-adding. */
+	for (i = nr_ioapics; i < MAX_IO_APICS; i++)
+		ioapics[i].mp_config.apicid = 0xff;
+
 	return 0;
 }
 
-- 
1.7.5.4

Re: [PATCH 2/6] x86,IRQ: Mark unused entries in 'ioapics' array as free at startup

[Yinghai Lu]

On Tue, Mar 20, 2012 at 9:20 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Unused entries in ioapics array should be marked as free at startup,
> so they could be used by hot-added IOAPICs.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  arch/x86/kernel/apic/io_apic.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 7412eb8..622374f 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -392,6 +392,10 @@ int __init arch_early_irq_init(void)
>                cpumask_set_cpu(0, cfg->domain);
>        }
>
> +       /* Mark all left entries as free for IOAPIC hot-adding. */
> +       for (i = nr_ioapics; i < MAX_IO_APICS; i++)
> +               ioapics[i].mp_config.apicid = 0xff;
> +
>        return 0;
>  }

this one looks like not needed, we did not search that after
nr_ioapics, and just use nr_ioapics if open spot is found.

Yinghai

Re: [PATCH 2/6] x86,IRQ: Mark unused entries in 'ioapics' array as free at startup

[Jiang Liu]

So seems another patch is needed to search all entries in the ioapics
array to find a free entry for real hot-added IOAPICs. Current
implementation can only support hot-replace scenario, but can't support
real hot-add. If needed, I will work out another patch to address that.

Gerry

On 2012-3-21 11:25, Yinghai Lu wrote:
> On Tue, Mar 20, 2012 at 9:20 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>> Unused entries in ioapics array should be marked as free at startup,
>> so they could be used by hot-added IOAPICs.
>>
>> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
>> ---
>>   arch/x86/kernel/apic/io_apic.c |    4 ++++
>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> index 7412eb8..622374f 100644
>> --- a/arch/x86/kernel/apic/io_apic.c
>> +++ b/arch/x86/kernel/apic/io_apic.c
>> @@ -392,6 +392,10 @@ int __init arch_early_irq_init(void)
>>                 cpumask_set_cpu(0, cfg->domain);
>>         }
>>
>> +       /* Mark all left entries as free for IOAPIC hot-adding. */
>> +       for (i = nr_ioapics; i<  MAX_IO_APICS; i++)
>> +               ioapics[i].mp_config.apicid = 0xff;
>> +
>>         return 0;
>>   }
>
> this one looks like not needed, we did not search that after
> nr_ioapics, and just use nr_ioapics if open spot is found.
>
> Yinghai
>
>

Re: [PATCH 2/6] x86,IRQ: Mark unused entries in 'ioapics' array as free at startup

[Jiang Liu]

Hi Yinghai,
	Get your points now and will drop this patch from the series.
	Thanks!

On 03/21/2012 11:32 AM, Jiang Liu wrote:
> So seems another patch is needed to search all entries in the ioapics
> array to find a free entry for real hot-added IOAPICs. Current
> implementation can only support hot-replace scenario, but can't support
> real hot-add. If needed, I will work out another patch to address that.
> 
> Gerry
> 
> On 2012-3-21 11:25, Yinghai Lu wrote:
>> On Tue, Mar 20, 2012 at 9:20 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>>> Unused entries in ioapics array should be marked as free at startup,
>>> so they could be used by hot-added IOAPICs.
>>>
>>> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
>>> ---
>>>   arch/x86/kernel/apic/io_apic.c |    4 ++++
>>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>>> index 7412eb8..622374f 100644
>>> --- a/arch/x86/kernel/apic/io_apic.c
>>> +++ b/arch/x86/kernel/apic/io_apic.c
>>> @@ -392,6 +392,10 @@ int __init arch_early_irq_init(void)
>>>                 cpumask_set_cpu(0, cfg->domain);
>>>         }
>>>
>>> +       /* Mark all left entries as free for IOAPIC hot-adding. */
>>> +       for (i = nr_ioapics; i<  MAX_IO_APICS; i++)
>>> +               ioapics[i].mp_config.apicid = 0xff;
>>> +
>>>         return 0;
>>>   }
>>
>> this one looks like not needed, we did not search that after
>> nr_ioapics, and just use nr_ioapics if open spot is found.
>>
>> Yinghai
>>
>>
> 
> 

[PATCH 3/6] x86,IRQ: Enhance irq allocation policy for hot-added IOAPICs

[Jiang Liu]

For hot-added IOAPICs, try to allocate irqs starting from
ioapic->gsi_base first, and fallback to starting from 0.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/kernel/apic/io_apic.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 622374f..e5b6ca4 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -294,13 +294,16 @@ static struct irq_cfg *realloc_irq_and_cfg_at(unsigned int at, int node)
 	return alloc_irq_and_cfg_at(at, node);
 }
 
-static int reserve_ioapic_gsi_irq_base(int idx)
+static int reserve_ioapic_gsi_irq_base(int idx, bool hotadd)
 {
 	int irq;
 	struct mp_ioapic_gsi *gsi_cfg = mp_ioapic_gsi_routing(idx);
 	int cnt = gsi_cfg->gsi_end - gsi_cfg->gsi_base + 1;
 
 	irq = __irq_reserve_irqs(-1, gsi_cfg->gsi_base, cnt);
+	if (irq < 0 && hotadd)
+		irq = __irq_reserve_irqs(-1, 0, cnt);
+
 	if (irq >= 0) {
 		gsi_cfg->irq_base = irq;
 		printk(KERN_INFO
@@ -378,7 +381,7 @@ int __init arch_early_irq_init(void)
 		alloc_ioapic_saved_registers(i);
 
 	for (i = 0; i < nr_ioapics; i++)
-		reserve_ioapic_gsi_irq_base(i);
+		reserve_ioapic_gsi_irq_base(i, false);
 
 	reserve_ioapic_gsi_irq_extra();
 
@@ -4181,7 +4184,7 @@ int __mp_register_ioapic(int id, u32 address, u32 gsi_base, bool hotadd)
 		if (gsi_cfg->gsi_end >= gsi_top)
 			gsi_top = gsi_cfg->gsi_end + 1;
 	} else {
-		int irq = reserve_ioapic_gsi_irq_base(idx);
+		int irq = reserve_ioapic_gsi_irq_base(idx, true);
 
 		if (irq < 0)
 			goto failed;
-- 
1.7.5.4

[PATCH 4/6] x86,IRQ: split out function ioapic_setup_resource()

[Jiang Liu]

Split out function ioapic_setup_resource(), which will be used by IOAPIC
hotplug logic.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/kernel/apic/io_apic.c |   69 ++++++++++++++++++++++------------------
 1 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index e5b6ca4..dfb6f45 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -85,6 +85,7 @@ static struct ioapic {
 	 * Saved state during suspend/resume, or while enabling intr-remap.
 	 */
 	struct IO_APIC_route_entry *saved_registers;
+	struct resource *resource;
 	/* I/O APIC config */
 	struct mpc_ioapic mp_config;
 	/* IO APIC gsi routing info */
@@ -3987,7 +3988,7 @@ void __init setup_ioapic_dest(void)
 
 static struct resource *ioapic_resources;
 
-static struct resource * __init ioapic_setup_resources(int nr_ioapics)
+static struct resource * __init ioapic_alloc_resources(int nr_ioapics)
 {
 	unsigned long n;
 	struct resource *res;
@@ -4010,6 +4011,7 @@ static struct resource * __init ioapic_setup_resources(int nr_ioapics)
 		res[i].flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 		snprintf(mem, IOAPIC_RESOURCE_NAME_SIZE, "IOAPIC %u", i);
 		mem += IOAPIC_RESOURCE_NAME_SIZE;
+		ioapics[i].resource = &res[i];
 	}
 
 	ioapic_resources = res;
@@ -4017,43 +4019,48 @@ static struct resource * __init ioapic_setup_resources(int nr_ioapics)
 	return res;
 }
 
-void __init ioapic_and_gsi_init(void)
+static void ioapic_setup_resource(int idx, struct resource *res, bool hotadd)
 {
-	unsigned long ioapic_phys, idx = FIX_IO_APIC_BASE_0;
-	struct resource *ioapic_res;
-	int i;
+	unsigned long ioapic_phys, vaddr;
 
-	ioapic_res = ioapic_setup_resources(nr_ioapics);
-	for (i = 0; i < nr_ioapics; i++) {
-		if (smp_found_config) {
-			ioapic_phys = mpc_ioapic_addr(i);
+	if (hotadd) {
+		ioapic_phys = mpc_ioapic_addr(idx);
+	} else if (smp_found_config) {
+		ioapic_phys = mpc_ioapic_addr(idx);
 #ifdef CONFIG_X86_32
-			if (!ioapic_phys) {
-				printk(KERN_ERR
-				       "WARNING: bogus zero IO-APIC "
-				       "address found in MPTABLE, "
-				       "disabling IO/APIC support!\n");
-				smp_found_config = 0;
-				skip_ioapic_setup = 1;
-				goto fake_ioapic_page;
-			}
+		if (!ioapic_phys) {
+			printk(KERN_ERR
+			       "WARNING: bogus zero IO-APIC address found "
+			       "in MPTABLE, disabling IO/APIC support!\n");
+			smp_found_config = 0;
+			skip_ioapic_setup = 1;
+			goto fake_ioapic_page;
+		}
 #endif
-		} else {
+	} else {
 #ifdef CONFIG_X86_32
 fake_ioapic_page:
 #endif
-			ioapic_phys = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
-			ioapic_phys = __pa(ioapic_phys);
-		}
-		set_fixmap_nocache(idx, ioapic_phys);
-		apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08lx (%08lx)\n",
-			__fix_to_virt(idx) + (ioapic_phys & ~PAGE_MASK),
-			ioapic_phys);
-		idx++;
-
-		ioapic_res->start = ioapic_phys;
-		ioapic_res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
-		ioapic_res++;
+		ioapic_phys = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
+		ioapic_phys = __pa(ioapic_phys);
+	}
+
+	vaddr = __fix_to_virt(FIX_IO_APIC_BASE_0 + idx);
+	vaddr += (ioapic_phys & ~PAGE_MASK);
+	apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08lx (%08lx)\n",
+		vaddr, ioapic_phys);
+
+	res->start = ioapic_phys;
+	res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
+}
+
+void __init ioapic_and_gsi_init(void)
+{
+	int i;
+
+	ioapic_alloc_resources(nr_ioapics);
+	for (i = 0; i < nr_ioapics; i++) {
+		ioapic_setup_resource(i, ioapics[i].resource, false);
 	}
 
 	probe_nr_irqs_gsi();
-- 
1.7.5.4

Re: [PATCH 4/6] x86,IRQ: split out function ioapic_setup_resource()

[Yinghai Lu]

On Tue, Mar 20, 2012 at 9:21 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Split out function ioapic_setup_resource(), which will be used by IOAPIC
> hotplug logic.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  arch/x86/kernel/apic/io_apic.c |   69 ++++++++++++++++++++++------------------
>  1 files changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index e5b6ca4..dfb6f45 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -85,6 +85,7 @@ static struct ioapic {
>         * Saved state during suspend/resume, or while enabling intr-remap.
>         */
>        struct IO_APIC_route_entry *saved_registers;
> +       struct resource *resource;
>        /* I/O APIC config */
>        struct mpc_ioapic mp_config;
>        /* IO APIC gsi routing info */
> @@ -3987,7 +3988,7 @@ void __init setup_ioapic_dest(void)
>
>  static struct resource *ioapic_resources;
>
> -static struct resource * __init ioapic_setup_resources(int nr_ioapics)
> +static struct resource * __init ioapic_alloc_resources(int nr_ioapics)
>  {
>        unsigned long n;
>        struct resource *res;
> @@ -4010,6 +4011,7 @@ static struct resource * __init ioapic_setup_resources(int nr_ioapics)
>                res[i].flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>                snprintf(mem, IOAPIC_RESOURCE_NAME_SIZE, "IOAPIC %u", i);
>                mem += IOAPIC_RESOURCE_NAME_SIZE;
> +               ioapics[i].resource = &res[i];
>        }
>
>        ioapic_resources = res;
> @@ -4017,43 +4019,48 @@ static struct resource * __init ioapic_setup_resources(int nr_ioapics)
>        return res;
>  }
>
> -void __init ioapic_and_gsi_init(void)
> +static void ioapic_setup_resource(int idx, struct resource *res, bool hotadd)
>  {
> -       unsigned long ioapic_phys, idx = FIX_IO_APIC_BASE_0;
> -       struct resource *ioapic_res;
> -       int i;
> +       unsigned long ioapic_phys, vaddr;
>
> -       ioapic_res = ioapic_setup_resources(nr_ioapics);
> -       for (i = 0; i < nr_ioapics; i++) {
> -               if (smp_found_config) {
> -                       ioapic_phys = mpc_ioapic_addr(i);
> +       if (hotadd) {
> +               ioapic_phys = mpc_ioapic_addr(idx);
> +       } else if (smp_found_config) {
> +               ioapic_phys = mpc_ioapic_addr(idx);
>  #ifdef CONFIG_X86_32
> -                       if (!ioapic_phys) {
> -                               printk(KERN_ERR
> -                                      "WARNING: bogus zero IO-APIC "
> -                                      "address found in MPTABLE, "
> -                                      "disabling IO/APIC support!\n");
> -                               smp_found_config = 0;
> -                               skip_ioapic_setup = 1;
> -                               goto fake_ioapic_page;
> -                       }
> +               if (!ioapic_phys) {
> +                       printk(KERN_ERR
> +                              "WARNING: bogus zero IO-APIC address found "
> +                              "in MPTABLE, disabling IO/APIC support!\n");
> +                       smp_found_config = 0;
> +                       skip_ioapic_setup = 1;
> +                       goto fake_ioapic_page;
> +               }
>  #endif
> -               } else {
> +       } else {
>  #ifdef CONFIG_X86_32
>  fake_ioapic_page:
>  #endif
> -                       ioapic_phys = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
> -                       ioapic_phys = __pa(ioapic_phys);
> -               }
> -               set_fixmap_nocache(idx, ioapic_phys);
> -               apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08lx (%08lx)\n",
> -                       __fix_to_virt(idx) + (ioapic_phys & ~PAGE_MASK),
> -                       ioapic_phys);
> -               idx++;
> -
> -               ioapic_res->start = ioapic_phys;
> -               ioapic_res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
> -               ioapic_res++;
> +               ioapic_phys = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
> +               ioapic_phys = __pa(ioapic_phys);

where is calling set_fixmap_nocache for fake ioapic one?

> +       }
> +
> +       vaddr = __fix_to_virt(FIX_IO_APIC_BASE_0 + idx);
> +       vaddr += (ioapic_phys & ~PAGE_MASK);
> +       apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08lx (%08lx)\n",
> +               vaddr, ioapic_phys);
> +
> +       res->start = ioapic_phys;
> +       res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
> +}
> +
> +void __init ioapic_and_gsi_init(void)
> +{
> +       int i;
> +
> +       ioapic_alloc_resources(nr_ioapics);
> +       for (i = 0; i < nr_ioapics; i++) {
> +               ioapic_setup_resource(i, ioapics[i].resource, false);
>        }

how about booting system with ioapic device and later hot remove it?

maybe we can just alloc one by one.

Yinghai

Re: [PATCH 4/6] x86,IRQ: split out function ioapic_setup_resource()

[Jiang Liu]

On 2012-3-21 11:34, Yinghai Lu wrote:
> On Tue, Mar 20, 2012 at 9:21 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>> Split out function ioapic_setup_resource(), which will be used by IOAPIC
>> hotplug logic.
>>
>> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
>> ---
>>   arch/x86/kernel/apic/io_apic.c |   69 ++++++++++++++++++++++------------------
>>   1 files changed, 38 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> index e5b6ca4..dfb6f45 100644
>> --- a/arch/x86/kernel/apic/io_apic.c
>> +++ b/arch/x86/kernel/apic/io_apic.c
>> @@ -85,6 +85,7 @@ static struct ioapic {
>>          * Saved state during suspend/resume, or while enabling intr-remap.
>>          */
>>         struct IO_APIC_route_entry *saved_registers;
>> +       struct resource *resource;
>>         /* I/O APIC config */
>>         struct mpc_ioapic mp_config;
>>         /* IO APIC gsi routing info */
>> @@ -3987,7 +3988,7 @@ void __init setup_ioapic_dest(void)
>>
>>   static struct resource *ioapic_resources;
>>
>> -static struct resource * __init ioapic_setup_resources(int nr_ioapics)
>> +static struct resource * __init ioapic_alloc_resources(int nr_ioapics)
>>   {
>>         unsigned long n;
>>         struct resource *res;
>> @@ -4010,6 +4011,7 @@ static struct resource * __init ioapic_setup_resources(int nr_ioapics)
>>                 res[i].flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>>                 snprintf(mem, IOAPIC_RESOURCE_NAME_SIZE, "IOAPIC %u", i);
>>                 mem += IOAPIC_RESOURCE_NAME_SIZE;
>> +               ioapics[i].resource =&res[i];
>>         }
>>
>>         ioapic_resources = res;
>> @@ -4017,43 +4019,48 @@ static struct resource * __init ioapic_setup_resources(int nr_ioapics)
>>         return res;
>>   }
>>
>> -void __init ioapic_and_gsi_init(void)
>> +static void ioapic_setup_resource(int idx, struct resource *res, bool hotadd)
>>   {
>> -       unsigned long ioapic_phys, idx = FIX_IO_APIC_BASE_0;
>> -       struct resource *ioapic_res;
>> -       int i;
>> +       unsigned long ioapic_phys, vaddr;
>>
>> -       ioapic_res = ioapic_setup_resources(nr_ioapics);
>> -       for (i = 0; i<  nr_ioapics; i++) {
>> -               if (smp_found_config) {
>> -                       ioapic_phys = mpc_ioapic_addr(i);
>> +       if (hotadd) {
>> +               ioapic_phys = mpc_ioapic_addr(idx);
>> +       } else if (smp_found_config) {
>> +               ioapic_phys = mpc_ioapic_addr(idx);
>>   #ifdef CONFIG_X86_32
>> -                       if (!ioapic_phys) {
>> -                               printk(KERN_ERR
>> -                                      "WARNING: bogus zero IO-APIC "
>> -                                      "address found in MPTABLE, "
>> -                                      "disabling IO/APIC support!\n");
>> -                               smp_found_config = 0;
>> -                               skip_ioapic_setup = 1;
>> -                               goto fake_ioapic_page;
>> -                       }
>> +               if (!ioapic_phys) {
>> +                       printk(KERN_ERR
>> +                              "WARNING: bogus zero IO-APIC address found "
>> +                              "in MPTABLE, disabling IO/APIC support!\n");
>> +                       smp_found_config = 0;
>> +                       skip_ioapic_setup = 1;
>> +                       goto fake_ioapic_page;
>> +               }
>>   #endif
>> -               } else {
>> +       } else {
>>   #ifdef CONFIG_X86_32
>>   fake_ioapic_page:
>>   #endif
>> -                       ioapic_phys = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
>> -                       ioapic_phys = __pa(ioapic_phys);
>> -               }
>> -               set_fixmap_nocache(idx, ioapic_phys);
>> -               apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08lx (%08lx)\n",
>> -                       __fix_to_virt(idx) + (ioapic_phys&  ~PAGE_MASK),
>> -                       ioapic_phys);
>> -               idx++;
>> -
>> -               ioapic_res->start = ioapic_phys;
>> -               ioapic_res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
>> -               ioapic_res++;
>> +               ioapic_phys = (unsigned long)alloc_bootmem_pages(PAGE_SIZE);
>> +               ioapic_phys = __pa(ioapic_phys);
>
> where is calling set_fixmap_nocache for fake ioapic one?
My bad, doesn't notice the case for fake ioapic, will add back the
set_fixmap_nocache. I just noticed that set_fixmap_nocache() will
be called twice for real IOAPICs.

>
>> +       }
>> +
>> +       vaddr = __fix_to_virt(FIX_IO_APIC_BASE_0 + idx);
>> +       vaddr += (ioapic_phys&  ~PAGE_MASK);
>> +       apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08lx (%08lx)\n",
>> +               vaddr, ioapic_phys);
>> +
>> +       res->start = ioapic_phys;
>> +       res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
>> +}
>> +
>> +void __init ioapic_and_gsi_init(void)
>> +{
>> +       int i;
>> +
>> +       ioapic_alloc_resources(nr_ioapics);
>> +       for (i = 0; i<  nr_ioapics; i++) {
>> +               ioapic_setup_resource(i, ioapics[i].resource, false);
>>         }
>
> how about booting system with ioapic device and later hot remove it?
>
> maybe we can just alloc one by one.
Here the issue is that, it uses bootmem allocator at boot time, which
is paged aligned. So seems a little waste to allocate one page for
each ioapic. The tradeoff here is to leak the memory if all IOAPICs
are removed at runtime. If needed, we could add counter to track the
allcoated bootmem and free it when counter reaches zero.

>
> Yinghai
>
> .
>

[PATCH 5/6] x86,IRQ: Correctly manage MMIO resource used by IOAPIC when hot-plugging IOPAICs

[Jiang Liu]

Correctly manage MMIO resource used by IOAPIC when hot-plugging IOAPICs.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/kernel/apic/io_apic.c |   46 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index dfb6f45..9c2ac7b 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -4054,6 +4054,42 @@ fake_ioapic_page:
 	res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
 }
 
+static struct resource *ioapic_add_resource(int idx)
+{
+	char *name;
+	struct resource *res;
+	size_t sz = IOAPIC_RESOURCE_NAME_SIZE + sizeof(struct resource);
+
+	res = kzalloc(sz, GFP_KERNEL);
+	if (res) {
+		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+		res->name = name = (char *)(res + 1);
+		snprintf(name, IOAPIC_RESOURCE_NAME_SIZE, "IOAPIC %u", idx);
+		ioapic_setup_resource(idx, res, true);
+		ioapics[idx].resource = res;
+
+		if (insert_resource(&iomem_resource, res)) {
+			kfree(res);
+			res = NULL;
+		}
+	}
+
+	return res;
+}
+
+static void ioapic_remove_resource(int idx)
+{
+	struct resource *res = ioapics[idx].resource;
+
+	if (res) {
+		if (res->parent)
+			release_resource(res);
+		if (PageSlab(virt_to_page(res)))
+			kfree(res);
+		ioapics[idx].resource = NULL;
+	}
+}
+
 void __init ioapic_and_gsi_init(void)
 {
 	int i;
@@ -4191,8 +4227,12 @@ int __mp_register_ioapic(int id, u32 address, u32 gsi_base, bool hotadd)
 		if (gsi_cfg->gsi_end >= gsi_top)
 			gsi_top = gsi_cfg->gsi_end + 1;
 	} else {
-		int irq = reserve_ioapic_gsi_irq_base(idx, true);
+		int irq;
 
+		if (ioapic_add_resource(idx) == NULL)
+			goto failed;
+
+		irq = reserve_ioapic_gsi_irq_base(idx, true);
 		if (irq < 0)
 			goto failed;
 
@@ -4210,6 +4250,7 @@ int __mp_register_ioapic(int id, u32 address, u32 gsi_base, bool hotadd)
 	return 0;
 
 failed:
+	ioapic_remove_resource(idx);
 	clear_fixmap(FIX_IO_APIC_BASE_0 + idx);
 	memset(&ioapics[idx], 0, sizeof(struct ioapic));
 	ioapics[idx].mp_config.apicid = 0xff;
@@ -4234,6 +4275,9 @@ int mp_unregister_ioapic(u32 gsi_base)
 	free_ioapic_gsi_irq_base(idx);
 
 	clear_fixmap(FIX_IO_APIC_BASE_0 + idx);
+
+	ioapic_remove_resource(idx);
+
 	memset(&ioapics[idx], 0, sizeof(struct ioapic));
 	ioapics[idx].mp_config.apicid = 0xff;
 
-- 
1.7.5.4

[PATCH 6/6] x86,IRQ: Use memory barriers to protect searching side code

[Jiang Liu]

Use memory barriers to protect searching side code when hot-plugging
IOAPICs.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/kernel/apic/io_apic.c |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 9c2ac7b..49c016a 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -352,12 +352,15 @@ static void free_ioapic_gsi_irq_base(int idx)
 
 static void alloc_ioapic_saved_registers(int idx)
 {
+	struct mp_ioapic_gsi *gsi_cfg = mp_ioapic_gsi_routing(idx);
+	int irq_cnt = gsi_cfg->gsi_end - gsi_cfg->gsi_base + 1;
+
 	if (ioapics[idx].saved_registers)
 		return;
 
 	ioapics[idx].saved_registers =
 			kzalloc(sizeof(struct IO_APIC_route_entry) *
-				ioapics[idx].nr_registers, GFP_KERNEL);
+				irq_cnt, GFP_KERNEL);
 
 	if (!ioapics[idx].saved_registers)
 		pr_err("IOAPIC %d: suspend/resume impossible!\n", idx);
@@ -1174,6 +1177,8 @@ int IO_APIC_get_PCI_irq_vector(int bus, int slot, int pin,
 		for (ioapic_idx = 0; ioapic_idx < nr_ioapics; ioapic_idx++) {
 			if (!ioapics[ioapic_idx].nr_registers)
 				continue;
+
+			smp_rmb();
 			if (mpc_ioapic_id(ioapic_idx) == mp_irqs[i].dstapic ||
 			    mp_irqs[i].dstapic == MP_APIC_ALL)
 				break;
@@ -2083,7 +2088,7 @@ void __init enable_IO_APIC(void)
 	if (!legacy_pic->nr_legacy_irqs)
 		return;
 
-	for(apic = 0; apic < nr_ioapics; apic++) {
+	for (apic = 0; apic < nr_ioapics; apic++) {
 		int pin;
 		/* See if any of the pins is in ExtINT mode */
 		for (pin = 0; pin < ioapics[apic].nr_registers; pin++) {
@@ -4134,6 +4139,7 @@ int mp_find_ioapic(u32 gsi)
 		if (!ioapics[i].nr_registers)
 			continue;
 
+		smp_rmb();
 		if ((gsi >= gsi_cfg->gsi_base) && (gsi <= gsi_cfg->gsi_end))
 			return i;
 	}
@@ -4214,11 +4220,6 @@ int __mp_register_ioapic(int id, u32 address, u32 gsi_base, bool hotadd)
 	gsi_cfg->gsi_base = gsi_base;
 	gsi_cfg->gsi_end = gsi_base + entries - 1;
 
-	/*
-	 * The number of IO-APIC IRQ registers (== #pins):
-	 */
-	ioapics[idx].nr_registers = entries;
-
 	if (!hotadd) {
 		/*
 		 * irqs will be reserved in arch_early_irq_init()
@@ -4247,6 +4248,13 @@ int __mp_register_ioapic(int id, u32 address, u32 gsi_base, bool hotadd)
 	if (idx == nr_ioapics)
 		nr_ioapics++;
 
+	smp_wmb();
+
+	/*
+	 * The number of IO-APIC IRQ registers (== #pins):
+	 */
+	ioapics[idx].nr_registers = entries;
+
 	return 0;
 
 failed:
@@ -4270,6 +4278,9 @@ int mp_unregister_ioapic(u32 gsi_base)
 	if (idx < 0)
 		return -EINVAL;
 
+	ioapics[idx].nr_registers = 0;
+	smp_wmb();
+
 	free_ioapic_saved_registers(idx);
 
 	free_ioapic_gsi_irq_base(idx);
-- 
1.7.5.4