From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 2/2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs Date: Mon, 27 Jan 2014 18:29:34 -0500 Message-ID: <52E6EBDE.7030303@oracle.com> References: <1205617825-10042-1-git-send-email-aravind.gopalakrishnan@amd.com> <1205617825-10042-3-git-send-email-aravind.gopalakrishnan@amd.com> <52E6B0DA.8070708@oracle.com> <52E6E1A9.3030903@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52E6E1A9.3030903@amd.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Aravind Gopalakrishnan Cc: jinsong.liu@intel.com, chegger@amazon.de, suravee.suthikulpanit@amd.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 01/27/2014 05:46 PM, Aravind Gopalakrishnan wrote: > On 1/27/2014 1:17 PM, Boris Ostrovsky wrote: >> On 03/15/2008 05:50 PM, Aravind Gopalakrishnan wrote: >>> >>> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c >>> b/xen/arch/x86/cpu/mcheck/vmce.c >>> index f6c35db..cb4fd12 100644 >>> --- 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)) >> >> Which MSRs are going to be handled in the non-default cases? >> MSR0000_040[0:3] and MSRC000_040[0:3]? The first four already have >> explicit cases and I think it would be more readable if you >> explicitly created case statements for the latter four and had a >> simple 'switch(msr)'. I didn't realize this was common code so ignore this. One thing I suggest you change is instead of (0x3<<30) use 0xc0000000 because the latter is immediately familiar to anyone used to AMD MSR spaces. And as for 0x13, I'd add a comment explaining why you have it. Also, Intel processors have 0x413+ registers as well and Intel folks should probably confirm that your change won't break code there. -boris >> > In non-default cases, we need to handle MSR0x40[0:7]. MSR0x40[0:3] is > bank 0 and remaining ones are bank 1. > We can't do a simple switch(msr) since if we do that, MSR0x40[4:7] > will fall to 'default' (which is incorrect) > >> In fact, do MSRC000_040[0:3] even exist? >> > Nope. They don't.. > > we only allow MCE MSR's here anyway: > ret = mce_bank_msr(cur, msr) ? bank_mce_rdmsr(cur, msr, val) : 0; > > 'mce_bank_msr' returns 1 only if we see MSR0x40[0:7] or some 'special > MSR's' which in AMD's case are the three thresholding regs > MSR_F10_MC4_MISC1 to MSR_F10_MC4_MISC3 > >> (You may also want to adjust your clock --- your emails are being >> sent from distant past ;-) ) >> >> > Yes, thanks for pointing that out :) > I'll correct that and send V2 > > -Aravind. >