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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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.