virtio: Add VirtIO PCI modern (V1) support

[bryanv]

Summary

virtio: Add VirtIO PCI modern (V1) support

Use the existing legacy PCI driver as the basis for shared code
between the legacy and modern PCI drivers.

Changes to the virtqueue and each device driver (network, block, etc)
for V1 support, and hooking them in as child devices will come in later
commits.

[jrtc27]

sys/dev/virtio/mmio/virtio_mmio.c

356

Should be 1 if vtmmio_version == 2 (or > 1?).

[bryanv]

Update

[bryanv]

sys/dev/virtio/mmio/virtio_mmio.c

356

This predates 046096d, but when I was rebasing this change, I was a little confused as to what was going on in this driver but forgot to circle back to it. I was unsure how the V1 support was working, at least against a V1 (mmio-v2) compliant device. 50a6b28 had since fixed one of my major questions, but there's still various spec compliance issues remaining:

• 4.2.2.2/4.2.3.1.1: Must check MagicValue
• 4.2.2.2/4.2.3.1.1: Must ignore when DeviceId is 0x0
• 4.2.2.2: Must write value to QueueNum
• 4.2.2.2: Must read back QueueReady after writing 0x0
• 4.2.3.1.1: Must follow 3.1 Device Initialization - FEATURES_OK was added in V1, so without this is behaving as legacy. The virtio_bus_finialize_features bus interface was added to support FEATURES_OK.
• 2.4.1: Generation support. The virtio_bus_config_generation bus method was added to support when the generation is needed outside of the driver. Note 046096d incorrectly defined VIRTIO_MMIO_CONFIG_GENERATION as 0x100 instead of 0x0FC

And a few older bugs or code issues:

• vtmmio_detach() calls vtmio_reset() which assumes sc->res[0], but then detach checks for res[0] != NULL
• vtmmio_child_detached() should set status to ACK before returning; some code could be deduped here from vtmmio_probe_and_attach_child() when the device_attach() fails
• vtmmio_alloc_virtqueue() selects the VQ index twice
• In vtmmio_set_virtqueue, 046096d added #if 0 code that appears to be a debugging leftover, and would reference an uninitialized paddr variable

We should strive to get as close as possible to the spec, since neither the transport or device drivers are going to run against just one VMM.

I'll add a >1 check here to keep the status quo going.

[bryanv]

Update

[grehan]

I've looked at all the non-vtnet phab changes, and looked at the diff of the cumulative vtnet changes.

Also built and run the cumulative change set as a guest on CURRENT bhyve with net/block/random devices, and no issues found.

I'm going to accept this and all follow-on changes.

[jrtc27]

Rebasing this on top of D28073 would help to reduce the diff (provided you're still happy with the concept, but if so it'd be good to land the cleanup first IMO before all the new code comes in).

sys/dev/virtio/mmio/virtio_mmio.c

356

D28070 fixes the first couple of points. I intend to revisit the others once modern PCI has landed, either by copying the code or (better) pushing as much of the generic negotiation stuff into virtio.c itself (maybe even introducing a common parent class to all the transports if common softc state is needed) so it can be shared between MMIO and PCI (there is currently far too much duplication between the two...).

I haven't taken a look at the other old code quality issues (except for the 046096d-related one, I should just have deleted that debug printf rather than copy it, will do so tomorrow because it's a waste of space) but those are lower priority, at least for me (others are welcome to jump in and clean it up!).

sys/dev/virtio/pci/virtio_pci_modern.c

443–444

This is no longer going to be applicable with a common virtio_pci device name (and the problem needs solving for virtio_mmio given they're the same driver). I think it's best to just have each device's feature flags include it if supported? Or at least that's about the only sensible way I can think of.

671

style(9) doesn't explicitly state it, but has several examples where the casts don't have a space after them.

[bryanv]

sys/dev/virtio/mmio/virtio_mmio.c

356

Some of feature negotiation can be combined, like

features =  virtio_negotiate_features(host_features, child_features)

and add the the features sysctl to virtio_mmio, but I'm not sure if there is too much opportunity to further share code without impacting readability.

sys/dev/virtio/pci/virtio_pci_modern.c

443–444

It is though because we've attached via PCI modern by this point.

Ideally, the device (network, block, etc) drivers should not control the transport features. F_INDIRECT_DESC may be an exception because we do pre-alloc those chains. For both PCI and MMIO, we can improve this by just reflecting our supported transport features, and tighten things by checking that both PCI modern or MMIO version > 1 advertises F_VERSION_1. I don't think what is added is necessarily wrong though.

[jrtc27]

sys/dev/virtio/pci/virtio_pci_modern.c

443–444

To clarify, my concern is the following:

1. virtio_pci_modern / virtio_mmio (for version 1) attaches to the device
2. virtio_foo attaches to the virtio_pci/virtio_mmio transport but virtio_foo has not been updated to support V1, only legacy
3. VIRTIO_F_VERSION_1 gets added by this (or the equivalent MMIO) function
4. We end up negotiating VIRTIO_F_VERSION_1 even though the virtio_foo device driver does not support it

My instinct is/was that each device should be putting VIRTIO_F_VERSION_1 in its features, not the transport (and the queue makes sure to not filter it out).

[jrtc27]

sys/dev/virtio/mmio/virtio_mmio.c

356

Perhaps not. I remember being saddened at how similar the two are in a lot of places, but maybe they're different enough that it's not a good idea to try.

[bryanv]

sys/dev/virtio/pci/virtio_pci_modern.c

443–444

Going forward, new devices are modern so there is no legacy.

[jrtc27]

sys/dev/virtio/pci/virtio_pci_modern.c

443–444

And are the existing ones all V1? If everything supports V1 then should we not just always advertise that regardless of the transport?