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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.19 at 18:44, <igor.druzhinin@xxxxxxxxxx> wrote:
> The logic currently tries to work out if a recent overflow (that indicates
> that NMI comes from the watchdog) happened by checking MSB of performance
> counter MSR that is initially sign extended from a negative value
> that we program it to. A possibly incorrect assumption here is that
> MSB is always bit 32 while on modern hardware it's usually 47 and
> the actual bit-width is reported through CPUID. Checking bit 32 for
> overflows is usually fine since we never program it to anything
> exceeding 32-bits and NMI is handled shortly after overflow occurs.
> 
> A problematic scenario that we saw occurs on systems where SMIs taking
> significant time are possible. In that case, NMI handling is deferred to
> the point firmware exits SMI which might take enough time for the counter
> to go through bit 32 and set it to 1 again. So the logic described above
> will misread it and report an unknown NMI erroneously.
> 
> Fortunately, we can use the actual MSB, which is usually higher than the
> currently hardcoded 32, and treat this case correctly at least on modern
> hardware.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>

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.

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.

Jan



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