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 <jbeul...@suse.com>
Jan
