X86/MCE: update vMCE injection for AMD
For Intel MCE, it broadcasts vMCE to all vcpus. For AMD MCE, it injects
vMCE only to vcpu0. This patch update inject_vmce for AMD.
Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
Suggested_by: Christoph Egger <Christoph.Egger@amd.com>
diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce.h
--- a/xen/arch/x86/cpu/mcheck/mce.h Thu Sep 20 00:03:25 2012 +0800
+++ b/xen/arch/x86/cpu/mcheck/mce.h Tue Sep 25 19:52:20 2012 +0800
@@ -168,7 +168,7 @@
int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,
uint64_t gstatus);
-int inject_vmce(struct domain *d);
+int inject_vmce(struct domain *d, bool_t vmce_broadcast);
static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr)
{
diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce_intel.c
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c Thu Sep 20 00:03:25 2012 +0800
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c Tue Sep 25 19:52:20 2012 +0800
@@ -365,7 +365,7 @@
}
/* We will inject vMCE to DOMU*/
- if ( inject_vmce(d) < 0 )
+ if ( inject_vmce(d, 1) < 0 )
{
mce_printk(MCE_QUIET, "inject vMCE to DOM%d"
" failed\n", d->domain_id);
diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/vmce.c
--- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 20 00:03:25 2012 +0800
+++ b/xen/arch/x86/cpu/mcheck/vmce.c Tue Sep 25 19:52:20 2012 +0800
@@ -344,11 +344,14 @@
HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, vmce_save_vcpu_ctxt,
vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
-int inject_vmce(struct domain *d)
+/*
+ * for Intel MCE, broadcast vMCE to all vcpus
+ * for AMD MCE, only inject vMCE to vcpu0
+ */
+int inject_vmce(struct domain *d, bool_t vmce_broadcast)
{
struct vcpu *v;
- /* inject vMCE to all vcpus */
for_each_vcpu(d, v)
{
if ( !test_and_set_bool(v->mce_pending) &&
@@ -365,6 +368,9 @@
d->domain_id, v->vcpu_id);
return -1;
}
+
+ if ( !vmce_broadcast )
+ break;
}
return 0;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Christoph Egger
2012-Sep-25 09:06 UTC
Re: [PATCH 6] X86/MCE: update vMCE injection for AMD
On 09/25/12 07:00, Liu, Jinsong wrote:> X86/MCE: update vMCE injection for AMD > > For Intel MCE, it broadcasts vMCE to all vcpus. For AMD MCE, it injects > vMCE only to vcpu0. This patch update inject_vmce for AMD. > > Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> > Suggested_by: Christoph Egger <Christoph.Egger@amd.com>Acked-by: Christoph Egger <Christoph.Egger@amd.com>> > diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce.h > --- a/xen/arch/x86/cpu/mcheck/mce.h Thu Sep 20 00:03:25 2012 +0800 > +++ b/xen/arch/x86/cpu/mcheck/mce.h Tue Sep 25 19:52:20 2012 +0800 > @@ -168,7 +168,7 @@ > > int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, > uint64_t gstatus); > -int inject_vmce(struct domain *d); > +int inject_vmce(struct domain *d, bool_t vmce_broadcast); > > static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr) > { > diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce_intel.c > --- a/xen/arch/x86/cpu/mcheck/mce_intel.c Thu Sep 20 00:03:25 2012 +0800 > +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c Tue Sep 25 19:52:20 2012 +0800 > @@ -365,7 +365,7 @@ > } > > /* We will inject vMCE to DOMU*/ > - if ( inject_vmce(d) < 0 ) > + if ( inject_vmce(d, 1) < 0 ) > { > mce_printk(MCE_QUIET, "inject vMCE to DOM%d" > " failed\n", d->domain_id); > diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/vmce.c > --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 20 00:03:25 2012 +0800 > +++ b/xen/arch/x86/cpu/mcheck/vmce.c Tue Sep 25 19:52:20 2012 +0800 > @@ -344,11 +344,14 @@ > HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, vmce_save_vcpu_ctxt, > vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU); > > -int inject_vmce(struct domain *d) > +/* > + * for Intel MCE, broadcast vMCE to all vcpus > + * for AMD MCE, only inject vMCE to vcpu0 > + */ > +int inject_vmce(struct domain *d, bool_t vmce_broadcast) > { > struct vcpu *v; > > - /* inject vMCE to all vcpus */ > for_each_vcpu(d, v) > { > if ( !test_and_set_bool(v->mce_pending) && > @@ -365,6 +368,9 @@ > d->domain_id, v->vcpu_id); > return -1; > } > + > + if ( !vmce_broadcast ) > + break; > } > > return 0;-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
>>> On 25.09.12 at 11:06, Christoph Egger <Christoph.Egger@amd.com> wrote: > On 09/25/12 07:00, Liu, Jinsong wrote: > >> X86/MCE: update vMCE injection for AMD >> >> For Intel MCE, it broadcasts vMCE to all vcpus. For AMD MCE, it injects >> vMCE only to vcpu0. This patch update inject_vmce for AMD. >> >> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> >> Suggested_by: Christoph Egger <Christoph.Egger@amd.com> > > > Acked-by: Christoph Egger <Christoph.Egger@amd.com>Are you sure (see below)?>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce.h >> --- a/xen/arch/x86/cpu/mcheck/mce.h Thu Sep 20 00:03:25 2012 +0800 >> +++ b/xen/arch/x86/cpu/mcheck/mce.h Tue Sep 25 19:52:20 2012 +0800 >> @@ -168,7 +168,7 @@ >> >> int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, >> uint64_t gstatus); >> -int inject_vmce(struct domain *d); >> +int inject_vmce(struct domain *d, bool_t vmce_broadcast); >> >> static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr) >> { >> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce_intel.c >> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c Thu Sep 20 00:03:25 2012 +0800 >> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c Tue Sep 25 19:52:20 2012 +0800 >> @@ -365,7 +365,7 @@ >> } >> >> /* We will inject vMCE to DOMU*/ >> - if ( inject_vmce(d) < 0 ) >> + if ( inject_vmce(d, 1) < 0 ) >> { >> mce_printk(MCE_QUIET, "inject vMCE to DOM%d" >> " failed\n", d->domain_id); >> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/vmce.c >> --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 20 00:03:25 2012 +0800 >> +++ b/xen/arch/x86/cpu/mcheck/vmce.c Tue Sep 25 19:52:20 2012 +0800 >> @@ -344,11 +344,14 @@ >> HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, vmce_save_vcpu_ctxt, >> vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU); >> >> -int inject_vmce(struct domain *d) >> +/* >> + * for Intel MCE, broadcast vMCE to all vcpus >> + * for AMD MCE, only inject vMCE to vcpu0 >> + */ >> +int inject_vmce(struct domain *d, bool_t vmce_broadcast) >> { >> struct vcpu *v; >> >> - /* inject vMCE to all vcpus */ >> for_each_vcpu(d, v) >> { >> if ( !test_and_set_bool(v->mce_pending) && >> @@ -365,6 +368,9 @@ >> d->domain_id, v->vcpu_id); >> return -1; >> } >> + >> + if ( !vmce_broadcast ) >> + break;That''ll allow (non-broadcast) injection to vCPU 0 only - is that really the right thing to do? I.e. shouldn''t the caller rather be given flexibility to specify which vCPU this is to go to (with a negative value meaning broadcast)? And I''m intending to fold this into patch 2 anyway before committing. Jan
Christoph Egger
2012-Sep-25 11:54 UTC
Re: [PATCH 6] X86/MCE: update vMCE injection for AMD
On 09/25/12 12:48, Jan Beulich wrote:>>>> On 25.09.12 at 11:06, Christoph Egger <Christoph.Egger@amd.com> wrote: >> On 09/25/12 07:00, Liu, Jinsong wrote: >> >>> X86/MCE: update vMCE injection for AMD >>> >>> For Intel MCE, it broadcasts vMCE to all vcpus. For AMD MCE, it injects >>> vMCE only to vcpu0. This patch update inject_vmce for AMD. >>> >>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> >>> Suggested_by: Christoph Egger <Christoph.Egger@amd.com> >> >> >> Acked-by: Christoph Egger <Christoph.Egger@amd.com> > > Are you sure (see below)?Yes, see below.>>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce.h >>> --- a/xen/arch/x86/cpu/mcheck/mce.h Thu Sep 20 00:03:25 2012 +0800 >>> +++ b/xen/arch/x86/cpu/mcheck/mce.h Tue Sep 25 19:52:20 2012 +0800 >>> @@ -168,7 +168,7 @@ >>> >>> int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, >>> uint64_t gstatus); >>> -int inject_vmce(struct domain *d); >>> +int inject_vmce(struct domain *d, bool_t vmce_broadcast); >>> >>> static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr) >>> { >>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce_intel.c >>> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c Thu Sep 20 00:03:25 2012 +0800 >>> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c Tue Sep 25 19:52:20 2012 +0800 >>> @@ -365,7 +365,7 @@ >>> } >>> >>> /* We will inject vMCE to DOMU*/ >>> - if ( inject_vmce(d) < 0 ) >>> + if ( inject_vmce(d, 1) < 0 ) >>> { >>> mce_printk(MCE_QUIET, "inject vMCE to DOM%d" >>> " failed\n", d->domain_id); >>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/vmce.c >>> --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 20 00:03:25 2012 +0800 >>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c Tue Sep 25 19:52:20 2012 +0800 >>> @@ -344,11 +344,14 @@ >>> HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, vmce_save_vcpu_ctxt, >>> vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU); >>> >>> -int inject_vmce(struct domain *d) >>> +/* >>> + * for Intel MCE, broadcast vMCE to all vcpus >>> + * for AMD MCE, only inject vMCE to vcpu0 >>> + */ >>> +int inject_vmce(struct domain *d, bool_t vmce_broadcast) >>> { >>> struct vcpu *v; >>> >>> - /* inject vMCE to all vcpus */ >>> for_each_vcpu(d, v) >>> { >>> if ( !test_and_set_bool(v->mce_pending) && >>> @@ -365,6 +368,9 @@ >>> d->domain_id, v->vcpu_id); >>> return -1; >>> } >>> + >>> + if ( !vmce_broadcast ) >>> + break; > > That''ll allow (non-broadcast) injection to vCPU 0 only - is that > really the right thing to do?On AMD side memory errors are found by the northbridge and the first cpu-core reports this. As long as we do not have NUMA support for the guest this is fine.> I.e. shouldn''t the caller rather be given flexibility to specify > which vCPU this is to go to (with a negative value meaning broadcast)?This is a good idea. Go for it.> And I''m intending to fold this into patch 2 anyway before > committing.Yes, please. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Christoph Egger wrote:> On 09/25/12 12:48, Jan Beulich wrote: > >>>>> On 25.09.12 at 11:06, Christoph Egger <Christoph.Egger@amd.com> >>>>> wrote: >>> On 09/25/12 07:00, Liu, Jinsong wrote: >>> >>>> X86/MCE: update vMCE injection for AMD >>>> >>>> For Intel MCE, it broadcasts vMCE to all vcpus. For AMD MCE, it >>>> injects vMCE only to vcpu0. This patch update inject_vmce for AMD. >>>> >>>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> >>>> Suggested_by: Christoph Egger <Christoph.Egger@amd.com> >>> >>> >>> Acked-by: Christoph Egger <Christoph.Egger@amd.com> >> >> Are you sure (see below)? > > Yes, see below. > >>>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce.h >>>> --- a/xen/arch/x86/cpu/mcheck/mce.h Thu Sep 20 00:03:25 2012 +0800 >>>> +++ b/xen/arch/x86/cpu/mcheck/mce.h Tue Sep 25 19:52:20 2012 +0800 >>>> @@ -168,7 +168,7 @@ >>>> >>>> int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, >>>> uint64_t gstatus); -int inject_vmce(struct domain *d); >>>> +int inject_vmce(struct domain *d, bool_t vmce_broadcast); >>>> >>>> static inline int mce_vendor_bank_msr(const struct vcpu *v, >>>> uint32_t msr) { diff -r a6d12a1bc758 >>>> xen/arch/x86/cpu/mcheck/mce_intel.c --- >>>> a/xen/arch/x86/cpu/mcheck/mce_intel.c Thu Sep 20 00:03:25 2012 >>>> +0800 +++ >>>> b/xen/arch/x86/cpu/mcheck/mce_intel.c Tue Sep 25 19:52:20 2012 >>>> +0800 @@ -365,7 +365,7 @@ } >>>> >>>> /* We will inject vMCE to DOMU*/ >>>> - if ( inject_vmce(d) < 0 ) >>>> + if ( inject_vmce(d, 1) < 0 ) >>>> { >>>> mce_printk(MCE_QUIET, "inject vMCE to DOM%d" >>>> " failed\n", d->domain_id); >>>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/vmce.c >>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 20 00:03:25 2012 +0800 >>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c Tue Sep 25 19:52:20 2012 >>>> +0800 @@ -344,11 +344,14 @@ HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, >>>> vmce_save_vcpu_ctxt, >>>> vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU); >>>> >>>> -int inject_vmce(struct domain *d) >>>> +/* >>>> + * for Intel MCE, broadcast vMCE to all vcpus >>>> + * for AMD MCE, only inject vMCE to vcpu0 >>>> + */ >>>> +int inject_vmce(struct domain *d, bool_t vmce_broadcast) { >>>> struct vcpu *v; >>>> >>>> - /* inject vMCE to all vcpus */ >>>> for_each_vcpu(d, v) >>>> { >>>> if ( !test_and_set_bool(v->mce_pending) && @@ -365,6 >>>> +368,9 @@ d->domain_id, v->vcpu_id); >>>> return -1; >>>> } >>>> + >>>> + if ( !vmce_broadcast ) >>>> + break; >> >> That''ll allow (non-broadcast) injection to vCPU 0 only - is that >> really the right thing to do? > > > On AMD side memory errors are found by the northbridge and the first > cpu-core reports this. As long as we do not have NUMA support for the > guest this is fine. > >> I.e. shouldn''t the caller rather be given flexibility to specify >> which vCPU this is to go to (with a negative value meaning >> broadcast)? > > This is a good idea. Go for it. > > >> And I''m intending to fold this into patch 2 anyway before >> committing. > > > Yes, please. > > ChristophI fold patch 6 to patch 2, will send out later. Thanks, Jinsong
>>> On 26.09.12 at 05:11, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: > I fold patch 6 to patch 2, will send out later.As I had indicated, I had done this already in preparation of things getting committed, so you will want to check once in staging (hopefully later today). Jan
Jan Beulich wrote:>>>> On 26.09.12 at 05:11, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: >> I fold patch 6 to patch 2, will send out later. > > As I had indicated, I had done this already in preparation of things > getting committed, so you will want to check once in staging > (hopefully later today). > > JanGot it, thanks! It''s OK to me, but how about add sanity check and some comments like =================Xen/MCE: add sanity check and comments for vmce injection Add sanity check for input vcpu so that malicious value would not return 0. Add comments since vcpu<0 (broadcast) is some implicit to code reader. Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.c --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 19:52:13 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 20:56:52 2012 +0800 @@ -341,10 +341,16 @@ /* * for Intel MCE, broadcast vMCE to all vcpus * for AMD MCE, only inject vMCE to vcpu0 + * + * @ d, domain to which would inject vmce + * @ vcpu, + * < 0, broadcast vMCE to all vcpus + * >= 0, vcpu who would be injected vMCE */ int inject_vmce(struct domain *d, int vcpu) { struct vcpu *v; + int ret = -EINVAL; for_each_vcpu ( d, v ) { @@ -358,19 +364,21 @@ mce_printk(MCE_VERBOSE, "MCE: inject vMCE to d%d:v%d\n", d->domain_id, v->vcpu_id); vcpu_kick(v); + ret = 0; } else { mce_printk(MCE_QUIET, "Failed to inject vMCE to d%d:v%d\n", d->domain_id, v->vcpu_id); - return -EBUSY; + ret = -EBUSY; + break; } if ( vcpu >= 0 ) - return 0; + break; } - return v ? -ESRCH : 0; + return ret; } int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Christoph Egger
2012-Sep-27 08:54 UTC
Re: [PATCH 6] X86/MCE: update vMCE injection for AMD
On 09/27/12 07:22, Liu, Jinsong wrote:> Jan Beulich wrote: >>>>> On 26.09.12 at 05:11, "Liu, Jinsong" <jinsong.liu@intel.com> wrote: >>> I fold patch 6 to patch 2, will send out later. >> >> As I had indicated, I had done this already in preparation of things >> getting committed, so you will want to check once in staging >> (hopefully later today). >> >> Jan > > Got it, thanks! It''s OK to me, but how about add sanity check and some comments like > > =================> Xen/MCE: add sanity check and comments for vmce injection > > Add sanity check for input vcpu so that malicious value would not return 0. > Add comments since vcpu<0 (broadcast) is some implicit to code reader.Yeah, a like #define VMCE_INJECT_BROADCAST is also helpful.> > Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> > > diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.c > --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 19:52:13 2012 +0800 > +++ b/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 20:56:52 2012 +0800 > @@ -341,10 +341,16 @@ > /* > * for Intel MCE, broadcast vMCE to all vcpus > * for AMD MCE, only inject vMCE to vcpu0 > + * > + * @ d, domain to which would inject vmce > + * @ vcpu, > + * < 0, broadcast vMCE to all vcpus > + * >= 0, vcpu who would be injected vMCEBetter wording: >= 0, vcpu, the vMCE is injected to Christoph> */ > int inject_vmce(struct domain *d, int vcpu) > { > struct vcpu *v; > + int ret = -EINVAL; > > for_each_vcpu ( d, v ) > { > @@ -358,19 +364,21 @@ > mce_printk(MCE_VERBOSE, "MCE: inject vMCE to d%d:v%d\n", > d->domain_id, v->vcpu_id); > vcpu_kick(v); > + ret = 0; > } > else > { > mce_printk(MCE_QUIET, "Failed to inject vMCE to d%d:v%d\n", > d->domain_id, v->vcpu_id); > - return -EBUSY; > + ret = -EBUSY; > + break; > } > > if ( vcpu >= 0 ) > - return 0; > + break; > } > > - return v ? -ESRCH : 0; > + return ret; > } > > int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
>> >> Got it, thanks! It''s OK to me, but how about add sanity check and >> some comments like >> >> =================>> Xen/MCE: add sanity check and comments for vmce injection >> >> Add sanity check for input vcpu so that malicious value would not >> return 0. >> Add comments since vcpu<0 (broadcast) is some implicit to code >> reader. > > > Yeah, a like #define VMCE_INJECT_BROADCAST is also helpful. > >> >> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> >> >> diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.c >> --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 19:52:13 2012 +0800 >> +++ b/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 20:56:52 2012 +0800 >> @@ -341,10 +341,16 @@ /* >> * for Intel MCE, broadcast vMCE to all vcpus >> * for AMD MCE, only inject vMCE to vcpu0 >> + * >> + * @ d, domain to which would inject vmce >> + * @ vcpu, >> + * < 0, broadcast vMCE to all vcpus >> + * >= 0, vcpu who would be injected vMCE > > > Better wording: > >= 0, vcpu, the vMCE is injected to > > Christoph >Fine to me, updated accordingly. Thanks, Jinsong ==============Xen/MCE: add sanity check and comments for vmce injection Add sanity check for input vcpu so that malicious value would not return 0. Add comments since vcpu=-1 (broadcast) is some implicit to code reader. Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> Suggested_by: Christoph Egger <Christoph.Egger@amd.com> diff -r adc7f057011e xen/arch/x86/cpu/mcheck/mce_intel.c --- a/xen/arch/x86/cpu/mcheck/mce_intel.c Thu Sep 27 19:52:13 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c Fri Sep 28 01:25:19 2012 +0800 @@ -360,7 +360,7 @@ } /* We will inject vMCE to DOMU*/ - if ( inject_vmce(d, -1) < 0 ) + if ( inject_vmce(d, VMCE_INJECT_BROADCAST) < 0 ) { mce_printk(MCE_QUIET, "inject vMCE to DOM%d" " failed\n", d->domain_id); diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.c --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 19:52:13 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/vmce.c Fri Sep 28 01:25:19 2012 +0800 @@ -341,14 +341,20 @@ /* * for Intel MCE, broadcast vMCE to all vcpus * for AMD MCE, only inject vMCE to vcpu0 + * + * @ d, domain to which would inject vmce + * @ vcpu, + * -1 (VMCE_INJECT_BROADCAST), broadcast vMCE to all vcpus + * >= 0, vcpu, the vMCE is injected to */ int inject_vmce(struct domain *d, int vcpu) { struct vcpu *v; + int ret = -EINVAL; for_each_vcpu ( d, v ) { - if ( vcpu >= 0 && v->vcpu_id != vcpu ) + if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id ) continue; if ( (is_hvm_domain(d) || @@ -358,19 +364,21 @@ mce_printk(MCE_VERBOSE, "MCE: inject vMCE to d%d:v%d\n", d->domain_id, v->vcpu_id); vcpu_kick(v); + ret = 0; } else { mce_printk(MCE_QUIET, "Failed to inject vMCE to d%d:v%d\n", d->domain_id, v->vcpu_id); - return -EBUSY; + ret = -EBUSY; + break; } if ( vcpu >= 0 ) - return 0; + break; } - return v ? -ESRCH : 0; + return ret; } int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.h --- a/xen/arch/x86/cpu/mcheck/vmce.h Thu Sep 27 19:52:13 2012 +0800 +++ b/xen/arch/x86/cpu/mcheck/vmce.h Fri Sep 28 01:25:19 2012 +0800 @@ -18,6 +18,8 @@ int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, uint64_t gstatus); + +#define VMCE_INJECT_BROADCAST -1 int inject_vmce(struct domain *d, int vcpu); #endif _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Christoph Egger
2012-Sep-27 09:56 UTC
Re: [PATCH 6] X86/MCE: update vMCE injection for AMD
On 09/27/12 11:45, Liu, Jinsong wrote:>>> >>> Got it, thanks! It''s OK to me, but how about add sanity check and >>> some comments like >>> >>> =================>>> Xen/MCE: add sanity check and comments for vmce injection >>> >>> Add sanity check for input vcpu so that malicious value would not >>> return 0. >>> Add comments since vcpu<0 (broadcast) is some implicit to code >>> reader. >> >> >> Yeah, a like #define VMCE_INJECT_BROADCAST is also helpful. >> >>> >>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> >>> >>> diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.c >>> --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 19:52:13 2012 +0800 >>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 20:56:52 2012 +0800 >>> @@ -341,10 +341,16 @@ /* >>> * for Intel MCE, broadcast vMCE to all vcpus >>> * for AMD MCE, only inject vMCE to vcpu0 >>> + * >>> + * @ d, domain to which would inject vmce >>> + * @ vcpu, >>> + * < 0, broadcast vMCE to all vcpus >>> + * >= 0, vcpu who would be injected vMCE >> >> >> Better wording: >> >= 0, vcpu, the vMCE is injected to >> >> Christoph >> > > Fine to me, updated accordingly. > > Thanks, > Jinsong > > ==============> Xen/MCE: add sanity check and comments for vmce injection > > Add sanity check for input vcpu so that malicious value would not return 0. > Add comments since vcpu=-1 (broadcast) is some implicit to code reader. > > Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com> > Suggested_by: Christoph Egger <Christoph.Egger@amd.com>Acked-by: Christoph Egger <Christoph.Egger@amd.com>> > diff -r adc7f057011e xen/arch/x86/cpu/mcheck/mce_intel.c > --- a/xen/arch/x86/cpu/mcheck/mce_intel.c Thu Sep 27 19:52:13 2012 +0800 > +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c Fri Sep 28 01:25:19 2012 +0800 > @@ -360,7 +360,7 @@ > } > > /* We will inject vMCE to DOMU*/ > - if ( inject_vmce(d, -1) < 0 ) > + if ( inject_vmce(d, VMCE_INJECT_BROADCAST) < 0 ) > { > mce_printk(MCE_QUIET, "inject vMCE to DOM%d" > " failed\n", d->domain_id); > diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.c > --- a/xen/arch/x86/cpu/mcheck/vmce.c Thu Sep 27 19:52:13 2012 +0800 > +++ b/xen/arch/x86/cpu/mcheck/vmce.c Fri Sep 28 01:25:19 2012 +0800 > @@ -341,14 +341,20 @@ > /* > * for Intel MCE, broadcast vMCE to all vcpus > * for AMD MCE, only inject vMCE to vcpu0 > + * > + * @ d, domain to which would inject vmce > + * @ vcpu, > + * -1 (VMCE_INJECT_BROADCAST), broadcast vMCE to all vcpus > + * >= 0, vcpu, the vMCE is injected to > */ > int inject_vmce(struct domain *d, int vcpu) > { > struct vcpu *v; > + int ret = -EINVAL; > > for_each_vcpu ( d, v ) > { > - if ( vcpu >= 0 && v->vcpu_id != vcpu ) > + if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id ) > continue; > > if ( (is_hvm_domain(d) || > @@ -358,19 +364,21 @@ > mce_printk(MCE_VERBOSE, "MCE: inject vMCE to d%d:v%d\n", > d->domain_id, v->vcpu_id); > vcpu_kick(v); > + ret = 0; > } > else > { > mce_printk(MCE_QUIET, "Failed to inject vMCE to d%d:v%d\n", > d->domain_id, v->vcpu_id); > - return -EBUSY; > + ret = -EBUSY; > + break; > } > > if ( vcpu >= 0 ) > - return 0; > + break; > } > > - return v ? -ESRCH : 0; > + return ret; > } > > int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, > diff -r adc7f057011e xen/arch/x86/cpu/mcheck/vmce.h > --- a/xen/arch/x86/cpu/mcheck/vmce.h Thu Sep 27 19:52:13 2012 +0800 > +++ b/xen/arch/x86/cpu/mcheck/vmce.h Fri Sep 28 01:25:19 2012 +0800 > @@ -18,6 +18,8 @@ > > int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d, > uint64_t gstatus); > + > +#define VMCE_INJECT_BROADCAST -1 > int inject_vmce(struct domain *d, int vcpu); > > #endif-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632