Re: [Xen-devel] [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 03/12/2013 12:34 PM, Jan Beulich wrote:
> > > > On 12.03.13 at 16:32, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> > MSR_IA32_MCx_MISC(4) register on AMD processors is used for error
> > thresholding. PV guests may try to set it up for threshold
> > interrupts which will fail and result in these warnings in the log:
> >
> >    [Firmware Bug]: cpu 0, try to use APIC510 (LVT offset 1) for vector
> >    0xf9, but the register is already in use for vector 0x0 on this cpu
> >
> > Mark this register as invalid to avoid this. While at it, also present
> > other MSR_IA32_MCx_MISC() registers as invalid (except for the first
> > GUEST_MC_BANK_NUM which are emulated).
> Hmm, I'm not convinced. A PV guest shouldn't, by definition, try to
> set up APIC LVTs (or else it is only partially PV).
In Linux, bank 4 is assumed to support LVT interrupts
(lvt_interrupt_supported()) and that leads to the guest trying to set it
up via mce_amd_feature_init()->setup_APIC_mce()->setup_APIC_eilvt().

> > --- a/xen/arch/x86/cpu/mcheck/mce.h
> > +++ b/xen/arch/x86/cpu/mcheck/mce.h
> > @@ -166,6 +166,7 @@ static inline int mce_vendor_bank_msr(const struct vcpu *v, 
> > uint32_t msr)
> >           case MSR_F10_MC4_MISC1:
> >           case MSR_F10_MC4_MISC2:
> >           case MSR_F10_MC4_MISC3:
> > +        case MSR_IA32_MCx_MISC(GUEST_MC_BANK_NUM)...MSR_IA32_MCx_MISC(6):
> And if we take this, then I'd like to see an explanation of the magic
> 6 here, including rationale why going forward there wouldn't be a
> need to bump this to 7, 8, etc.
As of today, 6 banks are supported (although bank #6, MSR0000_041B, 
appears to
be a stub and in fact APM lists only 5 banks).

I suppose we can look at MCG_CAP[BANK_CNT]. Is that what you are suggesting.

Note though that as of today, only the first 5 bank MSRs are 
architectural since that's
all that we have in APM. In theory, AMD may decide to place the ones 
above 5 anywhere.
I'd be surprised if they did but there have been precedents.

> Plus, even if it happens to work, it's not intended for architectural
> MSRs to be dealt with in mce_vendor_bank_msr() (as that code is
> expected to match the default cases in bank_mce_{rd,wr}msr()).
> In other words, the change as is would create a latent bug.
Not sure I follow this. My understanding is that mce_vendor_bank_msr() 
is there to
indicate that the register is a bank register and should be dealt with 
in bank_mce_rdmsr().
And with this change bank_mce_rdmsr() will indeed deal with it by 
returning 0.
-boris

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