[PATCH v8 0/2] Enable Secure TSC for SEV-SNP

[PATCH v8 0/2] Enable Secure TSC for SEV-SNP

[Nikunj A Dadhania]

The hypervisor controls TSC value calculations for the guest. A malicious
hypervisor can prevent the guest from progressing. The Secure TSC feature for
SEV-SNP allows guests to securely use the RDTSC and RDTSCP instructions. This
ensures the guest has a consistent view of time and prevents a malicious
hypervisor from manipulating time, such as making it appear to move backward or
advance too quickly. For more details, refer to the "Secure Nested Paging
(SEV-SNP)" section, subsection "Secure TSC" in APM Volume 2.

Patches are based on kvm-x86/next which includes MSR interception rework and a
fix adding missing desired_tsc_khz to struct sev_data_snp_launch_start

51a4273dcab3 ("KVM: SVM: Add missing member in SNP_LAUNCH_START command structure")

Testing Secure TSC
-----------------

Secure TSC guest patches are available as part of v6.14.

QEMU changes:
https://github.com/nikunjad/qemu/tree/snp-securetsc-latest

QEMU command line SEV-SNP with Secure TSC:

  qemu-system-x86_64 -cpu EPYC-Milan-v2 -smp 4 \
    -object memory-backend-memfd,id=ram1,size=1G,share=true,prealloc=false,reserve=false \
    -object sev-snp-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,secure-tsc=on,stsc-freq=2000000000 \
    -machine q35,confidential-guest-support=sev0,memory-backend=ram1 \
    ...

Changelog:
----------
v8:
* Commit message improvements (Kai Huang)
* Remove 'desired_tsc_khz' from 'struct Kim_sev_snp_launch_start' (Kai Huang)

v7: https://lore.kernel.org/kvm/20250630104426.13812-1-nikunj@amd.com/

* Rebased on top of kvm-x86/next that has MSR interception rework
* As snp_secure_tsc_enabled() is used only in sev.c, move it there (Sean) 
* Add checks to prevent user-triggerable WARN_ON_ONCE (Sean)
* Squash GUEST_TSC_FREQ MSR addition patch
* Dropped RB/TB as patch 3/4 got changed

Nikunj A Dadhania (2):
  x86/cpufeatures: Add SNP Secure TSC
  KVM: SVM: Enable Secure TSC for SNP guests

 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/svm.h         |  1 +
 arch/x86/kvm/svm/sev.c             | 34 +++++++++++++++++++++++++++---
 3 files changed, 33 insertions(+), 3 deletions(-)


base-commit: 6c7ecd725e503bf2ca69ff52c6cc48bb650b1f11
-- 
2.43.0

[PATCH v8 1/2] x86/cpufeatures: Add SNP Secure TSC

[Nikunj A Dadhania]

The Secure TSC feature for SEV-SNP allows guests to securely use the RDTSC
and RDTSCP instructions, ensuring that the parameters used cannot be
altered by the hypervisor once the guest is launched. For more details,
refer to the AMD64 APM Vol 2, Section "Secure TSC".

Acked-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Tested-by: Vaishali Thakkar <vaishali.thakkar@suse.com>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index ee176236c2be..e5001c3f7cd4 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -443,6 +443,7 @@
 #define X86_FEATURE_VM_PAGE_FLUSH	(19*32+ 2) /* VM Page Flush MSR is supported */
 #define X86_FEATURE_SEV_ES		(19*32+ 3) /* "sev_es" Secure Encrypted Virtualization - Encrypted State */
 #define X86_FEATURE_SEV_SNP		(19*32+ 4) /* "sev_snp" Secure Encrypted Virtualization - Secure Nested Paging */
+#define X86_FEATURE_SNP_SECURE_TSC	(19*32+ 8) /* SEV-SNP Secure TSC */
 #define X86_FEATURE_V_TSC_AUX		(19*32+ 9) /* Virtual TSC_AUX */
 #define X86_FEATURE_SME_COHERENT	(19*32+10) /* hardware-enforced cache coherency */
 #define X86_FEATURE_DEBUG_SWAP		(19*32+14) /* "debug_swap" SEV-ES full debug state swap support */
-- 
2.43.0

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

[Nikunj A Dadhania]

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>
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>

---

I have incorporated changes from Sean to prevent the setting of SecureTSC
for non-SNP guests. I have added his 'Co-developed-by' acknowledgment, but
I have not yet included his 'Signed-off-by'. I will leave that for him to
add.
---
 arch/x86/include/asm/svm.h |  1 +
 arch/x86/kvm/svm/sev.c     | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index ffc27f676243..17f6c3fedeee 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -299,6 +299,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_
 #define SVM_SEV_FEAT_RESTRICTED_INJECTION		BIT(3)
 #define SVM_SEV_FEAT_ALTERNATE_INJECTION		BIT(4)
 #define SVM_SEV_FEAT_DEBUG_SWAP				BIT(5)
+#define SVM_SEV_FEAT_SECURE_TSC				BIT(9)
 
 #define VMCB_ALLOWED_SEV_FEATURES_VALID			BIT_ULL(63)
 
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index fde328ed3f78..5ac4841f925d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -146,6 +146,14 @@ static bool sev_vcpu_has_debug_swap(struct vcpu_svm *svm)
 	return sev->vmsa_features & SVM_SEV_FEAT_DEBUG_SWAP;
 }
 
+static bool snp_secure_tsc_enabled(struct kvm *kvm)
+{
+	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
+
+	return (sev->vmsa_features & SVM_SEV_FEAT_SECURE_TSC) &&
+		!WARN_ON_ONCE(!sev_snp_guest(kvm));
+}
+
 /* Must be called with the sev_bitmap_lock held */
 static bool __sev_recycle_asids(unsigned int min_asid, unsigned int max_asid)
 {
@@ -405,6 +413,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
 	struct sev_platform_init_args init_args = {0};
 	bool es_active = vm_type != KVM_X86_SEV_VM;
+	bool snp_active = vm_type == KVM_X86_SNP_VM;
 	u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
 	int ret;
 
@@ -414,6 +423,9 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (data->flags)
 		return -EINVAL;
 
+	if (!snp_active)
+		valid_vmsa_features &= ~SVM_SEV_FEAT_SECURE_TSC;
+
 	if (data->vmsa_features & ~valid_vmsa_features)
 		return -EINVAL;
 
@@ -436,7 +448,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (sev->es_active && !sev->ghcb_version)
 		sev->ghcb_version = GHCB_VERSION_DEFAULT;
 
-	if (vm_type == KVM_X86_SNP_VM)
+	if (snp_active)
 		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
 	ret = sev_asid_new(sev);
@@ -449,7 +461,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 		goto e_free;
 
 	/* This needs to happen after SEV/SNP firmware initialization. */
-	if (vm_type == KVM_X86_SNP_VM) {
+	if (snp_active) {
 		ret = snp_guest_req_init(kvm);
 		if (ret)
 			goto e_free;
@@ -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;
+
+		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);
+
 		/*
 		 * SEV-ES (and thus SNP) guest mandates LBR Virtualization to
 		 * be _always_ ON. Enable it only after setting
@@ -3036,6 +3058,9 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (sev_snp_enabled && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
+		sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;
 }
 
 void sev_hardware_unsetup(void)
@@ -4487,6 +4512,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
 
 	/* Can't intercept XSETBV, HV can't modify XCR0 directly */
 	svm_clr_intercept(svm, INTERCEPT_XSETBV);
+
+	if (snp_secure_tsc_enabled(svm->vcpu.kvm))
+		svm_disable_intercept_for_msr(&svm->vcpu, MSR_AMD64_GUEST_TSC_FREQ, MSR_TYPE_RW);
 }
 
 void sev_init_vmcb(struct vcpu_svm *svm)
-- 
2.43.0

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

[Tom Lendacky]

On 7/7/25 05:10, 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>
> 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>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> 
> ---
> 
> I have incorporated changes from Sean to prevent the setting of SecureTSC
> for non-SNP guests. I have added his 'Co-developed-by' acknowledgment, but
> I have not yet included his 'Signed-off-by'. I will leave that for him to
> add.
> ---
>  arch/x86/include/asm/svm.h |  1 +
>  arch/x86/kvm/svm/sev.c     | 34 +++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index ffc27f676243..17f6c3fedeee 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -299,6 +299,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_
>  #define SVM_SEV_FEAT_RESTRICTED_INJECTION		BIT(3)
>  #define SVM_SEV_FEAT_ALTERNATE_INJECTION		BIT(4)
>  #define SVM_SEV_FEAT_DEBUG_SWAP				BIT(5)
> +#define SVM_SEV_FEAT_SECURE_TSC				BIT(9)
>  
>  #define VMCB_ALLOWED_SEV_FEATURES_VALID			BIT_ULL(63)
>  
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index fde328ed3f78..5ac4841f925d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -146,6 +146,14 @@ static bool sev_vcpu_has_debug_swap(struct vcpu_svm *svm)
>  	return sev->vmsa_features & SVM_SEV_FEAT_DEBUG_SWAP;
>  }
>  
> +static bool snp_secure_tsc_enabled(struct kvm *kvm)
> +{
> +	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> +
> +	return (sev->vmsa_features & SVM_SEV_FEAT_SECURE_TSC) &&
> +		!WARN_ON_ONCE(!sev_snp_guest(kvm));
> +}
> +
>  /* Must be called with the sev_bitmap_lock held */
>  static bool __sev_recycle_asids(unsigned int min_asid, unsigned int max_asid)
>  {
> @@ -405,6 +413,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
>  	struct sev_platform_init_args init_args = {0};
>  	bool es_active = vm_type != KVM_X86_SEV_VM;
> +	bool snp_active = vm_type == KVM_X86_SNP_VM;
>  	u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
>  	int ret;
>  
> @@ -414,6 +423,9 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	if (data->flags)
>  		return -EINVAL;
>  
> +	if (!snp_active)
> +		valid_vmsa_features &= ~SVM_SEV_FEAT_SECURE_TSC;
> +
>  	if (data->vmsa_features & ~valid_vmsa_features)
>  		return -EINVAL;
>  
> @@ -436,7 +448,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	if (sev->es_active && !sev->ghcb_version)
>  		sev->ghcb_version = GHCB_VERSION_DEFAULT;
>  
> -	if (vm_type == KVM_X86_SNP_VM)
> +	if (snp_active)
>  		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
>  
>  	ret = sev_asid_new(sev);
> @@ -449,7 +461,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  		goto e_free;
>  
>  	/* This needs to happen after SEV/SNP firmware initialization. */
> -	if (vm_type == KVM_X86_SNP_VM) {
> +	if (snp_active) {
>  		ret = snp_guest_req_init(kvm);
>  		if (ret)
>  			goto e_free;
> @@ -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;
> +
> +		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);
> +
>  		/*
>  		 * SEV-ES (and thus SNP) guest mandates LBR Virtualization to
>  		 * be _always_ ON. Enable it only after setting
> @@ -3036,6 +3058,9 @@ void __init sev_hardware_setup(void)
>  	sev_supported_vmsa_features = 0;
>  	if (sev_es_debug_swap_enabled)
>  		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
> +
> +	if (sev_snp_enabled && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
> +		sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;
>  }
>  
>  void sev_hardware_unsetup(void)
> @@ -4487,6 +4512,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
>  
>  	/* Can't intercept XSETBV, HV can't modify XCR0 directly */
>  	svm_clr_intercept(svm, INTERCEPT_XSETBV);
> +
> +	if (snp_secure_tsc_enabled(svm->vcpu.kvm))
> +		svm_disable_intercept_for_msr(&svm->vcpu, MSR_AMD64_GUEST_TSC_FREQ, MSR_TYPE_RW);
>  }
>  
>  void sev_init_vmcb(struct vcpu_svm *svm)

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

[Huang, Kai]

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 &&

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.

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.

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

[Nikunj A. Dadhania]

On 7/8/2025 7:51 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;

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.

> 
>> +
>> +		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.

> 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.

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?

Regards
Nikunj
 

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

[Xiaoyao Li]

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?

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

[Huang, Kai]

> > 
> > > @@ -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.

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

[Huang, Kai]

> > > -		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?

Good point.  Perhaps Paolo/Sean can also comment.  I believe the risk is
quite low too.  I won't bother using "opt-in" though.

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

[Sean Christopherson]

On Tue, Jul 08, 2025, Kai Huang wrote:
> > > 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?

Support for secure TSC should depend on tsc_khz being non-zero.  That way it'll
be impossible for arch.default_tsc_khz to be zero at runtime.  Then KVM can WARN
on arch.default_tsc_khz being zero during SNP_LAUNCH_START.

I.e.

	if (sev_snp_enabled && tsc_khz &&
	    cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
		sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;

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

[Sean Christopherson]

On Mon, Jul 07, 2025, Nikunj A Dadhania wrote:
> 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.

...

> @@ -4487,6 +4512,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
>  
>  	/* Can't intercept XSETBV, HV can't modify XCR0 directly */
>  	svm_clr_intercept(svm, INTERCEPT_XSETBV);
> +
> +	if (snp_secure_tsc_enabled(svm->vcpu.kvm))
> +		svm_disable_intercept_for_msr(&svm->vcpu, MSR_AMD64_GUEST_TSC_FREQ, MSR_TYPE_RW);

KVM shouldn't be disabling write interception for a read-only MSR.  And this
code belongs in sev_es_recalc_msr_intercepts().

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

[Sean Christopherson]

On Tue, Jul 08, 2025, Kai Huang wrote:
> 
> > > > -		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:

Yes, we definitely should.  And if it's not too ugly, KVM should also reject the
VM-scoped KVM_SET_TSC_KHZ if vCPUs have been created with guest_tsc_protected=true.
(or maybe we could get greedy and try to disallow KVM_SET_TSC_KHZ if vCPUs have
been created for any VM shape?)

> > > 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?

6.16 hasn't officially release yet, so any impact to userspace is irrelevant,
i.e. there is no established ABI at this time.

Can someone send a proper patch?

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

[Huang, Kai]

On Tue, 2025-07-08 at 07:34 -0700, Sean Christopherson wrote:
> On Tue, Jul 08, 2025, Kai Huang wrote:
> > > > 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?
> 
> Support for secure TSC should depend on tsc_khz being non-zero.  That way it'll
> be impossible for arch.default_tsc_khz to be zero at runtime.  Then KVM can WARN
> on arch.default_tsc_khz being zero during SNP_LAUNCH_START.
> 
> I.e.
> 
> 	if (sev_snp_enabled && tsc_khz &&
> 	    cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
> 		sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;

Yeah looks good.

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

[Huang, Kai]

On Tue, 2025-07-08 at 07:42 -0700, Sean Christopherson wrote:
> On Tue, Jul 08, 2025, Kai Huang wrote:
> > 
> > > > > -		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:
> 
> Yes, we definitely should.  And if it's not too ugly, KVM should also reject the
> VM-scoped KVM_SET_TSC_KHZ if vCPUs have been created with guest_tsc_protected=true.
> (or maybe we could get greedy and try to disallow KVM_SET_TSC_KHZ if vCPUs have
> been created for any VM shape?)

Technically, if we want to do, I think we should do the simple way (the
latter).  But I think this will have a real potential impact on the ABI?

I would prefer the simple way.  How do you think?

> 
> > > > 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?
> 
> 6.16 hasn't officially release yet, so any impact to userspace is irrelevant,
> i.e. there is no established ABI at this time.
> 
> Can someone send a proper patch?

I will try to do today.

But if you also want to reject KVM_SET_TSC_KHZ VM ioctl in either way,
then it could be in a separate patch, which means we should have 2
separate patches to handle?

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

[Sean Christopherson]

On Tue, Jul 08, 2025, Kai Huang wrote:
> On Tue, 2025-07-08 at 07:42 -0700, Sean Christopherson wrote:
> > On Tue, Jul 08, 2025, Kai Huang wrote:
> > > 
> > > > > > -		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:
> > 
> > Yes, we definitely should.  And if it's not too ugly, KVM should also reject the
> > VM-scoped KVM_SET_TSC_KHZ if vCPUs have been created with guest_tsc_protected=true.
> > (or maybe we could get greedy and try to disallow KVM_SET_TSC_KHZ if vCPUs have
> > been created for any VM shape?)
> 
> Technically, if we want to do, I think we should do the simple way (the
> latter).  But I think this will have a real potential impact on the ABI?
> 
> I would prefer the simple way.  How do you think?

I like the simple way too, but as you note, it'll most definitely be an ABI
change.  Which is what I was alluding to by "getting greedy" :-)

> > > > > 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?
> > 
> > 6.16 hasn't officially release yet, so any impact to userspace is irrelevant,
> > i.e. there is no established ABI at this time.
> > 
> > Can someone send a proper patch?
> 
> I will try to do today.

Thanks!

> But if you also want to reject KVM_SET_TSC_KHZ VM ioctl in either way,
> then it could be in a separate patch, which means we should have 2
> separate patches to handle?

Ya, probably two separate patches given that the VM-scoped change could break
userspace.

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

[Nikunj A. Dadhania]

On 7/8/2025 8:07 PM, Sean Christopherson wrote:
> On Mon, Jul 07, 2025, Nikunj A Dadhania wrote:
>> 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.
> 
> ...
> 
>> @@ -4487,6 +4512,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
>>  
>>  	/* Can't intercept XSETBV, HV can't modify XCR0 directly */
>>  	svm_clr_intercept(svm, INTERCEPT_XSETBV);
>> +
>> +	if (snp_secure_tsc_enabled(svm->vcpu.kvm))
>> +		svm_disable_intercept_for_msr(&svm->vcpu, MSR_AMD64_GUEST_TSC_FREQ, MSR_TYPE_RW);
> 
> KVM shouldn't be disabling write interception for a read-only MSR. 

Few of things to consider here:
1) GUEST_TSC_FREQ is a *guest only* MSR and what is the point in KVM intercepting writes
   to that MSR. The guest vCPU handles it appropriately when interception is disabled.

2) Guest does not expect GUEST_TSC_FREQ MSR to be intercepted(read or write), guest 
   will terminate if GUEST_TSC_FREQ MSR is intercepted by the hypervisor:

38cc6495cdec x86/sev: Prevent GUEST_TSC_FREQ MSR interception for Secure TSC enabled guests

>  And this
> code belongs in sev_es_recalc_msr_intercepts().

Sure.

Regards,
Nikunj

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

[Nikunj A. Dadhania]

On 7/8/2025 8:04 PM, Sean Christopherson wrote:
> On Tue, Jul 08, 2025, Kai Huang wrote:
>>>> 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?
> 
> Support for secure TSC should depend on tsc_khz being non-zero.  That way it'll
> be impossible for arch.default_tsc_khz to be zero at runtime.  Then KVM can WARN
> on arch.default_tsc_khz being zero during SNP_LAUNCH_START.

Sure.

> 
> I.e.
> 
> 	if (sev_snp_enabled && tsc_khz &&
> 	    cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
> 		sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;

Yes, this is better.

Regards
Nikunj

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

[Huang, Kai]

> 
> Ya, probably two separate patches given that the VM-scoped change could break
> userspace.

I just sent out a small series to do this, and added your Suggested-by in
the patches. :-)

Btw I need to take rest of this week off for some stuff but I may be able
to check emails to follow up.

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

[Sean Christopherson]

On Wed, Jul 09, 2025, Nikunj A. Dadhania wrote:
> On 7/8/2025 8:07 PM, Sean Christopherson wrote:
> > On Mon, Jul 07, 2025, Nikunj A Dadhania wrote:
> >> 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.
> > 
> > ...
> > 
> >> @@ -4487,6 +4512,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
> >>  
> >>  	/* Can't intercept XSETBV, HV can't modify XCR0 directly */
> >>  	svm_clr_intercept(svm, INTERCEPT_XSETBV);
> >> +
> >> +	if (snp_secure_tsc_enabled(svm->vcpu.kvm))
> >> +		svm_disable_intercept_for_msr(&svm->vcpu, MSR_AMD64_GUEST_TSC_FREQ, MSR_TYPE_RW);
> > 
> > KVM shouldn't be disabling write interception for a read-only MSR. 
> 
> Few of things to consider here:
> 1) GUEST_TSC_FREQ is a *guest only* MSR and what is the point in KVM intercepting writes
>    to that MSR.

Because there's zero point in not intercepting writes, and KVM shouldn't do
things for no reason as doing so tends to confuse readers.  E.g. I reacted to
this because I didn't read the changelog first, and was surprised that the guest
could adjust its TSC frequency (which it obviously can't, but that's what the
code implies to me).

>    The guest vCPU handles it appropriately when interception is disabled.
>
> 2) Guest does not expect GUEST_TSC_FREQ MSR to be intercepted(read or write), guest 
>    will terminate if GUEST_TSC_FREQ MSR is intercepted by the hypervisor:

But it's read-only, the guest shouldn't be writing.  If the vCPU handles #GPs
appropriately, then it should have no problem handling #VCs on bad writes.

> 38cc6495cdec x86/sev: Prevent GUEST_TSC_FREQ MSR interception for Secure TSC enabled guests

That's a guest bug, it shouldn't be complaining about the host intercepting writes.

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

[Nikunj A Dadhania]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jul 09, 2025, Nikunj A. Dadhania wrote:
>> On 7/8/2025 8:07 PM, Sean Christopherson wrote:
>> > On Mon, Jul 07, 2025, Nikunj A Dadhania wrote:
>> >> 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.
>> > 
>> > ...
>> > 
>> >> @@ -4487,6 +4512,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
>> >>  
>> >>  	/* Can't intercept XSETBV, HV can't modify XCR0 directly */
>> >>  	svm_clr_intercept(svm, INTERCEPT_XSETBV);
>> >> +
>> >> +	if (snp_secure_tsc_enabled(svm->vcpu.kvm))
>> >> +		svm_disable_intercept_for_msr(&svm->vcpu, MSR_AMD64_GUEST_TSC_FREQ, MSR_TYPE_RW);
>> > 
>> > KVM shouldn't be disabling write interception for a read-only MSR. 
>> 
>> Few of things to consider here:
>> 1) GUEST_TSC_FREQ is a *guest only* MSR and what is the point in KVM intercepting writes
>>    to that MSR.
>
> Because there's zero point in not intercepting writes, and KVM shouldn't do
> things for no reason as doing so tends to confuse readers.  E.g. I reacted to
> this because I didn't read the changelog first, and was surprised that the guest
> could adjust its TSC frequency (which it obviously can't, but that's what the
> code implies to me).
>

Agree to your point that MSR read-only and having a MSR_TYPE_RW
creates a special case. I can change this to MSR_TYPE_R. The only thing
which looks inefficient to me is the path to generate the #GP when the
MSR interception is enabled.

AFAIU, the GUEST_TSC_FREQ write handling for SEV-SNP guest:

sev_handle_vmgexit()
-> msr_interception()
  -> kvm_set_msr_common()
     -> kvm_emulate_wrmsr()
        -> kvm_set_msr_with_filter()
        -> svm_complete_emulated_msr() will inject the #GP

With MSR interception disabled: vCPU will directly generate #GP

>>    The guest vCPU handles it appropriately when interception is disabled.
>>
>> 2) Guest does not expect GUEST_TSC_FREQ MSR to be intercepted(read or write), guest 
>>    will terminate if GUEST_TSC_FREQ MSR is intercepted by the hypervisor:
>
> But it's read-only, the guest shouldn't be writing.  If the vCPU handles #GPs
> appropriately, then it should have no problem handling #VCs on bad writes.
>
>> 38cc6495cdec x86/sev: Prevent GUEST_TSC_FREQ MSR interception for Secure TSC enabled guests
>
> That's a guest bug, it shouldn't be complaining about the host
> intercepting writes.

The code was written with a perspective that host should not be
intercepting GUEST_TSC_FREQ, as it is a guest-only MSR. To fix guest we
will need to add something like below:

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index e50736d15e02..180a26d5751c 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -32,6 +32,7 @@ enum es_result {
 	ES_DECODE_FAILED,	/* Instruction decoding failed */
 	ES_EXCEPTION,		/* Instruction caused exception */
 	ES_RETRY,		/* Retry instruction emulation */
+	ES_CONTINUE,		/* Continue current operation */
 };
 
 struct es_fault_info {
diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
index 9a5e16f70e83..ed4b207ea362 100644
--- a/arch/x86/coco/sev/vc-handle.c
+++ b/arch/x86/coco/sev/vc-handle.c
@@ -369,20 +369,23 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
 }
 
 /*
- * TSC related accesses should not exit to the hypervisor when a guest is
- * executing with Secure TSC enabled, so special handling is required for
- * accesses of MSR_IA32_TSC and MSR_AMD64_GUEST_TSC_FREQ.
+ * Some of the TSC related accesses should not exit to the hypervisor when
+ * a guest is executing with Secure TSC enabled, so special handling is
+ * required for accesses of MSR_IA32_TSC and MSR_AMD64_GUEST_TSC_FREQ.
  */
 static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write)
 {
 	u64 tsc;
 
+	if (!(sev_status & MSR_AMD64_SNP_SECURE_TSC))
+		return ES_CONTINUE;
+
 	/*
-	 * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
-	 * Terminate the SNP guest when the interception is enabled.
+	 * GUEST_TSC_FREQ read should not be intercepted when Secure TSC is enabled.
+	 * Terminate the SNP guest when the read interception is enabled.
 	 */
 	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
-		return ES_VMM_ERROR;
+		return write ? ES_CONTINUE : ES_VMM_ERROR;
 
 	/*
 	 * Writes: Writing to MSR_IA32_TSC can cause subsequent reads of the TSC
@@ -417,8 +420,9 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 		return __vc_handle_msr_caa(regs, write);
 	case MSR_IA32_TSC:
 	case MSR_AMD64_GUEST_TSC_FREQ:
-		if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
-			return __vc_handle_secure_tsc_msrs(regs, write);
+		ret = __vc_handle_secure_tsc_msrs(regs, write);
+		if (ret != ES_CONTINUE)
+			return ret;
 		break;
 	default:
 		break;

Regards,
Nikunj

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

[Sean Christopherson]

On Thu, Jul 10, 2025, Nikunj A Dadhania wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > Because there's zero point in not intercepting writes, and KVM shouldn't do
> > things for no reason as doing so tends to confuse readers.  E.g. I reacted to
> > this because I didn't read the changelog first, and was surprised that the guest
> > could adjust its TSC frequency (which it obviously can't, but that's what the
> > code implies to me).
> >
> 
> Agree to your point that MSR read-only and having a MSR_TYPE_RW
> creates a special case. I can change this to MSR_TYPE_R. The only thing
> which looks inefficient to me is the path to generate the #GP when the
> MSR interception is enabled.
> 
> AFAIU, the GUEST_TSC_FREQ write handling for SEV-SNP guest:
> 
> sev_handle_vmgexit()
> -> msr_interception()
>   -> kvm_set_msr_common()
>      -> kvm_emulate_wrmsr()
>         -> kvm_set_msr_with_filter()
>         -> svm_complete_emulated_msr() will inject the #GP
> 
> With MSR interception disabled: vCPU will directly generate #GP

Yes, but no well-behaved guest will ever write the MSR, and if a guest does
manage to generate a WRMSR, the guest is beyond hosed if it affects performance.

> >>    The guest vCPU handles it appropriately when interception is disabled.
> >>
> >> 2) Guest does not expect GUEST_TSC_FREQ MSR to be intercepted(read or write), guest 
> >>    will terminate if GUEST_TSC_FREQ MSR is intercepted by the hypervisor:
> >
> > But it's read-only, the guest shouldn't be writing.  If the vCPU handles #GPs
> > appropriately, then it should have no problem handling #VCs on bad writes.
> >
> >> 38cc6495cdec x86/sev: Prevent GUEST_TSC_FREQ MSR interception for Secure TSC enabled guests
> >
> > That's a guest bug, it shouldn't be complaining about the host
> > intercepting writes.
> 
> The code was written with a perspective that host should not be
> intercepting GUEST_TSC_FREQ, as it is a guest-only MSR.

It's fine to panic on a _read_, I'm saying the guest shouldn't panic on a write,
because the guest shouldn't be writing in the first place.

diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
index 0989d98da130..353647339a79 100644
--- a/arch/x86/coco/sev/vc-handle.c
+++ b/arch/x86/coco/sev/vc-handle.c
@@ -369,24 +369,21 @@ static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool wri
        u64 tsc;
 
        /*
-        * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
-        * Terminate the SNP guest when the interception is enabled.
+        * Writing to MSR_IA32_TSC can cause subsequent reads of the TSC to
+        * return undefined values, and GUEST_TSC_FREQ is read-only.  Ignore
+        * all writes, but WARN to log the kernel bug.
+        */
+       if (WARN_ON_ONCE(write))
+               return ES_OK;
+
+       /*
+        * GUEST_TSC_FREQ should be not be intercepted when Secure TSC is
+        * enabled. Terminate the SNP guest when the interception is enabled.
         */
        if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
                return ES_VMM_ERROR;
 
-       /*
-        * Writes: Writing to MSR_IA32_TSC can cause subsequent reads of the TSC
-        *         to return undefined values, so ignore all writes.
-        *
-        * Reads: Reads of MSR_IA32_TSC should return the current TSC value, use
-        *        the value returned by rdtsc_ordered().
-        */
-       if (write) {
-               WARN_ONCE(1, "TSC MSR writes are verboten!\n");
-               return ES_OK;
-       }
-
+       /* Reads of MSR_IA32_TSC should return the current TSC value. */
        tsc = rdtsc_ordered();
        regs->ax = lower_32_bits(tsc);
        regs->dx = upper_32_bits(tsc);

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

[Nikunj A. Dadhania]

On 7/10/2025 6:50 PM, Sean Christopherson wrote:
> On Thu, Jul 10, 2025, Nikunj A Dadhania wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>>> Because there's zero point in not intercepting writes, and KVM shouldn't do
>>> things for no reason as doing so tends to confuse readers.  E.g. I reacted to
>>> this because I didn't read the changelog first, and was surprised that the guest
>>> could adjust its TSC frequency (which it obviously can't, but that's what the
>>> code implies to me).
>>>
>>
>> Agree to your point that MSR read-only and having a MSR_TYPE_RW
>> creates a special case. I can change this to MSR_TYPE_R. The only thing
>> which looks inefficient to me is the path to generate the #GP when the
>> MSR interception is enabled.
>>
>> AFAIU, the GUEST_TSC_FREQ write handling for SEV-SNP guest:
>>
>> sev_handle_vmgexit()
>> -> msr_interception()
>>   -> kvm_set_msr_common()
>>      -> kvm_emulate_wrmsr()
>>         -> kvm_set_msr_with_filter()
>>         -> svm_complete_emulated_msr() will inject the #GP
>>
>> With MSR interception disabled: vCPU will directly generate #GP
> 
> Yes, but no well-behaved guest will ever write the MSR, and if a guest does
> manage to generate a WRMSR, the guest is beyond hosed if it affects performance.
> 
>>>>    The guest vCPU handles it appropriately when interception is disabled.
>>>>
>>>> 2) Guest does not expect GUEST_TSC_FREQ MSR to be intercepted(read or write), guest 
>>>>    will terminate if GUEST_TSC_FREQ MSR is intercepted by the hypervisor:
>>>
>>> But it's read-only, the guest shouldn't be writing.  If the vCPU handles #GPs
>>> appropriately, then it should have no problem handling #VCs on bad writes.
>>>
>>>> 38cc6495cdec x86/sev: Prevent GUEST_TSC_FREQ MSR interception for Secure TSC enabled guests
>>>
>>> That's a guest bug, it shouldn't be complaining about the host
>>> intercepting writes.
>>
>> The code was written with a perspective that host should not be
>> intercepting GUEST_TSC_FREQ, as it is a guest-only MSR.
> 
> It's fine to panic on a _read_, I'm saying the guest shouldn't panic on a write,
> because the guest shouldn't be writing in the first place.

Agree, and the with the below change the write to GUEST_TSC_FREQ will be ignored.

Should I send a patch with your authorship/signed-off-by ?

> diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
> index 0989d98da130..353647339a79 100644
> --- a/arch/x86/coco/sev/vc-handle.c
> +++ b/arch/x86/coco/sev/vc-handle.c
> @@ -369,24 +369,21 @@ static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool wri
>         u64 tsc;
>  
>         /*
> -        * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
> -        * Terminate the SNP guest when the interception is enabled.
> +        * Writing to MSR_IA32_TSC can cause subsequent reads of the TSC to
> +        * return undefined values, and GUEST_TSC_FREQ is read-only.  Ignore
> +        * all writes, but WARN to log the kernel bug.
> +        */
> +       if (WARN_ON_ONCE(write))
> +               return ES_OK;
> +
> +       /*
> +        * GUEST_TSC_FREQ should be not be intercepted when Secure TSC is
> +        * enabled. Terminate the SNP guest when the interception is enabled.
>          */
>         if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
>                 return ES_VMM_ERROR;
>  
> -       /*
> -        * Writes: Writing to MSR_IA32_TSC can cause subsequent reads of the TSC
> -        *         to return undefined values, so ignore all writes.
> -        *
> -        * Reads: Reads of MSR_IA32_TSC should return the current TSC value, use
> -        *        the value returned by rdtsc_ordered().
> -        */
> -       if (write) {
> -               WARN_ONCE(1, "TSC MSR writes are verboten!\n");
> -               return ES_OK;
> -       }
> -
> +       /* Reads of MSR_IA32_TSC should return the current TSC value. */
>         tsc = rdtsc_ordered();
>         regs->ax = lower_32_bits(tsc);
>         regs->dx = upper_32_bits(tsc);

Regards,
Nikunj

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

[Sean Christopherson]

On Thu, Jul 10, 2025, Nikunj A. Dadhania wrote:
> On 7/10/2025 6:50 PM, Sean Christopherson wrote:
> > On Thu, Jul 10, 2025, Nikunj A Dadhania wrote:
> >> Sean Christopherson <seanjc@google.com> writes:
> >>> Because there's zero point in not intercepting writes, and KVM shouldn't do
> >>> things for no reason as doing so tends to confuse readers.  E.g. I reacted to
> >>> this because I didn't read the changelog first, and was surprised that the guest
> >>> could adjust its TSC frequency (which it obviously can't, but that's what the
> >>> code implies to me).
> >>>
> >>
> >> Agree to your point that MSR read-only and having a MSR_TYPE_RW
> >> creates a special case. I can change this to MSR_TYPE_R. The only thing
> >> which looks inefficient to me is the path to generate the #GP when the
> >> MSR interception is enabled.
> >>
> >> AFAIU, the GUEST_TSC_FREQ write handling for SEV-SNP guest:
> >>
> >> sev_handle_vmgexit()
> >> -> msr_interception()
> >>   -> kvm_set_msr_common()
> >>      -> kvm_emulate_wrmsr()
> >>         -> kvm_set_msr_with_filter()
> >>         -> svm_complete_emulated_msr() will inject the #GP
> >>
> >> With MSR interception disabled: vCPU will directly generate #GP
> > 
> > Yes, but no well-behaved guest will ever write the MSR, and if a guest does
> > manage to generate a WRMSR, the guest is beyond hosed if it affects performance.
> > 
> >>>>    The guest vCPU handles it appropriately when interception is disabled.
> >>>>
> >>>> 2) Guest does not expect GUEST_TSC_FREQ MSR to be intercepted(read or write), guest 
> >>>>    will terminate if GUEST_TSC_FREQ MSR is intercepted by the hypervisor:
> >>>
> >>> But it's read-only, the guest shouldn't be writing.  If the vCPU handles #GPs
> >>> appropriately, then it should have no problem handling #VCs on bad writes.
> >>>
> >>>> 38cc6495cdec x86/sev: Prevent GUEST_TSC_FREQ MSR interception for Secure TSC enabled guests
> >>>
> >>> That's a guest bug, it shouldn't be complaining about the host
> >>> intercepting writes.
> >>
> >> The code was written with a perspective that host should not be
> >> intercepting GUEST_TSC_FREQ, as it is a guest-only MSR.
> > 
> > It's fine to panic on a _read_, I'm saying the guest shouldn't panic on a write,
> > because the guest shouldn't be writing in the first place.
> 
> Agree, and the with the below change the write to GUEST_TSC_FREQ will be ignored.
> 
> Should I send a patch with your authorship/signed-off-by ?

Sure, that'd be wonderful!

Signed-off-by: Sean Christopherson <seanjc@google.com>

> > diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
> > index 0989d98da130..353647339a79 100644
> > --- a/arch/x86/coco/sev/vc-handle.c
> > +++ b/arch/x86/coco/sev/vc-handle.c
> > @@ -369,24 +369,21 @@ static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool wri
> >         u64 tsc;
> >  
> >         /*
> > -        * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
> > -        * Terminate the SNP guest when the interception is enabled.
> > +        * Writing to MSR_IA32_TSC can cause subsequent reads of the TSC to
> > +        * return undefined values, and GUEST_TSC_FREQ is read-only.  Ignore
> > +        * all writes, but WARN to log the kernel bug.
> > +        */
> > +       if (WARN_ON_ONCE(write))
> > +               return ES_OK;
> > +
> > +       /*
> > +        * GUEST_TSC_FREQ should be not be intercepted when Secure TSC is
> > +        * enabled. Terminate the SNP guest when the interception is enabled.
> >          */
> >         if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
> >                 return ES_VMM_ERROR;
> >  
> > -       /*
> > -        * Writes: Writing to MSR_IA32_TSC can cause subsequent reads of the TSC
> > -        *         to return undefined values, so ignore all writes.
> > -        *
> > -        * Reads: Reads of MSR_IA32_TSC should return the current TSC value, use
> > -        *        the value returned by rdtsc_ordered().
> > -        */
> > -       if (write) {
> > -               WARN_ONCE(1, "TSC MSR writes are verboten!\n");
> > -               return ES_OK;
> > -       }
> > -
> > +       /* Reads of MSR_IA32_TSC should return the current TSC value. */
> >         tsc = rdtsc_ordered();
> >         regs->ax = lower_32_bits(tsc);
> >         regs->dx = upper_32_bits(tsc);
> 
> Regards,
> Nikunj