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