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

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

On 27/02/2019 10:02, Jan Beulich wrote:
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> albeit ...
> 
>> @@ -323,6 +326,15 @@ static void setup_p6_watchdog(unsigned counter)
>>      unsigned int evntsel;
>>  
>>      nmi_perfctr_msr = MSR_P6_PERFCTR(0);
>> +    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_MIN;
>> +    if ( !nmi_p6_event_width )
>> +        nmi_p6_event_width = P6_EVENT_WIDTH_MIN;
> 
> ... I think this would now better be
> 
>     if ( !nmi_p6_event_width && 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_MIN;
> 
> Re-writing of which also mad me notice a hard tab in there. I'd be
> fine making the adjustment while committing, as long as you agree.

Thanks, I also didn't like how it looked eventually. I'll make the same
adjustment to my copy of the patch as well then.

> Btw, considering the combination of subject tag and Cc list I take it
> that you don't intend this to go into 4.12? I ask because generally
> I'd consider this a backporting candidate.

Yes, I didn't intend it to target 4.12 as I don't consider it a serious
issue - we've only seen it on one type of Supermicro machines
(unfortunately, our lab is now almost 50% of them) so far with poor
implementation of ERST. But I wouldn't mind if it was selected as a
candidate for 4.12 and potential backporting.

Igor


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