From: Dave Voutila Subject: Re: fix accidental vm reboot during `halt -p` To: tech@openbsd.org Cc: Claudio Jeker Date: Mon, 25 Nov 2024 17:47:37 -0500 Dave Voutila writes: > 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. > Now looking for ok's. > -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 *);