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 <[email protected]>
> Acked-by: Jan Beulich <[email protected]>
> ---
> CC: Jan Beulich <[email protected]>
> CC: Roger Pau Monné <[email protected]>
>
> 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)
OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
