[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 13.03.13 08:41, Jan Beulich wrote:
On 12.03.13 at 18:00, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
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().
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
