From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: [PATCH] fix and clean up c/s 19423 Date: Fri, 27 Mar 2009 16:37:44 +0000 Message-ID: <49CD0EE8.76E4.0078.0@novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: xen-devel@lists.xensource.com Cc: Yunhong Jiang , Liping Ke List-Id: xen-devel@lists.xenproject.org - fix inverted return value check for intel_mce_{rd,wr}msr() - fix broken initialization of d->arch.vmca_msrs.mci_ctl - remove pointless (!d || is_idle_domain(d)) checks - eliminate hard-coded limit to 9 banks - avoid redundant gdprintk()s Signed-off-by: Jan Beulich --- 2009-03-27.orig/xen/arch/x86/cpu/mcheck/mce_intel.c 2009-03-27 = 12:06:52.000000000 +0100 +++ 2009-03-27/xen/arch/x86/cpu/mcheck/mce_intel.c 2009-03-27 = 12:39:44.000000000 +0100 @@ -737,7 +737,7 @@ void mce_intel_feature_init(struct cpuin intel_init_cmci(c); } =20 -uint64_t g_mcg_cap; +static uint64_t g_mcg_cap; static void mce_cap_init(struct cpuinfo_x86 *c) { u32 l, h; @@ -749,9 +749,12 @@ static void mce_cap_init(struct cpuinfo_ if ((l & MCG_CMCI_P) && cpu_has_apic) cmci_support =3D 1; =20 - nr_mce_banks =3D l & 0xff; + nr_mce_banks =3D l & MCG_CAP_COUNT; if (nr_mce_banks > MAX_NR_BANKS) + { printk(KERN_WARNING "MCE: exceed max mce banks\n"); + g_mcg_cap =3D (g_mcg_cap & ~MCG_CAP_COUNT) | MAX_NR_BANKS; + } if (l & MCG_EXT_P) { nr_intel_ext_msrs =3D (l >> MCG_EXT_CNT) & 0xff; @@ -823,11 +826,22 @@ int intel_mcheck_init(struct cpuinfo_x86 } =20 /* Guest vMCE# MSRs virtualization ops (rdmsr/wrmsr) */ -int intel_mce_wrmsr(u32 msr, u32 lo, u32 hi) +void intel_mce_init_msr(struct domain *d) +{ + d->arch.vmca_msrs.mcg_status =3D 0x0; + d->arch.vmca_msrs.mcg_cap =3D g_mcg_cap; + d->arch.vmca_msrs.mcg_ctl =3D (uint64_t)~0x0; + d->arch.vmca_msrs.nr_injection =3D 0; + memset(d->arch.vmca_msrs.mci_ctl, ~0, + sizeof(d->arch.vmca_msrs.mci_ctl)); + INIT_LIST_HEAD(&d->arch.vmca_msrs.impact_header); +} + +int intel_mce_wrmsr(u32 msr, u64 value) { struct domain *d =3D current->domain; struct bank_entry *entry =3D NULL; - uint64_t value =3D (u64)hi << 32 | lo; + unsigned int bank; int ret =3D 1; =20 spin_lock(&mce_locks); @@ -835,86 +849,71 @@ int intel_mce_wrmsr(u32 msr, u32 lo, u32 { case MSR_IA32_MCG_CTL: if (value !=3D (u64)~0x0 && value !=3D 0x0) { - gdprintk(XENLOG_WARNING, "MCE: value writen to MCG_CTL" + gdprintk(XENLOG_WARNING, "MCE: value written to MCG_CTL" "should be all 0s or 1s\n"); ret =3D -1; break; } - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: wrmsr not in DOM context, = skip\n"); - break; - } d->arch.vmca_msrs.mcg_ctl =3D value; break; case MSR_IA32_MCG_STATUS: - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: wrmsr not in DOM context, = skip\n"); - break; - } d->arch.vmca_msrs.mcg_status =3D value; gdprintk(XENLOG_DEBUG, "MCE: wrmsr MCG_CTL %"PRIx64"\n", value); break; - case MSR_IA32_MC0_CTL2: - case MSR_IA32_MC1_CTL2: - case MSR_IA32_MC2_CTL2: - case MSR_IA32_MC3_CTL2: - case MSR_IA32_MC4_CTL2: - case MSR_IA32_MC5_CTL2: - case MSR_IA32_MC6_CTL2: - case MSR_IA32_MC7_CTL2: - case MSR_IA32_MC8_CTL2: + case MSR_IA32_MCG_CAP: + gdprintk(XENLOG_WARNING, "MCE: MCG_CAP is read-only\n"); + ret =3D -1; + break; + case MSR_IA32_MC0_CTL2 ... MSR_IA32_MC0_CTL2 + MAX_NR_BANKS - 1: gdprintk(XENLOG_WARNING, "We have disabled CMCI capability, " "Guest should not write this MSR!\n"); break; - case MSR_IA32_MC0_CTL: - case MSR_IA32_MC1_CTL: - case MSR_IA32_MC2_CTL: - case MSR_IA32_MC3_CTL: - case MSR_IA32_MC4_CTL: - case MSR_IA32_MC5_CTL: - case MSR_IA32_MC6_CTL: - case MSR_IA32_MC7_CTL: - case MSR_IA32_MC8_CTL: - if (value !=3D (u64)~0x0 && value !=3D 0x0) { - gdprintk(XENLOG_WARNING, "MCE: value writen to MCi_CTL" - "should be all 0s or 1s\n"); + case MSR_IA32_MC0_CTL ... MSR_IA32_MC0_CTL + 4 * MAX_NR_BANKS - 1: + bank =3D (msr - MSR_IA32_MC0_CTL) / 4; + if (bank >=3D (d->arch.vmca_msrs.mcg_cap & MCG_CAP_COUNT)) { + gdprintk(XENLOG_WARNING, "MCE: bank %u does not exist\n", = bank); ret =3D -1; break; } - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: wrmsr not in DOM context, = skip\n"); + switch (msr & (MSR_IA32_MC0_CTL | 3)) + { + case MSR_IA32_MC0_CTL: + if (value !=3D (u64)~0x0 && value !=3D 0x0) { + gdprintk(XENLOG_WARNING, "MCE: value written to MC%u_CTL" + "should be all 0s or 1s (is %"PRIx64")\n", + bank, value); + ret =3D -1; + break; + } + d->arch.vmca_msrs.mci_ctl[(msr - MSR_IA32_MC0_CTL)/4] =3D = value; break; - } - d->arch.vmca_msrs.mci_ctl[(msr - MSR_IA32_MC0_CTL)/4] =3D value; - break; - case MSR_IA32_MC0_STATUS: - case MSR_IA32_MC1_STATUS: - case MSR_IA32_MC2_STATUS: - case MSR_IA32_MC3_STATUS: - case MSR_IA32_MC4_STATUS: - case MSR_IA32_MC5_STATUS: - case MSR_IA32_MC6_STATUS: - case MSR_IA32_MC7_STATUS: - case MSR_IA32_MC8_STATUS: - if (!d || is_idle_domain(d)) { - /* Just skip */ - gdprintk(XENLOG_WARNING, "mce wrmsr: not in domain context!\n"= ); + case MSR_IA32_MC0_STATUS: + /* Give the first entry of the list, it corresponds to = current + * vMCE# injection. When vMCE# is finished processing by the + * the guest, this node will be deleted. + * Only error bank is written. Non-error banks simply return. + */ + if (!list_empty(&d->arch.vmca_msrs.impact_header)) { + entry =3D list_entry(d->arch.vmca_msrs.impact_header.next,= + struct bank_entry, list); + if ( entry->bank =3D=3D bank ) + entry->mci_status =3D value; + gdprintk(XENLOG_DEBUG, + "MCE: wr MC%u_STATUS %"PRIx64" in vMCE#\n", + bank, value); + } else + gdprintk(XENLOG_DEBUG, + "MCE: wr MC%u_STATUS %"PRIx64"\n", bank, value); + break; + case MSR_IA32_MC0_ADDR: + gdprintk(XENLOG_WARNING, "MCE: MC%u_ADDR is read-only\n", = bank); + ret =3D -1; + break; + case MSR_IA32_MC0_MISC: + gdprintk(XENLOG_WARNING, "MCE: MC%u_MISC is read-only\n", = bank); + ret =3D -1; break; } - /* Give the first entry of the list, it corresponds to current - * vMCE# injection. When vMCE# is finished processing by the - * the guest, this node will be deleted. - * Only error bank is written. Non-error bank simply return. - */ - if ( !list_empty(&d->arch.vmca_msrs.impact_header) ) { - entry =3D list_entry(d->arch.vmca_msrs.impact_header.next, - struct bank_entry, list); - if ( entry->bank =3D=3D (msr - MSR_IA32_MC0_STATUS)/4 ) { - entry->mci_status =3D value; - } - gdprintk(XENLOG_DEBUG, "MCE: wmrsr mci_status in vMCE# = context\n"); - } - gdprintk(XENLOG_DEBUG, "MCE: wrmsr mci_status val:%"PRIx64"\n", = value); break; default: ret =3D 0; @@ -928,6 +927,7 @@ int intel_mce_rdmsr(u32 msr, u32 *lo, u3 { struct domain *d =3D current->domain; int ret =3D 1; + unsigned int bank; struct bank_entry *entry =3D NULL; =20 *lo =3D *hi =3D 0x0; @@ -935,142 +935,82 @@ int intel_mce_rdmsr(u32 msr, u32 *lo, u3 switch(msr) { case MSR_IA32_MCG_STATUS: - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: rdmsr not in domain context!\n"= ); - *lo =3D *hi =3D 0x0; - break; - } *lo =3D (u32)d->arch.vmca_msrs.mcg_status; *hi =3D (u32)(d->arch.vmca_msrs.mcg_status >> 32); gdprintk(XENLOG_DEBUG, "MCE: rd MCG_STATUS lo %x hi %x\n", *lo, = *hi); break; case MSR_IA32_MCG_CAP: - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: rdmsr not in domain context!\n"= ); - *lo =3D *hi =3D 0x0; - break; - } *lo =3D (u32)d->arch.vmca_msrs.mcg_cap; *hi =3D (u32)(d->arch.vmca_msrs.mcg_cap >> 32); gdprintk(XENLOG_DEBUG, "MCE: rdmsr MCG_CAP lo %x hi %x\n", *lo, = *hi); break; case MSR_IA32_MCG_CTL: - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: rdmsr not in domain context!\n"= ); - *lo =3D *hi =3D 0x0; - break; - } *lo =3D (u32)d->arch.vmca_msrs.mcg_ctl; *hi =3D (u32)(d->arch.vmca_msrs.mcg_ctl >> 32); gdprintk(XENLOG_DEBUG, "MCE: rdmsr MCG_CTL lo %x hi %x\n", *lo, = *hi); break; - case MSR_IA32_MC0_CTL2: - case MSR_IA32_MC1_CTL2: - case MSR_IA32_MC2_CTL2: - case MSR_IA32_MC3_CTL2: - case MSR_IA32_MC4_CTL2: - case MSR_IA32_MC5_CTL2: - case MSR_IA32_MC6_CTL2: - case MSR_IA32_MC7_CTL2: - case MSR_IA32_MC8_CTL2: + case MSR_IA32_MC0_CTL2 ... MSR_IA32_MC0_CTL2 + MAX_NR_BANKS - 1: gdprintk(XENLOG_WARNING, "We have disabled CMCI capability, " "Guest should not read this MSR!\n"); break; - case MSR_IA32_MC0_CTL: - case MSR_IA32_MC1_CTL: - case MSR_IA32_MC2_CTL: - case MSR_IA32_MC3_CTL: - case MSR_IA32_MC4_CTL: - case MSR_IA32_MC5_CTL: - case MSR_IA32_MC6_CTL: - case MSR_IA32_MC7_CTL: - case MSR_IA32_MC8_CTL: - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: rdmsr not in domain context!\n"= ); - *lo =3D *hi =3D 0x0; - break; - } - *lo =3D (u32)d->arch.vmca_msrs.mci_ctl[(msr - MSR_IA32_MC0_CTL)/4]= ; - *hi =3D - (u32)(d->arch.vmca_msrs.mci_ctl[(msr - MSR_IA32_MC0_CTL)/4] - >> 32); - gdprintk(XENLOG_DEBUG, "MCE: rdmsr MCi_CTL lo %x hi %x\n", *lo, = *hi); - break; - case MSR_IA32_MC0_STATUS: - case MSR_IA32_MC1_STATUS: - case MSR_IA32_MC2_STATUS: - case MSR_IA32_MC3_STATUS: - case MSR_IA32_MC4_STATUS: - case MSR_IA32_MC5_STATUS: - case MSR_IA32_MC6_STATUS: - case MSR_IA32_MC7_STATUS: - case MSR_IA32_MC8_STATUS: - /* Only error bank is read. Non-error bank simply return */ - *lo =3D *hi =3D 0x0; - gdprintk(XENLOG_DEBUG, "MCE: rdmsr mci_status\n"); - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "mce_rdmsr: not in domain context!\n"= ); + case MSR_IA32_MC0_CTL ... MSR_IA32_MC0_CTL + 4 * MAX_NR_BANKS - 1: + bank =3D (msr - MSR_IA32_MC0_CTL) / 4; + if (bank >=3D (d->arch.vmca_msrs.mcg_cap & MCG_CAP_COUNT)) { + gdprintk(XENLOG_WARNING, "MCE: bank %u does not exist\n", = bank); + ret =3D -1; break; } - if (!list_empty(&d->arch.vmca_msrs.impact_header)) { - entry =3D list_entry(d->arch.vmca_msrs.impact_header.next, - struct bank_entry, list); - if ( entry->bank =3D=3D (msr - MSR_IA32_MC0_STATUS)/4 ) { - *lo =3D entry->mci_status; - *hi =3D entry->mci_status >> 32; - gdprintk(XENLOG_DEBUG, "MCE: rdmsr MCi_STATUS in vmCE# = context " - "lo %x hi %x\n", *lo, *hi); + switch (msr & (MSR_IA32_MC0_CTL | 3)) + { + case MSR_IA32_MC0_CTL: + *lo =3D (u32)d->arch.vmca_msrs.mci_ctl[bank]; + *hi =3D (u32)(d->arch.vmca_msrs.mci_ctl[bank] >> 32); + gdprintk(XENLOG_DEBUG, "MCE: rd MC%u_CTL lo %x hi %x\n", + bank, *lo, *hi); + break; + case MSR_IA32_MC0_STATUS: + /* Only error bank is read. Non-error banks simply return. */ + if (!list_empty(&d->arch.vmca_msrs.impact_header)) { + entry =3D list_entry(d->arch.vmca_msrs.impact_header.next,= + struct bank_entry, list); + if (entry->bank =3D=3D bank) { + *lo =3D entry->mci_status; + *hi =3D entry->mci_status >> 32; + gdprintk(XENLOG_DEBUG, + "MCE: rd MC%u_STATUS in vmCE# context " + "lo %x hi %x\n", bank, *lo, *hi); + } else + entry =3D NULL; } - } - break; - case MSR_IA32_MC0_ADDR: - case MSR_IA32_MC1_ADDR: - case MSR_IA32_MC2_ADDR: - case MSR_IA32_MC3_ADDR: - case MSR_IA32_MC4_ADDR: - case MSR_IA32_MC5_ADDR: - case MSR_IA32_MC6_ADDR: - case MSR_IA32_MC7_ADDR: - case MSR_IA32_MC8_ADDR: - *lo =3D *hi =3D 0x0; - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "mce_rdmsr: not in domain context!\n"= ); + if (!entry) + gdprintk(XENLOG_DEBUG, "MCE: rd MC%u_STATUS\n", bank); break; - } - if (!list_empty(&d->arch.vmca_msrs.impact_header)) { - entry =3D list_entry(d->arch.vmca_msrs.impact_header.next, - struct bank_entry, list); - if ( entry->bank =3D=3D (msr - MSR_IA32_MC0_ADDR)/4 ) { - *lo =3D entry->mci_addr; - *hi =3D entry->mci_addr >> 32; - gdprintk(XENLOG_DEBUG, "MCE: rdmsr MCi_ADDR in vMCE# = context " - "lo %x hi %x\n", *lo, *hi); + case MSR_IA32_MC0_ADDR: + if (!list_empty(&d->arch.vmca_msrs.impact_header)) { + entry =3D list_entry(d->arch.vmca_msrs.impact_header.next,= + struct bank_entry, list); + if (entry->bank =3D=3D bank) { + *lo =3D entry->mci_addr; + *hi =3D entry->mci_addr >> 32; + gdprintk(XENLOG_DEBUG, + "MCE: rd MC%u_ADDR in vMCE# context lo %x hi = %x\n", + bank, *lo, *hi); + } } - } - break; - case MSR_IA32_MC0_MISC: - case MSR_IA32_MC1_MISC: - case MSR_IA32_MC2_MISC: - case MSR_IA32_MC3_MISC: - case MSR_IA32_MC4_MISC: - case MSR_IA32_MC5_MISC: - case MSR_IA32_MC6_MISC: - case MSR_IA32_MC7_MISC: - case MSR_IA32_MC8_MISC: - *lo =3D *hi =3D 0x0; - if (!d || is_idle_domain(d)) { - gdprintk(XENLOG_WARNING, "MCE: rdmsr not in domain context!\n"= ); break; - } - if (!list_empty(&d->arch.vmca_msrs.impact_header)) { - entry =3D list_entry(d->arch.vmca_msrs.impact_header.next, - struct bank_entry, list); - if ( entry->bank =3D=3D (msr - MSR_IA32_MC0_MISC)/4 ) { - *lo =3D entry->mci_misc; - *hi =3D entry->mci_misc >> 32; - gdprintk(XENLOG_DEBUG, "MCE: rdmsr MCi_MISC in vMCE# = context " - " lo %x hi %x\n", *lo, *hi); + case MSR_IA32_MC0_MISC: + if (!list_empty(&d->arch.vmca_msrs.impact_header)) { + entry =3D list_entry(d->arch.vmca_msrs.impact_header.next,= + struct bank_entry, list); + if (entry->bank =3D=3D bank) { + *lo =3D entry->mci_misc; + *hi =3D entry->mci_misc >> 32; + gdprintk(XENLOG_DEBUG, + "MCE: rd MC%u_MISC in vMCE# context lo %x hi = %x\n", + bank, *lo, *hi); + } } + break; } break; default: --- 2009-03-27.orig/xen/arch/x86/domain.c 2009-03-27 13:20:57.0000000= 00 +0100 +++ 2009-03-27/xen/arch/x86/domain.c 2009-03-27 13:21:43.000000000 = +0100 @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -373,7 +374,6 @@ void vcpu_destroy(struct vcpu *v) hvm_vcpu_destroy(v); } =20 -extern uint64_t g_mcg_cap; int arch_domain_create(struct domain *d, unsigned int domcr_flags) { #ifdef __x86_64__ @@ -458,14 +458,8 @@ int arch_domain_create(struct domain *d, goto fail; =20 /* For Guest vMCE MSRs virtualization */ - d->arch.vmca_msrs.mcg_status =3D 0x0; - d->arch.vmca_msrs.mcg_cap =3D g_mcg_cap; - d->arch.vmca_msrs.mcg_ctl =3D (uint64_t)~0x0; - d->arch.vmca_msrs.nr_injection =3D 0; - memset(d->arch.vmca_msrs.mci_ctl, 0x1, - sizeof(d->arch.vmca_msrs.mci_ctl)); - INIT_LIST_HEAD(&d->arch.vmca_msrs.impact_header); - + if ( boot_cpu_data.x86_vendor =3D=3D X86_VENDOR_INTEL ) + intel_mce_init_msr(d); } =20 if ( is_hvm_domain(d) ) --- 2009-03-27.orig/xen/arch/x86/traps.c 2009-03-27 10:10:57.0000000= 00 +0100 +++ 2009-03-27/xen/arch/x86/traps.c 2009-03-27 10:31:05.000000000 = +0100 @@ -1637,10 +1637,6 @@ static int is_cpufreq_controller(struct=20 (d->domain_id =3D=3D 0)); } =20 -/*Intel vMCE MSRs virtualization*/ -extern int intel_mce_wrmsr(u32 msr, u32 lo, u32 hi); -extern int intel_mce_rdmsr(u32 msr, u32 *lo, u32 *hi); - static int emulate_privileged_op(struct cpu_user_regs *regs) { struct vcpu *v =3D current; @@ -2210,10 +2206,10 @@ static int emulate_privileged_op(struct=20 break; if ( boot_cpu_data.x86_vendor =3D=3D X86_VENDOR_INTEL ) { - int rc =3D intel_mce_wrmsr(regs->ecx, eax, edx); - if ( rc =3D=3D -1 ) + int rc =3D intel_mce_wrmsr(regs->ecx, res); + if ( rc < 0 ) goto fail; - if ( rc =3D=3D 0 ) + if ( rc ) break; } =20 @@ -2291,25 +2287,27 @@ static int emulate_privileged_op(struct=20 default: if ( rdmsr_hypervisor_regs(regs->ecx, &l, &h) ) { + rdmsr_writeback: regs->eax =3D l; regs->edx =3D h; break; } - /* Everyone can read the MSR space. */ - /* gdprintk(XENLOG_WARNING,"Domain attempted RDMSR %p.\n", - _p(regs->ecx));*/ - if ( rdmsr_safe(regs->ecx, regs->eax, regs->edx) ) - goto fail; =20 if ( boot_cpu_data.x86_vendor =3D=3D X86_VENDOR_INTEL ) { - int rc =3D intel_mce_rdmsr(regs->ecx, &eax, &edx); - if ( rc =3D=3D -1 ) + int rc =3D intel_mce_rdmsr(regs->ecx, &l, &h); + + if ( rc < 0 ) goto fail; - if ( rc =3D=3D 0 ) - break; + if ( rc ) + goto rdmsr_writeback; } =20 + /* Everyone can read the MSR space. */ + /* gdprintk(XENLOG_WARNING,"Domain attempted RDMSR %p.\n", + _p(regs->ecx));*/ + if ( rdmsr_safe(regs->ecx, regs->eax, regs->edx) ) + goto fail; break; } break; --- 2009-03-27.orig/xen/include/asm-x86/traps.h 2009-03-27 13:20:57.0000000= 00 +0100 +++ 2009-03-27/xen/include/asm-x86/traps.h 2009-03-27 10:48:02.0000000= 00 +0100 @@ -47,4 +47,9 @@ extern int guest_has_trap_callback(struc extern int send_guest_trap(struct domain *d, uint16_t vcpuid, unsigned int trap_nr); =20 +/* Intel vMCE MSRs virtualization */ +extern void intel_mce_init_msr(struct domain *d); +extern int intel_mce_wrmsr(u32 msr, u64 value); +extern int intel_mce_rdmsr(u32 msr, u32 *lo, u32 *hi); + #endif /* ASM_TRAP_H */