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

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

>-----Original Message-----
>From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
>[mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Keir Fraser
>Sent: Friday, February 05, 2010 8:54 PM
>To: Jiang, Yunhong; Tim Deegan
>Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
>Subject: [Xen-devel] Re: [PATCH] Don't enable irq for machine check vmexit
>
>How about the attached alternative, which avoids repeated reads of
>VM_INTR_INFO? Also I'm not sure whether checking for
>VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all,
>EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway.

Thanks for your patch. Yes, it is much better to avoid the repeated read.

I'm not sure if it is ok if we don't check the VM_EXIT_REASONS_FAILED_VMENTRY. 
Checking the SDM and seems it is ok. In fact, I didn't find effective method to 
test this VMEntry MCE failed case, althgouh I can test MCE VMExit with 
EXIT_REASON_EXCEPTION_NMI case easily. (I will try to find a method to test 
this VMEntry failure case next week, maybe poison the VMCS range can trigger 
it, I'm not sure).

Or maybe code like following? It will increase a "&" operation for each exit, 
but that should make us safe.

-    if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
+    switch ( exit_reason & (VMX_EXIT_REASONS_FAILED_VMENTRY | 0xFFFF))
+    {
+    case EXIT_REASON_EXTERNAL_INTERRUPT:
         vmx_do_extint(regs);
+        break;

....
+    case EXIT_REASON_MCE_DURING_VMENTRY| VMX_EXIT_REASONS_FAILED_VMENTRY: ---> 
(or define a new macro for it)
+        do_machine_check(regs);
+        break

>
>Perhaps we should mask exit_reason down to 16 bits right at the top of the
>vmexit handler? That would save us if new flags are added to
>exit_reason[30:16], also.

Or at least mask off the bits 27:16?

Thanks
Yunhong Jiang

>
>Anyway, let me know what you think.
>
> -- Keir
>
>On 05/02/2010 10:22, "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx> wrote:
>
>> Keir/Tim, here is attached patch. Please have a look onit.
>>
>> Thanks
>> Yunhong Jiang
>>
>> # HG changeset patch
>> # User Yunhong Jiang <yunhong.jiang@xxxxxxxxx>
>> # Date 1265363638 -28800
>> # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
>> # Parent  7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
>> 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>
>>
>> diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
>> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
>> @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
>>      case EXIT_REASON_MCE_DURING_VMENTRY:
>>          printk("caused by machine check.\n");
>>          HVMTRACE_0D(MCE);
>> -        do_machine_check(regs);
>> +        /* Handled already */
>>          break;
>>      default:
>>          printk("reason not known yet!");
>> @@ -2259,6 +2259,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)
>> @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
>>      /* Handle the interrupt we missed before allowing any more in. */
>>      if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
>>          vmx_do_extint(regs);
>> +
>> +    if ( vmx_mce_exit(exit_reason) )
>> +        do_machine_check(regs);
>>
>>      /* Now enable interrupts so it's safe to take locks. */
>>      local_irq_enable();
>> @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
>>              self_nmi(); /* Real NMI, vector 2: normal processing. */
>>              break;
>>          case TRAP_machine_check:
>> +            dprintk(XENLOG_INFO, "VMexit for machine check\n");
>>              HVMTRACE_0D(MCE);
>> -            do_machine_check(regs);
>> +            /* Handled already */
>>              break;
>>          case TRAP_invalid_op:
>>              vmx_vmexit_ud_intercept(regs);
>>
>>


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