Page MenuHomeFreeBSD

virtio: Add VirtIO PCI modern (V1) support
ClosedPublic

Authored by bryanv on Dec 30 2020, 11:21 PM.

Details

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.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bryanv published this revision for review.Dec 30 2020, 11:27 PM
bryanv edited the summary of this revision. (Show Details)
jrtc27 added inline comments.
sys/dev/virtio/mmio/virtio_mmio.c
355

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

bryanv edited the summary of this revision. (Show Details)

Update

sys/dev/virtio/mmio/virtio_mmio.c
355

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.

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.

This revision is now accepted and ready to land.Jan 11 2021, 9:15 AM

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
355

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
442–443

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.

670

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

This revision was automatically updated to reflect the committed changes.
sys/dev/virtio/mmio/virtio_mmio.c
355

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
442–443

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.

sys/dev/virtio/pci/virtio_pci_modern.c
442–443

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

sys/dev/virtio/mmio/virtio_mmio.c
355

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.

sys/dev/virtio/pci/virtio_pci_modern.c
442–443

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

sys/dev/virtio/pci/virtio_pci_modern.c
442–443

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