Re: [PATCH v5 09/44] perf/x86: Switch LVTPC to/from mediated PMI vector on guest load/put context

Sean Christopherson
2025-11-19T21:31:27Z

On Mon, Aug 18, 2025, Sean Christopherson wrote:
> On Mon, Aug 18, 2025, Peter Zijlstra wrote:
> > On Fri, Aug 15, 2025 at 08:55:25AM -0700, Sean Christopherson wrote:
> > > On Fri, Aug 15, 2025, Sean Christopherson wrote:
> > > > On Fri, Aug 15, 2025, Peter Zijlstra wrote:
> > > So if we're confident that switching the host LVTPC outside of
> > > perf_{load,put}_guest_context() is functionally safe, I'm a-ok with it.
> >
> > Let me see. So the hardware sets Masked when it raises the interrupt.
> >
> > The interrupt handler clears it from software -- depending on uarch in 3
> > different places:
> > 1) right at the start of the PMI
> > 2) in the middle, right before enabling the PMU (writing global control)
> > 3) at the end of the PMI
> >
> > the various changelogs adding that code mention spurious PMIs and
> > malformed PEBS records.
> >
> > So the fun all happens when the guest is doing PMI and gets a VM-exit
> > while still Masked.
> >
> > At that point, we can come in and completely rewrite the PMU state,
> > reroute the PMI and enable things again. Then later, we 'restore' the
> > PMU state, re-set LVTPC masked to the guest interrupt and 'resume'.
> >
> > What could possibly go wrong :/ Kan, I'm assuming, but not knowing, that
> > writing all the PMU MSRs is somehow serializing state sufficient to not
> > cause the above mentioned fails? Specifically, clearing PEBS_ENABLE
> > should inhibit those malformed PEBS records or something? What if the
> > host also has PEBS and we don't actually clear the bit?
> >
> > The current order ensures we rewrite LVTPC when global control is unset;
> > I think we want to keep that.
>
> Yes, for sure.
>
> > While staring at this, I note that perf_load_guest_context() will clear
> > global ctrl, clear all the counter programming, and re-enable an empty
> > pmu. Now, an empty PMU should result in global control being zero --
> > there is nothing run after all.
> >
> > But then kvm_mediated_pmu_load() writes an explicit 0 again. Perhaps
> > replace this with asserting it is 0 instead?
>
> Yeah, I like that idea, a lot. This?
>
> perf_load_guest_context();
>
> /*
> * Sanity check that "loading" guest context disabled all counters, as
> * modifying the LVTPC while host perf is active will cause explosions,
> * as will loading event selectors and PMCs with guest values.
> *
> * VMX will enable/disable counters at VM-Enter/VM-Exit by atomically
> * loading PERF_GLOBAL_CONTROL. SVM effectively performs the switch by
> * configuring all events to be GUEST_ONLY.
> */
> WARN_ON_ONCE(rdmsrq(kvm_pmu_ops.PERF_GLOBAL_CTRL));

This doesn't actually work, because perf_load_guest_context() doesn't guarantee
PERF_GLOBAL_CTRL is '0', it only guarantees all events are disabled. E.g. if
there are no perf events, perf_load_guest_context() is one big nop (I think).

And while it might seem reasonable to expect PERF_GLOBAL_CTRL to be '0' if
there are no perf events, that doesn't hold true today. E.g. amd_pmu_reload_virt()
unconditionally sets all supported MSR_AMD64_PERF_CNTR_GLOBAL_CTL bits.

I'm sure we could massage perf to really truly ensure PERF_GLOBAL_CTRL is '0',
but I don't see any value in explicitly doing that in perf_load_guest_context()
(versus simply doing it in KVM), and I would rather not play whack-a-mole in perf
as part of this series.

So unless someone really, really wants to lean on perf to clear PERF_GLOBAL_CTRL,
I'll go with this:

/*
* Explicitly clear PERF_GLOBAL_CTRL, as "loading" the guest's context
* disables all individual counters (if any were enabled), but doesn't
* globally disable the entire PMU. Loading event selectors and PMCs
* with guest values while PERF_GLOBAL_CTRL is non-zero will generate
* unexpected events and PMIs.
*
* VMX will enable/disable counters at VM-Enter/VM-Exit by atomically
* loading PERF_GLOBAL_CONTROL. SVM effectively performs the switch by
* configuring all events to be GUEST_ONLY. Clear PERF_GLOBAL_CONTROL
* even for SVM to minimize the damage if a perf event is left enabled,
* and to ensure a consistent starting state.
*/
wrmsrq(kvm_pmu_ops.PERF_GLOBAL_CTRL, 0);