Re: [Patch v2 1/7] perf/x86/intel: Support the 4 new OMR MSRs introduced in DMR and NVL

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

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.