Re: [PATCH v2 11/16] x86/msr: Change rdmsr() to have normal API

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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.

> @@ -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?

> @@ -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?

> --- 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.

Jan