Message-ID: <ce49cc3e3ae5885d992261589cd0f4adad118776.camel@redhat.com>
Subject: Re: Re: [PATCH v2 2/2] KVM: x86: use x86_get_freq to get freq for
 kvmclock
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     zhenwei pi <pizhenwei@bytedance.com>,
        Thomas Gleixner <tglx@linutronix.de>, pbonzini@redhat.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org
Date:   Fri, 03 Dec 2021 09:53:53 +0200
In-Reply-To: <20211202224555.GE16608@worktop.programming.kicks-ass.net>
References: <20211201024650.88254-1-pizhenwei@bytedance.com>
         <20211201024650.88254-3-pizhenwei@bytedance.com> <877dcn7md2.ffs@tglx>
         <b37ffc3d-4038-fc5e-d681-b89c04a37b04@bytedance.com>
         <ffbb8a16f267e73316084d1252696edaf81e35a9.camel@redhat.com>
         <20211202224555.GE16608@worktop.programming.kicks-ass.net>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2021-12-02 at 23:45 +0100, Peter Zijlstra wrote:
> On Thu, Dec 02, 2021 at 09:19:25AM +0200, Maxim Levitsky wrote:
> > On Thu, 2021-12-02 at 13:26 +0800, zhenwei pi wrote:
> > Note that on my Zen2 machine (3970X), aperf/mperf returns current cpu freqency,
> 
> Correct, and it computes it over a random period of history. IOW, it's a
> random number generator.
> 
> > 1. It sucks that on AMD, the TSC frequency is calibrated from other 
> > clocksources like PIT/HPET, since the result is not exact and varies
> > from boot to boot.
> 
> CPUID.15h is supposed to tell us the actual frequency; except even Intel
> find it very hard to actually put the right (or any, really) number in
> there :/ Bribe your friendly AMD engineer with beers or something.

That what I thought. I asked just in case maybe AMD does have some vendor specific msrs
you know about but we didn't bother to support it.
I didn't find any in their PRM.

> 
> > 2. In the guest on AMD, we mark the TSC as unsynchronized always due to the code
> > in unsynchronized_tsc, unless invariant tsc is used in guest cpuid,
> > which is IMHO not fair to AMD as we don't do this for  Intel cpus.
> > (look at unsynchronized_tsc function)
> 
> Possibly we could treat >= Zen similar to Intel there. Also that comment
> there is hillarious, it talks about multi-socket and then tests
> num_possible_cpus(). Clearly that code hasn't been touched in like
> forever.
Thank you!

> 
> > 3. I wish the kernel would export the tsc frequency it found to userspace
> > somewhere in /sys or /proc, as this would be very useful for userspace applications.
> > Currently it can only be found in dmesg if I am not mistaken..
> > I don't mind if such frequency would only be exported if the TSC is stable,
> > always running, not affected by CPUfreq, etc.
> 
> Perf exposes it, it's not really convenient if you're not using perf,
> but it can be found there.
That is good to know! I will check out the source but if you remember,
is there cli option in perf to show it, or it only uses it for internal
purposes?

> 
> 
> ---
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 2e076a459a0c..09da2935534a 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -29,6 +29,7 @@
>  #include <asm/intel-family.h>
>  #include <asm/i8259.h>
>  #include <asm/uv/uv.h>
> +#include <asm/topology.h>
>  
>  unsigned int __read_mostly cpu_khz;	/* TSC clocks / usec, not used here */
>  EXPORT_SYMBOL(cpu_khz);
> @@ -1221,9 +1222,20 @@ int unsynchronized_tsc(void)
>  	 * Intel systems are normally all synchronized.
>  	 * Exceptions must mark TSC as unstable:
>  	 */
> -	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
> +	switch (boot_cpu_data.x86_vendor) {
> +	case X86_VENDOR_INTEL:
> +		/* Really only Core and later */
> +		break;
> +
> +	case X86_VENDOR_AMD:
> +	case X86_VENDOR_HYGON:
> +		if (boot_cpu_data.x86 >= 0x17) /* >= Zen */
> +			break;
> +		fallthrough;
> +
> +	default:
>  		/* assume multi socket systems are not synchronized: */
> -		if (num_possible_cpus() > 1)
> +		if (topology_max_packages() > 1)
>  			return 1;
>  	}
>  

This makes sense!

> 


Best regards,
	Maxim Levitsky