linux-kernel - Re: [RFC PATCH v3 11/21] KVM: x86: Simplify and comment kvm_get_time

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: <Zr4ixaBGnk_6Zqef@google.com>
Date: Thu, 15 Aug 2024 08:46:13 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: David Woodhouse <dwmw2@...radead.org>
Cc: kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>, 
	Jonathan Corbet <corbet@....net>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Paul Durrant <paul@....org>, Peter Zijlstra <peterz@...radead.org>, 
	Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt <rostedt@...dmis.org>, 
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Daniel Bristot de Oliveira <bristot@...hat.com>, Valentin Schneider <vschneid@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	jalliste@...zon.co.uk, sveith@...zon.de, zide.chen@...el.com, 
	Dongli Zhang <dongli.zhang@...cle.com>, Chenyi Qiang <chenyi.qiang@...el.com>
Subject: Re: [RFC PATCH v3 11/21] KVM: x86: Simplify and comment kvm_get_time_scale()

On Wed, May 22, 2024, David Woodhouse wrote:
> From: David Woodhouse <dwmw@...zon.co.uk>
> 
> Commit 3ae13faac400 ("KVM: x86: pass kvm_get_time_scale arguments in hertz")
> made this function take 64-bit values in Hz rather than 32-bit kHz. Thus
> making it entrely pointless to shadow its arguments into local 64-bit
> variables. Just use scaled_hz and base_hz directly.
> 
> Also rename the 'tps32' variable to 'base32', having utterly failed to
> think of any reason why it might have been called that in the first place.

Ticks Per Second?

> +	/*
> +	 * Start by shifting the base_hz right until it fits in 32 bits, and
> +	 * is lower than double the target rate. This introduces a negative
> +	 * shift value which would result in pvclock_scale_delta() shifting
> +	 * the actual tick count right before performing the multiplication.
> +	 */
> +	while (base_hz > scaled_hz*2 || base_hz & 0xffffffff00000000ULL) {
> +		base_hz >>= 1;
>  		shift--;
>  	}
>  
> -	tps32 = (uint32_t)tps64;
> -	while (tps32 <= scaled64 || scaled64 & 0xffffffff00000000ULL) {
> -		if (scaled64 & 0xffffffff00000000ULL || tps32 & 0x80000000)
> -			scaled64 >>= 1;
> +	/* Now the shifted base_hz fits in 32 bits, copy it to base32 */
> +	base32 = (uint32_t)base_hz;
> +
> +	/*
> +	 * Next, shift the scaled_hz right until it fits in 32 bits, and ensure
> +	 * that the shifted base_hz is not larger (so that the result of the
> +	 * final division also fits in 32 bits).
> +	 */
> +	while (base32 <= scaled_hz || scaled_hz & 0xffffffff00000000ULL) {
> +		if (scaled_hz & 0xffffffff00000000ULL || base32 & 0x80000000)
> +			scaled_hz >>= 1;
>  		else
> -			tps32 <<= 1;
> +			base32 <<= 1;
>  		shift++;
>  	}

Any chance you'd want to do this on top, so that it's easier to see that the
loops are waiting for the upper bits to go to zero?  And so that readers don't
have to count effs and zeros :-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d30b12986e17..786d5a855459 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2417,7 +2417,7 @@ static void kvm_get_time_scale(uint64_t scaled_hz, uint64_t base_hz,
         * shift value which would result in pvclock_scale_delta() shifting
         * the actual tick count right before performing the multiplication.
         */
-       while (base_hz > scaled_hz*2 || base_hz & 0xffffffff00000000ULL) {
+       while (base_hz > scaled_hz*2 || base_hz >> 32) {
                base_hz >>= 1;
                shift--;
        }
@@ -2430,8 +2430,8 @@ static void kvm_get_time_scale(uint64_t scaled_hz, uint64_t base_hz,
         * that the shifted base_hz is not larger (so that the result of the
         * final division also fits in 32 bits).
         */
-       while (base32 <= scaled_hz || scaled_hz & 0xffffffff00000000ULL) {
-               if (scaled_hz & 0xffffffff00000000ULL || base32 & 0x80000000)
+       while (base32 <= scaled_hz || scaled_hz >> 32) {
+               if (scaled_hz >> 32 || base32 & BIT(31))
                        scaled_hz >>= 1;
                else
                        base32 <<= 1;