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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.08.2025 20:47, Andrew Cooper wrote:
> 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.

Okay, so mainly in need of having the description point out there is actually
a correction of something in here. Then:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan