Re: [PATCH] x86/resctrl: Rename AMD resctrl MSR defines to use MSR_AMD_ prefix

[Reinette Chatre]

Hi Babu,

On 7/22/26 9:57 AM, Babu Moger wrote:
> Several resctrl MSRs in the AMD address range (0xc0000xxx) were defined

"Several resctrl MSRs" -> "resctrl MSRs"?
(looks like all of them were defined this way)

"were defined" -> "are defined"?

> with an MSR_IA32_ prefix even though they are AMD-specific and unrelated
> to Intel's IA32 resctrl MSRs.  The MSR_IA32_ prefix is misleading when
> reading or grepping the codebase alongside the Intel MSR_IA32_* resctrl
> definitions in msr-index.h.
> 
> Rename the AMD resctrl MSR definitions, update all call sites, and keep
> the tools copy of msr-index.h in sync:
> 
>   MSR_IA32_MBA_BW_BASE      -> MSR_AMD_MBA_BW_BASE
>   MSR_IA32_SMBA_BW_BASE     -> MSR_AMD_SMBA_BW_BASE
>   MSR_IA32_L3_QOS_ABMC_CFG  -> MSR_AMD_L3_QOS_ABMC_CFG
>   MSR_IA32_L3_QOS_EXT_CFG   -> MSR_AMD_L3_QOS_EXT_CFG
>   MSR_IA32_EVT_CFG_BASE     -> MSR_AMD_EVT_CFG_BASE
> 
> No functional changes.
> 
> Suggested-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---

...

> @@ -114,7 +114,7 @@ static void _resctrl_sdciae_enable(struct rdt_resource *r, bool enable)
>  	/* Walking r->ctrl_domains, ensure it can't race with cpuhp */
>  	lockdep_assert_cpus_held();
>  
> -	/* Update MSR_IA32_L3_QOS_EXT_CFG MSR on all the CPUs in all domains */
> +	/* Update MSR_AMD_L3_QOS_EXT_CFG MSR on all the CPUs in all domains */

I think this is a good opportunity to remove the redundant text when referring to these MSRs.
For example, "Update MSR_AMD_L3_QOS_EXT_CFG MSR ..." can just be "Update MSR_AMD_L3_QOS_EXT_CFG ..."

This is indeed done by this patch, but not consistently. 

>  	list_for_each_entry_rcu(d, &r->ctrl_domains, hdr.list, lockdep_is_cpus_held())
>  		on_each_cpu_mask(&d->hdr.cpu_mask, resctrl_sdciae_set_one_amd, &enable, 1);
>  }

...

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 3838e0a13d36..a2c9acc7aec8 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -498,9 +498,9 @@ static void resctrl_abmc_set_one_amd(void *arg)
>  	bool *enable = arg;
>  
>  	if (*enable)
> -		msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, ABMC_ENABLE_BIT);
> +		msr_set_bit(MSR_AMD_L3_QOS_EXT_CFG, ABMC_ENABLE_BIT);
>  	else
> -		msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, ABMC_ENABLE_BIT);
> +		msr_clear_bit(MSR_AMD_L3_QOS_EXT_CFG, ABMC_ENABLE_BIT);
>  }
>  
>  /*

The comment appearing at this point contains "L3_QOS_EXT_CFG MSR" - the changelog mentions
support for "grepping the codebase" making this a candidate for changing also?

> @@ -542,7 +542,7 @@ static void resctrl_abmc_config_one_amd(void *info)
>  {
>  	union l3_qos_abmc_cfg *abmc_cfg = info;
>  
> -	wrmsrq(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg->full);
> +	wrmsrq(MSR_AMD_L3_QOS_ABMC_CFG, abmc_cfg->full);
>  }
>  
>  /*

...

> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 30df902ea776..30c4c57e0e4c 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -1785,8 +1785,8 @@ static void mbm_config_write_domain(struct rdt_resource *r,
>  	mon_info.mon_config = val;
>  
>  	/*
> -	 * Update MSR_IA32_EVT_CFG_BASE MSR on one of the CPUs in the
> -	 * domain. The MSRs offset from MSR MSR_IA32_EVT_CFG_BASE
> +	 * Update MSR_AMD_EVT_CFG_BASE MSR on one of the CPUs in the

"MSR_AMD_EVT_CFG_BASE MSR" -> "MSR_AMD_EVT_CFG_BASE"?

> +	 * domain. The MSRs offset from MSR_AMD_EVT_CFG_BASE

Here the redundant text is indeed removed.

>  	 * are scoped at the domain level. Writing any of these MSRs
>  	 * on one CPU is observed by all the CPUs in the domain.
>  	 */
I just have these nits. The patch looks good to me.

Reinette