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

[Sean Christopherson <seanjc@google.com>]

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

Sure, that'd be wonderful!

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

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