Re: [PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

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

On 1/28/2014 5:24 AM, Jan Beulich wrote:
>>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>> @@ -107,7 +107,7 @@ static int bank_mce_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>   
>>       *val = 0;
>>   
>> -    switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>> +    switch ( msr & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
> As one of the other reviewers already said - 0xC0000000 would
> be better recognizable here.
>
> As to the 3 -> 0x13 change - I don't think this is conceptually
> correct. While at present we emulate only 2 banks, this had
> been different in the past and may become different again.
> Hence introducing a dis-contiguity after bank 3 is undesirable.
>
>

IMHO, including the '0x13' is necessary. The reason is that 0x413, 
0xc0000408 and 0xc0000409
together form the set of MC4 thresholding registers. Not including 0x13 
in the mask would mean
that accesses to 0x413 alone would not be handled. (which would be 
confusing if someone new
were to look into the mce codebase)

Also, (in response to Boris's comments) - AFAICT, this should not affect 
Intel codepath..
Intel's vmce_* functions only care about MSR_IA32_MC0_CTL2 = 0x00000280 
(if bank0) or 0x00000281 (if bank 1)
and, having 0x13 in the mask does not affect the ability of 
bank_mce_[rd|wr]msr functions to call into vmce_intel*
functions..
(I haven't tested this, so if someone could test and let me know, that'd 
be great.)

I have addressed Christoph's and Boris.O's earlier comments too in the 
next version of this patch.


Thanks,
-Aravind.