WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[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@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