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

From: Wei, Jiangang
Date: Mon Jul 25 2016 - 05:15:36 EST

Next message: Benjamin Tissoires: "Re: [PATCH v2] HID: sony: disable descriptor fixup for FutureMax Dance Mat"
Previous message: Benjamin Tissoires: "Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat"
In reply to: Xunlei Pang: "Re: [PATCH 3/3] x86/apic: Improved the setting of interrupt mode for bsp"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Benjamin Tissoires: "Re: [PATCH v2] HID: sony: disable descriptor fixup for FutureMax Dance Mat"
Previous message: Benjamin Tissoires: "Re: [PATCH] HID: sony: disable descriptor fixup for FutureMax Dance Mat"
In reply to: Xunlei Pang: "Re: [PATCH 3/3] x86/apic: Improved the setting of interrupt mode for bsp"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]