Liu, Jinsong
2012-Jul-23 09:39 UTC
[Patch 2/6] Xen/MCE: remove mcg_ctl and other adjustment for future vMCE
Xen/MCE: remove mcg_ctl and other adjustment for future vMCE This patch is a middle-work patch, prepare for future new vMCE model. It remove mcg_ctl, disable MCG_CTL_P, and set bank number to 2. Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> diff -r 8067891037a6 xen/arch/x86/cpu/mcheck/mce.c --- a/xen/arch/x86/cpu/mcheck/mce.c Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/mce.c Thu Jul 19 21:38:15 2012 +0800 @@ -843,8 +843,6 @@ mctelem_init(sizeof(struct mc_info)); - vmce_init(c); - /* Turn on MCE now */ set_in_cr4(X86_CR4_MCE); diff -r 8067891037a6 xen/arch/x86/cpu/mcheck/mce.h --- a/xen/arch/x86/cpu/mcheck/mce.h Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/mce.h Thu Jul 19 21:38:15 2012 +0800 @@ -170,8 +170,6 @@ int inject_vmce(struct domain *d); int vmce_domain_inject(struct mcinfo_bank *bank, struct domain *d, struct mcinfo_global *global); -extern int vmce_init(struct cpuinfo_x86 *c); - static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr) { if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && diff -r 8067891037a6 xen/arch/x86/cpu/mcheck/vmce.c --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/vmce.c Thu Jul 19 21:38:15 2012 +0800 @@ -19,13 +19,18 @@ #include "mce.h" #include "x86_mca.h" +/* + * Emulalte 2 banks for guest + * Bank0: reserved for ''bank0 quirk'' occur at some very old processors: + * 1). Intel cpu whose family-model value < 06-1A; + * 2). AMD K7 + * Bank1: used to transfer error info to guest + */ +#define GUEST_BANK_NUM 2 +#define GUEST_MCG_CAP (MCG_TES_P | MCG_SER_P | GUEST_BANK_NUM) + #define dom_vmce(x) ((x)->arch.vmca_msrs) -static uint64_t __read_mostly g_mcg_cap; - -/* Real value in physical CTL MSR */ -static uint64_t __read_mostly h_mcg_ctl; - int vmce_init_msr(struct domain *d) { dom_vmce(d) = xmalloc(struct domain_mca_msrs); @@ -33,7 +38,6 @@ return -ENOMEM; dom_vmce(d)->mcg_status = 0x0; - dom_vmce(d)->mcg_ctl = ~(uint64_t)0x0; dom_vmce(d)->nr_injection = 0; INIT_LIST_HEAD(&dom_vmce(d)->impact_header); @@ -52,17 +56,17 @@ void vmce_init_vcpu(struct vcpu *v) { - v->arch.mcg_cap = g_mcg_cap; + v->arch.mcg_cap = GUEST_MCG_CAP; } int vmce_restore_vcpu(struct vcpu *v, uint64_t caps) { - if ( caps & ~g_mcg_cap & ~MCG_CAP_COUNT & ~MCG_CTL_P ) + if ( caps & ~GUEST_MCG_CAP & ~MCG_CAP_COUNT & ~MCG_CTL_P ) { dprintk(XENLOG_G_ERR, "%s restore: unsupported MCA capabilities" " %#" PRIx64 " for d%d:v%u (supported: %#Lx)\n", is_hvm_vcpu(v) ? "HVM" : "PV", caps, v->domain->domain_id, - v->vcpu_id, g_mcg_cap & ~MCG_CAP_COUNT); + v->vcpu_id, GUEST_MCG_CAP & ~MCG_CAP_COUNT); return -EPERM; } @@ -175,11 +179,16 @@ *val); break; case MSR_IA32_MCG_CTL: - /* Always 0 if no CTL support */ if ( cur->arch.mcg_cap & MCG_CTL_P ) - *val = vmce->mcg_ctl & h_mcg_ctl; - mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", - *val); + { + *val = ~0UL; + mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", *val); + } + else + { + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); + ret = -1; + } break; default: ret = mce_bank_msr(cur, msr) ? bank_mce_rdmsr(cur, msr, val) : 0; @@ -287,15 +296,16 @@ struct domain_mca_msrs *vmce = dom_vmce(cur->domain); int ret = 1; - if ( !g_mcg_cap ) - return 0; - spin_lock(&vmce->lock); switch ( msr ) { case MSR_IA32_MCG_CTL: - vmce->mcg_ctl = val; + if ( !(cur->arch.mcg_cap & MCG_CTL_P) ) + { + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); + ret = -1; + } break; case MSR_IA32_MCG_STATUS: vmce->mcg_status = val; @@ -510,31 +520,6 @@ } #endif -int vmce_init(struct cpuinfo_x86 *c) -{ - u64 value; - - rdmsrl(MSR_IA32_MCG_CAP, value); - /* For Guest vMCE usage */ - g_mcg_cap = value & (MCG_CAP_COUNT | MCG_CTL_P | MCG_TES_P | MCG_SER_P); - if (value & MCG_CTL_P) - rdmsrl(MSR_IA32_MCG_CTL, h_mcg_ctl); - - return 0; -} - -static int mca_ctl_conflict(struct mcinfo_bank *bank, struct domain *d) -{ - if ( !bank || !d ) - return 1; - - /* Will MCE happen in host if If host mcg_ctl is 0? */ - if ( ~d->arch.vmca_msrs->mcg_ctl & h_mcg_ctl ) - return 1; - - return 0; -} - static int is_hvm_vmce_ready(struct mcinfo_bank *bank, struct domain *d) { struct vcpu *v; @@ -588,14 +573,6 @@ if (no_vmce) return 0; - /* Guest has different MCE ctl value setting */ - if (mca_ctl_conflict(bank, d)) - { - dprintk(XENLOG_WARNING, - "No vmce, guest has different mca control setting\n"); - return 0; - } - return 1; } diff -r 8067891037a6 xen/include/asm-x86/mce.h --- a/xen/include/asm-x86/mce.h Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/include/asm-x86/mce.h Thu Jul 19 21:38:15 2012 +0800 @@ -16,7 +16,6 @@ struct domain_mca_msrs { /* Guest should not change below values after DOM boot up */ - uint64_t mcg_ctl; uint64_t mcg_status; uint16_t nr_injection; struct list_head impact_header; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2012-Jul-30 13:22 UTC
Re: [Patch 2/6] Xen/MCE: remove mcg_ctl and other adjustment for future vMCE
>>> On 23.07.12 at 11:39, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: > @@ -175,11 +179,16 @@ > *val); > break; > case MSR_IA32_MCG_CTL: > - /* Always 0 if no CTL support */ > if ( cur->arch.mcg_cap & MCG_CTL_P ) > - *val = vmce->mcg_ctl & h_mcg_ctl; > - mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", > - *val); > + { > + *val = ~0UL; > + mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", *val); > + } > + else > + { > + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); > + ret = -1;Is there a particular reason to make this access fault here, when it didn''t before? I.e. was there anything wrong with the previous approach of returning zero on reads and ignoring writes when !MCG_CTL_P?> + } > break; > default: > ret = mce_bank_msr(cur, msr) ? bank_mce_rdmsr(cur, msr, val) : 0; > @@ -287,15 +296,16 @@ > struct domain_mca_msrs *vmce = dom_vmce(cur->domain); > int ret = 1; > > - if ( !g_mcg_cap ) > - return 0; > - > spin_lock(&vmce->lock); > > switch ( msr ) > { > case MSR_IA32_MCG_CTL: > - vmce->mcg_ctl = val; > + if ( !(cur->arch.mcg_cap & MCG_CTL_P) ) > + { > + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); > + ret = -1; > + } > break; > case MSR_IA32_MCG_STATUS: > vmce->mcg_status = val;Other than that, the patch looks fine to me. Jan
Liu, Jinsong
2012-Aug-05 10:24 UTC
Re: [Patch 2/6] Xen/MCE: remove mcg_ctl and other adjustment for future vMCE
Jan Beulich wrote:>>>> On 23.07.12 at 11:39, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: >> @@ -175,11 +179,16 @@ >> *val); >> break; >> case MSR_IA32_MCG_CTL: >> - /* Always 0 if no CTL support */ >> if ( cur->arch.mcg_cap & MCG_CTL_P ) >> - *val = vmce->mcg_ctl & h_mcg_ctl; >> - mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", >> - *val); >> + { >> + *val = ~0UL; >> + mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL >> 0x%"PRIx64"\n", *val); + } + else >> + { >> + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); >> + ret = -1; > > Is there a particular reason to make this access fault here, when > it didn''t before? I.e. was there anything wrong with the previous > approach of returning zero on reads and ignoring writes when > !MCG_CTL_P? >Semantically this code is better than previous approach, since !MCG_CTL_P means unimplemented MCG_CTL so access it would generate GP#. Thanks, Jinsong>> + } >> break; >> default: >> ret = mce_bank_msr(cur, msr) ? bank_mce_rdmsr(cur, msr, >> val) : 0; @@ -287,15 +296,16 @@ struct domain_mca_msrs *vmce >> dom_vmce(cur->domain); int ret = 1; >> >> - if ( !g_mcg_cap ) >> - return 0; >> - >> spin_lock(&vmce->lock); >> >> switch ( msr ) >> { >> case MSR_IA32_MCG_CTL: >> - vmce->mcg_ctl = val; >> + if ( !(cur->arch.mcg_cap & MCG_CTL_P) ) >> + { >> + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); + >> ret = -1; + } >> break; >> case MSR_IA32_MCG_STATUS: >> vmce->mcg_status = val; > > Other than that, the patch looks fine to me. > > Jan
Jan Beulich
2012-Aug-06 06:49 UTC
Re: [Patch 2/6] Xen/MCE: remove mcg_ctl and other adjustment for future vMCE
>>> On 05.08.12 at 12:24, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: > Jan Beulich wrote: >>>>> On 23.07.12 at 11:39, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: >>> @@ -175,11 +179,16 @@ >>> *val); >>> break; >>> case MSR_IA32_MCG_CTL: >>> - /* Always 0 if no CTL support */ >>> if ( cur->arch.mcg_cap & MCG_CTL_P ) >>> - *val = vmce->mcg_ctl & h_mcg_ctl; >>> - mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", >>> - *val); >>> + { >>> + *val = ~0UL; >>> + mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL >>> 0x%"PRIx64"\n", *val); + } + else >>> + { >>> + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); >>> + ret = -1; >> >> Is there a particular reason to make this access fault here, when >> it didn''t before? I.e. was there anything wrong with the previous >> approach of returning zero on reads and ignoring writes when >> !MCG_CTL_P? >> > > Semantically this code is better than previous approach, since !MCG_CTL_P > means unimplemented MCG_CTL so access it would generate GP#.Agreed. But nevertheless I''d like to be a little more conservative here. After all, "knowing" that this won''t break Windows or Linux isn''t covering all possible HVM guests (and the quotes are there to indicate that (a) unless you have access to Windows sources, you can''t really know, you may at best have empirical data suggesting so, and (b) makes you/us dependent on all older Windows/Linux versions you didn''t try out/look at behave correctly here too). Jan
Liu, Jinsong
2012-Aug-06 13:51 UTC
Re: [Patch 2/6] Xen/MCE: remove mcg_ctl and other adjustment for future vMCE
>>> >>> Is there a particular reason to make this access fault here, when >>> it didn''t before? I.e. was there anything wrong with the previous >>> approach of returning zero on reads and ignoring writes when >>> !MCG_CTL_P? >>> >> >> Semantically this code is better than previous approach, since >> !MCG_CTL_P means unimplemented MCG_CTL so access it would generate >> GP#. > > Agreed. But nevertheless I''d like to be a little more conservative > here. After all, "knowing" that this won''t break Windows or Linux > isn''t covering all possible HVM guests (and the quotes are there > to indicate that (a) unless you have access to Windows sources, > you can''t really know, you may at best have empirical data > suggesting so, and (b) makes you/us dependent on all older > Windows/Linux versions you didn''t try out/look at behave > correctly here too).OK, fine to me to use previous approach, updated as attached. Thanks, Jinsong ============================== Xen/MCE: remove mcg_ctl and other adjustment for future vMCE This patch is a middle-work patch, prepare for future new vMCE model. It remove mcg_ctl, disable MCG_CTL_P, and set bank number to 2. Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> diff -r 8067891037a6 xen/arch/x86/cpu/mcheck/mce.c --- a/xen/arch/x86/cpu/mcheck/mce.c Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/mce.c Tue Aug 07 04:26:56 2012 +0800 @@ -843,8 +843,6 @@ mctelem_init(sizeof(struct mc_info)); - vmce_init(c); - /* Turn on MCE now */ set_in_cr4(X86_CR4_MCE); diff -r 8067891037a6 xen/arch/x86/cpu/mcheck/mce.h --- a/xen/arch/x86/cpu/mcheck/mce.h Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/mce.h Tue Aug 07 04:26:56 2012 +0800 @@ -170,8 +170,6 @@ int inject_vmce(struct domain *d); int vmce_domain_inject(struct mcinfo_bank *bank, struct domain *d, struct mcinfo_global *global); -extern int vmce_init(struct cpuinfo_x86 *c); - static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr) { if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && diff -r 8067891037a6 xen/arch/x86/cpu/mcheck/vmce.c --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/vmce.c Tue Aug 07 04:26:56 2012 +0800 @@ -19,13 +19,18 @@ #include "mce.h" #include "x86_mca.h" +/* + * Emulalte 2 banks for guest + * Bank0: reserved for ''bank0 quirk'' occur at some very old processors: + * 1). Intel cpu whose family-model value < 06-1A; + * 2). AMD K7 + * Bank1: used to transfer error info to guest + */ +#define GUEST_BANK_NUM 2 +#define GUEST_MCG_CAP (MCG_TES_P | MCG_SER_P | GUEST_BANK_NUM) + #define dom_vmce(x) ((x)->arch.vmca_msrs) -static uint64_t __read_mostly g_mcg_cap; - -/* Real value in physical CTL MSR */ -static uint64_t __read_mostly h_mcg_ctl; - int vmce_init_msr(struct domain *d) { dom_vmce(d) = xmalloc(struct domain_mca_msrs); @@ -33,7 +38,6 @@ return -ENOMEM; dom_vmce(d)->mcg_status = 0x0; - dom_vmce(d)->mcg_ctl = ~(uint64_t)0x0; dom_vmce(d)->nr_injection = 0; INIT_LIST_HEAD(&dom_vmce(d)->impact_header); @@ -52,17 +56,17 @@ void vmce_init_vcpu(struct vcpu *v) { - v->arch.mcg_cap = g_mcg_cap; + v->arch.mcg_cap = GUEST_MCG_CAP; } int vmce_restore_vcpu(struct vcpu *v, uint64_t caps) { - if ( caps & ~g_mcg_cap & ~MCG_CAP_COUNT & ~MCG_CTL_P ) + if ( caps & ~GUEST_MCG_CAP & ~MCG_CAP_COUNT & ~MCG_CTL_P ) { dprintk(XENLOG_G_ERR, "%s restore: unsupported MCA capabilities" " %#" PRIx64 " for d%d:v%u (supported: %#Lx)\n", is_hvm_vcpu(v) ? "HVM" : "PV", caps, v->domain->domain_id, - v->vcpu_id, g_mcg_cap & ~MCG_CAP_COUNT); + v->vcpu_id, GUEST_MCG_CAP & ~MCG_CAP_COUNT); return -EPERM; } @@ -175,11 +179,10 @@ *val); break; case MSR_IA32_MCG_CTL: - /* Always 0 if no CTL support */ + /* Stick all 1''s when CTL support, and 0''s when no CTL support */ if ( cur->arch.mcg_cap & MCG_CTL_P ) - *val = vmce->mcg_ctl & h_mcg_ctl; - mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", - *val); + *val = ~0ULL; + mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n", *val); break; default: ret = mce_bank_msr(cur, msr) ? bank_mce_rdmsr(cur, msr, val) : 0; @@ -287,15 +290,11 @@ struct domain_mca_msrs *vmce = dom_vmce(cur->domain); int ret = 1; - if ( !g_mcg_cap ) - return 0; - spin_lock(&vmce->lock); switch ( msr ) { case MSR_IA32_MCG_CTL: - vmce->mcg_ctl = val; break; case MSR_IA32_MCG_STATUS: vmce->mcg_status = val; @@ -510,31 +509,6 @@ } #endif -int vmce_init(struct cpuinfo_x86 *c) -{ - u64 value; - - rdmsrl(MSR_IA32_MCG_CAP, value); - /* For Guest vMCE usage */ - g_mcg_cap = value & (MCG_CAP_COUNT | MCG_CTL_P | MCG_TES_P | MCG_SER_P); - if (value & MCG_CTL_P) - rdmsrl(MSR_IA32_MCG_CTL, h_mcg_ctl); - - return 0; -} - -static int mca_ctl_conflict(struct mcinfo_bank *bank, struct domain *d) -{ - if ( !bank || !d ) - return 1; - - /* Will MCE happen in host if If host mcg_ctl is 0? */ - if ( ~d->arch.vmca_msrs->mcg_ctl & h_mcg_ctl ) - return 1; - - return 0; -} - static int is_hvm_vmce_ready(struct mcinfo_bank *bank, struct domain *d) { struct vcpu *v; @@ -588,14 +562,6 @@ if (no_vmce) return 0; - /* Guest has different MCE ctl value setting */ - if (mca_ctl_conflict(bank, d)) - { - dprintk(XENLOG_WARNING, - "No vmce, guest has different mca control setting\n"); - return 0; - } - return 1; } diff -r 8067891037a6 xen/include/asm-x86/mce.h --- a/xen/include/asm-x86/mce.h Thu Jul 19 20:48:25 2012 +0800 +++ b/xen/include/asm-x86/mce.h Tue Aug 07 04:26:56 2012 +0800 @@ -16,7 +16,6 @@ struct domain_mca_msrs { /* Guest should not change below values after DOM boot up */ - uint64_t mcg_ctl; uint64_t mcg_status; uint16_t nr_injection; struct list_head impact_header; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2012-Aug-07 06:21 UTC
Re: [Patch 2/6] Xen/MCE: remove mcg_ctl and other adjustment for future vMCE
>>> On 06.08.12 at 15:51, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: >>>> >>>> Is there a particular reason to make this access fault here, when >>>> it didn''t before? I.e. was there anything wrong with the previous >>>> approach of returning zero on reads and ignoring writes when >>>> !MCG_CTL_P? >>>> >>> >>> Semantically this code is better than previous approach, since >>> !MCG_CTL_P means unimplemented MCG_CTL so access it would generate >>> GP#. >> >> Agreed. But nevertheless I''d like to be a little more conservative >> here. After all, "knowing" that this won''t break Windows or Linux >> isn''t covering all possible HVM guests (and the quotes are there >> to indicate that (a) unless you have access to Windows sources, >> you can''t really know, you may at best have empirical data >> suggesting so, and (b) makes you/us dependent on all older >> Windows/Linux versions you didn''t try out/look at behave >> correctly here too). > > OK, fine to me to use previous approach, updated as attached.Thanks, committed. Now what''s your take on pulling the previous patch #5 ahead (at once addressing the compatibility issue I pointed out), as that''s really the meat of the save/restore forward compatibility? The other four patches could then be postponed until after 4.2 went out afaict. Jan