Page MenuHomeFreeBSD

virtio(4): Add PNP match metadata for virtio devices
ClosedPublic

Authored by cem on May 25 2019, 4:48 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 5:37 AM
Unknown Object (File)
Wed, Dec 4, 11:25 AM
Unknown Object (File)
Tue, Dec 3, 11:49 AM
Unknown Object (File)
Oct 4 2024, 12:31 PM
Unknown Object (File)
Oct 3 2024, 9:22 PM
Unknown Object (File)
Oct 3 2024, 1:36 AM
Unknown Object (File)
Oct 2 2024, 11:48 PM
Unknown Object (File)
Oct 2 2024, 1:19 PM
Subscribers
None

Details

Summary

Register MODULE_PNP_INFO for virtio devices using the newbus PNP information
provided by the previous commit. Matching can be quite simple; existing
probe routines only matched on bus (implicit) and device_type. The same
matching criteria are retained exactly, but is now also available to
devmatch(8).

Test Plan
<boot>
...
Starting devd.
Autoloading module: virtio_random.ko
vtrnd0: <VirtIO Entropy Adapter> on virtio_pci0

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24498
Build 23297: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.May 25 2019, 3:19 PM
sys/dev/virtio/block/virtio_blk.c
256

This forces the use of modevent_nop(), which returns EBUSY on unload. Does that not prevent the corresponding KLDs from being unloaded?

sys/dev/virtio/block/virtio_blk.c
256

Ah, it does. I did not realize that the default prevented MOD_UNLOAD. I would have assumed a NULL event handler would be treated the same as these implementations, which did nothing.

I guess if we want to retain the NULL => block unload behavior (do we?), I'd rename modevent_nop => modevent_no_unload, add a true modevent_nop with extern visibility, and reference it in these consumers instead of NULL. But that's outside the scope of this change.

For now I'll revert the modevent portion of these changes. Thanks for pointing this out.

sys/dev/virtio/block/virtio_blk.c
256

There are many uses of DRIVER_MODULE with a null evh, so I'm wary of changing the existing behaviour. I note that that behaviour doesn't appear to be documented anywhere though. Your proposal sounds reasonable to me.

sys/dev/virtio/balloon/virtio_balloon.c
163

Sorry, I'm not very familiar with the PNP_INFO stuff: in the near future, the virtio_pci module will contain two drivers: VirtIO legacy and VirtIO modern (V1). Is this able to support that?

sys/dev/virtio/balloon/virtio_balloon.c
163

The virtio_pci here refers to the bus name, rather than a driver name. Any driver that exposes a virtio_pci bus with the right PNP data will match. I don't know of a reason why it wouldn't work off hand. (But I have not looked closely at how virtio modern and legacy coexist.)

If the virtio modern bus has a different name (e.g., "virtio_pci2"), we would just add a 2nd line:

VIRTIO_SIMPLE_PNPINFO(virtio_pci2, virtio_balloon);

(Much like we have two lines for the drivers that attach to the virtio_mmio bus as well as the virtio_pci bus.)

cem marked 2 inline comments as done.
  • Drop modevent handler changes.
This revision now requires review to proceed.May 27 2019, 1:44 AM
This revision is now accepted and ready to land.May 27 2019, 8:30 PM
sys/dev/virtio/balloon/virtio_balloon.c
168

style(9) wants to keep the blank line at the start of the function FWIW.

sys/dev/virtio/block/virtio_blk.c
256

Huh? This is confusing. DRIVER_MODULE always uses 'driver_module_handler' as the actual module handler. 'vtblk_modevent' is stored in the 'dmd_chainevh' function pointer, and it being NULL does not prevent unloading.

261–262

This can definitely die, but I would perhaps do that as a separate change.

sys/dev/virtio/block/virtio_blk.c
256

I see, my bad. I misread EARLY_DRIVER_MODULE_ORDERED's definition.

sys/dev/virtio/balloon/virtio_balloon.c
168

I am under the impression that general consensus was that lack of blank line for 1-2 liners without locals is broadly ok. (And AFAIK, people aren't especially attached to it for longer functions, either, but it's unusual for a longer function not to require any local variables.)

style.9 does say "New core kernel code should be reasonably compliant…", if that helps :-).

I guess I could propose a style.9 amendment (separately), to codify that assumption.

sys/dev/virtio/block/virtio_blk.c
256

Oh, that's confusing. Maybe D20420 can just be dropped.

261–262

Ah, ok. I can do another separate differential for nixing it from the virtio driver set.

sys/dev/virtio/balloon/virtio_balloon.c
168

I still follow it myself FWIW.

cem marked 2 inline comments as done.May 28 2019, 5:41 PM
cem added inline comments.
sys/dev/virtio/balloon/virtio_balloon.c
168

I proposed making it optional, rather than mandatory include or exclude, in limited cases: D20448 . (Leaving room for personal taste, such as use of {} when not strictly required.)

@bryanv, do you have any remaining concerns about this one or could it go in?

This revision was automatically updated to reflect the committed changes.