linux-kernel - Re: [Patch v4 09/13] perf/x86/intel: Setup PEBS data configuration and enable legacy groups

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b31633cf-4127-4f0e-94b1-233d76aa38d6@linux.intel.com>
Date: Mon, 23 Jun 2025 09:38:06 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo
 <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
 Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Kan Liang <kan.liang@...ux.intel.com>, Andi Kleen <ak@...ux.intel.com>,
 Eranian Stephane <eranian@...gle.com>, linux-kernel@...r.kernel.org,
 linux-perf-users@...r.kernel.org, Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [Patch v4 09/13] perf/x86/intel: Setup PEBS data configuration
 and enable legacy groups


On 6/21/2025 5:34 PM, Peter Zijlstra wrote:
> On Fri, Jun 20, 2025 at 10:39:05AM +0000, Dapeng Mi wrote:
>
>> +static void intel_pmu_enable_event_ext(struct perf_event *event)
>> +{
>> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	union arch_pebs_index cached, index;
>> +	struct arch_pebs_cap cap;
>> +	u64 ext = 0;
>> +
>> +	if (!x86_pmu.arch_pebs)
>> +		return;
>> +
>> +	cap = hybrid(cpuc->pmu, arch_pebs_cap);
>> +
>> +	if (event->attr.precise_ip) {
>> +		u64 pebs_data_cfg = intel_get_arch_pebs_data_config(event);
>> +
>> +		ext |= ARCH_PEBS_EN;
>> +		if (hwc->flags & PERF_X86_EVENT_AUTO_RELOAD)
>> +			ext |= (-hwc->sample_period) & ARCH_PEBS_RELOAD;
>> +
>> +		if (pebs_data_cfg && cap.caps) {
>> +			if (pebs_data_cfg & PEBS_DATACFG_MEMINFO)
>> +				ext |= ARCH_PEBS_AUX & cap.caps;
>> +
>> +			if (pebs_data_cfg & PEBS_DATACFG_GP)
>> +				ext |= ARCH_PEBS_GPR & cap.caps;
>> +
>> +			if (pebs_data_cfg & PEBS_DATACFG_XMMS)
>> +				ext |= ARCH_PEBS_VECR_XMM & cap.caps;
>> +
>> +			if (pebs_data_cfg & PEBS_DATACFG_LBRS)
>> +				ext |= ARCH_PEBS_LBR & cap.caps;
>> +		}
>> +
>> +		if (cpuc->n_pebs == cpuc->n_large_pebs)
>> +			index.split.thresh = ARCH_PEBS_THRESH_MUL;
>> +		else
>> +			index.split.thresh = ARCH_PEBS_THRESH_SINGLE;
>> +
>> +		rdmsrl(MSR_IA32_PEBS_INDEX, cached.full);
> Its unclear to me we need this rdmrsl(); does anything actually change
> in there or is it just the value we wrote last? The naming seems to
> suggested you want it cached instead of re-read. Most confusing.

HW could write MSR_IA32_PEBS_INDEX as well, so strictly speaking it's not
we wrote last. The aim of this part of code is to check if large PEBS is
used by last time, if so we need to drain up the PEBS buffer before
re-enable PEBS. 

Yes, the "cache" is some kind of misleading. I would change it.


>
> Also, if you do:
>
> union arch_perf_index {
> 	u64 full;
> 	struct {
> 		u64 foo:1,
> 		    bar:2;
> 	};
> };
>
> Then you can get rid of that .split naming.

Sure. Thanks.


>
>> +		if (index.split.thresh != cached.split.thresh || !cached.split.en) {
>> +			if (cached.split.thresh == ARCH_PEBS_THRESH_MUL &&
>> +			    cached.split.wr > 0) {
>> +				/*
>> +				 * Large PEBS was enabled.
>> +				 * Drain PEBS buffer before applying the single PEBS.
>> +				 */
>> +				intel_pmu_drain_pebs_buffer();
>> +			} else {
>> +				index.split.wr = 0;
>> +				index.split.full = 0;
>> +				index.split.en = 1;
>> +				wrmsrq(MSR_IA32_PEBS_INDEX, index.full);
>> +			}
>> +		}
>> +	}
>> +
>> +	if (cpuc->cfg_c_val[hwc->idx] != ext)
>> +		__intel_pmu_update_event_ext(hwc->idx, ext);
>> +}