vmm: Merge vmm_dev.c
ClosedPublic
Actions

Authored by markj on Aug 24 2024, 4:59 PM.

Details

Reviewers

andrew
manu
corvink
jhb

Group Reviewers

bhyve

Commits

rGb9ef152bec6c: vmm: Merge vmm_dev.c

Summary

This file contains the vmm device file implementation. Most of this
code is not machine-dependent and so shouldn't be duplicated this way.
Move most of it into a generic dev/vmm/vmm_dev.c. This will make it
easier to introduce a cdev-based interface for VM creation, which in
turn makes it possible to implement support for running bhyve as an
unprivileged user.

Machine-dependent ioctls continue to be handled in machine-dependent
code. To make the split a bit easier to handle, introduce a pair of
tables which define MI and MD ioctls. Each table entry can set flags
which determine which locks need to be held in order to execute the
handler. vmmdev_ioctl() now looks up the ioctl in one of the tables,
acquires locks and either handles the ioctl directly or calls
vmmdev_machdep_ioctl() to handle it.

No functional change intended. There is a lot of churn in this change
but the underlying logic in the ioctl handlers is the same. For now,
vmm_dev.h is still mostly separate, even though some parts could be
merged in principle. This would involve changing include paths for
userspace, though.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 59180
Build 56067: arc lint + arc unit

Event Timeline

markj created this revision.Aug 24 2024, 4:59 PM

Herald added a reviewer: andrew. · View Herald TranscriptAug 24 2024, 4:59 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: bcran, rgrimes, emaste, imp. · View Herald Transcript

markj requested review of this revision.Aug 24 2024, 4:59 PM

Harbormaster completed remote builds in B59170: Diff 142408.Aug 24 2024, 4:59 PM

markj added a parent revision: D46430: vmm: Move duplicated stats code into a generic file.Aug 24 2024, 4:59 PM

markj added a child revision: D46432: vmm: Move compat ioctl definitions to vmm_dev.c.

Add some missing amd64 ioctl table entries.

Harbormaster completed remote builds in B59176: Diff 142421.Aug 25 2024, 4:41 PM

corvink accepted this revision.Aug 26 2024, 6:57 AM

corvink added a subscriber: corvink.

corvink added inline comments.

sys/dev/vmm/vmm_dev.c
174–175	How is this change related to your patch? Wouldn't it be better to split this into it's own patch and explain, why we can safely remove it?
727	Looks like a bug which should get it's own patch and explanation.

This revision is now accepted and ready to land.Aug 26 2024, 6:57 AM

markj added a parent revision: D46439: arm64: Add a missing vm_disable_vcpu_creation() call in vmmdev_destroy().Aug 26 2024, 1:39 PM

markj added a parent revision: D46438: vmm: Enable assertions in vmmdev_lookup().

Split a few small changes into their own patches.

This revision now requires review to proceed.Aug 26 2024, 1:41 PM

Harbormaster completed remote builds in B59180: Diff 142430.Aug 26 2024, 1:41 PM

jhb added a subscriber: jhb.Aug 26 2024, 4:25 PM

jhb added inline comments.

sys/amd64/vmm/vmm_dev_machdep.c
69	An old bug (so probably worth fixing in a separate commit), but I'd prefer if we were more consistent with naming these (and not sure which of these is my fault). So far we have _FBSD12 suffixes for old ioctls for 12.x, at least one _13 suffix as well as a couple of _OLD suffixes for 13.x ioctls. I'd definitely prefer having the major number instead of _OLD. I'm not sure I have preference between _FBSDnn vs _nn. Possibly the latter.
sys/dev/vmm/vmm_dev.c
328	Can this not live in the amd64 machdep.c file or is it too gross to do so? I guess because alloc_memseg() lives in this file it's a bit too gross.
sys/dev/vmm/vmm_dev.h
30
38	This one at least warrants a comment I think, that the vcpuid can either be -1 or a specific vcpu. Might be easier to explain though with a larger comment. I've taken a stab above.

jhb accepted this revision.Aug 26 2024, 4:26 PM

This revision is now accepted and ready to land.Aug 26 2024, 4:26 PM

markj added inline comments.Aug 26 2024, 4:37 PM

sys/amd64/vmm/vmm_dev_machdep.c
69	I will submit a follow-up revision with that change.
sys/dev/vmm/vmm_dev.c
328	Yeah, I had to pick between two evils and this one felt a bit less evil.
sys/dev/vmm/vmm_dev.h
30	Thanks, I'll extend this a bit and include it in the patch. In fact, ioctls do not have to specify any of those flags, though in some cases it's not clear to me that that's intentional. For instance, `VM_GET_TOPOLOGY` and `VM_SET_TOPOLOGY` operate with no locking, which seems mostly harmless but still incorrect.