From: Dave Voutila Subject: fix accidental vm reboot during `halt -p` To: tech@openbsd.org Cc: Claudio Jeker Date: Wed, 20 Nov 2024 17:28:01 -0500 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 #include #include +#include #include @@ -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(¤t_vm->vm_iev, IMSG_VMDOP_VM_SHUTDOWN, 0, 0, -1, NULL, 0); - break; - case VMMCI_REBOOT: + else (void)imsg_compose_event(¤t_vm->vm_iev, IMSG_VMDOP_VM_REBOOT, 0, 0, -1, NULL, 0); - break; - default: - fatalx("invalid vm ctl command: %d", cmd); - } imsg_flush(¤t_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 *);