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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.02.19 at 19:40, <igor.druzhinin@xxxxxxxxxx> wrote:
> @@ -123,7 +124,8 @@ int nmi_active;
>  #define P6_EVNTSEL_USR               (1 << 16)
>  #define P6_EVENT_CPU_CLOCKS_NOT_HALTED        0x79
>  #define CORE_EVENT_CPU_CLOCKS_NOT_HALTED 0x3c
> -#define P6_EVENT_WIDTH          32
> +/* Bit width of IA32_PMCx MSRs is reported using CPUID.0AH:EAX[23:16]. */
> +#define P6_EVENT_WIDTH_MASK  (((1 << 8) - 1) << 16)
>  
>  #define P4_ESCR_EVENT_SELECT(N)      ((N)<<25)
>  #define P4_CCCR_OVF_PMI0     (1<<26)
> @@ -323,6 +325,7 @@ static void setup_p6_watchdog(unsigned counter)
>      unsigned int evntsel;
>  
>      nmi_perfctr_msr = MSR_P6_PERFCTR(0);
> +    nmi_p6_event_width = MASK_EXTR(cpuid_eax(0xa), P6_EVENT_WIDTH_MASK);

You need to deal with max-leaf being less than 0xa, and you need
to fall back to 32 in case the field is zero, and perhaps also in the
(perhaps just theoretical) case the field holds a value smaller than
32. You may also want to apply an upper cap, such that ...

> @@ -529,7 +532,7 @@ bool nmi_watchdog_tick(const struct cpu_user_regs *regs)
>          else if ( nmi_perfctr_msr == MSR_P6_PERFCTR(0) )
>          {
>              rdmsrl(MSR_P6_PERFCTR(0), msr_content);
> -            if ( msr_content & (1ULL << P6_EVENT_WIDTH) )
> +            if ( msr_content & (1ULL << (nmi_p6_event_width - 1)) )

... doesn't degenerate into undefined behavior.

Jan



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