virtio feature negotiation and the viona API version number #1224

NOTE: This is a bit of a rambling train of thought. Sorry, but I had to get something down to help clear my thoughts. I'm not sure this issue has a clear action associated with it either. I just wanted to get something written down about my wrestling with this issue.

As some of you already know I've done off the deep end while pondering various implications of how virtio feature negotiation works and how it relates to our viona API version and propolis's assumptions based on said number. This became a hot topic due to concurrent virtio work between myself and @zeeshanlakhani: namely my work on F_EVENT_IDX (#1171) and Zeeshan's work on mac filters (#1210). Both of these features require work in the viona device as well as the VMM (c-bhyve or propolis). And both of these features could reasonably be expected to bump the viona API version even though its documentation makes clear that there is no well-defined rhyme or reason behind API number updates. In a lot of ways it's really just a bunch of pedantic, boring, inside baseball because at the end of the day it's not like there is some large VMM ecosystem for illumos bhyve.

Anyways, the problem is that the both features must race for the viona API number bump. Zeeshan's work landed first in our stlouis branch of illumos-gate, taking number 7. Whereas my work was reviewed as a patch straight to illumos-gate, also taking the version 7. But since Z's work beat me downstream, F_EVENT_IDX must now use a version of 8. Problem is, the mac filter work is still in the process of being upstreamed to illumos-gate, so F_EVENT_IDX is effectively blocked until that lands. However, F_EVENT_IDX is available to land downstream, which is good because we really want that feature in the next version of the Oxide Rack.

HOWEVER, there is also the proplis side of things, which tracks the viona API version; and the mac filter work (#1210) is still in progress, which means the F_EVENT_IDX is still ultimately blocked.

EXCEPT, @iximeow floated the version number update separately in #1216 for reasons relating to CI (another discussion which I'm not getting in here). So that means #1171 COULD land without waiting on #1216. Great, grand, wonderful.

But there's another little wrinkle that I added to this. For the F_EVENT_IDX work I did two things that have some ramifications on landing this work:

1. I made it so the viona device itself advertises F_EVENT_IDX. This felt perfectly natural to me because it does, afterall, contain an implementation of F_EVENT_IDX to support the Tx/Rx virtual queues.
2. I added the ability to disable viona features an runtime via a global tunable under viona_feat_disable_mask. This also made sense to me as a prudent choice to put in a relief valve in case the feature breaks someone and they need a quick fix. These features are meant to be dynamic, after all.

These choices change the calculus compared to previous enhancements such as multi-queue or the control queue. For those features we leave it up purely to the VMM to advertise/negotiate them even though many of these features require code/support in BOTH the VMM and the device. Example 1, VIRTIO_NET_F_CTRL_RX requests land in the VMM, but requires ioctls to the device in order to be applied. Example 2, once you enable VIRTIO_NET_F_CTRL_VQ you MUST have support in both the VMM and device for features like F_EVENT_IDX which apply to ALL virtqueues.

So what's the problem? Well, there are a few.

Propolis up to this point has relied purely on the viona version API to determine if a feature is implemented and available for runtime. That is, feature advertisement is effectively defined at compile-time. What happens when the host viona is updated with the new F_EVENT_IDX and you are running a propolis that hasn't been updated? Rhetorical question, it doesn't work. Why does this happen? Because propolis simply bitwise-ORs the viona device features onto its own list (see PciVirtioViona::features()). So now it would be prudent to update proplis first so that it's ready for the new feature, it's a propolis flag day for anyone who updates their host to the new viona. The viona API version does nothing to help us here, because it's a greather-than-or-equal check. So even if we bump it for F_EVENT_IDX, old propolis happily passes on the new feature to the guest sending it straight into a brick wall.

Furthermore, based on point (2) above, even with an updated propolis we can't just assume F_EVENT_IDX is usable: it could be disabled at runtime. So propolis feature negotiation needs to get a bit smarter and make sure to enable it only when the device advertises it.

So then I thought, well, maybe I shouldn't have done F_EVENT_IDX that way? Maybe I am the cause of all my problems!

And sure, we could make it so F_EVENT_IDX is determined purely by the VMM. That would allow us to land it without breaking old propolis instances on newer hosts, because it's up to the VMM when to enable it, and only a newer propolis will choose to do so. When I first realized this I thought it sounded like the obvious solution: duh!

But then I thought of another problem, and reason to keep it dynamic and advertised by the viona device (besides the fact that this dynamism allows us to get out of a jam if this feature proves problematic in production).

What happens if we decide to revert a stlouis/gate commit related to viona?

Let's fast-forward a month, both features are landed in both stlouis and gate, V7 = mac filters, V8 = F_EVENT_IDX. We're testing in the lab and find an issue with F_EVENT_IDX, we decide to back it out.

What happens to the version number?

Do we just go back to V7? Well we better back it out in propolis too. But what about newer propolis instances (on V8)? They are dead in the water once the viona backout is complete and their host is updated. Maybe that's okay, these things will always be welded together in the product, but we make no guarantees in dev. It's inconvenient, but not a deal breaker.

What about after we fix it and put it back? It goes back to V8, right? Well now we technically have to versions of V8 floating around, how is one to know if it's the good V8 or the bad V8? The fact that this number can go backwards while the system as a whole moves forwards is a bit surprising. But maybe we don't care, fine.

What if we decide the mac filter code needs to be backed out? Then what do we do? Do we go back to V6? Effectively backing out F_EVENT_IDX as well as far as propolis is concerned.

I guess it really depends how we back these things out. Because since they are features we could just remove their flags from the VMM's advertised set, but leave the code in place. Then we fix and code and put the flag back in place. Perhaps that is fine?

And now that I'm writing this I'm realizing that the backout problem is actually orthogonal to the issue of whether or not I keep F_EVENT_IDX dynamic/in the viona device. Because if you move the number backwards you make it so that newer propolis instances can no longer run. I think the only thing that might make sense is for the number to always increase, even for a backout, and for propolis to always make a strict equality check. That essentially allows us to pretend that it's an in-gate consumer, because we know it can only run against the exact version it was programmed against. Ignoring the fact that we (or some other illumos-gate contributor) could accidentally make a viona update that changes the API without bumping the number, but that's just life.

Anyways, all this said, I think it's basically an argument for being more dynamic in how we negotiate between the VMM and the virtio devices. With c-bhyve this was moot because it's all in gate, but with out-of-gate VMMs like propolis we end up with a very load-bearing and potentially brittle number. But maybe I'm overthinking all of this. Eventually there comes a time when features have proved themselves out and all this code becomes pretty static. I guess it's a question of how much runtime dynamism of feature selection is really worth?