| 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
| ||
|
Message-ID: <f6c436e4-0d42-45d3-a183-749d9662993b@linux.intel.com>
Date: Tue, 13 Jan 2026 09:22:09 +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>,
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>, Zide Chen <zide.chen@...el.com>,
Falcon Thomas <thomas.falcon@...el.com>, Xudong Hao <xudong.hao@...el.com>
Subject: Re: [Patch v2 1/7] perf/x86/intel: Support the 4 new OMR MSRs
introduced in DMR and NVL
On 1/12/2026 6:27 PM, Peter Zijlstra wrote:
> On Mon, Jan 12, 2026 at 01:16:43PM +0800, Dapeng Mi wrote:
>
>> ISE link: https://www.intel.com/content/www/us/en/content-details/869288/intel-architecture-instruction-set-extensions-programming-reference.html
> Does intel guarantee this link is stable? If not, it is not appropriate
> to stick in a changelog that will live 'forever'.
Maybe not. I suppose it's good enough to put the ISE link into cover
letter. I would remove the ISE link from the commit messages.
>
>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 1840ca1918d1..6ea3260f6422 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -3532,17 +3532,28 @@ static int intel_alt_er(struct cpu_hw_events *cpuc,
>> struct extra_reg *extra_regs = hybrid(cpuc->pmu, extra_regs);
>> int alt_idx = idx;
>>
>> - if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
>> - return idx;
>> -
>> - if (idx == EXTRA_REG_RSP_0)
>> - alt_idx = EXTRA_REG_RSP_1;
>> -
>> - if (idx == EXTRA_REG_RSP_1)
>> - alt_idx = EXTRA_REG_RSP_0;
>> + if (idx == EXTRA_REG_RSP_0 || idx == EXTRA_REG_RSP_1) {
>> + if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
>> + return idx;
>> + if (++alt_idx > EXTRA_REG_RSP_1)
>> + alt_idx = EXTRA_REG_RSP_0;
>> + if (config & ~extra_regs[alt_idx].valid_mask)
>> + return idx;
>> + }
>>
>> - if (config & ~extra_regs[alt_idx].valid_mask)
>> - return idx;
>> + if (idx >= EXTRA_REG_OMR_0 && idx <= EXTRA_REG_OMR_3) {
>> + if (!(x86_pmu.flags & PMU_FL_HAS_OMR))
>> + return idx;
>> + if (++alt_idx > EXTRA_REG_OMR_3)
>> + alt_idx = EXTRA_REG_OMR_0;
>> + /*
>> + * Subtracting EXTRA_REG_OMR_0 ensures to get correct
>> + * OMR extra_reg entries which start from 0.
>> + */
>> + if (config &
>> + ~extra_regs[alt_idx - EXTRA_REG_OMR_0].valid_mask)
>> + return idx;
>> + }
>>
>> return alt_idx;
>> }
>> @@ -3550,16 +3561,24 @@ static int intel_alt_er(struct cpu_hw_events *cpuc,
>> static void intel_fixup_er(struct perf_event *event, int idx)
>> {
>> struct extra_reg *extra_regs = hybrid(event->pmu, extra_regs);
>> - event->hw.extra_reg.idx = idx;
>> + int er_idx;
>>
>> - if (idx == EXTRA_REG_RSP_0) {
>> - event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
>> - event->hw.config |= extra_regs[EXTRA_REG_RSP_0].event;
>> - event->hw.extra_reg.reg = MSR_OFFCORE_RSP_0;
>> - } else if (idx == EXTRA_REG_RSP_1) {
>> + event->hw.extra_reg.idx = idx;
>> + switch (idx) {
>> + case EXTRA_REG_RSP_0 ... EXTRA_REG_RSP_1:
>> + er_idx = idx - EXTRA_REG_RSP_0;
>> event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
>> - event->hw.config |= extra_regs[EXTRA_REG_RSP_1].event;
>> - event->hw.extra_reg.reg = MSR_OFFCORE_RSP_1;
>> + event->hw.config |= extra_regs[er_idx].event;
>> + event->hw.extra_reg.reg = MSR_OFFCORE_RSP_0 + er_idx;
>> + break;
>> + case EXTRA_REG_OMR_0 ... EXTRA_REG_OMR_3:
>> + er_idx = idx - EXTRA_REG_OMR_0;
>> + event->hw.config &= ~ARCH_PERFMON_EVENTSEL_UMASK;
>> + event->hw.config |= 1ULL << (8 + er_idx);
>> + event->hw.extra_reg.reg = MSR_OMR_0 + er_idx;
>> + break;
>> + default:
>> + pr_warn("The extra reg idx %d is not supported.\n", idx);
>> }
>> }
> I found it jarring to have these two functions so dissimilar; I've
> changed both to be a switch statement.
>
> ---
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3532,16 +3532,17 @@ static int intel_alt_er(struct cpu_hw_ev
> struct extra_reg *extra_regs = hybrid(cpuc->pmu, extra_regs);
> int alt_idx = idx;
>
> - if (idx == EXTRA_REG_RSP_0 || idx == EXTRA_REG_RSP_1) {
> + switch (idx) {
> + case EXTRA_REG_RSP_0 ... EXTRA_REG_RSP_1:
> if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
> return idx;
> if (++alt_idx > EXTRA_REG_RSP_1)
> alt_idx = EXTRA_REG_RSP_0;
> if (config & ~extra_regs[alt_idx].valid_mask)
> return idx;
> - }
> + break;
>
> - if (idx >= EXTRA_REG_OMR_0 && idx <= EXTRA_REG_OMR_3) {
> + case EXTRA_REG_OMR_0 ... EXTRA_REG_OMR_3:
> if (!(x86_pmu.flags & PMU_FL_HAS_OMR))
> return idx;
> if (++alt_idx > EXTRA_REG_OMR_3)
> @@ -3550,9 +3551,12 @@ static int intel_alt_er(struct cpu_hw_ev
> * Subtracting EXTRA_REG_OMR_0 ensures to get correct
> * OMR extra_reg entries which start from 0.
> */
> - if (config &
> - ~extra_regs[alt_idx - EXTRA_REG_OMR_0].valid_mask)
> + if (config & ~extra_regs[alt_idx - EXTRA_REG_OMR_0].valid_mask)
> return idx;
> + break;
> +
> + default:
> + break;
> }
>
> return alt_idx;
> @@ -3571,12 +3575,14 @@ static void intel_fixup_er(struct perf_e
> event->hw.config |= extra_regs[er_idx].event;
> event->hw.extra_reg.reg = MSR_OFFCORE_RSP_0 + er_idx;
> break;
> +
> case EXTRA_REG_OMR_0 ... EXTRA_REG_OMR_3:
> er_idx = idx - EXTRA_REG_OMR_0;
> event->hw.config &= ~ARCH_PERFMON_EVENTSEL_UMASK;
> event->hw.config |= 1ULL << (8 + er_idx);
> event->hw.extra_reg.reg = MSR_OMR_0 + er_idx;
> break;
> +
> default:
> pr_warn("The extra reg idx %d is not supported.\n", idx);
> }
Yeah, this looks prettier. Would change it.
Powered by blists - more mailing lists