| Message ID | 20200612115200.1006881-2-vicamo.yang@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix boot regression caused by a disabled PIT | expand |
On 12.06.20 13:52, You-Sheng Yang wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > BugLink: https://bugs.launchpad.net/bugs/1856387 > > Tony reported a boot regression caused by the recent workaround for systems > which have a disabled (clock gate off) PIT. > > On his machine the kernel fails to initialize the PIT because > apic_needs_pit() does not take into account whether the local APIC > interrupt delivery mode will actually allow to setup and use the local > APIC timer. This should be easy to reproduce with acpi=off on the > command line which also disables HPET. > > Due to the way the PIT/HPET and APIC setup ordering works (APIC setup can > require working PIT/HPET) the information is not available at the point > where apic_needs_pit() makes this decision. > > To address this, split out the interrupt mode selection from > apic_intr_mode_init(), invoke the selection before making the decision > whether PIT is required or not, and add the missing checks into > apic_needs_pit(). > > Fixes: c8c4076723da ("x86/timer: Skip PIT initialization on modern chipsets") > Reported-by: Anthony Buckley <tony.buckley000@gmail.com> > Tested-by: Anthony Buckley <tony.buckley000@gmail.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Ingo Molnar <mingo@kernel.org> > Cc: Daniel Drake <drake@endlessm.com> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206125 > Link: https://lore.kernel.org/r/87sgk6tmk2.fsf@nanos.tec.linutronix.de > (backported from commit 979923871f69a4dc926658f9f9a1a4c1bde57552) > Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Positive testing in bug report. > arch/x86/include/asm/apic.h | 2 ++ > arch/x86/include/asm/x86_init.h | 2 ++ > arch/x86/kernel/apic/apic.c | 23 ++++++++++++++++++----- > arch/x86/kernel/time.c | 12 ++++++++++-- > arch/x86/kernel/x86_init.c | 1 + > arch/x86/xen/enlighten_pv.c | 1 + > 6 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h > index 3df5d195d647..314a5b54a485 100644 > --- a/arch/x86/include/asm/apic.h > +++ b/arch/x86/include/asm/apic.h > @@ -138,6 +138,7 @@ extern void disable_local_APIC(void); > extern void lapic_shutdown(void); > extern void sync_Arb_IDs(void); > extern void init_bsp_APIC(void); > +extern void apic_intr_mode_select(void); > extern void apic_intr_mode_init(void); > extern void setup_local_APIC(void); > extern void init_apic_mappings(void); > @@ -185,6 +186,7 @@ static inline void disable_local_APIC(void) { } > # define setup_boot_APIC_clock x86_init_noop > # define setup_secondary_APIC_clock x86_init_noop > static inline void lapic_update_tsc_freq(void) { } > +static inline void apic_intr_mode_select(void) { } > static inline void apic_intr_mode_init(void) { } > static inline void lapic_assign_system_vectors(void) { } > static inline void lapic_assign_legacy_vector(unsigned int i, bool r) { } > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h > index aa4747569e23..2803cef02c14 100644 > --- a/arch/x86/include/asm/x86_init.h > +++ b/arch/x86/include/asm/x86_init.h > @@ -51,12 +51,14 @@ struct x86_init_resources { > * are set up. > * @intr_init: interrupt init code > * @trap_init: platform specific trap setup > + * @intr_mode_select: interrupt delivery mode selection > * @intr_mode_init: interrupt delivery mode setup > */ > struct x86_init_irqs { > void (*pre_vector_init)(void); > void (*intr_init)(void); > void (*trap_init)(void); > + void (*intr_mode_select)(void); > void (*intr_mode_init)(void); > }; > > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index f15084536abe..d0cd6722c180 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -805,8 +805,17 @@ bool __init apic_needs_pit(void) > if (!tsc_khz || !cpu_khz) > return true; > > - /* Is there an APIC at all? */ > - if (!boot_cpu_has(X86_FEATURE_APIC)) > + /* Is there an APIC at all or is it disabled? */ > + if (!boot_cpu_has(X86_FEATURE_APIC) || disable_apic) > + return true; > + > + /* > + * If interrupt delivery mode is legacy PIC or virtual wire without > + * configuration, the local APIC timer wont be set up. Make sure > + * that the PIT is initialized. > + */ > + if (apic_intr_mode == APIC_PIC || > + apic_intr_mode == APIC_VIRTUAL_WIRE_NO_CONFIG) > return true; > > /* Deadline timer is based on TSC so no further PIT action required */ > @@ -1293,7 +1302,7 @@ void __init sync_Arb_IDs(void) > > enum apic_intr_mode_id apic_intr_mode; > > -static int __init apic_intr_mode_select(void) > +static int __init __apic_intr_mode_select(void) > { > /* Check kernel option */ > if (disable_apic) { > @@ -1355,6 +1364,12 @@ static int __init apic_intr_mode_select(void) > return APIC_SYMMETRIC_IO; > } > > +/* Select the interrupt delivery mode for the BSP */ > +void __init apic_intr_mode_select(void) > +{ > + apic_intr_mode = __apic_intr_mode_select(); > +} > + > /* > * An initial setup of the virtual wire mode. > */ > @@ -1409,8 +1424,6 @@ void __init apic_intr_mode_init(void) > { > bool upmode = IS_ENABLED(CONFIG_UP_LATE_INIT); > > - apic_intr_mode = apic_intr_mode_select(); > - > switch (apic_intr_mode) { > case APIC_PIC: > pr_info("APIC: Keep in PIC mode(8259)\n"); > diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c > index 5339ffd488d0..fd6deda443f9 100644 > --- a/arch/x86/kernel/time.c > +++ b/arch/x86/kernel/time.c > @@ -88,10 +88,18 @@ void __init hpet_time_init(void) > > static __init void x86_late_time_init(void) > { > + /* > + * Before PIT/HPET init, select the interrupt mode. This is required > + * to make the decision whether PIT should be initialized correct. > + */ > + x86_init.irqs.intr_mode_select(); > + > + /* Setup the legacy timers */ > x86_init.timers.timer_init(); > + > /* > - * After PIT/HPET timers init, select and setup > - * the final interrupt mode for delivering IRQs. > + * After PIT/HPET timers init, set up the final interrupt mode for > + * delivering IRQs. > */ > x86_init.irqs.intr_mode_init(); > tsc_init(); > diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c > index 1151ccd72ce9..6c9adc14df26 100644 > --- a/arch/x86/kernel/x86_init.c > +++ b/arch/x86/kernel/x86_init.c > @@ -57,6 +57,7 @@ struct x86_init_ops x86_init __initdata = { > .pre_vector_init = init_ISA_irqs, > .intr_init = native_init_IRQ, > .trap_init = x86_init_noop, > + .intr_mode_select = apic_intr_mode_select, > .intr_mode_init = apic_intr_mode_init > }, > > diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c > index 88c6d9d7e284..c2556f32f156 100644 > --- a/arch/x86/xen/enlighten_pv.c > +++ b/arch/x86/xen/enlighten_pv.c > @@ -1246,6 +1246,7 @@ asmlinkage __visible void __init xen_start_kernel(void) > x86_platform.get_nmi_reason = xen_get_nmi_reason; > > x86_init.resources.memory_setup = xen_memory_setup; > + x86_init.irqs.intr_mode_select = x86_init_noop; > x86_init.irqs.intr_mode_init = x86_init_noop; > x86_init.oem.arch_setup = xen_arch_setup; > x86_init.oem.banner = xen_banner; >
Acked-by: Sultan Alsawaf <sultan.alsawaf@canonical.com> On Fri, Jun 12, 2020 at 07:52:00PM +0800, You-Sheng Yang wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > BugLink: https://bugs.launchpad.net/bugs/1856387 > > Tony reported a boot regression caused by the recent workaround for systems > which have a disabled (clock gate off) PIT. > > On his machine the kernel fails to initialize the PIT because > apic_needs_pit() does not take into account whether the local APIC > interrupt delivery mode will actually allow to setup and use the local > APIC timer. This should be easy to reproduce with acpi=off on the > command line which also disables HPET. > > Due to the way the PIT/HPET and APIC setup ordering works (APIC setup can > require working PIT/HPET) the information is not available at the point > where apic_needs_pit() makes this decision. > > To address this, split out the interrupt mode selection from > apic_intr_mode_init(), invoke the selection before making the decision > whether PIT is required or not, and add the missing checks into > apic_needs_pit(). > > Fixes: c8c4076723da ("x86/timer: Skip PIT initialization on modern chipsets") > Reported-by: Anthony Buckley <tony.buckley000@gmail.com> > Tested-by: Anthony Buckley <tony.buckley000@gmail.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Ingo Molnar <mingo@kernel.org> > Cc: Daniel Drake <drake@endlessm.com> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206125 > Link: https://lore.kernel.org/r/87sgk6tmk2.fsf@nanos.tec.linutronix.de > (backported from commit 979923871f69a4dc926658f9f9a1a4c1bde57552) > Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com> > --- > arch/x86/include/asm/apic.h | 2 ++ > arch/x86/include/asm/x86_init.h | 2 ++ > arch/x86/kernel/apic/apic.c | 23 ++++++++++++++++++----- > arch/x86/kernel/time.c | 12 ++++++++++-- > arch/x86/kernel/x86_init.c | 1 + > arch/x86/xen/enlighten_pv.c | 1 + > 6 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h > index 3df5d195d647..314a5b54a485 100644 > --- a/arch/x86/include/asm/apic.h > +++ b/arch/x86/include/asm/apic.h > @@ -138,6 +138,7 @@ extern void disable_local_APIC(void); > extern void lapic_shutdown(void); > extern void sync_Arb_IDs(void); > extern void init_bsp_APIC(void); > +extern void apic_intr_mode_select(void); > extern void apic_intr_mode_init(void); > extern void setup_local_APIC(void); > extern void init_apic_mappings(void); > @@ -185,6 +186,7 @@ static inline void disable_local_APIC(void) { } > # define setup_boot_APIC_clock x86_init_noop > # define setup_secondary_APIC_clock x86_init_noop > static inline void lapic_update_tsc_freq(void) { } > +static inline void apic_intr_mode_select(void) { } > static inline void apic_intr_mode_init(void) { } > static inline void lapic_assign_system_vectors(void) { } > static inline void lapic_assign_legacy_vector(unsigned int i, bool r) { } > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h > index aa4747569e23..2803cef02c14 100644 > --- a/arch/x86/include/asm/x86_init.h > +++ b/arch/x86/include/asm/x86_init.h > @@ -51,12 +51,14 @@ struct x86_init_resources { > * are set up. > * @intr_init: interrupt init code > * @trap_init: platform specific trap setup > + * @intr_mode_select: interrupt delivery mode selection > * @intr_mode_init: interrupt delivery mode setup > */ > struct x86_init_irqs { > void (*pre_vector_init)(void); > void (*intr_init)(void); > void (*trap_init)(void); > + void (*intr_mode_select)(void); > void (*intr_mode_init)(void); > }; > > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index f15084536abe..d0cd6722c180 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -805,8 +805,17 @@ bool __init apic_needs_pit(void) > if (!tsc_khz || !cpu_khz) > return true; > > - /* Is there an APIC at all? */ > - if (!boot_cpu_has(X86_FEATURE_APIC)) > + /* Is there an APIC at all or is it disabled? */ > + if (!boot_cpu_has(X86_FEATURE_APIC) || disable_apic) > + return true; > + > + /* > + * If interrupt delivery mode is legacy PIC or virtual wire without > + * configuration, the local APIC timer wont be set up. Make sure > + * that the PIT is initialized. > + */ > + if (apic_intr_mode == APIC_PIC || > + apic_intr_mode == APIC_VIRTUAL_WIRE_NO_CONFIG) > return true; > > /* Deadline timer is based on TSC so no further PIT action required */ > @@ -1293,7 +1302,7 @@ void __init sync_Arb_IDs(void) > > enum apic_intr_mode_id apic_intr_mode; > > -static int __init apic_intr_mode_select(void) > +static int __init __apic_intr_mode_select(void) > { > /* Check kernel option */ > if (disable_apic) { > @@ -1355,6 +1364,12 @@ static int __init apic_intr_mode_select(void) > return APIC_SYMMETRIC_IO; > } > > +/* Select the interrupt delivery mode for the BSP */ > +void __init apic_intr_mode_select(void) > +{ > + apic_intr_mode = __apic_intr_mode_select(); > +} > + > /* > * An initial setup of the virtual wire mode. > */ > @@ -1409,8 +1424,6 @@ void __init apic_intr_mode_init(void) > { > bool upmode = IS_ENABLED(CONFIG_UP_LATE_INIT); > > - apic_intr_mode = apic_intr_mode_select(); > - > switch (apic_intr_mode) { > case APIC_PIC: > pr_info("APIC: Keep in PIC mode(8259)\n"); > diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c > index 5339ffd488d0..fd6deda443f9 100644 > --- a/arch/x86/kernel/time.c > +++ b/arch/x86/kernel/time.c > @@ -88,10 +88,18 @@ void __init hpet_time_init(void) > > static __init void x86_late_time_init(void) > { > + /* > + * Before PIT/HPET init, select the interrupt mode. This is required > + * to make the decision whether PIT should be initialized correct. > + */ > + x86_init.irqs.intr_mode_select(); > + > + /* Setup the legacy timers */ > x86_init.timers.timer_init(); > + > /* > - * After PIT/HPET timers init, select and setup > - * the final interrupt mode for delivering IRQs. > + * After PIT/HPET timers init, set up the final interrupt mode for > + * delivering IRQs. > */ > x86_init.irqs.intr_mode_init(); > tsc_init(); > diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c > index 1151ccd72ce9..6c9adc14df26 100644 > --- a/arch/x86/kernel/x86_init.c > +++ b/arch/x86/kernel/x86_init.c > @@ -57,6 +57,7 @@ struct x86_init_ops x86_init __initdata = { > .pre_vector_init = init_ISA_irqs, > .intr_init = native_init_IRQ, > .trap_init = x86_init_noop, > + .intr_mode_select = apic_intr_mode_select, > .intr_mode_init = apic_intr_mode_init > }, > > diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c > index 88c6d9d7e284..c2556f32f156 100644 > --- a/arch/x86/xen/enlighten_pv.c > +++ b/arch/x86/xen/enlighten_pv.c > @@ -1246,6 +1246,7 @@ asmlinkage __visible void __init xen_start_kernel(void) > x86_platform.get_nmi_reason = xen_get_nmi_reason; > > x86_init.resources.memory_setup = xen_memory_setup; > + x86_init.irqs.intr_mode_select = x86_init_noop; > x86_init.irqs.intr_mode_init = x86_init_noop; > x86_init.oem.arch_setup = xen_arch_setup; > x86_init.oem.banner = xen_banner; > -- > 2.27.0.rc0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h index 3df5d195d647..314a5b54a485 100644 --- a/arch/x86/include/asm/apic.h +++ b/arch/x86/include/asm/apic.h @@ -138,6 +138,7 @@ extern void disable_local_APIC(void); extern void lapic_shutdown(void); extern void sync_Arb_IDs(void); extern void init_bsp_APIC(void); +extern void apic_intr_mode_select(void); extern void apic_intr_mode_init(void); extern void setup_local_APIC(void); extern void init_apic_mappings(void); @@ -185,6 +186,7 @@ static inline void disable_local_APIC(void) { } # define setup_boot_APIC_clock x86_init_noop # define setup_secondary_APIC_clock x86_init_noop static inline void lapic_update_tsc_freq(void) { } +static inline void apic_intr_mode_select(void) { } static inline void apic_intr_mode_init(void) { } static inline void lapic_assign_system_vectors(void) { } static inline void lapic_assign_legacy_vector(unsigned int i, bool r) { } diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h index aa4747569e23..2803cef02c14 100644 --- a/arch/x86/include/asm/x86_init.h +++ b/arch/x86/include/asm/x86_init.h @@ -51,12 +51,14 @@ struct x86_init_resources { * are set up. * @intr_init: interrupt init code * @trap_init: platform specific trap setup + * @intr_mode_select: interrupt delivery mode selection * @intr_mode_init: interrupt delivery mode setup */ struct x86_init_irqs { void (*pre_vector_init)(void); void (*intr_init)(void); void (*trap_init)(void); + void (*intr_mode_select)(void); void (*intr_mode_init)(void); }; diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index f15084536abe..d0cd6722c180 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -805,8 +805,17 @@ bool __init apic_needs_pit(void) if (!tsc_khz || !cpu_khz) return true; - /* Is there an APIC at all? */ - if (!boot_cpu_has(X86_FEATURE_APIC)) + /* Is there an APIC at all or is it disabled? */ + if (!boot_cpu_has(X86_FEATURE_APIC) || disable_apic) + return true; + + /* + * If interrupt delivery mode is legacy PIC or virtual wire without + * configuration, the local APIC timer wont be set up. Make sure + * that the PIT is initialized. + */ + if (apic_intr_mode == APIC_PIC || + apic_intr_mode == APIC_VIRTUAL_WIRE_NO_CONFIG) return true; /* Deadline timer is based on TSC so no further PIT action required */ @@ -1293,7 +1302,7 @@ void __init sync_Arb_IDs(void) enum apic_intr_mode_id apic_intr_mode; -static int __init apic_intr_mode_select(void) +static int __init __apic_intr_mode_select(void) { /* Check kernel option */ if (disable_apic) { @@ -1355,6 +1364,12 @@ static int __init apic_intr_mode_select(void) return APIC_SYMMETRIC_IO; } +/* Select the interrupt delivery mode for the BSP */ +void __init apic_intr_mode_select(void) +{ + apic_intr_mode = __apic_intr_mode_select(); +} + /* * An initial setup of the virtual wire mode. */ @@ -1409,8 +1424,6 @@ void __init apic_intr_mode_init(void) { bool upmode = IS_ENABLED(CONFIG_UP_LATE_INIT); - apic_intr_mode = apic_intr_mode_select(); - switch (apic_intr_mode) { case APIC_PIC: pr_info("APIC: Keep in PIC mode(8259)\n"); diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c index 5339ffd488d0..fd6deda443f9 100644 --- a/arch/x86/kernel/time.c +++ b/arch/x86/kernel/time.c @@ -88,10 +88,18 @@ void __init hpet_time_init(void) static __init void x86_late_time_init(void) { + /* + * Before PIT/HPET init, select the interrupt mode. This is required + * to make the decision whether PIT should be initialized correct. + */ + x86_init.irqs.intr_mode_select(); + + /* Setup the legacy timers */ x86_init.timers.timer_init(); + /* - * After PIT/HPET timers init, select and setup - * the final interrupt mode for delivering IRQs. + * After PIT/HPET timers init, set up the final interrupt mode for + * delivering IRQs. */ x86_init.irqs.intr_mode_init(); tsc_init(); diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c index 1151ccd72ce9..6c9adc14df26 100644 --- a/arch/x86/kernel/x86_init.c +++ b/arch/x86/kernel/x86_init.c @@ -57,6 +57,7 @@ struct x86_init_ops x86_init __initdata = { .pre_vector_init = init_ISA_irqs, .intr_init = native_init_IRQ, .trap_init = x86_init_noop, + .intr_mode_select = apic_intr_mode_select, .intr_mode_init = apic_intr_mode_init }, diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 88c6d9d7e284..c2556f32f156 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -1246,6 +1246,7 @@ asmlinkage __visible void __init xen_start_kernel(void) x86_platform.get_nmi_reason = xen_get_nmi_reason; x86_init.resources.memory_setup = xen_memory_setup; + x86_init.irqs.intr_mode_select = x86_init_noop; x86_init.irqs.intr_mode_init = x86_init_noop; x86_init.oem.arch_setup = xen_arch_setup; x86_init.oem.banner = xen_banner;