Re: [Patch v4 3/5] perf/x86/intel: Disable PMI for self-reloaded ACR events

On 4/30/2026 10:36 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Event PMIs can be permanently disabled due to a stale `hw.config1` state when an ACR group is broken up.
> --
>
> commit cfc30b699ff51679e8ae587b226916cfd6fbe7bc
> Author: Dapeng Mi <[email protected]>
>
> perf/x86/intel: Disable PMI for self-reloaded ACR events
>
> This commit disables Performance Monitoring Interrupts (PMI) for all events
> configured with Auto Counter Reload (ACR) self-reload to prevent spurious
> "unknown reason" NMI warnings.
>
> [ ... ]
>
>> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
>> index fad87d3c8b2c..524668dcf4cc 100644
>> --- a/arch/x86/events/perf_event.h
>> +++ b/arch/x86/events/perf_event.h
>> @@ -137,6 +137,16 @@ static inline bool is_acr_event_group(struct perf_event *event)
>>  	return check_leader_group(event->group_leader, PERF_X86_EVENT_ACR);
>>  }
>>  
>> +static inline bool is_acr_self_reload_event(struct perf_event *event)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +
>> +	if (hwc->idx < 0)
>> +		return false;
>> +
>> +	return test_bit(hwc->idx, (unsigned long *)&hwc->config1);
>> +}
> Can this cause PMIs to be permanently disabled if an ACR group is broken up?
>
> If the original group leader is closed, perf_group_detach() promotes the
> siblings to standalone events, making them their own leaders. These new
> standalone events no longer have the PERF_X86_EVENT_ACR flag (as it was only
> set on the original leader).
>
> Because they lack the flag, intel_pmu_acr_late_setup() skips them, and
> their hw.config1 field retains the stale physical counter bitmask from when
> they were in the ACR group.
>
> Since is_acr_self_reload_event() checks hw.config1 without validating if the
> event is still in an ACR group via is_acr_event_group(), could a detached
> event that gets assigned to a counter matching the stale bitmask incorrectly
> return true here?
>
> This would cause intel_pmu_enable_event() to incorrectly clear the
> ARCH_PERFMON_EVENTSEL_INT bit, breaking PMI generation and sampling for a
> now-normal event.

I'm not sure if the issue could really happen in reality. If the group
leader is disabled (I'm not fully understand the what does the "closed"
mean, I suppose it means disable), all ACR events would be disabled as well.

But it's fine to add a sanity check whether the event is an ACR event in
the is_acr_self_reload_event().

Thanks.


>
>>  struct amd_nb {
>>  	int nb_id;  /* NorthBridge id */
>>  	int refcnt; /* reference count */