On Thu, Jan 22, 2026 at 8:33 AM Sean Christopherson <[email protected]> wrote:
>
> On Wed, Jan 21, 2026, Jim Mattson wrote:
> > Introduce amd_pmu_dormant_hg_event(), which determines whether an AMD PMC
> > should be dormant (i.e. not count) based on the guest's Host-Only and
> > Guest-Only event selector bits and the current vCPU state.
> >
> > Update amd_pmu_set_eventsel_hw() to clear the event selector's enable bit
> > when the event is dormant.
> >
> > Signed-off-by: Jim Mattson <[email protected]>
> > ---
> > arch/x86/include/asm/perf_event.h | 2 ++
> > arch/x86/kvm/svm/pmu.c | 23 +++++++++++++++++++++++
> > 2 files changed, 25 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/perf_event.h
> > b/arch/x86/include/asm/perf_event.h
> > index 0d9af4135e0a..7649d79d91a6 100644
> > --- a/arch/x86/include/asm/perf_event.h
> > +++ b/arch/x86/include/asm/perf_event.h
> > @@ -58,6 +58,8 @@
> > #define AMD64_EVENTSEL_INT_CORE_ENABLE (1ULL << 36)
> > #define AMD64_EVENTSEL_GUESTONLY (1ULL << 40)
> > #define AMD64_EVENTSEL_HOSTONLY (1ULL << 41)
> > +#define AMD64_EVENTSEL_HG_ONLY \
>
> I would strongly prefer to avoid the HG acronym, as it's not immediately
> obvious
> that it's HOST_GUEST, and avoiding long lines even with the full HOST_GUEST is
> pretty easy.
In this instance, I'm happy to make the suggested change, but I think
your overall distaste for HG_ONLY is unwarranted.
These bits are documented in the APM as:
> HG_ONLY (Host/Guest Only)—Bits 41:40, read/write
> The name should also have "MASK" at the end to make it more obvious this is a
> multi-flag macro, i.e. not a single-flag value. Otherwise the intent and thus
> correctness of code like this isn't obvious:
>
> if (eventsel & AMD64_EVENTSEL_HG_ONLY)
>
> How about AMD64_EVENTSEL_HOST_GUEST_MASK?
Sure.
> > + (AMD64_EVENTSEL_HOSTONLY | AMD64_EVENTSEL_GUESTONLY)
> >
> > #define AMD64_EVENTSEL_INT_CORE_SEL_SHIFT 37
> > #define AMD64_EVENTSEL_INT_CORE_SEL_MASK \
> > diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> > index 33c139b23a9e..f619417557f9 100644
> > --- a/arch/x86/kvm/svm/pmu.c
> > +++ b/arch/x86/kvm/svm/pmu.c
> > @@ -147,10 +147,33 @@ static int amd_pmu_get_msr(struct kvm_vcpu *vcpu,
> > struct msr_data *msr_info)
> > return 1;
> > }
> >
> > +static bool amd_pmu_dormant_hg_event(struct kvm_pmc *pmc)
>
> I think I would prefer to flip the polarity, even though the only caller would
> then need to invert the return value. Partly because I think we can come up
> with
> a more intuitive name, partly because it'll make the last check in particular
> more intutive, i.e. IMO, checking "guest == guest"
>
> return !!(hg_only & AMD64_EVENTSEL_GUESTONLY) == is_guest_mode(vcpu);
>
> is more obvious than checking "host == guest":
>
> return !!(hg_only & AMD64_EVENTSEL_GUESTONLY) == is_guest_mode(vcpu);
>
> Maybe amd_pmc_is_active() or amd_pmc_counts_in_current_mode()?
I think amd_pmc_is_active() is a much stronger statement, implying
that both enable bits are also set.
Similarly, amd_pmc_counts_in_current_mode() sounds like it looks at
OS/USR bits as well.
I'll see if I can think of a better name that isn't misleading. I
actually went with this polarity because of the naming problem. But, I
agree that the reverse polarity is marginally better.
> > +{
> > + u64 hg_only = pmc->eventsel & AMD64_EVENTSEL_HG_ONLY;
> > + struct kvm_vcpu *vcpu = pmc->vcpu;
> > +
> > + if (hg_only == 0)
>
> !hg_only
Now, you're just being petty. But, okay.
> In the spirit of avoiding the "hg" acronym, what if we do something like this?
>
> const u64 HOST_GUEST_MASK = AMD64_EVENTSEL_HOST_GUEST_MASK;
Ugh. No. You can't both prefer the longer name and yet avoid it like
the plague. If you need to introduce a shorter alias, the longer name
is a bad choice.
> struct kvm_vcpu *vcpu = pmc->vcpu;
> u64 eventsel = pmc->eventsel;
>
> /*
> * PMCs count in both host and guest if neither {HOST,GUEST}_ONLY
> flags
> * are set, or if both flags are set.
> */
> if (!(eventsel & HOST_GUEST_MASK) ||
> ((eventsel & HOST_GUEST_MASK) == HOST_GUEST_MASK))
> return true;
>
> /* {HOST,GUEST}_ONLY bits are ignored when SVME is clear. */
> if (!(vcpu->arch.efer & EFER_SVME))
> return true;
>
> return !!(eventsel & AMD64_EVENTSEL_GUESTONLY) == is_guest_mode(vcpu);
>
> > + /* Not an HG_ONLY event */
>
> Please don't put comments inside single-line if-statements. 99% of the time
> it's easy to put the comment outside of the if-statement, and doing so
> encourages
> a more verbose comment and avoids a "does this if-statement need curly-braces"
> debate.
There is no debate. A comment is not a statement. But, okay.
> > + return false;
> > +
> > + if (!(vcpu->arch.efer & EFER_SVME))
> > + /* HG_ONLY bits are ignored when SVME is clear */
> > + return false;
> > +
> > + /* Always active if both HG_ONLY bits are set */
> > + if (hg_only == AMD64_EVENTSEL_HG_ONLY)
>
> I vote to check this condition at the same time !hg_only is checked. From a
> *very* pedantic perspective, one could argue it's "wrong" to check the bits
> when
> SVME=0, but the purpose of the helper is to detect if the PMC is active or
> not.
> Precisely following the architectural behavior is unnecessary.
Even I am not that pedantic.
> > + return false;
> > +
> > + return !!(hg_only & AMD64_EVENTSEL_HOSTONLY) == is_guest_mode(vcpu);
> > +}
> > +
> > static void amd_pmu_set_eventsel_hw(struct kvm_pmc *pmc)
> > {
> > pmc->eventsel_hw = (pmc->eventsel & ~AMD64_EVENTSEL_HOSTONLY) |
> > AMD64_EVENTSEL_GUESTONLY;
> > +
> > + if (amd_pmu_dormant_hg_event(pmc))
> > + pmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
> > }
> >
> > static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data
> > *msr_info)
> > --
> > 2.52.0.457.g6b5491de43-goog
> >
