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
