Re: [PATCH 5/5] x86/mcheck/AMD: Set MCAX Enable bit

[Aravind Gopalakrishnan]

On 1/14/2016 4:46 PM, Borislav Petkov wrote:
> On Thu, Jan 14, 2016 at 04:05:40PM -0600, Aravind Gopalakrishnan wrote:
> >   
> > +/* SMCA defined MSR register set for AMD64 */
> > +#define MSR_AMD64_SMCA_MC0_CTL		0xc0002000
> > +#define MSR_AMD64_SMCA_MC0_STATUS	0xc0002001
> > +#define MSR_AMD64_SMCA_MC0_ADDR		0xc0002002
> > +#define MSR_AMD64_SMCA_MC0_MISC0	0xc0002003
> > +#define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
> >
> > +
> > +#define MSR_AMD64_SMCA_MCx_CTL(x)	(MSR_AMD64_SMCA_MC0_CTL + 0x10*(x))
> > +#define MSR_AMD64_SMCA_MCx_STATUS(x)	(MSR_AMD64_SMCA_MC0_STATUS + 0x10*(x))
> > +#define MSR_AMD64_SMCA_MCx_ADDR(x)	(MSR_AMD64_SMCA_MC0_ADDR + 0x10*(x))
> > +#define MSR_AMD64_SMCA_MCx_MISC(x)	(MSR_AMD64_SMCA_MC0_MISC0 + 0x10*(x))
> > +#define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
> Please add MSRs only with the respective patch that uses them.
>
> AFAICT, you need to add only MSR_AMD64_SMCA_MCx_CONFIG() here.

Ok, Will fix this.

> >   
> >   /* SMCA settings */
> >   #define SMCA_THR_LVT_OFF	0xF000
> > +#define SMCA_MCAX_EN_OFF	0x1
> SMCA *and* MCAX.
>
> SMCA_EN_OFF is not enough?

Well McaX is name of the field in the MSR. I retained the "SMCA" prefix 
as these are all still part of the ScalableMCA changes.
I would prefer if "MCAX" is retained as it is indicative of which bit we 
are touching. So how about just MCAX_EN_OFF ?

> >   
> >
> > +
> > +				smca_high = (smca_high & ~SMCA_MCAX_EN_OFF) | 0x1;
> So this can simply be:
>
> 				smca_high |= SMCA_MCAX_EN_OFF;
>
> ?

Yes. Will fix this.

Thanks,
-Aravind.