Re: [PATCH] Don't enable irq for machine check vmexit

[Keir Fraser <keir.fraser@eu.citrix.com>]

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
>