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

[cem]

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

[markj]

sys/dev/virtio/block/virtio_blk.c

256 ↗

(On Diff #57882)

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

[cem]

sys/dev/virtio/block/virtio_blk.c

256 ↗

(On Diff #57882)

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.

[markj]

sys/dev/virtio/block/virtio_blk.c

256 ↗

(On Diff #57882)

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.

[bryanv]

sys/dev/virtio/balloon/virtio_balloon.c

163 ↗

(On Diff #57882)

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?

[cem]

sys/dev/virtio/balloon/virtio_balloon.c

163 ↗

(On Diff #57882)

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]

• Drop modevent handler changes.

[jhb]

sys/dev/virtio/balloon/virtio_balloon.c

168 ↗

(On Diff #57931)

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

sys/dev/virtio/block/virtio_blk.c

269 ↗

(On Diff #57931)

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

256 ↗

(On Diff #57882)

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.

[markj]

sys/dev/virtio/block/virtio_blk.c

256 ↗

(On Diff #57882)

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

[cem]

sys/dev/virtio/balloon/virtio_balloon.c

168 ↗

(On Diff #57931)

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

269 ↗

(On Diff #57931)

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

256 ↗

(On Diff #57882)

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

[jhb]

sys/dev/virtio/balloon/virtio_balloon.c

168 ↗

(On Diff #57931)

I still follow it myself FWIW.

[cem]

sys/dev/virtio/balloon/virtio_balloon.c

168 ↗

(On Diff #57931)

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

[cem]

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

[bryanv]

@cem LGTM