vmd: proper concurrency control when pausing a vm

Removes an XXX which slept for 1s waiting for the vcpu thread to reach HLT and
pause.  We now define a paused and unpaused condition so that a call to
pause_vm() / vmctl pause blocks till the vm really reaches a paused state.

Also, detach events for devices from event loop when pausing and add them back
when unpausing.  This is because some callbacks call pthread_mutex_lock and if
the vm is paused, it would block also causing the libevent thread to block.
This would mean that we would not be able to process any IMSGs received from vmm
(parent process) including a message to unpause.


ok mlarkin@

6 files changed, 169 insertions, 42 deletions

diff --git a/usr.sbin/vmd/ns8250.c b/usr.sbin/vmd/ns8250.c
index a6fd4a1f508..497e6fad550 100644
--- a/usr.sbin/vmd/ns8250.c
+++ b/usr.sbin/vmd/ns8250.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ns8250.c,v 1.24 2019/12/08 20:14:59 tb Exp $ */
+/* $OpenBSD: ns8250.c,v 1.25 2019/12/11 06:45:16 pd Exp $ */
 /*
  * Copyright (c) 2016 Mike Larkin <mlarkin@openbsd.org>
  *
@@ -663,7 +663,22 @@ ns8250_restore(int fd, int con_fd, uint32_t vmid)
 
 	event_set(&com1_dev.wake, com1_dev.fd, EV_WRITE,
 	    com_rcv_event, (void *)(intptr_t)vmid);
-	event_add(&com1_dev.wake, NULL);
 
 	return (0);
 }
+
+void
+ns8250_stop()
+{
+	if(event_del(&com1_dev.event))
+		log_warn("could not delete ns8250 event handler");
+	evtimer_del(&com1_dev.rate);
+}
+
+void
+ns8250_start()
+{
+	event_add(&com1_dev.event, NULL);
+	event_add(&com1_dev.wake, NULL);
+	evtimer_add(&com1_dev.rate, &com1_dev.rate_tv);
+}
diff --git a/usr.sbin/vmd/ns8250.h b/usr.sbin/vmd/ns8250.h
index cc9c8f9e894..a5d80df9a51 100644
--- a/usr.sbin/vmd/ns8250.h
+++ b/usr.sbin/vmd/ns8250.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ns8250.h,v 1.8 2019/05/28 07:36:37 mlarkin Exp $ */
+/* $OpenBSD: ns8250.h,v 1.9 2019/12/11 06:45:16 pd Exp $ */
 /*
  * Copyright (c) 2016 Mike Larkin <mlarkin@openbsd.org>
  *
@@ -88,3 +88,5 @@ void vcpu_process_com_msr(struct vm_exit *);
 void vcpu_process_com_scr(struct vm_exit *);
 int ns8250_dump(int);
 int ns8250_restore(int, int, uint32_t);
+void ns8250_stop(void);
+void ns8250_start(void);
diff --git a/usr.sbin/vmd/virtio.c b/usr.sbin/vmd/virtio.c
index 92168f59156..8800594fc61 100644
--- a/usr.sbin/vmd/virtio.c
+++ b/usr.sbin/vmd/virtio.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: virtio.c,v 1.81 2019/11/30 00:51:29 mlarkin Exp $	*/
+/*	$OpenBSD: virtio.c,v 1.82 2019/12/11 06:45:16 pd Exp $	*/
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -2140,11 +2140,6 @@ vionet_restore(int fd, struct vmd_vm *vm, int *child_taps)
 			memset(&vionet[i].event, 0, sizeof(struct event));
 			event_set(&vionet[i].event, vionet[i].fd,
 			    EV_READ | EV_PERSIST, vionet_rx_event, &vionet[i]);
-			if (event_add(&vionet[i].event, NULL)) {
-				log_warn("could not initialize vionet event "
-				    "handler");
-				return (-1);
-			}
 		}
 	}
 	return (0);
@@ -2338,3 +2333,29 @@ virtio_dump(int fd)
 
 	return (0);
 }
+
+void
+virtio_stop(struct vm_create_params *vcp)
+{
+	uint8_t i;
+	for (i = 0; i < vcp->vcp_nnics; i++) {
+		if (event_del(&vionet[i].event)) {
+			log_warn("could not initialize vionet event "
+			    "handler");
+			return;
+		}
+	}
+}
+
+void
+virtio_start(struct vm_create_params *vcp)
+{
+	uint8_t i;
+	for (i = 0; i < vcp->vcp_nnics; i++) {
+		if (event_add(&vionet[i].event, NULL)) {
+			log_warn("could not initialize vionet event "
+			    "handler");
+			return;
+		}
+	}
+}
diff --git a/usr.sbin/vmd/virtio.h b/usr.sbin/vmd/virtio.h
index f0a5513ab51..9a64973ab99 100644
--- a/usr.sbin/vmd/virtio.h
+++ b/usr.sbin/vmd/virtio.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: virtio.h,v 1.34 2018/12/06 09:20:06 claudio Exp $	*/
+/*	$OpenBSD: virtio.h,v 1.35 2019/12/11 06:45:16 pd Exp $	*/
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -318,3 +318,5 @@ int vioscsi_io(int, uint16_t, uint32_t *, uint8_t *, void *, uint8_t);
 void vioscsi_update_qs(struct vioscsi_dev *);
 void vioscsi_update_qa(struct vioscsi_dev *);
 int vioscsi_notifyq(struct vioscsi_dev *);
+void virtio_stop(struct vm_create_params *vcp);
+void virtio_start(struct vm_create_params *vcp);
diff --git a/usr.sbin/vmd/vm.c b/usr.sbin/vmd/vm.c
index 1e687924a61..f9fe1897ea1 100644
--- a/usr.sbin/vmd/vm.c
+++ b/usr.sbin/vmd/vm.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: vm.c,v 1.53 2019/11/30 00:51:29 mlarkin Exp $	*/
+/*	$OpenBSD: vm.c,v 1.54 2019/12/11 06:45:16 pd Exp $	*/
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -111,6 +111,10 @@ pthread_cond_t threadcond;
 
 pthread_cond_t vcpu_run_cond[VMM_MAX_VCPUS_PER_VM];
 pthread_mutex_t vcpu_run_mtx[VMM_MAX_VCPUS_PER_VM];
+pthread_cond_t vcpu_pause_cond[VMM_MAX_VCPUS_PER_VM];
+pthread_mutex_t vcpu_pause_mtx[VMM_MAX_VCPUS_PER_VM];
+pthread_cond_t vcpu_unpause_cond[VMM_MAX_VCPUS_PER_VM];
+pthread_mutex_t vcpu_unpause_mtx[VMM_MAX_VCPUS_PER_VM];
 uint8_t vcpu_hlt[VMM_MAX_VCPUS_PER_VM];
 uint8_t vcpu_done[VMM_MAX_VCPUS_PER_VM];
 
@@ -365,10 +369,10 @@ start_vm(struct vmd_vm *vm, int fd)
 	if (vm->vm_state & VM_STATE_RECEIVED) {
 		restore_emulated_hw(vcp, vm->vm_receive_fd, nicfds,
 		    vm->vm_disks, vm->vm_cdrom);
-		mc146818_start();
 		restore_mem(vm->vm_receive_fd, vcp);
 		if (restore_vm_params(vm->vm_receive_fd, vcp))
 			fatal("restore vm params failed");
+		unpause_vm(vcp);
 	}
 
 	if (vmm_pipe(vm, fd, vm_dispatch_vmm) == -1)
@@ -732,32 +736,70 @@ restore_vmr(int fd, struct vm_mem_range *vmr)
 void
 pause_vm(struct vm_create_params *vcp)
 {
+	unsigned int n;
+	int ret;
 	if (current_vm->vm_state & VM_STATE_PAUSED)
 		return;
 
 	current_vm->vm_state |= VM_STATE_PAUSED;
 
-	/* XXX: vcpu_run_loop is running in another thread and we have to wait
-	 * for the vm to exit before returning */
-	sleep(1);
+	for (n = 0; n < vcp->vcp_ncpus; n++) {
+		ret = pthread_mutex_lock(&vcpu_pause_mtx[n]);
+		if (ret) {
+			log_warnx("%s: can't lock vcpu pause mtx (%d)",
+			    __func__, (int)ret);
+			return;
+		}
+
+		ret = pthread_cond_broadcast(&vcpu_run_cond[n]);
+		if (ret) {
+			log_warnx("%s: can't broadcast vcpu run cond (%d)",
+			    __func__, (int)ret);
+			return;
+		}
+
+		ret = pthread_cond_wait(&vcpu_pause_cond[n], &vcpu_pause_mtx[n]);
+		if (ret) {
+			log_warnx("%s: can't wait on vcpu pause cond (%d)",
+			    __func__, (int)ret);
+			return;
+		}
+		ret = pthread_mutex_unlock(&vcpu_pause_mtx[n]);
+		if (ret) {
+			log_warnx("%s: can't unlock vcpu mtx (%d)",
+			    __func__, (int)ret);
+			return;
+		}
+	}
 
 	i8253_stop();
 	mc146818_stop();
+	ns8250_stop();
+	virtio_stop(vcp);
 }
 
 void
 unpause_vm(struct vm_create_params *vcp)
 {
 	unsigned int n;
+	int ret;
 	if (!(current_vm->vm_state & VM_STATE_PAUSED))
 		return;
 
 	current_vm->vm_state &= ~VM_STATE_PAUSED;
+	for (n = 0; n < vcp->vcp_ncpus; n++) {
+		ret = pthread_cond_broadcast(&vcpu_unpause_cond[n]);
+		if (ret) {
+			log_warnx("%s: can't broadcast vcpu unpause cond (%d)",
+			    __func__, (int)ret);
+			return;
+		}
+	}
 
 	i8253_start();
 	mc146818_start();
-	for (n = 0; n <= vcp->vcp_ncpus; n++)
-		pthread_cond_broadcast(&vcpu_run_cond[n]);
+	ns8250_start();
+	virtio_start(vcp);
 }
 
 /*
@@ -1218,6 +1260,32 @@ run_vm(int child_cdrom, int child_disks[][VM_MAX_BASE_PER_DISK],
 			    __progname, ret);
 			return (ret);
 		}
+		ret = pthread_cond_init(&vcpu_pause_cond[i], NULL);
+		if (ret) {
+			log_warnx("%s: cannot initialize pause cond var (%d)",
+			    __progname, ret);
+			return (ret);
+		}
+
+		ret = pthread_mutex_init(&vcpu_pause_mtx[i], NULL);
+		if (ret) {
+			log_warnx("%s: cannot initialize pause mtx (%d)",
+			    __progname, ret);
+			return (ret);
+		}
+		ret = pthread_cond_init(&vcpu_unpause_cond[i], NULL);
+		if (ret) {
+			log_warnx("%s: cannot initialize unpause var (%d)",
+			    __progname, ret);
+			return (ret);
+		}
+
+		ret = pthread_mutex_init(&vcpu_unpause_mtx[i], NULL);
+		if (ret) {
+			log_warnx("%s: cannot initialize unpause mtx (%d)",
+			    __progname, ret);
+			return (ret);
+		}
 
 		vcpu_hlt[i] = 0;
 
@@ -1340,32 +1408,50 @@ vcpu_run_loop(void *arg)
 			return ((void *)ret);
 		}
 
-		/* If we are halted or paused, wait */
-		if (vcpu_hlt[n]) {
-			while (current_vm->vm_state & VM_STATE_PAUSED) {
-				ret = pthread_cond_wait(&vcpu_run_cond[n],
-				    &vcpu_run_mtx[n]);
-				if (ret) {
-					log_warnx(
-					    "%s: can't wait on cond (%d)",
-					    __func__, (int)ret);
-					(void)pthread_mutex_unlock(
-					    &vcpu_run_mtx[n]);
-					break;
-				}
+		/* If we are halted and need to pause, pause */
+		if (vcpu_hlt[n] && (current_vm->vm_state & VM_STATE_PAUSED)) {
+			ret = pthread_cond_broadcast(&vcpu_pause_cond[n]);
+			if (ret) {
+				log_warnx("%s: can't broadcast vcpu pause mtx"
+				    "(%d)", __func__, (int)ret);
+				return ((void *)ret);
+			}
+
+			ret = pthread_mutex_lock(&vcpu_unpause_mtx[n]);
+			if (ret) {
+				log_warnx("%s: can't lock vcpu unpause mtx (%d)",
+				    __func__, (int)ret);
+				return ((void *)ret);
 			}
-			if (vcpu_hlt[n]) {
-				ret = pthread_cond_wait(&vcpu_run_cond[n],
-				    &vcpu_run_mtx[n]);
 
-				if (ret) {
-					log_warnx(
-					    "%s: can't wait on cond (%d)",
-					    __func__, (int)ret);
-					(void)pthread_mutex_unlock(
-					    &vcpu_run_mtx[n]);
-					break;
-				}
+			ret = pthread_cond_wait(&vcpu_unpause_cond[n],
+			    &vcpu_unpause_mtx[n]);
+			if (ret) {
+				log_warnx(
+				    "%s: can't wait on unpause cond (%d)",
+				    __func__, (int)ret);
+				break;
+			}
+			ret = pthread_mutex_unlock(&vcpu_unpause_mtx[n]);
+			if (ret) {
+				log_warnx("%s: can't unlock unpause mtx (%d)",
+				    __func__, (int)ret);
+				break;
+			}
+		}
+
+		/* If we are halted and not paused, wait */
+		if (vcpu_hlt[n]) {
+			ret = pthread_cond_wait(&vcpu_run_cond[n],
+			    &vcpu_run_mtx[n]);
+
+			if (ret) {
+				log_warnx(
+				    "%s: can't wait on cond (%d)",
+				    __func__, (int)ret);
+				(void)pthread_mutex_unlock(
+				    &vcpu_run_mtx[n]);
+				break;
 			}
 		}
 
diff --git a/usr.sbin/vmd/vmm.c b/usr.sbin/vmd/vmm.c
index bb3fdd75e0d..fcc0c1e4be9 100644
--- a/usr.sbin/vmd/vmm.c
+++ b/usr.sbin/vmd/vmm.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: vmm.c,v 1.94 2019/10/25 09:57:33 kn Exp $	*/
+/*	$OpenBSD: vmm.c,v 1.95 2019/12/11 06:45:17 pd Exp $	*/
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -316,6 +316,7 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 		    imsg->hdr.peerid, vmc.vmc_owner.uid);
 		vm->vm_tty = imsg->fd;
 		vm->vm_state |= VM_STATE_RECEIVED;
+		vm->vm_state |= VM_STATE_PAUSED;
 		break;
 	case IMSG_VMDOP_RECEIVE_VM_END:
 		if ((vm = vm_getbyvmid(imsg->hdr.peerid)) == NULL) {