[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.