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

[Nikunj A Dadhania <nikunj@amd.com>]

Sean Christopherson <seanjc@google.com> writes:

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

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

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

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

With MSR interception disabled: vCPU will directly generate #GP

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

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

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

Regards,
Nikunj