Page MenuHomeFreeBSD

vmm: Merge vmm_dev.c
ClosedPublic

Authored by markj on Aug 24 2024, 4:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 11, 8:44 AM
Unknown Object (File)
Mon, Nov 11, 3:25 AM
Unknown Object (File)
Thu, Oct 17, 6:01 AM
Unknown Object (File)
Oct 15 2024, 6:03 PM
Unknown Object (File)
Oct 15 2024, 6:03 PM
Unknown Object (File)
Oct 14 2024, 9:32 AM
Unknown Object (File)
Oct 11 2024, 1:27 PM
Unknown Object (File)
Oct 10 2024, 4:34 PM

Details

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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

Add some missing amd64 ioctl table entries.

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 marked 2 inline comments as done.

Split a few small changes into their own patches.

This revision now requires review to proceed.Aug 26 2024, 1:41 PM
jhb added inline comments.
sys/amd64/vmm/vmm_dev_machdep.c
70

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
31
39

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.

This revision is now accepted and ready to land.Aug 26 2024, 4:26 PM
sys/amd64/vmm/vmm_dev_machdep.c
70

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
31

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.

This revision was automatically updated to reflect the committed changes.
markj marked 3 inline comments as done.