On Fri, Aug 11, 2023 at 02:52:47PM -0700, Sean Christopherson wrote:
> Date: Fri, 11 Aug 2023 14:52:47 -0700
> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> Subject: Re: [PATCH] KVM: x86/pmu: Manipulate FIXED_CTR_CTRL MSR with macros
>
> On Tue, Aug 08, 2023, Dapeng Mi wrote:
> > Magic numbers are used to manipulate the bit fields of
> > FIXED_CTR_CTRL MSR. This is not read-friendly and use macros to replace
> > these magic numbers to increase the readability.
> >
> > Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
> > ---
> > arch/x86/include/asm/perf_event.h | 2 ++
> > arch/x86/kvm/pmu.c | 16 +++++++---------
> > arch/x86/kvm/pmu.h | 10 +++++++---
> > 3 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> > index 85a9fd5a3ec3..018441211af1 100644
> > --- a/arch/x86/include/asm/perf_event.h
> > +++ b/arch/x86/include/asm/perf_event.h
> > @@ -38,6 +38,8 @@
> > #define INTEL_FIXED_0_USER (1ULL << 1)
> > #define INTEL_FIXED_0_ANYTHREAD (1ULL << 2)
> > #define INTEL_FIXED_0_ENABLE_PMI (1ULL << 3)
> > +#define INTEL_FIXED_0_ENABLE \
> > + (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER)
>
> I vote to omit INTEL_FIXED_0_ENABLE and open code the "KERNEL | USER" logic in
> the one place that uses it. I dislike macros that sneakily cover multiple bits,
> i.e. don't have a name that strongly suggests a multi-bit masks. It's too easy
> to misread the code, especially for readers that aren't familiar with PMCs, e.g.
> in the usage below, it's not at all obvious that the "in use" check will evaluate
> true if the PMC is enabled for the kernel *or* user.
>
Ok, you convinced me. I would drop this change. Thanks.
> > #define HSW_IN_TX (1ULL << 32)
> > #define HSW_IN_TX_CHECKPOINTED (1ULL << 33)
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index edb89b51b383..03fb6b4bca2c 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -418,13 +418,12 @@ static void reprogram_counter(struct kvm_pmc *pmc)
> > printk_once("kvm pmu: pin control bit is ignored\n");
> >
> > if (pmc_is_fixed(pmc)) {
> > - fixed_ctr_ctrl = fixed_ctrl_field(pmu->fixed_ctr_ctrl,
> > - pmc->idx - INTEL_PMC_IDX_FIXED);
> > - if (fixed_ctr_ctrl & 0x1)
> > + fixed_ctr_ctrl = pmu_fixed_ctrl_field(pmu, pmc->idx);
> > + if (fixed_ctr_ctrl & INTEL_FIXED_0_KERNEL)
> > eventsel |= ARCH_PERFMON_EVENTSEL_OS;
>
> Please split this into two patches, one to do a straight replacement of literals
> with existing macros, and one to add pmu_fixed_ctrl_field(). Using existing
> macros is a no-brainer, but I'm less convinced that pmu_fixed_ctrl_field() is
> a good idea, e.g. both pmu_fixed_ctrl_field() and fixed_ctrl_field() take "idx",
> but use a different base. That is bound to cause problems.
Ok. It makes sense. It may be a good thing that let users know some
details. Would drop this new macro. Thanks.
>
> > - if (fixed_ctr_ctrl & 0x2)
> > + if (fixed_ctr_ctrl & INTEL_FIXED_0_USER)
> > eventsel |= ARCH_PERFMON_EVENTSEL_USR;
> > - if (fixed_ctr_ctrl & 0x8)
> > + if (fixed_ctr_ctrl & INTEL_FIXED_0_ENABLE_PMI)
> > eventsel |= ARCH_PERFMON_EVENTSEL_INT;
> > new_config = (u64)fixed_ctr_ctrl;
> > }
> > @@ -747,10 +746,9 @@ static inline bool cpl_is_matched(struct kvm_pmc *pmc)
> > select_os = config & ARCH_PERFMON_EVENTSEL_OS;
> > select_user = config & ARCH_PERFMON_EVENTSEL_USR;
> > } else {
> > - config = fixed_ctrl_field(pmc_to_pmu(pmc)->fixed_ctr_ctrl,
> > - pmc->idx - INTEL_PMC_IDX_FIXED);
> > - select_os = config & 0x1;
> > - select_user = config & 0x2;
> > + config = pmu_fixed_ctrl_field(pmc_to_pmu(pmc), pmc->idx);
> > + select_os = config & INTEL_FIXED_0_KERNEL;
> > + select_user = config & INTEL_FIXED_0_USER;
> > }
> >
> > return (static_call(kvm_x86_get_cpl)(pmc->vcpu) == 0) ? select_os : select_user;
> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index 7d9ba301c090..2d098aa2fcc6 100644
> > --- a/arch/x86/kvm/pmu.h
> > +++ b/arch/x86/kvm/pmu.h
> > @@ -12,7 +12,11 @@
> > MSR_IA32_MISC_ENABLE_BTS_UNAVAIL)
> >
> > /* retrieve the 4 bits for EN and PMI out of IA32_FIXED_CTR_CTRL */
> > -#define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
> > +#define fixed_ctrl_field(ctrl_reg, idx) \
> > + (((ctrl_reg) >> ((idx) * INTEL_FIXED_BITS_STRIDE)) & INTEL_FIXED_BITS_MASK)
> > +
> > +#define pmu_fixed_ctrl_field(pmu, idx) \
> > + fixed_ctrl_field((pmu)->fixed_ctr_ctrl, (idx) - INTEL_PMC_IDX_FIXED)
> >
> > #define VMWARE_BACKDOOR_PMC_HOST_TSC 0x10000
> > #define VMWARE_BACKDOOR_PMC_REAL_TIME 0x10001
> > @@ -164,8 +168,8 @@ static inline bool pmc_speculative_in_use(struct kvm_pmc *pmc)
> > struct kvm_pmu *pmu = pmc_to_pmu(pmc);
> >
> > if (pmc_is_fixed(pmc))
> > - return fixed_ctrl_field(pmu->fixed_ctr_ctrl,
> > - pmc->idx - INTEL_PMC_IDX_FIXED) & 0x3;
> > + return pmu_fixed_ctrl_field(pmu, pmc->idx) &
> > + INTEL_FIXED_0_ENABLE;
> >
> > return pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE;
> > }
> > --
> > 2.34.1
> >
--
Thanks,
Dapeng Mi
