| Message ID | 20170217063936.13208-16-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > @@ -190,6 +191,25 @@ int vmce_rdmsr(uint32_t msr, uint64_t *val) > *val = ~0ULL; > mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_CTL %#"PRIx64"\n", cur, *val); > break; > + case MSR_IA32_MCG_EXT_CTL: > + /* > + * If MCG_LMCE_P is present in guest MSR_IA32_MCG_CAP, the LMCE and LOCK > + * bits are always set in guest MSR_IA32_FEATURE_CONTROL by Xen , so it Stray blank before comma. > + * does not need to check them here. > + */ > + if ( !(cur->arch.vmce.mcg_cap & MCG_LMCE_P) ) Please invert the condition (swapping if and else blocks): This is not only easier to read, but also gives the compiler correct information on which case we expect to be the preferred (normal) one (at least in the long run). > + { > + ret = -1; > + mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL, not supported\n", > + cur); > + } > + else > + { > + *val = cur->arch.vmce.lmce_enabled ? MCG_EXT_CTL_LMCE_EN : 0; > + mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL %#"PRIx64"\n", > + cur, *val); > + } > + break; > default: Even if this isn't currently the case for the rest of this switch statement, please add blank lines between non-fall-through case blocks. > --- a/xen/include/asm-x86/mce.h > +++ b/xen/include/asm-x86/mce.h > @@ -29,6 +29,7 @@ struct vmce { > uint64_t mcg_status; > spinlock_t lock; > struct vmce_bank bank[GUEST_MC_BANK_NUM]; > + bool lmce_enabled; I think this better goes ahead of bank[]. > --- a/xen/include/public/arch-x86/hvm/save.h > +++ b/xen/include/public/arch-x86/hvm/save.h > @@ -599,6 +599,8 @@ struct hvm_vmce_vcpu { > uint64_t caps; > uint64_t mci_ctl2_bank0; > uint64_t mci_ctl2_bank1; > + uint8_t lmce_enabled; > + uint8_t _pad[7]; This implicitly is a domctl interface change, so you need to bump the interface version there (this hasn't happened yet afaics after 4.8 was branched off). Plus - no leading underscore please. All of this said - is this really a per-vCPU property, instead of a per-domain one? Jan
On 02/22/17 08:36 -0700, Jan Beulich wrote: > >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > > @@ -190,6 +191,25 @@ int vmce_rdmsr(uint32_t msr, uint64_t *val) > > *val = ~0ULL; > > mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_CTL %#"PRIx64"\n", cur, *val); > > break; > > + case MSR_IA32_MCG_EXT_CTL: > > + /* > > + * If MCG_LMCE_P is present in guest MSR_IA32_MCG_CAP, the LMCE and LOCK > > + * bits are always set in guest MSR_IA32_FEATURE_CONTROL by Xen , so it > > Stray blank before comma. > > > + * does not need to check them here. > > + */ > > + if ( !(cur->arch.vmce.mcg_cap & MCG_LMCE_P) ) > > Please invert the condition (swapping if and else blocks): This is not > only easier to read, but also gives the compiler correct information > on which case we expect to be the preferred (normal) one (at least > in the long run). > will do > > + { > > + ret = -1; > > + mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL, not supported\n", > > + cur); > > + } > > + else > > + { > > + *val = cur->arch.vmce.lmce_enabled ? MCG_EXT_CTL_LMCE_EN : 0; > > + mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL %#"PRIx64"\n", > > + cur, *val); > > + } > > + break; > > default: > > Even if this isn't currently the case for the rest of this switch > statement, please add blank lines between non-fall-through case > blocks. > ditto > > --- a/xen/include/asm-x86/mce.h > > +++ b/xen/include/asm-x86/mce.h > > @@ -29,6 +29,7 @@ struct vmce { > > uint64_t mcg_status; > > spinlock_t lock; > > struct vmce_bank bank[GUEST_MC_BANK_NUM]; > > + bool lmce_enabled; > > I think this better goes ahead of bank[]. > ditto > > --- a/xen/include/public/arch-x86/hvm/save.h > > +++ b/xen/include/public/arch-x86/hvm/save.h > > @@ -599,6 +599,8 @@ struct hvm_vmce_vcpu { > > uint64_t caps; > > uint64_t mci_ctl2_bank0; > > uint64_t mci_ctl2_bank1; > > + uint8_t lmce_enabled; > > + uint8_t _pad[7]; > > This implicitly is a domctl interface change, so you need to bump the > interface version there (this hasn't happened yet afaics after 4.8 > was branched off). > ditto > Plus - no leading underscore please. ditto > > All of this said - is this really a per-vCPU property, instead of a > per-domain one? Per-vCPU. At least it can be set in the per-CPU way. Patch 16, which implements the vLMCE injection, checks this per-vcpu flag when injecting vMCE. If the flag is cleared, vMCE (w/ MCG_STATUS_LMCE removed) will be broadcast to all vcpus. Thanks, Haozhong
>>> On 23.02.17 at 05:26, <haozhong.zhang@intel.com> wrote: > On 02/22/17 08:36 -0700, Jan Beulich wrote: >> >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: >> All of this said - is this really a per-vCPU property, instead of a >> per-domain one? > > Per-vCPU. At least it can be set in the per-CPU way. Patch 16, which > implements the vLMCE injection, checks this per-vcpu flag when > injecting vMCE. If the flag is cleared, vMCE (w/ MCG_STATUS_LMCE > removed) will be broadcast to all vcpus. You answer my question based on the mechanics of your patches, but the question was from a conceptual / architectural perspective. Jan
On 02/23/17 00:53 -0700, Jan Beulich wrote: > >>> On 23.02.17 at 05:26, <haozhong.zhang@intel.com> wrote: > > On 02/22/17 08:36 -0700, Jan Beulich wrote: > >> >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > >> All of this said - is this really a per-vCPU property, instead of a > >> per-domain one? > > > > Per-vCPU. At least it can be set in the per-CPU way. Patch 16, which > > implements the vLMCE injection, checks this per-vcpu flag when > > injecting vMCE. If the flag is cleared, vMCE (w/ MCG_STATUS_LMCE > > removed) will be broadcast to all vcpus. > > You answer my question based on the mechanics of your patches, > but the question was from a conceptual / architectural perspective. > LMCE can be enabled by SW in the per-CPU way on the real hardware, i.e. it can be enabled on some CPUs while disabled on others. lmce_enabled is to track whether guest SW enables LMCE on a vcpu, so it should be a per-vcpu property. Haozhong
>>> On 23.02.17 at 09:54, <haozhong.zhang@intel.com> wrote: > On 02/23/17 00:53 -0700, Jan Beulich wrote: >> >>> On 23.02.17 at 05:26, <haozhong.zhang@intel.com> wrote: >> > On 02/22/17 08:36 -0700, Jan Beulich wrote: >> >> >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: >> >> All of this said - is this really a per-vCPU property, instead of a >> >> per-domain one? >> > >> > Per-vCPU. At least it can be set in the per-CPU way. Patch 16, which >> > implements the vLMCE injection, checks this per-vcpu flag when >> > injecting vMCE. If the flag is cleared, vMCE (w/ MCG_STATUS_LMCE >> > removed) will be broadcast to all vcpus. >> >> You answer my question based on the mechanics of your patches, >> but the question was from a conceptual / architectural perspective. >> > > LMCE can be enabled by SW in the per-CPU way on the real hardware, > i.e. it can be enabled on some CPUs while disabled on others. > lmce_enabled is to track whether guest SW enables LMCE on a vcpu, so > it should be a per-vcpu property. Good point. Jan
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c index 456d6f3..1278839 100644 --- a/xen/arch/x86/cpu/mcheck/vmce.c +++ b/xen/arch/x86/cpu/mcheck/vmce.c @@ -90,6 +90,7 @@ int vmce_restore_vcpu(struct vcpu *v, const struct hvm_vmce_vcpu *ctxt) v->arch.vmce.mcg_cap = ctxt->caps; v->arch.vmce.bank[0].mci_ctl2 = ctxt->mci_ctl2_bank0; v->arch.vmce.bank[1].mci_ctl2 = ctxt->mci_ctl2_bank1; + v->arch.vmce.lmce_enabled = ctxt->lmce_enabled; return 0; } @@ -190,6 +191,25 @@ int vmce_rdmsr(uint32_t msr, uint64_t *val) *val = ~0ULL; mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_CTL %#"PRIx64"\n", cur, *val); break; + case MSR_IA32_MCG_EXT_CTL: + /* + * If MCG_LMCE_P is present in guest MSR_IA32_MCG_CAP, the LMCE and LOCK + * bits are always set in guest MSR_IA32_FEATURE_CONTROL by Xen , so it + * does not need to check them here. + */ + if ( !(cur->arch.vmce.mcg_cap & MCG_LMCE_P) ) + { + ret = -1; + mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL, not supported\n", + cur); + } + else + { + *val = cur->arch.vmce.lmce_enabled ? MCG_EXT_CTL_LMCE_EN : 0; + mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL %#"PRIx64"\n", + cur, *val); + } + break; default: ret = mce_bank_msr(cur, msr) ? bank_mce_rdmsr(cur, msr, val) : 0; break; @@ -290,6 +310,15 @@ int vmce_wrmsr(uint32_t msr, uint64_t val) */ mce_printk(MCE_VERBOSE, "MCE: %pv: MCG_CAP is r/o\n", cur); break; + case MSR_IA32_MCG_EXT_CTL: + if ( !(cur->arch.vmce.mcg_cap & MCG_LMCE_P) || + (val & ~MCG_EXT_CTL_LMCE_EN) ) + ret = -1; + else + cur->arch.vmce.lmce_enabled = !!(val & MCG_EXT_CTL_LMCE_EN); + mce_printk(MCE_VERBOSE, "MCE: %pv: wr MCG_EXT_CTL %"PRIx64"%s\n", + cur, val, (ret == -1) ? ", not supported" : ""); + break; default: ret = mce_bank_msr(cur, msr) ? bank_mce_wrmsr(cur, msr, val) : 0; break; @@ -308,7 +337,8 @@ static int vmce_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) struct hvm_vmce_vcpu ctxt = { .caps = v->arch.vmce.mcg_cap, .mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2, - .mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2 + .mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2, + .lmce_enabled = v->arch.vmce.lmce_enabled, }; err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt); diff --git a/xen/include/asm-x86/mce.h b/xen/include/asm-x86/mce.h index 6b827ef..525a9e8 100644 --- a/xen/include/asm-x86/mce.h +++ b/xen/include/asm-x86/mce.h @@ -29,6 +29,7 @@ struct vmce { uint64_t mcg_status; spinlock_t lock; struct vmce_bank bank[GUEST_MC_BANK_NUM]; + bool lmce_enabled; }; /* Guest vMCE MSRs virtualization */ diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h index 8d73b51..2d62ec3 100644 --- a/xen/include/public/arch-x86/hvm/save.h +++ b/xen/include/public/arch-x86/hvm/save.h @@ -599,6 +599,8 @@ struct hvm_vmce_vcpu { uint64_t caps; uint64_t mci_ctl2_bank0; uint64_t mci_ctl2_bank1; + uint8_t lmce_enabled; + uint8_t _pad[7]; }; DECLARE_HVM_SAVE_TYPE(VMCE_VCPU, 18, struct hvm_vmce_vcpu);
If MCG_LMCE_P is present in guest MSR_IA32_MCG_CAP, then allow guest to read/write MSR_IA32_MCG_EXT_CTL. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- Cc: Christoph Egger <chegger@amazon.de> Cc: Liu Jinsong <jinsong.liu@alibaba-inc.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/cpu/mcheck/vmce.c | 32 +++++++++++++++++++++++++++++++- xen/include/asm-x86/mce.h | 1 + xen/include/public/arch-x86/hvm/save.h | 2 ++ 3 files changed, 34 insertions(+), 1 deletion(-)