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

[sashiko-bot@kernel.org]

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515061143.338553-1-dapeng1.mi@linux.intel.com?part=4