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

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

[-- Attachment #1: Type: text/plain, Size: 3372 bytes --]

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.

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.

Anyway, let me know what you think.

 -- Keir

On 05/02/2010 10:22, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> Keir/Tim, here is attached patch. Please have a look onit.
> 
> Thanks
> Yunhong Jiang
> 
> # HG changeset patch
> # User Yunhong Jiang <yunhong.jiang@intel.com>
> # 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@intel.com>
> 
> 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);
> 
> 


[-- Attachment #2: 00-mce --]
[-- Type: application/octet-stream, Size: 2868 bytes --]

diff -r 989014ce7b4a xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Fri Feb 05 10:37:24 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Fri Feb 05 12:48:39 2010 +0000
@@ -2168,7 +2168,7 @@
     case EXIT_REASON_MCE_DURING_VMENTRY:
         printk("caused by machine check.\n");
         HVMTRACE_0D(MCE);
-        do_machine_check(regs);
+        /* Already handled. */
         break;
     default:
         printk("reason not known yet!");
@@ -2263,7 +2263,7 @@
 
 asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
 {
-    unsigned int exit_reason, idtv_info;
+    unsigned int exit_reason, idtv_info, intr_info = 0, vector = 0;
     unsigned long exit_qualification, inst_len = 0;
     struct vcpu *v = current;
 
@@ -2285,8 +2285,22 @@
     perfc_incra(vmexits, exit_reason);
 
     /* Handle the interrupt we missed before allowing any more in. */
-    if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
+    switch ( (uint16_t)exit_reason )
+    {
+    case EXIT_REASON_EXTERNAL_INTERRUPT:
         vmx_do_extint(regs);
+        break;
+    case EXIT_REASON_EXCEPTION_NMI:
+        intr_info = __vmread(VM_EXIT_INTR_INFO);
+        BUG_ON(!(intr_info & INTR_INFO_VALID_MASK));
+        vector = intr_info & INTR_INFO_VECTOR_MASK;
+        if ( vector == TRAP_machine_check )
+            do_machine_check(regs);
+        break;
+    case EXIT_REASON_MCE_DURING_VMENTRY:
+        do_machine_check(regs);
+        break;
+    }
 
     /* Now enable interrupts so it's safe to take locks. */
     local_irq_enable();
@@ -2296,8 +2310,6 @@
 
     if ( v->arch.hvm_vmx.vmx_realmode )
     {
-        unsigned int vector;
-
         /* Put RFLAGS back the way the guest wants it */
         regs->eflags &= ~(X86_EFLAGS_VM | X86_EFLAGS_IOPL);
         regs->eflags |= (v->arch.hvm_vmx.vm86_saved_eflags & X86_EFLAGS_IOPL);
@@ -2307,7 +2319,6 @@
         switch ( exit_reason )
         {
         case EXIT_REASON_EXCEPTION_NMI:
-            vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
             if ( vector != TRAP_page_fault
                  && vector != TRAP_nmi 
                  && vector != TRAP_machine_check ) 
@@ -2366,12 +2377,6 @@
          * (1) We can get an exception (e.g. #PG) in the guest, or
          * (2) NMI
          */
-        unsigned int intr_info, vector;
-
-        intr_info = __vmread(VM_EXIT_INTR_INFO);
-        BUG_ON(!(intr_info & INTR_INFO_VALID_MASK));
-
-        vector = intr_info & INTR_INFO_VECTOR_MASK;
 
         /*
          * Re-set the NMI shadow if vmexit caused by a guest IRET fault (see 3B
@@ -2448,7 +2453,7 @@
             break;
         case TRAP_machine_check:
             HVMTRACE_0D(MCE);
-            do_machine_check(regs);
+            /* Already handled above. */
             break;
         case TRAP_invalid_op:
             vmx_vmexit_ud_intercept(regs);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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