Re: [PATCH v8 2/2] KVM: SVM: Enable Secure TSC for SNP guests

["Huang, Kai" <kai.huang@intel.com>]

> > 
> > > @@ -2146,6 +2158,14 @@ 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)) {
> > > +		if (!kvm->arch.default_tsc_khz)
> > > +			return -EINVAL;
> > 
> > Here snp_context_create() has been called successfully therefore IIUC you
> > need to use
> > 
> > 		goto e_free_context;
> 
> Ack.
> 
> > 
> > instead.
> > 
> > Btw, IIUC it shouldn't be possible for the kvm->arch.default_tsc_khz to be
> > 0.  Perhaps we can just remove the check.
> 
> I will keep this check and correct the goto.
> 
> > 
> > Even some bug results in the default_tsc_khz being 0, will the
> > SNP_LAUNCH_START command catch this and return error?
> 
> No, that is an invalid configuration, desired_tsc_khz is set to 0 when
> SecureTSC is disabled. If SecureTSC is enabled, desired_tsc_khz should
> have correct value.

So it's an invalid configuration that when Secure TSC is enabled and
desired_tsc_khz is 0.  Assuming the SNP_LAUNCH_START will return an error
if such configuration is used, wouldn't it be simpler if you remove the
above check and depend on the SNP_LAUNCH_START command to catch the
invalid configuration?

Anyway, no problem to me if you have the check.  I just thought w/o check
the code will be simpler and you can still get what you want (supposedly).

> 
> > 
> > > +
> > > +		start.desired_tsc_khz = kvm->arch.default_tsc_khz;
> > > +	}
> > > +
> > >  	memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
> > >  	rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_START, &start, &argp->error);
> > >  	if (rc) {
> > > @@ -2386,7 +2406,9 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >  			return ret;
> > >  		}
> > >  
> > > -		svm->vcpu.arch.guest_state_protected = true;
> > > +		vcpu->arch.guest_state_protected = true;
> > > +		vcpu->arch.guest_tsc_protected = snp_secure_tsc_enabled(kvm);
> > > +
> > 
> > + Xiaoyao.
> > 
> > The KVM_SET_TSC_KHZ can also be a vCPU ioctl (in fact, the support of VM
> > ioctl of it was added later).  I am wondering whether we should reject
> > this vCPU ioctl for TSC protected guests, like:
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2806f7104295..699ca5e74bba 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6186,6 +6186,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >                 u32 user_tsc_khz;
> >  
> >                 r = -EINVAL;
> > +
> > +               if (vcpu->arch.guest_tsc_protected)
> > +                       goto out;
> > +
> >                 user_tsc_khz = (u32)arg;
> >  
> >                 if (kvm_caps.has_tsc_control &&
> 
> Ack, more below.
> 
> > 
> > TDX doesn't do this either, but TDX has its own version for TSC related
> > kvm_x86_ops callbacks:
> > 
> >         .get_l2_tsc_offset = vt_op(get_l2_tsc_offset),                   
> >         .get_l2_tsc_multiplier = vt_op(get_l2_tsc_multiplier),           
> >         .write_tsc_offset = vt_op(write_tsc_offset),                     
> >         .write_tsc_multiplier = vt_op(write_tsc_multiplier),
> > 
> > which basically ignore the operations for TDX guests, so no harm even
> > KVM_SET_TSC_KHZ ioctl is called for vCPU I suppose.
> > 
> > But IIRC, for AMD side they just use default version of SVM guests thus
> > SEV/SNP guests are not ignored:
> > 
> >         .get_l2_tsc_offset = svm_get_l2_tsc_offset,
> >         .get_l2_tsc_multiplier = svm_get_l2_tsc_multiplier,
> >         .write_tsc_offset = svm_write_tsc_offset,
> >         .write_tsc_multiplier = svm_write_tsc_multiplier,
> > 
> > So I am not sure whether there will be problem here.
> 
> For the guest, changing TSC frequency after SNP_LAUNCH_START will not
> matter. As the guest TSC frequency cannot be changed after that.

But will there any side effect if doing so?  E.g., what if
svm_write_tsc_offset()/svm_write_tsc_multiplier() are called for vCPU for
SNP guest?

> 
> > Anyway, conceptually, I think we should just reject the KVM_SET_TSC_KHZ
> > vCPU ioctl for TSC protected guests.
> > 
> > However, it seems for SEV/SNP the setting of guest_state_protected and
> > guest_tsc_protected is done at a rather late time in
> > snp_launch_update_vmsa() as shown in this patch.  This means checking of
> > guest_tsc_protected won't work if KVM_SET_TSC_KHZ is called at earlier
> > time.
> > > TDX sets those two at early time when initializing the VM.  I think the
> > SEV/SNP guests should do the same.
> 
> Setting of guest_state_protected is correct as it is part of
> LAUNCH_UPDATE_VMSA, from this point onward the guest state is protected.

Oh I made a mistake that TDX sets them when initializing the VM.  They are
actually set when vCPU is created at which point you already know the VM
type thus already know the vCPU is for TDX guest.

I thought the same logic could be applied to vCPU of SNP guest?

> 
> For guest_tsc_protected, vCPUs are created after SNP_LAUNCH_START, so setting
> vcpu->arch.guest_tsc_protected there is not possible. We might need to have a
> kvm->arch.guest_tsc_protected which can be set and then percolate it to
> vcpu->arch.guest_tsc_protected when vCPUs are created, comments?

See above.  I think we can set vcpu->arch.guest_tsc_protected when
creating the vCPU, at which point you already know the VM type.