Re: [PATCH v5 07/13] KVM: x86/pmu: Disable counters based on Host-Only/Guest-Only bits in SVM

[Sean Christopherson]

On Tue, May 05, 2026, Yosry Ahmed wrote:
> On Tue, May 5, 2026 at 11:11 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > So I think we actually want to handle this in pmc_is_locally_enabled(), because
> > the host/guest bits are "local" controls.  One option would be to add the guest/host
> > masks as constants in kvm_pmu_ops, and bleed the logic into pmc_is_locally_enabled(),
> > e.g. to avoid the CALL+RET overhead.  But if make the callback a "negative", then
> > we can make the static call OPTIONAL_RET0, which will turn the call into a glorified
> > nop for everything except AMD with a mediated PMU.  E.g.
> >
> > diff --git arch/x86/kvm/pmu.h arch/x86/kvm/pmu.h
> > index 0925246731cb..d8ce0938fcbe 100644
> > --- arch/x86/kvm/pmu.h
> > +++ arch/x86/kvm/pmu.h
> > @@ -190,7 +190,8 @@ static inline bool pmc_is_locally_enabled(struct kvm_pmc *pmc)
> >                                         pmc->idx - KVM_FIXED_PMC_BASE_IDX) &
> >                                         (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER);
> >
> > -       return pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE;
> > +       return (pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE) &&
> > +              !kvm_pmu_call(pmc_is_locally_disabled(pmc));
> 
> We still get the overhead on AMD with mediated PMU enabled, but more
> importantly, I am not sure what pmc_is_locally_disabled() would test
> for here? Do we re-check EFER, guest mode, etc to figure it out? I
> don't think this is what you mean as it would be redundant, but I am
> not sure what else.

Yep, that's exactly what I mean.

> Did you see my other replies and code snippet tracking disabling
> reasons? I think the code snippet would still work, I just need to
> move the pmc_is_nested_disabled() check into pmc_is_locally_enabled().

I did.  IMO, all of what you proposed is an optimization to avoid the "costly"
checks at the time of pmc_is_locally_enabled().  In quotes because I don't think
the _overall_ cost is actually all that high.  pmc_is_locally_enabled() is only
called in relatively slow paths, and my guess is the CALL+RET (or untrained RET,
ugh) is probably more expensive than the logic itself.

The very nice side effect of incorporating the logic into pmc_is_locally_enabled()
is that I _think_ we can drop kvm_pmu_ops.reprogram_counters(), because
amd_mediated_pmu_handle_host_guest_bits() will instead be:

  static bool amd_pmc_is_locally_disabled(struct kvm_pmc *pmc)
  {
	struct kvm_pmu *pmu = pmc_to_pmu(pmc);
	struct kvm_vcpu *vcpu = pmu_to_vcpu(pmu);
	u64 host_guest_bits;

	/* Common code is supposed to check the common enable bit. */
	if (WARN_ON_ONCE(!(pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE)))
		return false;

	/*
	 * If both bits are cleared, always keep the counter enabled. Otherwise,
	 * counter enablement needs to be re-evaluated on every nested
	 * transition (and EFER.SVME change).
	 */
	host_guest_bits = pmc->eventsel & AMD64_EVENTSEL_HOST_GUEST_MASK;
	if (!host_guest_bits)
		return true;

	/* If either bit is set and EFER.SVME=0, the counter is disabled. */
	if (!(vcpu->arch.efer & EFER_SVME))
		return false;

	if (host_guest_bits == AMD64_EVENTSEL_HOST_GUEST_MASK)
		return true;

	return !!(host_guest_bits & AMD64_EVENTSEL_GUESTONLY) == is_guest_mode(vcpu);
  }


reprogram_pmcs_on_nested_transitions would need to be handled somewhere else, but
(a) that's probably the correct approach anyways (hook writes to the eventsel)
and (b) is _also_ an optimization, because KVM can start with the naive approach
of always reprogramming counters on nested transitions (if guest/host bits are
supported).

And if we're clever, we can optimize pmc_is_locally_enabled() by putting
reprogram_pmcs_on_nested_transitions in kvm_pmu, e.g. as something like
pmc_has_mode_specific_enables, and then doing:

	if (!(pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE))
		return false;

	if (!test_bit(pmc->idx, &pmu->pmc_has_mode_specific_enables))
		return true;

	return !kvm_pmu_call(pmc_is_locally_disabled(pmc));