ACK: [PATCH 1/1][SRU][B/OEM-B] x86/timer: Don't skip PIT setup when APIC is disabled or in legacy mode
Sultan Alsawaf
sultan.alsawaf at canonical.com
Tue Jun 23 16:58:31 UTC 2020
Acked-by: Sultan Alsawaf <sultan.alsawaf at canonical.com>
On Fri, Jun 12, 2020 at 07:52:00PM +0800, You-Sheng Yang wrote:
> From: Thomas Gleixner <tglx at 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 at gmail.com>
> Tested-by: Anthony Buckley <tony.buckley000 at gmail.com>
> Signed-off-by: Thomas Gleixner <tglx at linutronix.de>
> Signed-off-by: Ingo Molnar <mingo at kernel.org>
> Cc: Daniel Drake <drake at 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 at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list