Re: [Xen-devel] [PATCH for-4.5 RFC v2] x86/HVM: Unconditionally crash guests on repeated vmentry failures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.11.14 at 18:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
> identify a failed vmentry and lets it fall into the default case of the
> vmexit handler.  As such, with v1, the infinite loop still affects AMD
> hardware.

Right; I should have said "something along the lines of v1". An SVM
part is needed, but that should probably extend beyond what you
proposed in v2: There are a number of "goto exit_and_crash"
statements ahead of where you place your addition. I think they
all need to be treated similarly.

I therefore think we should revert the VMX part of 28b4baacd5
and make SVM behavior consistent with what results for VMX:
Crash the guest unconditionally on VMEXIT_INVALID, without
looking for recurring VM entry failures. See below/attached.

Jan

x86/HVM: prevent infinite VM entry retries

This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
guest upon problems occurring in user mode") and gets SVM in line with
the resulting VMX behavior. This is because Andrew validly says

"A failed vmentry is overwhelmingly likely to be caused by corrupt
 VMC[SB] state.  As a result, injecting a fault and retrying the the
 vmentry is likely to fail in the same way."

Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2338,6 +2338,7 @@ void svm_vmexit_handler(struct cpu_user_
         struct nestedvcpu *nv = &vcpu_nestedhvm(v);
         struct vmcb_struct *ns_vmcb = nv->nv_vvmcx;
         uint64_t exitinfo1, exitinfo2;
+        bool_t crash = 0;
 
         paging_update_nestedmode(v);
 
@@ -2371,13 +2372,16 @@ void svm_vmexit_handler(struct cpu_user_
                 goto out;
             case NESTEDHVM_VMEXIT_FATALERROR:
                 gdprintk(XENLOG_ERR, "unexpected nestedsvm_vmexit() error\n");
-                goto exit_and_crash;
-
+                crash = 1;
+                break;
             default:
                 BUG();
             case NESTEDHVM_VMEXIT_ERROR:
                 break;
             }
+            if ( crash )
+                break;
+            /* fall through */
         case NESTEDHVM_VMEXIT_ERROR:
             gdprintk(XENLOG_ERR,
                 "nestedsvm_check_intercepts() returned 
NESTEDHVM_VMEXIT_ERROR\n");
@@ -2385,18 +2389,25 @@ void svm_vmexit_handler(struct cpu_user_
         case NESTEDHVM_VMEXIT_FATALERROR:
             gdprintk(XENLOG_ERR,
                 "unexpected nestedsvm_check_intercepts() error\n");
-            goto exit_and_crash;
+            crash = 1;
+            break;
         default:
             gdprintk(XENLOG_INFO, "nestedsvm_check_intercepts() returned %i\n",
                 nsret);
-            goto exit_and_crash;
+            crash = 1;
+            break;
         }
+
+        if ( unlikely(crash) && exit_reason != VMEXIT_INVALID )
+            goto exit_and_crash;
     }
 
     if ( unlikely(exit_reason == VMEXIT_INVALID) )
     {
+        gdprintk(XENLOG_ERR, "invalid VMCB state:\n");
         svm_vmcb_dump(__func__, vmcb);
-        goto exit_and_crash;
+        domain_crash(v->domain);
+        return;
     }
 
     perfc_incra(svmexits, exit_reason);
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -134,18 +134,6 @@ static void vmx_vcpu_destroy(struct vcpu
     passive_domain_destroy(v);
 }
 
-/* Only crash the guest if the problem originates in kernel mode. */
-static void vmx_crash_or_fault(struct vcpu *v)
-{
-    struct segment_register ss;
-
-    vmx_get_segment_register(v, x86_seg_ss, &ss);
-    if ( ss.attr.fields.dpl )
-        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
-    else
-        domain_crash(v->domain);
-}
-
 static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
 
 static const u32 msr_index[] =
@@ -2520,7 +2508,7 @@ static void vmx_failed_vmentry(unsigned 
     vmcs_dump_vcpu(curr);
     printk("**************************************\n");
 
-    vmx_crash_or_fault(curr);
+    domain_crash(curr->domain);
 }
 
 void vmx_enter_realmode(struct cpu_user_regs *regs)
@@ -3173,8 +3161,19 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
-        vmx_crash_or_fault(v);
+        {
+            struct segment_register ss;
+
+            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
+                     exit_reason);
+
+            vmx_get_segment_register(v, x86_seg_ss, &ss);
+            if ( ss.attr.fields.dpl )
+                hvm_inject_hw_exception(TRAP_invalid_op,
+                                        HVM_DELIVER_NO_ERROR_CODE);
+            else
+                domain_crash(v->domain);
+        }
         break;
     }

Attachment: x86-HVM-prevent-infinite-VMentry-loop.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel