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

[Sean Christopherson <seanjc@google.com>]

On Wed, Jan 08, 2025, Nikunj A. Dadhania wrote:
> 
> On 1/8/2025 2:04 PM, Nikunj A. Dadhania wrote:
> > 
> >> If you want to take care only of STSC now, I'd take a patch which is known
> >> good and tested properly. And that should happen very soon because the merge
> >> window is closing in. 
> > 
> > In that case, let me limit this only to STSC for now, i will send updated patch.
> 
> From: Nikunj A Dadhania <nikunj@amd.com>
> Date: Wed, 8 Jan 2025 14:18:04 +0530
> Subject: [PATCH] x86/kvmclock: Prefer TSC as the scheduler clock for Secure
>  TSC guests
> 
> Although the kernel switches over to a stable TSC clocksource instead of
> kvmclock, the scheduler still keeps on using kvmclock as the sched clock.
> This is due to kvm_sched_clock_init() updating the pv_sched_clock()
> unconditionally. Do not override the PV sched clock when Secure TSC is
> enabled.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>  arch/x86/kernel/kvmclock.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index d8fef3a65a35..82c4743a5e7a 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -324,8 +324,10 @@ void __init kvmclock_init(void)
>  	if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE_STABLE_BIT))
>  		pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>  
> -	flags = pvclock_read_flags(&hv_clock_boot[0].pvti);
> -	kvm_sched_clock_init(flags & PVCLOCK_TSC_STABLE_BIT);
> +	if (!cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC)) {
> +		flags = pvclock_read_flags(&hv_clock_boot[0].pvti);
> +		kvm_sched_clock_init(flags & PVCLOCK_TSC_STABLE_BIT);
> +	}

This still misses my point.  Ditto for the "x86/tsc: Switch Secure TSC guests away
from kvm-clock".

I object to singling out kvmclock.  It's weird and misleading, because handling
only kvmclock suggests that other PV clocks are somehow trusted/ok, when in
reality the only reason kvmclock is getting singled out is (presumably) because
it's what Nikunj and the other folks enabling KVM SNP test on.

What I care most about is having a sane, consistent policy throughout the kernel.
E.g. so that a user/reader walks away with an understanding PV clocks are a
theoretical host attack vector and so should be avoided when Secure TSC is
available.

Ideally, if the TSC is the preferred clocksource, then the scheduler will use the
TSC and not a PV clock irrespective of STSC.  But I 100% agree with Boris that
it needs buy-in from other maintainers (including Paolo), because it's entirely
possible (likely, even) that there's an angle to scheduling I'm not considering.