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@xxxxxxxxx> 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@xxxxxxxxxx
> >>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> >>> 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@xxxxxxxxxx
> >>>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> >>>> 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@xxxxxxxxx>
> >>>>
> >>>> This is in hotspot code path, I try to use unlikely, hope to reduce the
> >>>> performance
> >>>> impact
> >>>>
> >>>> Thanks
> >>>> Yunhong Jiang
> >>
> >
>
--
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home