linux-kernel - RE: [PATCH] clockevents/drivers/i8253: Do not zero timer counter in shutdown

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BYAPR21MB168840B3814336ED510845C0D7D89@BYAPR21MB1688.namprd21.prod.outlook.com>
Date:   Wed, 8 Feb 2023 01:04:19 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     "lirongqing@...du.com" <lirongqing@...du.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] clockevents/drivers/i8253: Do not zero timer counter in
 shutdown

From: lirongqing@...du.com <lirongqing@...du.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@...rosoft.com>

> 
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Li RongQing <lirongqing@...du.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