Re: [PATCH v4 5/5] KVM: SVM: Enable Secure TSC for SNP guests

[Sean Christopherson <seanjc@google.com>]

On Tue, Mar 11, 2025, Tom Lendacky wrote:
> On 3/11/25 06:05, Nikunj A. Dadhania wrote:
> > On 3/11/2025 2:41 PM, Nikunj A. Dadhania wrote:
> >> On 3/10/2025 9:09 PM, Tom Lendacky wrote:
> >>> On 3/10/25 01:45, Nikunj A Dadhania wrote:
> >>
> >>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> >>>> index 50263b473f95..b61d6bd75b37 100644
> >>>> --- a/arch/x86/kvm/svm/sev.c
> >>>> +++ b/arch/x86/kvm/svm/sev.c
> >>>> @@ -2205,6 +2205,20 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >>>>  
> >>>>  	start.gctx_paddr = __psp_pa(sev->snp_context);
> >>>>  	start.policy = params.policy;
> >>>> +
> >>>> +	if (snp_secure_tsc_enabled(kvm)) {
> >>>> +		u32 user_tsc_khz = params.desired_tsc_khz;
> >>>> +
> >>>> +		/* Use tsc_khz if the VMM has not provided the TSC frequency */
> >>>> +		if (!user_tsc_khz)
> >>>> +			user_tsc_khz = tsc_khz;
> >>>> +
> >>>> +		start.desired_tsc_khz = user_tsc_khz;

The code just below this clobbers kvm->arch.default_tsc_khz, which could already
have been set by userspace.  Why?  Either require params.desired_tsc_khz to match
kvm->arch.default_tsc_khz, or have KVM's ABI be that KVM stuffs desired_tsc_khz
based on kvm->arch.default_tsc_khz.  I don't see any reason to add yet another
way to control TSC.

> >>> Do we need to perform any sanity checking against this value?
> >>
> >> On the higher side, sev-snp-guest.stsc-freq is u32, a Secure TSC guest boots fine with
> >> TSC frequency set to the highest value (stsc-freq=0xFFFFFFFF).
> >>
> >> On the lower side as MSR_AMD64_GUEST_TSC_FREQ is in MHz, TSC clock should at least be 1Mhz. 
> > 
> > Something like this ?
> > 
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index b61d6bd75b37..c46b6afa969d 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2213,6 +2213,14 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >  		if (!user_tsc_khz)
> >  			user_tsc_khz = tsc_khz;
> >  
> > +		/*
> > +		 * The minimum granularity for reporting Secure TSC frequency is
> > +		 * 1MHz. Return an error if the user specifies a TSC frequency
> > +		 * less than 1MHz.
> > +		 */
> > +		if (user_tsc_khz < 1000)
> > +			return -EINVAL;
> 
> Seems reasonable to me. I'll let Sean or Paolo weigh in on it. I don't
> think we need a message, there should be a check in the VMM, too, which
> would be able to provide information to the end user?

Why bother?  Userspace can DoS the guest anytime it wants.  A TSC frequency of
1MHz on a modern CPU is absolutely absurd.  Making that the minimum is is likely
going to do nothing but sow confusion.