bhyve: dynamically register FwCtl ports
ClosedPublic
Actions

Authored by corvink on Dec 16 2021, 8:26 AM.

Details

Reviewers

markj
jhb
manu

Group Reviewers

bhyve

Commits

rGff1e10c8f626: bhyve: dynamically register FwCtl ports
rG9fe79f2f2b22: bhyve: dynamically register FwCtl ports

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

corvink created this revision.Dec 16 2021, 8:26 AM

Herald added subscribers: bcran, rgrimes, imp. · View Herald TranscriptDec 16 2021, 8:26 AM

corvink requested review of this revision.Dec 16 2021, 8:26 AM

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
555	Can we reasonably assert `error == 0`, instead of forcing the caller to handle it?

This revision is now accepted and ready to land.Dec 16 2021, 4:17 PM

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
555	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.

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
555	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.

use asserts for checking the return code of register_inout

This revision now requires review to proceed.Dec 17 2021, 7:50 AM

corvink added inline comments.Dec 17 2021, 7:50 AM

usr.sbin/bhyve/fwctl.c
555	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.