Re: [Patch v7 23/24] perf/x86: Activate back-to-back NMI detection for arch-PEBS induced NMIs

On 4/25/2026 11:31 AM, [email protected] wrote:
> Sashiko AI review found 2 potential issue(s):
> - [High] Incorrect bitwise operation in intel_pmu_drain_pebs_nhm() corrupts the processed events count.
> - [High] Removing the unconditional `handled` increment causes valid PMIs to be reported as unhandled when `drain_pebs()` returns 0.
> --
>
> commit 0317f48597bdef9b4d93c696bde043840ac1fc6b
> Author: Dapeng Mi <[email protected]>
>
> perf/x86: Activate back-to-back NMI detection for arch-PEBS induced NMIs
>
> This patch modifies the PEBS handlers to return the count of actually
> processed events instead of void. This count is used in the PMI handler
> to activate back-to-back NMI detection and avoid "suspicious NMI" warnings.
>
>> 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
> [ ... ]
>
>> @@ -3558,9 +3558,8 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
>>  	if (__test_and_clear_bit(GLOBAL_STATUS_BUFFER_OVF_BIT, (unsigned long *)&status)) {
>>  		u64 pebs_enabled = cpuc->pebs_enabled;
>>  
>> -		handled++;
>>  		x86_pmu_handle_guest_pebs(regs, &data);
>> -		static_call(x86_pmu_drain_pebs)(regs, &data);
>> +		handled += static_call(x86_pmu_drain_pebs)(regs, &data);
> If x86_pmu_drain_pebs() returns 0, will handled fail to increment even though
> the hardware asserted an NMI and set the MSR status bit?
>
> This might happen if the host PEBS buffer is empty, such as when a guest PEBS
> event triggers the PMI and is handled by x86_pmu_handle_guest_pebs() right
> above it. 
>
> Could this cause the core NMI framework to treat the interrupt as unhandled
> and trigger an unknown NMI warning? Is it necessary to ensure handled
> increments by at least 1 when the status bit is explicitly acknowledged?

It's indeed an issue of the guest PEBS event. But for other host events, if
x86_pmu_drain_pebs() returns 0, then it means there must be something
wrong, we should not hide the issue.

I would add an exception for the guest PEBS event.


>
>>  		/*
>>  		 * PMI throttle may be triggered, which stops the PEBS event.
> [ ... ]
>
>> @@ -3587,8 +3586,7 @@ static int handle_pmi_common(struct pt_regs *regs, u64 status)
>>  	 */
>>  	if (__test_and_clear_bit(GLOBAL_STATUS_ARCH_PEBS_THRESHOLD_BIT,
>>  				 (unsigned long *)&status)) {
>> -		handled++;
>> -		static_call(x86_pmu_drain_pebs)(regs, &data);
>> +		handled += static_call(x86_pmu_drain_pebs)(regs, &data);
> Does this share the same issue as above, where a return value of 0 from
> drain_pebs() would result in handled not being incremented for a valid PMI?

As the above mentioned, we should expose the issue if there are something
wrong.


>
> [ ... ]
>
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
> [ ... ]
>
>> @@ -3189,6 +3192,7 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>>  		if ((counts[bit] == 0) && (error[bit] == 0))
>>  			continue;
>>  
>> +		events_bitmap |= bit;
>>  		event = cpuc->events[bit];
> Will this perform a bitwise OR with the integer index itself rather than
> the expected bitmask?
>
> Because bit is an integer index (e.g., 0 for PMC0, 3 for PMC3), if only PMC3
> overflows, events_bitmap |= 3 (binary 011) results in hweight64(3) returning
> 2. If PMC0 overflows, events_bitmap |= 0 leaves the bitmap at 0.
>
> Should this be events_bitmap |= BIT_ULL(bit) or 1ULL << bit to correctly
> record the processed events?

Yes, we also found this issue locally. Would fix it in next version.

Thanks.


>