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

[Babu Moger <babu.moger@amd.com>]

Hi Reinette,

Sorry.  Missed this thread.

Taken care of all the comments. Will send v2 soon.

Thanks
Babu

On 8/12/26 17:29, Reinette Chatre wrote:
> 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@alien8.de>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> @@ -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