[PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Wei Jiangang]

v2:
Just about the commit ("x86/apic: Improved the setting of interrupt
 mode for bsp")

- Unify the name
  s/virtual_wire_via_*/virt_wire_through_*
- Add check for PIC mode
  suggested-by Baoquan He <bhe@redhat.com>
- Add check enable/disable flag for IO-APIC
  suggested-by Xunlei Pang <xpang@redhat.com>
- Update comments

v1:
The goal is to fix dump-capture kernel with notsc option hangs
in calibrate_delay_converge()

Wei Jiangang (3):
  x86/apic: Remove "focus disabled" for 64bit case
  x86/apic: Update comment about disabling processor focus
  x86/apic: Improved the setting of interrupt mode for bsp

 arch/x86/include/asm/io_apic.h |  5 ++++
 arch/x86/kernel/apic/apic.c    | 63 +++++++++++++++++++++++++++++++++++++++---
 arch/x86/kernel/apic/io_apic.c | 28 +++++++++++++++++++
 3 files changed, 92 insertions(+), 4 deletions(-)

-- 
1.9.3

[PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Wei Jiangang]

v2:
Just about the commit ("x86/apic: Improved the setting of interrupt
 mode for bsp")

- Unify the name
  s/virtual_wire_via_*/virt_wire_through_*
- Add check for PIC mode
  suggested-by Baoquan He <bhe@redhat.com>
- Add check enable/disable flag for IO-APIC
  suggested-by Xunlei Pang <xpang@redhat.com>
- Update comments

v1:
The goal is to fix dump-capture kernel with notsc option hangs
in calibrate_delay_converge()

Wei Jiangang (3):
  x86/apic: Remove "focus disabled" for 64bit case
  x86/apic: Update comment about disabling processor focus
  x86/apic: Improved the setting of interrupt mode for bsp

 arch/x86/include/asm/io_apic.h |  5 ++++
 arch/x86/kernel/apic/apic.c    | 63 +++++++++++++++++++++++++++++++++++++++---
 arch/x86/kernel/apic/io_apic.c | 28 +++++++++++++++++++
 3 files changed, 92 insertions(+), 4 deletions(-)

-- 
1.9.3




_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 1/3] x86/apic: Remove "focus disabled" for 64bit case

[Wei Jiangang]

Disable processor focus for 64bit causes a crash,
Call Trace as following:

  [<ffffffff8133499f>] dump_stack+0x63/0x84
  [<ffffffff810800f1>] __warn+0xd1/0xf0
  [<ffffffff8108016f>] warn_slowpath_fmt+0x5f/0x80
  [<ffffffff81068ab2>] ex_handler_wrmsr_unsafe+0x62/0x70
  [<ffffffff81068b29>] fixup_exception+0x39/0x50
  [<ffffffff8102e030>] do_general_protection+0x80/0x160
  [<ffffffff816a89a8>] general_protection+0x28/0x30
  [<ffffffff81062554>] ? native_write_msr+0x4/0x30
  [<ffffffff81059c52>] ? native_apic_msr_write+0x32/0x40
  [<ffffffff81d92964>] init_bsp_APIC+0x5f/0x118
  [<ffffffff81d87489>] init_ISA_irqs+0x19/0x4c
  [<ffffffff81d87512>] native_init_IRQ+0xd/0x377
  [<ffffffff81d874fe>] init_IRQ+0x42/0x49
  [<ffffffff81d7afc6>] start_kernel+0x2ce/0x4c8
  [<ffffffff81d7aae6>] ? set_init_arg+0x55/0x55
  [<ffffffff81d7a120>] ? early_idt_handler_array+0x120/0x120
  [<ffffffff81d7a5db>] x86_64_start_reservations+0x2f/0x31
  [<ffffffff81d7a729>] x86_64_start_kernel+0x14c/0x16f

Keep a consistent implementation with the setup_local_APIC(),
always use processor focus for 64bit.
more details refer to commit 89c38c2867eb ("x86: apic - unify
setup_local_APIC")

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/apic/apic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 60078a67d7e3..0273b652c689 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1154,9 +1154,7 @@ void __init init_bsp_APIC(void)
 	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
 	    (boot_cpu_data.x86 == 15))
 		value &= ~APIC_SPIV_FOCUS_DISABLED;
-	else
 #endif
-		value |= APIC_SPIV_FOCUS_DISABLED;
 	value |= SPURIOUS_APIC_VECTOR;
 	apic_write(APIC_SPIV, value);
 
-- 
1.9.3

[PATCH v2 1/3] x86/apic: Remove "focus disabled" for 64bit case

[Wei Jiangang]

Disable processor focus for 64bit causes a crash,
Call Trace as following:

  [<ffffffff8133499f>] dump_stack+0x63/0x84
  [<ffffffff810800f1>] __warn+0xd1/0xf0
  [<ffffffff8108016f>] warn_slowpath_fmt+0x5f/0x80
  [<ffffffff81068ab2>] ex_handler_wrmsr_unsafe+0x62/0x70
  [<ffffffff81068b29>] fixup_exception+0x39/0x50
  [<ffffffff8102e030>] do_general_protection+0x80/0x160
  [<ffffffff816a89a8>] general_protection+0x28/0x30
  [<ffffffff81062554>] ? native_write_msr+0x4/0x30
  [<ffffffff81059c52>] ? native_apic_msr_write+0x32/0x40
  [<ffffffff81d92964>] init_bsp_APIC+0x5f/0x118
  [<ffffffff81d87489>] init_ISA_irqs+0x19/0x4c
  [<ffffffff81d87512>] native_init_IRQ+0xd/0x377
  [<ffffffff81d874fe>] init_IRQ+0x42/0x49
  [<ffffffff81d7afc6>] start_kernel+0x2ce/0x4c8
  [<ffffffff81d7aae6>] ? set_init_arg+0x55/0x55
  [<ffffffff81d7a120>] ? early_idt_handler_array+0x120/0x120
  [<ffffffff81d7a5db>] x86_64_start_reservations+0x2f/0x31
  [<ffffffff81d7a729>] x86_64_start_kernel+0x14c/0x16f

Keep a consistent implementation with the setup_local_APIC(),
always use processor focus for 64bit.
more details refer to commit 89c38c2867eb ("x86: apic - unify
setup_local_APIC")

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/apic/apic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 60078a67d7e3..0273b652c689 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1154,9 +1154,7 @@ void __init init_bsp_APIC(void)
 	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
 	    (boot_cpu_data.x86 == 15))
 		value &= ~APIC_SPIV_FOCUS_DISABLED;
-	else
 #endif
-		value |= APIC_SPIV_FOCUS_DISABLED;
 	value |= SPURIOUS_APIC_VECTOR;
 	apic_write(APIC_SPIV, value);
 
-- 
1.9.3




_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 2/3] x86/apic: Update comment about disabling processor focus

[Wei Jiangang]

Fix references to discarded end_level_ioapic_irq().

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/apic/apic.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 0273b652c689..8e25b9b2d351 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1346,7 +1346,6 @@ void setup_local_APIC(void)
 	 * Actually disabling the focus CPU check just makes the hang less
 	 * frequent as it makes the interrupt distributon model be more
 	 * like LRU than MRU (the short-term load is more even across CPUs).
-	 * See also the comment in end_level_ioapic_irq().  --macro
 	 */
 
 	/*
-- 
1.9.3

[PATCH v2 2/3] x86/apic: Update comment about disabling processor focus

[Wei Jiangang]

Fix references to discarded end_level_ioapic_irq().

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/apic/apic.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 0273b652c689..8e25b9b2d351 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1346,7 +1346,6 @@ void setup_local_APIC(void)
 	 * Actually disabling the focus CPU check just makes the hang less
 	 * frequent as it makes the interrupt distributon model be more
 	 * like LRU than MRU (the short-term load is more even across CPUs).
-	 * See also the comment in end_level_ioapic_irq().  --macro
 	 */
 
 	/*
-- 
1.9.3




_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Wei Jiangang]

If we specify the 'notsc' parameter for the dump-capture kernel,
and then trigger a crash(panic) by using "ALT-SysRq-c" or
"echo c > /proc/sysrq-trigger", the dump-capture kernel will
hang in calibrate_delay_converge() and wait for jiffies changes.
serial log as follows:

    tsc: Fast TSC calibration using PIT
    tsc: Detected 2099.947 MHz processor
    Calibrating delay loop...

The reason for jiffies not changes is there's no timer interrupt
passed to dump-capture kernel.

In fact, once kernel panic occurs, the local APIC is disabled
by lapic_shutdown() in reboot path.
generly speaking, local APIC state can be initialized by BIOS
after Power-Up or Reset, which doesn't apply to kdump case.
so the kernel has to be responsible for initialize the interrupt
mode properly according the latest status of APIC in bootup path.

An MP operating system is booted under either PIC mode or
virtual wire mode. Later, the operating system switches to
symmetric I/O mode as it enters multiprocessor mode.
Two kinds of virtual wire mode are defined in Intel MP spec:
virtual wire mode via local APIC or via I/O APIC.

Now we determine the mode of APIC only through a SMP BIOS(MP table).
That's not enough. It's better to do further check if APIC works
with effective interrupt mode, and then, do some proper setting.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
---
 arch/x86/include/asm/io_apic.h |  5 ++++
 arch/x86/kernel/apic/apic.c    | 60 +++++++++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/apic/io_apic.c | 28 ++++++++++++++++++++
 3 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 6cbf2cfb3f8a..a3257366bf7f 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -190,6 +190,7 @@ static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
 }
 
 extern void setup_IO_APIC(void);
+extern bool virt_wire_through_ioapic(void);
 extern void enable_IO_APIC(void);
 extern void disable_IO_APIC(void);
 extern void setup_ioapic_dest(void);
@@ -231,6 +232,10 @@ static inline void io_apic_init_mappings(void) { }
 #define native_disable_io_apic		NULL
 
 static inline void setup_IO_APIC(void) { }
+static inline bool virt_wire_through_ioapic(void)
+{
+	return false;
+}
 static inline void enable_IO_APIC(void) { }
 static inline void setup_ioapic_dest(void) { }
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 8e25b9b2d351..a3939fb130cc 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1124,6 +1124,58 @@ void __init sync_Arb_IDs(void)
 }
 
 /*
+ * Check APIC enable/disable flag
+ */
+static bool check_apic_enabled(void)
+{
+	unsigned int value;
+
+	/*
+	 * If APIC is disabled globally (IA32_APIC_BASE[11] == 0)
+	 * the boot cpu hasn't X86_FEATURE_APIC,
+	 * and init_bsp_APIC() has already checked it before.
+	 * so no need to check global enable/disable flag here
+	 */
+
+	/* Check the software enable/disable flag */
+	value = apic_read(APIC_SPIV);
+	if (!(value & APIC_SPIV_APIC_ENABLED))
+		return false;
+
+	return true;
+}
+
+/*
+ * Return false means the through-local-APIC virtual wire mode is inactive
+ */
+static bool virt_wire_through_lapic(void)
+{
+	unsigned int value;
+
+	/*
+	 * The through-local-APIC virtual wire mode requests
+	 * local APIC to enable LINT0 for ExtINT delivery mode
+	 * and LINT1 for NMI delivery mode
+	 */
+	value = apic_read(APIC_LVT0);
+	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_EXTINT)
+		return false;
+
+	value = apic_read(APIC_LVT1);
+	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_NMI)
+		return false;
+
+	return true;
+}
+
+static bool check_virt_wire_mode(void)
+{
+	/* If neither of virtual wire mode is active, return false */
+	return (check_apic_enabled() && (virt_wire_through_lapic() ||
+		virt_wire_through_ioapic()));
+}
+
+/*
  * An initial setup of the virtual wire mode.
  */
 void __init init_bsp_APIC(void)
@@ -1133,8 +1185,14 @@ void __init init_bsp_APIC(void)
 	/*
 	 * Don't do the setup now if we have a SMP BIOS as the
 	 * through-I/O-APIC virtual wire mode might be active.
+	 *
+	 * It's better to do further check if either through-I/O-APIC
+	 * or through-local-APIC is active.
+	 * the worst case is that both of them are inactive, If so,
+	 * we need to enable the through-local-APIC virtual wire mode
 	 */
-	if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC))
+	if (pic_mode || !boot_cpu_has(X86_FEATURE_APIC) ||
+	    (smp_found_config && check_virt_wire_mode()))
 		return;
 
 	/*
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 446702ed99dc..f794d389ba85 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1379,6 +1379,34 @@ void __init print_IO_APICs(void)
 /* Where if anywhere is the i8259 connect in external int mode */
 static struct { int pin, apic; } ioapic_i8259 = { -1, -1 };
 
+/*
+ * Return false means the through-I/O-APIC virtual wire mode is inactive
+ */
+bool virt_wire_through_ioapic(void)
+{
+	int apic, pin;
+
+	for_each_ioapic_pin(apic, pin) {
+		/* See if any of the pins is in ExtINT mode */
+		struct IO_APIC_route_entry entry = ioapic_read_entry(apic, pin);
+
+		/*
+		 * If the interrupt line is enabled and in ExtInt mode
+		 * I have found the pin where the i8259 is connected.
+		 */
+		if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT))
+			return true;
+	}
+
+	/*
+	 * The through-I/O-APIC virtual wire mode requests
+	 * I/O APIC to be connected i8259 in chapter 3.6.2.2 of the MP v1.4 spec
+	 * If no pin in ExtInt mode, the through-I/O-APIC virtual wire mode can
+	 * be regarded as inactive
+	 */
+	return false;
+}
+
 void __init enable_IO_APIC(void)
 {
 	int i8259_apic, i8259_pin;
-- 
1.9.3

[PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Wei Jiangang]

If we specify the 'notsc' parameter for the dump-capture kernel,
and then trigger a crash(panic) by using "ALT-SysRq-c" or
"echo c > /proc/sysrq-trigger", the dump-capture kernel will
hang in calibrate_delay_converge() and wait for jiffies changes.
serial log as follows:

    tsc: Fast TSC calibration using PIT
    tsc: Detected 2099.947 MHz processor
    Calibrating delay loop...

The reason for jiffies not changes is there's no timer interrupt
passed to dump-capture kernel.

In fact, once kernel panic occurs, the local APIC is disabled
by lapic_shutdown() in reboot path.
generly speaking, local APIC state can be initialized by BIOS
after Power-Up or Reset, which doesn't apply to kdump case.
so the kernel has to be responsible for initialize the interrupt
mode properly according the latest status of APIC in bootup path.

An MP operating system is booted under either PIC mode or
virtual wire mode. Later, the operating system switches to
symmetric I/O mode as it enters multiprocessor mode.
Two kinds of virtual wire mode are defined in Intel MP spec:
virtual wire mode via local APIC or via I/O APIC.

Now we determine the mode of APIC only through a SMP BIOS(MP table).
That's not enough. It's better to do further check if APIC works
with effective interrupt mode, and then, do some proper setting.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
---
 arch/x86/include/asm/io_apic.h |  5 ++++
 arch/x86/kernel/apic/apic.c    | 60 +++++++++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/apic/io_apic.c | 28 ++++++++++++++++++++
 3 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 6cbf2cfb3f8a..a3257366bf7f 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -190,6 +190,7 @@ static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
 }
 
 extern void setup_IO_APIC(void);
+extern bool virt_wire_through_ioapic(void);
 extern void enable_IO_APIC(void);
 extern void disable_IO_APIC(void);
 extern void setup_ioapic_dest(void);
@@ -231,6 +232,10 @@ static inline void io_apic_init_mappings(void) { }
 #define native_disable_io_apic		NULL
 
 static inline void setup_IO_APIC(void) { }
+static inline bool virt_wire_through_ioapic(void)
+{
+	return false;
+}
 static inline void enable_IO_APIC(void) { }
 static inline void setup_ioapic_dest(void) { }
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 8e25b9b2d351..a3939fb130cc 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1124,6 +1124,58 @@ void __init sync_Arb_IDs(void)
 }
 
 /*
+ * Check APIC enable/disable flag
+ */
+static bool check_apic_enabled(void)
+{
+	unsigned int value;
+
+	/*
+	 * If APIC is disabled globally (IA32_APIC_BASE[11] == 0)
+	 * the boot cpu hasn't X86_FEATURE_APIC,
+	 * and init_bsp_APIC() has already checked it before.
+	 * so no need to check global enable/disable flag here
+	 */
+
+	/* Check the software enable/disable flag */
+	value = apic_read(APIC_SPIV);
+	if (!(value & APIC_SPIV_APIC_ENABLED))
+		return false;
+
+	return true;
+}
+
+/*
+ * Return false means the through-local-APIC virtual wire mode is inactive
+ */
+static bool virt_wire_through_lapic(void)
+{
+	unsigned int value;
+
+	/*
+	 * The through-local-APIC virtual wire mode requests
+	 * local APIC to enable LINT0 for ExtINT delivery mode
+	 * and LINT1 for NMI delivery mode
+	 */
+	value = apic_read(APIC_LVT0);
+	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_EXTINT)
+		return false;
+
+	value = apic_read(APIC_LVT1);
+	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_NMI)
+		return false;
+
+	return true;
+}
+
+static bool check_virt_wire_mode(void)
+{
+	/* If neither of virtual wire mode is active, return false */
+	return (check_apic_enabled() && (virt_wire_through_lapic() ||
+		virt_wire_through_ioapic()));
+}
+
+/*
  * An initial setup of the virtual wire mode.
  */
 void __init init_bsp_APIC(void)
@@ -1133,8 +1185,14 @@ void __init init_bsp_APIC(void)
 	/*
 	 * Don't do the setup now if we have a SMP BIOS as the
 	 * through-I/O-APIC virtual wire mode might be active.
+	 *
+	 * It's better to do further check if either through-I/O-APIC
+	 * or through-local-APIC is active.
+	 * the worst case is that both of them are inactive, If so,
+	 * we need to enable the through-local-APIC virtual wire mode
 	 */
-	if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC))
+	if (pic_mode || !boot_cpu_has(X86_FEATURE_APIC) ||
+	    (smp_found_config && check_virt_wire_mode()))
 		return;
 
 	/*
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 446702ed99dc..f794d389ba85 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1379,6 +1379,34 @@ void __init print_IO_APICs(void)
 /* Where if anywhere is the i8259 connect in external int mode */
 static struct { int pin, apic; } ioapic_i8259 = { -1, -1 };
 
+/*
+ * Return false means the through-I/O-APIC virtual wire mode is inactive
+ */
+bool virt_wire_through_ioapic(void)
+{
+	int apic, pin;
+
+	for_each_ioapic_pin(apic, pin) {
+		/* See if any of the pins is in ExtINT mode */
+		struct IO_APIC_route_entry entry = ioapic_read_entry(apic, pin);
+
+		/*
+		 * If the interrupt line is enabled and in ExtInt mode
+		 * I have found the pin where the i8259 is connected.
+		 */
+		if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT))
+			return true;
+	}
+
+	/*
+	 * The through-I/O-APIC virtual wire mode requests
+	 * I/O APIC to be connected i8259 in chapter 3.6.2.2 of the MP v1.4 spec
+	 * If no pin in ExtInt mode, the through-I/O-APIC virtual wire mode can
+	 * be regarded as inactive
+	 */
+	return false;
+}
+
 void __init enable_IO_APIC(void)
 {
 	int i8259_apic, i8259_pin;
-- 
1.9.3




_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Eric W. Biederman]

Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:

> If we specify the 'notsc' parameter for the dump-capture kernel,
> and then trigger a crash(panic) by using "ALT-SysRq-c" or
> "echo c > /proc/sysrq-trigger", the dump-capture kernel will
> hang in calibrate_delay_converge() and wait for jiffies changes.
> serial log as follows:
>
>     tsc: Fast TSC calibration using PIT
>     tsc: Detected 2099.947 MHz processor
>     Calibrating delay loop...
>
> The reason for jiffies not changes is there's no timer interrupt
> passed to dump-capture kernel.
>
> In fact, once kernel panic occurs, the local APIC is disabled
> by lapic_shutdown() in reboot path.
> generly speaking, local APIC state can be initialized by BIOS
> after Power-Up or Reset, which doesn't apply to kdump case.
> so the kernel has to be responsible for initialize the interrupt
> mode properly according the latest status of APIC in bootup path.
>
> An MP operating system is booted under either PIC mode or
> virtual wire mode. Later, the operating system switches to
> symmetric I/O mode as it enters multiprocessor mode.
> Two kinds of virtual wire mode are defined in Intel MP spec:
> virtual wire mode via local APIC or via I/O APIC.
>
> Now we determine the mode of APIC only through a SMP BIOS(MP table).
> That's not enough. It's better to do further check if APIC works
> with effective interrupt mode, and then, do some proper setting.

Reading through the code let me pause a moment and say:
"Yowzers the interrupt initialization code has gotten hard to follow.  It
is now full of indirection with ill defined semantics."  pre_vector_init
indeed.

I will argue this is the wrong fix.

We really should not have to worry about getting the system functional
in virtual wire mode on a modern system.  And looking at the code
someone has done half the work and made it conditional under
acpi_gbl_reduced_hardware.

Now reduced hardware implies a bit more than we ware talking about but
if there is ACPI apic information we should not need to worry about
external interrupts and can just enable the apics.

In fact I think having MPtable information is enough for that.

So I think what needs to happens is for the apic initialization to get
an overhaul that makes apic initialization the happy path and the other
irq controllers the odd backwards compatibility path.  And when we
are done we never run in anything except full apic mode unless the
hardware doesn't support it.

I think that will leave things more robust as we don't need to setup
and then reset up the interrupts during boot.

Eric


> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
> ---
>  arch/x86/include/asm/io_apic.h |  5 ++++
>  arch/x86/kernel/apic/apic.c    | 60 +++++++++++++++++++++++++++++++++++++++++-
>  arch/x86/kernel/apic/io_apic.c | 28 ++++++++++++++++++++
>  3 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index 6cbf2cfb3f8a..a3257366bf7f 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -190,6 +190,7 @@ static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
>  }
>  
>  extern void setup_IO_APIC(void);
> +extern bool virt_wire_through_ioapic(void);
>  extern void enable_IO_APIC(void);
>  extern void disable_IO_APIC(void);
>  extern void setup_ioapic_dest(void);
> @@ -231,6 +232,10 @@ static inline void io_apic_init_mappings(void) { }
>  #define native_disable_io_apic		NULL
>  
>  static inline void setup_IO_APIC(void) { }
> +static inline bool virt_wire_through_ioapic(void)
> +{
> +	return false;
> +}
>  static inline void enable_IO_APIC(void) { }
>  static inline void setup_ioapic_dest(void) { }
>  
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 8e25b9b2d351..a3939fb130cc 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1124,6 +1124,58 @@ void __init sync_Arb_IDs(void)
>  }
>  
>  /*
> + * Check APIC enable/disable flag
> + */
> +static bool check_apic_enabled(void)
> +{
> +	unsigned int value;
> +
> +	/*
> +	 * If APIC is disabled globally (IA32_APIC_BASE[11] == 0)
> +	 * the boot cpu hasn't X86_FEATURE_APIC,
> +	 * and init_bsp_APIC() has already checked it before.
> +	 * so no need to check global enable/disable flag here
> +	 */
> +
> +	/* Check the software enable/disable flag */
> +	value = apic_read(APIC_SPIV);
> +	if (!(value & APIC_SPIV_APIC_ENABLED))
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * Return false means the through-local-APIC virtual wire mode is inactive
> + */
> +static bool virt_wire_through_lapic(void)
> +{
> +	unsigned int value;
> +
> +	/*
> +	 * The through-local-APIC virtual wire mode requests
> +	 * local APIC to enable LINT0 for ExtINT delivery mode
> +	 * and LINT1 for NMI delivery mode
> +	 */
> +	value = apic_read(APIC_LVT0);
> +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_EXTINT)
> +		return false;
> +
> +	value = apic_read(APIC_LVT1);
> +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_NMI)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool check_virt_wire_mode(void)
> +{
> +	/* If neither of virtual wire mode is active, return false */
> +	return (check_apic_enabled() && (virt_wire_through_lapic() ||
> +		virt_wire_through_ioapic()));
> +}
> +
> +/*
>   * An initial setup of the virtual wire mode.
>   */
>  void __init init_bsp_APIC(void)
> @@ -1133,8 +1185,14 @@ void __init init_bsp_APIC(void)
>  	/*
>  	 * Don't do the setup now if we have a SMP BIOS as the
>  	 * through-I/O-APIC virtual wire mode might be active.
> +	 *
> +	 * It's better to do further check if either through-I/O-APIC
> +	 * or through-local-APIC is active.
> +	 * the worst case is that both of them are inactive, If so,
> +	 * we need to enable the through-local-APIC virtual wire mode
>  	 */
> -	if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC))
> +	if (pic_mode || !boot_cpu_has(X86_FEATURE_APIC) ||
> +	    (smp_found_config && check_virt_wire_mode()))
>  		return;
>  
>  	/*
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 446702ed99dc..f794d389ba85 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1379,6 +1379,34 @@ void __init print_IO_APICs(void)
>  /* Where if anywhere is the i8259 connect in external int mode */
>  static struct { int pin, apic; } ioapic_i8259 = { -1, -1 };
>  
> +/*
> + * Return false means the through-I/O-APIC virtual wire mode is inactive
> + */
> +bool virt_wire_through_ioapic(void)
> +{
> +	int apic, pin;
> +
> +	for_each_ioapic_pin(apic, pin) {
> +		/* See if any of the pins is in ExtINT mode */
> +		struct IO_APIC_route_entry entry = ioapic_read_entry(apic, pin);
> +
> +		/*
> +		 * If the interrupt line is enabled and in ExtInt mode
> +		 * I have found the pin where the i8259 is connected.
> +		 */
> +		if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT))
> +			return true;
> +	}
> +
> +	/*
> +	 * The through-I/O-APIC virtual wire mode requests
> +	 * I/O APIC to be connected i8259 in chapter 3.6.2.2 of the MP v1.4 spec
> +	 * If no pin in ExtInt mode, the through-I/O-APIC virtual wire mode can
> +	 * be regarded as inactive
> +	 */
> +	return false;
> +}
> +
>  void __init enable_IO_APIC(void)
>  {
>  	int i8259_apic, i8259_pin;

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Eric W. Biederman]

Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:

> If we specify the 'notsc' parameter for the dump-capture kernel,
> and then trigger a crash(panic) by using "ALT-SysRq-c" or
> "echo c > /proc/sysrq-trigger", the dump-capture kernel will
> hang in calibrate_delay_converge() and wait for jiffies changes.
> serial log as follows:
>
>     tsc: Fast TSC calibration using PIT
>     tsc: Detected 2099.947 MHz processor
>     Calibrating delay loop...
>
> The reason for jiffies not changes is there's no timer interrupt
> passed to dump-capture kernel.
>
> In fact, once kernel panic occurs, the local APIC is disabled
> by lapic_shutdown() in reboot path.
> generly speaking, local APIC state can be initialized by BIOS
> after Power-Up or Reset, which doesn't apply to kdump case.
> so the kernel has to be responsible for initialize the interrupt
> mode properly according the latest status of APIC in bootup path.
>
> An MP operating system is booted under either PIC mode or
> virtual wire mode. Later, the operating system switches to
> symmetric I/O mode as it enters multiprocessor mode.
> Two kinds of virtual wire mode are defined in Intel MP spec:
> virtual wire mode via local APIC or via I/O APIC.
>
> Now we determine the mode of APIC only through a SMP BIOS(MP table).
> That's not enough. It's better to do further check if APIC works
> with effective interrupt mode, and then, do some proper setting.

Reading through the code let me pause a moment and say:
"Yowzers the interrupt initialization code has gotten hard to follow.  It
is now full of indirection with ill defined semantics."  pre_vector_init
indeed.

I will argue this is the wrong fix.

We really should not have to worry about getting the system functional
in virtual wire mode on a modern system.  And looking at the code
someone has done half the work and made it conditional under
acpi_gbl_reduced_hardware.

Now reduced hardware implies a bit more than we ware talking about but
if there is ACPI apic information we should not need to worry about
external interrupts and can just enable the apics.

In fact I think having MPtable information is enough for that.

So I think what needs to happens is for the apic initialization to get
an overhaul that makes apic initialization the happy path and the other
irq controllers the odd backwards compatibility path.  And when we
are done we never run in anything except full apic mode unless the
hardware doesn't support it.

I think that will leave things more robust as we don't need to setup
and then reset up the interrupts during boot.

Eric


> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
> ---
>  arch/x86/include/asm/io_apic.h |  5 ++++
>  arch/x86/kernel/apic/apic.c    | 60 +++++++++++++++++++++++++++++++++++++++++-
>  arch/x86/kernel/apic/io_apic.c | 28 ++++++++++++++++++++
>  3 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index 6cbf2cfb3f8a..a3257366bf7f 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -190,6 +190,7 @@ static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
>  }
>  
>  extern void setup_IO_APIC(void);
> +extern bool virt_wire_through_ioapic(void);
>  extern void enable_IO_APIC(void);
>  extern void disable_IO_APIC(void);
>  extern void setup_ioapic_dest(void);
> @@ -231,6 +232,10 @@ static inline void io_apic_init_mappings(void) { }
>  #define native_disable_io_apic		NULL
>  
>  static inline void setup_IO_APIC(void) { }
> +static inline bool virt_wire_through_ioapic(void)
> +{
> +	return false;
> +}
>  static inline void enable_IO_APIC(void) { }
>  static inline void setup_ioapic_dest(void) { }
>  
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 8e25b9b2d351..a3939fb130cc 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1124,6 +1124,58 @@ void __init sync_Arb_IDs(void)
>  }
>  
>  /*
> + * Check APIC enable/disable flag
> + */
> +static bool check_apic_enabled(void)
> +{
> +	unsigned int value;
> +
> +	/*
> +	 * If APIC is disabled globally (IA32_APIC_BASE[11] == 0)
> +	 * the boot cpu hasn't X86_FEATURE_APIC,
> +	 * and init_bsp_APIC() has already checked it before.
> +	 * so no need to check global enable/disable flag here
> +	 */
> +
> +	/* Check the software enable/disable flag */
> +	value = apic_read(APIC_SPIV);
> +	if (!(value & APIC_SPIV_APIC_ENABLED))
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * Return false means the through-local-APIC virtual wire mode is inactive
> + */
> +static bool virt_wire_through_lapic(void)
> +{
> +	unsigned int value;
> +
> +	/*
> +	 * The through-local-APIC virtual wire mode requests
> +	 * local APIC to enable LINT0 for ExtINT delivery mode
> +	 * and LINT1 for NMI delivery mode
> +	 */
> +	value = apic_read(APIC_LVT0);
> +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_EXTINT)
> +		return false;
> +
> +	value = apic_read(APIC_LVT1);
> +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_NMI)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool check_virt_wire_mode(void)
> +{
> +	/* If neither of virtual wire mode is active, return false */
> +	return (check_apic_enabled() && (virt_wire_through_lapic() ||
> +		virt_wire_through_ioapic()));
> +}
> +
> +/*
>   * An initial setup of the virtual wire mode.
>   */
>  void __init init_bsp_APIC(void)
> @@ -1133,8 +1185,14 @@ void __init init_bsp_APIC(void)
>  	/*
>  	 * Don't do the setup now if we have a SMP BIOS as the
>  	 * through-I/O-APIC virtual wire mode might be active.
> +	 *
> +	 * It's better to do further check if either through-I/O-APIC
> +	 * or through-local-APIC is active.
> +	 * the worst case is that both of them are inactive, If so,
> +	 * we need to enable the through-local-APIC virtual wire mode
>  	 */
> -	if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC))
> +	if (pic_mode || !boot_cpu_has(X86_FEATURE_APIC) ||
> +	    (smp_found_config && check_virt_wire_mode()))
>  		return;
>  
>  	/*
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 446702ed99dc..f794d389ba85 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1379,6 +1379,34 @@ void __init print_IO_APICs(void)
>  /* Where if anywhere is the i8259 connect in external int mode */
>  static struct { int pin, apic; } ioapic_i8259 = { -1, -1 };
>  
> +/*
> + * Return false means the through-I/O-APIC virtual wire mode is inactive
> + */
> +bool virt_wire_through_ioapic(void)
> +{
> +	int apic, pin;
> +
> +	for_each_ioapic_pin(apic, pin) {
> +		/* See if any of the pins is in ExtINT mode */
> +		struct IO_APIC_route_entry entry = ioapic_read_entry(apic, pin);
> +
> +		/*
> +		 * If the interrupt line is enabled and in ExtInt mode
> +		 * I have found the pin where the i8259 is connected.
> +		 */
> +		if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT))
> +			return true;
> +	}
> +
> +	/*
> +	 * The through-I/O-APIC virtual wire mode requests
> +	 * I/O APIC to be connected i8259 in chapter 3.6.2.2 of the MP v1.4 spec
> +	 * If no pin in ExtInt mode, the through-I/O-APIC virtual wire mode can
> +	 * be regarded as inactive
> +	 */
> +	return false;
> +}
> +
>  void __init enable_IO_APIC(void)
>  {
>  	int i8259_apic, i8259_pin;

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Wei, Jiangang]

Hi Eric,

Thanks for your response.
But I have some different ideas...

On Mon, 2016-07-25 at 22:53 -0500, Eric W. Biederman wrote:
> Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:
> 
> > If we specify the 'notsc' parameter for the dump-capture kernel,
> > and then trigger a crash(panic) by using "ALT-SysRq-c" or
> > "echo c > /proc/sysrq-trigger", the dump-capture kernel will
> > hang in calibrate_delay_converge() and wait for jiffies changes.
> > serial log as follows:
> >
> >     tsc: Fast TSC calibration using PIT
> >     tsc: Detected 2099.947 MHz processor
> >     Calibrating delay loop...
> >
> > The reason for jiffies not changes is there's no timer interrupt
> > passed to dump-capture kernel.
> >
> > In fact, once kernel panic occurs, the local APIC is disabled
> > by lapic_shutdown() in reboot path.
> > generly speaking, local APIC state can be initialized by BIOS
> > after Power-Up or Reset, which doesn't apply to kdump case.
> > so the kernel has to be responsible for initialize the interrupt
> > mode properly according the latest status of APIC in bootup path.
> >
> > An MP operating system is booted under either PIC mode or
> > virtual wire mode. Later, the operating system switches to
> > symmetric I/O mode as it enters multiprocessor mode.
> > Two kinds of virtual wire mode are defined in Intel MP spec:
> > virtual wire mode via local APIC or via I/O APIC.
> >
> > Now we determine the mode of APIC only through a SMP BIOS(MP table).
> > That's not enough. It's better to do further check if APIC works
> > with effective interrupt mode, and then, do some proper setting.
> 
> Reading through the code let me pause a moment and say:
> "Yowzers the interrupt initialization code has gotten hard to follow.  It
> is now full of indirection with ill defined semantics."  pre_vector_init
> indeed.
> 
> I will argue this is the wrong fix.
> 
> We really should not have to worry about getting the system functional
> in virtual wire mode on a modern system.  And looking at the code
> someone has done half the work and made it conditional under
> acpi_gbl_reduced_hardware.
> 
> Now reduced hardware implies a bit more than we ware talking about but
> if there is ACPI apic information we should not need to worry about
> external interrupts and can just enable the apics.
> 
> In fact I think having MPtable information is enough for that.

In my opinion, The MP table is not to be trusted if system starts
without BIOS phrase. 

The chapter 3.8 System Initial State (MP v1.4 spec) said below:
"1. All local APICs are disabled, except for the local APIC of the BSP
if the system starts in Virtual Wire Mode."

And
.....
"The BIOS must disable interrupts to all processors and set the APICs to
the system initial state before giving control to the operating system.
The operating system is responsible for initializing all of the APICs"

The dump-capture kernel won't through the BIOS phrase,
so there is no guarantee of the interrupt mode of APIC and the status of
local APIC.

Although the MP table is read-only,
the dump-capture kernel uses the MP table which be generated before the
first kernel boots up is unreasonable. 
At least, only checking MP table is not enough.
That's the starting point of my patch.

You said “this is the wrong fix”, 
Is there any wrong with the codes or my solution to check the status of
APIC in bootup path?


> So I think what needs to happens is for the apic initialization to get
> an overhaul that makes apic initialization the happy path and the other
> irq controllers the odd backwards compatibility path.  And when we
> are done we never run in anything except full apic mode unless the
> hardware doesn't support it.
The spec requests "An MP operating system is booted under either one of
the two PC/AT-compatible modes. Later the operating system switches to
Symmetric I/O Mode as it enters multiprocessor mode."
And it seems that the latest kernel follows the rule.
Does the apic initialization really need to be changed?

Thanks,
wei
> 
> I think that will leave things more robust as we don't need to setup
> and then reset up the interrupts during boot.
> 
> Eric
> 
> 
> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
> > ---
> >  arch/x86/include/asm/io_apic.h |  5 ++++
> >  arch/x86/kernel/apic/apic.c    | 60 +++++++++++++++++++++++++++++++++++++++++-
> >  arch/x86/kernel/apic/io_apic.c | 28 ++++++++++++++++++++
> >  3 files changed, 92 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> > index 6cbf2cfb3f8a..a3257366bf7f 100644
> > --- a/arch/x86/include/asm/io_apic.h
> > +++ b/arch/x86/include/asm/io_apic.h
> > @@ -190,6 +190,7 @@ static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
> >  }
> >  
> >  extern void setup_IO_APIC(void);
> > +extern bool virt_wire_through_ioapic(void);
> >  extern void enable_IO_APIC(void);
> >  extern void disable_IO_APIC(void);
> >  extern void setup_ioapic_dest(void);
> > @@ -231,6 +232,10 @@ static inline void io_apic_init_mappings(void) { }
> >  #define native_disable_io_apic		NULL
> >  
> >  static inline void setup_IO_APIC(void) { }
> > +static inline bool virt_wire_through_ioapic(void)
> > +{
> > +	return false;
> > +}
> >  static inline void enable_IO_APIC(void) { }
> >  static inline void setup_ioapic_dest(void) { }
> >  
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index 8e25b9b2d351..a3939fb130cc 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1124,6 +1124,58 @@ void __init sync_Arb_IDs(void)
> >  }
> >  
> >  /*
> > + * Check APIC enable/disable flag
> > + */
> > +static bool check_apic_enabled(void)
> > +{
> > +	unsigned int value;
> > +
> > +	/*
> > +	 * If APIC is disabled globally (IA32_APIC_BASE[11] == 0)
> > +	 * the boot cpu hasn't X86_FEATURE_APIC,
> > +	 * and init_bsp_APIC() has already checked it before.
> > +	 * so no need to check global enable/disable flag here
> > +	 */
> > +
> > +	/* Check the software enable/disable flag */
> > +	value = apic_read(APIC_SPIV);
> > +	if (!(value & APIC_SPIV_APIC_ENABLED))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * Return false means the through-local-APIC virtual wire mode is inactive
> > + */
> > +static bool virt_wire_through_lapic(void)
> > +{
> > +	unsigned int value;
> > +
> > +	/*
> > +	 * The through-local-APIC virtual wire mode requests
> > +	 * local APIC to enable LINT0 for ExtINT delivery mode
> > +	 * and LINT1 for NMI delivery mode
> > +	 */
> > +	value = apic_read(APIC_LVT0);
> > +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_EXTINT)
> > +		return false;
> > +
> > +	value = apic_read(APIC_LVT1);
> > +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_NMI)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static bool check_virt_wire_mode(void)
> > +{
> > +	/* If neither of virtual wire mode is active, return false */
> > +	return (check_apic_enabled() && (virt_wire_through_lapic() ||
> > +		virt_wire_through_ioapic()));
> > +}
> > +
> > +/*
> >   * An initial setup of the virtual wire mode.
> >   */
> >  void __init init_bsp_APIC(void)
> > @@ -1133,8 +1185,14 @@ void __init init_bsp_APIC(void)
> >  	/*
> >  	 * Don't do the setup now if we have a SMP BIOS as the
> >  	 * through-I/O-APIC virtual wire mode might be active.
> > +	 *
> > +	 * It's better to do further check if either through-I/O-APIC
> > +	 * or through-local-APIC is active.
> > +	 * the worst case is that both of them are inactive, If so,
> > +	 * we need to enable the through-local-APIC virtual wire mode
> >  	 */
> > -	if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC))
> > +	if (pic_mode || !boot_cpu_has(X86_FEATURE_APIC) ||
> > +	    (smp_found_config && check_virt_wire_mode()))
> >  		return;
> >  
> >  	/*
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index 446702ed99dc..f794d389ba85 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -1379,6 +1379,34 @@ void __init print_IO_APICs(void)
> >  /* Where if anywhere is the i8259 connect in external int mode */
> >  static struct { int pin, apic; } ioapic_i8259 = { -1, -1 };
> >  
> > +/*
> > + * Return false means the through-I/O-APIC virtual wire mode is inactive
> > + */
> > +bool virt_wire_through_ioapic(void)
> > +{
> > +	int apic, pin;
> > +
> > +	for_each_ioapic_pin(apic, pin) {
> > +		/* See if any of the pins is in ExtINT mode */
> > +		struct IO_APIC_route_entry entry = ioapic_read_entry(apic, pin);
> > +
> > +		/*
> > +		 * If the interrupt line is enabled and in ExtInt mode
> > +		 * I have found the pin where the i8259 is connected.
> > +		 */
> > +		if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT))
> > +			return true;
> > +	}
> > +
> > +	/*
> > +	 * The through-I/O-APIC virtual wire mode requests
> > +	 * I/O APIC to be connected i8259 in chapter 3.6.2.2 of the MP v1.4 spec
> > +	 * If no pin in ExtInt mode, the through-I/O-APIC virtual wire mode can
> > +	 * be regarded as inactive
> > +	 */
> > +	return false;
> > +}
> > +
> >  void __init enable_IO_APIC(void)
> >  {
> >  	int i8259_apic, i8259_pin;
> 
> 

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Wei, Jiangang]

Hi Eric,

Thanks for your response.
But I have some different ideas...

On Mon, 2016-07-25 at 22:53 -0500, Eric W. Biederman wrote:
> Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:
> 
> > If we specify the 'notsc' parameter for the dump-capture kernel,
> > and then trigger a crash(panic) by using "ALT-SysRq-c" or
> > "echo c > /proc/sysrq-trigger", the dump-capture kernel will
> > hang in calibrate_delay_converge() and wait for jiffies changes.
> > serial log as follows:
> >
> >     tsc: Fast TSC calibration using PIT
> >     tsc: Detected 2099.947 MHz processor
> >     Calibrating delay loop...
> >
> > The reason for jiffies not changes is there's no timer interrupt
> > passed to dump-capture kernel.
> >
> > In fact, once kernel panic occurs, the local APIC is disabled
> > by lapic_shutdown() in reboot path.
> > generly speaking, local APIC state can be initialized by BIOS
> > after Power-Up or Reset, which doesn't apply to kdump case.
> > so the kernel has to be responsible for initialize the interrupt
> > mode properly according the latest status of APIC in bootup path.
> >
> > An MP operating system is booted under either PIC mode or
> > virtual wire mode. Later, the operating system switches to
> > symmetric I/O mode as it enters multiprocessor mode.
> > Two kinds of virtual wire mode are defined in Intel MP spec:
> > virtual wire mode via local APIC or via I/O APIC.
> >
> > Now we determine the mode of APIC only through a SMP BIOS(MP table).
> > That's not enough. It's better to do further check if APIC works
> > with effective interrupt mode, and then, do some proper setting.
> 
> Reading through the code let me pause a moment and say:
> "Yowzers the interrupt initialization code has gotten hard to follow.  It
> is now full of indirection with ill defined semantics."  pre_vector_init
> indeed.
> 
> I will argue this is the wrong fix.
> 
> We really should not have to worry about getting the system functional
> in virtual wire mode on a modern system.  And looking at the code
> someone has done half the work and made it conditional under
> acpi_gbl_reduced_hardware.
> 
> Now reduced hardware implies a bit more than we ware talking about but
> if there is ACPI apic information we should not need to worry about
> external interrupts and can just enable the apics.
> 
> In fact I think having MPtable information is enough for that.

In my opinion, The MP table is not to be trusted if system starts
without BIOS phrase. 

The chapter 3.8 System Initial State (MP v1.4 spec) said below:
"1. All local APICs are disabled, except for the local APIC of the BSP
if the system starts in Virtual Wire Mode."

And
.....
"The BIOS must disable interrupts to all processors and set the APICs to
the system initial state before giving control to the operating system.
The operating system is responsible for initializing all of the APICs"

The dump-capture kernel won't through the BIOS phrase,
so there is no guarantee of the interrupt mode of APIC and the status of
local APIC.

Although the MP table is read-only,
the dump-capture kernel uses the MP table which be generated before the
first kernel boots up is unreasonable. 
At least, only checking MP table is not enough.
That's the starting point of my patch.

You said “this is the wrong fix”, 
Is there any wrong with the codes or my solution to check the status of
APIC in bootup path?


> So I think what needs to happens is for the apic initialization to get
> an overhaul that makes apic initialization the happy path and the other
> irq controllers the odd backwards compatibility path.  And when we
> are done we never run in anything except full apic mode unless the
> hardware doesn't support it.
The spec requests "An MP operating system is booted under either one of
the two PC/AT-compatible modes. Later the operating system switches to
Symmetric I/O Mode as it enters multiprocessor mode."
And it seems that the latest kernel follows the rule.
Does the apic initialization really need to be changed?

Thanks,
wei
> 
> I think that will leave things more robust as we don't need to setup
> and then reset up the interrupts during boot.
> 
> Eric
> 
> 
> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > Signed-off-by: Wei Jiangang <weijg.fnst@cn.fujitsu.com>
> > ---
> >  arch/x86/include/asm/io_apic.h |  5 ++++
> >  arch/x86/kernel/apic/apic.c    | 60 +++++++++++++++++++++++++++++++++++++++++-
> >  arch/x86/kernel/apic/io_apic.c | 28 ++++++++++++++++++++
> >  3 files changed, 92 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> > index 6cbf2cfb3f8a..a3257366bf7f 100644
> > --- a/arch/x86/include/asm/io_apic.h
> > +++ b/arch/x86/include/asm/io_apic.h
> > @@ -190,6 +190,7 @@ static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
> >  }
> >  
> >  extern void setup_IO_APIC(void);
> > +extern bool virt_wire_through_ioapic(void);
> >  extern void enable_IO_APIC(void);
> >  extern void disable_IO_APIC(void);
> >  extern void setup_ioapic_dest(void);
> > @@ -231,6 +232,10 @@ static inline void io_apic_init_mappings(void) { }
> >  #define native_disable_io_apic		NULL
> >  
> >  static inline void setup_IO_APIC(void) { }
> > +static inline bool virt_wire_through_ioapic(void)
> > +{
> > +	return false;
> > +}
> >  static inline void enable_IO_APIC(void) { }
> >  static inline void setup_ioapic_dest(void) { }
> >  
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > index 8e25b9b2d351..a3939fb130cc 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1124,6 +1124,58 @@ void __init sync_Arb_IDs(void)
> >  }
> >  
> >  /*
> > + * Check APIC enable/disable flag
> > + */
> > +static bool check_apic_enabled(void)
> > +{
> > +	unsigned int value;
> > +
> > +	/*
> > +	 * If APIC is disabled globally (IA32_APIC_BASE[11] == 0)
> > +	 * the boot cpu hasn't X86_FEATURE_APIC,
> > +	 * and init_bsp_APIC() has already checked it before.
> > +	 * so no need to check global enable/disable flag here
> > +	 */
> > +
> > +	/* Check the software enable/disable flag */
> > +	value = apic_read(APIC_SPIV);
> > +	if (!(value & APIC_SPIV_APIC_ENABLED))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * Return false means the through-local-APIC virtual wire mode is inactive
> > + */
> > +static bool virt_wire_through_lapic(void)
> > +{
> > +	unsigned int value;
> > +
> > +	/*
> > +	 * The through-local-APIC virtual wire mode requests
> > +	 * local APIC to enable LINT0 for ExtINT delivery mode
> > +	 * and LINT1 for NMI delivery mode
> > +	 */
> > +	value = apic_read(APIC_LVT0);
> > +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_EXTINT)
> > +		return false;
> > +
> > +	value = apic_read(APIC_LVT1);
> > +	if (GET_APIC_DELIVERY_MODE(value) != APIC_MODE_NMI)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static bool check_virt_wire_mode(void)
> > +{
> > +	/* If neither of virtual wire mode is active, return false */
> > +	return (check_apic_enabled() && (virt_wire_through_lapic() ||
> > +		virt_wire_through_ioapic()));
> > +}
> > +
> > +/*
> >   * An initial setup of the virtual wire mode.
> >   */
> >  void __init init_bsp_APIC(void)
> > @@ -1133,8 +1185,14 @@ void __init init_bsp_APIC(void)
> >  	/*
> >  	 * Don't do the setup now if we have a SMP BIOS as the
> >  	 * through-I/O-APIC virtual wire mode might be active.
> > +	 *
> > +	 * It's better to do further check if either through-I/O-APIC
> > +	 * or through-local-APIC is active.
> > +	 * the worst case is that both of them are inactive, If so,
> > +	 * we need to enable the through-local-APIC virtual wire mode
> >  	 */
> > -	if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC))
> > +	if (pic_mode || !boot_cpu_has(X86_FEATURE_APIC) ||
> > +	    (smp_found_config && check_virt_wire_mode()))
> >  		return;
> >  
> >  	/*
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index 446702ed99dc..f794d389ba85 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -1379,6 +1379,34 @@ void __init print_IO_APICs(void)
> >  /* Where if anywhere is the i8259 connect in external int mode */
> >  static struct { int pin, apic; } ioapic_i8259 = { -1, -1 };
> >  
> > +/*
> > + * Return false means the through-I/O-APIC virtual wire mode is inactive
> > + */
> > +bool virt_wire_through_ioapic(void)
> > +{
> > +	int apic, pin;
> > +
> > +	for_each_ioapic_pin(apic, pin) {
> > +		/* See if any of the pins is in ExtINT mode */
> > +		struct IO_APIC_route_entry entry = ioapic_read_entry(apic, pin);
> > +
> > +		/*
> > +		 * If the interrupt line is enabled and in ExtInt mode
> > +		 * I have found the pin where the i8259 is connected.
> > +		 */
> > +		if ((entry.mask == 0) && (entry.delivery_mode == dest_ExtINT))
> > +			return true;
> > +	}
> > +
> > +	/*
> > +	 * The through-I/O-APIC virtual wire mode requests
> > +	 * I/O APIC to be connected i8259 in chapter 3.6.2.2 of the MP v1.4 spec
> > +	 * If no pin in ExtInt mode, the through-I/O-APIC virtual wire mode can
> > +	 * be regarded as inactive
> > +	 */
> > +	return false;
> > +}
> > +
> >  void __init enable_IO_APIC(void)
> >  {
> >  	int i8259_apic, i8259_pin;
> 
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Wei, Jiangang]

Ping ...
May I ask for some community attention to this series?
I purpose is fixing  the dump-capture kernel hangs in
calibrate_delay_converge() while specifying notsc.

Thanks in advance.
wei
On Tue, 2016-07-26 at 10:59 +0800, Wei Jiangang wrote:
> v2:
> Just about the commit ("x86/apic: Improved the setting of interrupt
>  mode for bsp")
> 
> - Unify the name
>   s/virtual_wire_via_*/virt_wire_through_*
> - Add check for PIC mode
>   suggested-by Baoquan He <bhe@redhat.com>
> - Add check enable/disable flag for IO-APIC
>   suggested-by Xunlei Pang <xpang@redhat.com>
> - Update comments
> 
> v1:
> The goal is to fix dump-capture kernel with notsc option hangs
> in calibrate_delay_converge()
> 
> Wei Jiangang (3):
>   x86/apic: Remove "focus disabled" for 64bit case
>   x86/apic: Update comment about disabling processor focus
>   x86/apic: Improved the setting of interrupt mode for bsp
> 
>  arch/x86/include/asm/io_apic.h |  5 ++++
>  arch/x86/kernel/apic/apic.c    | 63 +++++++++++++++++++++++++++++++++++++++---
>  arch/x86/kernel/apic/io_apic.c | 28 +++++++++++++++++++
>  3 files changed, 92 insertions(+), 4 deletions(-)
> 

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Wei, Jiangang]

Ping ...
May I ask for some community attention to this series?
I purpose is fixing  the dump-capture kernel hangs in
calibrate_delay_converge() while specifying notsc.

Thanks in advance.
wei
On Tue, 2016-07-26 at 10:59 +0800, Wei Jiangang wrote:
> v2:
> Just about the commit ("x86/apic: Improved the setting of interrupt
>  mode for bsp")
> 
> - Unify the name
>   s/virtual_wire_via_*/virt_wire_through_*
> - Add check for PIC mode
>   suggested-by Baoquan He <bhe@redhat.com>
> - Add check enable/disable flag for IO-APIC
>   suggested-by Xunlei Pang <xpang@redhat.com>
> - Update comments
> 
> v1:
> The goal is to fix dump-capture kernel with notsc option hangs
> in calibrate_delay_converge()
> 
> Wei Jiangang (3):
>   x86/apic: Remove "focus disabled" for 64bit case
>   x86/apic: Update comment about disabling processor focus
>   x86/apic: Improved the setting of interrupt mode for bsp
> 
>  arch/x86/include/asm/io_apic.h |  5 ++++
>  arch/x86/kernel/apic/apic.c    | 63 +++++++++++++++++++++++++++++++++++++++---
>  arch/x86/kernel/apic/io_apic.c | 28 +++++++++++++++++++
>  3 files changed, 92 insertions(+), 4 deletions(-)
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Eric W. Biederman]

"Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:

> Ping ...
> May I ask for some community attention to this series?
> I purpose is fixing  the dump-capture kernel hangs in
> calibrate_delay_converge() while specifying notsc.

Did you not see my reply to patch 3/3?

The short version of my feedback is that you seem to be fixing a case
that should not exist.  So the good fix is to skip completely past
virtual wire mode and into full apic mode as soon as possible.

For a subset of cases the code already supports that.

Eric

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Eric W. Biederman]

"Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:

> Ping ...
> May I ask for some community attention to this series?
> I purpose is fixing  the dump-capture kernel hangs in
> calibrate_delay_converge() while specifying notsc.

Did you not see my reply to patch 3/3?

The short version of my feedback is that you seem to be fixing a case
that should not exist.  So the good fix is to skip completely past
virtual wire mode and into full apic mode as soon as possible.

For a subset of cases the code already supports that.

Eric

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Wei, Jiangang]

Hi Eric,

Thanks for your reply firstly.

On Mon, 2016-08-01 at 12:09 -0500, Eric W. Biederman wrote:
> "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:
> 
> > Ping ...
> > May I ask for some community attention to this series?
> > I purpose is fixing  the dump-capture kernel hangs in
> > calibrate_delay_converge() while specifying notsc.
> 
> Did you not see my reply to patch 3/3?

Yes, I read your email and made a reply
(https://lkml.org/lkml/2016/7/26/112) . I put forward several questions
in that letter, but no feedback...

> 
> The short version of my feedback is that you seem to be fixing a case
> that should not exist.  So the good fix is to skip completely past
> virtual wire mode and into full apic mode as soon as possible.

I am afraid that there are some disagreements between us.

1)  The case that dump-capture kernel boot up with the disabled APIC is
very real, and the bug can be reproduced 100%.  I want to emphasize that
there is no guarantee of the interrupt mode of APIC and status of local
APIC, Especially for the dump-capture kernel that won't through the BIOS
phrase. That's why I do more check in init_bsp_APIC(), not only depends
on the MP tables which be generated before the first kernel boots up.

Make a point here, The BIOS must disable interrupts to all processors
and set the APICs to the system initial state before giving control to
the operating system. That means APICs won't be reset to initial state
without BIOS phrase.

2)  Your proposal (switch into full apic mode as soon as possible) seems
to contradict the Intel Spec, "An MP operating system is booted under
either one of the two PC/AT-compatible modes. Later the operating system
switches to Symmetric I/O Mode **as it enters multiprocessor mode**."
And in other words, the BSP should be in PIC mode or Virtual wire mode
in startup stage.

3)  The apic initialization codes maybe need a overhaul, but it goes out
the scope of this patch. I focus on fixing kdump failure with notsc. And
the apic initialization codes has no modification for a long time and
can be regard as stable.  Overhaul of it increases the chances of
hitting a bug.

If there's anything wrong with my understanding, please point out.

Thanks,
wei
> 
> For a subset of cases the code already supports that.
> 
> Eric
> 
> 

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[Wei, Jiangang]

Hi Eric,

Thanks for your reply firstly.

On Mon, 2016-08-01 at 12:09 -0500, Eric W. Biederman wrote:
> "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:
> 
> > Ping ...
> > May I ask for some community attention to this series?
> > I purpose is fixing  the dump-capture kernel hangs in
> > calibrate_delay_converge() while specifying notsc.
> 
> Did you not see my reply to patch 3/3?

Yes, I read your email and made a reply
(https://lkml.org/lkml/2016/7/26/112) . I put forward several questions
in that letter, but no feedback...

> 
> The short version of my feedback is that you seem to be fixing a case
> that should not exist.  So the good fix is to skip completely past
> virtual wire mode and into full apic mode as soon as possible.

I am afraid that there are some disagreements between us.

1)  The case that dump-capture kernel boot up with the disabled APIC is
very real, and the bug can be reproduced 100%.  I want to emphasize that
there is no guarantee of the interrupt mode of APIC and status of local
APIC, Especially for the dump-capture kernel that won't through the BIOS
phrase. That's why I do more check in init_bsp_APIC(), not only depends
on the MP tables which be generated before the first kernel boots up.

Make a point here, The BIOS must disable interrupts to all processors
and set the APICs to the system initial state before giving control to
the operating system. That means APICs won't be reset to initial state
without BIOS phrase.

2)  Your proposal (switch into full apic mode as soon as possible) seems
to contradict the Intel Spec, "An MP operating system is booted under
either one of the two PC/AT-compatible modes. Later the operating system
switches to Symmetric I/O Mode **as it enters multiprocessor mode**."
And in other words, the BSP should be in PIC mode or Virtual wire mode
in startup stage.

3)  The apic initialization codes maybe need a overhaul, but it goes out
the scope of this patch. I focus on fixing kdump failure with notsc. And
the apic initialization codes has no modification for a long time and
can be regard as stable.  Overhaul of it increases the chances of
hitting a bug.

If there's anything wrong with my understanding, please point out.

Thanks,
wei
> 
> For a subset of cases the code already supports that.
> 
> Eric
> 
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[bhe]

On 08/02/16 at 07:45am, Wei, Jiangang wrote:
> Hi Eric,
> 
> Thanks for your reply firstly.
> 
> On Mon, 2016-08-01 at 12:09 -0500, Eric W. Biederman wrote:
> > "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:
> > 
> > > Ping ...
> > > May I ask for some community attention to this series?
> > > I purpose is fixing  the dump-capture kernel hangs in
> > > calibrate_delay_converge() while specifying notsc.
> > 
> > Did you not see my reply to patch 3/3?
> 
> Yes, I read your email and made a reply
> (https://lkml.org/lkml/2016/7/26/112) . I put forward several questions
> in that letter, but no feedback...
> 
> > 
> > The short version of my feedback is that you seem to be fixing a case
> > that should not exist.  So the good fix is to skip completely past
> > virtual wire mode and into full apic mode as soon as possible.
> 
> I am afraid that there are some disagreements between us.
> 
> 1)  The case that dump-capture kernel boot up with the disabled APIC is
> very real, and the bug can be reproduced 100%.  I want to emphasize that
> there is no guarantee of the interrupt mode of APIC and status of local
> APIC, Especially for the dump-capture kernel that won't through the BIOS
> phrase. That's why I do more check in init_bsp_APIC(), not only depends
> on the MP tables which be generated before the first kernel boots up.
> 
> Make a point here, The BIOS must disable interrupts to all processors
> and set the APICs to the system initial state before giving control to
> the operating system. That means APICs won't be reset to initial state
> without BIOS phrase.
> 
> 2)  Your proposal (switch into full apic mode as soon as possible) seems
> to contradict the Intel Spec, "An MP operating system is booted under
> either one of the two PC/AT-compatible modes. Later the operating system
> switches to Symmetric I/O Mode **as it enters multiprocessor mode**."
> And in other words, the BSP should be in PIC mode or Virtual wire mode
> in startup stage.

Well, Eric has clearly told hardware-reduced ACPI platform doesn't have
legacy mode irq. It only has APIC mode. The quotation from MP spec is
very old.

I check code and think now you should investigate the current
implementation, see if APIC mode can be enabled as soon as possible.
Though it can't, detailed explanation need be given to convince people.

> 
> 3)  The apic initialization codes maybe need a overhaul, but it goes out
> the scope of this patch. I focus on fixing kdump failure with notsc. And
> the apic initialization codes has no modification for a long time and
> can be regard as stable.  Overhaul of it increases the chances of
> hitting a bug.
> If there's anything wrong with my understanding, please point out.
> 
> Thanks,
> wei
> > 
> > For a subset of cases the code already supports that.
> > 
> > Eric
> > 
> > 
> 
> 
> 

Re: [PATCH v2 0/3] Fix dump-capture kernel hangs with notsc

[bhe]

On 08/02/16 at 07:45am, Wei, Jiangang wrote:
> Hi Eric,
> 
> Thanks for your reply firstly.
> 
> On Mon, 2016-08-01 at 12:09 -0500, Eric W. Biederman wrote:
> > "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:
> > 
> > > Ping ...
> > > May I ask for some community attention to this series?
> > > I purpose is fixing  the dump-capture kernel hangs in
> > > calibrate_delay_converge() while specifying notsc.
> > 
> > Did you not see my reply to patch 3/3?
> 
> Yes, I read your email and made a reply
> (https://lkml.org/lkml/2016/7/26/112) . I put forward several questions
> in that letter, but no feedback...
> 
> > 
> > The short version of my feedback is that you seem to be fixing a case
> > that should not exist.  So the good fix is to skip completely past
> > virtual wire mode and into full apic mode as soon as possible.
> 
> I am afraid that there are some disagreements between us.
> 
> 1)  The case that dump-capture kernel boot up with the disabled APIC is
> very real, and the bug can be reproduced 100%.  I want to emphasize that
> there is no guarantee of the interrupt mode of APIC and status of local
> APIC, Especially for the dump-capture kernel that won't through the BIOS
> phrase. That's why I do more check in init_bsp_APIC(), not only depends
> on the MP tables which be generated before the first kernel boots up.
> 
> Make a point here, The BIOS must disable interrupts to all processors
> and set the APICs to the system initial state before giving control to
> the operating system. That means APICs won't be reset to initial state
> without BIOS phrase.
> 
> 2)  Your proposal (switch into full apic mode as soon as possible) seems
> to contradict the Intel Spec, "An MP operating system is booted under
> either one of the two PC/AT-compatible modes. Later the operating system
> switches to Symmetric I/O Mode **as it enters multiprocessor mode**."
> And in other words, the BSP should be in PIC mode or Virtual wire mode
> in startup stage.

Well, Eric has clearly told hardware-reduced ACPI platform doesn't have
legacy mode irq. It only has APIC mode. The quotation from MP spec is
very old.

I check code and think now you should investigate the current
implementation, see if APIC mode can be enabled as soon as possible.
Though it can't, detailed explanation need be given to convince people.

> 
> 3)  The apic initialization codes maybe need a overhaul, but it goes out
> the scope of this patch. I focus on fixing kdump failure with notsc. And
> the apic initialization codes has no modification for a long time and
> can be regard as stable.  Overhaul of it increases the chances of
> hitting a bug.
> If there's anything wrong with my understanding, please point out.
> 
> Thanks,
> wei
> > 
> > For a subset of cases the code already supports that.
> > 
> > Eric
> > 
> > 
> 
> 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Eric W. Biederman]

"Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:

> Hi Eric,
>
> Thanks for your response.
> But I have some different ideas...

Apologies for not replying to this earlier your reply got lost in my
spam folder and I overlooked it.

> On Mon, 2016-07-25 at 22:53 -0500, Eric W. Biederman wrote:
>> Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:
>> 
>> > If we specify the 'notsc' parameter for the dump-capture kernel,
>> > and then trigger a crash(panic) by using "ALT-SysRq-c" or
>> > "echo c > /proc/sysrq-trigger", the dump-capture kernel will
>> > hang in calibrate_delay_converge() and wait for jiffies changes.
>> > serial log as follows:
>> >
>> >     tsc: Fast TSC calibration using PIT
>> >     tsc: Detected 2099.947 MHz processor
>> >     Calibrating delay loop...
>> >
>> > The reason for jiffies not changes is there's no timer interrupt
>> > passed to dump-capture kernel.
>> >
>> > In fact, once kernel panic occurs, the local APIC is disabled
>> > by lapic_shutdown() in reboot path.
>> > generly speaking, local APIC state can be initialized by BIOS
>> > after Power-Up or Reset, which doesn't apply to kdump case.
>> > so the kernel has to be responsible for initialize the interrupt
>> > mode properly according the latest status of APIC in bootup path.
>> >
>> > An MP operating system is booted under either PIC mode or
>> > virtual wire mode. Later, the operating system switches to
>> > symmetric I/O mode as it enters multiprocessor mode.
>> > Two kinds of virtual wire mode are defined in Intel MP spec:
>> > virtual wire mode via local APIC or via I/O APIC.
>> >
>> > Now we determine the mode of APIC only through a SMP BIOS(MP table).
>> > That's not enough. It's better to do further check if APIC works
>> > with effective interrupt mode, and then, do some proper setting.
>> 
>> Reading through the code let me pause a moment and say:
>> "Yowzers the interrupt initialization code has gotten hard to follow.  It
>> is now full of indirection with ill defined semantics."  pre_vector_init
>> indeed.
>> 
>> I will argue this is the wrong fix.
>> 
>> We really should not have to worry about getting the system functional
>> in virtual wire mode on a modern system.  And looking at the code
>> someone has done half the work and made it conditional under
>> acpi_gbl_reduced_hardware.
>> 
>> Now reduced hardware implies a bit more than we ware talking about but
>> if there is ACPI apic information we should not need to worry about
>> external interrupts and can just enable the apics.
>> 
>> In fact I think having MPtable information is enough for that.
>
> In my opinion, The MP table is not to be trusted if system starts
> without BIOS phrase. 
>
> The chapter 3.8 System Initial State (MP v1.4 spec) said below:
> "1. All local APICs are disabled, except for the local APIC of the BSP
> if the system starts in Virtual Wire Mode."
>
> And
> .....
> "The BIOS must disable interrupts to all processors and set the APICs to
> the system initial state before giving control to the operating system.
> The operating system is responsible for initializing all of the APICs"
>
> The dump-capture kernel won't through the BIOS phrase,
> so there is no guarantee of the interrupt mode of APIC and the status of
> local APIC.
>
> Although the MP table is read-only,
> the dump-capture kernel uses the MP table which be generated before the
> first kernel boots up is unreasonable. 
> At least, only checking MP table is not enough.
> That's the starting point of my patch.
>
> You said “this is the wrong fix”, 
> Is there any wrong with the codes or my solution to check the status of
> APIC in bootup path?

Today in a dump capture kernel it uses the MP table or the ACPI MP
information to set up the apics.

How I read your patch is you do something clever to get in virtual wire
mode.  AKA a historical PC/AT compatible mode.  I think we should just
skip that mode entirely.

>> So I think what needs to happens is for the apic initialization to get
>> an overhaul that makes apic initialization the happy path and the other
>> irq controllers the odd backwards compatibility path.  And when we
>> are done we never run in anything except full apic mode unless the
>> hardware doesn't support it.
> The spec requests "An MP operating system is booted under either one of
> the two PC/AT-compatible modes. Later the operating system switches to
> Symmetric I/O Mode as it enters multiprocessor mode."
> And it seems that the latest kernel follows the rule.
> Does the apic initialization really need to be changed?

For simplicity when MP support was first added, a few parts of early
bootup were left handled in PC/AT compatible mode.  Mostly it is just
silly.

Unless there is a good reason I think we should strive to remove those
few moments when the system is using interrupts in PC/AT mode and always
run interrupts in the final mode we intend to run interrupts in.
In fact the code under acpi_gbl_reduced_hardware does that for some
special cases already today.

At the end of the day that should end up with a simpler more reliable
kernel.  Especially for the dump capture kernel.

AKA:
I am proposing make the code say something like:
	if (pc_at_compatible_pic_mode) {
        	do_legacy_pic_mode();
        } else {
        	do_mp_mode();
        }

If my memory serves virtual wire mode does not need to be enabled at all
in mp mode, so if we are coming from mp mode (with no shutdown of the
apics in the first kernel).  Then there will be no information anywhere
about which apics need to be programmed into apic mode.

As such to simplify the kdump process it is the wrong process to go back
into virtual wire PC/AT compatible pic mode.  Because once we remove the
code from the crashing kernel we won't have enough information to make
it work.  Unfortunately in some cases it is a one way transition.

Eric

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Eric W. Biederman]

"Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:

> Hi Eric,
>
> Thanks for your response.
> But I have some different ideas...

Apologies for not replying to this earlier your reply got lost in my
spam folder and I overlooked it.

> On Mon, 2016-07-25 at 22:53 -0500, Eric W. Biederman wrote:
>> Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:
>> 
>> > If we specify the 'notsc' parameter for the dump-capture kernel,
>> > and then trigger a crash(panic) by using "ALT-SysRq-c" or
>> > "echo c > /proc/sysrq-trigger", the dump-capture kernel will
>> > hang in calibrate_delay_converge() and wait for jiffies changes.
>> > serial log as follows:
>> >
>> >     tsc: Fast TSC calibration using PIT
>> >     tsc: Detected 2099.947 MHz processor
>> >     Calibrating delay loop...
>> >
>> > The reason for jiffies not changes is there's no timer interrupt
>> > passed to dump-capture kernel.
>> >
>> > In fact, once kernel panic occurs, the local APIC is disabled
>> > by lapic_shutdown() in reboot path.
>> > generly speaking, local APIC state can be initialized by BIOS
>> > after Power-Up or Reset, which doesn't apply to kdump case.
>> > so the kernel has to be responsible for initialize the interrupt
>> > mode properly according the latest status of APIC in bootup path.
>> >
>> > An MP operating system is booted under either PIC mode or
>> > virtual wire mode. Later, the operating system switches to
>> > symmetric I/O mode as it enters multiprocessor mode.
>> > Two kinds of virtual wire mode are defined in Intel MP spec:
>> > virtual wire mode via local APIC or via I/O APIC.
>> >
>> > Now we determine the mode of APIC only through a SMP BIOS(MP table).
>> > That's not enough. It's better to do further check if APIC works
>> > with effective interrupt mode, and then, do some proper setting.
>> 
>> Reading through the code let me pause a moment and say:
>> "Yowzers the interrupt initialization code has gotten hard to follow.  It
>> is now full of indirection with ill defined semantics."  pre_vector_init
>> indeed.
>> 
>> I will argue this is the wrong fix.
>> 
>> We really should not have to worry about getting the system functional
>> in virtual wire mode on a modern system.  And looking at the code
>> someone has done half the work and made it conditional under
>> acpi_gbl_reduced_hardware.
>> 
>> Now reduced hardware implies a bit more than we ware talking about but
>> if there is ACPI apic information we should not need to worry about
>> external interrupts and can just enable the apics.
>> 
>> In fact I think having MPtable information is enough for that.
>
> In my opinion, The MP table is not to be trusted if system starts
> without BIOS phrase. 
>
> The chapter 3.8 System Initial State (MP v1.4 spec) said below:
> "1. All local APICs are disabled, except for the local APIC of the BSP
> if the system starts in Virtual Wire Mode."
>
> And
> .....
> "The BIOS must disable interrupts to all processors and set the APICs to
> the system initial state before giving control to the operating system.
> The operating system is responsible for initializing all of the APICs"
>
> The dump-capture kernel won't through the BIOS phrase,
> so there is no guarantee of the interrupt mode of APIC and the status of
> local APIC.
>
> Although the MP table is read-only,
> the dump-capture kernel uses the MP table which be generated before the
> first kernel boots up is unreasonable. 
> At least, only checking MP table is not enough.
> That's the starting point of my patch.
>
> You said “this is the wrong fix”, 
> Is there any wrong with the codes or my solution to check the status of
> APIC in bootup path?

Today in a dump capture kernel it uses the MP table or the ACPI MP
information to set up the apics.

How I read your patch is you do something clever to get in virtual wire
mode.  AKA a historical PC/AT compatible mode.  I think we should just
skip that mode entirely.

>> So I think what needs to happens is for the apic initialization to get
>> an overhaul that makes apic initialization the happy path and the other
>> irq controllers the odd backwards compatibility path.  And when we
>> are done we never run in anything except full apic mode unless the
>> hardware doesn't support it.
> The spec requests "An MP operating system is booted under either one of
> the two PC/AT-compatible modes. Later the operating system switches to
> Symmetric I/O Mode as it enters multiprocessor mode."
> And it seems that the latest kernel follows the rule.
> Does the apic initialization really need to be changed?

For simplicity when MP support was first added, a few parts of early
bootup were left handled in PC/AT compatible mode.  Mostly it is just
silly.

Unless there is a good reason I think we should strive to remove those
few moments when the system is using interrupts in PC/AT mode and always
run interrupts in the final mode we intend to run interrupts in.
In fact the code under acpi_gbl_reduced_hardware does that for some
special cases already today.

At the end of the day that should end up with a simpler more reliable
kernel.  Especially for the dump capture kernel.

AKA:
I am proposing make the code say something like:
	if (pc_at_compatible_pic_mode) {
        	do_legacy_pic_mode();
        } else {
        	do_mp_mode();
        }

If my memory serves virtual wire mode does not need to be enabled at all
in mp mode, so if we are coming from mp mode (with no shutdown of the
apics in the first kernel).  Then there will be no information anywhere
about which apics need to be programmed into apic mode.

As such to simplify the kdump process it is the wrong process to go back
into virtual wire PC/AT compatible pic mode.  Because once we remove the
code from the crashing kernel we won't have enough information to make
it work.  Unfortunately in some cases it is a one way transition.

Eric

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Wei, Jiangang]

Hi Eric,

I have several questions about kdump and APIC mode.
specific issues at the end of the mail.

On Tue, 2016-08-02 at 09:26 -0500, Eric W. Biederman wrote:
> "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:
> 
> > Hi Eric,
> >
> > Thanks for your response.
> > But I have some different ideas...
> 
> Apologies for not replying to this earlier your reply got lost in my
> spam folder and I overlooked it.
> 
> > On Mon, 2016-07-25 at 22:53 -0500, Eric W. Biederman wrote:
> >> Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:
> >> 
> >> > If we specify the 'notsc' parameter for the dump-capture kernel,
> >> > and then trigger a crash(panic) by using "ALT-SysRq-c" or
> >> > "echo c > /proc/sysrq-trigger", the dump-capture kernel will
> >> > hang in calibrate_delay_converge() and wait for jiffies changes.
> >> > serial log as follows:
> >> >
> >> >     tsc: Fast TSC calibration using PIT
> >> >     tsc: Detected 2099.947 MHz processor
> >> >     Calibrating delay loop...
> >> >
> >> > The reason for jiffies not changes is there's no timer interrupt
> >> > passed to dump-capture kernel.
> >> >
> >> > In fact, once kernel panic occurs, the local APIC is disabled
> >> > by lapic_shutdown() in reboot path.
> >> > generly speaking, local APIC state can be initialized by BIOS
> >> > after Power-Up or Reset, which doesn't apply to kdump case.
> >> > so the kernel has to be responsible for initialize the interrupt
> >> > mode properly according the latest status of APIC in bootup path.
> >> >
> >> > An MP operating system is booted under either PIC mode or
> >> > virtual wire mode. Later, the operating system switches to
> >> > symmetric I/O mode as it enters multiprocessor mode.
> >> > Two kinds of virtual wire mode are defined in Intel MP spec:
> >> > virtual wire mode via local APIC or via I/O APIC.
> >> >
> >> > Now we determine the mode of APIC only through a SMP BIOS(MP table).
> >> > That's not enough. It's better to do further check if APIC works
> >> > with effective interrupt mode, and then, do some proper setting.
> >> 
> >> Reading through the code let me pause a moment and say:
> >> "Yowzers the interrupt initialization code has gotten hard to follow.  It
> >> is now full of indirection with ill defined semantics."  pre_vector_init
> >> indeed.
> >> 
> >> I will argue this is the wrong fix.
> >> 
> >> We really should not have to worry about getting the system functional
> >> in virtual wire mode on a modern system.  And looking at the code
> >> someone has done half the work and made it conditional under
> >> acpi_gbl_reduced_hardware.
> >> 
> >> Now reduced hardware implies a bit more than we ware talking about but
> >> if there is ACPI apic information we should not need to worry about
> >> external interrupts and can just enable the apics.
> >> 
> >> In fact I think having MPtable information is enough for that.
> >
> > In my opinion, The MP table is not to be trusted if system starts
> > without BIOS phrase. 
> >
> > The chapter 3.8 System Initial State (MP v1.4 spec) said below:
> > "1. All local APICs are disabled, except for the local APIC of the BSP
> > if the system starts in Virtual Wire Mode."
> >
> > And
> > .....
> > "The BIOS must disable interrupts to all processors and set the APICs to
> > the system initial state before giving control to the operating system.
> > The operating system is responsible for initializing all of the APICs"
> >
> > The dump-capture kernel won't through the BIOS phrase,
> > so there is no guarantee of the interrupt mode of APIC and the status of
> > local APIC.
> >
> > Although the MP table is read-only,
> > the dump-capture kernel uses the MP table which be generated before the
> > first kernel boots up is unreasonable. 
> > At least, only checking MP table is not enough.
> > That's the starting point of my patch.
> >
> > You said “this is the wrong fix”, 
> > Is there any wrong with the codes or my solution to check the status of
> > APIC in bootup path?
> 
> Today in a dump capture kernel it uses the MP table or the ACPI MP
> information to set up the apics.
> 
> How I read your patch is you do something clever to get in virtual wire
> mode.  AKA a historical PC/AT compatible mode.  I think we should just
> skip that mode entirely.
> 
> >> So I think what needs to happens is for the apic initialization to get
> >> an overhaul that makes apic initialization the happy path and the other
> >> irq controllers the odd backwards compatibility path.  And when we
> >> are done we never run in anything except full apic mode unless the
> >> hardware doesn't support it.
> > The spec requests "An MP operating system is booted under either one of
> > the two PC/AT-compatible modes. Later the operating system switches to
> > Symmetric I/O Mode as it enters multiprocessor mode."
> > And it seems that the latest kernel follows the rule.
> > Does the apic initialization really need to be changed?
> 
> For simplicity when MP support was first added, a few parts of early
> bootup were left handled in PC/AT compatible mode.  Mostly it is just
> silly.
> 
> Unless there is a good reason I think we should strive to remove those
> few moments when the system is using interrupts in PC/AT mode and always
> run interrupts in the final mode we intend to run interrupts in.
> In fact the code under acpi_gbl_reduced_hardware does that for some
> special cases already today.
> 
> At the end of the day that should end up with a simpler more reliable
> kernel.  Especially for the dump capture kernel.
> 
> AKA:
> I am proposing make the code say something like:
> 	if (pc_at_compatible_pic_mode) {
>         	do_legacy_pic_mode();
>         } else {
>         	do_mp_mode();
>         }
> 
> If my memory serves virtual wire mode does not need to be enabled at all
> in mp mode, so if we are coming from mp mode (with no shutdown of the
> apics in the first kernel).  Then there will be no information anywhere
> about which apics need to be programmed into apic mode.
"mp mode" you mentioned means "Symmetric I/O Mode", right ?

> 
> As such to simplify the kdump process it is the wrong process to go back
> into virtual wire PC/AT compatible pic mode.  Because once we remove the
> code from the crashing kernel we won't have enough information to make
> it work.  Unfortunately in some cases it is a one way transition.

You ever told me that the only reason we have do any apic shutdown is
bugs in older kernels. and looks like you're inclined to remove the
codes used to shutdown APIC.
That's an import start point you suggest me to enable "Symmetric I/O
Mode" at the bootup stage of kernel, right?

but In my opinion, the kdump-capture kernel don't need to run on MP
system, but only on BSP. generally, we use kdump with nr_cpus=1. 
If only one cpu is enabled and used,  do we still need to set "Symmetric
I/O Mode" for APIC ? I think the answer is no.

In other words, It's intended that go back into virtual wire PC/AT
compatible pic mode by shutdown APIC.

If there's something wrong with my understanding, Please correct me.

Thanks,
wei
> 
> Eric
> 
> 

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Wei, Jiangang]

Hi Eric,

I have several questions about kdump and APIC mode.
specific issues at the end of the mail.

On Tue, 2016-08-02 at 09:26 -0500, Eric W. Biederman wrote:
> "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:
> 
> > Hi Eric,
> >
> > Thanks for your response.
> > But I have some different ideas...
> 
> Apologies for not replying to this earlier your reply got lost in my
> spam folder and I overlooked it.
> 
> > On Mon, 2016-07-25 at 22:53 -0500, Eric W. Biederman wrote:
> >> Wei Jiangang <weijg.fnst@cn.fujitsu.com> writes:
> >> 
> >> > If we specify the 'notsc' parameter for the dump-capture kernel,
> >> > and then trigger a crash(panic) by using "ALT-SysRq-c" or
> >> > "echo c > /proc/sysrq-trigger", the dump-capture kernel will
> >> > hang in calibrate_delay_converge() and wait for jiffies changes.
> >> > serial log as follows:
> >> >
> >> >     tsc: Fast TSC calibration using PIT
> >> >     tsc: Detected 2099.947 MHz processor
> >> >     Calibrating delay loop...
> >> >
> >> > The reason for jiffies not changes is there's no timer interrupt
> >> > passed to dump-capture kernel.
> >> >
> >> > In fact, once kernel panic occurs, the local APIC is disabled
> >> > by lapic_shutdown() in reboot path.
> >> > generly speaking, local APIC state can be initialized by BIOS
> >> > after Power-Up or Reset, which doesn't apply to kdump case.
> >> > so the kernel has to be responsible for initialize the interrupt
> >> > mode properly according the latest status of APIC in bootup path.
> >> >
> >> > An MP operating system is booted under either PIC mode or
> >> > virtual wire mode. Later, the operating system switches to
> >> > symmetric I/O mode as it enters multiprocessor mode.
> >> > Two kinds of virtual wire mode are defined in Intel MP spec:
> >> > virtual wire mode via local APIC or via I/O APIC.
> >> >
> >> > Now we determine the mode of APIC only through a SMP BIOS(MP table).
> >> > That's not enough. It's better to do further check if APIC works
> >> > with effective interrupt mode, and then, do some proper setting.
> >> 
> >> Reading through the code let me pause a moment and say:
> >> "Yowzers the interrupt initialization code has gotten hard to follow.  It
> >> is now full of indirection with ill defined semantics."  pre_vector_init
> >> indeed.
> >> 
> >> I will argue this is the wrong fix.
> >> 
> >> We really should not have to worry about getting the system functional
> >> in virtual wire mode on a modern system.  And looking at the code
> >> someone has done half the work and made it conditional under
> >> acpi_gbl_reduced_hardware.
> >> 
> >> Now reduced hardware implies a bit more than we ware talking about but
> >> if there is ACPI apic information we should not need to worry about
> >> external interrupts and can just enable the apics.
> >> 
> >> In fact I think having MPtable information is enough for that.
> >
> > In my opinion, The MP table is not to be trusted if system starts
> > without BIOS phrase. 
> >
> > The chapter 3.8 System Initial State (MP v1.4 spec) said below:
> > "1. All local APICs are disabled, except for the local APIC of the BSP
> > if the system starts in Virtual Wire Mode."
> >
> > And
> > .....
> > "The BIOS must disable interrupts to all processors and set the APICs to
> > the system initial state before giving control to the operating system.
> > The operating system is responsible for initializing all of the APICs"
> >
> > The dump-capture kernel won't through the BIOS phrase,
> > so there is no guarantee of the interrupt mode of APIC and the status of
> > local APIC.
> >
> > Although the MP table is read-only,
> > the dump-capture kernel uses the MP table which be generated before the
> > first kernel boots up is unreasonable. 
> > At least, only checking MP table is not enough.
> > That's the starting point of my patch.
> >
> > You said “this is the wrong fix”, 
> > Is there any wrong with the codes or my solution to check the status of
> > APIC in bootup path?
> 
> Today in a dump capture kernel it uses the MP table or the ACPI MP
> information to set up the apics.
> 
> How I read your patch is you do something clever to get in virtual wire
> mode.  AKA a historical PC/AT compatible mode.  I think we should just
> skip that mode entirely.
> 
> >> So I think what needs to happens is for the apic initialization to get
> >> an overhaul that makes apic initialization the happy path and the other
> >> irq controllers the odd backwards compatibility path.  And when we
> >> are done we never run in anything except full apic mode unless the
> >> hardware doesn't support it.
> > The spec requests "An MP operating system is booted under either one of
> > the two PC/AT-compatible modes. Later the operating system switches to
> > Symmetric I/O Mode as it enters multiprocessor mode."
> > And it seems that the latest kernel follows the rule.
> > Does the apic initialization really need to be changed?
> 
> For simplicity when MP support was first added, a few parts of early
> bootup were left handled in PC/AT compatible mode.  Mostly it is just
> silly.
> 
> Unless there is a good reason I think we should strive to remove those
> few moments when the system is using interrupts in PC/AT mode and always
> run interrupts in the final mode we intend to run interrupts in.
> In fact the code under acpi_gbl_reduced_hardware does that for some
> special cases already today.
> 
> At the end of the day that should end up with a simpler more reliable
> kernel.  Especially for the dump capture kernel.
> 
> AKA:
> I am proposing make the code say something like:
> 	if (pc_at_compatible_pic_mode) {
>         	do_legacy_pic_mode();
>         } else {
>         	do_mp_mode();
>         }
> 
> If my memory serves virtual wire mode does not need to be enabled at all
> in mp mode, so if we are coming from mp mode (with no shutdown of the
> apics in the first kernel).  Then there will be no information anywhere
> about which apics need to be programmed into apic mode.
"mp mode" you mentioned means "Symmetric I/O Mode", right ?

> 
> As such to simplify the kdump process it is the wrong process to go back
> into virtual wire PC/AT compatible pic mode.  Because once we remove the
> code from the crashing kernel we won't have enough information to make
> it work.  Unfortunately in some cases it is a one way transition.

You ever told me that the only reason we have do any apic shutdown is
bugs in older kernels. and looks like you're inclined to remove the
codes used to shutdown APIC.
That's an import start point you suggest me to enable "Symmetric I/O
Mode" at the bootup stage of kernel, right?

but In my opinion, the kdump-capture kernel don't need to run on MP
system, but only on BSP. generally, we use kdump with nr_cpus=1. 
If only one cpu is enabled and used,  do we still need to set "Symmetric
I/O Mode" for APIC ? I think the answer is no.

In other words, It's intended that go back into virtual wire PC/AT
compatible pic mode by shutdown APIC.

If there's something wrong with my understanding, Please correct me.

Thanks,
wei
> 
> Eric
> 
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Eric W. Biederman]

"Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:

> Hi Eric,
>
> I have several questions about kdump and APIC mode.
> specific issues at the end of the mail.
>
> On Tue, 2016-08-02 at 09:26 -0500, Eric W. Biederman wrote:
[snip]
>> For simplicity when MP support was first added, a few parts of early
>> bootup were left handled in PC/AT compatible mode.  Mostly it is just
>> silly.
>> 
>> Unless there is a good reason I think we should strive to remove those
>> few moments when the system is using interrupts in PC/AT mode and always
>> run interrupts in the final mode we intend to run interrupts in.
>> In fact the code under acpi_gbl_reduced_hardware does that for some
>> special cases already today.
>> 
>> At the end of the day that should end up with a simpler more reliable
>> kernel.  Especially for the dump capture kernel.
>> 
>> AKA:
>> I am proposing make the code say something like:
>> 	if (pc_at_compatible_pic_mode) {
>>         	do_legacy_pic_mode();
>>         } else {
>>         	do_mp_mode();
>>         }
>> 
>> If my memory serves virtual wire mode does not need to be enabled at all
>> in mp mode, so if we are coming from mp mode (with no shutdown of the
>> apics in the first kernel).  Then there will be no information anywhere
>> about which apics need to be programmed into apic mode.
> "mp mode" you mentioned means "Symmetric I/O Mode", right ?

Yes.

>> As such to simplify the kdump process it is the wrong process to go back
>> into virtual wire PC/AT compatible pic mode.  Because once we remove the
>> code from the crashing kernel we won't have enough information to make
>> it work.  Unfortunately in some cases it is a one way transition.
>
> You ever told me that the only reason we have do any apic shutdown is
> bugs in older kernels. and looks like you're inclined to remove the
> codes used to shutdown APIC.
> That's an import start point you suggest me to enable "Symmetric I/O
> Mode" at the bootup stage of kernel, right?

Limitations in older kernels that made it very hard to initialize
Symmetric I/O mode during bootup.  So the code started by doing the
pragmatic thing.  The limitations in the older kernels that required we
transition through PIC mode on bootup appear long since gone.  But the
code in the bootup path is still doing that silly thing, and enabling
interrupts and running in PIC mode for just a moment before switching to
the final way we handle interrupts.

Except for the history that got us there it is a ludicrous way to
operation.

> but In my opinion, the kdump-capture kernel don't need to run on MP
> system, but only on BSP. generally, we use kdump with nr_cpus=1. 
> If only one cpu is enabled and used,  do we still need to set "Symmetric
> I/O Mode" for APIC ? I think the answer is no.

We still need to set "Symmetric I/O Mode"  Part of the problem is that
we can not guarantee that we are running on the BSP when we crash.  We
can not guarantee that we can switch CPUs.  Which makes anything that
explicitly requires the use of the bootstrap processor a problem.

Today the state of the code is we transition through PIC mode before
landing in "Symmetric I/O Mode".

Further the most reliable thing we can do is to disable interrupts (aka
local_irq_disable) with the APICs in "Symmetric I/O Mode" in the
crashing kernel.  Then in the kernel that is comming up we can reprogram
the APICs to be in the same mode they are already in, and then call
(local_irq_enable).

Returning to PIC mode even with a fully working kernel is a little
tricky during ordinary kexec (as not all of the information is there) I
updated the code to probe for the missing information at bootup (while
we are still in PIC mode) years ago.  So it seems to work when
everything is perfect.

We got into this entire conversation because what we are doing on the
crash shutdown epath is not enough to return us to PIC mode and allow a
kernel that assumes virtual wire mode is working to boot.  Which to me
means it is time to short circuit things and go directly to the normal
non-PIC mode and not have a detour through PIC mode of any sort during
boot of up a crash capture kernel.

> In other words, It's intended that go back into virtual wire PC/AT
> compatible pic mode by shutdown APIC.

On the kexec on panic apic shutdown path yes.  That is what the code
currently attempts to do and as demonstrated by the existence of this
thread that code is no longer good enough.

Eric

Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp

[Eric W. Biederman]

"Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> writes:

> Hi Eric,
>
> I have several questions about kdump and APIC mode.
> specific issues at the end of the mail.
>
> On Tue, 2016-08-02 at 09:26 -0500, Eric W. Biederman wrote:
[snip]
>> For simplicity when MP support was first added, a few parts of early
>> bootup were left handled in PC/AT compatible mode.  Mostly it is just
>> silly.
>> 
>> Unless there is a good reason I think we should strive to remove those
>> few moments when the system is using interrupts in PC/AT mode and always
>> run interrupts in the final mode we intend to run interrupts in.
>> In fact the code under acpi_gbl_reduced_hardware does that for some
>> special cases already today.
>> 
>> At the end of the day that should end up with a simpler more reliable
>> kernel.  Especially for the dump capture kernel.
>> 
>> AKA:
>> I am proposing make the code say something like:
>> 	if (pc_at_compatible_pic_mode) {
>>         	do_legacy_pic_mode();
>>         } else {
>>         	do_mp_mode();
>>         }
>> 
>> If my memory serves virtual wire mode does not need to be enabled at all
>> in mp mode, so if we are coming from mp mode (with no shutdown of the
>> apics in the first kernel).  Then there will be no information anywhere
>> about which apics need to be programmed into apic mode.
> "mp mode" you mentioned means "Symmetric I/O Mode", right ?

Yes.

>> As such to simplify the kdump process it is the wrong process to go back
>> into virtual wire PC/AT compatible pic mode.  Because once we remove the
>> code from the crashing kernel we won't have enough information to make
>> it work.  Unfortunately in some cases it is a one way transition.
>
> You ever told me that the only reason we have do any apic shutdown is
> bugs in older kernels. and looks like you're inclined to remove the
> codes used to shutdown APIC.
> That's an import start point you suggest me to enable "Symmetric I/O
> Mode" at the bootup stage of kernel, right?

Limitations in older kernels that made it very hard to initialize
Symmetric I/O mode during bootup.  So the code started by doing the
pragmatic thing.  The limitations in the older kernels that required we
transition through PIC mode on bootup appear long since gone.  But the
code in the bootup path is still doing that silly thing, and enabling
interrupts and running in PIC mode for just a moment before switching to
the final way we handle interrupts.

Except for the history that got us there it is a ludicrous way to
operation.

> but In my opinion, the kdump-capture kernel don't need to run on MP
> system, but only on BSP. generally, we use kdump with nr_cpus=1. 
> If only one cpu is enabled and used,  do we still need to set "Symmetric
> I/O Mode" for APIC ? I think the answer is no.

We still need to set "Symmetric I/O Mode"  Part of the problem is that
we can not guarantee that we are running on the BSP when we crash.  We
can not guarantee that we can switch CPUs.  Which makes anything that
explicitly requires the use of the bootstrap processor a problem.

Today the state of the code is we transition through PIC mode before
landing in "Symmetric I/O Mode".

Further the most reliable thing we can do is to disable interrupts (aka
local_irq_disable) with the APICs in "Symmetric I/O Mode" in the
crashing kernel.  Then in the kernel that is comming up we can reprogram
the APICs to be in the same mode they are already in, and then call
(local_irq_enable).

Returning to PIC mode even with a fully working kernel is a little
tricky during ordinary kexec (as not all of the information is there) I
updated the code to probe for the missing information at bootup (while
we are still in PIC mode) years ago.  So it seems to work when
everything is perfect.

We got into this entire conversation because what we are doing on the
crash shutdown epath is not enough to return us to PIC mode and allow a
kernel that assumes virtual wire mode is working to boot.  Which to me
means it is time to short circuit things and go directly to the normal
non-PIC mode and not have a detour through PIC mode of any sort during
boot of up a crash capture kernel.

> In other words, It's intended that go back into virtual wire PC/AT
> compatible pic mode by shutdown APIC.

On the kexec on panic apic shutdown path yes.  That is what the code
currently attempts to do and as demonstrated by the existence of this
thread that code is no longer good enough.

Eric

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec