clockevents/drivers/i8253: Do not zero timer counter in shutdown

  From: Li RongQing <lirongqing@baidu.com>

Zeroing the counter register in pit_shutdown() isn't actually supposed to
stop it from counting,  will causes the PIT to start running again,
From the spec:

  The largest possible initial count is 0; this is equivalent to 216 for
  binary counting and 104 for BCD counting.

  The Counter does not stop when it reaches zero. In Modes 0, 1, 4, and 5 the
  Counter "wraps around" to the highest count, either FFFF hex for binary
  count- ing or 9999 for BCD counting, and continues counting.

  Mode 0 is typically used for event counting. After the Control Word is
  written, OUT is initially low, and will remain low until the Counter
  reaches zero. OUT then goes high and remains high until a new count or a
  new Mode 0 Control Word is written into the Counter.

Hyper-V and KVM follow the spec, the issue that 35b69a42 "(clockevents/drivers/
i8253: Add support for PIT shutdown quirk") fixed is in i8253 drivers, not Hyper-v,
so delete the zero timer counter register in shutdown, and delete PIT shutdown
quirk for Hyper-v

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 arch/x86/kernel/cpu/mshyperv.c | 11 -----------
 drivers/clocksource/i8253.c    | 12 ------------
 include/linux/i8253.h          |  1 -
 3 files changed, 24 deletions(-)
  

From: lirongqing@baidu.com <lirongqing@baidu.com> Sent: Monday, February 6, 2023 5:15 PM
> 
> Zeroing the counter register in pit_shutdown() isn't actually supposed to
> stop it from counting,  will causes the PIT to start running again,
> From the spec:
> 
>   The largest possible initial count is 0; this is equivalent to 216 for
>   binary counting and 104 for BCD counting.
> 
>   The Counter does not stop when it reaches zero. In Modes 0, 1, 4, and 5 the
>   Counter "wraps around" to the highest count, either FFFF hex for binary
>   count- ing or 9999 for BCD counting, and continues counting.
> 
>   Mode 0 is typically used for event counting. After the Control Word is
>   written, OUT is initially low, and will remain low until the Counter
>   reaches zero. OUT then goes high and remains high until a new count or a
>   new Mode 0 Control Word is written into the Counter.
> 
> Hyper-V and KVM follow the spec, the issue that 35b69a42 "(clockevents/drivers/
> i8253: Add support for PIT shutdown quirk") fixed is in i8253 drivers, not Hyper-v,
> so delete the zero timer counter register in shutdown, and delete PIT shutdown
> quirk for Hyper-v

From the standpoint of Hyper-V, I'm good with this change.  But there's a
risk that old hardware might not be compliant with the spec, and needs the
zero'ing for some reason. The experts in the x86 space will be in the best
position to assess the risk.  At the time, the quirk approach was taken so
the change applied only to Hyper-V, and any such risk was avoided.

For Hyper-V,
Reviewed-by: Michael Kelley <mikelley@microsoft.com>

> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kernel/cpu/mshyperv.c | 11 -----------
>  drivers/clocksource/i8253.c    | 12 ------------
>  include/linux/i8253.h          |  1 -
>  3 files changed, 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 46668e2..f788889 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -16,7 +16,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/kexec.h>
> -#include <linux/i8253.h>
>  #include <linux/random.h>
>  #include <linux/swiotlb.h>
>  #include <asm/processor.h>
> @@ -399,16 +398,6 @@ static void __init ms_hyperv_init_platform(void)
>  	if (efi_enabled(EFI_BOOT))
>  		x86_platform.get_nmi_reason = hv_get_nmi_reason;
> 
> -	/*
> -	 * Hyper-V VMs have a PIT emulation quirk such that zeroing the
> -	 * counter register during PIT shutdown restarts the PIT. So it
> -	 * continues to interrupt @18.2 HZ. Setting i8253_clear_counter
> -	 * to false tells pit_shutdown() not to zero the counter so that
> -	 * the PIT really is shutdown. Generation 2 VMs don't have a PIT,
> -	 * and setting this value has no effect.
> -	 */
> -	i8253_clear_counter_on_shutdown = false;
> -
>  #if IS_ENABLED(CONFIG_HYPERV)
>  	/*
>  	 * Setup the hook to get control post apic initialization.
> diff --git a/drivers/clocksource/i8253.c b/drivers/clocksource/i8253.c
> index d4350bb..169474d 100644
> --- a/drivers/clocksource/i8253.c
> +++ b/drivers/clocksource/i8253.c
> @@ -20,13 +20,6 @@
>  DEFINE_RAW_SPINLOCK(i8253_lock);
>  EXPORT_SYMBOL(i8253_lock);
> 
> -/*
> - * Handle PIT quirk in pit_shutdown() where zeroing the counter register
> - * restarts the PIT, negating the shutdown. On platforms with the quirk,
> - * platform specific code can set this to false.
> - */
> -bool i8253_clear_counter_on_shutdown __ro_after_init = true;
> -
>  #ifdef CONFIG_CLKSRC_I8253
>  /*
>   * Since the PIT overflows every tick, its not very useful
> @@ -117,11 +110,6 @@ static int pit_shutdown(struct clock_event_device *evt)
> 
>  	outb_p(0x30, PIT_MODE);
> 
> -	if (i8253_clear_counter_on_shutdown) {
> -		outb_p(0, PIT_CH0);
> -		outb_p(0, PIT_CH0);
> -	}
> -
>  	raw_spin_unlock(&i8253_lock);
>  	return 0;
>  }
> diff --git a/include/linux/i8253.h b/include/linux/i8253.h
> index 8336b2f..e6bb36a 100644
> --- a/include/linux/i8253.h
> +++ b/include/linux/i8253.h
> @@ -21,7 +21,6 @@
>  #define PIT_LATCH	((PIT_TICK_RATE + HZ/2) / HZ)
> 
>  extern raw_spinlock_t i8253_lock;
> -extern bool i8253_clear_counter_on_shutdown;
>  extern struct clock_event_device i8253_clockevent;
>  extern void clockevent_i8253_init(bool oneshot);
> 
> --
> 2.9.4
  

On Wed, Feb 08, 2023, Michael Kelley (LINUX) wrote:
> From: lirongqing@baidu.com <lirongqing@baidu.com> Sent: Monday, February 6, 2023 5:15 PM
> > 
> > Zeroing the counter register in pit_shutdown() isn't actually supposed to
> > stop it from counting,  will causes the PIT to start running again,
> > From the spec:
> > 
> >   The largest possible initial count is 0; this is equivalent to 216 for
> >   binary counting and 104 for BCD counting.
> > 
> >   The Counter does not stop when it reaches zero. In Modes 0, 1, 4, and 5 the
> >   Counter "wraps around" to the highest count, either FFFF hex for binary
> >   count- ing or 9999 for BCD counting, and continues counting.
> > 
> >   Mode 0 is typically used for event counting. After the Control Word is
> >   written, OUT is initially low, and will remain low until the Counter
> >   reaches zero. OUT then goes high and remains high until a new count or a
> >   new Mode 0 Control Word is written into the Counter.
> > 
> > Hyper-V and KVM follow the spec, the issue that 35b69a42 "(clockevents/drivers/
> > i8253: Add support for PIT shutdown quirk") fixed is in i8253 drivers, not Hyper-v,
> > so delete the zero timer counter register in shutdown, and delete PIT shutdown
> > quirk for Hyper-v
> 
> From the standpoint of Hyper-V, I'm good with this change.  But there's a
> risk that old hardware might not be compliant with the spec, and needs the
> zero'ing for some reason. The experts in the x86 space will be in the best
> position to assess the risk.

Yep, my feeling exactly.  My input is purely from reading those crusty old specs.
  

> > > Hyper-V and KVM follow the spec, the issue that 35b69a42
> > > "(clockevents/drivers/
> > > i8253: Add support for PIT shutdown quirk") fixed is in i8253
> > > drivers, not Hyper-v, so delete the zero timer counter register in
> > > shutdown, and delete PIT shutdown quirk for Hyper-v
> >
> > From the standpoint of Hyper-V, I'm good with this change.  But
> > there's a risk that old hardware might not be compliant with the spec,
> > and needs the zero'ing for some reason. The experts in the x86 space
> > will be in the best position to assess the risk.
> 
> Yep, my feeling exactly.  My input is purely from reading those crusty old specs.

Hi Thomas:

Could you give some suggestion about this patch?

Thanks

-Li
  

On Wed, Apr 12, 2023 at 12:02:04PM +0000, Li,Rongqing wrote:
> 
> 
> > -----Original Message-----
> > From: Sean Christopherson <seanjc@google.com>
> > Sent: Wednesday, February 8, 2023 11:04 PM
> > To: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > Cc: Li,Rongqing <lirongqing@baidu.com>; KY Srinivasan <kys@microsoft.com>;
> > Haiyang Zhang <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> > <decui@microsoft.com>; tglx@linutronix.de; mingo@redhat.com;
> > bp@alien8.de; dave.hansen@linux.intel.com; x86@kernel.org;
> > linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] clockevents/drivers/i8253: Do not zero timer counter in
> > shutdown
> > 
> > On Wed, Feb 08, 2023, Michael Kelley (LINUX) wrote:
> > > From: lirongqing@baidu.com <lirongqing@baidu.com> Sent: Monday,
> > > February 6, 2023 5:15 PM
> > > >
> > > > Zeroing the counter register in pit_shutdown() isn't actually
> > > > supposed to stop it from counting,  will causes the PIT to start
> > > > running again, From the spec:
> > > >
> > > >   The largest possible initial count is 0; this is equivalent to 216 for
> > > >   binary counting and 104 for BCD counting.
> > > >
> > > >   The Counter does not stop when it reaches zero. In Modes 0, 1, 4, and 5
> > the
> > > >   Counter "wraps around" to the highest count, either FFFF hex for binary
> > > >   count- ing or 9999 for BCD counting, and continues counting.
> > > >
> > > >   Mode 0 is typically used for event counting. After the Control Word is
> > > >   written, OUT is initially low, and will remain low until the Counter
> > > >   reaches zero. OUT then goes high and remains high until a new count or a
> > > >   new Mode 0 Control Word is written into the Counter.
> > > >
> > > > Hyper-V and KVM follow the spec, the issue that 35b69a42
> > > > "(clockevents/drivers/
> > > > i8253: Add support for PIT shutdown quirk") fixed is in i8253
> > > > drivers, not Hyper-v, so delete the zero timer counter register in
> > > > shutdown, and delete PIT shutdown quirk for Hyper-v
> > >
> > > From the standpoint of Hyper-V, I'm good with this change.  But
> > > there's a risk that old hardware might not be compliant with the spec,
> > > and needs the zero'ing for some reason. The experts in the x86 space
> > > will be in the best position to assess the risk.
> > 
> > Yep, my feeling exactly.  My input is purely from reading those crusty old specs.
> 
> 
> 
> Ping

I guess you want Thomas Gleixner and Daniel Lezcano's attention.

> 
> Thanks
> 
> -Li
  

> >
> >
> > Ping
> 
> I guess you want Thomas Gleixner and Daniel Lezcano's attention.
> 

Yes, 

Thanks you

-Li