[Xen-devel] RE: [PATCH] Don't enable irq for machine check vmexit

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

I got the issue. Thanks!
I will re-do the patch tomorrow.

Thanks
Yunhong Jiang


>-----Original Message-----
>From: Tim Deegan [mailto:Tim.Deegan@xxxxxxxxxx]
>Sent: Thursday, February 04, 2010 10:22 PM
>To: Jiang, Yunhong
>Cc: Keir Fraser; xen-devel@xxxxxxxxxxxxxxxxxxx
>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@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