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: <20250621093437.GV1613376@noisy.programming.kicks-ass.net>
Date: Sat, 21 Jun 2025 11:34:37 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Dapeng Mi <dapeng1.mi@...ux.intel.com>
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 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.

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.

> +		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);
> +}