Re: [PATCH v2 12/16] x86/msr: Change wrmsr() to take a single parameter

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.08.2025 22:41, Andrew Cooper wrote:
> --- 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);

Since you switch to non-serializing here, ...

> @@ -308,11 +308,11 @@ static void setup_k7_watchdog(void)
>          | K7_EVNTSEL_USR
>          | K7_NMI_EVENT;
>  
> -    wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
> +    wrmsr(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);
> +    wrmsr(MSR_K7_EVNTSEL0, evntsel);
>  }

... why not also here?

> --- 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) do { wrmsr(msrs->counters[(c)].addr, -l); } 
> while (0)

This isn't obviously correct (as in: no functional change): The macro is,
for example, passed reset_value[] contents, which is of type unsigned long.
Quite possible that the original code was wrong, though.

In any event l wants parenthesizing.

Jan