On Mon, 2016-07-25 at 16:21 +0800, Xunlei Pang wrote: > 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) Nope, I suggest you take a look at default_get_smp_config(). ......... #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86_32) if (mpf->feature2 & (1 << 7)) { pr_info(" IMCR and PIC compatibility mode.\n"); pic_mode = 1; } else { pr_info(" Virtual Wire compatibility mode.\n"); pic_mode = 0; } #endif ....... if we declare CONFIG_X86_32 and CONFIG_X86_LOCAL_APIC, . init_bsp_APIC may be called. so check pic_mode is useful. > > > || > > !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. Yes, your opinion looks reasonable. both through-IO-APIC and through-local-APIC enable the software flag. /* For the spurious interrupt use vector F, and enable it */ value = apic_read(APIC_SPIV); value &= ~APIC_VECTOR_MASK; value |= APIC_SPIV_APIC_ENABLED; value |= 0xf; apic_write(APIC_SPIV, value); I will accept this point in v2 patch. Thanks, wei > > 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 at cn.fujitsu.com> > >>> Signed-off-by: Wei Jiangang <weijg.fnst at cn.fujitsu.com> > >>> --- > >>> 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 > >>> > >>> > >>> > >> > > > > > > >