Re: [PATCH v16 12/13] x86/tsc: Switch to native sched clock

[Nikunj A Dadhania <nikunj@amd.com>]

Sean Christopherson <seanjc@google.com> writes:

>
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 0864b314c26a..9baffb425386 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -663,7 +663,12 @@ unsigned long native_calibrate_tsc(void)
>  	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;
>  	unsigned int crystal_khz;
>  
> -	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> +	/*
> +	 * Ignore the vendor when running as a VM, if the hypervisor provides
> +	 * garbage CPUID information then the vendor is also suspect.
> +	 */
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL &&
> +	    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
>  		return 0;
>  
>  	if (boot_cpu_data.cpuid_level < 0x15)
> @@ -713,10 +718,13 @@ unsigned long native_calibrate_tsc(void)
>  		return 0;
>  
>  	/*
> -	 * For Atom SoCs TSC is the only reliable clocksource.
> -	 * Mark TSC reliable so no watchdog on it.
> +	 * For Atom SoCs TSC is the only reliable clocksource.  Similarly, in a
> +	 * VM, any watchdog is going to be less reliable than the TSC as the
> +	 * watchdog source will be emulated in software.  In both cases, mark
> +	 * the TSC reliable so that no watchdog runs on it.
>  	 */
> -	if (boot_cpu_data.x86_vfm == INTEL_ATOM_GOLDMONT)
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR) ||
> +	    boot_cpu_data.x86_vfm == INTEL_ATOM_GOLDMONT)
>  		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);

One more point here is for AMD guests, TSC will not be marked reliable
as per the above change, it will only be effective for CPUs supporting
CPUID 15H/16H. Although, the watchdog should be stopped for AMD guests
as well.

Will it make sense to move this before cpuid_level check ?

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index e7abcc4d02c3..2769d1598c0d 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -672,6 +672,14 @@ unsigned long native_calibrate_tsc(void)
 	    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
 		return 0;
 
+	/*
+	 * In a VM, any watchdog is going to be less reliable than the TSC as
+	 * the watchdog source will be emulated in software. Mark the TSC
+	 * reliable so that no watchdog runs on it.
+	 */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
+
 	if (boot_cpu_data.cpuid_level < CPUID_LEAF_TSC)
 		return 0;
 
@@ -719,13 +727,10 @@ unsigned long native_calibrate_tsc(void)
 		return 0;
 
 	/*
-	 * For Atom SoCs TSC is the only reliable clocksource.  Similarly, in a
-	 * VM, any watchdog is going to be less reliable than the TSC as the
-	 * watchdog source will be emulated in software.  In both cases, mark
-	 * the TSC reliable so that no watchdog runs on it.
+	 * For Atom SoCs TSC is the only reliable clocksource.
+	 * Mark TSC reliable so no watchdog on it.
 	 */
-	if (boot_cpu_has(X86_FEATURE_HYPERVISOR) ||
-	    boot_cpu_data.x86_vfm == INTEL_ATOM_GOLDMONT)
+	if (boot_cpu_data.x86_vfm == INTEL_ATOM_GOLDMONT)
 		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
 
 #ifdef CONFIG_X86_LOCAL_APIC

Regards
Nikunj