Re: [PATCH v9 09/12] perf/x86: Add INTEL_TD_METRIC_FIELD_{BITS,MASK} constants

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

On 9/19/2026 3:39 AM, Zide Chen wrote:
> Replace the hard coded field width (8) and the equivalent 0xff mask
> used for PERF_METRICS MSR field access with named constants
> INTEL_TD_METRIC_FIELD_{BITS,MASK}.
>
> INTEL_TD_METRIC_FIELD_BITS will be used by a subsequent KVM patch to
> validat reserved bits in the PERF_METRICS MSR.
>
> No functional change intended.
>
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
> v9: new patch.
> ---
>  arch/x86/events/intel/core.c      | 8 +++++---
>  arch/x86/include/asm/perf_event.h | 4 ++++
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index cc13164d948f..cbc9a179f2e5 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3159,6 +3159,7 @@ DEFINE_STATIC_CALL(intel_pmu_set_topdown_event_period, x86_perf_event_set_period
>  
>  static inline u64 icl_get_metrics_event_value(u64 metric, u64 slots, int idx)
>  {
> +	int shift = (idx - INTEL_PMC_IDX_METRIC_BASE) * INTEL_TD_METRIC_FIELD_BITS;
>  	u32 val;
>  
>  	/*
> @@ -3166,8 +3167,8 @@ static inline u64 icl_get_metrics_event_value(u64 metric, u64 slots, int idx)
>  	 * summing up to 0xff.
>  	 * slots-in-metric = (Metric / 0xff) * slots
>  	 */
> -	val = (metric >> ((idx - INTEL_PMC_IDX_METRIC_BASE) * 8)) & 0xff;
> -	return  mul_u64_u32_div(slots, val, 0xff);
> +	val = (metric >> shift) & INTEL_TD_METRIC_FIELD_MASK;
> +	return mul_u64_u32_div(slots, val, INTEL_TD_METRIC_FIELD_MASK);
>  }
>  
>  static u64 icl_get_topdown_value(struct perf_event *event,
> @@ -4742,7 +4743,8 @@ static int core_pmu_hw_config(struct perf_event *event)
>  }
>  
>  #define INTEL_TD_METRIC_AVAILABLE_MAX	(INTEL_TD_METRIC_RETIRING + \
> -					 ((x86_pmu.num_topdown_events - 1) << 8))
> +					 ((x86_pmu.num_topdown_events - 1) << \
> +					  INTEL_TD_METRIC_FIELD_BITS))

Sashiko comments,

"

Is it safe to replace the UMASK shift here with the hardware MSR metric
field width?
The original << 8 was used to shift the metric ID into the UMASK field
(bits 8-15) of the software perf event configuration (attr.config).
INTEL_TD_METRIC_FIELD_BITS represents the hardware width of metrics within
the PERF_METRICS MSR.
When evaluated in is_available_metric_event() during perf_event_open(), this
currently functions because both values are 8. However, conflating a software
API layout offset with a hardware register field width could cause this macro
to calculate an invalid configuration mask if the MSR field width changes in
future hardware.

"

It looks reasonable, we either don't change this, or define a macro to
represent the UMASK shift. Thanks.



>  
>  static bool is_available_metric_event(struct perf_event *event)
>  {
> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index 1eb13673e889..ceb6188e5217 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -418,6 +418,10 @@ static inline bool use_fixed_pseudo_encoding(u64 code)
>  #define INTEL_TD_METRIC_MAX			INTEL_TD_METRIC_MEM_BOUND
>  #define INTEL_TD_METRIC_NUM			8
>  
> +/* Width, in bits, of each metric's field within the PERF_METRICS MSR. */
> +#define INTEL_TD_METRIC_FIELD_BITS		8
> +#define INTEL_TD_METRIC_FIELD_MASK		GENMASK_ULL(INTEL_TD_METRIC_FIELD_BITS - 1, 0)
> +
>  #define INTEL_TD_CFG_METRIC_CLEAR_BIT		0
>  #define INTEL_TD_CFG_METRIC_CLEAR		BIT_ULL(INTEL_TD_CFG_METRIC_CLEAR_BIT)
>