[PATCH v2 0/4] Enable Secure TSC for SEV-SNP

[PATCH v2 0/4] 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 patchset is also available at:

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

and is based on kvm/next

Testing SecureTSC
-----------------

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

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

QEMU commandline SEV-SNP with SecureTSC:

  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:
----------
v2:
* Address cpufeatures comment from Boris
* Squashed Secure TSC enablement and setting frequency patch
* Set the default TSC KHz for proper calulation of guest offset/multiplier

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

Nikunj A Dadhania (3):
  x86/cpufeatures: Add SNP Secure TSC
  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             | 12 ++++++++++++
 arch/x86/kvm/svm/svm.h             | 14 +++++++++++++-
 include/linux/psp-sev.h            |  2 ++
 7 files changed, 53 insertions(+), 2 deletions(-)


base-commit: 43fb96ae78551d7bfa4ecca956b258f085d67c40
-- 
2.43.0

[PATCH v2 1/4] 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>
---
 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 508c0dad116b..921ed26b0be7 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -448,6 +448,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

Re: [PATCH v2 1/4] x86/cpufeatures: Add SNP Secure TSC

[Borislav Petkov]

On Mon, Feb 10, 2025 at 02:52:27PM +0530, 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>
> ---
>  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 508c0dad116b..921ed26b0be7 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -448,6 +448,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 */
> -- 

Acked-by: Borislav Petkov (AMD) <bp@alien8.de>

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 2/4] 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
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     |  2 ++
 arch/x86/kvm/svm/svm.c     |  1 +
 arch/x86/kvm/svm/svm.h     | 14 +++++++++++++-
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index e2fac21471f5..a04346068c60 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -289,6 +289,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 SVM_SEV_FEAT_INT_INJ_MODES		\
 	(SVM_SEV_FEAT_RESTRICTED_INJECTION |	\
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a2a794c32050..0a1fd5c034e2 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -849,6 +849,8 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	save->dr6  = svm->vcpu.arch.dr6;
 
 	save->sev_features = sev->vmsa_features;
+	if (snp_secure_tsc_enabled(vcpu->kvm))
+		set_msr_interception(&svm->vcpu, svm->msrpm, MSR_AMD64_GUEST_TSC_FREQ, 1, 1);
 
 	/*
 	 * Skip FPU and AVX setup with KVM_SEV_ES_INIT to avoid
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7640a84e554a..d7a0428aa2ae 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 9d7cdb8fbf87..8ef582c463e0 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;
@@ -385,6 +385,18 @@ static __always_inline bool sev_snp_guest(struct kvm *kvm)
 #define sev_snp_guest(kvm) false
 #endif
 
+static inline bool snp_secure_tsc_enabled(struct kvm *kvm)
+{
+#ifdef CONFIG_KVM_AMD_SEV
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+
+	return (sev->vmsa_features & SVM_SEV_FEAT_SECURE_TSC) &&
+	       !WARN_ON_ONCE(!sev_snp_guest(kvm));
+#else
+	return false;
+#endif
+}
+
 static inline bool ghcb_gpa_is_registered(struct vcpu_svm *svm, u64 val)
 {
 	return svm->sev_es.ghcb_registered_gpa == val;
-- 
2.43.0

[PATCH v2 3/4] 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, as such
writes are not expected. Log the error and return #GP to the guest.

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

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d7a0428aa2ae..929f35a2f542 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3161,6 +3161,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 
 		svm->tsc_aux = data;
 		break;
+	case MSR_IA32_TSC:
+		/*
+		 * If Secure TSC is enabled, KVM doesn't expect to receive
+		 * a VMEXIT for a TSC write, record the error and return a
+		 * #GP
+		 */
+		if (vcpu->arch.guest_state_protected && snp_secure_tsc_enabled(vcpu->kvm)) {
+			vcpu_unimpl(vcpu, "unimplemented IA32_TSC for secure tsc\n");
+			return 1;
+		}
+		break;
 	case MSR_IA32_DEBUGCTLMSR:
 		if (!lbrv) {
 			kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
-- 
2.43.0

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

[Tom Lendacky]

On 2/10/25 03:22, Nikunj A Dadhania wrote:
> Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
> writes are not expected. Log the error and return #GP to the guest.

Re-word this to make it a bit clearer about why this is needed. It is
expected that the guest won't write to MSR_IA32_TSC or, if it does, it
will ignore any writes to it and not exit to the HV. So this is catching
the case where that behavior is not occurring.

> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>  arch/x86/kvm/svm/svm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index d7a0428aa2ae..929f35a2f542 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3161,6 +3161,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  
>  		svm->tsc_aux = data;
>  		break;
> +	case MSR_IA32_TSC:
> +		/*
> +		 * If Secure TSC is enabled, KVM doesn't expect to receive
> +		 * a VMEXIT for a TSC write, record the error and return a
> +		 * #GP
> +		 */
> +		if (vcpu->arch.guest_state_protected && snp_secure_tsc_enabled(vcpu->kvm)) {

Does it matter if the VMSA has already been encrypted? Can this just be

  if (sev_snp_guest() && snp_secure_tsc_enabled(vcpu->kvm)) {

?

> +			vcpu_unimpl(vcpu, "unimplemented IA32_TSC for secure tsc\n");
> +			return 1;
> +		}
> +		break;
>  	case MSR_IA32_DEBUGCTLMSR:
>  		if (!lbrv) {
>  			kvm_pr_unimpl_wrmsr(vcpu, ecx, data);

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

[Nikunj A Dadhania]

Tom Lendacky <thomas.lendacky@amd.com> writes:

> On 2/10/25 03:22, Nikunj A Dadhania wrote:
>> Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
>> writes are not expected. Log the error and return #GP to the guest.
>
> Re-word this to make it a bit clearer about why this is needed. It is
> expected that the guest won't write to MSR_IA32_TSC or, if it does, it
> will ignore any writes to it and not exit to the HV. So this is catching
> the case where that behavior is not occurring.
>
Sure, will update.

>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> ---
>>  arch/x86/kvm/svm/svm.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index d7a0428aa2ae..929f35a2f542 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3161,6 +3161,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>  
>>  		svm->tsc_aux = data;
>>  		break;
>> +	case MSR_IA32_TSC:
>> +		/*
>> +		 * If Secure TSC is enabled, KVM doesn't expect to receive
>> +		 * a VMEXIT for a TSC write, record the error and return a
>> +		 * #GP
>> +		 */
>> +		if (vcpu->arch.guest_state_protected && snp_secure_tsc_enabled(vcpu->kvm)) {
>
> Does it matter if the VMSA has already been encrypted? Can this just be
>
>   if (sev_snp_guest() && snp_secure_tsc_enabled(vcpu->kvm)) {
>
> ?
>

QEMU initializes the IA32_TSC MSR to zero resulting in the below
error if I use the above.

qemu-system-x86_64: error: failed to set MSR 0x10 to 0x0
qemu-system-x86_64: ../target/i386/kvm/kvm.c:3849: kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.

Once the guest state is protected, we do not expect any writes from VMM.

Regards,
Nikunj

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

[Tom Lendacky]

On 2/11/25 02:24, Nikunj A Dadhania wrote:
> Tom Lendacky <thomas.lendacky@amd.com> writes:
> 
>> On 2/10/25 03:22, Nikunj A Dadhania wrote:
>>> Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
>>> writes are not expected. Log the error and return #GP to the guest.
>>
>> Re-word this to make it a bit clearer about why this is needed. It is
>> expected that the guest won't write to MSR_IA32_TSC or, if it does, it
>> will ignore any writes to it and not exit to the HV. So this is catching
>> the case where that behavior is not occurring.
>>
> Sure, will update.
> 
>>>
>>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>>> ---
>>>  arch/x86/kvm/svm/svm.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index d7a0428aa2ae..929f35a2f542 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -3161,6 +3161,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>>  
>>>  		svm->tsc_aux = data;
>>>  		break;
>>> +	case MSR_IA32_TSC:
>>> +		/*
>>> +		 * If Secure TSC is enabled, KVM doesn't expect to receive
>>> +		 * a VMEXIT for a TSC write, record the error and return a
>>> +		 * #GP
>>> +		 */
>>> +		if (vcpu->arch.guest_state_protected && snp_secure_tsc_enabled(vcpu->kvm)) {
>>
>> Does it matter if the VMSA has already been encrypted? Can this just be
>>
>>   if (sev_snp_guest() && snp_secure_tsc_enabled(vcpu->kvm)) {
>>
>> ?
>>
> 
> QEMU initializes the IA32_TSC MSR to zero resulting in the below
> error if I use the above.
> 
> qemu-system-x86_64: error: failed to set MSR 0x10 to 0x0
> qemu-system-x86_64: ../target/i386/kvm/kvm.c:3849: kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.

Should KVM be doing anything related to MSR_IA32_TSC for a Secure TSC
guest, even handling this Qemu write? That Qemu write takes it through
the kvm_synchronize_tsc() path, does it need to? I'm just wondering if
the Secure TSC HV support needs more handling of MSR_IA32_TSC (in both
set and get) than what's here. Thoughts?

Thanks,
Tom

> 
> Once the guest state is protected, we do not expect any writes from VMM.
> 
> Regards,
> Nikunj

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

[Tom Lendacky]

On 2/11/25 08:03, Tom Lendacky wrote:
> On 2/11/25 02:24, Nikunj A Dadhania wrote:
>> Tom Lendacky <thomas.lendacky@amd.com> writes:
>>
>>> On 2/10/25 03:22, Nikunj A Dadhania wrote:
>>>> Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
>>>> writes are not expected. Log the error and return #GP to the guest.
>>>
>>> Re-word this to make it a bit clearer about why this is needed. It is
>>> expected that the guest won't write to MSR_IA32_TSC or, if it does, it
>>> will ignore any writes to it and not exit to the HV. So this is catching
>>> the case where that behavior is not occurring.
>>>
>> Sure, will update.
>>
>>>>
>>>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>>>> ---
>>>>  arch/x86/kvm/svm/svm.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>> index d7a0428aa2ae..929f35a2f542 100644
>>>> --- a/arch/x86/kvm/svm/svm.c
>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>> @@ -3161,6 +3161,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>>>  
>>>>  		svm->tsc_aux = data;
>>>>  		break;
>>>> +	case MSR_IA32_TSC:
>>>> +		/*
>>>> +		 * If Secure TSC is enabled, KVM doesn't expect to receive
>>>> +		 * a VMEXIT for a TSC write, record the error and return a
>>>> +		 * #GP
>>>> +		 */
>>>> +		if (vcpu->arch.guest_state_protected && snp_secure_tsc_enabled(vcpu->kvm)) {
>>>
>>> Does it matter if the VMSA has already been encrypted? Can this just be
>>>
>>>   if (sev_snp_guest() && snp_secure_tsc_enabled(vcpu->kvm)) {
>>>
>>> ?
>>>
>>
>> QEMU initializes the IA32_TSC MSR to zero resulting in the below
>> error if I use the above.
>>
>> qemu-system-x86_64: error: failed to set MSR 0x10 to 0x0
>> qemu-system-x86_64: ../target/i386/kvm/kvm.c:3849: kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> 
> Should KVM be doing anything related to MSR_IA32_TSC for a Secure TSC
> guest, even handling this Qemu write? That Qemu write takes it through
> the kvm_synchronize_tsc() path, does it need to? I'm just wondering if

Ah, strike that, the write from Qemu is just ignored in this situation.
But doesn't that break existing support since we no longer call into
kvm_set_msr_common() for MSR_IA32_TSC? This looks like it needs to
invoke kvm_set_msr_common() if it isn't a Secure TSC guest.

Thanks,
Tom

> the Secure TSC HV support needs more handling of MSR_IA32_TSC (in both
> set and get) than what's here. Thoughts?
> 
> Thanks,
> Tom
> 
>>
>> Once the guest state is protected, we do not expect any writes from VMM.
>>
>> Regards,
>> Nikunj

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

[Nikunj A Dadhania]

Tom Lendacky <thomas.lendacky@amd.com> writes:

> On 2/11/25 08:03, Tom Lendacky wrote:
>> On 2/11/25 02:24, Nikunj A Dadhania wrote:
>>> Tom Lendacky <thomas.lendacky@amd.com> writes:
>>>
>>>> On 2/10/25 03:22, Nikunj A Dadhania wrote:
>
> Ah, strike that, the write from Qemu is just ignored in this situation.
> But doesn't that break existing support since we no longer call into
> kvm_set_msr_common() for MSR_IA32_TSC? This looks like it needs to
> invoke kvm_set_msr_common() if it isn't a Secure TSC guest.

Right, I will add that.

Regards
Nikunj

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

[Sean Christopherson]

On Mon, Feb 10, 2025, Tom Lendacky wrote:
> On 2/10/25 03:22, Nikunj A Dadhania wrote:
> > Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
> > writes are not expected. Log the error and return #GP to the guest.
> 
> Re-word this to make it a bit clearer about why this is needed. It is
> expected that the guest won't write to MSR_IA32_TSC or, if it does, it
> will ignore any writes to it and not exit to the HV. So this is catching
> the case where that behavior is not occurring.

Unless it's architectural impossible for KVM to modify MSR_IA32_TSC, I don't see
any reason for KVM to care.  If the guest wants to modify TSC, that's the guest's
prerogative.

If KVM _can't_ honor the write, then that's something else entirely, and the
changelog should pretty much write itself.

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

[Nikunj A Dadhania]

Sean Christopherson <seanjc@google.com> writes:

> On Mon, Feb 10, 2025, Tom Lendacky wrote:
>> On 2/10/25 03:22, Nikunj A Dadhania wrote:
>> > Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
>> > writes are not expected. Log the error and return #GP to the guest.
>> 
>> Re-word this to make it a bit clearer about why this is needed. It is
>> expected that the guest won't write to MSR_IA32_TSC or, if it does, it
>> will ignore any writes to it and not exit to the HV. So this is catching
>> the case where that behavior is not occurring.
>
> Unless it's architectural impossible for KVM to modify MSR_IA32_TSC, I don't see
> any reason for KVM to care.  If the guest wants to modify TSC, that's the guest's
> prerogative.
>
> If KVM _can't_ honor the write, then that's something else entirely, and the
> changelog should pretty much write itself.

How about the below changelog:

    KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled

    Secure TSC enabled SNP guests should not write to the TSC MSR. Any such
    writes should be identified and ignored by the guest kernel in the #VC
    handler. As a safety measure, detect and disallow writes to MSR_IA32_TSC by
    Secure TSC enabled guests, as these writes are not expected to reach the
    hypervisor. Log the error and return #GP to the guest.

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

Regards,
Nikunj

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

[Sean Christopherson]

On Wed, Feb 12, 2025, Nikunj A Dadhania wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Mon, Feb 10, 2025, Tom Lendacky wrote:
> >> On 2/10/25 03:22, Nikunj A Dadhania wrote:
> >> > Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
> >> > writes are not expected. Log the error and return #GP to the guest.
> >> 
> >> Re-word this to make it a bit clearer about why this is needed. It is
> >> expected that the guest won't write to MSR_IA32_TSC or, if it does, it
> >> will ignore any writes to it and not exit to the HV. So this is catching
> >> the case where that behavior is not occurring.
> >
> > Unless it's architectural impossible for KVM to modify MSR_IA32_TSC, I don't see
> > any reason for KVM to care.  If the guest wants to modify TSC, that's the guest's
> > prerogative.
> >
> > If KVM _can't_ honor the write, then that's something else entirely, and the
> > changelog should pretty much write itself.
> 
> How about the below changelog:
> 
>     KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled
> 
>     Secure TSC enabled SNP guests should not write to the TSC MSR. Any such

This is a host write, not a guest write.  What guest's "should" or should not do
is irrelevant.

>     writes should be identified and ignored by the guest kernel in the #VC

Again, I don't care what the guest does.  Talking about #VC just adds noise.
E.g. if the guest requests WRMSR emulation without ever doing WRMSR, there will
be no #VC.

>     handler. As a safety measure, detect and disallow writes to MSR_IA32_TSC by

No, KVM is not the trusted monitor.  "safety measure" makes it sound like KVM is
protecting the guest from a malicious VMM.  That is not KVM's responsibility.

>     Secure TSC enabled guests, as these writes are not expected to reach the
>     hypervisor. Log the error and return #GP to the guest.

Again, none of this ever says what actually happens if KVM tries to emulate a
write to MSR_IA32_TSC.  Based on what the APM says, the TSC fields in the control
area are ignored.  _That's_ what's relevant.

  The TSC value is first scaled with the GUEST_TSC_SCALE value from the VMSA and
  then is added to the VMSA GUEST_TSC_OFFSET value. The P0 frequency, TSC_RATIO
  (C001_0104h) and TSC_OFFSET (VMCB offset 50h) values are not used in the calculation.

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

[Nikunj A. Dadhania]

On 2/12/2025 7:34 PM, Sean Christopherson wrote:
> On Wed, Feb 12, 2025, Nikunj A Dadhania wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>>     Secure TSC enabled guests, as these writes are not expected to reach the
>>     hypervisor. Log the error and return #GP to the guest.
> 
> Again, none of this ever says what actually happens if KVM tries to emulate a
> write to MSR_IA32_TSC.  Based on what the APM says, the TSC fields in the control
> area are ignored.  _That's_ what's relevant.
> >   The TSC value is first scaled with the GUEST_TSC_SCALE value from the VMSA and
>   then is added to the VMSA GUEST_TSC_OFFSET value. The P0 frequency, TSC_RATIO
>   (C001_0104h) and TSC_OFFSET (VMCB offset 50h) values are not used in the calculation.

Sure, I will update the commit.

Thanks,
Nikunj

subscribe

[list archives]

[PATCH v2 4/4] 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          | 20 ++++++++++++++++++++
 include/linux/psp-sev.h         |  2 ++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 9e75da97bce0..8e090cab9aa0 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -836,7 +836,8 @@ struct kvm_sev_snp_launch_start {
 	__u64 policy;
 	__u8 gosvw[16];
 	__u16 flags;
-	__u8 pad0[6];
+	__u32 desired_tsc_khz;
+	__u8 pad0[2];
 	__u64 pad1[4];
 };
 
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 0a1fd5c034e2..0edd473749f7 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2228,6 +2228,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) {
@@ -2949,6 +2963,9 @@ 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);
+
+		if (cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
+			kvm_cpu_cap_set(X86_FEATURE_SNP_SECURE_TSC);
 	}
 }
 
@@ -3071,6 +3088,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)
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 903ddfea8585..613a8209bed2 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

Re: [PATCH v2 4/4] KVM: SVM: Enable Secure TSC for SNP guests

[Tom Lendacky]

On 2/10/25 03:22, 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          | 20 ++++++++++++++++++++
>  include/linux/psp-sev.h         |  2 ++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 9e75da97bce0..8e090cab9aa0 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -836,7 +836,8 @@ struct kvm_sev_snp_launch_start {
>  	__u64 policy;
>  	__u8 gosvw[16];
>  	__u16 flags;
> -	__u8 pad0[6];
> +	__u32 desired_tsc_khz;

This will put the __u32 field misaligned in the struct. You should
probably move the now 2-byte pad0 field to before the desired_tsc_khz field.

> +	__u8 pad0[2];
>  	__u64 pad1[4];
>  };
>  
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0a1fd5c034e2..0edd473749f7 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2228,6 +2228,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) {
> @@ -2949,6 +2963,9 @@ 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);
> +
> +		if (cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
> +			kvm_cpu_cap_set(X86_FEATURE_SNP_SECURE_TSC);
>  	}
>  }
>  
> @@ -3071,6 +3088,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)
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 903ddfea8585..613a8209bed2 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 */

Shouldn't there be a separate fix for this before this patch? The
desired_tsc_freq should have been here all along, so before this patch,
the gosvw field is off by 4 bytes with sev_data_snp_launch_start not
being large enough compared to what the firmware is accessing, right?

Thanks,
Tom

>  	u8 gosvw[16];				/* In */
>  } __packed;
>  

Re: [PATCH v2 4/4] KVM: SVM: Enable Secure TSC for SNP guests

[Nikunj A Dadhania]

Tom Lendacky <thomas.lendacky@amd.com> writes:

> On 2/10/25 03:22, 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          | 20 ++++++++++++++++++++
>>  include/linux/psp-sev.h         |  2 ++
>>  3 files changed, 24 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
>> index 9e75da97bce0..8e090cab9aa0 100644
>> --- a/arch/x86/include/uapi/asm/kvm.h
>> +++ b/arch/x86/include/uapi/asm/kvm.h
>> @@ -836,7 +836,8 @@ struct kvm_sev_snp_launch_start {
>>  	__u64 policy;
>>  	__u8 gosvw[16];
>>  	__u16 flags;
>> -	__u8 pad0[6];
>> +	__u32 desired_tsc_khz;
>
> This will put the __u32 field misaligned in the struct. You should
> probably move the now 2-byte pad0 field to before the desired_tsc_khz field.
>

Sure, will update.

>> +	__u8 pad0[2];
>>  	__u64 pad1[4];
>>  };
>>  
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 0a1fd5c034e2..0edd473749f7 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2228,6 +2228,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) {
>> @@ -2949,6 +2963,9 @@ 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);
>> +
>> +		if (cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
>> +			kvm_cpu_cap_set(X86_FEATURE_SNP_SECURE_TSC);
>>  	}
>>  }
>>  
>> @@ -3071,6 +3088,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)
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 903ddfea8585..613a8209bed2 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 */
>
> Shouldn't there be a separate fix for this before this patch? The
> desired_tsc_freq should have been here all along, so before this patch,
> the gosvw field is off by 4 bytes with sev_data_snp_launch_start not
> being large enough compared to what the firmware is accessing, right?

Yes, this should have been part of SNP series, I will separate this
change in a new patch with Fixes + stable tag.

Thanks
Nikunj