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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Wei Jiangang <weijg.fnst@xxxxxxxxxxxxxx>
> > ---
> >  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;
> 
>