Page MenuHomeFreeBSD

bhyve: Move vcpu initialization into a MD source file
ClosedPublic

Authored by markj on Jul 12 2023, 1:51 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 22 2023, 11:58 PM
Unknown Object (File)
Dec 11 2023, 4:35 AM
Unknown Object (File)
Dec 9 2023, 2:31 PM
Unknown Object (File)
Nov 21 2023, 3:32 AM
Unknown Object (File)
Nov 20 2023, 9:41 PM
Unknown Object (File)
Nov 20 2023, 9:21 PM
Unknown Object (File)
Nov 20 2023, 9:20 PM
Unknown Object (File)
Nov 12 2023, 8:35 AM
Subscribers

Details

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 requested review of this revision.Jul 12 2023, 1:51 PM

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?

This revision is now accepted and ready to land.Jul 13 2023, 5:41 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 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.

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?