Re: [PATCH v4 4/6] KVM: x86/pmu: Re-evaluate Host-Only/Guest-Only on nested SVM transitions

[Yosry Ahmed <yosry@kernel.org>]

On Thu, Apr 09, 2026 at 02:21:14PM -0700, Sean Christopherson wrote:
> On Thu, Apr 09, 2026, Sean Christopherson wrote:
> > On Thu, Apr 09, 2026, Jim Mattson wrote:
> > > On Thu, Apr 9, 2026 at 10:48 AM Sean Christopherson <seanjc@google.com> wrote:
> > > > On Thu, Apr 09, 2026, Jim Mattson wrote:
> > > > > > > In general, this deferral is misguided. The G/H bits should be
> > > > > > > re-evaluated before we call kvm_pmu_instruction_retired() for an
> > > > > > > emulated instruction.
> > > > > > >
> > > > > > > > ...
> > > > > > > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > > > > > > > index f1c29ac306917..966e4138308f6 100644
> > > > > > > > --- a/arch/x86/kvm/x86.h
> > > > > > > > +++ b/arch/x86/kvm/x86.h
> > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > >  #include "kvm_cache_regs.h"
> > > > > > > >  #include "kvm_emulate.h"
> > > > > > > >  #include "cpuid.h"
> > > > > > > > +#include "pmu.h"
> > > > > > > >
> > > > > > > >  #define KVM_MAX_MCE_BANKS 32
> > > > > > > >
> > > > > > > > @@ -152,6 +153,8 @@ static inline void enter_guest_mode(struct kvm_vcpu *vcpu)
> > > > > > > >  {
> > > > > > > >         vcpu->arch.hflags |= HF_GUEST_MASK;
> > > > > > > >         vcpu->stat.guest_mode = 1;
> > > > > > > > +
> > > > > > > > +       kvm_pmu_handle_nested_transition(vcpu);
> > > > > > > >  }
> > > > > > >
> > > > > > > This happens too late for VMRUN, since we have already called
> > > > > > > kvm_pmu_instruction_retired() via kvm_skip_emulated_instruction(), and
> > > > > > > VMRUN counts as a *guest* instruction.
> > > > > >
> > > > > > It's just VMRUN that's problematic though, correct?  I.e. the scheme as a whole
> > > > > > is fine, we just need to special case VMRUN due to SVM's erratum^Warchitecture.
> > > > > > Alternatively, maybe we could get AMD to document the silly VMRUN behavior as an
> > > > > > erratum, then we could claim KVM is architecturally superior. :-D
> > > > >
> > > > > Here, it's just VMRUN. Above, it's WRMSR(EFER).
> > > >
> > > > But clearing EFER.SVME while in the guest generates architecturally undefined
> > > > behavior.  I don't see any reason to complicate PMU virtualization for that
> > > > scenario, especially now that KVM synthesizes triple fault for L1.
> > > 
> > > L1 can clear the virtual EFER.SVME. That is well-defined.
> > 
> > Gah, I forgot that the H/G bits are ignored when EFER.SVME=0.  That's really
> > annoying.
> 
> What do you think about having two flavors of kvm_pmu_handle_nested_transition()?
> One that defers via a request, and a "special" (SVM-only?) version that does
> direct updates.
> 
> Poking into PMU state in arbitrary contexts makes me nervous.  E.g. when calling
> svm_leave_nested(), odds are good EFER isn't even correct, and the update *needs*
> to be deferred.

Hmm is it really that bad?

- In the emulated VMRUN and #VMEXIT paths, EFER.SVME should be set in
  both L1 and L2, so it should be fine.

- In the restore path entering guest mode, EFER.SVME should also be set
  in both L1 and L2.

So I think svm_leave_nested() is the only interesting case:

- In the restore path, svm_leave_nested() should only be called if the
  CPU is in guest mode and EFER.SVME is set in both L1 and L2.

- In the EFER update path, L1 will get a shutdown if we forcefully leave
  nested anyway, unless userspace is setting state. Either way, the
  value of EFER should be correct and valid to use to update the PMU
  here.

- In the vCPU free path, it shouldn't really matter, but the value of
  EFER should still be correct.

So I *think* generally the value of EFER should be fine to use. The
other inputs are is_guest_mode() and eventsel. In both cases we should
just make sure to update the PMU *after* updating the state.

So I think we'd end up with something similar to Jim's v2:
https://lore.kernel.org/kvm/20260129232835.3710773-1-jmattson@google.com/

We will directly re-evaluate eventsel_hw on nested transitions, EFER
updates, and PMU MSR updates -- without deferring anything.

We'd still need to make other changes:
- Always disable the PMC if EFER.SVME is clear and either H/G bit (or
  both) is set.

- Handle VMRUN correctly. I was going to suggest just moving the call to
  kvm_skip_emulated_instruction() to the end of the function, but that
  doesn't handle the case where we inject #VMEXIT(INVALID) due to a
  VMRUN failure (e.g. consistency checks, loading CR3, etc).

  I am actually not sure if the instruction should count in host or
  guest mode in this case. Logically, we never entered the guest, so
  perhaps counting it in host mode is the right thing to do? I think
  we'll also need to test what HW does.

  Honestly, it would be a lot easier of someone from AMD could just tell
  us these things :)

  Basically:
  - Does the PMU generally count based on processor state (e.g. guest
    mode, EFER.SVME) before or after instruction retirement?
  - A successful VMRUN will be counted in guest mode, what about a
    failed VMRUN that produces #VMEXIT(INVALID)?

> I definitely don't love having two separate update mechanisms, but it seems like
> the safest option in this case.

Same here, and I like the deferred handling, but to Jim's point I think
we can use it anywhere :/