Hi,

I made patches to unify crash_32/64.c.
There are three patches;
1. add lapic_shutdown for x86_64
2. add safe_smp_processor_id for x86_64
3. unify crash_32/64.c

I'm not sure that it's good to split to these patches.

I've compiled on both of 32bit and 64bit, and tested
kdump on 64bit.

Thanks
Hiroshi Shimamoto
-

[ message continues ]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/kernel/apic_64.c |   14 ++++++++++++++
 include/asm-x86/apic_64.h |    1 +
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index f47bc49..f28ccb5 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -287,6 +287,20 @@ void disable_local_APIC(void)
 	apic_write(APIC_SPIV, value);
 }
 
+void lapic_shutdown(void)
+{
+	unsigned long flags;
+
+	if (!cpu_has_apic)
+		return;
+
+	local_irq_save(flags);
+
+	disable_local_APIC();
+
+	local_irq_restore(flags);
+}
+
 /*
  * This is to verify that we're looking at a real local APIC.
  * Check these against your board if the CPUs aren't getting
diff --git a/include/asm-x86/apic_64.h b/include/asm-x86/apic_64.h
index 3c8f21e..2747a11 100644
--- a/include/asm-x86/apic_64.h
+++ b/include/asm-x86/apic_64.h
@@ -69,6 +69,7 @@ extern void clear_local_APIC (void);
 extern void connect_bsp_APIC (void);
 extern void disconnect_bsp_APIC (int virt_wire_setup);
 extern void disable_local_APIC (void);
+extern void lapic_shutdown (void);
 extern int verify_local_APIC (void);
 extern void cache_APIC_registers (void);
 extern void sync_Arb_IDs (void);
-- 
1.5.2.3
-

[ message continues ]

Do we really have to introduce this function for 64bit? I remember some
issues were faced on i386 w.r.t kernel enabling the LAPIC against the
wishes of BIOS hence kernel was disabling it while shutting down. No
such problems were reported for x86_64 hence this function existed only
for i386.

If that is the case, probably we don't have to introduce lapic_shutdown()
for x86_64. Instead call lapic_shutdown() for X86_32, and disble_local_APIC()
otherwise?

Thanks
Vivek
-

[ message continues ]

Thanks for the comment. I didn't know the issues, so I'd simply added

I will do that. I was thinking which is good when posting these patches.

Thanks
Hiroshi Shimamoto
-

[ message continues ]

I'm a little concerned here.  This sounds like forced unification.
If we can't clean up the infrastructure so things are obviously better
and cleanly factored for both architectures we should not unify the files.

As a general principle I would rather have two crudy files side by
side the one super crudy file.

So for unification I suggest finally fixing this right and taking the
apics completely out of the kexec on panic path.

Eric
-

[ message continues ]

Thanks for the suggestion.
But it's hard for me to imagine.
I'll try to consider about it.

Thanks
Hiroshi Shimamoto
-

[ message continues ]

lapic_shutdown is useless on x86_64.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/kernel/apic_64.c |   14 --------------
 arch/x86/kernel/crash.c   |    5 +++++
 include/asm-x86/apic_64.h |    1 -
 3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/apic_64.c b/arch/x86/kernel/apic_64.c
index f28ccb5..f47bc49 100644
--- a/arch/x86/kernel/apic_64.c
+++ b/arch/x86/kernel/apic_64.c
@@ -287,20 +287,6 @@ void disable_local_APIC(void)
 	apic_write(APIC_SPIV, value);
 }
 
-void lapic_shutdown(void)
-{
-	unsigned long flags;
-
-	if (!cpu_has_apic)
-		return;
-
-	local_irq_save(flags);
-
-	disable_local_APIC();
-
-	local_irq_restore(flags);
-}
-
 /*
  * This is to verify that we're looking at a real local APIC.
  * Check these against your board if the CPUs aren't getting
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 8bb482f..79a5a25 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -136,7 +136,12 @@ void machine_crash_shutdown(struct pt_regs *regs)
 	/* Make a note of crashing cpu. Will be used in NMI callback.*/
 	crashing_cpu = safe_smp_processor_id();
 	nmi_shootdown_cpus();
+#ifdef CONFIG_X86_32
 	lapic_shutdown();
+#else
+	if (cpu_has_apic)
+		disable_local_APIC();
+#endif
 #if defined(CONFIG_X86_IO_APIC)
 	disable_IO_APIC();
 #endif
diff --git a/include/asm-x86/apic_64.h b/include/asm-x86/apic_64.h
index 2747a11..3c8f21e 100644
--- a/include/asm-x86/apic_64.h
+++ b/include/asm-x86/apic_64.h
@@ -69,7 +69,6 @@ extern void clear_local_APIC (void);
 extern void connect_bsp_APIC (void);
 extern void disconnect_bsp_APIC (int virt_wire_setup);
 extern void disable_local_APIC (void);
-extern void lapic_shutdown (void);
 extern int verify_local_APIC (void);
 extern void cache_APIC_registers (void);
 extern void sync_Arb_IDs (void);
-- 
1.5.3.4

-

[ message continues ]

On Mon, 29 Oct 2007 15:39:46 -0700

.... but since the goal is to get apic_32.c and apic_64.c to be more
converging (to the point of becoming the same file)... isn't your patch
going in the opposite direction?

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
-

[ message continues ]

Hmm, I'm not sure that this revert affects x86 unification.
Vivek said that probably we don't have to introduce lapic_shutdown() for 64bit.
So I submitted this patch which reverts my previous post, it was applied before
the comment.

Thanks
Hiroshi Shimamoto
-

[ message continues ]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 include/asm-x86/smp_64.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/asm-x86/smp_64.h b/include/asm-x86/smp_64.h
index 6f0e027..ab612b0 100644
--- a/include/asm-x86/smp_64.h
+++ b/include/asm-x86/smp_64.h
@@ -76,6 +76,8 @@ extern unsigned __cpuinitdata disabled_cpus;
 
 #endif /* CONFIG_SMP */
 
+#define safe_smp_processor_id()		smp_processor_id()
+
 static inline int hard_smp_processor_id(void)
 {
 	/* we don't want to mark this access volatile - bad code generation */
-- 
1.5.2.3
-

[ message continues ]

Can you please implement a patch for safe_smp_processor_id() instead of
using smp_processor_id(). safe_smp_processor_id() was introduced to make
sure that we are not dependent on the stack of threads after kernel has
crashed instead read the apic id and convert it to cpu id with other
data structures. This helped in stack overflow case.

Hardcoding it to smp_processor_id() will give the false impression.

Thanks
Vivek
-

[ message continues ]

Just now Aneesh pointed that x86_64 using pda for retrieving processor id
and not kernel stack.

I think it is fine then.

Thanks
Vivek
-

[ message continues ]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Most of contents in crash are same.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/kernel/Makefile_32 |    2 +-
 arch/x86/kernel/Makefile_64 |    2 +-
 arch/x86/kernel/crash.c     |  144 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/crash_32.c  |  137 ----------------------------------------
 arch/x86/kernel/crash_64.c  |  135 ----------------------------------------
 5 files changed, 146 insertions(+), 274 deletions(-)
 create mode 100644 arch/x86/kernel/crash.c
 delete mode 100644 arch/x86/kernel/crash_32.c
 delete mode 100644 arch/x86/kernel/crash_64.c

diff --git a/arch/x86/kernel/Makefile_32 b/arch/x86/kernel/Makefile_32
index ccea590..b9d6798 100644
--- a/arch/x86/kernel/Makefile_32
+++ b/arch/x86/kernel/Makefile_32
@@ -26,7 +26,7 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse_32.o
 obj-$(CONFIG_X86_LOCAL_APIC)	+= apic_32.o nmi_32.o
 obj-$(CONFIG_X86_IO_APIC)	+= io_apic_32.o
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
-obj-$(CONFIG_KEXEC)		+= machine_kexec_32.o relocate_kernel_32.o crash_32.o
+obj-$(CONFIG_KEXEC)		+= machine_kexec_32.o relocate_kernel_32.o crash.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_32.o
 obj-$(CONFIG_X86_NUMAQ)		+= numaq_32.o
 obj-$(CONFIG_X86_SUMMIT_NUMA)	+= summit_32.o
diff --git a/arch/x86/kernel/Makefile_64 b/arch/x86/kernel/Makefile_64
index dec06e7..7b917d4 100644
--- a/arch/x86/kernel/Makefile_64
+++ b/arch/x86/kernel/Makefile_64
@@ -23,7 +23,7 @@ obj-$(CONFIG_X86_CPUID)		+= cpuid.o
 obj-$(CONFIG_SMP)		+= smp_64.o smpboot_64.o trampoline_64.o tsc_sync.o
 obj-y				+= apic_64.o  nmi_64.o
 obj-y				+= io_apic_64.o mpparse_64.o genapic_64.o genapic_flat_64.o
-obj-$(CONFIG_KEXEC)		+= machine_kexec_64.o relocate_kernel_64.o crash_64.o
+obj-$(CONFIG_KEXEC)		+= machine_kexec_64.o relocate_kernel_64.o crash.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_64.o
 obj-$(CONFIG_PM)		+= suspend_64.o
 obj-$(CONFIG_HIBERNATION)	+= suspend_asm_64.o
diff --git ...
[ message continues ]

It's fine. So the steps leading to unification are clear.

Applied. Thanks,

	 tglx


	
-

[ message continues ]

Hi Hiroshi,

Thanks for the patches. Can you please also test it on 32bit to make
sure nothing is broken.

Thanks
Vivek
-

[ message continues ]

Okay, I'll test it on 32bit.
A build problem already has been found on 32bit.

Thanks,
Hiroshi
-

[ message continues ]

I found that the following patch makes my machine stopped.
bfe0c1cc6456bba1f4e3cc1fe29c0ea578ac763a
x86: HPET force enable for ICH5

It means that after applied this patch, HPET is enabled
automatically on 1st kernel and after crash/kexec the 2nd
kernel stopped at checking 'hlt'.

I also tested the latest kernel(2.6.24-rc1-gec3b67c1).
Boot parameter "nohpet" resolves this issue and kdump
works well on 32bit.
So I guess HPET affects this.
But I don't know why 64bit kernel with HPET is OK.

Thanks
Hiroshi Shimamoto
-

[ message continues ]

On Fri, 26 Oct 2007, Hiroshi Shimamoto wrote:


Hmm. Does the 64 bit code shutdown HPET and restore the IRQ routing to
PIT and 32 bit is missing this ?

    tglx
-

[ message continues ]

Sorry, I'm not sure how I can get these informations.
Can you please tell me what I should do?
I'll continue to dig the issue.

I also have the following message.
..MP-BIOS bug: 8254 timer not connected to IO-APIC
Kernel panic - not syncing: IO-APIC + timer doesn't work! Try using the 'noapic' kernel parameter

It appeared only on 64bit and the 2nd kernel without
boot parameter noapic.


Thanks
Hiroshi Shimamoto
-

[ message continues ]

I attached the .config files and console logs.
config32/64 are for 1st kernel, and cap32/64 are for 2nd capture kernel.
kdump1.log is boot with nohpet on 32bit.
kdump2.log is boot without nohpet on 32bit and the 2nd kernel hangs.
kdump3.log is on 64bit. And the first kdump is failed because of
without noapic.

Thanks
Hiroshi Shimamoto
[ message continues ]