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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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.

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