Re: [PATCH v3 02/22] x86/msr: Change wrmsr() to take a single parameter

[Demi Marie Obenour <demiobenour@xxxxxxxxx>]

On 10/3/25 18:53, Andrew Cooper wrote:
> Mirroring the cleanup to rdmsr(), do the same to wrmsr().  It now has the same
> API as wrmsrl(), but we'll want to drop that wrapper in due course.
> 
> It's telling that almost all remaining users pass in 0.  Most are converted
> directly to WRMSRNS, but a few are not.
> 
> MSR_VIRT_SPEC_CTRL is unconditionally intercepted is orders of magnitude more

"is unconditionally intercepted is" is this a typo?

> expensive than just serialising.  In disable_lapic_nmi_watchdog(), the P4 case
> won't run on hardware which has anything more than plain WRMSR.
> 
> For CTR_WRITE() in op_model_athlon.c there is a logical change in behaviour,
> but it's fixing a bug.  Peformance counters typically get written to -(count)
> as they generate an interrupt on overflow.  The performance counters even in
> the K8 were 48 bits wide, and this wrmsr() not being a wrmsrl() appears to
> have been an oversight in commit b5103d692aa7 ("x86 oprofile: use
> rdmsrl/wrmsrl") which converted all other users, and appears to be the last
> time there was an attempt to unify the MSR APIs.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> v3:
>  * Swap to wrmsrns() in setup_k7_watchdog()
>  * Reinstate correct bracketing in op_model_athlon.c's CTR_WRITE(), drop
>    useless do{}while().
> ---
>  xen/arch/x86/cpu/amd.c                  |  2 +-
>  xen/arch/x86/hvm/vmx/vmcs.c             |  2 +-
>  xen/arch/x86/include/asm/msr.h          | 20 ++++++++++----------
>  xen/arch/x86/nmi.c                      | 18 +++++++++---------
>  xen/arch/x86/oprofile/op_model_athlon.c |  2 +-
>  5 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 43481daa8e26..9b02e1ba675c 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -934,7 +934,7 @@ void amd_set_legacy_ssbd(bool enable)
>               return;
>  
>       if (cpu_has_virt_ssbd)
> -             wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
> +             wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0);
>       else if (amd_legacy_ssbd)
>               core_set_legacy_ssbd(enable);
>       else
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index b639818b6ea6..cd5ac8a5f0e3 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -754,7 +754,7 @@ static int _vmx_cpu_up(bool bsp)
>          eax |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
>          if ( test_bit(X86_FEATURE_SMX, &boot_cpu_data.x86_capability) )
>              eax |= IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX;
> -        wrmsr(MSR_IA32_FEATURE_CONTROL, eax, 0);
> +        wrmsrns(MSR_IA32_FEATURE_CONTROL, eax);
>      }
>  
>      if ( (rc = vmx_init_vmcs_config(bsp)) != 0 )
> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
> index 188a50f9cea4..941a7612f4ba 100644
> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -15,13 +15,17 @@
>   *   uint64_t foo = rdmsr(MSR_BAR);
>   *   wrmsrns(MSR_BAR, foo);
>   *
> + * and, if architectural serialisaition is necessary, or there are other
> + * reasons that WRMSRNS is inapplicable, then:
> + *
> + *   wrmsr(MSR_BAR, foo);
> + *
>   * In addition, *_safe() wrappers exist to cope gracefully with a #GP.
>   *
>   *
>   * All legacy forms are to be phased out:
>   *
>   *  rdmsrl(MSR_FOO, val);
> - *  wrmsr(MSR_FOO, lo, hi);
>   *  wrmsrl(MSR_FOO, val);
>   */
>  
> @@ -43,17 +47,13 @@ static always_inline uint64_t rdmsr(unsigned int msr)
>         val = a__ | ((u64)b__<<32); \
>  } while(0)
>  
> -#define wrmsr(msr,val1,val2) \
> -     __asm__ __volatile__("wrmsr" \
> -                       : /* no outputs */ \
> -                       : "c" (msr), "a" (val1), "d" (val2))
> -
> -static inline void wrmsrl(unsigned int msr, uint64_t val)
> +static inline void wrmsr(unsigned int msr, uint64_t val)
>  {
> -        uint32_t lo = val, hi = val >> 32;
> +    uint32_t lo = val, hi = val >> 32;
>  
> -        wrmsr(msr, lo, hi);
> +    asm volatile ( "wrmsr" :: "a" (lo), "d" (hi), "c" (msr) );
>  }
> +#define wrmsrl(msr, val) wrmsr(msr, val)
>  
>  /* Non-serialising WRMSR, when available.  Falls back to a serialising 
> WRMSR. */
>  static inline void wrmsrns(uint32_t msr, uint64_t val)
> @@ -150,7 +150,7 @@ static inline void wrmsr_tsc_aux(uint32_t val)
>  
>      if ( *this_tsc_aux != val )
>      {
> -        wrmsr(MSR_TSC_AUX, val, 0);
> +        wrmsrns(MSR_TSC_AUX, val);
>          *this_tsc_aux = val;
>      }
>  }
> diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
> index 9793fa23168d..a0c9194ff032 100644
> --- a/xen/arch/x86/nmi.c
> +++ b/xen/arch/x86/nmi.c
> @@ -218,16 +218,16 @@ void disable_lapic_nmi_watchdog(void)
>          return;
>      switch (boot_cpu_data.x86_vendor) {
>      case X86_VENDOR_AMD:
> -        wrmsr(MSR_K7_EVNTSEL0, 0, 0);
> +        wrmsrns(MSR_K7_EVNTSEL0, 0);
>          break;
>      case X86_VENDOR_INTEL:
>          switch (boot_cpu_data.x86) {
>          case 6:
> -            wrmsr(MSR_P6_EVNTSEL(0), 0, 0);
> +            wrmsrns(MSR_P6_EVNTSEL(0), 0);
>              break;
>          case 15:
> -            wrmsr(MSR_P4_IQ_CCCR0, 0, 0);
> -            wrmsr(MSR_P4_CRU_ESCR0, 0, 0);
> +            wrmsr(MSR_P4_IQ_CCCR0, 0);
> +            wrmsr(MSR_P4_CRU_ESCR0, 0);
>              break;
>          }
>          break;
> @@ -282,7 +282,7 @@ static void clear_msr_range(unsigned int base, unsigned 
> int n)
>      unsigned int i;
>  
>      for (i = 0; i < n; i++)
> -        wrmsr(base+i, 0, 0);
> +        wrmsrns(base + i, 0);
>  }
>  
>  static inline void write_watchdog_counter(const char *descr)
> @@ -308,11 +308,11 @@ static void setup_k7_watchdog(void)
>          | K7_EVNTSEL_USR
>          | K7_NMI_EVENT;
>  
> -    wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
> +    wrmsrns(MSR_K7_EVNTSEL0, evntsel);
>      write_watchdog_counter("K7_PERFCTR0");
>      apic_write(APIC_LVTPC, APIC_DM_NMI);
>      evntsel |= K7_EVNTSEL_ENABLE;
> -    wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
> +    wrmsrns(MSR_K7_EVNTSEL0, evntsel);
>  }
>  
>  static void setup_p6_watchdog(unsigned counter)
> @@ -338,11 +338,11 @@ static void setup_p6_watchdog(unsigned counter)
>          | P6_EVNTSEL_USR
>          | counter;
>  
> -    wrmsr(MSR_P6_EVNTSEL(0), evntsel, 0);
> +    wrmsrns(MSR_P6_EVNTSEL(0), evntsel);
>      write_watchdog_counter("P6_PERFCTR0");
>      apic_write(APIC_LVTPC, APIC_DM_NMI);
>      evntsel |= P6_EVNTSEL0_ENABLE;
> -    wrmsr(MSR_P6_EVNTSEL(0), evntsel, 0);
> +    wrmsrns(MSR_P6_EVNTSEL(0), evntsel);
>  }
>  
>  static void setup_p4_watchdog(void)
> diff --git a/xen/arch/x86/oprofile/op_model_athlon.c 
> b/xen/arch/x86/oprofile/op_model_athlon.c
> index bf897a4b6328..4c016624a69b 100644
> --- a/xen/arch/x86/oprofile/op_model_athlon.c
> +++ b/xen/arch/x86/oprofile/op_model_athlon.c
> @@ -34,7 +34,7 @@
>  #define MAX_COUNTERS FAM15H_NUM_COUNTERS
>  
>  #define CTR_READ(msr_content,msrs,c) do {rdmsrl(msrs->counters[(c)].addr, 
> (msr_content));} while (0)
> -#define CTR_WRITE(l,msrs,c) do {wrmsr(msrs->counters[(c)].addr, -(unsigned 
> int)(l), -1);} while (0)
> +#define CTR_WRITE(l,msrs,c) wrmsr(msrs->counters[(c)].addr, -(l))
>  #define CTR_OVERFLOWED(n) (!((n) & (1ULL<<31)))
>  
>  #define CTRL_READ(msr_content,msrs,c) do {rdmsrl(msrs->controls[(c)].addr, 
> (msr_content));} while (0)


-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Attachment: OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature