From: Dave Voutila Subject: Re: fix accidental vm reboot during `halt -p` To: tech@openbsd.org Cc: Claudio Jeker Date: Fri, 29 Nov 2024 13:53:35 -0500 ping Dave Voutila writes: > 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 *);