bhyve: Start moving machine-dependent code into subdirectories

[markj]

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.

[markj]

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.

[andrew]

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

[markj]

In D40501#922212, @andrew wrote:

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]

usr.sbin/bhyve/amd64/task_switch.c

703–704

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

92

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?

[markj]

usr.sbin/bhyve/amd64/task_switch.c

703–704

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

92

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.

[corvink]

usr.sbin/bhyve/amd64/task_switch.c

703–704

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

92

Sound good.

[markj]

usr.sbin/bhyve/amd64/task_switch.c

703–704

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

[jhb]

usr.sbin/bhyve/Makefile

88

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

703–704

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

92

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.

[corvink]

usr.sbin/bhyve/Makefile

89

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]

usr.sbin/bhyve/Makefile

88

That seems cleaner, thanks.

usr.sbin/bhyve/amd64/task_switch.c

703–704

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

92

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

[markj]

Rebase, handle review feedback.

[markj]

Move more stuff into amd64/Makefile.inc

[markj]

Set the include path in the main makefile.

[corvink]

usr.sbin/bhyve/Makefile

102

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

usr.sbin/bhyve/amd64/Makefile.inc

2

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

[markj]

usr.sbin/bhyve/Makefile

102

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

[jhb]

usr.sbin/bhyve/amd64/Makefile.inc

2

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