x86 apic,ioapic fixes
cover x86 apic,ioapic fixes
Hi,
here is a series of trivial patches together
with a few not that insignificant.
Please review.
Thanks, Cyrill
RFC 1/4 x86,ioapic: define IO_APIC_DEFAULT_PHYS_BASE constant
We already have APIC_DEFAULT_PHYS_BASE so just to
be consistent.
--- arch/x86/include/asm/apicdef.h | 3 ++- arch/x86/kernel/mpparse.c | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-)
arch/x86/include/asm/apicdef.h +2 -1
@@ -8,7 +8,8 @@* Ingo Molnar <[email protected]>, 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
arch/x86/kernel/mpparse.c +5 -5
@@ -482,11 +482,11 @@ static void __init construct_ioapic_tablMP_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);/*
[tip:x86/apic] x86, ioapic: Define IO_APIC_DEFAULT_PHYS_BASE constant
Commit-ID: 8f3e1df48baf728bbb0f242c9dff9c9d7108218a Gitweb: http://git.kernel.org/tip/8f3e1df48baf728bbb0f242c9dff9c9d7108218a Author: Cyrill Gorcunov <[email protected]> AuthorDate: Mon, 24 Aug 2009 21:53:36 +0400 Committer: Ingo Molnar <[email protected]> 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.
LKML-Reference: <[email protected]>
--- arch/x86/include/asm/apicdef.h | 3 ++- arch/x86/kernel/mpparse.c | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-)
arch/x86/include/asm/apicdef.h +2 -1
@@ -8,7 +8,8 @@* Ingo Molnar <[email protected]>, 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
arch/x86/kernel/mpparse.c +5 -5
@@ -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);/*
RFC 2/4 x86,e820: __e820_add_region -- guard against array overflowed
In reply to Cyrill GorcunovBetter to be paranoid against unpredicted nr_map modifications.
--- arch/x86/kernel/e820.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/e820.c +1 -1
@@ -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 ; }
[tip:x86/setup] x86, e820: Guard against array overflowed in __e820_add_region()
Commit-ID: 5051fd69773d2d044734b78516317a04d3774871 Gitweb: http://git.kernel.org/tip/5051fd69773d2d044734b78516317a04d3774871 Author: Cyrill Gorcunov <[email protected]> AuthorDate: Mon, 24 Aug 2009 21:53:37 +0400 Committer: Ingo Molnar <[email protected]> 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.
LKML-Reference: <[email protected]>
--- arch/x86/kernel/e820.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/e820.c +1 -1
@@ -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 ; }
RFC 3/4 x86,apic: fix missed handling of discrete apics
In reply to Cyrill GorcunovIn 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.
---
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(-)
arch/x86/kernel/apic/apic.c +1 -1
@@ -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);
arch/x86/kernel/apic/io_apic.c +2 -2
@@ -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 ); }
Re: [rfc -tip 3/4] x86,apic: fix missed handling of discrete apics
Review
Quoted 3 lines · Cyrill Gorcunov
(quoted diff) +3
-0+ 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
Re: [rfc -tip 3/4] x86,apic: fix missed handling of discrete apics
Quoted 12 lines · Ingo Molnar
Review
Quoted 3 lines · Cyrill Gorcunov
(quoted diff) +3
-0+ 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!
Re: [rfc -tip 3/4] x86,apic: fix missed handling of discrete apics
In reply to Ingo Molnar[Ingo Molnar - Wed, Aug 26, 2009 at 08:12:41AM +0200]
| | * Cyrill Gorcunov <[email protected]> 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.
---
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(-)
arch/x86/include/asm/apic.h +13 -0
@@ -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
arch/x86/kernel/apic/apic.c +1 -1
@@ -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);
arch/x86/kernel/apic/io_apic.c +2 -2
@@ -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 ); }
RFC 4/4 x86,ioapic: ioapic_setup_resources - get rid of needless check and simplify
In reply to Cyrill Gorcunovalloc_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.
--- arch/x86/kernel/apic/io_apic.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)
arch/x86/kernel/apic/io_apic.c +11 -15
@@ -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++; } }
[tip:x86/apic] x86, ioapic: Get rid of needless check and simplify ioapic_setup_resources()
Commit-ID: ffc438366c2660a6a811b94ba33229bf217f8254 Gitweb: http://git.kernel.org/tip/ffc438366c2660a6a811b94ba33229bf217f8254 Author: Cyrill Gorcunov <[email protected]> AuthorDate: Mon, 24 Aug 2009 21:53:39 +0400 Committer: Ingo Molnar <[email protected]> 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.
LKML-Reference: <[email protected]>
--- arch/x86/kernel/apic/io_apic.c | 26 +++++++++++--------------- 1 files changed, 11 insertions(+), 15 deletions(-)
arch/x86/kernel/apic/io_apic.c +11 -15
@@ -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++; } }