Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Aug 2023 14:52:47 -0700
In-Reply-To: <20230808051502.1831199-1-dapeng1.mi@linux.intel.com>
Mime-Version: 1.0
References: <20230808051502.1831199-1-dapeng1.mi@linux.intel.com>
Message-ID: <ZNatr1M6xddAxaWG@google.com>
Subject: Re: [PATCH] KVM: x86/pmu: Manipulate FIXED_CTR_CTRL MSR with macros
From:   Sean Christopherson <seanjc@google.com>
To:     Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Like Xu <likexu@tencent.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Zhenyu Wang <zhenyuw@linux.intel.com>,
        Zhang Xiong <xiong.y.zhang@intel.com>,
        Lv Zhiyuan <zhiyuan.lv@intel.com>,
        Yang Weijiang <weijiang.yang@intel.com>,
        Dapeng Mi <dapeng1.mi@intel.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.

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

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