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

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 7/8/2025 10:21 AM, Huang, Kai wrote:
> On Mon, 2025-07-07 at 15:40 +0530, Nikunj A Dadhania wrote:
>> Add support for Secure TSC, allowing userspace to configure the Secure TSC
>> feature for SNP guests. Use the SNP specification's desired TSC frequency
>> parameter during the SNP_LAUNCH_START command to set the mean TSC
>> frequency in KHz for Secure TSC enabled guests.
>>
>> Always use kvm->arch.arch.default_tsc_khz as the TSC frequency that is
>> passed to SNP guests in the SNP_LAUNCH_START command.  The default value
>> is the host TSC frequency.  The userspace can optionally change the TSC
>> frequency via the KVM_SET_TSC_KHZ ioctl before calling the
>> SNP_LAUNCH_START ioctl.
>>
>> Introduce the read-only MSR GUEST_TSC_FREQ (0xc0010134) that returns
>> guest's effective frequency in MHZ when Secure TSC is enabled for SNP
>> guests. Disable interception of this MSR when Secure TSC is enabled. Note
>> that GUEST_TSC_FREQ MSR is accessible only to the guest and not from the
>> hypervisor context.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> 
> This SoB isn't needed.
> 
>> Co-developed-by: Ketan Chaturvedi <Ketan.Chaturvedi@amd.com>
>> Signed-off-by: Ketan Chaturvedi <Ketan.Chaturvedi@amd.com>
>> Co-developed-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.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;
> 
> 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.
> 
> Even some bug results in the default_tsc_khz being 0, will the
> SNP_LAUNCH_START command catch this and return error?
> 
>> +
>> +		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 &&

It seems to need to be opt-in since it changes the ABI somehow. E.g., it 
at least works before when the VMM calls KVM_SET_TSC_KHZ at vcpu with 
the same value passed to KVM_SET_TSC_KHZ at vm. But with the above 
change, it would fail.

Well, in reality, it's OK for QEMU since QEMU explicitly doesn't call 
KVM_SET_TSC_KHZ at vcpu for TDX VMs. But I'm not sure about the impact 
on other VMMs. Considering KVM TDX support just gets in from v6.16-rc1, 
maybe it doesn't have real impact for other VMMs as well?