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

On 4/27/2026 8:12 PM, [email protected] wrote:
> 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 <[email protected]>
>
> 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;

No, it's intended.  The aim is to avoid introducing any coupling between
ACR and arch-PEBS. We hope the cfg_c_val[] caches the real value of HW MSR
at any time and we don't want to write an mixture of ACR reload and 
arch-PEBS configuration into CFG_C MSR. We can't prove the mixture would be
always correct. Thanks.


>