Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450342 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp Wei Jiangang <weijg.fnst@cn.fujitsu.com> - 2016-07-26 05:10 +0200
    Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp ebiederm@xmission.com (Eric W. Biederman) - 2016-07-26 06:10 +0200
      Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode  for bsp "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> - 2016-07-26 10:10 +0200
        Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp ebiederm@xmission.com (Eric W. Biederman) - 2016-08-02 16:40 +0200
          Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode  for bsp "Wei, Jiangang" <weijg.fnst@cn.fujitsu.com> - 2016-08-04 12:20 +0200
            Re: [PATCH v2 3/3] x86/apic: Improved the setting of interrupt mode for bsp ebiederm@xmission.com (Eric W. Biederman) - 2016-08-04 17:00 +0200

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

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

[toc] | [next] | [standalone]

#1450354

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;

[toc] | [prev] | [next] | [standalone]

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

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;
> 
> 

[toc] | [prev] | [next] | [standalone]

#1454601

"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

[toc] | [prev] | [next] | [standalone]

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

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
> 
> 

[toc] | [prev] | [next] | [standalone]

#1456453

"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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web