Index | Thread | Search

From:
Dave Voutila <dv@sisu.io>
Subject:
fix accidental vm reboot during `halt -p`
To:
tech@openbsd.org
Cc:
Claudio Jeker <claudio@openbsd.org>
Date:
Wed, 20 Nov 2024 17:28:01 -0500

Download raw body.

Thread
claudio brought this to my attention recently and it took some thread
pulling to figure out.

If one runs `halt -p` in an OpenBSD guest under vmd(8) and then smacks
ENTER as soon as you see:

  The operating system has halted.
  Please press any key to reboot.

...you'll end up with a rebooting vm because you won a race with a
timeout in vmd. (The tl;dr: we don't really implement "poweroff" support
in vmd yet, so we anticipate the triple-fault used for reboot and just
don't re-create the vm.)

I'm not too happy with the diff because it perpetuates some
BadIdeas(tm), mainly how it's using imsg_flush(3) and relying on the
boothowto kernel global, but this diff "works for me."

To test, make sure you build a kernel with the vmmci(4) changes and
install or boot *that* kernel in the guest. You can either copy it into
the guest or use the `-b` option with vmctl(8) to provide an override
kernel.

-dv


diffstat refs/heads/master refs/heads/vmmci-reboot
 M  sys/dev/pv/vmmci.c     |   8+   3-
 M  usr.sbin/vmd/virtio.c  |   8+   3-
 M  usr.sbin/vmd/vm.c      |  10+  13-
 M  usr.sbin/vmd/vmd.h     |   2+   1-

4 files changed, 28 insertions(+), 20 deletions(-)

diff refs/heads/master refs/heads/vmmci-reboot
commit - e283d8fbe4710b00487a897b3ac59307fdc34b04
commit + fbc277543dd1b43724bb914838fd4059ac2f5a14
blob - 580228bdb4cb6d7fac30248b07b4c920b427a471
blob + 665e7389dc3372c28f60f674f8ccd124b44b4d24
--- sys/dev/pv/vmmci.c
+++ sys/dev/pv/vmmci.c
@@ -21,6 +21,7 @@
 #include <sys/timeout.h>
 #include <sys/device.h>
 #include <sys/sensors.h>
+#include <sys/reboot.h>
 
 #include <machine/bus.h>
 
@@ -123,10 +124,14 @@ vmmci_activate(struct device *self, int act)
 {
 	struct vmmci_softc	*sc = (struct vmmci_softc *)self;
 	struct virtio_softc	*vsc = sc->sc_virtio;
+	int			 powerdown = 0;
 
 	if (virtio_has_feature(vsc, VMMCI_F_ACK) == 0)
 		return (0);
 
+	if (boothowto & RB_HALT)
+		powerdown = 1;
+
 	switch (act) {
 	case DVACT_POWERDOWN:
 		printf("%s: powerdown\n", sc->sc_dev.dv_xname);
@@ -139,7 +144,7 @@ vmmci_activate(struct device *self, int act)
 		 * without hooking into the MD code directly.
 		 */
 		virtio_write_device_config_4(vsc, VMMCI_CONFIG_COMMAND,
-		    VMMCI_SHUTDOWN);
+		    powerdown ? VMMCI_SHUTDOWN : VMMCI_REBOOT);
 		break;
 	default:
 		break;
@@ -172,10 +177,10 @@ vmmci_config_change(struct virtio_softc *vsc)
 	case VMMCI_SYNCRTC:
 		inittodr(gettime());
 		sc->sc_cmd = VMMCI_NONE;
-		break;	
+		break;
 	default:
 		printf("%s: invalid command %d\n", sc->sc_dev.dv_xname, cmd);
-		cmd = VMMCI_NONE;		
+		cmd = VMMCI_NONE;
 		break;
 	}
 
blob - fa924bfc2640497ee042b7ef089b3822e6f29502
blob + 50b2d93fef83a043746b87c3b6d56b087cf64885
--- usr.sbin/vmd/virtio.c
+++ usr.sbin/vmd/virtio.c
@@ -349,10 +349,10 @@ vmmci_ack(unsigned int cmd)
 			    vmmci.vm_id);
 			tv.tv_sec = VMMCI_TIMEOUT;
 			evtimer_add(&vmmci.timeout, &tv);
+			vm_notify_vmm(1);
 			return;
 		}
-		/* FALLTHROUGH */
-	case VMMCI_REBOOT:
+
 		/*
 		 * If the VM acknowledged our shutdown request, give it
 		 * enough time to shutdown or reboot gracefully.  This
@@ -365,9 +365,14 @@ vmmci_ack(unsigned int cmd)
 			log_debug("%s: vm %u acknowledged shutdown request",
 			    __func__, vmmci.vm_id);
 			tv.tv_sec = VMMCI_SHUTDOWN_TIMEOUT;
+			vm_notify_vmm(1);
 			evtimer_add(&vmmci.timeout, &tv);
 		}
 		break;
+	case VMMCI_REBOOT:
+		log_debug("%s: vm %u requested reboot", __func__, vmmci.vm_id);
+		vm_notify_vmm(0);
+		break;
 	case VMMCI_SYNCRTC:
 		log_debug("%s: vm %u acknowledged RTC sync request",
 		    __func__, vmmci.vm_id);
@@ -383,7 +388,7 @@ void
 vmmci_timeout(int fd, short type, void *arg)
 {
 	log_debug("%s: vm %u shutdown", __progname, vmmci.vm_id);
-	vm_shutdown(vmmci.cmd == VMMCI_REBOOT ? VMMCI_REBOOT : VMMCI_SHUTDOWN);
+	vm_shutdown();
 }
 
 int
blob - 8b94fbc8a590072cfdc94344abe6b0fd3f28e01e
blob + 679ab17c64e2e9371d80b178e4859d77f53d5df6
--- usr.sbin/vmd/vm.c
+++ usr.sbin/vmd/vm.c
@@ -439,28 +439,25 @@ vm_dispatch_vmm(int fd, short event, void *arg)
 }
 
 /*
- * vm_shutdown
+ * vm_notify_vmm
  *
- * Tell the vmm parent process to shutdown or reboot the VM and exit.
+ * Tell the vmm parent process to shutdown or reboot the VM on process exit.
  */
-__dead void
-vm_shutdown(unsigned int cmd)
+void
+vm_notify_vmm(int shutdown)
 {
-	switch (cmd) {
-	case VMMCI_NONE:
-	case VMMCI_SHUTDOWN:
+	if (shutdown)
 		(void)imsg_compose_event(&current_vm->vm_iev,
 		    IMSG_VMDOP_VM_SHUTDOWN, 0, 0, -1, NULL, 0);
-		break;
-	case VMMCI_REBOOT:
+	else
 		(void)imsg_compose_event(&current_vm->vm_iev,
 		    IMSG_VMDOP_VM_REBOOT, 0, 0, -1, NULL, 0);
-		break;
-	default:
-		fatalx("invalid vm ctl command: %d", cmd);
-	}
 	imsg_flush(&current_vm->vm_iev.ibuf);
+}
 
+__dead void
+vm_shutdown(void)
+{
 	if (sev_shutdown(current_vm))
 		log_warnx("%s: could not shutdown SEV", __func__);
 
blob - b84656fc20c20c9a6cdfff4a1b7b420b2f472bac
blob + 8866574bc1baf1424fee826ac897df11d0f932ec
--- usr.sbin/vmd/vmd.h
+++ usr.sbin/vmd/vmd.h
@@ -547,7 +547,8 @@ void	 vm_pipe_send(struct vm_dev_pipe *, enum pipe_msg
 enum pipe_msg_type vm_pipe_recv(struct vm_dev_pipe *);
 int	 write_mem(paddr_t, const void *buf, size_t);
 int	 remap_guest_mem(struct vmd_vm *, int);
-__dead void vm_shutdown(unsigned int);
+__dead void vm_shutdown(void);
+void	 vm_notify_vmm(int);
 
 /* config.c */
 int	 config_init(struct vmd *);