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

["Egger, Christoph" <chegger@xxxxxxxxx>]

On 06.02.14 10:07, Jan Beulich wrote:
>>>> 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.

I prefer to see a comment in the code that makes this clear.

Christoph


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