Re: [PATCH 04/11] perf/x86/intel: Update event constraints and cache_extra_regs[] for ADL

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

On 5/15/2026 2:38 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The commit introduces a bisectability regression for MTL and ARL-U platforms by temporarily applying incorrect Alderlake-specific cache event structures.
> - [Low] Unnecessary duplication of the Golden Cove hardware cache event IDs array.
> --
>
> commit 58bdcbf21858fcf6337dcba2e5e091a86ddafd3c
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86/intel: Update event constraints and cache_extra_regs[] for ADL
>     
> This commit updates the hard-coded event constraints and cache_extra_regs[]
> for Alderlake according to the latest ADL perfmon events. It introduces
> ADL-specific variants of structures and functions to reflect differences
> in L3/node related OCR events.
>
>> Please note these changes would temporarily impact other platforms like
>> MTL/ARL-U which shares hard-coded event structures, but it would be
>> fixed soon in subsequent patches.
> Does this introduce a bisectability regression for Meteorlake and Arrowlake-U
> platforms?
>
> Every commit needs to be self-contained and functionally correct to keep the
> tree bisectable. I noticed the fixes are included later in the series.
>
> Could the patch series be reorganized to avoid leaving any platform in a
> regressed state during bisection?

Hmm, it would not introduce a bisectability regression. The hard coded
cache event MSR value table are already inaccurate before this patch.
Although the changes would impact the MTL/ARL-U, but it doesn't introduce
new issues.


>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 0d0edc2d1b906..7948e3afc2916 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
> [ ... ]
>
>> @@ -713,6 +715,80 @@ static __initconst const u64 glc_hw_cache_event_ids
>>   },
>>  };
>>  
>> +/* ADL P-core (Golden cove) specific event code. */
>> +static __initconst const u64 adl_glc_hw_cache_event_ids
>> +				[PERF_COUNT_HW_CACHE_MAX]
>> +				[PERF_COUNT_HW_CACHE_OP_MAX]
>> +				[PERF_COUNT_HW_CACHE_RESULT_MAX] =
>> +{
>> + [ C(L1D ) ] = {
>> +	[ C(OP_READ) ] = {
>> +		[ C(RESULT_ACCESS) ] = 0x81d0,
>> +		[ C(RESULT_MISS)   ] = 0xe124,
>> +	},
>> +	[ C(OP_WRITE) ] = {
>> +		[ C(RESULT_ACCESS) ] = 0x82d0,
>> +	},
>> + },
> [ ... ]
>
>> + [ C(BPU ) ] = {
>> +	[ C(OP_READ) ] = {
>> +		[ C(RESULT_ACCESS) ] = 0x4c4,
>> +		[ C(RESULT_MISS)   ] = 0x4c5,
>> +	},
>> +	[ C(OP_WRITE) ] = {
>> +		[ C(RESULT_ACCESS) ] = -1,
>> +		[ C(RESULT_MISS)   ] = -1,
>> +	},
>> +	[ C(OP_PREFETCH) ] = {
>> +		[ C(RESULT_ACCESS) ] = -1,
>> +		[ C(RESULT_MISS)   ] = -1,
>> +	},
>> + },
>> +};
> This array appears to be identical to the existing glc_hw_cache_event_ids
> array, except that the C(NODE) entries are omitted and default to zero.
>
> Instead of duplicating the entire array and risking silent divergence when
> future updates are made, could we dynamically clear the C(NODE) entries for
> Alderlake during initialization?
>
> A similar pattern of localized overrides is already utilized in
> intel_pmu_init_grt() for the C(ITLB) event.

The new introduced adl_glc_hw_cache_event_ids[] table would be called by
multiple places, It looks clearer to define a new table instead of
overwriting the old table.

Thanks.


>