Re: [Xen-devel] [PATCH v2] x86/nmi: correctly check MSB of P6 performance counter MSR in watchdog

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.19 at 14:36, <igor.druzhinin@xxxxxxxxxx> wrote:
> On 26/02/2019 13:25, Andrew Cooper wrote:
>> On 26/02/2019 13:12, Igor Druzhinin wrote:
>> 
>>> @@ -323,6 +327,10 @@ static void setup_p6_watchdog(unsigned counter)
>>>      unsigned int evntsel;
>>>  
>>>      nmi_perfctr_msr = MSR_P6_PERFCTR(0);
>>> +    if ( current_cpu_data.cpuid_level >= 0xa )
>>> +        nmi_p6_event_width = MASK_EXTR(cpuid_eax(0xa), 
> P6_EVENT_WIDTH_MASK);
>>> +    if ( !nmi_p6_event_width )
>>> +        nmi_p6_event_width = P6_EVENT_WIDTH_DEFAULT;
>> 
>> This is called on each cpu, but writes to a shared variable.  This
>> entire block wants to be something like:
>> 
>> if ( !nmi_p6_event_width )
>>     nmi_p6_event_width = (current_cpu_data.cpuid_level >= 0xa) ?
>> MASK_EXTR(cpuid_eax(0xa), P6_EVENT_WIDTH_MASK) : P6_EVENT_WIDTH_DEFAULT;
>> 
>> (suitably wrapped) so it gets filled only once.
>> 
> 
> This pattern seems to persist over the whole file - see nmi_perfctr_msr
> above or setup_p4_watchdog, etc. I either need to fix all of them or
> leave it like that as it's not harmful or anything.

I don't think it hurts to do better than the surrounding code
for new code additions, without as a prereq fixing all prior
less optimal coding (but you're of course welcome to do so).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel