linux-kernel - Re: [PATCH 2/6] KVM: x86/pmu: Disable HG_ONLY events as appropriate for current vCPU state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aXK4DzweZLy9O8n1@google.com>
Date: Thu, 22 Jan 2026 15:51:43 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Jim Mattson <jmattson@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Peter Zijlstra <peterz@...radead.org>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, 
	James Clark <james.clark@...aro.org>, Shuah Khan <shuah@...nel.org>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 2/6] KVM: x86/pmu: Disable HG_ONLY events as appropriate
 for current vCPU state

On Thu, Jan 22, 2026, Jim Mattson wrote:
> On Thu, Jan 22, 2026 at 8:33 AM Sean Christopherson <seanjc@...gle.com> 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 <jmattson@...gle.com>
> > > ---
> > >  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

Ugh, stupid APM.  That makes me hate it a little less, but still, ugh.

> > 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.

Ooh, good point.

> Similarly, amd_pmc_counts_in_current_mode() sounds like it looks at
> OS/USR bits as well.

Yeah, I didn't like that collision either.  :-/

> 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.

Eh, that's a very standard kernel style thing.

> > 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.

IMO, there's a big difference between a global macro that may be read in a variety
of contexts, and a variable that's scoped to a function and consumed within a few
lines of its definition.

That said, I'm definitely open to other ways to write this code that don't require
a local const, it's HG_ONLY that I really dislike.

> >         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.

LOL, dollars to donuts says I can find someone to debate you on the "correct"
style. :-D