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

[PATCH v7 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.

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:
----------
v7:
* 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

v6:
* Rebased on top of kvm/master
* Collected Reviewed-by/Tested-by
* s/svm->vcpu/vcpu/ in snp_launch_update_vmsa() as vcpu pointer is already available (Tom)
* Simplify assignment of guest_protected_tsc (Tom)

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/include/uapi/asm/kvm.h    |  3 ++-
 arch/x86/kvm/svm/sev.c             | 34 +++++++++++++++++++++++++++---
 4 files changed, 35 insertions(+), 4 deletions(-)


base-commit: 6c7ecd725e503bf2ca69ff52c6cc48bb650b1f11
-- 
2.43.0

[PATCH v7 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 v7 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.

As the frequency needs to be set in the SNP_LAUNCH_START command, userspace
should set the frequency using the KVM_CAP_SET_TSC_KHZ VM ioctl instead of
the VCPU ioctl. The desired_tsc_khz defaults to kvm->arch.default_tsc_khz.

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

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/include/uapi/asm/kvm.h |  3 ++-
 arch/x86/kvm/svm/sev.c          | 34 ++++++++++++++++++++++++++++++---
 3 files changed, 34 insertions(+), 4 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/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 13da87c05098..4a36cda05f38 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -840,7 +840,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 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 v7 2/2] KVM: SVM: Enable Secure TSC for SNP guests

[Huang, Kai]

On Mon, 2025-06-30 at 16:14 +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.
> 
> As the frequency needs to be set in the SNP_LAUNCH_START command, userspace
> should set the frequency using the KVM_CAP_SET_TSC_KHZ VM ioctl instead of
				     ^

I believe you meant KVM_SET_TSC_KHZ ioctl?  Since I am not able to find
KVM_CAP_SET_TSC_KHZ. :-)

> the VCPU ioctl. The desired_tsc_khz defaults to kvm->arch.default_tsc_khz.

IIRC the KVM_SET_TSC_KHZ ioctl updates the kvm->arch.default_tsc_khz, and
the snp_launch_start() always just uses it.

The last sentence is kinda confusing since it sounds like that
desired_tsc_khz is used by the SEV command and it could have a different
value from kvm->arch.default_tsc_khz.

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

Well I guess you at least need to put your SoB at the end of the chain. :-)

[...]

> @@ -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;
> +	}

I didn't dig the full history so apologize if I missed anything.

IIUC this code basically only sets start.desired_tsc_khz to default_tsc_khz
w/o reading anything from params.desired_tsc_khz.

Actually IIRC params.desired_tsc_khz isn't used at all in this patch, except
it is defined in the userspace ABI structure.

Do we actually need it?  Since IIUC the userspace is supposed to use
KVM_SET_TSC_KHZ ioctl to set the kvm->arch.default_tsc_khz before
snp_launch_start() so here in snp_launch_start() we can just feed the
default_tsc_khz to SEV command. 

Btw, in fact, I was wondering whether this patch can even compile because
the 'desired_tsc_khz' was added to 'struct kvm_sev_snp_launch_start' but not
'struct sev_data_snp_launch_start', while the code:

	start.desired_tsc_khz = kvm->arch.default_tsc_khz;

indicates it is the latter which should have this desired_tsc_khz member.

Then I found it depends one commit that has already been merged to Sean's
kvm-x86 tree but not in upstream yet (nor Paolo's tree):

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

IMHO it would be helpful to somehow call this in the coverletter otherwise
other people may get confused.

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

[Nikunj A. Dadhania]

Thanks for the review.

On 7/1/2025 6:21 AM, Huang, Kai wrote:
> On Mon, 2025-06-30 at 16:14 +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.
>>
>> As the frequency needs to be set in the SNP_LAUNCH_START command, userspace
>> should set the frequency using the KVM_CAP_SET_TSC_KHZ VM ioctl instead of
> 				     ^
> 
> I believe you meant KVM_SET_TSC_KHZ ioctl?  Since I am not able to find
> KVM_CAP_SET_TSC_KHZ. :-)

Yes, will update.

> 
>> the VCPU ioctl. The desired_tsc_khz defaults to kvm->arch.default_tsc_khz.
> 
> IIRC the KVM_SET_TSC_KHZ ioctl updates the kvm->arch.default_tsc_khz, and
> the snp_launch_start() always just uses it.

Correct

> The last sentence is kinda confusing since it sounds like that
> desired_tsc_khz is used by the SEV command and it could have a different
> value from kvm->arch.default_tsc_khz.


start.desired_tsc_khz is indeed used as part of SNP_LAUNCH_START command.

How about something like the below:

"In case, user has not set the TSC Frequency, desired_tsc_khz will default to
the host tsc frequency saved in kvm->arch.default_tsc_khz"
 
>>
>> 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>
>> ---
>>
>> 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.
> 
> Well I guess you at least need to put your SoB at the end of the chain. :-)

Sure, will add.

> 
> [...]
> 
>> @@ -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;
>> +	}
> 
> I didn't dig the full history so apologize if I missed anything.
> 
> IIUC this code basically only sets start.desired_tsc_khz to default_tsc_khz
> w/o reading anything from params.desired_tsc_khz.
> 
> Actually IIRC params.desired_tsc_khz isn't used at all in this patch, except
> it is defined in the userspace ABI structure.
> 
> Do we actually need it?  Since IIUC the userspace is supposed to use
> KVM_SET_TSC_KHZ ioctl to set the kvm->arch.default_tsc_khz before
> snp_launch_start() so here in snp_launch_start() we can just feed the
> default_tsc_khz to SEV command. 
> 
> Btw, in fact, I was wondering whether this patch can even compile because
> the 'desired_tsc_khz' was added to 'struct kvm_sev_snp_launch_start' but not
> 'struct sev_data_snp_launch_start', while the code:
> 
> 	start.desired_tsc_khz = kvm->arch.default_tsc_khz;
> 
> indicates it is the latter which should have this desired_tsc_khz member.
> 
> Then I found it depends one commit that has already been merged to Sean's
> kvm-x86 tree but not in upstream yet (nor Paolo's tree):
> 
>   51a4273dcab3 ("KVM: SVM: Add missing member in SNP_LAUNCH_START command
> structure"
> 
> IMHO it would be helpful to somehow call this in the coverletter otherwise
> other people may get confused.

I did mention in the v7 change log that patches are rebased on kvm-x86/next.
Next time I will make it more explicit.

Regards
Nikunj

1. https://lore.kernel.org/kvm/Z9BOEtM6bm-ng68c@google.com/

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

[Huang, Kai]

> > 
> > > the VCPU ioctl. The desired_tsc_khz defaults to kvm->arch.default_tsc_khz.
> > 
> > IIRC the KVM_SET_TSC_KHZ ioctl updates the kvm->arch.default_tsc_khz, and
> > the snp_launch_start() always just uses it.
> 
> Correct
> 
> > The last sentence is kinda confusing since it sounds like that
> > desired_tsc_khz is used by the SEV command and it could have a different
> > value from kvm->arch.default_tsc_khz.
> 
> 
> start.desired_tsc_khz is indeed used as part of SNP_LAUNCH_START command.
> 
> How about something like the below:
> 
> "In case, user has not set the TSC Frequency, desired_tsc_khz will default to
> the host tsc frequency saved in kvm->arch.default_tsc_khz"

Hmm.. If user has set the TSC frequency, desired_tsc_khz will still be the
value in kvm->arch.default_tsc_khz.

Not intended to nitpicking here, but how about something like:

  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 it via
  the KVM_SET_TSC_KHZ ioctl before calling the SNP_LAUNCH_START ioctl.

> 
> > 
> > > @@ -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;
> > > +	}
> > 
> > I didn't dig the full history so apologize if I missed anything.
> > 
> > IIUC this code basically only sets start.desired_tsc_khz to default_tsc_khz
> > w/o reading anything from params.desired_tsc_khz.
> > 
> > Actually IIRC params.desired_tsc_khz isn't used at all in this patch, except
> > it is defined in the userspace ABI structure.
> > 
> > Do we actually need it?  Since IIUC the userspace is supposed to use
> > KVM_SET_TSC_KHZ ioctl to set the kvm->arch.default_tsc_khz before
> > snp_launch_start() so here in snp_launch_start() we can just feed the
> > default_tsc_khz to SEV command. 
> > 
> > Btw, in fact, I was wondering whether this patch can even compile because
> > the 'desired_tsc_khz' was added to 'struct kvm_sev_snp_launch_start' but not
> > 'struct sev_data_snp_launch_start', while the code:
> > 
> > 	start.desired_tsc_khz = kvm->arch.default_tsc_khz;
> > 
> > indicates it is the latter which should have this desired_tsc_khz member.
> > 
> > Then I found it depends one commit that has already been merged to Sean's
> > kvm-x86 tree but not in upstream yet (nor Paolo's tree):
> > 
> >   51a4273dcab3 ("KVM: SVM: Add missing member in SNP_LAUNCH_START command
> > structure"
> > 
> > IMHO it would be helpful to somehow call this in the coverletter otherwise
> > other people may get confused.
> 
> I did mention in the v7 change log that patches are rebased on kvm-x86/next.
> Next time I will make it more explicit.

So could you confirm that we don't need the new 'desired_tsc_khz' in 'struct
kvm_sev_snp_launch_start' as part of userspace ABI?

I think this is where I got confused at the beginning.

For explicitly calling out the 51a4273dcab3 as dependency, I guess it's
perhaps just me, so feel free to ignore.  Again, no intention of nitpicking
here.

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

[Nikunj A. Dadhania]

On 7/1/2025 11:26 AM, Huang, Kai wrote:
>>>
>>>> the VCPU ioctl. The desired_tsc_khz defaults to kvm->arch.default_tsc_khz.
>>>
>>> IIRC the KVM_SET_TSC_KHZ ioctl updates the kvm->arch.default_tsc_khz, and
>>> the snp_launch_start() always just uses it.
>>
>> Correct
>>
>>> The last sentence is kinda confusing since it sounds like that
>>> desired_tsc_khz is used by the SEV command and it could have a different
>>> value from kvm->arch.default_tsc_khz.
>>
>>
>> start.desired_tsc_khz is indeed used as part of SNP_LAUNCH_START command.
>>
>> How about something like the below:
>>
>> "In case, user has not set the TSC Frequency, desired_tsc_khz will default to
>> the host tsc frequency saved in kvm->arch.default_tsc_khz"
> 
> Hmm.. If user has set the TSC frequency, desired_tsc_khz will still be the
> value in kvm->arch.default_tsc_khz.
> 
> Not intended to nitpicking here, but how about something like:
> 
>   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 it via
>   the KVM_SET_TSC_KHZ ioctl before calling the SNP_LAUNCH_START ioctl.

Yes, this works as well.

>>
>>>
>>>> @@ -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;
>>>> +	}
>>>
>>> I didn't dig the full history so apologize if I missed anything.
>>>
>>> IIUC this code basically only sets start.desired_tsc_khz to default_tsc_khz
>>> w/o reading anything from params.desired_tsc_khz.
>>>
>>> Actually IIRC params.desired_tsc_khz isn't used at all in this patch, except
>>> it is defined in the userspace ABI structure.
>>>
>>> Do we actually need it?  Since IIUC the userspace is supposed to use
>>> KVM_SET_TSC_KHZ ioctl to set the kvm->arch.default_tsc_khz before
>>> snp_launch_start() so here in snp_launch_start() we can just feed the
>>> default_tsc_khz to SEV command. 
>>>
>>> Btw, in fact, I was wondering whether this patch can even compile because
>>> the 'desired_tsc_khz' was added to 'struct kvm_sev_snp_launch_start' but not
>>> 'struct sev_data_snp_launch_start', while the code:
>>>
>>> 	start.desired_tsc_khz = kvm->arch.default_tsc_khz;
>>>
>>> indicates it is the latter which should have this desired_tsc_khz member.
>>>
>>> Then I found it depends one commit that has already been merged to Sean's
>>> kvm-x86 tree but not in upstream yet (nor Paolo's tree):
>>>
>>>   51a4273dcab3 ("KVM: SVM: Add missing member in SNP_LAUNCH_START command
>>> structure"
>>>
>>> IMHO it would be helpful to somehow call this in the coverletter otherwise
>>> other people may get confused.
>>
>> I did mention in the v7 change log that patches are rebased on kvm-x86/next.
>> Next time I will make it more explicit.
> 
> So could you confirm that we don't need the new 'desired_tsc_khz' in 'struct
> kvm_sev_snp_launch_start' as part of userspace ABI?

I agree, we can drop it from userspace ABI.

> I think this is where I got confused at the beginning.
> 
> For explicitly calling out the 51a4273dcab3 as dependency, I guess it's
> perhaps just me, so feel free to ignore.  Again, no intention of nitpicking
> here.

Sure.

Thanks
Nikunj