Jiang, Yunhong
2010-Feb-01 08:48 UTC
[Xen-devel] [PATCH] Don''t enable irq for machine check vmexit
We should not enable irq for machine check VMExit In changeset 18658:824892134573, IRQ is enabled during VMExit except external interrupt. The exception should apply for machine check also, because : a) The mce_logout_lock should be held in irq_disabled context. b) The machine check event should be handled as quickly as possible, enable irq will increase the period greatly. Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> This is in hotspot code path, I try to use unlikely, hope to reduce the performance impact Thanks Yunhong Jiang _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-01 09:18 UTC
[Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
Sorry forgot the patch.
--jyh
diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
@@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
printk("caused by machine check.\n");
HVMTRACE_0D(MCE);
do_machine_check(regs);
+ local_irq_enable();
break;
default:
printk("reason not known yet!");
@@ -2243,6 +2244,23 @@ err:
err:
vmx_inject_hw_exception(TRAP_gp_fault, 0);
return -1;
+}
+
+int vmx_mce_exit(int exit_reason)
+{
+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
+ return 1;
+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
+ {
+ uint32_t vector;
+
+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
+ if (vector == TRAP_machine_check)
+ return 1;
+ }
+
+ return 0;
}
asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
@@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
vmx_do_extint(regs);
/* Now enable interrupts so it''s safe to take locks. */
- local_irq_enable();
+ if ( !(vmx_mce_exit(exit_reason)) )
+ local_irq_enable();
if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
return vmx_failed_vmentry(exit_reason, regs);
@@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
case TRAP_machine_check:
HVMTRACE_0D(MCE);
do_machine_check(regs);
+ local_irq_enable();
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
>-----Original Message-----
>From: Jiang, Yunhong
>Sent: Monday, February 01, 2010 4:48 PM
>To: Keir Fraser; Tim.Deegan@citrix.com
>Cc: xen-devel@lists.xensource.com
>Subject: [PATCH] Don''t enable irq for machine check vmexit
>
>We should not enable irq for machine check VMExit
>
>In changeset 18658:824892134573, IRQ is enabled during VMExit except
external
>interrupt. The exception should apply for machine check also, because :
>a) The mce_logout_lock should be held in irq_disabled context.
>b) The machine check event should be handled as quickly as possible, enable
irq will
>increase the period greatly.
>
>Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>
>This is in hotspot code path, I try to use unlikely, hope to reduce the
performance
>impact
>
>Thanks
>Yunhong Jiang
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-01 11:14 UTC
[Xen-devel] Re: [PATCH] Don''t enable irq for machine check vmexit
Take a look at changeset 18658: local_irq_enable() was made unconditional in the vmexit handler function for a good reason! If you make it conditional on !mce, then in the mce case you can crash in any later function that acquires a spinlock: maybe_deassert_evtchn_irq(), for example. You will crash because of taking a irq-unsafe spinlock with irqs disabled. Kind of the opposite way round to the mce_logout_lock: so your patch would fix acquisition of that lock but break others. I''m afraid your only option is to hoist all the mce handling up to the same place we handle extints. Take c/s 18658 as your template for what to do. -- Keir On 01/02/2010 09:18, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:> Sorry forgot the patch. > > --jyh > > diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c > --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000 > +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800 > @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned > printk("caused by machine check.\n"); > HVMTRACE_0D(MCE); > do_machine_check(regs); > + local_irq_enable(); > break; > default: > printk("reason not known yet!"); > @@ -2243,6 +2244,23 @@ err: > err: > vmx_inject_hw_exception(TRAP_gp_fault, 0); > return -1; > +} > + > +int vmx_mce_exit(int exit_reason) > +{ > + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && > + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) > + return 1; > + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) > + { > + uint32_t vector; > + > + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK; > + if (vector == TRAP_machine_check) > + return 1; > + } > + > + return 0; > } > > asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) > @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc > vmx_do_extint(regs); > > /* Now enable interrupts so it''s safe to take locks. */ > - local_irq_enable(); > + if ( !(vmx_mce_exit(exit_reason)) ) > + local_irq_enable(); > > if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) ) > return vmx_failed_vmentry(exit_reason, regs); > @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc > case TRAP_machine_check: > HVMTRACE_0D(MCE); > do_machine_check(regs); > + local_irq_enable(); > break; > case TRAP_invalid_op: > vmx_vmexit_ud_intercept(regs); > > > >> -----Original Message----- >> From: Jiang, Yunhong >> Sent: Monday, February 01, 2010 4:48 PM >> To: Keir Fraser; Tim.Deegan@citrix.com >> Cc: xen-devel@lists.xensource.com >> Subject: [PATCH] Don''t enable irq for machine check vmexit >> >> We should not enable irq for machine check VMExit >> >> In changeset 18658:824892134573, IRQ is enabled during VMExit except external >> interrupt. The exception should apply for machine check also, because : >> a) The mce_logout_lock should be held in irq_disabled context. >> b) The machine check event should be handled as quickly as possible, enable >> irq will >> increase the period greatly. >> >> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >> >> This is in hotspot code path, I try to use unlikely, hope to reduce the >> performance >> impact >> >> Thanks >> Yunhong Jiang >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-04 09:26 UTC
[Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
Keir, any comments to this patch? Thanks Yunhong Jiang>-----Original Message----- >From: Jiang, Yunhong >Sent: Monday, February 01, 2010 5:18 PM >To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com >Cc: xen-devel@lists.xensource.com >Subject: RE: [PATCH] Don''t enable irq for machine check vmexit > >Sorry forgot the patch. > >--jyh > >diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c >--- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000 >+++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800 >@@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned > printk("caused by machine check.\n"); > HVMTRACE_0D(MCE); > do_machine_check(regs); >+ local_irq_enable(); > break; > default: > printk("reason not known yet!"); >@@ -2243,6 +2244,23 @@ err: > err: > vmx_inject_hw_exception(TRAP_gp_fault, 0); > return -1; >+} >+ >+int vmx_mce_exit(int exit_reason) >+{ >+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && >+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) >+ return 1; >+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) >+ { >+ uint32_t vector; >+ >+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK; >+ if (vector == TRAP_machine_check) >+ return 1; >+ } >+ >+ return 0; > } > > asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) >@@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc > vmx_do_extint(regs); > > /* Now enable interrupts so it''s safe to take locks. */ >- local_irq_enable(); >+ if ( !(vmx_mce_exit(exit_reason)) ) >+ local_irq_enable(); > > if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) ) > return vmx_failed_vmentry(exit_reason, regs); >@@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc > case TRAP_machine_check: > HVMTRACE_0D(MCE); > do_machine_check(regs); >+ local_irq_enable(); > break; > case TRAP_invalid_op: > vmx_vmexit_ud_intercept(regs); > > > >>-----Original Message----- >>From: Jiang, Yunhong >>Sent: Monday, February 01, 2010 4:48 PM >>To: Keir Fraser; Tim.Deegan@citrix.com >>Cc: xen-devel@lists.xensource.com >>Subject: [PATCH] Don''t enable irq for machine check vmexit >> >>We should not enable irq for machine check VMExit >> >>In changeset 18658:824892134573, IRQ is enabled during VMExit except external >>interrupt. The exception should apply for machine check also, because : >>a) The mce_logout_lock should be held in irq_disabled context. >>b) The machine check event should be handled as quickly as possible, enable irq will >>increase the period greatly. >> >>Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >> >>This is in hotspot code path, I try to use unlikely, hope to reduce the performance >>impact >> >>Thanks >>Yunhong Jiang_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-04 10:03 UTC
[Xen-devel] Re: [PATCH] Don''t enable irq for machine check vmexit
Yeah, I already replied to you on Monday. What I said was: """ Take a look at changeset 18658: local_irq_enable() was made unconditional in the vmexit handler function for a good reason! If you make it conditional on !mce, then in the mce case you can crash in any later function that acquires a spinlock: maybe_deassert_evtchn_irq(), for example. You will crash because of taking a irq-unsafe spinlock with irqs disabled. Kind of the opposite way round to the mce_logout_lock: so your patch would fix acquisition of that lock but break others. I''m afraid your only option is to hoist all the mce handling up to the same place we handle extints. Take c/s 18658 as your template for what to do. """ -- Keir On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:> Keir, any comments to this patch? > > Thanks > Yunhong Jiang > >> -----Original Message----- >> From: Jiang, Yunhong >> Sent: Monday, February 01, 2010 5:18 PM >> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com >> Cc: xen-devel@lists.xensource.com >> Subject: RE: [PATCH] Don''t enable irq for machine check vmexit >> >> Sorry forgot the patch. >> >> --jyh >> >> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c >> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000 >> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800 >> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned >> printk("caused by machine check.\n"); >> HVMTRACE_0D(MCE); >> do_machine_check(regs); >> + local_irq_enable(); >> break; >> default: >> printk("reason not known yet!"); >> @@ -2243,6 +2244,23 @@ err: >> err: >> vmx_inject_hw_exception(TRAP_gp_fault, 0); >> return -1; >> +} >> + >> +int vmx_mce_exit(int exit_reason) >> +{ >> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && >> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) >> + return 1; >> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) >> + { >> + uint32_t vector; >> + >> + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK; >> + if (vector == TRAP_machine_check) >> + return 1; >> + } >> + >> + return 0; >> } >> >> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) >> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc >> vmx_do_extint(regs); >> >> /* Now enable interrupts so it''s safe to take locks. */ >> - local_irq_enable(); >> + if ( !(vmx_mce_exit(exit_reason)) ) >> + local_irq_enable(); >> >> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) ) >> return vmx_failed_vmentry(exit_reason, regs); >> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc >> case TRAP_machine_check: >> HVMTRACE_0D(MCE); >> do_machine_check(regs); >> + local_irq_enable(); >> break; >> case TRAP_invalid_op: >> vmx_vmexit_ud_intercept(regs); >> >> >> >>> -----Original Message----- >>> From: Jiang, Yunhong >>> Sent: Monday, February 01, 2010 4:48 PM >>> To: Keir Fraser; Tim.Deegan@citrix.com >>> Cc: xen-devel@lists.xensource.com >>> Subject: [PATCH] Don''t enable irq for machine check vmexit >>> >>> We should not enable irq for machine check VMExit >>> >>> In changeset 18658:824892134573, IRQ is enabled during VMExit except >>> external >>> interrupt. The exception should apply for machine check also, because : >>> a) The mce_logout_lock should be held in irq_disabled context. >>> b) The machine check event should be handled as quickly as possible, enable >>> irq will >>> increase the period greatly. >>> >>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >>> >>> This is in hotspot code path, I try to use unlikely, hope to reduce the >>> performance >>> impact >>> >>> Thanks >>> Yunhong Jiang >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-04 12:25 UTC
[Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
>-----Original Message----- >From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] >Sent: Thursday, February 04, 2010 6:04 PM >To: Jiang, Yunhong; Tim Deegan >Cc: xen-devel@lists.xensource.com >Subject: Re: [PATCH] Don''t enable irq for machine check vmexit > >Yeah, I already replied to you on Monday. What I said was:Sorry, seems I didn''t got your original replay.> >""" >Take a look at changeset 18658: local_irq_enable() was made unconditional in >the vmexit handler function for a good reason! > >If you make it conditional on !mce, then in the mce case you can crash in >any later function that acquires a spinlock: maybe_deassert_evtchn_irq(), >for example. You will crash because of taking a irq-unsafe spinlock with >irqs disabled. Kind of the opposite way round to the mce_logout_lock: so >your patch would fix acquisition of that lock but break others.In mce case, we can''t take any lock that wil be shared with non-mce context at all, because MCE can happen in any context. The mce_logout_lock is the only spin_lock AFAIK that is used in mce handler, and that lock is stated as " mce_logout_lock should only be used in the trap handler". That should apply to any spin_lock that will be used in MCE handler in future. Yes, the mcheck_cmn_handler() takes vcpu_schedule_lock_irq(v), but that common handler is now only used in AMD platform, and I assume will be replaced after xen 4.0. For the functoin that be called after the machine check handler, I enable the interrupt like following code, which is same as current logic, and I think that make sense. @@ -2449,6 +2468,7 @@ asmlinkage void vmx_vmexit_handler(struc case TRAP_machine_check: HVMTRACE_0D(MCE); do_machine_check(regs); + local_irq_enable(); break; case TRAP_invalid_op: vmx_vmexit_ud_intercept(regs);> >I''m afraid your only option is to hoist all the mce handling up to the same >place we handle extints. Take c/s 18658 as your template for what to do.Maybe I missed anything, but I didn''t find the difference with the external interrupt case. Because I can''t distinguish if a VMExit caused by MCE simply through a exit_reason, I wrap it through vmx_mce_exit().>""" > > -- Keir > >On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > >> Keir, any comments to this patch? >> >> Thanks >> Yunhong Jiang >> >>> -----Original Message----- >>> From: Jiang, Yunhong >>> Sent: Monday, February 01, 2010 5:18 PM >>> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com >>> Cc: xen-devel@lists.xensource.com >>> Subject: RE: [PATCH] Don''t enable irq for machine check vmexit >>> >>> Sorry forgot the patch. >>> >>> --jyh >>> >>> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c >>> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000 >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800 >>> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned >>> printk("caused by machine check.\n"); >>> HVMTRACE_0D(MCE); >>> do_machine_check(regs); >>> + local_irq_enable(); >>> break; >>> default: >>> printk("reason not known yet!"); >>> @@ -2243,6 +2244,23 @@ err: >>> err: >>> vmx_inject_hw_exception(TRAP_gp_fault, 0); >>> return -1; >>> +} >>> + >>> +int vmx_mce_exit(int exit_reason) >>> +{ >>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && >>> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) >>> + return 1; >>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) >>> + { >>> + uint32_t vector; >>> + >>> + vector = __vmread(VM_EXIT_INTR_INFO) & >INTR_INFO_VECTOR_MASK; >>> + if (vector == TRAP_machine_check) >>> + return 1; >>> + } >>> + >>> + return 0; >>> } >>> >>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) >>> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc >>> vmx_do_extint(regs); >>> >>> /* Now enable interrupts so it''s safe to take locks. */ >>> - local_irq_enable(); >>> + if ( !(vmx_mce_exit(exit_reason)) ) >>> + local_irq_enable(); >>> >>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) ) >>> return vmx_failed_vmentry(exit_reason, regs); >>> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc >>> case TRAP_machine_check: >>> HVMTRACE_0D(MCE); >>> do_machine_check(regs); >>> + local_irq_enable(); >>> break; >>> case TRAP_invalid_op: >>> vmx_vmexit_ud_intercept(regs); >>> >>> >>> >>>> -----Original Message----- >>>> From: Jiang, Yunhong >>>> Sent: Monday, February 01, 2010 4:48 PM >>>> To: Keir Fraser; Tim.Deegan@citrix.com >>>> Cc: xen-devel@lists.xensource.com >>>> Subject: [PATCH] Don''t enable irq for machine check vmexit >>>> >>>> We should not enable irq for machine check VMExit >>>> >>>> In changeset 18658:824892134573, IRQ is enabled during VMExit except >>>> external >>>> interrupt. The exception should apply for machine check also, because : >>>> a) The mce_logout_lock should be held in irq_disabled context. >>>> b) The machine check event should be handled as quickly as possible, enable >>>> irq will >>>> increase the period greatly. >>>> >>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >>>> >>>> This is in hotspot code path, I try to use unlikely, hope to reduce the >>>> performance >>>> impact >>>> >>>> Thanks >>>> Yunhong Jiang >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Feb-04 14:21 UTC
[Xen-devel] Re: [PATCH] Don''t enable irq for machine check vmexit
At 12:25 +0000 on 04 Feb (1265286343), Jiang, Yunhong wrote:> For the functoin that be called after the machine check handler, I > enable the interrupt like following code, which is same as current > logic, and I think that make sense.You can''t leave interrupts disabled in the main body of the VMEXIT handler. It''s fragile, and more importantly it reintroduces the bug that 18658 fixed: you end up with spinlocks that are taken both with interrupts enabled and with interrupts disabled, leading to a deadlock. If you need to run the MCE handler before interrupts are re-enabled, then it must be moved to the top of the vmexit handler, right beside where EXTINTs are hadled. Cheers, Tim.> @@ -2449,6 +2468,7 @@ asmlinkage void vmx_vmexit_handler(struc > case TRAP_machine_check: > HVMTRACE_0D(MCE); > do_machine_check(regs); > + local_irq_enable(); > break; > case TRAP_invalid_op: > vmx_vmexit_ud_intercept(regs); > > > > >I''m afraid your only option is to hoist all the mce handling up to the same > >place we handle extints. Take c/s 18658 as your template for what to do. > > Maybe I missed anything, but I didn''t find the difference with the external interrupt case. Because I can''t distinguish if a VMExit caused by MCE simply through a exit_reason, I wrap it through vmx_mce_exit(). > >""" > > > > -- Keir > > > >On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > > > >> Keir, any comments to this patch? > >> > >> Thanks > >> Yunhong Jiang > >> > >>> -----Original Message----- > >>> From: Jiang, Yunhong > >>> Sent: Monday, February 01, 2010 5:18 PM > >>> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com > >>> Cc: xen-devel@lists.xensource.com > >>> Subject: RE: [PATCH] Don''t enable irq for machine check vmexit > >>> > >>> Sorry forgot the patch. > >>> > >>> --jyh > >>> > >>> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c > >>> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000 > >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800 > >>> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned > >>> printk("caused by machine check.\n"); > >>> HVMTRACE_0D(MCE); > >>> do_machine_check(regs); > >>> + local_irq_enable(); > >>> break; > >>> default: > >>> printk("reason not known yet!"); > >>> @@ -2243,6 +2244,23 @@ err: > >>> err: > >>> vmx_inject_hw_exception(TRAP_gp_fault, 0); > >>> return -1; > >>> +} > >>> + > >>> +int vmx_mce_exit(int exit_reason) > >>> +{ > >>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && > >>> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) > >>> + return 1; > >>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) > >>> + { > >>> + uint32_t vector; > >>> + > >>> + vector = __vmread(VM_EXIT_INTR_INFO) & > >INTR_INFO_VECTOR_MASK; > >>> + if (vector == TRAP_machine_check) > >>> + return 1; > >>> + } > >>> + > >>> + return 0; > >>> } > >>> > >>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) > >>> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc > >>> vmx_do_extint(regs); > >>> > >>> /* Now enable interrupts so it''s safe to take locks. */ > >>> - local_irq_enable(); > >>> + if ( !(vmx_mce_exit(exit_reason)) ) > >>> + local_irq_enable(); > >>> > >>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) ) > >>> return vmx_failed_vmentry(exit_reason, regs); > >>> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc > >>> case TRAP_machine_check: > >>> HVMTRACE_0D(MCE); > >>> do_machine_check(regs); > >>> + local_irq_enable(); > >>> break; > >>> case TRAP_invalid_op: > >>> vmx_vmexit_ud_intercept(regs); > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: Jiang, Yunhong > >>>> Sent: Monday, February 01, 2010 4:48 PM > >>>> To: Keir Fraser; Tim.Deegan@citrix.com > >>>> Cc: xen-devel@lists.xensource.com > >>>> Subject: [PATCH] Don''t enable irq for machine check vmexit > >>>> > >>>> We should not enable irq for machine check VMExit > >>>> > >>>> In changeset 18658:824892134573, IRQ is enabled during VMExit except > >>>> external > >>>> interrupt. The exception should apply for machine check also, because : > >>>> a) The mce_logout_lock should be held in irq_disabled context. > >>>> b) The machine check event should be handled as quickly as possible, enable > >>>> irq will > >>>> increase the period greatly. > >>>> > >>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> > >>>> > >>>> This is in hotspot code path, I try to use unlikely, hope to reduce the > >>>> performance > >>>> impact > >>>> > >>>> Thanks > >>>> Yunhong Jiang > >> > > >-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Citrix Systems (R&D) Ltd. [Company #02300071, SL9 0DZ, UK.] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-04 14:35 UTC
[Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
I got the issue. Thanks! I will re-do the patch tomorrow. Thanks Yunhong Jiang>-----Original Message----- >From: Tim Deegan [mailto:Tim.Deegan@citrix.com] >Sent: Thursday, February 04, 2010 10:22 PM >To: Jiang, Yunhong >Cc: Keir Fraser; xen-devel@lists.xensource.com >Subject: Re: [PATCH] Don''t enable irq for machine check vmexit > >At 12:25 +0000 on 04 Feb (1265286343), Jiang, Yunhong wrote: > >> For the functoin that be called after the machine check handler, I >> enable the interrupt like following code, which is same as current >> logic, and I think that make sense. > >You can''t leave interrupts disabled in the main body of the VMEXIT >handler. It''s fragile, and more importantly it reintroduces the bug >that 18658 fixed: you end up with spinlocks that are taken both with >interrupts enabled and with interrupts disabled, leading to a deadlock. > >If you need to run the MCE handler before interrupts are re-enabled, >then it must be moved to the top of the vmexit handler, right beside >where EXTINTs are hadled. > >Cheers, > >Tim. > >> @@ -2449,6 +2468,7 @@ asmlinkage void vmx_vmexit_handler(struc >> case TRAP_machine_check: >> HVMTRACE_0D(MCE); >> do_machine_check(regs); >> + local_irq_enable(); >> break; >> case TRAP_invalid_op: >> vmx_vmexit_ud_intercept(regs); >> >> > >> >I''m afraid your only option is to hoist all the mce handling up to the same >> >place we handle extints. Take c/s 18658 as your template for what to do. >> >> Maybe I missed anything, but I didn''t find the difference with the external interrupt >case. Because I can''t distinguish if a VMExit caused by MCE simply through a >exit_reason, I wrap it through vmx_mce_exit(). >> >""" >> > >> > -- Keir >> > >> >On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: >> > >> >> Keir, any comments to this patch? >> >> >> >> Thanks >> >> Yunhong Jiang >> >> >> >>> -----Original Message----- >> >>> From: Jiang, Yunhong >> >>> Sent: Monday, February 01, 2010 5:18 PM >> >>> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com >> >>> Cc: xen-devel@lists.xensource.com >> >>> Subject: RE: [PATCH] Don''t enable irq for machine check vmexit >> >>> >> >>> Sorry forgot the patch. >> >>> >> >>> --jyh >> >>> >> >>> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c >> >>> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000 >> >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800 >> >>> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned >> >>> printk("caused by machine check.\n"); >> >>> HVMTRACE_0D(MCE); >> >>> do_machine_check(regs); >> >>> + local_irq_enable(); >> >>> break; >> >>> default: >> >>> printk("reason not known yet!"); >> >>> @@ -2243,6 +2244,23 @@ err: >> >>> err: >> >>> vmx_inject_hw_exception(TRAP_gp_fault, 0); >> >>> return -1; >> >>> +} >> >>> + >> >>> +int vmx_mce_exit(int exit_reason) >> >>> +{ >> >>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && >> >>> + (uint16_t)exit_reason =>EXIT_REASON_MCE_DURING_VMENTRY) ) >> >>> + return 1; >> >>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) >> >>> + { >> >>> + uint32_t vector; >> >>> + >> >>> + vector = __vmread(VM_EXIT_INTR_INFO) & >> >INTR_INFO_VECTOR_MASK; >> >>> + if (vector == TRAP_machine_check) >> >>> + return 1; >> >>> + } >> >>> + >> >>> + return 0; >> >>> } >> >>> >> >>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) >> >>> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc >> >>> vmx_do_extint(regs); >> >>> >> >>> /* Now enable interrupts so it''s safe to take locks. */ >> >>> - local_irq_enable(); >> >>> + if ( !(vmx_mce_exit(exit_reason)) ) >> >>> + local_irq_enable(); >> >>> >> >>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) ) >> >>> return vmx_failed_vmentry(exit_reason, regs); >> >>> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc >> >>> case TRAP_machine_check: >> >>> HVMTRACE_0D(MCE); >> >>> do_machine_check(regs); >> >>> + local_irq_enable(); >> >>> break; >> >>> case TRAP_invalid_op: >> >>> vmx_vmexit_ud_intercept(regs); >> >>> >> >>> >> >>> >> >>>> -----Original Message----- >> >>>> From: Jiang, Yunhong >> >>>> Sent: Monday, February 01, 2010 4:48 PM >> >>>> To: Keir Fraser; Tim.Deegan@citrix.com >> >>>> Cc: xen-devel@lists.xensource.com >> >>>> Subject: [PATCH] Don''t enable irq for machine check vmexit >> >>>> >> >>>> We should not enable irq for machine check VMExit >> >>>> >> >>>> In changeset 18658:824892134573, IRQ is enabled during VMExit except >> >>>> external >> >>>> interrupt. The exception should apply for machine check also, because : >> >>>> a) The mce_logout_lock should be held in irq_disabled context. >> >>>> b) The machine check event should be handled as quickly as possible, enable >> >>>> irq will >> >>>> increase the period greatly. >> >>>> >> >>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >> >>>> >> >>>> This is in hotspot code path, I try to use unlikely, hope to reduce the >> >>>> performance >> >>>> impact >> >>>> >> >>>> Thanks >> >>>> Yunhong Jiang >> >> >> > >> > >-- >Tim Deegan <Tim.Deegan@citrix.com> >Principal Software Engineer, Citrix Systems (R&D) Ltd. >[Company #02300071, SL9 0DZ, UK.]_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-05 10:22 UTC
[Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
Keir/Tim, here is attached patch. Please have a look onit.
Thanks
Yunhong Jiang
# HG changeset patch
# User Yunhong Jiang <yunhong.jiang@intel.com>
# Date 1265363638 -28800
# Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
# Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
We should not enable irq for machine check VMExit
In changeset 18658:824892134573, IRQ is enabled during VMExit except external
interrupt. The exception should apply for machine check also, because :
a) The mce_logout_lock should be held in irq_disabled context.
b) The machine check event should be handled as quickly as possible, enable irq
will increase the period greatly.
Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
@@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
case EXIT_REASON_MCE_DURING_VMENTRY:
printk("caused by machine check.\n");
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Handled already */
break;
default:
printk("reason not known yet!");
@@ -2259,6 +2259,23 @@ err:
err:
vmx_inject_hw_exception(TRAP_gp_fault, 0);
return -1;
+}
+
+int vmx_mce_exit(int exit_reason)
+{
+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
+ return 1;
+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
+ {
+ uint32_t vector;
+
+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
+ if (vector == TRAP_machine_check)
+ return 1;
+ }
+
+ return 0;
}
asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
@@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
/* Handle the interrupt we missed before allowing any more in. */
if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
vmx_do_extint(regs);
+
+ if ( vmx_mce_exit(exit_reason) )
+ do_machine_check(regs);
/* Now enable interrupts so it''s safe to take locks. */
local_irq_enable();
@@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
self_nmi(); /* Real NMI, vector 2: normal processing. */
break;
case TRAP_machine_check:
+ dprintk(XENLOG_INFO, "VMexit for machine check\n");
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Handled already */
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Tim Deegan
2010-Feb-05 10:42 UTC
Re: [Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
At 10:22 +0000 on 05 Feb (1265365365), Jiang, Yunhong wrote:> Keir/Tim, here is attached patch. Please have a look onit.Yep, that looks better. Not sure that the dprintk is useful, though. Cheers, Tim.> Thanks > Yunhong Jiang > > # HG changeset patch > # User Yunhong Jiang <yunhong.jiang@intel.com> > # Date 1265363638 -28800 > # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187 > # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c > We should not enable irq for machine check VMExit > > In changeset 18658:824892134573, IRQ is enabled during VMExit except external interrupt. The exception should apply for machine check also, because : > a) The mce_logout_lock should be held in irq_disabled context. > b) The machine check event should be handled as quickly as possible, enable irq will increase the period greatly. > > Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> > > diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c > --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000 > +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800 > @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned > case EXIT_REASON_MCE_DURING_VMENTRY: > printk("caused by machine check.\n"); > HVMTRACE_0D(MCE); > - do_machine_check(regs); > + /* Handled already */ > break; > default: > printk("reason not known yet!"); > @@ -2259,6 +2259,23 @@ err: > err: > vmx_inject_hw_exception(TRAP_gp_fault, 0); > return -1; > +} > + > +int vmx_mce_exit(int exit_reason) > +{ > + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && > + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) > + return 1; > + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) > + { > + uint32_t vector; > + > + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK; > + if (vector == TRAP_machine_check) > + return 1; > + } > + > + return 0; > } > > asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) > @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc > /* Handle the interrupt we missed before allowing any more in. */ > if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT ) > vmx_do_extint(regs); > + > + if ( vmx_mce_exit(exit_reason) ) > + do_machine_check(regs); > > /* Now enable interrupts so it''s safe to take locks. */ > local_irq_enable(); > @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc > self_nmi(); /* Real NMI, vector 2: normal processing. */ > break; > case TRAP_machine_check: > + dprintk(XENLOG_INFO, "VMexit for machine check\n"); > HVMTRACE_0D(MCE); > - do_machine_check(regs); > + /* Handled already */ > break; > case TRAP_invalid_op: > vmx_vmexit_ud_intercept(regs); > >Content-Description: ATT00001.txt> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Citrix Systems (R&D) Ltd. [Company #02300071, SL9 0DZ, UK.] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-05 12:53 UTC
[Xen-devel] Re: [PATCH] Don''t enable irq for machine check vmexit
How about the attached alternative, which avoids repeated reads of VM_INTR_INFO? Also I''m not sure whether checking for VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all, EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway. Perhaps we should mask exit_reason down to 16 bits right at the top of the vmexit handler? That would save us if new flags are added to exit_reason[30:16], also. Anyway, let me know what you think. -- Keir On 05/02/2010 10:22, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:> Keir/Tim, here is attached patch. Please have a look onit. > > Thanks > Yunhong Jiang > > # HG changeset patch > # User Yunhong Jiang <yunhong.jiang@intel.com> > # Date 1265363638 -28800 > # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187 > # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c > We should not enable irq for machine check VMExit > > In changeset 18658:824892134573, IRQ is enabled during VMExit except external > interrupt. The exception should apply for machine check also, because : > a) The mce_logout_lock should be held in irq_disabled context. > b) The machine check event should be handled as quickly as possible, enable > irq will increase the period greatly. > > Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> > > diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c > --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000 > +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800 > @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned > case EXIT_REASON_MCE_DURING_VMENTRY: > printk("caused by machine check.\n"); > HVMTRACE_0D(MCE); > - do_machine_check(regs); > + /* Handled already */ > break; > default: > printk("reason not known yet!"); > @@ -2259,6 +2259,23 @@ err: > err: > vmx_inject_hw_exception(TRAP_gp_fault, 0); > return -1; > +} > + > +int vmx_mce_exit(int exit_reason) > +{ > + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && > + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) > + return 1; > + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) > + { > + uint32_t vector; > + > + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK; > + if (vector == TRAP_machine_check) > + return 1; > + } > + > + return 0; > } > > asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) > @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc > /* Handle the interrupt we missed before allowing any more in. */ > if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT ) > vmx_do_extint(regs); > + > + if ( vmx_mce_exit(exit_reason) ) > + do_machine_check(regs); > > /* Now enable interrupts so it''s safe to take locks. */ > local_irq_enable(); > @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc > self_nmi(); /* Real NMI, vector 2: normal processing. */ > break; > case TRAP_machine_check: > + dprintk(XENLOG_INFO, "VMexit for machine check\n"); > HVMTRACE_0D(MCE); > - do_machine_check(regs); > + /* Handled already */ > break; > case TRAP_invalid_op: > vmx_vmexit_ud_intercept(regs); > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-05 14:36 UTC
[Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
>-----Original Message----- >From: xen-devel-bounces@lists.xensource.com >[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser >Sent: Friday, February 05, 2010 8:54 PM >To: Jiang, Yunhong; Tim Deegan >Cc: xen-devel@lists.xensource.com >Subject: [Xen-devel] Re: [PATCH] Don''t enable irq for machine check vmexit > >How about the attached alternative, which avoids repeated reads of >VM_INTR_INFO? Also I''m not sure whether checking for >VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all, >EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway.Thanks for your patch. Yes, it is much better to avoid the repeated read. I''m not sure if it is ok if we don''t check the VM_EXIT_REASONS_FAILED_VMENTRY. Checking the SDM and seems it is ok. In fact, I didn''t find effective method to test this VMEntry MCE failed case, althgouh I can test MCE VMExit with EXIT_REASON_EXCEPTION_NMI case easily. (I will try to find a method to test this VMEntry failure case next week, maybe poison the VMCS range can trigger it, I''m not sure). Or maybe code like following? It will increase a "&" operation for each exit, but that should make us safe. - if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT ) + switch ( exit_reason & (VMX_EXIT_REASONS_FAILED_VMENTRY | 0xFFFF)) + { + case EXIT_REASON_EXTERNAL_INTERRUPT: vmx_do_extint(regs); + break; .... + case EXIT_REASON_MCE_DURING_VMENTRY| VMX_EXIT_REASONS_FAILED_VMENTRY: ---> (or define a new macro for it) + do_machine_check(regs); + break> >Perhaps we should mask exit_reason down to 16 bits right at the top of the >vmexit handler? That would save us if new flags are added to >exit_reason[30:16], also.Or at least mask off the bits 27:16? Thanks Yunhong Jiang> >Anyway, let me know what you think. > > -- Keir > >On 05/02/2010 10:22, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > >> Keir/Tim, here is attached patch. Please have a look onit. >> >> Thanks >> Yunhong Jiang >> >> # HG changeset patch >> # User Yunhong Jiang <yunhong.jiang@intel.com> >> # Date 1265363638 -28800 >> # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187 >> # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c >> We should not enable irq for machine check VMExit >> >> In changeset 18658:824892134573, IRQ is enabled during VMExit except external >> interrupt. The exception should apply for machine check also, because : >> a) The mce_logout_lock should be held in irq_disabled context. >> b) The machine check event should be handled as quickly as possible, enable >> irq will increase the period greatly. >> >> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >> >> diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c >> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000 >> +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800 >> @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned >> case EXIT_REASON_MCE_DURING_VMENTRY: >> printk("caused by machine check.\n"); >> HVMTRACE_0D(MCE); >> - do_machine_check(regs); >> + /* Handled already */ >> break; >> default: >> printk("reason not known yet!"); >> @@ -2259,6 +2259,23 @@ err: >> err: >> vmx_inject_hw_exception(TRAP_gp_fault, 0); >> return -1; >> +} >> + >> +int vmx_mce_exit(int exit_reason) >> +{ >> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && >> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) >> + return 1; >> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) >> + { >> + uint32_t vector; >> + >> + vector = __vmread(VM_EXIT_INTR_INFO) & >INTR_INFO_VECTOR_MASK; >> + if (vector == TRAP_machine_check) >> + return 1; >> + } >> + >> + return 0; >> } >> >> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) >> @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc >> /* Handle the interrupt we missed before allowing any more in. */ >> if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT ) >> vmx_do_extint(regs); >> + >> + if ( vmx_mce_exit(exit_reason) ) >> + do_machine_check(regs); >> >> /* Now enable interrupts so it''s safe to take locks. */ >> local_irq_enable(); >> @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc >> self_nmi(); /* Real NMI, vector 2: normal processing. */ >> break; >> case TRAP_machine_check: >> + dprintk(XENLOG_INFO, "VMexit for machine check\n"); >> HVMTRACE_0D(MCE); >> - do_machine_check(regs); >> + /* Handled already */ >> break; >> case TRAP_invalid_op: >> vmx_vmexit_ud_intercept(regs); >> >>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-05 14:37 UTC
RE: [Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
Tim, thanks for review. I add printk here because that is something not always happen. --jyh>-----Original Message----- >From: Tim Deegan [mailto:Tim.Deegan@citrix.com] >Sent: Friday, February 05, 2010 6:42 PM >To: Jiang, Yunhong >Cc: xen-devel@lists.xensource.com; Keir Fraser >Subject: Re: [Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit > >At 10:22 +0000 on 05 Feb (1265365365), Jiang, Yunhong wrote: >> Keir/Tim, here is attached patch. Please have a look onit. > >Yep, that looks better. Not sure that the dprintk is useful, though. > >Cheers, > >Tim. > >> Thanks >> Yunhong Jiang >> >> # HG changeset patch >> # User Yunhong Jiang <yunhong.jiang@intel.com> >> # Date 1265363638 -28800 >> # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187 >> # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c >> We should not enable irq for machine check VMExit >> >> In changeset 18658:824892134573, IRQ is enabled during VMExit except external >interrupt. The exception should apply for machine check also, because : >> a) The mce_logout_lock should be held in irq_disabled context. >> b) The machine check event should be handled as quickly as possible, enable irq will >increase the period greatly. >> >> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >> >> diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c >> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000 >> +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800 >> @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned >> case EXIT_REASON_MCE_DURING_VMENTRY: >> printk("caused by machine check.\n"); >> HVMTRACE_0D(MCE); >> - do_machine_check(regs); >> + /* Handled already */ >> break; >> default: >> printk("reason not known yet!"); >> @@ -2259,6 +2259,23 @@ err: >> err: >> vmx_inject_hw_exception(TRAP_gp_fault, 0); >> return -1; >> +} >> + >> +int vmx_mce_exit(int exit_reason) >> +{ >> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY && >> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) ) >> + return 1; >> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI)) >> + { >> + uint32_t vector; >> + >> + vector = __vmread(VM_EXIT_INTR_INFO) & >INTR_INFO_VECTOR_MASK; >> + if (vector == TRAP_machine_check) >> + return 1; >> + } >> + >> + return 0; >> } >> >> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs) >> @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc >> /* Handle the interrupt we missed before allowing any more in. */ >> if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT ) >> vmx_do_extint(regs); >> + >> + if ( vmx_mce_exit(exit_reason) ) >> + do_machine_check(regs); >> >> /* Now enable interrupts so it''s safe to take locks. */ >> local_irq_enable(); >> @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc >> self_nmi(); /* Real NMI, vector 2: normal processing. */ >> break; >> case TRAP_machine_check: >> + dprintk(XENLOG_INFO, "VMexit for machine check\n"); >> HVMTRACE_0D(MCE); >> - do_machine_check(regs); >> + /* Handled already */ >> break; >> case TRAP_invalid_op: >> vmx_vmexit_ud_intercept(regs); >> >> > > >Content-Description: ATT00001.txt >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > > >-- >Tim Deegan <Tim.Deegan@citrix.com> >Principal Software Engineer, Citrix Systems (R&D) Ltd. >[Company #02300071, SL9 0DZ, UK.]_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-05 14:59 UTC
Re: [Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
On 05/02/2010 14:36, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:>> How about the attached alternative, which avoids repeated reads of >> VM_INTR_INFO? Also I''m not sure whether checking for >> VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all, >> EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway. > > Thanks for your patch. Yes, it is much better to avoid the repeated read. > > I''m not sure if it is ok if we don''t check the VM_EXIT_REASONS_FAILED_VMENTRY. > Checking the SDM and seems it is ok. In fact, I didn''t find effective method > to test this VMEntry MCE failed case, althgouh I can test MCE VMExit with > EXIT_REASON_EXCEPTION_NMI case easily. (I will try to find a method to test > this VMEntry failure case next week, maybe poison the VMCS range can trigger > it, I''m not sure).Well, we don''t seem to know what bit 31 is for. Or, at least, we don''t know how it should affect our behaviour in the vmexit handler. So looking at it does seem a bit pointless. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Feb-07 04:25 UTC
RE: [Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit
Yes, agree and this patch is ok for me. Thanks very much. --jyh>-----Original Message----- >From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] >Sent: Friday, February 05, 2010 10:59 PM >To: Jiang, Yunhong; Tim Deegan >Cc: xen-devel@lists.xensource.com >Subject: Re: [Xen-devel] RE: [PATCH] Don''t enable irq for machine check vmexit > >On 05/02/2010 14:36, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > >>> How about the attached alternative, which avoids repeated reads of >>> VM_INTR_INFO? Also I''m not sure whether checking for >>> VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all, >>> EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway. >> >> Thanks for your patch. Yes, it is much better to avoid the repeated read. >> >> I''m not sure if it is ok if we don''t check the VM_EXIT_REASONS_FAILED_VMENTRY. >> Checking the SDM and seems it is ok. In fact, I didn''t find effective method >> to test this VMEntry MCE failed case, althgouh I can test MCE VMExit with >> EXIT_REASON_EXCEPTION_NMI case easily. (I will try to find a method to test >> this VMEntry failure case next week, maybe poison the VMCS range can trigger >> it, I''m not sure). > >Well, we don''t seem to know what bit 31 is for. Or, at least, we don''t know >how it should affect our behaviour in the vmexit handler. So looking at it >does seem a bit pointless. > > -- Keir >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel