From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] x86/MCE: Present MSR_IA32_MCx_MISC(2-6) as invalid on AMD Date: Tue, 12 Mar 2013 13:00:15 -0400 Message-ID: <513F5F1F.6080107@oracle.com> References: <1363102363-1719-1-git-send-email-boris.ostrovsky@oracle.com> <513F674202000078000C504A@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <513F674202000078000C504A@nat28.tlf.novell.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: Jan Beulich Cc: bp@alien.de, chegger@amazon.de, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 03/12/2013 12:34 PM, Jan Beulich wrote: >>>> On 12.03.13 at 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). > 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