From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Egger Subject: Re: [PATCH 6] X86/MCE: update vMCE injection for AMD Date: Tue, 25 Sep 2012 13:54:09 +0200 Message-ID: <50619B61.2030609@amd.com> References: <506173FB.8070603@amd.com> <5061A833020000780009DA1A@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5061A833020000780009DA1A@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Jinsong Liu , "xen-devel@lists.xensource.com" , "keir@xen.org" , "Ian.Campbell@citrix.com" List-Id: xen-devel@lists.xenproject.org On 09/25/12 12:48, Jan Beulich wrote: >>>> On 25.09.12 at 11:06, Christoph Egger 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 >>> Suggested_by: Christoph Egger >> >> >> Acked-by: Christoph Egger > > 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