bhyve: dynamically register FwCtl ports

[corvink]

Summary

Qemu's FwCfg uses the same ports as Bhyve's FwCtl. Static allocated
ports wouldn't allow to switch between Qemu's FwCfg and Bhyve's
FwCtl.

[markj]

Sorry if I missed it, but is there another diff to add alternate handlers for these I/O ports?

usr.sbin/bhyve/bhyverun.c

1541 ↗

(On Diff #100123)

Should be errx() instead of perror(), fwctl_init() doesn't set errno.

usr.sbin/bhyve/fwctl.c

557

Can we reasonably assert error == 0, instead of forcing the caller to handle it?

[corvink]

In D33496#757249, @markj wrote:

Sorry if I missed it, but is there another diff to add alternate handlers for these I/O ports?

Sry, I should have linked it. Qemu's FwCfg uses the same I/O ports. I already created a patch to add support for Qemu's FwCfg D31578.

usr.sbin/bhyve/fwctl.c

557

I don't like assert because it could be removed in release builds. If it's removed, fwctl_init will silently fail and users may wonder why it doesn't work.
If you prefer an assert, I'll add it.

[markj]

In D33496#757273, @c.koehne_beckhoff.com wrote:

In D33496#757249, @markj wrote:

Sorry if I missed it, but is there another diff to add alternate handlers for these I/O ports?

Sry, I should have linked it. Qemu's FwCfg uses the same I/O ports. I already created a patch to add support for Qemu's FwCfg D31578.

Thanks.

usr.sbin/bhyve/fwctl.c

557

For better or worse, bhyve(8) already uses assert() all over the place to catch invalid states and fail closed. assert() is enabled even in release builds of FreeBSD (one has to set WITHOUT_ASSERT_DEBUG= in src.conf to turn them off).

I suspect we should switch to some mechanism internal to bhyve that ensures that assertions are never compiled out. It's much better to have bhyve die than to run in an undefined state, in my opinion.

So, I somewhat prefer an assert here just because I think the error should only occur due to a code bug, and because most other register_inout() callers handle it that way, but I don't insist.

[corvink]

• use asserts for checking the return code of register_inout

[corvink]

usr.sbin/bhyve/fwctl.c

557

Yeah, I know that asserts are used at many places and they aren't removed on release build. Therefore, I'm fine with using an assert.

I totally agree with you that bhyve should die in invalid states. However, asserts could potentially be removed. So, they aren't a safe way to kill bhyve. Adding more of them will make it harder to remove them :) But that might be out of the scope of this review.