Re: [Patch v3 4/4] perf/x86/intel: Consolidate MSR_IA32_PERF_CFG_C tracking

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] intel_pmu_config_acr() clobbers upper Architectural PEBS configuration bits in cfg_c_val and the PERF_CFG_C MSR.
--

commit 5ded8e15814c1efc4e7b5c766abdd782de4d360c
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Consolidate MSR_IA32_PERF_CFG_C tracking

This commit consolidates the tracking of the PERF_CFG_C MSR into a single
variable, cfg_c_val, to maintain a unified view of the MSR state for both
Auto Counter Reload (ACR) and Architectural PEBS configurations.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
[ ... ]
> @@ -3169,10 +3169,10 @@ static void intel_pmu_config_acr(int idx, u64 mask, u32 reload)
>  		wrmsrl(msr_b + msr_offset, mask);
>  		cpuc->acr_cfg_b[idx] = mask;
>  	}
> -	/* Only need to update the reload value when there is a valid config value. */
> -	if (mask && cpuc->acr_cfg_c[idx] != reload) {
> +	/* Only update CFG_C reload when ACR is actively enabled (mask != 0) */
> +	if (mask && ((cpuc->cfg_c_val[idx] & ARCH_PEBS_RELOAD) != reload)) {
>  		wrmsrl(msr_c + msr_offset, reload);
> -		cpuc->acr_cfg_c[idx] = reload;
> +		cpuc->cfg_c_val[idx] = reload;
>  	}
>  }

Can this assignment wipe out the upper Architectural PEBS configuration bits?

Since reload is passed as a u32 and cfg_c_val is a 64-bit value, assigning 
reload directly zero-extends the value. This will clear the upper 32 bits, 
unintentionally removing any existing Architectural PEBS configurations 
like ARCH_PEBS_EN from the software cache, and writing zeroes to the 
upper bits of the physical MSR.

Although intel_pmu_enable_event_ext() recalculates the full MSR state shortly
after and detects the mismatch, resulting in a second wrmsrq() to restore the
missing PEBS bits, this causes an inefficient double MSR write and seems to 
defeat the goal of maintaining a consistent MSR view.

Would it be better to use a read-modify-write pattern here instead? 
For example:

cpuc->cfg_c_val[idx] = (cpuc->cfg_c_val[idx] & ~ARCH_PEBS_RELOAD) | reload;

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