Re: [PATCH RFC 5/6] x86/events: Switch core parts to use new MSR access functions

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Inconsistent application of `msr_write_noser()` and `msr_write_ser()` APIs, violating the commit's design rules and risking hardware-level race conditions.
--

commit 3fc3352a68324488e42156725645cf2d490b75c7
Author: Juergen Gross <jgross@suse.com>

x86/events: Switch core parts to use new MSR access functions

This commit switches the core x86 events subsystem to use the new msr_*() 
access functions. The commit message establishes a rule to use 
msr_write_noser() when another MSR write follows in the same function, and 
msr_write_ser() for the final MSR write.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 810ab21ffd991..c15e0d1a66584 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
[ ... ]
> @@ -713,13 +713,13 @@ void x86_pmu_disable_all(void)
>  
>  		if (!test_bit(idx, cpuc->active_mask))
>  			continue;
> -		rdmsrq(x86_pmu_config_addr(idx), val);
> +		val = msr_read(x86_pmu_config_addr(idx));
>  		if (!(val & ARCH_PERFMON_EVENTSEL_ENABLE))
>  			continue;
>  		val &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
> -		wrmsrq(x86_pmu_config_addr(idx), val);
> +		msr_write_noser(x86_pmu_config_addr(idx), val);
>  		if (is_counter_pair(hwc))
> -			wrmsrq(x86_pmu_config_addr(idx + 1), 0);
> +			msr_write_noser(x86_pmu_config_addr(idx + 1), 0);
>  	}
>  }

The commit message mentions using msr_write_ser() for the last MSR write in a
function. Since msr_write_noser() relies on a non-serializing instruction,
could leaving the final write non-serialized here allow the CPU to proceed 
into critical sections before the hardware PMU disable actually completes? 

If the hardware is still actively counting, this might risk triggering 
unexpected PMIs.

[ ... ]
> @@ -1446,14 +1446,14 @@ int x86_perf_event_set_period(struct perf_event *event)
>  	 */
>  	local64_set(&hwc->prev_count, (u64)-left);
>  
> -	wrmsrq(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
> +	msr_write_noser(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
>  
>  	/*
>  	 * Sign extend the Merge event counter's upper 16 bits since
>  	 * we currently declare a 48-bit counter width
>  	 */
>  	if (is_counter_pair(hwc))
> -		wrmsrq(x86_pmu_event_addr(idx + 1), 0xffff);
> +		msr_write_noser(x86_pmu_event_addr(idx + 1), 0xffff);
>  
>  	perf_event_update_userpage(event);
>  
>  	return ret;
>  }

Similar to the above, this function finishes without a serializing MSR write
or a barrier. Should this final write use msr_write_ser() to match the design
rules outlined in the commit description?

[ ... ]
> @@ -2560,9 +2560,9 @@ void perf_clear_dirty_counters(void)
>  			if (!test_bit(i - INTEL_PMC_IDX_FIXED, hybrid(cpuc->pmu, fixed_cntr_mask)))
>  				continue;
>  
> -			wrmsrq(x86_pmu_fixed_ctr_addr(i - INTEL_PMC_IDX_FIXED), 0);
> +			msr_write_noser(x86_pmu_fixed_ctr_addr(i - INTEL_PMC_IDX_FIXED), 0);
>  		} else {
> -			wrmsrq(x86_pmu_event_addr(i), 0);
> +			msr_write_noser(x86_pmu_event_addr(i), 0);
>  		}
>  	}
>  

The final MSR write in this loop also exclusively uses msr_write_noser().
Is it intended to leave the PMU clearing un-serialized upon returning from 
this function?

> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index fad87d3c8b2ca..cce2e7b67c01f 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
[ ... ]
> @@ -1296,10 +1296,10 @@ static inline void x86_pmu_disable_event(struct perf_event *event)
>  	u64 disable_mask = __this_cpu_read(cpu_hw_events.perf_ctr_virt_mask);
>  	struct hw_perf_event *hwc = &event->hw;
>  
> -	wrmsrq(hwc->config_base, hwc->config & ~disable_mask);
> +	msr_write_ser(hwc->config_base, hwc->config & ~disable_mask);
>  
>  	if (is_counter_pair(hwc))
> -		wrmsrq(x86_pmu_config_addr(hwc->idx + 1), 0);
> +		msr_write_ser(x86_pmu_config_addr(hwc->idx + 1), 0);
>  }

Here the first write uses msr_write_ser() even when a second write might follow 
(when is_counter_pair(hwc) is true). 

Should the first write use msr_write_noser() instead to take advantage of the 
optimization mentioned in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420091634.128787-1-jgross@suse.com?part=5