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

[Xunlei Pang]

On 2016/07/25 at 11:04, Wei, Jiangang wrote:
> 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())

I think this is needless, as init_bsp_APIC() is made under the following condition:
#if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)

> ||
>                 !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().

For through-IO-APIC mode, I think the bsp local apic should be on, but its LINT0 pin must not
be set to be "ExtINT"(I guess with this mode set will make it to be through-LAPIC mode), and
we can clearly see the fact from disconnect_bsp_APIC()'s implementation.

Regards,
Xunlei
> 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
>>>
>>>
>>>
>>
>
>