Re: [Patch v5 16/19] perf/x86: Activate back-to-back NMI detection for arch-PEBS induced NMIs

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 12/5/2025 8:39 PM, Peter Zijlstra wrote:
> On Wed, Dec 03, 2025 at 02:54:57PM +0800, Dapeng Mi wrote:
>> When two or more identical PEBS events with the same sampling period are
>> programmed on a mix of PDIST and non-PDIST counters, multiple
>> back-to-back NMIs can be triggered.
> This is a hardware defect -- albeit a fairly common one.
>
>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index da48bcde8fce..a130d3f14844 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -3351,8 +3351,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);
>>  
>>  		if (cpuc->events[INTEL_PMC_IDX_FIXED_SLOTS] &&
>>  		    is_pebs_counter_event_group(cpuc->events[INTEL_PMC_IDX_FIXED_SLOTS]))
> Note that the old code would return handled++, while the new code:
>
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> index a01c72c03bd6..c7cdcd585574 100644
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
>> @@ -2759,7 +2759,7 @@ __intel_pmu_pebs_events(struct perf_event *event,
>>  	__intel_pmu_pebs_last_event(event, iregs, regs, data, at, count, setup_sample);
>>  }
>>  
>> -static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_data *data)
>> +static int intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_data *data)
>>  {
>>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>  	struct debug_store *ds = cpuc->ds;
>> @@ -2768,7 +2768,7 @@ static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_
>>  	int n;
>>  
>>  	if (!x86_pmu.pebs_active)
>> -		return;
>> +		return 0;
>>  
>>  	at  = (struct pebs_record_core *)(unsigned long)ds->pebs_buffer_base;
>>  	top = (struct pebs_record_core *)(unsigned long)ds->pebs_index;
>> @@ -2779,22 +2779,24 @@ static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_
>>  	ds->pebs_index = ds->pebs_buffer_base;
>>  
>>  	if (!test_bit(0, cpuc->active_mask))
>> -		return;
>> +		return 0;
>>  
>>  	WARN_ON_ONCE(!event);
>>  
>>  	if (!event->attr.precise_ip)
>> -		return;
>> +		return 0;
>>  
>>  	n = top - at;
>>  	if (n <= 0) {
>>  		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
>>  			intel_pmu_save_and_restart_reload(event, 0);
>> -		return;
>> +		return 0;
>>  	}
>>  
>>  	__intel_pmu_pebs_events(event, iregs, data, at, top, 0, n,
>>  				setup_pebs_fixed_sample_data);
>> +
>> +	return 0;
>>  }
>>  
>>  static void intel_pmu_pebs_event_update_no_drain(struct cpu_hw_events *cpuc, u64 mask)
>> @@ -2817,7 +2819,7 @@ static void intel_pmu_pebs_event_update_no_drain(struct cpu_hw_events *cpuc, u64
>>  	}
>>  }
>>  
>> -static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_data *data)
>> +static int intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_data *data)
>>  {
>>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>  	struct debug_store *ds = cpuc->ds;
>> @@ -2830,7 +2832,7 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>>  	u64 mask;
>>  
>>  	if (!x86_pmu.pebs_active)
>> -		return;
>> +		return 0;
>>  
>>  	base = (struct pebs_record_nhm *)(unsigned long)ds->pebs_buffer_base;
>>  	top = (struct pebs_record_nhm *)(unsigned long)ds->pebs_index;
>> @@ -2846,7 +2848,7 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>>  
>>  	if (unlikely(base >= top)) {
>>  		intel_pmu_pebs_event_update_no_drain(cpuc, mask);
>> -		return;
>> +		return 0;
>>  	}
>>  
>>  	for (at = base; at < top; at += x86_pmu.pebs_record_size) {
>> @@ -2931,6 +2933,8 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>>  						setup_pebs_fixed_sample_data);
>>  		}
>>  	}
>> +
>> +	return 0;
>>  }
>>  
>>  static __always_inline void
>> @@ -2984,7 +2988,7 @@ __intel_pmu_handle_last_pebs_record(struct pt_regs *iregs,
>>  
>>  }
>>  
>> -static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_data *data)
>> +static int intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_data *data)
>>  {
>>  	short counts[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS] = {};
>>  	void *last[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS];
>> @@ -2997,7 +3001,7 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>>  	u64 mask;
>>  
>>  	if (!x86_pmu.pebs_active)
>> -		return;
>> +		return 0;
>>  
>>  	base = (struct pebs_basic *)(unsigned long)ds->pebs_buffer_base;
>>  	top = (struct pebs_basic *)(unsigned long)ds->pebs_index;
>> @@ -3010,7 +3014,7 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>>  
>>  	if (unlikely(base >= top)) {
>>  		intel_pmu_pebs_event_update_no_drain(cpuc, mask);
>> -		return;
>> +		return 0;
>>  	}
>>  
>>  	if (!iregs)
>> @@ -3032,9 +3036,11 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>>  
>>  	__intel_pmu_handle_last_pebs_record(iregs, regs, data, mask, counts, last,
>>  					    setup_pebs_adaptive_sample_data);
>> +
>> +	return 0;
>>  }
> will now return handled+=0 for all these. Which is a change in
> behaviour. Also:

This change only take effects for arch-PEBS. For the legacy PEBS, the
"handled" would still be added 1 unconditionally even the *_drain_pebs()
helpers always return 0.

    /*
     * PEBS overflow sets bit 62 in the global status register
     */
    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);


>
>> -static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>> +static int intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>>  				      struct perf_sample_data *data)
>>  {
>>  	short counts[INTEL_PMC_IDX_FIXED + MAX_FIXED_PEBS_EVENTS] = {};
>> @@ -3044,13 +3050,14 @@ static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>>  	struct x86_perf_regs perf_regs;
>>  	struct pt_regs *regs = &perf_regs.regs;
>>  	void *base, *at, *top;
>> +	u64 events_bitmap = 0;
>>  	u64 mask;
>>  
>>  	rdmsrq(MSR_IA32_PEBS_INDEX, index.whole);
>>  
>>  	if (unlikely(!index.wr)) {
>>  		intel_pmu_pebs_event_update_no_drain(cpuc, X86_PMC_IDX_MAX);
>> -		return;
>> +		return 0;

If index.wr is 0, then it indicates there is no any PEBS record written in
buffer since last drain of PEBS buffer. In this case, the PEBS PMI should
not be generated. If it's generated, then it implies there must be
something wrong. The 0 return value would lead to a "suspicious NMI"
warning which is good to warn us there are something wrong.


>>  	}
>>  
>>  	base = cpuc->pebs_vaddr;
>> @@ -3089,6 +3096,7 @@ static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>>  
>>  		basic = at + sizeof(struct arch_pebs_header);
>>  		pebs_status = mask & basic->applicable_counters;
>> +		events_bitmap |= pebs_status;
>>  		__intel_pmu_handle_pebs_record(iregs, regs, data, at,
>>  					       pebs_status, counts, last,
>>  					       setup_arch_pebs_sample_data);
>> @@ -3108,6 +3116,8 @@ static void intel_pmu_drain_arch_pebs(struct pt_regs *iregs,
>>  	__intel_pmu_handle_last_pebs_record(iregs, regs, data, mask,
>>  					    counts, last,
>>  					    setup_arch_pebs_sample_data);
>> +
> 	/*
> 	 * Comment that explains the arch pebs defect goes here.
> 	 */
>> +	return hweight64(events_bitmap);
>>  }