bhyve: Support other schemes for naming pass-through devices.
ClosedPublic
Actions

Authored by jhb on Aug 11 2022, 11:32 PM.

Details

Reviewers

emaste
markj
imp

Group Reviewers

manpages
bhyve

Commits

rGe5217bedc964: bhyve: Support other schemes for naming pass-through devices.
rGbaf753cc196e: bhyve: Support other schemes for naming pass-through devices.

Summary

Permit naming pass through devices using the syntax accepted by
pciconf (pci[<domain>:]<bus>:<slot>:<func>) as well as by device name
(e.g. "ppt0").

While here, fix an error in the manpage that had the bus and slot
arguments for the original /-delimited scheme swapped.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 47006
Build 43895: arc lint + arc unit

Event Timeline

jhb created this revision.Aug 11 2022, 11:32 PM

Herald added a reviewer: manpages. · View Herald TranscriptAug 11 2022, 11:32 PM

Herald added a reviewer: bhyve. · View Herald Transcript

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

jhb requested review of this revision.Aug 11 2022, 11:32 PM

Harbormaster completed remote builds in B46870: Diff 109208.Aug 11 2022, 11:32 PM

jhb added a parent revision: D36146: vmm: Rework mapping of PCI passthrough bars into guests..Aug 11 2022, 11:32 PM

Modulo a few nits noted, this looks good to my eye.

usr.sbin/bhyve/pci_passthru.c
125–126	Unrelated change, but it's good and I'm not sure it's worth a separate commit
155–156	unrelated, but correct, etc
175–176	unrelated, but correct, etc
826–831	I'd be tempted to either indent this, or put it before the if. Having it in the #if is a bit weird.

This revision is now accepted and ready to land.Aug 11 2022, 11:37 PM

OK from manpages.

jhb added inline comments.Aug 12 2022, 5:18 PM

usr.sbin/bhyve/pci_passthru.c
826–831	It's only used in the else, but yeah, I could move it up.

markj added inline comments.Aug 15 2022, 4:48 PM

usr.sbin/bhyve/pci_passthru.c
674	Why add whitespace before the function number?
685
760	With this decrement, the first iteration of the following loop will check the second-last character, not the last character. Is that intentional?
761	Can this be UB? With NDEBUG defined the compiler has no guarantee that `cp` is a valid pointer.

jhb marked 5 inline comments as done.Aug 16 2022, 7:27 PM

jhb added inline comments.

usr.sbin/bhyve/pci_passthru.c
155–156	I've moved these out to a separate commit.
674	A stray tab. :)
760	Hmm, this is just copied from pciconf (which is also code I wrote). However, I do think it's true we do want to check that the last character is a valid digit. I think a string like "ppt0a" still ends up getting caught when strtol() below returns with `*cp` not equal to `'\0'`. Probably I did the `cp--` originally to avoid checking the trailing nul. However, it's probably safer to drop the decrement, and I think it will work fine to reject values like an empty string (though the caller will never pass an empty string).
761	I don't think it can be given the constraint that `name` must be a valid C string? The one edge case currently that doesn't work is if `name` is an empty string and dropping the `cp--` should handle that.

Various review fixes.

This revision now requires review to proceed.Aug 16 2022, 7:37 PM

Harbormaster completed remote builds in B46976: Diff 109415.Aug 16 2022, 7:37 PM

markj accepted this revision as: markj.Aug 17 2022, 1:48 PM

markj added inline comments.

usr.sbin/bhyve/bhyve_config.5
580	Isn't it the driver that attaches to the device, not the other way around?
usr.sbin/bhyve/pci_passthru.c
761	In theory `name` could be NULL, assertion notwithstanding. I'm not sure I'd trust the compiler to not do something silly, but as you say, pciconf is doing the same thing already.

This revision is now accepted and ready to land.Aug 17 2022, 1:48 PM

jhb added inline comments.Aug 17 2022, 4:53 PM

usr.sbin/bhyve/bhyve_config.5
580	Yeah, I guess it would be "must be attached to by the ppt(4) device driver" but that's kind of a mouthful. In a way, you can view the "attach" relationship as bi-directional? Once the driver claims a device, the device is then attached to that driver? Could perhaps say "device .. must be owned/managed by ppt(4)", but "attached" seems to be the most accurate jargon in FreeBSD terms.

Move the macro out of the else.

This revision now requires review to proceed.Aug 17 2022, 6:36 PM

Harbormaster completed remote builds in B47006: Diff 109479.Aug 17 2022, 6:36 PM

jhb marked 2 inline comments as done.Aug 17 2022, 6:37 PM

markj accepted this revision as: markj.Aug 18 2022, 1:41 PM

markj added inline comments.

usr.sbin/bhyve/bhyve_config.5
580	I might write `The ppt(4) driver must be attached to the PCI device being passed through.`, but that also sounds a bit awkward. It's clear enough anyway.