vmm: Add ability to destroy VMs on close
AcceptedPublic
Actions

Authored by bnovkov on Thu, Nov 13, 11:13 AM.

Details

Reviewers

markj

Group Reviewers

bhyve

Summary

This change adds the ability to tie a virtual machine's lifecycle to
a /dev/vmmctl file descriptor. A user can request vmmctl to destroy a
virtual machine on close using the VMMCTL_CREATE_DESTROY_ON_CLOSE flag
when creating the virtual machine. vmmctl tracks such virtual machines
in per-descriptor lists.

Sponsored by: The FreeBSD Foundation
Sponsored by: Klara, Inc.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 68595
Build 65478: arc lint + arc unit

Event Timeline

bnovkov created this revision.Thu, Nov 13, 11:13 AM

Herald added subscribers: rgrimes, imp. · View Herald TranscriptThu, Nov 13, 11:13 AM

bnovkov requested review of this revision.Thu, Nov 13, 11:13 AM

bnovkov added a parent revision: D53728: kern: Introduce RLIMIT_VMM.

Harbormaster completed remote builds in B68595: Diff 166351.Thu, Nov 13, 11:13 AM

bnovkov added a child revision: D53730: bhyve: Eliminate exit status magic numbers.Thu, Nov 13, 11:13 AM

Rebase.

Looks good overall.

sys/dev/vmm/vmm_dev.c
1013	Let's please make it an error to specify undefined flags.
1108	This is the case where we're racing with `bhyvectl --destroy`, right? Shouldn't we immediately proceed to the next VM in the list in this case?
1114	Can vm_suspend() fail here? I think we don't care about errors there so please add a void cast.

Address @markj 's comments.

sys/dev/vmm/vmm_dev.c
1108	We could move on to the next element, but after giving this some though I think it would unnecessarily complicate things. Please correct me if I'm wrong, but IMO there are two problems at play here: We can't defer VM removal (i.e., move to the softc to a stack-based list) without marking `sc->cdev` with NULL, which forces us to immediately destroy the VM since we'd lose the `cdev` pointer otherwise, Safe forward traversal of the list using the current element isn't really possible since we drop the lock to allow `vmmdev_destroy` to run. Other `bhyvectl` commands might invalidate a previously cached pointer to the next element. We would also need to wait for pending `bhyvectl` commands by spinning on `!LIST_EMPTY(priv->softcs)` before freeing the per-descriptor context.
1114	Done, thanks. I've added a void cast to a similar spot in `vmmdev_lookup_and_destroy` as well.

markj added inline comments.Thu, Nov 20, 2:51 PM

sys/dev/vmm/vmm_dev.c
1108	The problem with the current approach is that if bhyvectl is concurrently destroying the first VM on the list, then we're just going to busy-loop here until it's done, which is not really nice. You're right that we can't free the per-descriptor context until all VMs have been torn down. Maybe this instead: if `sc->cdev == NULL`, sx_sleep() on vmmdev_mtx until vmmdev_destroy() finishes, i.e., vmmdev_destroy() should issue a wakeup after finishing its teardown.