x86_64: clear IO_APIC before enabing apic error vector. v2

PATCH In review Yinghai Lu 4 participants 15 messages Dec 30, 2007 – Jan 1, 2008 x86

lore

Share & tools

lore

Yinghai Lu

author

2007-12-30 03:48 UTC #0 lore raw

PATCH 1/1 x86_64: clear IO_APIC before enabing apic error vector. v2

please check if you can replace the one in the x86-mm

http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76

the updated one avoid one link warning.

[PATCH] x86_64: clear IO_APIC before enabing apic error vector. v2

some apic id lifting system: 4 socket quad core, 8 socket quad core will do apic id lifting for BSP.

but io-apic regs for ExtINT still use 0 as dest.

so when we enable apic error vector in BSP, we will get one APIC error.

CPU: L1 I Cache: 64K (64 bytes/line), D cache 64K (64 bytes/line)
CPU: L2 Cache: 512K (64 bytes/line)
CPU 0/4 -> Node 0
CPU: Physical Processor ID: 1
CPU: Processor Core ID: 0
SMP alternatives: switching to UP code
ACPI: Core revision 20070126
enabled ExtINT on CPU#0
ESR value after enabling vector: 00000000, after 0000000c APIC error on CPU0: 0c(08)
ENABLING IO-APIC IRQs
Synchronizing Arb IDs.

So move enable_IO_APIC from setup_IO_APIC into setup_local_APIC and call it before enabling apic error vector.

this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.

Signed-off-by Yinghai Lu <[email protected]>

arch/x86/kernel/apic_64.c +14 -2

Index: linux-2.6/arch/x86/kernel/apic_64.c
===================================================================
@@ -418,7 +418,7 @@ void __init init_bsp_APIC(void)
 	apic_write(APIC_LVT1, value);
 }
 
-void __cpuinit setup_local_APIC (void)
+void __cpuinit setup_local_APIC (void (*extra_call)(void))
 {
 	unsigned int value, maxlvt;
 	int i, j;

@@ -517,6 +517,13 @@ void __cpuinit setup_local_APIC (void)
 		value = APIC_DM_NMI | APIC_LVT_MASKED;
 	apic_write(APIC_LVT1, value);
 
+	/*
+	 * Now enable IO-APICs, actually call clear_IO_APIC
+	 * We need clear_IO_APIC before enabling vector on BP
+	 */
+	if (extra_call)
+		(*extra_call)();
+
 	{
 		unsigned oldvalue;
 		maxlvt = get_maxlvt();

@@ -1198,6 +1205,8 @@ int disable_apic;
  */
 int __init APIC_init_uniprocessor (void)
 {
+	void (*extra_call)(void) = NULL;
+
 	if (disable_apic) {
 		printk(KERN_INFO "Apic disabled\n");
 		return -1;

@@ -1213,7 +1222,10 @@ int __init APIC_init_uniprocessor (void)
 	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_id);
 	apic_write(APIC_ID, SET_APIC_ID(boot_cpu_id));
 
-	setup_local_APIC();
+	if (!skip_ioapic_setup && nr_ioapics)
+		extra_call = enable_IO_APIC;
+
+	setup_local_APIC(extra_call);
 
 	if (smp_found_config && !skip_ioapic_setup && nr_ioapics)
 		setup_IO_APIC();

arch/x86/kernel/io_apic_64.c +5 -2

Index: linux-2.6/arch/x86/kernel/io_apic_64.c
===================================================================
@@ -1171,7 +1171,7 @@ void __apicdebuginit print_PIC(void)
 
 #endif  /*  0  */
 
-static void __init enable_IO_APIC(void)
+void __init enable_IO_APIC(void)
 {
 	union IO_APIC_reg_01 reg_01;
 	int i8259_apic, i8259_pin;

@@ -1790,7 +1790,10 @@ __setup("no_timer_check", notimercheck);
 
 void __init setup_IO_APIC(void)
 {
-	enable_IO_APIC();
+
+	/*
+	 * calling enable_IO_APIC() is moved to setup_local_APIC for BP
+	 */
 
 	if (acpi_ioapic)
 		io_apic_irqs = ~0;	/* all IRQs go through IOAPIC */

include/asm-x86/hw_irq_64.h +1 -0

Index: linux-2.6/include/asm-x86/hw_irq_64.h
===================================================================
@@ -135,6 +135,7 @@ extern void init_8259A(int aeoi);
 extern void send_IPI_self(int vector);
 extern void init_VISWS_APIC_irqs(void);
 extern void setup_IO_APIC(void);
+extern void enable_IO_APIC(void);
 extern void disable_IO_APIC(void);
 extern void print_IO_APIC(void);
 extern int IO_APIC_get_PCI_irq_vector(int bus, int slot, int fn);

arch/x86/kernel/smpboot_64.c +7 -2

Index: linux-2.6/arch/x86/kernel/smpboot_64.c
===================================================================
@@ -211,7 +211,7 @@ void __cpuinit smp_callin(void)
 	 */
 
 	Dprintk("CALLIN, before setup_local_APIC().\n");
-	setup_local_APIC();
+	setup_local_APIC(NULL);
 
 	/*
 	 * Get our bogomips.

@@ -879,6 +879,8 @@ static void __init smp_cpu_index_default
  */
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
+	void (*extra_call)(void) = NULL;
+
 	nmi_watchdog_default();
 	smp_cpu_index_default();
 	current_cpu_data = boot_cpu_data;

@@ -896,7 +898,10 @@ void __init smp_prepare_cpus(unsigned in
 	/*
 	 * Switch from PIC to APIC mode.
 	 */
-	setup_local_APIC();
+        if (!skip_ioapic_setup && nr_ioapics)
+                extra_call = enable_IO_APIC;
+
+	setup_local_APIC(extra_call);
 
 	if (GET_APIC_ID(apic_read(APIC_ID)) != boot_cpu_id) {
 		panic("Boot APIC ID in local APIC unexpected (%d vs %d)",

include/asm-x86/apic_64.h +1 -1

Index: linux-2.6/include/asm-x86/apic_64.h
===================================================================
@@ -74,7 +74,7 @@ extern int verify_local_APIC (void);
 extern void cache_APIC_registers (void);
 extern void sync_Arb_IDs (void);
 extern void init_bsp_APIC (void);
-extern void setup_local_APIC (void);
+extern void setup_local_APIC (void (*extra_call)(void));
 extern void init_apic_mappings (void);
 extern void smp_local_timer_interrupt (void);
 extern void setup_boot_APIC_clock (void);

Ingo Molnar

2007-12-30 14:55 UTC #1 lore raw

Quoted 5 lines · Yinghai Lu

please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.

please send delta patches instead - so that we can review the changes.

Quoted 2 lines

this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.

hm, what link warning did you get? Perhaps the following __cpuinit:

Quoted 1 line

-void __cpuinit setup_local_APIC (void)

does not mix well with an __init call:

Quoted 1 line

+void __init enable_IO_APIC(void)

and you hack it around by using a function pointer. Nasty and still buggy. The proper solution would be to mark enable_IO_APIC as __cpuinit too (or something like that).

Ingo

Yinghai Lu

2007-12-30 20:48 UTC #2 lore raw

Quoted 10 lines · Ingo Molnar

Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.

will do that in another patch.

Quoted 5 lines

Quoted 3 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:

WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')

Quoted 10 lines

Quoted 2 lines
-void __cpuinit setup_local_APIC (void)
does not mix well with an __init call:
Quoted 1 line
+void __init enable_IO_APIC(void)
and you hack it around by using a function pointer. Nasty and still buggy. The proper solution would be to mark enable_IO_APIC as __cpuinit too (or something like that).

after change that to __cpuinit, i got

WARNING: vmlinux.o(.text+0x17f84): Section mismatch: reference to .init.text: (between 'enable_IO_APIC' and 'ioapic_resume')
WARNING: vmlinux.o(.text+0x17f92): Section mismatch: reference to .init.text: (between 'enable_IO_APIC' and 'ioapic_resume')

Adrian Bunk

2007-12-30 22:06 UTC #3 lore raw

Quoted 15 lines · Yinghai Lu

Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.
will do that in another patch.
Quoted 1 line

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:
WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')

So you are doing complicated things for silencing the warning (there is an easier ways for achieving it), but the real bug that you will get an Oops when calling enable_IO_APIC() after bootup since it already got freed stays?

Quoted 10 lines

Quoted 9 lines
Quoted 1 line
-void __cpuinit setup_local_APIC (void)
does not mix well with an __init call:
Quoted 1 line
+void __init enable_IO_APIC(void)
and you hack it around by using a function pointer. Nasty and still buggy. The proper solution would be to mark enable_IO_APIC as __cpuinit too (or something like that).

Quoted 6 lines

after change that to __cpuinit, i got
WARNING: vmlinux.o(.text+0x17f84): Section mismatch: reference to .init.text: (between 'enable_IO_APIC' and 'ioapic_resume')
WARNING: vmlinux.o(.text+0x17f92): Section mismatch: reference to .init.text: (between 'enable_IO_APIC' and 'ioapic_resume')

find_isa_irq_{apic,pin} are called by enable_IO_APIC() and therefore also have to be changed from __init to __cpuinit.

Quoted 1 line

YH

cu
Adrian

BTW: Is there a reason why your patch doesn't touch the 32bit code?

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Yinghai Lu

2007-12-30 22:43 UTC #4 lore raw

Quoted 21 lines · Adrian Bunk

Quoted 20 lines · Yinghai Lu
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.
will do that in another patch.
Quoted 5 lines
Quoted 3 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:

Quoted 8 lines

Quoted 3 lines
WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')
So you are doing complicated things for silencing the warning (there is an easier ways for achieving it), but the real bug that you will get an Oops when calling enable_IO_APIC() after bootup since it already got freed stays?

the enable_IO_APIC is actually doing clear_IO_APIC. and it is only called by BSP via setup_local_APIC
and it is not called again after bootup

Adrian Bunk

2007-12-30 23:24 UTC #5 lore raw

Quoted 26 lines · Yinghai Lu

Quoted 25 lines · Adrian Bunk
Quoted 23 lines · Yinghai Lu
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.
will do that in another patch.
Quoted 5 lines
Quoted 3 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:
WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')

Quoted 8 lines

Quoted 4 lines
So you are doing complicated things for silencing the warning (there is an easier ways for achieving it), but the real bug that you will get an Oops when calling enable_IO_APIC() after bootup since it already got freed stays?
the enable_IO_APIC is actually doing clear_IO_APIC. and it is only called by BSP via setup_local_APIC
and it is not called again after bootup

OK, this was a bit hidden inside your pointer games.

Please send a patch that ignores the warning (and therefore doesn't do these pointer games), and I'll fix the warning in a followup patch.

Quoted 1 line

YH

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Yinghai Lu

2007-12-31 00:01 UTC #6 lore raw

Quoted 33 lines · Adrian Bunk

Quoted 31 lines · Yinghai Lu
Quoted 29 lines · Adrian Bunk
Quoted 15 lines · Yinghai Lu
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.
will do that in another patch.
Quoted 1 line
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:
WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')
So you are doing complicated things for silencing the warning (there is an easier ways for achieving it), but the real bug that you will get an Oops when calling enable_IO_APIC() after bootup since it already got freed stays?

Quoted 8 lines

Quoted 3 lines
the enable_IO_APIC is actually doing clear_IO_APIC. and it is only called by BSP via setup_local_APIC
and it is not called again after bootup
OK, this was a bit hidden inside your pointer games.
Please send a patch that ignores the warning (and therefore doesn't do these pointer games), and I'll fix the warning in a followup patch.

the old one is in x86-mm tree

http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76

function call delta
http://lkml.org/lkml/2007/12/30/211

thanks

Adrian Bunk

2007-12-31 00:29 UTC #7 lore raw

Quoted 40 lines · Yinghai Lu

Quoted 39 lines · Adrian Bunk
Quoted 26 lines · Yinghai Lu
Quoted 25 lines · Adrian Bunk
Quoted 23 lines · Yinghai Lu
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.
will do that in another patch.
Quoted 5 lines
Quoted 3 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:
WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')
Quoted 8 lines
Quoted 4 lines
So you are doing complicated things for silencing the warning (there is an easier ways for achieving it), but the real bug that you will get an Oops when calling enable_IO_APIC() after bootup since it already got freed stays?
the enable_IO_APIC is actually doing clear_IO_APIC. and it is only called by BSP via setup_local_APIC
and it is not called again after bootup
OK, this was a bit hidden inside your pointer games.

Quoted 7 lines

Quoted 2 lines
Please send a patch that ignores the warning (and therefore doesn't do these pointer games), and I'll fix the warning in a followup patch.
the old one is in x86-mm tree
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
...

Sorry for the dumb question, but what in

+ if (!smp_processor_id() && !skip_ioapic_setup && nr_ioapics)
+ enable_IO_APIC();

guarantees that this call doesn't happen when you hotplug CPU 0 ?

Quoted 3 lines

thanks
YH

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Yinghai Lu

2007-12-31 00:56 UTC #8 lore raw

Quoted 55 lines · Adrian Bunk

Quoted 40 lines · Yinghai Lu
Quoted 39 lines · Adrian Bunk
Quoted 26 lines · Yinghai Lu
Quoted 25 lines · Adrian Bunk
Quoted 23 lines · Yinghai Lu
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.
will do that in another patch.
Quoted 5 lines
Quoted 3 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:
WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')
Quoted 8 lines
Quoted 4 lines
So you are doing complicated things for silencing the warning (there is an easier ways for achieving it), but the real bug that you will get an Oops when calling enable_IO_APIC() after bootup since it already got freed stays?
the enable_IO_APIC is actually doing clear_IO_APIC. and it is only called by BSP via setup_local_APIC
and it is not called again after bootup
OK, this was a bit hidden inside your pointer games.
Quoted 7 lines
Quoted 2 lines
Please send a patch that ignores the warning (and therefore doesn't do these pointer games), and I'll fix the warning in a followup patch.
the old one is in x86-mm tree
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
...
Sorry for the dumb question, but what in
(quoted diff) +2 -0
+       if (!smp_processor_id() && !skip_ioapic_setup && nr_ioapics)
+               enable_IO_APIC();

Quoted 1 line

guarantees that this call doesn't happen when you hotplug CPU 0 ?

so we can hotplug cpu0 or the bsp?

Adrian Bunk

2008-01-01 23:17 UTC #9 lore raw

Quoted 10 lines · Yinghai Lu

On Dec 30, 2007 4:28 PM, Adrian Bunk <[email protected]> wrote:
...
Quoted 6 lines
Sorry for the dumb question, but what in
(quoted diff) +2 -0
+       if (!smp_processor_id() && !skip_ioapic_setup && nr_ioapics)
+               enable_IO_APIC();
guarantees that this call doesn't happen when you hotplug CPU 0 ?
so we can hotplug cpu0 or the bsp?

Honestly I don't know the amd64 architecture well enough for telling whether the hardware allows it or not.

Quoted 1 line

YH

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Yinghai Lu

2007-12-31 01:04 UTC #10 lore raw

In reply to Adrian Bunk

Quoted 55 lines · Adrian Bunk

Quoted 40 lines · Yinghai Lu
Quoted 39 lines · Adrian Bunk
Quoted 26 lines · Yinghai Lu
Quoted 25 lines · Adrian Bunk
Quoted 23 lines · Yinghai Lu
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Yinghai Lu
please check if you can replace the one in the x86-mm
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
the updated one avoid one link warning.
please send delta patches instead - so that we can review the changes.
will do that in another patch.
Quoted 5 lines
Quoted 3 lines
this is the updated verison that take enable_IO_APIC as extra call for setup_local_APIC to avoid linking warning.
hm, what link warning did you get? Perhaps the following __cpuinit:
WARNING: vmlinux.o(.text+0x163d5): Section mismatch: reference to .init.text:enable_IO_APIC (between 'setup_local_APIC' and 'apic_is_clustered_box')
Quoted 8 lines
Quoted 4 lines
So you are doing complicated things for silencing the warning (there is an easier ways for achieving it), but the real bug that you will get an Oops when calling enable_IO_APIC() after bootup since it already got freed stays?
the enable_IO_APIC is actually doing clear_IO_APIC. and it is only called by BSP via setup_local_APIC
and it is not called again after bootup
OK, this was a bit hidden inside your pointer games.
Quoted 7 lines
Quoted 2 lines
Please send a patch that ignores the warning (and therefore doesn't do these pointer games), and I'll fix the warning in a followup patch.
the old one is in x86-mm tree
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
...
Sorry for the dumb question, but what in
(quoted diff) +2 -0
+       if (!smp_processor_id() && !skip_ioapic_setup && nr_ioapics)
+               enable_IO_APIC();

Quoted 2 lines

guarantees that this call doesn't happen when you hotplug CPU 0 ?

if CPU 0 (BSP) can be hotplug, it will be restarted via smp_callin. and the delta function call patch will use setup_local_APIC(NULL), then it will be safe.

Adrian Bunk

2008-01-01 23:18 UTC #11 lore raw

Quoted 14 lines · Yinghai Lu

Quoted 2 lines · Adrian Bunk
On Sun, Dec 30, 2007 at 04:01:39PM -0800, Yinghai Lu wrote:
...
Quoted 10 lines
Quoted 4 lines
the old one is in x86-mm tree
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=ffcbdc220a1520d006a837f33589c7c19ffbeb76
...
Sorry for the dumb question, but what in
(quoted diff) +2 -0
+       if (!smp_processor_id() && !skip_ioapic_setup && nr_ioapics)
+               enable_IO_APIC();

Quoted 6 lines

Quoted 2 lines
guarantees that this call doesn't happen when you hotplug CPU 0 ?
if CPU 0 (BSP) can be hotplug, it will be restarted via smp_callin. and the delta function call patch will use setup_local_APIC(NULL), then it will be safe.

We are talking about the version without the pointer games.

Quoted 1 line

YH

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Yinghai Lu

author

2007-12-30 21:25 UTC #12 lore raw

PATCH 1/1 x86_64: fix section warning about enable_IO_APIC and setup_local_APIC

In reply to Ingo Molnar

[PATCH] x86_64: fix section warning about enable_IO_APIC and setup_local_APIC

clear IO_APIC before enabing apic error vector cause link warning

use function call in setup_local_APIC to avoid that.

Signed-off-by Yinghai Lu <[email protected]>

arch/x86/kernel/smpboot_64.c +7 -2

Index: linux-2.6/arch/x86/kernel/smpboot_64.c
===================================================================
@@ -211,7 +211,7 @@ void __cpuinit smp_callin(void)
 	 */
 
 	Dprintk("CALLIN, before setup_local_APIC().\n");
-	setup_local_APIC();
+	setup_local_APIC(NULL);
 
 	/*
 	 * Get our bogomips.

@@ -867,6 +867,8 @@ void __init smp_set_apicids(void)
  */
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
+	void (*extra_call)(void) = NULL;
+
 	nmi_watchdog_default();
 	current_cpu_data = boot_cpu_data;
 	current_thread_info()->cpu = 0;  /* needed? */

@@ -883,7 +885,10 @@ void __init smp_prepare_cpus(unsigned in
 	/*
 	 * Switch from PIC to APIC mode.
 	 */
-	setup_local_APIC();
+	if (!skip_ioapic_setup && nr_ioapics)
+		extra_call = enable_IO_APIC;
+
+	setup_local_APIC(extra_call);
 
 	if (GET_APIC_ID(apic_read(APIC_ID)) != boot_cpu_id) {
 		panic("Boot APIC ID in local APIC unexpected (%d vs %d)",

include/asm-x86/apic_64.h +1 -1

Index: linux-2.6/include/asm-x86/apic_64.h
===================================================================
@@ -74,7 +74,7 @@ extern int verify_local_APIC (void);
 extern void cache_APIC_registers (void);
 extern void sync_Arb_IDs (void);
 extern void init_bsp_APIC (void);
-extern void setup_local_APIC (void);
+extern void setup_local_APIC (void (*extra_call)(void));
 extern void init_apic_mappings (void);
 extern void smp_local_timer_interrupt (void);
 extern void setup_boot_APIC_clock (void);

arch/x86/kernel/apic_64.c +9 -5

Index: linux-2.6/arch/x86/kernel/apic_64.c
===================================================================
@@ -418,7 +418,7 @@ void __init init_bsp_APIC(void)
 	apic_write(APIC_LVT1, value);
 }
 
-void __cpuinit setup_local_APIC (void)
+void __cpuinit setup_local_APIC (void (*extra_call)(void))
 {
 	unsigned int value, maxlvt;
 	int i, j;

@@ -521,9 +521,8 @@ void __cpuinit setup_local_APIC (void)
 	 * Now enable IO-APICs, actually call clear_IO_APIC
 	 * We need clear_IO_APIC before enabling vector on BP
 	 */
-	if (!smp_processor_id())
-	if (!skip_ioapic_setup && nr_ioapics)
-		enable_IO_APIC();
+	if (extra_call)
+		(*extra_call)();
 
 	{
 		unsigned oldvalue;

@@ -1206,6 +1205,8 @@ int disable_apic;
  */
 int __init APIC_init_uniprocessor (void)
 {
+	void (*extra_call)(void) = NULL;
+
 	if (disable_apic) {
 		printk(KERN_INFO "Apic disabled\n");
 		return -1;

@@ -1221,7 +1222,10 @@ int __init APIC_init_uniprocessor (void)
 	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_id);
 	apic_write(APIC_ID, SET_APIC_ID(boot_cpu_id));
 
-	setup_local_APIC();
+	if (!skip_ioapic_setup && nr_ioapics)
+		extra_call = enable_IO_APIC;
+
+	setup_local_APIC(extra_call);
 
 	if (smp_found_config && !skip_ioapic_setup && nr_ioapics)
 		setup_IO_APIC();

Ingo Molnar

2007-12-30 21:30 UTC #13 lore raw

Re: [PATCH] x86_64: fix section warning about enable_IO_APIC and setup_local_APIC

Quoted 5 lines · Yinghai Lu

[PATCH] x86_64: fix section warning about enable_IO_APIC and setup_local_APIC
clear IO_APIC before enabing apic error vector cause link warning
use function call in setup_local_APIC to avoid that.

as i said, this just works around the link warning - now we've traded the link warning for a runtime crash.

Ingo

Yinghai Lu

2007-12-30 21:53 UTC #14 lore raw

Re: [PATCH] x86_64: fix section warning about enable_IO_APIC and setup_local_APIC

Quoted 11 lines · Ingo Molnar

Quoted 5 lines · Yinghai Lu
[PATCH] x86_64: fix section warning about enable_IO_APIC and setup_local_APIC
clear IO_APIC before enabing apic error vector cause link warning
use function call in setup_local_APIC to avoid that.
as i said, this just works around the link warning - now we've traded the link warning for a runtime crash.

you got one runtime crash by this fix?