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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 02/22/17 08:10 -0700, Jan Beulich wrote:
> >>> 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)

I'll fix these boolean stuffs here and in other patches.

> 
> > +    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.
>

I'll fix this check.

> >  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).

sure

> 
> > @@ -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).

The problem is the existing style in this file is not consistent. Both
if ( cond ) and if (cond) are being used in this file. I chose to use
Xen style in the new code.

> 
> > +        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.

sure

Thanks,
Haozhong

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