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 14:17:46 -0500 Message-ID: <52E6B0DA.8070708@oracle.com> References: <1205617825-10042-1-git-send-email-aravind.gopalakrishnan@amd.com> <1205617825-10042-3-git-send-email-aravind.gopalakrishnan@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1205617825-10042-3-git-send-email-aravind.gopalakrishnan@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 03/15/2008 05:50 PM, Aravind Gopalakrishnan wrote: > vmce_amd_[rd|wr]msr functions can handle accesses to AMD thresholding > registers. But due to this statement here: > switch ( msr & (MSR_IA32_MC0_CTL | 3) ) > we are wrongly masking off top two bits and bit 4 which meant the > register accesses never made it to vmce_amd_* functions. > > We correct this problem by modifying the mask in this patch to allow > AMD thresholding registers to fall to 'default' case which in turn > allows vmce_amd_* functions to handle access to the registers. > > Signed-off-by: Aravind Gopalakrishnan > Reviewed-by: Suravee Suthikulpanit > --- > xen/arch/x86/cpu/mcheck/vmce.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > 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)'. In fact, do MSRC000_040[0:3] even exist? (You may also want to adjust your clock --- your emails are being sent from distant past ;-) ) -boris > { > case MSR_IA32_MC0_CTL: > /* stick all 1's to MCi_CTL */ > @@ -210,7 +210,7 @@ static int bank_mce_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) > int ret = 1; > unsigned int bank = (msr - MSR_IA32_MC0_CTL) / 4; > > - switch ( msr & (MSR_IA32_MC0_CTL | 3) ) > + switch ( msr & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13)) > { > case MSR_IA32_MC0_CTL: > /*