Re: [PATCH] KVM: x86/pmu: Manipulate FIXED_CTR_CTRL MSR with macros

[Dapeng Mi <dapeng1.mi@linux.intel.com>]

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@google.com>
> 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@linux.intel.com>
> > ---
> >  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