Page MenuHomeFreeBSD

bhyve: Start moving machine-dependent code into subdirectories
ClosedPublic

Authored by markj on Jun 12 2023, 8:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 11:52 PM
Unknown Object (File)
Fri, Nov 22, 4:17 PM
Unknown Object (File)
Thu, Nov 7, 3:49 PM
Unknown Object (File)
Thu, Nov 7, 1:06 PM
Unknown Object (File)
Sun, Nov 3, 12:55 PM
Unknown Object (File)
Oct 21 2024, 5:11 PM
Unknown Object (File)
Oct 3 2024, 11:48 PM
Unknown Object (File)
Oct 3 2024, 8:33 PM

Details

Summary

In preparation for an arm64 port, make an easy change which puts some
machine-dependent code in its own directory.

Going forward, code which is only used on one platform should live in a
MD directory. We should strive to layer modules in such a way as to
avoid polluting shared code with lots of ifdefs. For some existing
files this will take some effort.

task_switch.c and fwctl.c are an easy place to start: the former is very
x86-specific, and the latter provides an I/O port interface which can't
be used on anything other than x86. (fwcfg as implemented has the same
problem, but QEMU also supports a MMIO fwcfg interface.) So I propose
that we start by simply making those files conditional.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 52012
Build 48903: arc lint + arc unit

Event Timeline

markj requested review of this revision.Jun 12 2023, 8:11 PM

bhyverun.c probably needs to be split up. It implements main() and VMEXIT handlers, both of which are quite machine dependent. Even many command-line flags are machine-dependent. I suspect that the cleanest thing to do for arm64 will be to move the existing bhyverun.c into amd64/, and factor out as much duplicated code (e.g., memory setup, vcpu setup) as possible.

e820 is also amd64 specific & should be easy to split out.

e820 is also amd64 specific & should be easy to split out.

Yes, I'm looking at that one as well. I'll upload it as a separate patch, though, since I'd like to also move the etc/e820 code in bhyverun.c to e820.c.

corvink added inline comments.
usr.sbin/bhyve/amd64/task_switch.c
705–706

This function is defined right below of this line. Additionally, this declaration is only visible in this file. So, it makes no sense to declare it here.

usr.sbin/bhyve/bhyverun.c
96

Doesn't work for fwctl as it won't be implemented by arm64:

Is it possible to compile bhyve in a way that we can include such files as machine/fwctl.h to avoid an ifdef?

This revision is now accepted and ready to land.Jun 13 2023, 6:06 AM
usr.sbin/bhyve/amd64/task_switch.c
705–706

Our build flags require all externally visible function definitions to be preceded by a declaration (-W-missing-prototypes).

I removed the declaration from bhyverun.h, so both .c files which refer to vmexit_task_switch() need a declaration. I think bhyverun.h is probably not the right place, but I'm also not sure yet how to deal with vmexit handlers: amd64 and arm64 have mostly different VM_EXITCODE_* definitions. So, I just put the declaration here for now.

Maybe the better approach is to replace the handler table with a linker set. That is, for each VM_EXITCODE_* handler, write:

static int
vmexit_task_switch(...)
{
}
VM_EXIT_HANDLER_DEFINE(VM_EXITCODE_TASK_SWITCH, vmexit_task_switch);

Then bhyve can build a lookup table before entering the main loop and use that instead of handler. What do you think?

usr.sbin/bhyve/bhyverun.c
96

It should be possible to create a symlink at build time, sure.

My suggestion in the review comments is to have separate bhyverun.c files for amd64 and arm64. In this scheme, the ifdef will be removed, so it is here only temporarily. In general I think that machine-dependent header files should only be included by machine-dependent .c files, so hopefully a machine symlink won't be necessary.

usr.sbin/bhyve/amd64/task_switch.c
705–706

What about:

 --- bhyverun.h
extern vmexit_handler_t *handler;

int vmexit_machine_independent(struct vmexit *);
 --- bhyverun.c
vm_loop()
{
  vmexit = vm_run();
  if (vmexit->code < VM_EXITCODE_MAX && handler[vmexit->code] != NULL)
    handler[vmexit->code](vmexit);
}
 --- amd64/vmexit.c
static int
vmexit_machine_dependent(struct vmexit *vme)
{
  ...
}

struct vmexit_handler_t handler[VM_EXITCODE_MAX] = {
  [VM_EXITCODE_MACHINE_INDEPENDENT] = vmexit_machine_independent,
  [VM_EXITCODE_MACHINE_DEPENDENT] = vmexit_machine_dependent,
};
 --- arm64/vmexit.c
struct vmexit_handler_t handler[VM_EXITCODE_MAX] = ...;
usr.sbin/bhyve/bhyverun.c
96

Sound good.

usr.sbin/bhyve/amd64/task_switch.c
705–706

This makes sense, I'll give it a try.

usr.sbin/bhyve/Makefile
89

I think you might want to consider amd64/Makefile.inc instead which matches what we do in libc, etc. Just need to .include ${MACHINE_CPUARCH}/Makefile.inc and move the amd64-specific bits there?

usr.sbin/bhyve/amd64/task_switch.c
705–706

I like this structure, though in the commit that adds that I think the prototype for this function is still duplicated? Long term I think you want an amd64/internal.h or some such that amd64/vmexit.c would be able to include that would have this prototype? (Maybe call it amd64/bhyve_md.h or some such to match some other places in the tree like libthr?)

usr.sbin/bhyve/bhyverun.c
96

The other long term strategy is to add CURDIR/MACHINE_CPUARCH to the -I path in CFLAGS if we find we need it, but I think having the #ifdef temporarily is fine.

usr.sbin/bhyve/Makefile
90

Nit and only a suggestion: The diff in your next commit is a bit more readable by using

SCRS+= \
        fwctl.c \
        task_switch.c
markj marked 6 inline comments as done.
markj added inline comments.
usr.sbin/bhyve/Makefile
89

That seems cleaner, thanks.

usr.sbin/bhyve/amd64/task_switch.c
705–706

Yes, I haven't yet put the prototype into a header, mostly since I don't know what else might need to go in there yet. bhyve_md.h might be reasonable, yeah.

usr.sbin/bhyve/bhyverun.c
96

Yeah, I think I'd prefer to use explicit ifdefs as breadcrumbs for some later reshuffling and reorganization.

markj marked 2 inline comments as done.

Rebase, handle review feedback.

This revision now requires review to proceed.Jun 15 2023, 10:13 PM

Move more stuff into amd64/Makefile.inc

Set the include path in the main makefile.

corvink added inline comments.
usr.sbin/bhyve/Makefile
105

Looks like machine dependent code. Shouldn't it be moved to amd64/Makefile.inc as well?

usr.sbin/bhyve/amd64/Makefile.inc
1 ↗(On Diff #123311)

Suggestion: Instead of repeating this line in every md Makefile, I'd add it to the main Makefile.

This revision is now accepted and ready to land.Jun 16 2023, 5:30 AM
markj added inline comments.
usr.sbin/bhyve/Makefile
105

Yes, this happens in D40554, I just failed to update the makefiles in that patch.

jhb added inline comments.
usr.sbin/bhyve/amd64/Makefile.inc
1 ↗(On Diff #123311)

I concur, btw, as you will end up with some files that are defined MD (vmexit.c) but in the MI list I think?