bhyve: Move vcpu initialization into a MD source file
ClosedPublic
Actions

Authored by markj on Jul 12 2023, 1:51 PM.

Details

Reviewers

jhb
corvink
andrew

Group Reviewers

bhyve

Commits

rGcd60f1af560a: bhyve: Move vcpu initialization into a MD source file
rGe20b74da223d: bhyve: Move vcpu initialization into a MD source file

Summary

Make handling of x86 config options, like x86.x2apic, conditional to amd64.
Move fbsdrun_set_capabilities() and spinup_vcpu() to a new file, bhyverun_machdep.c. The moved code is all highly x86 specific.

I'm not sure how best to handle the namespace. I'm using "bhyve_" for
MD functions called from MI code. We also have "fbsdrun_" for some MI
routines that are typically called from MD code. The file name is
prefixed by "bhyverun_". I'm happy to change this if there's a
suggestion for something better.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Jul 12 2023, 1:51 PM

Herald added a reviewer: bhyve. · View Herald TranscriptJul 12 2023, 1:51 PM

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

markj requested review of this revision.Jul 12 2023, 1:51 PM

Harbormaster completed remote builds in B52580: Diff 124549.Jul 12 2023, 1:51 PM

markj added a parent revision: D40986: bhyve: Make gdb support optional.Jul 12 2023, 1:51 PM

markj added a child revision: D40988: bhyve: Remove an unneeded vm_get_register() call in main().

fbsdrun is certainly a bit of a legacy name I'd be happy to move away from. I would even like to maybe 's/fbsdrun_/bhyve_/' at some point. I think the current naming is fine. One other possible suggestion is to use 'cpu_*' as the prefix for MD routines (this is what we do in the kernel for many APIs, though certainly not all).

usr.sbin/bhyve/bhyverun.c
480	Maybe s/vcpu/vcpuid/ for this function? I've tried to use `vcpuid` for IDs and `vcpu` for `struct vcpu *` in more recent changes.
817	While set_defaults certainly seems MD, the call to `init_config` seems MI?

corvink accepted this revision.Jul 13 2023, 5:41 AM

This revision is now accepted and ready to land.Jul 13 2023, 5:41 AM

corvink added inline comments.Jul 13 2023, 5:47 AM

usr.sbin/bhyve/bhyverun.c
480	I do agree with Johns suggestion. It makes things clearer. Additionally, some parts of the commit (e.g. this input type change) could be done in a separate commit. Nevertheless, I don't have a strong preference on that.

markj marked an inline comment as done.Jul 17 2023, 3:31 PM

markj added inline comments.

usr.sbin/bhyve/bhyverun.c
480	My main goal in splitting up the patches is to keep them reviewable, not necessarily to produce minimal diffs. If it would really help for me to split up some of these reviews further, please feel free to ask, but otherwise I'll avoid it, since it's more work and creates more opportunities to introduce bugs.
480	I was matching fbsdrun_deletecpu() below, but I'll change it here.
817	It just seemed a bit cleaner to have all config initialization done with one function call from main(), even though that creates a bit of redundancy. In particular, init_config() needs to be called before bhyve_init_config(). If you prefer to keep the init_config() call here, I can move it back.

jhb added inline comments.Jul 17 2023, 5:49 PM

usr.sbin/bhyve/bhyverun.c
817	I do think in this case it would be best to keep `init_config` just before `bhyve_init_config` in main. You could perhaps even rename `bhyve_init_config` to `bhyve_set_defaults` in that case if you wanted?