[PATCH v4 0/5] Enable Secure TSC for SEV-SNP

[PATCH v4 0/5] 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.

This patch set is also available at:

  https://github.com/AMDESE/linux-kvm/tree/sectsc-host-latest

and is based on kvm-x86/next

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

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

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:
----------
v4:
* Rebased on top of latest kvm-x86/next
* Collect Reviewed-by from Tom
* Use "KVM: SVM" instead of "crypto: ccp" (Tom)
* Clear the intercept in sev_es_init_vmcb() (Tom)
* Differentiate between guest and host MSR_IA32_TSC writes (Tom)

v3: https://lore.kernel.org/kvm/20250217102237.16434-1-nikunj@amd.com/
* Rebased on top of kvm-x86/next
* Collect Acked-by
* Separate patch to add missing desired_tsc_khz field (Tom)
* Invoke kvm_set_msr_common() for non-SecureTSC guests (Tom)
* To align desired_tsc_khz to 4-byte boundary, move the 2-byte pad0 above it (Tom)
* Update commit logs (Tom, Sean)

Ketan Chaturvedi (1):
  KVM: SVM: Enable Secure TSC for SNP guests

Nikunj A Dadhania (4):
  x86/cpufeatures: Add SNP Secure TSC
  KVM: SVM: Add missing member in SNP_LAUNCH_START command structure
  KVM: SVM: Add GUEST_TSC_FREQ MSR for Secure TSC enabled guests
  KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled

 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/svm.h         |  1 +
 arch/x86/include/uapi/asm/kvm.h    |  3 ++-
 arch/x86/kvm/svm/sev.c             | 22 ++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c             | 20 ++++++++++++++++++++
 arch/x86/kvm/svm/svm.h             | 11 ++++++++++-
 include/linux/psp-sev.h            |  2 ++
 7 files changed, 58 insertions(+), 2 deletions(-)


base-commit: c9ea48bb6ee6b28bbc956c1e8af98044618fed5e
-- 
2.43.0

[PATCH v4 1/5] 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".

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
Acked-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 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 8f8aaf94dc00..68a4d6b4cc11 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -449,6 +449,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 v4 2/5] KVM: SVM: Add missing member in SNP_LAUNCH_START command structure

[Nikunj A Dadhania]

The sev_data_snp_launch_start structure should include a 4-byte
desired_tsc_khz field before the gosvw field, which was missed in the
initial implementation. As a result, the structure is 4 bytes shorter than
expected by the firmware, causing the gosvw field to start 4 bytes early.
Fix this by adding the missing 4-byte member for the desired TSC frequency.

Fixes: 3a45dc2b419e ("crypto: ccp: Define the SEV-SNP commands")
Cc: stable@vger.kernel.org
Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 include/linux/psp-sev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index f3cad182d4ef..1f3620aaa4e7 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -594,6 +594,7 @@ struct sev_data_snp_addr {
  * @imi_en: launch flow is launching an IMI (Incoming Migration Image) for the
  *          purpose of guest-assisted migration.
  * @rsvd: reserved
+ * @desired_tsc_khz: hypervisor desired mean TSC freq in kHz of the guest
  * @gosvw: guest OS-visible workarounds, as defined by hypervisor
  */
 struct sev_data_snp_launch_start {
@@ -603,6 +604,7 @@ struct sev_data_snp_launch_start {
 	u32 ma_en:1;				/* In */
 	u32 imi_en:1;				/* In */
 	u32 rsvd:30;
+	u32 desired_tsc_khz;			/* In */
 	u8 gosvw[16];				/* In */
 } __packed;
 
-- 
2.43.0

[PATCH v4 3/5] KVM: SVM: Add GUEST_TSC_FREQ MSR for Secure TSC enabled guests

[Nikunj A Dadhania]

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>
---
 arch/x86/include/asm/svm.h |  1 +
 arch/x86/kvm/svm/sev.c     |  3 +++
 arch/x86/kvm/svm/svm.c     |  1 +
 arch/x86/kvm/svm/svm.h     | 11 ++++++++++-
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 9b7fa99ae951..6ab66b80e751 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -290,6 +290,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)
 
 struct vmcb_seg {
 	u16 selector;
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 0bc708ee2788..50263b473f95 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -4504,6 +4504,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
 	/* Clear intercepts on selected MSRs */
 	set_msr_interception(vcpu, svm->msrpm, MSR_EFER, 1, 1);
 	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_CR_PAT, 1, 1);
+
+	if (snp_secure_tsc_enabled(vcpu->kvm))
+		set_msr_interception(vcpu, svm->msrpm, MSR_AMD64_GUEST_TSC_FREQ, 1, 1);
 }
 
 void sev_init_vmcb(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8abeab91d329..e65721db1f81 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -143,6 +143,7 @@ static const struct svm_direct_access_msrs {
 	{ .index = X2APIC_MSR(APIC_TMICT),		.always = false },
 	{ .index = X2APIC_MSR(APIC_TMCCT),		.always = false },
 	{ .index = X2APIC_MSR(APIC_TDCR),		.always = false },
+	{ .index = MSR_AMD64_GUEST_TSC_FREQ,		.always = false },
 	{ .index = MSR_INVALID,				.always = false },
 };
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index d4490eaed55d..711e21b7a3d0 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -44,7 +44,7 @@ static inline struct page *__sme_pa_to_page(unsigned long pa)
 #define	IOPM_SIZE PAGE_SIZE * 3
 #define	MSRPM_SIZE PAGE_SIZE * 2
 
-#define MAX_DIRECT_ACCESS_MSRS	48
+#define MAX_DIRECT_ACCESS_MSRS	49
 #define MSRPM_OFFSETS	32
 extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
 extern bool npt_enabled;
@@ -377,10 +377,19 @@ static __always_inline bool sev_snp_guest(struct kvm *kvm)
 	return (sev->vmsa_features & SVM_SEV_FEAT_SNP_ACTIVE) &&
 	       !WARN_ON_ONCE(!sev_es_guest(kvm));
 }
+
+static inline 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));
+}
 #else
 #define sev_guest(kvm) false
 #define sev_es_guest(kvm) false
 #define sev_snp_guest(kvm) false
+#define snp_secure_tsc_enabled(kvm) false
 #endif
 
 static inline bool ghcb_gpa_is_registered(struct vcpu_svm *svm, u64 val)
-- 
2.43.0

[PATCH v4 4/5] KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled

[Nikunj A Dadhania]

Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests. Even if
KVM attempts to emulate such writes, TSC calculation will ignore the
TSC_SCALE and TSC_OFFSET present in the VMCB. Instead, it will use
GUEST_TSC_SCALE and GUEST_TSC_OFFSET stored in the VMSA.

Additionally, incorporate a check for protected guest state to allow the
VMM to initialize the TSC MSR.

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/kvm/svm/svm.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e65721db1f81..1652848b0240 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3161,6 +3161,25 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 
 		svm->tsc_aux = data;
 		break;
+	case MSR_IA32_TSC:
+		/*
+		 * For Secure TSC enabled VM, do not emulate TSC write as the
+		 * TSC calculation ignores the TSC_OFFSET and TSC_SCALE control
+		 * fields.
+		 *
+		 * Guest writes: Record the error and return a #GP.
+		 * Host writes are ignored.
+		 */
+		if (snp_secure_tsc_enabled(vcpu->kvm)) {
+			if (!msr->host_initiated) {
+				vcpu_unimpl(vcpu, "unimplemented IA32_TSC for Secure TSC\n");
+				return 1;
+			} else
+				return 0;
+		}
+
+		ret = kvm_set_msr_common(vcpu, msr);
+		break;
 	case MSR_IA32_DEBUGCTLMSR:
 		if (!lbrv) {
 			kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
-- 
2.43.0

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

[Nikunj A Dadhania]

From: Ketan Chaturvedi <Ketan.Chaturvedi@amd.com>

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. If the frequency is not
specified by the VMM, default to tsc_khz.

Signed-off-by: Ketan Chaturvedi <Ketan.Chaturvedi@amd.com>
Co-developed-by: Nikunj A Dadhania <nikunj@amd.com>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/include/uapi/asm/kvm.h |  3 ++-
 arch/x86/kvm/svm/sev.c          | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 460306b35a4b..075af0dcee25 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -839,7 +839,8 @@ struct kvm_sev_snp_launch_start {
 	__u64 policy;
 	__u8 gosvw[16];
 	__u16 flags;
-	__u8 pad0[6];
+	__u8 pad0[2];
+	__u32 desired_tsc_khz;
 	__u64 pad1[4];
 };
 
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;
+
+		/* Set the arch default TSC for the VM*/
+		kvm->arch.default_tsc_khz = user_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) {
@@ -2927,6 +2941,8 @@ void __init sev_set_cpu_caps(void)
 	if (sev_snp_enabled) {
 		kvm_cpu_cap_set(X86_FEATURE_SEV_SNP);
 		kvm_caps.supported_vm_types |= BIT(KVM_X86_SNP_VM);
+
+		kvm_cpu_cap_check_and_set(X86_FEATURE_SNP_SECURE_TSC);
 	}
 }
 
@@ -3059,6 +3075,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)
-- 
2.43.0

Re: [PATCH v4 1/5] x86/cpufeatures: Add SNP Secure TSC

[Tom Lendacky]

On 3/10/25 01:43, Nikunj A Dadhania wrote:
> 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".
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> Acked-by: Borislav Petkov (AMD) <bp@alien8.de>

Reviewed-by: Tom Lendacky <thomas.lendacky@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 8f8aaf94dc00..68a4d6b4cc11 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -449,6 +449,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 */

Re: [PATCH v4 3/5] KVM: SVM: Add GUEST_TSC_FREQ MSR for Secure TSC enabled guests

[Tom Lendacky]

On 3/10/25 01:45, 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.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>

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

> ---
>  arch/x86/include/asm/svm.h |  1 +
>  arch/x86/kvm/svm/sev.c     |  3 +++
>  arch/x86/kvm/svm/svm.c     |  1 +
>  arch/x86/kvm/svm/svm.h     | 11 ++++++++++-
>  4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 9b7fa99ae951..6ab66b80e751 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -290,6 +290,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)
>  
>  struct vmcb_seg {
>  	u16 selector;
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0bc708ee2788..50263b473f95 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -4504,6 +4504,9 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm)
>  	/* Clear intercepts on selected MSRs */
>  	set_msr_interception(vcpu, svm->msrpm, MSR_EFER, 1, 1);
>  	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_CR_PAT, 1, 1);
> +
> +	if (snp_secure_tsc_enabled(vcpu->kvm))
> +		set_msr_interception(vcpu, svm->msrpm, MSR_AMD64_GUEST_TSC_FREQ, 1, 1);
>  }
>  
>  void sev_init_vmcb(struct vcpu_svm *svm)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8abeab91d329..e65721db1f81 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -143,6 +143,7 @@ static const struct svm_direct_access_msrs {
>  	{ .index = X2APIC_MSR(APIC_TMICT),		.always = false },
>  	{ .index = X2APIC_MSR(APIC_TMCCT),		.always = false },
>  	{ .index = X2APIC_MSR(APIC_TDCR),		.always = false },
> +	{ .index = MSR_AMD64_GUEST_TSC_FREQ,		.always = false },
>  	{ .index = MSR_INVALID,				.always = false },
>  };
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index d4490eaed55d..711e21b7a3d0 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -44,7 +44,7 @@ static inline struct page *__sme_pa_to_page(unsigned long pa)
>  #define	IOPM_SIZE PAGE_SIZE * 3
>  #define	MSRPM_SIZE PAGE_SIZE * 2
>  
> -#define MAX_DIRECT_ACCESS_MSRS	48
> +#define MAX_DIRECT_ACCESS_MSRS	49
>  #define MSRPM_OFFSETS	32
>  extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
>  extern bool npt_enabled;
> @@ -377,10 +377,19 @@ static __always_inline bool sev_snp_guest(struct kvm *kvm)
>  	return (sev->vmsa_features & SVM_SEV_FEAT_SNP_ACTIVE) &&
>  	       !WARN_ON_ONCE(!sev_es_guest(kvm));
>  }
> +
> +static inline 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));
> +}
>  #else
>  #define sev_guest(kvm) false
>  #define sev_es_guest(kvm) false
>  #define sev_snp_guest(kvm) false
> +#define snp_secure_tsc_enabled(kvm) false
>  #endif
>  
>  static inline bool ghcb_gpa_is_registered(struct vcpu_svm *svm, u64 val)

Re: [PATCH v4 4/5] KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled

[Tom Lendacky]

On 3/10/25 01:45, Nikunj A Dadhania wrote:
> Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests. Even if
> KVM attempts to emulate such writes, TSC calculation will ignore the

s/TSC calculation/the TSC calculation performed by hardware/

> TSC_SCALE and TSC_OFFSET present in the VMCB. Instead, it will use

s/present/values present/
s/VMCB. Instead, it will use/VMCB and instead use the/
> GUEST_TSC_SCALE and GUEST_TSC_OFFSET stored in the VMSA.

s/stored/values stored/

> 
> Additionally, incorporate a check for protected guest state to allow the
> VMM to initialize the TSC MSR.

I don't see this in the patch.

> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>

With cleanup to the commit message and a formatting nit below:

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

> ---
>  arch/x86/kvm/svm/svm.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e65721db1f81..1652848b0240 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3161,6 +3161,25 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  
>  		svm->tsc_aux = data;
>  		break;
> +	case MSR_IA32_TSC:
> +		/*
> +		 * For Secure TSC enabled VM, do not emulate TSC write as the
> +		 * TSC calculation ignores the TSC_OFFSET and TSC_SCALE control
> +		 * fields.
> +		 *
> +		 * Guest writes: Record the error and return a #GP.
> +		 * Host writes are ignored.
> +		 */
> +		if (snp_secure_tsc_enabled(vcpu->kvm)) {
> +			if (!msr->host_initiated) {
> +				vcpu_unimpl(vcpu, "unimplemented IA32_TSC for Secure TSC\n");
> +				return 1;
> +			} else
> +				return 0;

You need "{" and "}" around this.

Thanks,
Tom

> +		}
> +
> +		ret = kvm_set_msr_common(vcpu, msr);
> +		break;
>  	case MSR_IA32_DEBUGCTLMSR:
>  		if (!lbrv) {
>  			kvm_pr_unimpl_wrmsr(vcpu, ecx, data);

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

[Tom Lendacky]

On 3/10/25 01:45, Nikunj A Dadhania wrote:
> From: Ketan Chaturvedi <Ketan.Chaturvedi@amd.com>
> 
> 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. If the frequency is not
> specified by the VMM, default to tsc_khz.
> 
> Signed-off-by: Ketan Chaturvedi <Ketan.Chaturvedi@amd.com>
> Co-developed-by: Nikunj A Dadhania <nikunj@amd.com>
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>  arch/x86/include/uapi/asm/kvm.h |  3 ++-
>  arch/x86/kvm/svm/sev.c          | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 460306b35a4b..075af0dcee25 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -839,7 +839,8 @@ struct kvm_sev_snp_launch_start {
>  	__u64 policy;
>  	__u8 gosvw[16];
>  	__u16 flags;
> -	__u8 pad0[6];
> +	__u8 pad0[2];
> +	__u32 desired_tsc_khz;
>  	__u64 pad1[4];
>  };
>  
> 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;

Do we need to perform any sanity checking against this value?

Thanks,
Tom

> +
> +		/* Set the arch default TSC for the VM*/
> +		kvm->arch.default_tsc_khz = user_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) {
> @@ -2927,6 +2941,8 @@ void __init sev_set_cpu_caps(void)
>  	if (sev_snp_enabled) {
>  		kvm_cpu_cap_set(X86_FEATURE_SEV_SNP);
>  		kvm_caps.supported_vm_types |= BIT(KVM_X86_SNP_VM);
> +
> +		kvm_cpu_cap_check_and_set(X86_FEATURE_SNP_SECURE_TSC);
>  	}
>  }
>  
> @@ -3059,6 +3075,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)

Re: [PATCH v4 4/5] KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled

[Nikunj A. Dadhania]

On 3/10/2025 8:53 PM, Tom Lendacky wrote:
> On 3/10/25 01:45, Nikunj A Dadhania wrote:
>> Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests. Even if
>> KVM attempts to emulate such writes, TSC calculation will ignore the
> 
> s/TSC calculation/the TSC calculation performed by hardware/
> 
>> TSC_SCALE and TSC_OFFSET present in the VMCB. Instead, it will use
> 
> s/present/values present/
> s/VMCB. Instead, it will use/VMCB and instead use the/
>> GUEST_TSC_SCALE and GUEST_TSC_OFFSET stored in the VMSA.
> 
> s/stored/values stored/
> 

Ack

>>
>> Additionally, incorporate a check for protected guest state to allow the
>> VMM to initialize the TSC MSR.
> 
> I don't see this in the patch.

This change is removed as we are differentiating between host/guest writes
and host writes are ignored. I will remove this.
 
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> 
> With cleanup to the commit message and a formatting nit below:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
>> ---
>>  arch/x86/kvm/svm/svm.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index e65721db1f81..1652848b0240 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3161,6 +3161,25 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>  
>>  		svm->tsc_aux = data;
>>  		break;
>> +	case MSR_IA32_TSC:
>> +		/*
>> +		 * For Secure TSC enabled VM, do not emulate TSC write as the
>> +		 * TSC calculation ignores the TSC_OFFSET and TSC_SCALE control
>> +		 * fields.
>> +		 *
>> +		 * Guest writes: Record the error and return a #GP.
>> +		 * Host writes are ignored.
>> +		 */
>> +		if (snp_secure_tsc_enabled(vcpu->kvm)) {
>> +			if (!msr->host_initiated) {
>> +				vcpu_unimpl(vcpu, "unimplemented IA32_TSC for Secure TSC\n");
>> +				return 1;
>> +			} else
>> +				return 0;
> 
> You need "{" and "}" around this.

Ack

Regards
Nikunj

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

[Nikunj A. Dadhania]

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;
> 
> 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. 
Any values below would either triggers a splat or crashes the guest kernel

For stsc-freq=1000 (1Khz), guest crash with the below:

  kvm-clock: using sched offset of 4782335885 cycles
  CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.14.0-rc5-00537-gcecc16fa7fac #254
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS unknown 02/02/2022
  RIP: 0010:pvclock_tsc_khz+0x13/0x40

For stsc-freq=500000 (500KHz), boots but with the below warning: basically tsc_khz is 
zero as we are reading zero from MSR_AMD64_GUEST_TSC_FREQ.

  WARNING: CPU: 0 PID: 0 at arch/x86/kernel/tsc.c:1463 determine_cpu_tsc_frequencies+0x11b/0x120

Regards
Nikunj

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

[Nikunj A. Dadhania]

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;
>>
>> 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;
+
 		start.desired_tsc_khz = user_tsc_khz;
 
 		/* Set the arch default TSC for the VM*/

Regards
Nikunj

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

[Tom Lendacky]

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

Thanks,
Tom

> +
>  		start.desired_tsc_khz = user_tsc_khz;
>  
>  		/* Set the arch default TSC for the VM*/
> 
> Regards
> Nikunj

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

[Sean Christopherson]

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.

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

[Nikunj A. Dadhania]

On 3/11/2025 8:22 PM, Sean Christopherson wrote:
> 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.

Setting of the desired TSC frequency needs to be done during SNP_LAUNCH_START,
while parsing of the tsc-frequency happens as part of the cpu common class, 
and kvm->arch.default_tsc_khz is set pretty late.

Intially, I had planned using "-cpu model,tsc-frequency=<>", but couldn't
find a way to get this parsed early during SNP_LAUNCH_START.

Regards,
Nikunj

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

[Sean Christopherson]

On Tue, Mar 11, 2025, Nikunj A. Dadhania wrote:
> 
> 
> On 3/11/2025 8:22 PM, Sean Christopherson wrote:
> > 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.
> 
> Setting of the desired TSC frequency needs to be done during SNP_LAUNCH_START,
> while parsing of the tsc-frequency happens as part of the cpu common class, 
> and kvm->arch.default_tsc_khz is set pretty late.

That's a QEMU problem, not a KVM problem, no?

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

[Nikunj A. Dadhania]

On 3/11/2025 9:23 PM, Sean Christopherson wrote:
> On Tue, Mar 11, 2025, Nikunj A. Dadhania wrote:
>>
>>
>> On 3/11/2025 8:22 PM, Sean Christopherson wrote:
>>> 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.
>>
>> Setting of the desired TSC frequency needs to be done during SNP_LAUNCH_START,
>> while parsing of the tsc-frequency happens as part of the cpu common class, 
>> and kvm->arch.default_tsc_khz is set pretty late.
> 
> That's a QEMU problem, not a KVM problem, no?

Yes.

I had missed that KVM_CAP_SET_TSC_KHZ can be a VM ioctl as well when
KVM_CAP_VM_TSC_CONTROL is set. Let me use this interface to set the TSC frequency 
and then during SNP_LAUNCH_START use kvm->arch.default_tsc_khz when using SecureTSC.

Does that sound ok?

Regards,
Nikunj

Re: [PATCH v4 1/5] x86/cpufeatures: Add SNP Secure TSC

[Vaishali Thakkar]

On 3/10/25 7:43 AM, Nikunj A Dadhania wrote:
> 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".
> 

Hi Nikunj,

Glad to see Secure TSC so close to being enabled upstream.

> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> Acked-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>   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 8f8aaf94dc00..68a4d6b4cc11 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -449,6 +449,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 */

I think it'll be nice to add this as a flag for /proc/cpuinfo. (similar to
"svsm" and "debug_swap"). And regardless, shouldn't it also be added to
tools/arch/x86/include/asm/cpufeatures.h?

Thanks!

>   #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 */

Re: [PATCH v4 1/5] x86/cpufeatures: Add SNP Secure TSC

[Borislav Petkov]

On Thu, Mar 13, 2025 at 04:15:14PM +0100, Vaishali Thakkar wrote:
> I think it'll be nice to add this as a flag for /proc/cpuinfo.

Documentation/arch/x86/cpuinfo.rst

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette