Wire up viona notify fast path (#754)

The viona driver has the ability to hook an ioport (as specified by the
userspace vmm) in order to more efficiently process notifications from
the guest that virtqueues have new available entries.  This cuts down on
the number of VM exits which must be processed by userspace.

---------

Co-authored-by: iximeow <iximeow@oxide.computer>

Author: pfmooney

crates/viona-api/src/lib.rs

@@ -124,7 +124,7 @@ impl VionaFd {
         Ok(minor(&meta))
     }
 
-    /// Check VMM ioctl command against those known to not require any
+    /// Check viona ioctl command against those known to not require any
     /// copyin/copyout to function.
     const fn ioctl_usize_safe(cmd: i32) -> bool {
         matches!(
@@ -135,9 +135,10 @@ impl VionaFd {
                 | ioctls::VNA_IOC_RING_PAUSE
                 | ioctls::VNA_IOC_RING_INTR_CLR
                 | ioctls::VNA_IOC_VERSION
+                | ioctls::VNA_IOC_SET_NOTIFY_IOP
                 | ioctls::VNA_IOC_SET_PROMISC
                 | ioctls::VNA_IOC_GET_MTU
-                | ioctls::VNA_IOC_SET_MTU
+                | ioctls::VNA_IOC_SET_MTU,
         )
     }
 }

lib/propolis/src/hw/pci/bar.rs

@@ -121,33 +121,35 @@ impl Bars {
         &mut self,
         bar: BarN,
         val: u32,
-    ) -> Option<(BarDefine, u64, u64)> {
+    ) -> Option<WriteResult> {
         let idx = bar as usize;
         let ent = &mut self.entries[idx];
-        let (def, old, new) = match ent.kind {
+        let (id, def, val_old, val_new) = match ent.kind {
             EntryKind::Empty => return None,
             EntryKind::Pio(size) => {
                 let mask = u32::from(!(size - 1));
                 let old = ent.value;
                 ent.value = u64::from(val & mask);
-                (BarDefine::Pio(size), old, ent.value)
+                (bar, BarDefine::Pio(size), old, ent.value)
             }
             EntryKind::Mmio(size) => {
                 let mask = !(size - 1);
                 let old = ent.value;
                 ent.value = u64::from(val & mask);
-                (BarDefine::Mmio(size), old, ent.value)
+                (bar, BarDefine::Mmio(size), old, ent.value)
             }
             EntryKind::Mmio64(size) => {
                 let old = ent.value;
                 let mask = !(size - 1) as u32;
                 let low = val & mask;
                 ent.value = (old & (0xffffffff << 32)) | u64::from(low);
-                (BarDefine::Mmio64(size), old, ent.value)
+                (bar, BarDefine::Mmio64(size), old, ent.value)
             }
             EntryKind::Mmio64High => {
                 assert!(idx > 0);
-                let ent = &mut self.entries[idx - 1];
+                let real_idx = idx - 1;
+                let id = BarN::from_repr(real_idx as u8).unwrap();
+                let ent = &mut self.entries[real_idx];
                 let size = match ent.kind {
                     EntryKind::Mmio64(sz) => sz,
                     _ => panic!(),
@@ -156,11 +158,11 @@ impl Bars {
                 let old = ent.value;
                 let high = ((u64::from(val) << 32) & mask) & 0xffffffff00000000;
                 ent.value = high | (old & 0xffffffff);
-                (BarDefine::Mmio64(size), old, ent.value)
+                (id, BarDefine::Mmio64(size), old, ent.value)
             }
         };
-        if old != new {
-            return Some((def, old, new));
+        if val_old != val_new {
+            return Some(WriteResult { id, def, val_old, val_new });
         }
         None
     }
@@ -287,6 +289,18 @@ impl Bars {
     }
 }
 
+/// Result from a write to a BAR
+pub struct WriteResult {
+    /// Identifier of the actual impacted BAR.
+    ///
+    /// If write was to the high word of a 64-bit BAR, this would hold the
+    /// `BarN` for the lower word.
+    pub id: BarN,
+    pub def: BarDefine,
+    pub val_old: u64,
+    pub val_new: u64,
+}
+
 pub mod migrate {
     use serde::{Deserialize, Serialize};
 

lib/propolis/src/hw/pci/device.rs

@@ -15,11 +15,11 @@ use crate::migrate::*;
 use crate::util::regmap::{Flags, RegMap};
 
 use lazy_static::lazy_static;
+use strum::IntoEnumIterator;
 
 pub trait Device: Send + Sync + 'static {
     fn device_state(&self) -> &DeviceState;
 
-    #[allow(unused_variables)]
     fn bar_rw(&self, bar: BarN, rwo: RWOp) {
         match rwo {
             RWOp::Read(ro) => {
@@ -30,7 +30,6 @@ pub trait Device: Send + Sync + 'static {
             }
         }
     }
-    #[allow(unused_variables)]
     fn cfg_rw(&self, region: u8, rwo: RWOp) {
         match rwo {
             RWOp::Read(ro) => {
@@ -46,6 +45,13 @@ pub trait Device: Send + Sync + 'static {
     fn interrupt_mode_change(&self, mode: IntrMode) {}
     #[allow(unused_variables)]
     fn msi_update(&self, info: MsiUpdate) {}
+
+    /// Notification that configuration of BAR(s) has changed, either due to
+    /// writes to the BARs themselves, or an overall status change (via the
+    /// Command register or a device reset).
+    #[allow(unused_variables)]
+    fn bar_update(&self, bstate: BarState) {}
+
     // TODO
     // fn cap_read(&self);
     // fn cap_write(&self);
@@ -181,6 +187,18 @@ impl State {
     fn attached(&self) -> &bus::Attachment {
         self.attach.as_ref().unwrap()
     }
+    /// Is MMIO access decoding enabled?
+    fn mmio_en(&self) -> bool {
+        self.reg_command.contains(RegCmd::MMIO_EN)
+    }
+    /// Is PIO access decoding enabled?
+    fn pio_en(&self) -> bool {
+        self.reg_command.contains(RegCmd::IO_EN)
+    }
+    /// Given the device state, is decoding enabled for a specified [BarDefine]
+    fn decoding_active(&self, bar: &BarDefine) -> bool {
+        (bar.is_pio() && self.pio_en()) || (bar.is_mmio() && self.mmio_en())
+    }
 }
 
 pub(super) struct Cap {
@@ -375,15 +393,17 @@ impl DeviceState {
             StdCfgReg::Bar(bar) => {
                 let val = wo.read_u32();
                 let mut state = self.state.lock().unwrap();
-                if let Some((def, _old, new)) = state.bars.reg_write(*bar, val)
-                {
-                    let pio_en = state.reg_command.contains(RegCmd::IO_EN);
-                    let mmio_en = state.reg_command.contains(RegCmd::MMIO_EN);
-
+                if let Some(res) = state.bars.reg_write(*bar, val) {
                     let attach = state.attached();
-                    if (pio_en && def.is_pio()) || (mmio_en && def.is_mmio()) {
-                        attach.bar_unregister(*bar);
-                        attach.bar_register(*bar, def, new);
+                    if state.decoding_active(&res.def) {
+                        attach.bar_unregister(res.id);
+                        attach.bar_register(res.id, res.def, res.val_new);
+                        dev.bar_update(BarState {
+                            id: res.id,
+                            def: res.def,
+                            value: res.val_new,
+                            decode_en: true,
+                        });
                     }
                 }
             }
@@ -425,6 +445,8 @@ impl DeviceState {
 
         // Update BAR registrations
         if diff.intersects(RegCmd::IO_EN | RegCmd::MMIO_EN) {
+            let pio_en = val.contains(RegCmd::IO_EN);
+            let mmio_en = val.contains(RegCmd::MMIO_EN);
             for n in BarN::iter() {
                 let bar = state.bars.get(n);
                 if bar.is_none() {
@@ -433,18 +455,30 @@ impl DeviceState {
                 let (def, v) = bar.unwrap();
 
                 if diff.contains(RegCmd::IO_EN) && def.is_pio() {
-                    if val.contains(RegCmd::IO_EN) {
+                    if pio_en {
                         attach.bar_register(n, def, v);
                     } else {
                         attach.bar_unregister(n);
                     }
+                    dev.bar_update(BarState {
+                        id: n,
+                        def,
+                        value: v,
+                        decode_en: pio_en,
+                    });
                 }
                 if diff.contains(RegCmd::MMIO_EN) && def.is_mmio() {
-                    if val.contains(RegCmd::MMIO_EN) {
+                    if mmio_en {
                         attach.bar_register(n, def, v);
                     } else {
                         attach.bar_unregister(n);
                     }
+                    dev.bar_update(BarState {
+                        id: n,
+                        def,
+                        value: v,
+                        decode_en: mmio_en,
+                    });
                 }
             }
         }
@@ -482,6 +516,17 @@ impl DeviceState {
         self.which_intr_mode(&state)
     }
 
+    pub(crate) fn bar(&self, id: BarN) -> Option<BarState> {
+        let state = self.state.lock().unwrap();
+        state.bars.get(id).map(|(def, value)| {
+            let decode_en = match def {
+                BarDefine::Pio(_) => state.pio_en(),
+                BarDefine::Mmio(_) | BarDefine::Mmio64(_) => state.mmio_en(),
+            };
+            BarState { id, def, value, decode_en }
+        })
+    }
+
     fn cfg_cap_rw(&self, dev: &dyn Device, id: &CfgReg, rwo: RWOp) {
         match id {
             CfgReg::CapId(i) => {
@@ -608,10 +653,7 @@ impl DeviceState {
         let attach = inner.attached();
         for n in BarN::iter() {
             if let Some((def, addr)) = inner.bars.get(n) {
-                let pio_en = inner.reg_command.contains(RegCmd::IO_EN);
-                let mmio_en = inner.reg_command.contains(RegCmd::MMIO_EN);
-
-                if (pio_en && def.is_pio()) || (mmio_en && def.is_mmio()) {
+                if inner.decoding_active(&def) {
                     attach.bar_register(n, def, addr);
                 }
             }
@@ -1093,6 +1135,15 @@ impl Clone for MsixHdl {
     }
 }
 
+/// Describes the state of a BAR
+pub struct BarState {
+    pub id: BarN,
+    pub def: BarDefine,
+    pub value: u64,
+    /// Is decoding for this BAR enabled in the device control?
+    pub decode_en: bool,
+}
+
 pub struct Builder {
     ident: Ident,
     lintr_support: bool,

lib/propolis/src/hw/pci/mod.rs

@@ -11,7 +11,7 @@ use std::sync::{Arc, Mutex};
 use crate::common::*;
 use crate::intr_pins::IntrPin;
 
-use strum::FromRepr;
+use strum::{EnumIter, FromRepr};
 
 pub mod bar;
 pub mod bits;
@@ -273,7 +273,9 @@ impl Display for Bdf {
     }
 }
 
-#[derive(Copy, Clone, Eq, PartialEq, Debug, Ord, PartialOrd, FromRepr)]
+#[derive(
+    Copy, Clone, Eq, PartialEq, Debug, Ord, PartialOrd, FromRepr, EnumIter,
+)]
 #[repr(u8)]
 pub enum BarN {
     BAR0 = 0,
@@ -283,23 +285,6 @@ pub enum BarN {
     BAR4,
     BAR5,
 }
-impl BarN {
-    fn iter() -> BarIter {
-        BarIter { n: 0 }
-    }
-}
-struct BarIter {
-    n: u8,
-}
-impl Iterator for BarIter {
-    type Item = BarN;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        let res = BarN::from_repr(self.n)?;
-        self.n += 1;
-        Some(res)
-    }
-}
 
 #[repr(u8)]
 #[derive(Copy, Clone)]

lib/propolis/src/hw/virtio/pci.rs

@@ -3,6 +3,7 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 use std::ffi::c_void;
+use std::num::NonZeroU16;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Condvar, Mutex, MutexGuard, Weak};
 
@@ -88,6 +89,36 @@ impl VirtioState {
 pub trait PciVirtio: VirtioDevice + Send + Sync + 'static {
     fn virtio_state(&self) -> &PciVirtioState;
     fn pci_state(&self) -> &pci::DeviceState;
+
+    #[allow(unused_variables)]
+    /// Notification that the IO port representing the queue notification
+    /// register in the device BAR has changed.
+    fn notify_port_update(&self, state: Option<NonZeroU16>) {}
+
+    /// Notification from the PCI emulation that one of the BARs has undergone a
+    /// change of configuration
+    fn bar_update(&self, bstate: pci::BarState) {
+        if bstate.id == pci::BarN::BAR0 {
+            // Notify the device about the location (if any) of the Queue Notify
+            // register in the containing BAR region.
+            let port = if bstate.decode_en {
+                // Having registered `bstate.value` as the address in BAR0 only
+                // succeeds if that address through to the size of the
+                // registered region - the virtio legacy config registers - does
+                // not wrap. The base address *could* be zero, unwise as that
+                // would be, but adding LEGACY_REG_OFF_QUEUE_NOTIFY guarantees
+                // that the computed offset here is non-zero.
+                let notify_port_addr = NonZeroU16::new(
+                    bstate.value as u16 + LEGACY_REG_OFF_QUEUE_NOTIFY as u16,
+                )
+                .expect("addition does not wrap");
+                Some(notify_port_addr)
+            } else {
+                None
+            };
+            self.notify_port_update(port);
+        }
+    }
 }
 
 impl<D: PciVirtio + Send + Sync + 'static> pci::Device for D {
@@ -163,6 +194,10 @@ impl<D: PciVirtio + Send + Sync + 'static> pci::Device for D {
         state.intr_mode_updating = false;
         vs.state_cv.notify_all();
     }
+
+    fn bar_update(&self, bstate: pci::BarState) {
+        PciVirtio::bar_update(self, bstate);
+    }
 }
 
 pub struct PciVirtioState {
@@ -428,9 +463,9 @@ impl PciVirtioState {
 
     /// Indicate to the guest that the VirtIO device has encountered an error of
     /// some sort and requires a reset.
-    pub fn set_needs_reset(&self, _dev: &dyn VirtioDevice) {
+    pub fn set_needs_reset(&self, dev: &dyn VirtioDevice) {
         let mut state = self.state.lock().unwrap();
-        self.needs_reset_locked(_dev, &mut state);
+        self.needs_reset_locked(dev, &mut state);
     }
 
     fn queue_notify(&self, dev: &dyn VirtioDevice, queue: u16) {
@@ -624,6 +659,12 @@ impl MigrateMulti for dyn PciVirtio {
         // to the VirtIO state.
         vs.set_intr_mode(ps, ps.get_intr_mode().into(), true);
 
+        // Perform a (potentially spurious) update notification for the BAR
+        // containing the virtio registers.  This ensures that anything
+        // interested in the placement of that BAR (such as the notify-port
+        // logic) is kept well aware
+        self.bar_update(ps.bar(pci::BarN::BAR0).unwrap());
+
         Ok(())
     }
 }
@@ -796,6 +837,7 @@ enum VirtioTop {
 
 const LEGACY_REG_SZ: usize = 0x18;
 const LEGACY_REG_SZ_NO_MSIX: usize = 0x14;
+const LEGACY_REG_OFF_QUEUE_NOTIFY: usize = 0x10;
 
 #[derive(Copy, Clone, Eq, PartialEq, Debug)]
 enum LegacyReg {