| 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
| ||
|
Message-ID: <aXJRWtdzjcb8_APA@google.com>
Date: Thu, 22 Jan 2026 08:33:30 -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 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.
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
>
Powered by blists - more mailing lists