From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Egger, Christoph" <chegger@amazon.de>
Subject: Re: [PATCH 2/2 V2] mcheck,
 vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
Date: Wed, 12 Feb 2014 10:36:26 +0100
Message-ID: <52FB409A.3010506@amazon.de>
References: <52E7A17D020000780011784E@nat28.tlf.novell.com>
	<52F2A1E4.9030700@amd.com>
	<52F35EE60200007800119A84@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <52F35EE60200007800119A84@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Cc: jinsong.liu@intel.com, boris.ostrovsky@oracle.com, suravee.suthikulpanit@amd.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On 06.02.14 10:07, Jan Beulich wrote:
>>>> On 05.02.14 at 21:41, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> wrote:
>> 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)
> 
> 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