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

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

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel