1. Patchew
  2. linux
  3. [PATCH 0/6] KVM: x86/pmu: Add support for AMD HG_ONLY bits
  4. View patch

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

Jim Mattson posted 6 patches 2 weeks, 3 days ago
[PATCH 2/6] KVM: x86/pmu: Disable HG_ONLY events as appropriate for current vCPU state
Posted by Jim Mattson 2 weeks, 3 days ago 
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@google.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				\
+	(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)
+{
+	u64 hg_only = pmc->eventsel & AMD64_EVENTSEL_HG_ONLY;
+	struct kvm_vcpu *vcpu = pmc->vcpu;
+
+	if (hg_only == 0)
+		/* Not an HG_ONLY event */
+		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)
+		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
Re: [PATCH 2/6] KVM: x86/pmu: Disable HG_ONLY events as appropriate for current vCPU state
Posted by Sean Christopherson 2 weeks, 3 days ago 
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@google.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.

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?

> +	(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()?

> +{
> +	u64 hg_only = pmc->eventsel & AMD64_EVENTSEL_HG_ONLY;
> +	struct kvm_vcpu *vcpu = pmc->vcpu;
> +
> +	if (hg_only == 0)

!hg_only

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

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

> +		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
>
Re: [PATCH 2/6] KVM: x86/pmu: Disable HG_ONLY events as appropriate for current vCPU state
Posted by Jim Mattson 2 weeks, 2 days ago 
On Thu, Jan 22, 2026 at 8:33 AM Sean Christopherson <seanjc@google.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@google.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

> 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
> >
Re: [PATCH 2/6] KVM: x86/pmu: Disable HG_ONLY events as appropriate for current vCPU state
Posted by Sean Christopherson 2 weeks, 2 days ago 
On Thu, Jan 22, 2026, Jim Mattson wrote:
> On Thu, Jan 22, 2026 at 8:33 AM Sean Christopherson <seanjc@google.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@google.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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.