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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/08/2025 1:38 pm, Jan Beulich wrote:
> 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?

An oversight.  Fixed.

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

I'm pretty sure the change is correct.

Perf counters get programmed to -(count), as they generate an interrupt
when they overflow.  The K8 is the oldest BKDG I can easily access, and
the counters are 48 bits wide.  The same is true of Intel systems of of
the same age.

Interestingly, it is the singular omission from b5103d692aa7 which
converts everything including the Intel version of CTR_WRITE() of this
to use wrmsrl().

While looking at the mail list archives for b5103d692aa7, I found
https://lists.xenproject.org/archives/html/xen-devel/2010-06/msg01660.html
which shows that it was Christoph's attempt to turn rdmsr() and wrmsr()
into a real C functions, so I'm pretty certain that CTR_WRITE() was an
omission in b5103d692aa7.

Only 15 years late on that todo...

> In any event l wants parenthesizing.

Oh, so it does.  Fixed.

~Andrew