Re: [Xen-devel] [PATCH 13/19] x86/mce_intel: detect and enable LMCE on Intel host

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.02.17 at 07:39, <haozhong.zhang@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -38,6 +38,7 @@ enum mcheck_type {
>  };
>  
>  extern uint8_t cmci_apic_vector;
> +extern bool lmce_support;
>  
>  /* Init functions */
>  enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
> diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c 
> b/xen/arch/x86/cpu/mcheck/mce_intel.c
> index 9e5ee3d..b4cc41a 100644
> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
> @@ -29,6 +29,9 @@ boolean_param("mce_fb", mce_force_broadcast);
>  
>  static int __read_mostly nr_intel_ext_msrs;
>  
> +/* If mce_force_broadcast == 1, lmce_support will be disabled forcibly. */
> +bool __read_mostly lmce_support = 0;

false (but really there's no need for an initializer here)

> @@ -677,10 +680,34 @@ static int mce_is_broadcast(struct cpuinfo_x86 *c)
>      return 0;
>  }
>  
> +static bool intel_enable_lmce(void)
> +{
> +    uint64_t msr_content;
> +
> +    /*
> +     * Section "Enabling Local Machine Check" in Intel SDM Vol 3
> +     * requires software must ensure the LOCK bit and LMCE_ON bit
> +     * of MSR_IA32_FEATURE_CONTROL are set before setting
> +     * MSR_IA32_MCG_EXT_CTL.LMCE_EN.
> +     */
> +
> +    if ( rdmsr_safe(MSR_IA32_FEATURE_CONTROL, msr_content) )
> +        return 0;

false (and so on further down)

> +    if ( msr_content &
> +         (IA32_FEATURE_CONTROL_LOCK | IA32_FEATURE_CONTROL_LMCE_ON) )

This checks whether at least one of the bits is on, which isn't in
line with the comment.

>  static void intel_init_mca(struct cpuinfo_x86 *c)
>  {
> -    bool_t broadcast, cmci = 0, ser = 0;
> +    bool_t broadcast, cmci = 0, ser = 0, lmce = 0;

Please use the opportunity to change to plain bool (and false).

> @@ -700,26 +727,31 @@ static void intel_init_mca(struct cpuinfo_x86 *c)
>  
>      first = mce_firstbank(c);
>  
> +    if ( !mce_force_broadcast && (msr_content & MCG_LMCE_P) )

Please make all your additions match the prevailing coding style in
this file (which admittedly is neither ours nor Linux'es, but a mix).

> +        lmce = intel_enable_lmce();
> +
>      if (smp_processor_id() == 0)
>      {
>          dprintk(XENLOG_INFO, "MCA Capability: BCAST %x SER %x"
> -                " CMCI %x firstbank %x extended MCE MSR %x\n",
> -                broadcast, ser, cmci, first, ext_num);
> +                " CMCI %x firstbank %x extended MCE MSR %x LMCE %x\n",
> +                broadcast, ser, cmci, first, ext_num, lmce);

Please can you switch over to not printing booleans as numbers
here, but simply omitting the respective string from the output if
a feature is not there? Only actual numbers should be printed as
such.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel