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

[sashiko-bot@kernel.org]

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 <dapeng1.mi@linux.intel.com>

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?

>  		/*
>  		 * 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?

[ ... ]

> 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?

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