[rfc -tip 0/4] x86 apic,ioapic fixes

[rfc -tip 0/4] x86 apic,ioapic fixes

[Cyrill Gorcunov]

Hi,

here is a series of trivial patches together
with a few not that insignificant.

Please review.

Thanks, Cyrill

[rfc -tip 1/4] x86,ioapic: define IO_APIC_DEFAULT_PHYS_BASE constant

[Cyrill Gorcunov]

[-- Attachment #1: x86-io-apic-def-base --]
[-- Type: text/plain, Size: 1411 bytes --]

We already have APIC_DEFAULT_PHYS_BASE so just to
be consistent.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/include/asm/apicdef.h |    3 ++-
 arch/x86/kernel/mpparse.c      |   10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/apicdef.h
=====================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/apicdef.h
+++ linux-2.6.git/arch/x86/include/asm/apicdef.h
@@ -8,7 +8,8 @@
  * Ingo Molnar <mingo@redhat.com>, 1999, 2000
  */
 
-#define	APIC_DEFAULT_PHYS_BASE	0xfee00000
+#define IO_APIC_DEFAULT_PHYS_BASE	0xfec00000
+#define	APIC_DEFAULT_PHYS_BASE		0xfee00000
 
 #define	APIC_ID		0x20
 
Index: linux-2.6.git/arch/x86/kernel/mpparse.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/mpparse.c
+++ linux-2.6.git/arch/x86/kernel/mpparse.c
@@ -482,11 +482,11 @@ static void __init construct_ioapic_tabl
 		MP_bus_info(&bus);
 	}
 
-	ioapic.type = MP_IOAPIC;
-	ioapic.apicid = 2;
-	ioapic.apicver = mpc_default_type > 4 ? 0x10 : 0x01;
-	ioapic.flags = MPC_APIC_USABLE;
-	ioapic.apicaddr = 0xFEC00000;
+	ioapic.type	= MP_IOAPIC;
+	ioapic.apicid	= 2;
+	ioapic.apicver	= mpc_default_type > 4 ? 0x10 : 0x01;
+	ioapic.flags	= MPC_APIC_USABLE;
+	ioapic.apicaddr	= IO_APIC_DEFAULT_PHYS_BASE;
 	MP_ioapic_info(&ioapic);
 
 	/*

[rfc -tip 2/4] x86,e820: __e820_add_region -- guard against array overflowed

[Cyrill Gorcunov]

[-- Attachment #1: x86-e820-array --]
[-- Type: text/plain, Size: 654 bytes --]

Better to be paranoid against unpredicted nr_map modifications.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/kernel/e820.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.git/arch/x86/kernel/e820.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/e820.c
+++ linux-2.6.git/arch/x86/kernel/e820.c
@@ -115,7 +115,7 @@ static void __init __e820_add_region(str
 {
 	int x = e820x->nr_map;
 
-	if (x == ARRAY_SIZE(e820x->map)) {
+	if (x >= ARRAY_SIZE(e820x->map)) {
 		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
 		return;
 	}

[rfc -tip 3/4] x86,apic: fix missed handling of discrete apics

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-discrete-fixups-2 --]
[-- Type: text/plain, Size: 1914 bytes --]

In case of discrete (pretty old) apics we may have
cpu_has_apic bit not set but have to check if
smp_found_config (MP spec) is there and apic was
not disabled. So in case of discrete apic we need
to proceed apic handling.

Also don't forget to print apic/io-apic for such
case as well.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: "Maciej W. Rozycki" <macro@linux-mips.org>
---

Perhaps this smp_found_config,disable_apic (which are
useful for insoectation in x86-32 only apart of cpu_has_apic)
are good candidate for helper function, say lapic_from_mp.
Dunno.Please comment.

 arch/x86/kernel/apic/apic.c    |    2 +-
 arch/x86/kernel/apic/io_apic.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/apic/apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/apic.c
@@ -978,7 +978,7 @@ void lapic_shutdown(void)
 {
 	unsigned long flags;
 
-	if (!cpu_has_apic)
+	if (!cpu_has_apic && (!smp_found_config || disable_apic))
 		return;
 
 	local_irq_save(flags);
Index: linux-2.6.git/arch/x86/kernel/apic/io_apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/io_apic.c
@@ -1860,7 +1860,7 @@ __apicdebuginit(int) print_all_ICs(void)
 	print_PIC();
 
 	/* don't print out if apic is not there */
-	if (!cpu_has_apic || disable_apic)
+	if (!cpu_has_apic && (!smp_found_config || disable_apic))
 		return 0;
 
 	print_all_local_APICs();
@@ -1978,7 +1978,7 @@ void disable_IO_APIC(void)
 	/*
 	 * Use virtual wire A mode when interrupt remapping is enabled.
 	 */
-	if (cpu_has_apic)
+	if (cpu_has_apic || (smp_found_config && !disable_apic))
 		disconnect_bsp_APIC(!intr_remapping_enabled &&
 				ioapic_i8259.pin != -1);
 }

[rfc -tip 4/4] x86,ioapic: ioapic_setup_resources - get rid of needless check and simplify

[Cyrill Gorcunov]

[-- Attachment #1: x86-io-apic-bootmem --]
[-- Type: text/plain, Size: 2385 bytes --]

alloc_bootmem do panic on allication failure.
There is no need to check the result.

Also there is a way to unbind global variable from its
body and use it as a parameter which allow us to simplify
ioapic_init_mappings as well -- "for" cycle already uses
nr_ioapics as a conditional variable and there is no need
to check if ioapic_setup_resources was returning NULL again.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Yinghai Lu <yinghai@kernel.org>
---
 arch/x86/kernel/apic/io_apic.c |   26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

Index: linux-2.6.git/arch/x86/kernel/apic/io_apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/io_apic.c
@@ -4056,7 +4056,7 @@ void __init setup_ioapic_dest(void)
 
 static struct resource *ioapic_resources;
 
-static struct resource * __init ioapic_setup_resources(void)
+static struct resource * __init ioapic_setup_resources(int nr_ioapics)
 {
 	unsigned long n;
 	struct resource *res;
@@ -4072,15 +4072,13 @@ static struct resource * __init ioapic_s
 	mem = alloc_bootmem(n);
 	res = (void *)mem;
 
-	if (mem != NULL) {
-		mem += sizeof(struct resource) * nr_ioapics;
+	mem += sizeof(struct resource) * nr_ioapics;
 
-		for (i = 0; i < nr_ioapics; i++) {
-			res[i].name = mem;
-			res[i].flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-			sprintf(mem,  "IOAPIC %u", i);
-			mem += IOAPIC_RESOURCE_NAME_SIZE;
-		}
+	for (i = 0; i < nr_ioapics; i++) {
+		res[i].name = mem;
+		res[i].flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+		sprintf(mem,  "IOAPIC %u", i);
+		mem += IOAPIC_RESOURCE_NAME_SIZE;
 	}
 
 	ioapic_resources = res;
@@ -4094,7 +4092,7 @@ void __init ioapic_init_mappings(void)
 	struct resource *ioapic_res;
 	int i;
 
-	ioapic_res = ioapic_setup_resources();
+	ioapic_res = ioapic_setup_resources(nr_ioapics);
 	for (i = 0; i < nr_ioapics; i++) {
 		if (smp_found_config) {
 			ioapic_phys = mp_ioapics[i].apicaddr;
@@ -4123,11 +4121,9 @@ fake_ioapic_page:
 			    __fix_to_virt(idx), ioapic_phys);
 		idx++;
 
-		if (ioapic_res != NULL) {
-			ioapic_res->start = ioapic_phys;
-			ioapic_res->end = ioapic_phys + (4 * 1024) - 1;
-			ioapic_res++;
-		}
+		ioapic_res->start = ioapic_phys;
+		ioapic_res->end = ioapic_phys + (4 * 1024) - 1;
+		ioapic_res++;
 	}
 }
 

Re: [rfc -tip 3/4] x86,apic: fix missed handling of discrete apics

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> +	if (!cpu_has_apic && (!smp_found_config || disable_apic))
> +	if (!cpu_has_apic && (!smp_found_config || disable_apic))
> +	if (cpu_has_apic || (smp_found_config && !disable_apic))

these are now repeating patterns, so that should be moved into a 
helper inline or so.

	Ingo

[tip:x86/apic] x86, ioapic: Define IO_APIC_DEFAULT_PHYS_BASE constant

[tip-bot for Cyrill Gorcunov]

Commit-ID:  8f3e1df48baf728bbb0f242c9dff9c9d7108218a
Gitweb:     http://git.kernel.org/tip/8f3e1df48baf728bbb0f242c9dff9c9d7108218a
Author:     Cyrill Gorcunov <gorcunov@openvz.org>
AuthorDate: Mon, 24 Aug 2009 21:53:36 +0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 26 Aug 2009 08:16:38 +0200

x86, ioapic: Define IO_APIC_DEFAULT_PHYS_BASE constant

We already have APIC_DEFAULT_PHYS_BASE so just to be
consistent.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
LKML-Reference: <20090824175550.927946757@openvz.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/include/asm/apicdef.h |    3 ++-
 arch/x86/kernel/mpparse.c      |   10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 7ddb36a..7386bfa 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -8,7 +8,8 @@
  * Ingo Molnar <mingo@redhat.com>, 1999, 2000
  */
 
-#define	APIC_DEFAULT_PHYS_BASE	0xfee00000
+#define IO_APIC_DEFAULT_PHYS_BASE	0xfec00000
+#define	APIC_DEFAULT_PHYS_BASE		0xfee00000
 
 #define	APIC_ID		0x20
 
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index 651c93b..fcd513b 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -482,11 +482,11 @@ static void __init construct_ioapic_table(int mpc_default_type)
 		MP_bus_info(&bus);
 	}
 
-	ioapic.type = MP_IOAPIC;
-	ioapic.apicid = 2;
-	ioapic.apicver = mpc_default_type > 4 ? 0x10 : 0x01;
-	ioapic.flags = MPC_APIC_USABLE;
-	ioapic.apicaddr = 0xFEC00000;
+	ioapic.type	= MP_IOAPIC;
+	ioapic.apicid	= 2;
+	ioapic.apicver	= mpc_default_type > 4 ? 0x10 : 0x01;
+	ioapic.flags	= MPC_APIC_USABLE;
+	ioapic.apicaddr	= IO_APIC_DEFAULT_PHYS_BASE;
 	MP_ioapic_info(&ioapic);
 
 	/*

Re: [rfc -tip 3/4] x86,apic: fix missed handling of discrete apics

[Cyrill Gorcunov]

On 8/26/09, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>
>> +	if (!cpu_has_apic && (!smp_found_config || disable_apic))
>> +	if (!cpu_has_apic && (!smp_found_config || disable_apic))
>> +	if (cpu_has_apic || (smp_found_config && !disable_apic))
>
> these are now repeating patterns, so that should be moved into a
> helper inline or so.
>
> 	Ingo
>
Absolutely yes. Though i didn't invent more or less convenient name
for this helper. lapic_from_mp perhaps. Have to think. Thanks for
review, Ingo!

[tip:x86/apic] x86, ioapic: Get rid of needless check and simplify ioapic_setup_resources()

[tip-bot for Cyrill Gorcunov]

Commit-ID:  ffc438366c2660a6a811b94ba33229bf217f8254
Gitweb:     http://git.kernel.org/tip/ffc438366c2660a6a811b94ba33229bf217f8254
Author:     Cyrill Gorcunov <gorcunov@openvz.org>
AuthorDate: Mon, 24 Aug 2009 21:53:39 +0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 26 Aug 2009 08:16:38 +0200

x86, ioapic: Get rid of needless check and simplify ioapic_setup_resources()

alloc_bootmem() already panics on allocation failure. There is
no need to check the result.

Also there is a way to unbind global variable from its body and
use it as a parameter which allow us to simplify
ioapic_init_mappings as well -- "for" cycle already uses
nr_ioapics as a conditional variable and there is no need to
check if ioapic_setup_resources was returning NULL again.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Yinghai Lu <yinghai@kernel.org>
LKML-Reference: <20090824175551.493629148@openvz.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/apic/io_apic.c |   26 +++++++++++---------------
 1 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 2999f3d..d836b4d 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -4053,7 +4053,7 @@ void __init setup_ioapic_dest(void)
 
 static struct resource *ioapic_resources;
 
-static struct resource * __init ioapic_setup_resources(void)
+static struct resource * __init ioapic_setup_resources(int nr_ioapics)
 {
 	unsigned long n;
 	struct resource *res;
@@ -4069,15 +4069,13 @@ static struct resource * __init ioapic_setup_resources(void)
 	mem = alloc_bootmem(n);
 	res = (void *)mem;
 
-	if (mem != NULL) {
-		mem += sizeof(struct resource) * nr_ioapics;
+	mem += sizeof(struct resource) * nr_ioapics;
 
-		for (i = 0; i < nr_ioapics; i++) {
-			res[i].name = mem;
-			res[i].flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-			sprintf(mem,  "IOAPIC %u", i);
-			mem += IOAPIC_RESOURCE_NAME_SIZE;
-		}
+	for (i = 0; i < nr_ioapics; i++) {
+		res[i].name = mem;
+		res[i].flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+		sprintf(mem,  "IOAPIC %u", i);
+		mem += IOAPIC_RESOURCE_NAME_SIZE;
 	}
 
 	ioapic_resources = res;
@@ -4091,7 +4089,7 @@ void __init ioapic_init_mappings(void)
 	struct resource *ioapic_res;
 	int i;
 
-	ioapic_res = ioapic_setup_resources();
+	ioapic_res = ioapic_setup_resources(nr_ioapics);
 	for (i = 0; i < nr_ioapics; i++) {
 		if (smp_found_config) {
 			ioapic_phys = mp_ioapics[i].apicaddr;
@@ -4120,11 +4118,9 @@ fake_ioapic_page:
 			    __fix_to_virt(idx), ioapic_phys);
 		idx++;
 
-		if (ioapic_res != NULL) {
-			ioapic_res->start = ioapic_phys;
-			ioapic_res->end = ioapic_phys + (4 * 1024) - 1;
-			ioapic_res++;
-		}
+		ioapic_res->start = ioapic_phys;
+		ioapic_res->end = ioapic_phys + (4 * 1024) - 1;
+		ioapic_res++;
 	}
 }
 

[tip:x86/setup] x86, e820: Guard against array overflowed in __e820_add_region()

[tip-bot for Cyrill Gorcunov]

Commit-ID:  5051fd69773d2d044734b78516317a04d3774871
Gitweb:     http://git.kernel.org/tip/5051fd69773d2d044734b78516317a04d3774871
Author:     Cyrill Gorcunov <gorcunov@openvz.org>
AuthorDate: Mon, 24 Aug 2009 21:53:37 +0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 26 Aug 2009 08:17:47 +0200

x86, e820: Guard against array overflowed in __e820_add_region()

Better to be paranoid against unpredicted nr_map modifications.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
LKML-Reference: <20090824175551.146070377@openvz.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/e820.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 7271fa3..2e5e0fa 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -115,7 +115,7 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
 {
 	int x = e820x->nr_map;
 
-	if (x == ARRAY_SIZE(e820x->map)) {
+	if (x >= ARRAY_SIZE(e820x->map)) {
 		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
 		return;
 	}

Re: [rfc -tip 3/4] x86,apic: fix missed handling of discrete apics

[Cyrill Gorcunov]

[Ingo Molnar - Wed, Aug 26, 2009 at 08:12:41AM +0200]
| 
| * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
| 
| > +	if (!cpu_has_apic && (!smp_found_config || disable_apic))
| > +	if (!cpu_has_apic && (!smp_found_config || disable_apic))
| > +	if (cpu_has_apic || (smp_found_config && !disable_apic))
| 
| these are now repeating patterns, so that should be moved into a 
| helper inline or so.
| 
| 	Ingo
| 

Something like this perhaps? I'm open for new proposals :)

	-- Cyrill
---
x86,apic: fix missed handling of discrete apics

In case of discrete (pretty old) apics we may have
cpu_has_apic bit not set but have to check if
smp_found_config (MP spec) is there and apic was
not disabled.

Also don't forget to print apic/io-apic for such
case as well.

CC: "Maciej W. Rozycki" <macro@linux-mips.org>
CC: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

I thought about use plain (smp_found_config & disable_apic)
but it would look fishy and fragile.

Also I don't use #ifdef (yes, this helper make sense
for x86-32 mostly since on 64bit we rely on cpuid
and additional checking is not needed, but since
the helper is called only three times and mostly
during shutdown procedure -- a few ticks would not
hurt much but save us from code deformation and
make the helper more "general" in sense like
just checking two flags at once).

Note as well that cpu_has_config is not incorporated
into the helper by purpose. Lets leave this bit
being cheking explicitly. One day we would not
need this helper (it's already that hard to find
discrete apic systems not reporting apic bit via
cpuid (amdk5 which uses this bit for PGE support
is not taken into account :)

Please review. Comments are appreciated.

 arch/x86/include/asm/apic.h    |   13 +++++++++++++
 arch/x86/kernel/apic/apic.c    |    2 +-
 arch/x86/kernel/apic/io_apic.c |    4 ++--
 3 files changed, 16 insertions(+), 3 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/apic.h
=====================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/apic.h
+++ linux-2.6.git/arch/x86/include/asm/apic.h
@@ -66,6 +66,19 @@ static inline void default_inquire_remot
 }
 
 /*
+ * With 82489DX we can't rely on apic feature bit
+ * retrieved via cpuid but still have to deal with
+ * such an apic chip so we assume that SMP configuration
+ * is found from MP table (64bit case uses ACPI mostly
+ * which set smp presence flag as well so we are safe
+ * to use this helper too). --cvg
+ */
+static inline bool apic_from_smp_config(void)
+{
+	return (smp_found_config && !disable_apic);
+}
+
+/*
  * Basic functions accessing APICs.
  */
 #ifdef CONFIG_PARAVIRT
Index: linux-2.6.git/arch/x86/kernel/apic/apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/apic.c
@@ -978,7 +978,7 @@ void lapic_shutdown(void)
 {
 	unsigned long flags;
 
-	if (!cpu_has_apic)
+	if (!cpu_has_apic && !apic_from_smp_config())
 		return;
 
 	local_irq_save(flags);
Index: linux-2.6.git/arch/x86/kernel/apic/io_apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/io_apic.c
@@ -1860,7 +1860,7 @@ __apicdebuginit(int) print_all_ICs(void)
 	print_PIC();
 
 	/* don't print out if apic is not there */
-	if (!cpu_has_apic || disable_apic)
+	if (!cpu_has_apic && !apic_from_smp_config())
 		return 0;
 
 	print_all_local_APICs();
@@ -1978,7 +1978,7 @@ void disable_IO_APIC(void)
 	/*
 	 * Use virtual wire A mode when interrupt remapping is enabled.
 	 */
-	if (cpu_has_apic)
+	if (cpu_has_apic || apic_from_smp_config())
 		disconnect_bsp_APIC(!intr_remapping_enabled &&
 				ioapic_i8259.pin != -1);
 }