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

[Wei, Jiangang]

Hi He,

Thanks for your response firstly.

On Fri, 2016-07-22 at 18:40 +0800, Baoquan He wrote:
> Hi Jiangang,
> 
> This is very nice, should be the stuff Eric and Ingo would like to see.
> But I have several questions:
> 
> 1) Are you not going to clean up the old legacy irq mode setting code in
> 1st kernel?
Yes, I would like to pay more attention on fixing kdump's failure with
notsc.
No plan to clean up the irq mode setting codes in the crash kernel
reboot path.
If you are interested in it, please go on.

> 
> 2)I call them legacy irq mode because not only apic virtual wire mode
> exists, but the PIC mode in x86 32bit system. You need consider it too.
> Then init_bsp_APIC need be renamaed to an appropriate one. And assume
> this has been tested on x86 32bit system. 

Thanks for your reminders.
As the comment of init_bsp_APIC(), it's just used to setup the virtual
wire mode.
so I won't change its name.

But i agree with that PIC mode should be considered.
Next version will be like below,

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 04358e0cf1e2..d40bab947a2a 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1186,7 +1186,7 @@ void __init init_bsp_APIC(void)
         * the worst case is that both of them are inactive,
         * If so, We need to enable the virtual wire mode via
through-local-APIC
         */
-       if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC)
+       if ( pic_mode || (smp_found_config && check_virtual_wire_mode())
||
                !boot_cpu_has(X86_FEATURE_APIC))
                return;
> 
> 3) 
> 
> *)About IO-APIC setting as virtual wire mode, I am always confused. In
> MP Spec 3.6.2.2, it says "the interrupt signal passes through both the
> I/O APIC and the BSPâs local APIC". That means IO-APIC virtual wire mode
> need both IO-APIC and LAPIC to be set, and with my understanding only
> pin of IO-APIC is set as ExtInt, LAPIC should be pass-through.
> 
> *)But in Intel Arch manual 3A 10.1, there's only below sentences to mention
> it:
> 
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> The I/O APIC also has a âvirtual wire modeâ that allows it to communicate
> with a standard 8259A-style external interrupt controller. Note that the
> local APIC can be disabled. This allows an associated processor core to
> receive interrupts directly from an 8259A interrupt controller.
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Eric's code in native_disable_io_apic() has the same point as above
> words.

IMOï
the through-IO-APIC mode has no relationship with the setting of local
APIC.
that's why i only check the pin of IO-APIC in virtual_wire_via_ioapic().

Thanks
wei
> 
> *)However please read code comments in irq_remapping_disable_io_apic(),
> Joerg's description give me a different impression that we can choose
> to only use LAPIC virtual wire mode. Joerg is IOMMU maintainers, he is
> very familiar with io-apic since IOMMU need take over io-apic entry
> filling, there must be reason he wrote that. Add Joerg to CC list.
> 
> Seems it's difficult to find a system with IO-APIC virtual wire mode,
> maybe we can just keep it as is. Not sure if Intel engineers can help
> explain and confirm this.
> 
> That's all I can think of.
> 
> Thanks
> Baoquan
> 
> On 07/22/16 at 04:10pm, Wei Jiangang wrote:
> > 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 mode,
> > and do some porper setting.
> > 
> > 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    | 55 +++++++++++++++++++++++++++++++++++++++++-
> >  arch/x86/kernel/apic/io_apic.c | 28 +++++++++++++++++++++
> >  3 files changed, 87 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> > index 6cbf2cfb3f8a..6550bd43fa39 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 virtual_wire_via_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 virtual_wire_via_ioapic(void)
> > +{
> > +	return true;
> > +}
> >  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..04358e0cf1e2 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1124,6 +1124,53 @@ void __init sync_Arb_IDs(void)
> >  }
> >  
> >  /*
> > + * Return false means the virtual wire mode through-local-apic is inactive
> > + */
> > +static bool virtual_wire_via_lapic(void)
> > +{
> > +	unsigned int value;
> > +
> > +	/* Check the APIC global enable/disable flag firstly */
> > +	if (boot_cpu_data.x86 >= 6) {
> > +		u32 h, l;
> > +
> > +		rdmsr(MSR_IA32_APICBASE, l, h);
> > +		/*
> > +		 * If local APIC is disabled by BIOS
> > +		 * do nothing, but return true
> > +		 */
> > +		if (!(l & MSR_IA32_APICBASE_ENABLE))
> > +			return true;
> > +	}
> > +
> > +	/* Check the software enable/disable flag */
> > +	value = apic_read(APIC_SPIV);
> > +	if (!(value & APIC_SPIV_APIC_ENABLED))
> > +		return false;
> > +
> > +	/*
> > +	 * Virtual wire mode via local APIC requests
> > +	 * APIC to enable the LINT0 for edge-trggered ExtINT delivery mode
> > +	 * and LINT1 for level-triggered 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_virtual_wire_mode(void)
> > +{
> > +	/* Neither of virtual wire mode is active, return false */
> > +	return  virtual_wire_via_lapic() || virtual_wire_via_ioapic();
> > +}
> > +
> > +/*
> >   * An initial setup of the virtual wire mode.
> >   */
> >  void __init init_bsp_APIC(void)
> > @@ -1133,8 +1180,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 virtual wire mode via through-local-APIC
> >  	 */
> > -	if (smp_found_config || !boot_cpu_has(X86_FEATURE_APIC))
> > +	if ((smp_found_config && check_virtual_wire_mode()) ||
> > +		!boot_cpu_has(X86_FEATURE_APIC))
> >  		return;
> >  
> >  	/*
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index 446702ed99dc..5a32c26938ac 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 virtual wire mode via I/O APIC is inactive
> > + */
> > +bool virtual_wire_via_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;
> > +	}
> > +
> > +	/*
> > +	 * Virtual wire mode via I/O APIC requests
> > +	 * I/O APIC be connected to 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 inactive.
> > +	 */
> > +	return false;
> > +}
> > +
> >  void __init enable_IO_APIC(void)
> >  {
> >  	int i8259_apic, i8259_pin;
> > -- 
> > 1.9.3
> > 
> > 
> > 
> 
>