* [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD @ 2013-03-12 15:32 Boris Ostrovsky 2013-03-12 15:43 ` Egger Christoph 2013-03-12 16:34 ` Jan Beulich 0 siblings, 2 replies; 6+ messages in thread From: Boris Ostrovsky @ 2013-03-12 15:32 UTC (permalink / raw) To: chegger; +Cc: bp, boris.ostrovsky, xen-devel 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). Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- xen/arch/x86/cpu/mcheck/mce.h | 1 + 1 file changed, 1 insertion(+) diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h index f2aeacb..d6526a4 100644 --- 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): return 1; } break; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD 2013-03-12 15:32 [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD Boris Ostrovsky @ 2013-03-12 15:43 ` Egger Christoph 2013-03-12 16:34 ` Jan Beulich 1 sibling, 0 replies; 6+ messages in thread From: Egger Christoph @ 2013-03-12 15:43 UTC (permalink / raw) To: Boris Ostrovsky; +Cc: bp, xen-devel On 12.03.13 16:32, Boris Ostrovsky 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). > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Acked-by: Christoph Egger <chegger@amazon.de> > --- > xen/arch/x86/cpu/mcheck/mce.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h > index f2aeacb..d6526a4 100644 > --- 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): > return 1; > } > break; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD 2013-03-12 15:32 [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD Boris Ostrovsky 2013-03-12 15:43 ` Egger Christoph @ 2013-03-12 16:34 ` Jan Beulich 2013-03-12 17:00 ` Boris Ostrovsky 1 sibling, 1 reply; 6+ messages in thread From: Jan Beulich @ 2013-03-12 16:34 UTC (permalink / raw) To: chegger, boris.ostrovsky; +Cc: bp, xen-devel >>> On 12.03.13 at 16:32, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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). > --- 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. 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. Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD 2013-03-12 16:34 ` Jan Beulich @ 2013-03-12 17:00 ` Boris Ostrovsky 2013-03-13 7:41 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Boris Ostrovsky @ 2013-03-12 17:00 UTC (permalink / raw) To: Jan Beulich; +Cc: bp, chegger, xen-devel On 03/12/2013 12:34 PM, Jan Beulich wrote: >>>> On 12.03.13 at 16:32, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD 2013-03-12 17:00 ` Boris Ostrovsky @ 2013-03-13 7:41 ` Jan Beulich 2013-03-13 9:16 ` Egger Christoph 0 siblings, 1 reply; 6+ messages in thread From: Jan Beulich @ 2013-03-13 7:41 UTC (permalink / raw) To: Boris Ostrovsky; +Cc: bp, chegger, xen-devel >>> On 12.03.13 at 18:00, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote: > On 03/12/2013 12:34 PM, Jan Beulich wrote: >>>>> On 12.03.13 at 16:32, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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(). So for the PV case this call chain needs to be broken at some suitable point. Nothing LAPIC related should be done in a PV kernel. >>> --- 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. Yes, that sounds like the right thing. >> 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(). No. CTL, STATUS, ADDR, and MISC aren't vendor specific, and hence should be dealt with in bank_mce_rdmsr() in any case. The upper bound here is intentionally MSR_IA32_MCx_CTL(v->arch.vmce.mcg_cap & MCG_CAP_COUNT) as that's what the guest gets announced through MCG_CAP. > And with this change bank_mce_rdmsr() will indeed deal with it by > returning 0. But the guest shouldn't be accessing higher banks' MSRs, as it never was told these banks exist. Or else we have a problem _there_, not in the handling of the MSR accesses. Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD 2013-03-13 7:41 ` Jan Beulich @ 2013-03-13 9:16 ` Egger Christoph 0 siblings, 0 replies; 6+ messages in thread From: Egger Christoph @ 2013-03-13 9:16 UTC (permalink / raw) To: Jan Beulich; +Cc: bp, Boris Ostrovsky, xen-devel On 13.03.13 08:41, Jan Beulich wrote: >>>> On 12.03.13 at 18:00, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote: >> On 03/12/2013 12:34 PM, Jan Beulich wrote: >>>>>> On 12.03.13 at 16:32, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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(). > > So for the PV case this call chain needs to be broken at some > suitable point. Nothing LAPIC related should be done in a PV > kernel. > >>>> --- 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. > > Yes, that sounds like the right thing. > >>> 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(). > > No. CTL, STATUS, ADDR, and MISC aren't vendor specific, and > hence should be dealt with in bank_mce_rdmsr() in any case. The > upper bound here is intentionally > > MSR_IA32_MCx_CTL(v->arch.vmce.mcg_cap & MCG_CAP_COUNT) > > as that's what the guest gets announced through MCG_CAP. > >> And with this change bank_mce_rdmsr() will indeed deal with it by >> returning 0. > > But the guest shouldn't be accessing higher banks' MSRs, as it > never was told these banks exist. Or else we have a problem > _there_, not in the handling of the MSR accesses. Good point Jan. My fault was I only considered the HVM part. I fully agree with you and withdraw my Acked-by. Christoph ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-13 9:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-12 15:32 [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD Boris Ostrovsky 2013-03-12 15:43 ` Egger Christoph 2013-03-12 16:34 ` Jan Beulich 2013-03-12 17:00 ` Boris Ostrovsky 2013-03-13 7:41 ` Jan Beulich 2013-03-13 9:16 ` Egger Christoph
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).