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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.14 at 21:41, Aravind Gopalakrishnan 
>>> <aravind.gopalakrishnan@xxxxxxx>
wrote:
> On 1/28/2014 5:24 AM, Jan Beulich wrote:
>>>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan 
>>>>> <aravind.gopalakrishnan@xxxxxxx> 
> 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)

No - bit 4 is part of what forms the bank number. Hence it must
be masked out in the switch() expression.

Jan


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