virtio(4): Expose PNP metadata through newbus

[cem]

Summary

Expose the same fields and widths from both vtio buses, even though they
don't quite line up; several virtio drivers can attach to both buses,
and sharing a PNP info table for both seems more convenient.

In practice, I doubt any virtio driver really needs to match on anything
other than bus and device_type (eliminating the unused entries for
vtmmio), and also in practice device_type is << 2^16 (so far, values
range from 1 to 20). So it might be fine to only expose a 16-bit
device_type for PNP purposes. On the other hand, I don't see much harm
in overkill here.

Test Plan

$ devinfo -v
...
        virtio_pci1 pnpinfo vendor=0x1af4 device=0x1000 subvendor=0x1af4 subdevice=0x0001 class=0x020000 at slot=3 function=0 dbsf=pci0:0:3:0
          vtnet0 pnpinfo vendor=0x00001af4 device=0x1000 subvendor=0x1af4 device_type=0x00000001
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[markj]

Looks ok to me, but I don't understand the tradeoffs involved in sharing the pnpinfo format among multiple bus types. Is it theoretically possible for there to be a collision, e.g., where two different drivers, one for mmio and one for pci, match on the same string?

sys/dev/virtio/mmio/virtio_mmio.c

6 ↗

(On Diff #57881)

If you didn't have a specific reason to add it, I think the general consensus after many long threads on the topic is that we don't need to add "All rights reserved." anymore.

[cem]

In D20406#440587, @markj wrote:

Looks ok to me, but I don't understand the tradeoffs involved in sharing the pnpinfo format among multiple bus types. Is it theoretically possible for there to be a collision, e.g., where two different drivers, one for mmio and one for pci, match on the same string?

Thanks for taking a look!

No, I don't think a collision is any more possible than with two conventional PCI drivers colliding on the same metadata. Matching is bus-specific; a vtpci driver cannot match to a vtmmio bus device regardless of driver match criteria or PNP string exposed by the bus.

sys/dev/virtio/mmio/virtio_mmio.c

6 ↗

(On Diff #57881)

I have a specific reason to add it.

[bryanv]

sys/dev/virtio/mmio/virtio_mmio.c

335 ↗

(On Diff #57881)

Can have just the vendor and VirtIO device type sufficient here or does have to confirm some devd expected format? I think those two fields should be enough to identify what VirtIO device driver to load regardless of the transport.

FWIW in the future subdevice may be used: https://github.com/bryanv/freebsd/blob/virtio/sys/dev/virtio/pci/virtio_pci_modern.c#L264

339 ↗

(On Diff #57881)

I'd prefer to not have the format string duplicated with a comment saying they should be the same when I think we can make it obvious w/o comments and enforce it in code. Can we have a virtio_child_pnpinfo_str(dev, buf, buflen) (or whatever) in virtio/virtio.c that uses the IVAR accessors (virtio_get_{vendor,device,subvendor,subdevice}).

I think MMIO will need this change: https://github.com/bryanv/freebsd/commit/8e7ccd2ed735cbc6accd3d7247e200a840eb3745

[cem]

sys/dev/virtio/mmio/virtio_mmio.c

335 ↗

(On Diff #57881)

For our current set of drivers, those would be sufficient. I don't know if we expect them to stay sufficient.

I'd rather err on the side of export everything we have — for example, most PCI drivers don't need subvendor/subdevice/class for PNP, but the PCI bus exports all of those fields anyway. The cost of unused fields is fairly low.

The linked subdevice is on the opposite side of the interface, pci bus <-> vtpci(modern); this PNP data is for the vtpci bus <-> virtio_foo device. The use linked seems like exactly the same redundancy as with this version of the vtpci driver.

339 ↗

(On Diff #57881)

Sure, that seems like a good idea to me.

Currently, that would truncate the 4-byte VIRTIO_MMIO_VENDOR_ID and DEVICE_ID to the uint16_t return type of the virtio_get_{vendor,device}() ivar getters. Is that ok, or should the ivar accessors be expanded to uint32_t?

[cem]

• Extract child_pnp_info routine to virtio.c using generic ivar routines.
• Add missing IVAR accessors to virtio_mmio(4).

[bryanv]

sys/dev/virtio/mmio/virtio_mmio.c

340 ↗

(On Diff #57930)

Seems like the second sentence comment could easy become stale: these are used in the SCSI driver. The person that added MMIO didn't add it as a child bus but the VirtIO device drivers are transport agnostic.

339 ↗

(On Diff #57881)

Casting should be fine although the VirtIO ivar wrapper returns an int

sys/dev/virtio/virtio.c

287 ↗

(On Diff #57930)

Nit: does initial caller of this pass in a big enough buffer or do we need to worry about truncation and return !0 in that case?

[cem]

sys/dev/virtio/mmio/virtio_mmio.c

340 ↗

(On Diff #57930)

I don't see how the second sentence would become stale. The purpose of the sentence is "please don't garbage collect this code even though it appears to do nothing." pnpinfo_str is provided as an example, not an exhaustive list. pnpinfo_str probably isn't going away any time soon.

The person that added MMIO didn't add it as a child bus but the VirtIO device drivers are transport agnostic.

This is outside the scope of this change, but should all Virtio drivers be associated with both buses? If so, would it make sense for the virtio_pci and virtio_mmio drivers to provide the same a pseudo-bus (e.g., virtio_bus) that child drivers attach to, being transport-agnostic, instead of the two separate virtio_pci and virtio_mmio buses?

339 ↗

(On Diff #57881)

Sorry, my mistake; I was looking at the PCI IVAR accessors rather than the VIRTIO ones at the time.

sys/dev/virtio/virtio.c

287 ↗

(On Diff #57930)

The buf is big enough (1-3kB depending on the caller) and no caller checks the return value of BUS_CHILD_PNPINFO_STR() anyway.

[cem]

@bryanv , do you have any remaining concerns with this one? Thanks!

[bryanv]

@cem LGTM