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

[Chen, Zide]

On 9/21/2026 2:01 AM, Mi, Dapeng wrote:
> 
> 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@xxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> ---
>> 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.

Yes, I can revert this change.