Re: [PATCH v11 10/23] x86/resctrl: Remove MSR reading of event configuration value

[Moger, Babu]

Hi Xin,

On 2/7/25 04:07, Xin Li wrote:
> On 2/6/2025 8:17 AM, Reinette Chatre wrote:
>>>> +    wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>>> This is the existing code, however it would be better to use wrmsrl()
>>> when the higher 32-bit are all 0s:
>>>
>>>      wrmsrl(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config);
>>>
>> Could you please elaborate what makes this change better?
> 
> In short, it takes one less argument, and doesn't pass an argument of 0.
> 
> The longer story is that hpa and I are refactoring the MSR access APIs
> to accommodate the immediate form of MSR access instructions.  And we
> are not happy about that there are too many MSR access APIs and their
> uses are *random*.  The native wrmsr() and wrmsrl() are essentially the
> same and the only difference is that wrmsr() passes a 64-bit value to be
> written into a MSR in *2* u32 arguments.  But we already have struct msr
> defined in asm/shared/msr.h as:
>     struct msr {
>             union {
>                     struct {
>                             u32 l;
>                             u32 h;
>                     };
>                     u64 q;
>             };
>     };
> 
> it's more natural to do the same job with this data structure in most
> cases.  And we want to remove wrmsr() and only keep wrmsrl(), thus a
> developer won't have to figure out which one is better to use :-P.
> 
> For that to happen, one cleanup is to replace wrmsr(msr, low, 0) with
> wrmsrl(msr, low) (low is automatically converted to u64 from u32).
> 
> However, I'm fine if Babu wants to keep it as-is.

Thanks for the explanation.  Changed it to use wrmsrl().

-- 
Thanks
Babu Moger