svn commit: r259737 - in head: sys/amd64/include sys/amd64/vmm usr.sbin/bhyve

[Neel Natu]

Author: neel
Date: Sun Dec 22 20:29:59 2013
New Revision: 259737
URL: http://svnweb.freebsd.org/changeset/base/259737

Log:
  Add a parameter to 'vcpu_set_state()' to enforce that the vcpu is in the IDLE
  state before the requested state transition. This guarantees that there is
  exactly one ioctl() operating on a vcpu at any point in time and prevents
  unintended state transitions.
  
  More details available here:
  http://lists.freebsd.org/pipermail/freebsd-virtualization/2013-December/001825.html
  
  Reviewed by:	grehan
  Reported by:	Markiyan Kushnir (markiyan.kushnir at gmail.com)
  MFC after:	3 days

Modified:
  head/sys/amd64/include/vmm.h
  head/sys/amd64/vmm/vmm.c
  head/sys/amd64/vmm/vmm_dev.c
  head/usr.sbin/bhyve/bhyverun.c

Modified: head/sys/amd64/include/vmm.h
==============================================================================
--- head/sys/amd64/include/vmm.h	Sun Dec 22 19:47:22 2013	(r259736)
+++ head/sys/amd64/include/vmm.h	Sun Dec 22 20:29:59 2013	(r259737)
@@ -146,7 +146,8 @@ enum vcpu_state {
 	VCPU_SLEEPING,
 };
 
-int vcpu_set_state(struct vm *vm, int vcpu, enum vcpu_state state);
+int vcpu_set_state(struct vm *vm, int vcpu, enum vcpu_state state,
+    bool from_idle);
 enum vcpu_state vcpu_get_state(struct vm *vm, int vcpu, int *hostcpu);
 
 static int __inline

Modified: head/sys/amd64/vmm/vmm.c
==============================================================================
--- head/sys/amd64/vmm/vmm.c	Sun Dec 22 19:47:22 2013	(r259736)
+++ head/sys/amd64/vmm/vmm.c	Sun Dec 22 20:29:59 2013	(r259737)
@@ -804,13 +804,27 @@ save_guest_fpustate(struct vcpu *vcpu)
 static VMM_STAT(VCPU_IDLE_TICKS, "number of ticks vcpu was idle");
 
 static int
-vcpu_set_state_locked(struct vcpu *vcpu, enum vcpu_state newstate)
+vcpu_set_state_locked(struct vcpu *vcpu, enum vcpu_state newstate,
+    bool from_idle)
 {
 	int error;
 
 	vcpu_assert_locked(vcpu);
 
 	/*
+	 * State transitions from the vmmdev_ioctl() must always begin from
+	 * the VCPU_IDLE state. This guarantees that there is only a single
+	 * ioctl() operating on a vcpu at any point.
+	 */
+	if (from_idle) {
+		while (vcpu->state != VCPU_IDLE)
+			msleep_spin(&vcpu->state, &vcpu->mtx, "vmstat", hz);
+	} else {
+		KASSERT(vcpu->state != VCPU_IDLE, ("invalid transition from "
+		    "vcpu idle state"));
+	}
+
+	/*
 	 * The following state transitions are allowed:
 	 * IDLE -> FROZEN -> IDLE
 	 * FROZEN -> RUNNING -> FROZEN
@@ -830,12 +844,14 @@ vcpu_set_state_locked(struct vcpu *vcpu,
 		break;
 	}
 
-	if (error == 0)
-		vcpu->state = newstate;
-	else
-		error = EBUSY;
+	if (error)
+		return (EBUSY);
 
-	return (error);
+	vcpu->state = newstate;
+	if (newstate == VCPU_IDLE)
+		wakeup(&vcpu->state);
+
+	return (0);
 }
 
 static void
@@ -843,7 +859,7 @@ vcpu_require_state(struct vm *vm, int vc
 {
 	int error;
 
-	if ((error = vcpu_set_state(vm, vcpuid, newstate)) != 0)
+	if ((error = vcpu_set_state(vm, vcpuid, newstate, false)) != 0)
 		panic("Error %d setting state to %d\n", error, newstate);
 }
 
@@ -852,7 +868,7 @@ vcpu_require_state_locked(struct vcpu *v
 {
 	int error;
 
-	if ((error = vcpu_set_state_locked(vcpu, newstate)) != 0)
+	if ((error = vcpu_set_state_locked(vcpu, newstate, false)) != 0)
 		panic("Error %d setting state to %d", error, newstate);
 }
 
@@ -1235,7 +1251,8 @@ vm_iommu_domain(struct vm *vm)
 }
 
 int
-vcpu_set_state(struct vm *vm, int vcpuid, enum vcpu_state newstate)
+vcpu_set_state(struct vm *vm, int vcpuid, enum vcpu_state newstate,
+    bool from_idle)
 {
 	int error;
 	struct vcpu *vcpu;
@@ -1246,7 +1263,7 @@ vcpu_set_state(struct vm *vm, int vcpuid
 	vcpu = &vm->vcpu[vcpuid];
 
 	vcpu_lock(vcpu);
-	error = vcpu_set_state_locked(vcpu, newstate);
+	error = vcpu_set_state_locked(vcpu, newstate, from_idle);
 	vcpu_unlock(vcpu);
 
 	return (error);

Modified: head/sys/amd64/vmm/vmm_dev.c
==============================================================================
--- head/sys/amd64/vmm/vmm_dev.c	Sun Dec 22 19:47:22 2013	(r259736)
+++ head/sys/amd64/vmm/vmm_dev.c	Sun Dec 22 20:29:59 2013	(r259737)
@@ -197,7 +197,7 @@ vmmdev_ioctl(struct cdev *cdev, u_long c
 			goto done;
 		}
 
-		error = vcpu_set_state(sc->vm, vcpu, VCPU_FROZEN);
+		error = vcpu_set_state(sc->vm, vcpu, VCPU_FROZEN, true);
 		if (error)
 			goto done;
 
@@ -214,14 +214,14 @@ vmmdev_ioctl(struct cdev *cdev, u_long c
 		 */
 		error = 0;
 		for (vcpu = 0; vcpu < VM_MAXCPU; vcpu++) {
-			error = vcpu_set_state(sc->vm, vcpu, VCPU_FROZEN);
+			error = vcpu_set_state(sc->vm, vcpu, VCPU_FROZEN, true);
 			if (error)
 				break;
 		}
 
 		if (error) {
 			while (--vcpu >= 0)
-				vcpu_set_state(sc->vm, vcpu, VCPU_IDLE);
+				vcpu_set_state(sc->vm, vcpu, VCPU_IDLE, false);
 			goto done;
 		}
 
@@ -382,10 +382,10 @@ vmmdev_ioctl(struct cdev *cdev, u_long c
 	}
 
 	if (state_changed == 1) {
-		vcpu_set_state(sc->vm, vcpu, VCPU_IDLE);
+		vcpu_set_state(sc->vm, vcpu, VCPU_IDLE, false);
 	} else if (state_changed == 2) {
 		for (vcpu = 0; vcpu < VM_MAXCPU; vcpu++)
-			vcpu_set_state(sc->vm, vcpu, VCPU_IDLE);
+			vcpu_set_state(sc->vm, vcpu, VCPU_IDLE, false);
 	}
 
 done:

Modified: head/usr.sbin/bhyve/bhyverun.c
==============================================================================
--- head/usr.sbin/bhyve/bhyverun.c	Sun Dec 22 19:47:22 2013	(r259736)
+++ head/usr.sbin/bhyve/bhyverun.c	Sun Dec 22 20:29:59 2013	(r259737)
@@ -485,19 +485,8 @@ vm_loop(struct vmctx *ctx, int vcpu, uin
 
 	while (1) {
 		error = vm_run(ctx, vcpu, rip, &vmexit[vcpu]);
-		if (error != 0) {
-			/*
-			 * It is possible that 'vmmctl' or some other process
-			 * has transitioned the vcpu to CANNOT_RUN state right
-			 * before we tried to transition it to RUNNING.
-			 *
-			 * This is expected to be temporary so just retry.
-			 */
-			if (errno == EBUSY)
-				continue;
-			else
-				break;
-		}
+		if (error != 0)
+			break;
 
 		prevcpu = vcpu;