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

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

On 1/16/2025 3:07 AM, Sean Christopherson wrote:

> My strong vote is prefer TSC over kvmclock for sched_clock if the TSC is constant,
> nonstop, and not marked stable via command line.  I.e. use the same criteria as
> tweaking the clocksource rating.  As above, sched_clock is more tolerant of slop
> than clocksource, so it's a bit ridiculous to care whether the TSC or kvmclock
> (or something else entirely) is used for the clocksource.
> 
> If we wanted to go with a more conservative approach, e.g. to minimize the risk
> of breaking existing setups, we could also condition the change on the TSC being
> reliable and having a known frequency.  I.e. require SNP's Secure TSC, or require
> the hypervisor to enumerate the TSC frequency via CPUID.  I don't see a ton of
> value in that approach though, and long-term it would lead to some truly weird
> code due to holding sched_clock to a higher standard than clocksource.
> 
> But wait, there's more!  Because TDX doesn't override .calibrate_tsc() or
> .calibrate_cpu(), even though TDX provides a trusted TSC *and* enumerates the
> frequency of the TSC, unless I'm missing something, tsc_early_init() will compute
> the TSC frequency using the information provided by KVM, i.e. the untrusted host.
> 
> The "obvious" solution is to leave the calibration functions as-is if the TSC has
> a known, reliable frequency, but even _that_ is riddled with problems, because
> as-is, the kernel sets TSC_KNOWN_FREQ and TSC_RELIABLE in tsc_early_init(), which
> runs *after* init_hypervisor_platform().  SNP Secure TSC fudges around this by
> overiding the calibration routines, but that's a bit gross and easy to fix if we
> also fix TDX.  And fixing TDX by running native_calibrate_tsc() would give the
> same love to setups where the hypervisor provides CPUID 0x15 and/or 0x16.

One change that I wasn't sure was about non-Intel guests using CPUID 0x15H/0x16H,
that you have already answered in the subsequent emails. At present, AMD platform
does not implement either of them and will bail out from the below check:

        /* CPUID 15H TSC/Crystal ratio, plus optionally Crystal Hz */
        cpuid(CPUID_LEAF_TSC, &eax_denominator, &ebx_numerator, &ecx_hz, &edx);

        if (ebx_numerator == 0 || eax_denominator == 0)
                return 0;

> All in all, I'm thinking something like this (across multiple patches):

Tested on AMD Milan and changes are working as intended:

For SecureTSC guests with TSC_INVARIANT set:
* Raw TSC is used as clocksource and sched-clock
* Calibration is done using GUEST_TSC_FREQ MSR

For non-SecureTSC guests with TSC_INVARIANT set:
* Raw TSC is used as clocksource and sched-clock
* Calibration is done using kvm-clock

For non-SecureTSC guests without TSC_INVARIANT:
* kvm-clock(based on TSC) is used as clocksource and sched-clock
* Calibration is done using kvm-clock

> 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)

Regards
Nikunj