viona: want F_EVENT_IDX support by rzezeski · Pull Request #1171 · oxidecomputer/propolis

rzezeski · 2026-07-14T19:01:54Z

This adds F_EVENT_IDX support for all virtqueues, but for this commit we enable it for viona only. This is needed to support F_EVENT_IDX for the viona control queue, which is required so that the illumos viona device may offer it for the data queues. It was found that Tx completion interrupts are greatly reduced with this feature support, presumably due to the Linux driver's better use of avail.used_event for interrupt moderation.

Adding this support has shown me a 10x reduction in guest Tx completion interrupts when running iperf benchmarks.

We might want to consider another change that lands prior to this one that actively prevents the negotiation of F_EVENT_IDX. Currently, if the host offers it, propolis blindly passes it through to the guest which will happily enable it: leading to the condition where the host and guest use avail_event/used_event, but the viona ctrl queue implemented by propolis is still using the flags method, ultimately leading to a guest that hangs during boot because it cannot make progress.

We can either do that, or we can make extra sure that Omicron picks up the latest propolis before we land this feature in Helios.

This adds F_EVENT_IDX support for all virtqueues, but for this commit we enable it for viona only. This is needed to support F_EVENT_IDX for the viona control queue, which is required so that the illumos viona device may offer it for the data queues. It was found that Tx completion interrupts are greatly reduced with this feature support, presumably due to the Linux driver's better use of `avail.used_event` for interrupt moderation.

rzezeski · 2026-07-14T19:10:21Z

I have only tested this with an Ubuntu Server 24.04 guest thus far. I'll see about trying to test some other platforms, but I don't have the slightest idea on testing Windows.

FelixMcFelix

Thanks Ryan, Double checking some things to make sure I understand what's going on with the wrapping math 100%.

- remove eprintln - add probe for viona set_features - properly negotiate F_EVENT_IDX - undo API version bump, features shouldn't be tied to this number - tests shouldn't assume F_EVENT_IDX is or is-not enabled - remove Features::RING_EVENT_IDX, this feature should never be assumed to work across all devices and instead must be negotiated per-device

rzezeski · 2026-10-01T18:04:46Z

I have successfully tested this in propolis-standalone with the following guests:

Ubuntu 24.04
FreeBSD 15.1
Windows Server 2022

For all of them I confirmed network connectivity for both an opte-based link used to test VPC traffic as it looks in an Oxide rack, as well as a non-opte VNIC link for external traffic. I use the external link to SSH to the guest and download software, and then I used the VPC link to run iperf3 tests between guests running on another host (to ensure that traffic leaves opte and hits vinoa/mac/cxgbe). Using in-guest tools and/or the new dtrace probe I also verified that F_EVENT_IDX is negotiated, as shown for windows below:

rpz@nibelheim:~/src/oxidecomputer/windows-image-builder$ pfexec dtrace -Zqn 'virtio_viona_set_features { printf("%s link: %u feat: 0x%x dev_feat: 0x%x\n", probeprov, arg0, arg1, arg2); }'
propolis9342 link: 174 feat: 0x1304799ab dev_feat: 0x31009983
propolis9346 link: 176 feat: 0x1304799ab dev_feat: 0x31009983
propolis9346 link: 177 feat: 0x1304799ab dev_feat: 0x31009983

Note that F_EVENT_IDX == 29 or 0x20000000. In this output dev_feat represents what features viona advertised and feat represents the features that were set by the VMM upon request from the guest.

iximeow

I spent wayyyyy too long puzzling over the same math Kyle did but at least I think I see what we all are thinking and hopefully my words made sense there.

I'm +1 on this as-is iff the diff on VIONA_CURRENT_INTERFACE_VERSION really is because this is an old base (maybe merge master?). in as much as I've asked about changes, my comments are very easily followup that I'm happy to do if you want. but if so I'll tag you for review (and if you take a month in retribution, I understand 😔)

iximeow · 2026-10-06T19:54:22Z

+#[cfg(target_os = "illumos")]
+
+mod os {
+    unsafe extern "C" {
+        pub fn membar_enter();
+    }
+}
+
+#[inline(always)]
+pub fn membar_enter() {
+    #[cfg(target_os = "illumos")]
+    unsafe {
+        os::membar_enter();
+    }
+}


if you wanted to put this somewhere like lib/propolis/src/util/mod.rs that would probably be good. you may have been surprised that we don't have membar_enter anywhere in propolis to date and that's mostly because we've (mis)used std::sync::atomic::fence as a barrier. so if you don't move it here then I will later to use in the other places.

because we've (mis)used std::sync::atomic::fence as a barrier

Yes. I forget the details, but basically I figured out that rust atomics are not sufficient as they will not, at runtime, invoke the host's membar_enter(), which is needed for correctness.

I started moving this for you and realized that I should leave it to you as you probably have stronger ideas on how you like things organized, and whether you like it or not I consider you the head custodian of this repo.

iximeow · 2026-10-06T21:25:25Z

 /// against.  All constants and structs defined by the crate are done so in
 /// terms of that specific version.
-pub const VIONA_CURRENT_INTERFACE_VERSION: u32 = 6;
+pub const VIONA_CURRENT_INTERFACE_VERSION: u32 = 7;


I think this is only in the diff still because your base is older than the commit on master that also bumped VIONA_CURRENT_INTERFACE_VERSION?

iximeow · 2026-10-07T21:33:32Z

+    /// Return used_event value.
+    fn used_event(&self, mem: &MemCtx) -> u16 {
+        if self.valid {
+            *mem.read(self.gpa_used_event).unwrap()
+        } else {
+            0
+        }


I realize this is How We Handle if !self.valid already elsewhere, but it seems to me like if the queue has become invalid we are within our rights to bail out of whatever we're doing on that queue. so like, we should return a Result<u16, TheQueueGotUnmapped>, and we should probably do that for flags too.

if we go processing a queue and send a spurious interrupt that's .. fine, because the guest will know it got the queue unmapped and not doing something too ridiculous, or we go processing a queue and don't send an interrupt based on the bogus-ish state we're working from. so this is OK and I'm really not saying we change it, just wondering if you think that change would be a reasonable later change.

That sounds reasonable. I haven't paged all the code back in to know for certain, but I also wonder if this is just defensive programming anyways, and if we never should be in this situation to begin with? That is, could the device lifecycle state be encoded in such a way (like type-state) that it's impossible to get here and the read could be unconditional?

In any case, I agree leaving it as its own change makes sense as I imagine it could end up touching quite a bit of code.

iximeow · 2026-10-07T21:50:16Z

well, i guess you'd need to merge master or rebase or something so that CI is green anyway.

FelixMcFelix

I think my original comments were answered at the time. Thanks for the extra exposition!

rzezeski requested review from FelixMcFelix and iximeow July 14, 2026 19:01

FelixMcFelix reviewed Jul 20, 2026

View reviewed changes

Comment thread lib/propolis/src/hw/virtio/queue.rs Outdated

Comment thread lib/propolis/src/hw/virtio/queue.rs Outdated

Comment thread lib/propolis/src/hw/virtio/queue.rs Outdated

Comment thread lib/propolis/src/hw/virtio/queue.rs

rzezeski added 6 commits July 21, 2026 09:29

xxx event_idx fixes

a72bd75

no longer needed

5d95de6

update tests

cae2861

update viona api version

55c9bd7

use correct type for avail_event offset

88c04fe

tests should not silently pass

9226b0d

rzezeski mentioned this pull request Sep 30, 2026

virtio feature negotiation and the viona API version number #1224

Open

rzezeski self-assigned this Oct 1, 2026

rzezeski added this to the 24 milestone Oct 1, 2026

iximeow approved these changes Oct 7, 2026

View reviewed changes

rzezeski added 2 commits October 9, 2026 15:30

add comment

ad1d64c

merge master

de83274

FelixMcFelix approved these changes Oct 9, 2026

View reviewed changes

rzezeski merged commit 6588611 into oxidecomputer:master Oct 10, 2026
14 checks passed

Conversation

rzezeski commented Jul 14, 2026

Uh oh!

rzezeski commented Jul 14, 2026

Uh oh!

FelixMcFelix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzezeski commented Oct 1, 2026

Uh oh!

iximeow left a comment

Choose a reason for hiding this comment

Uh oh!

iximeow Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

rzezeski Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iximeow Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iximeow Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

rzezeski Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

iximeow commented Oct 7, 2026

Uh oh!

FelixMcFelix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants