On 19/08/2025 1:04 pm, Jan Beulich wrote:
> On 15.08.2025 22:41, Andrew Cooper wrote:
>> We want a consistent MSR API, and these want to be named rdmsr() and wrmsr(),
>> but not with their current APIs. The current rdmsr() flavours writing to
>> their parameters by name makes code that reads like invalid C, and is
>> unergonomic to use in lots of cases.
>>
>> Change the API, and update the callers all in one go. Where appropriate,
>> update the write side to wrmsrns() as per the recommendation.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <[email protected]>
>> ---
>> CC: Jan Beulich <[email protected]>
>> CC: Roger Pau Monné <[email protected]>
>>
>> I do have a more creative solution if this patch is considered to be too
>> large.
>> https://gitlab.com/xen-project/hardware/xen-staging/-/commit/e13cf25d06d08481e2c138daa1fd902cf36d757b
> I'm not concerned by the size of this patch.
>
>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -23,17 +23,17 @@ static uint32_t __ro_after_init mcu_opt_ctrl_val;
>>
>> void update_mcu_opt_ctrl(void)
>> {
>> - uint32_t mask = mcu_opt_ctrl_mask, lo, hi;
>> + uint32_t mask = mcu_opt_ctrl_mask, val;
>>
>> if ( !mask )
>> return;
>>
>> - rdmsr(MSR_MCU_OPT_CTRL, lo, hi);
>> + val = rdmsr(MSR_MCU_OPT_CTRL);
>>
>> - lo &= ~mask;
>> - lo |= mcu_opt_ctrl_val;
>> + val &= ~mask;
>> + val |= mcu_opt_ctrl_val;
>>
>> - wrmsr(MSR_MCU_OPT_CTRL, lo, hi);
>> + wrmsrns(MSR_MCU_OPT_CTRL, val);
>> }
> I don't consider it a good idea to suddenly clear the upper half of this
> MSR, and ...
>
>> @@ -51,17 +51,17 @@ static uint32_t __ro_after_init pb_opt_ctrl_val;
>>
>> void update_pb_opt_ctrl(void)
>> {
>> - uint32_t mask = pb_opt_ctrl_mask, lo, hi;
>> + uint32_t mask = pb_opt_ctrl_mask, val;
>>
>> if ( !mask )
>> return;
>>
>> - rdmsr(MSR_PB_OPT_CTRL, lo, hi);
>> + val = rdmsr(MSR_PB_OPT_CTRL);
>>
>> - lo &= ~mask;
>> - lo |= pb_opt_ctrl_val;
>> + val &= ~mask;
>> + val |= pb_opt_ctrl_val;
>>
>> - wrmsr(MSR_PB_OPT_CTRL, lo, hi);
>> + wrmsrns(MSR_PB_OPT_CTRL, val);
>> }
> ... this one.
Yeah, both the local variables need turning into uint64_t here.
>
>> @@ -456,15 +456,15 @@ static void __init probe_mwait_errata(void)
>> */
>> static void Intel_errata_workarounds(struct cpuinfo_x86 *c)
>> {
>> - unsigned long lo, hi;
>> + uint64_t val;
>>
>> if ((c->x86 == 15) && (c->x86_model == 1) && (c->x86_mask == 1)) {
>> - rdmsr (MSR_IA32_MISC_ENABLE, lo, hi);
>> - if ((lo & (1<<9)) == 0) {
>> + val = rdmsr(MSR_IA32_MISC_ENABLE);
>> + if ((val & (1 << 9)) == 0) {
>> printk (KERN_INFO "CPU: C0 stepping P4 Xeon
>> detected.\n");
>> printk (KERN_INFO "CPU: Disabling hardware prefetching
>> (Errata 037)\n");
>> - lo |= (1<<9); /* Disable hw prefetching */
>> - wrmsr (MSR_IA32_MISC_ENABLE, lo, hi);
>> + val |= (1 << 9); /* Disable hw prefetching */
>> + wrmsrns(MSR_IA32_MISC_ENABLE, val);
>> }
>> }
> Move val into the more narrow scope at the same time?
No, not based on the history of this function.
>
>> @@ -699,7 +715,7 @@ void cf_check vmx_cpu_dead(unsigned int cpu)
>>
>> static int _vmx_cpu_up(bool bsp)
>> {
>> - u32 eax, edx;
>> + u32 eax;
> Like you do elsewhere, switch to uint32_t at the same time?
Will do.
>
>> --- a/xen/arch/x86/tsx.c
>> +++ b/xen/arch/x86/tsx.c
>> @@ -42,6 +42,8 @@ void tsx_init(void)
>> {
>> static bool __read_mostly once;
>>
>> + uint64_t val;
>> +
>> /*
> No real need for yet another newline, I would say.
Where? Before? that's separation of static and not. After? that's
separation of variables and code. All as we do elsewhere.
~Andrew
