[multicast] viona CTRL_RX/CTRL_MAC receive filtering + softnpu egress lock scoping + dep updates

.github/buildomat/jobs/check-headers.sh

@@ -18,11 +18,13 @@ set -e
 
 GATE_REF="$(./tools/check_headers gate_ref)"
 
-# TODO: `--branch` is overly restrictive, but it's what we've got. In git 2.49
-# the --revision flag was added to `git-clone`, and can clone an arbitrary
-# revision, which is more appropriate here. We might be tracking an arbitrary
-# commit with some changes in illumos-gate that isn't yet merged, after all.
-git clone --depth 1 --branch "$GATE_REF" \
-	https://code.oxide.computer/illumos-gate ./gate_src
+# `git clone --branch` only accepts branches and tags, but GATE_REF may be an
+# arbitrary ref (for example, a Gerrit change ref for an illumos-gate change
+# that has not yet merged). An init-and-fetch handles any ref the remote
+# advertises.
+git init ./gate_src
+git -C ./gate_src fetch --depth 1 \
+	https://code.oxide.computer/illumos-gate "$GATE_REF"
+git -C ./gate_src checkout --detach FETCH_HEAD
 
 ./tools/check_headers run ./gate_src

Cargo.toml

@@ -78,7 +78,7 @@ dlpi = { git = "https://github.com/oxidecomputer/dlpi-sys", branch = "main" }
 ispf = { git = "https://github.com/oxidecomputer/ispf" }
 libloading = "0.7"
 p9ds = { git = "https://github.com/oxidecomputer/p9fs" }
-softnpu = { git = "https://github.com/oxidecomputer/softnpu" }
+softnpu = { git = "https://github.com/oxidecomputer/softnpu", branch = "zl/multicast" }
 
 # Omicron-related
 internal-dns-resolver = { git = "https://github.com/oxidecomputer/omicron", rev = "5fd53a9c9ff2b0e47bfef3fe842b7516877b0162" }
@@ -149,7 +149,7 @@ newtype-uuid = { version = "1.0.1", features = [ "v4" ] }
 # "feature" for `utsname`, "poll" for `PollFlags`
 nix = { version = "0.31", features = [ "feature", "poll" ] }
 owo-colors = "4"
-oxide-tokio-rt = "0.1.2"
+oxide-tokio-rt = "0.1.4"
 paste = "1.0.15"
 pin-project-lite = "0.2.13"
 proc-macro2 = "1.0"

crates/viona-api/src/ffi.rs

@@ -39,6 +39,10 @@ pub const VNA_IOC_GET_MTU: i32 = vna_ioc(0x27);
 pub const VNA_IOC_SET_MTU: i32 = vna_ioc(0x28);
 pub const VNA_IOC_SET_NOTIFY_MMIO: i32 = vna_ioc(0x29);
 pub const VNA_IOC_INTR_POLL_MQ: i32 = vna_ioc(0x2a);
+pub const VNA_IOC_SET_MAC_FILTERS: i32 = vna_ioc(0x2b);
+pub const VNA_IOC_GET_MAC_FILTERS: i32 = vna_ioc(0x2c);
+pub const VNA_IOC_SET_MAC_ADDR: i32 = vna_ioc(0x2d);
+pub const VNA_IOC_GET_MAC_ADDR: i32 = vna_ioc(0x2e);
 
 /// VirtIO 1.2 queue pair support.
 pub const VNA_IOC_GET_PAIRS: i32 = vna_ioc(0x30);
@@ -135,6 +139,100 @@ pub const VIONA_PROMISC_ALL: i32 = 2;
 #[cfg(feature = "falcon")]
 pub const VIONA_PROMISC_ALL_VLAN: i32 = 3;
 
+/// Number of bytes in an Ethernet address, per `sys/ethernet.h`.
+pub const ETHERADDRL: usize = 6;
+
+/// The current multicast filter table capacity of a viona device.
+///
+/// This matches the 64-entry table convention of other virtio-net devices, per
+/// QEMU's [`MAC_TABLE_ENTRIES`]. A property of the device rather than a
+/// promise of the ABI: it may change, and consumers can discover it at
+/// runtime via [`vmf_nmcast`] on a [`VMF_ERR_COUNT`] failure.
+///
+/// [`MAC_TABLE_ENTRIES`]: https://github.com/qemu/qemu/blob/f893c46c3931b3684d235d221bf8b7844ddbf1d7/include/hw/virtio/virtio-net.h
+/// [`vmf_nmcast`]: vioc_mac_filters::vmf_nmcast
+pub const VIONA_MAX_MCAST_FILTERS: usize = 64;
+
+/// Semantic error codes reported through [`vmf_err`]
+/// (`vioc_mac_filter_err_t`).
+///
+/// [`vmf_err`]: vioc_mac_filters::vmf_err
+pub const VMF_OK: u32 = 0;
+/// Entry count exceeds device capacity (rewritten into [`vmf_nmcast`]).
+///
+/// [`vmf_nmcast`]: vioc_mac_filters::vmf_nmcast
+pub const VMF_ERR_COUNT: u32 = 1;
+/// Entry (in [`vmf_erraddr`]) is not a multicast address.
+///
+/// [`vmf_erraddr`]: vioc_mac_filters::vmf_erraddr
+pub const VMF_ERR_NOT_MCAST: u32 = 2;
+/// MAC layer refused installation of the entry (in [`vmf_erraddr`]).
+///
+/// [`vmf_erraddr`]: vioc_mac_filters::vmf_erraddr
+pub const VMF_ERR_INSTALL: u32 = 3;
+/// Client holds no unicast address (after a failed restoration).
+pub const VMF_ERR_NO_UNICAST: u32 = 4;
+
+/// Complete multicast MAC filter table, passed out-of-band through
+/// [`vmf_addrs`], which holds the user address of an array of
+/// [`vmf_nmcast`] Ethernet addresses ([`ETHERADDRL`] bytes each).
+///
+/// For [`VNA_IOC_SET_MAC_FILTERS`], the table replaces any previously
+/// installed one, and a count of zero clears all filters. On failure,
+/// [`vmf_err`] reports the failed check, with [`vmf_erraddr`] naming the
+/// offending entry for the checks that implicate one.
+///
+/// For [`VNA_IOC_GET_MAC_FILTERS`], [`vmf_nmcast`] counts the entries the
+/// buffer at [`vmf_addrs`] can hold, and is rewritten with the installed
+/// count.
+///
+/// [`vmf_addrs`]: vioc_mac_filters::vmf_addrs
+/// [`vmf_nmcast`]: vioc_mac_filters::vmf_nmcast
+/// [`vmf_err`]: vioc_mac_filters::vmf_err
+/// [`vmf_erraddr`]: vioc_mac_filters::vmf_erraddr
+#[repr(C)]
+#[derive(Default)]
+pub struct vioc_mac_filters {
+    pub vmf_nmcast: u32,
+    pub vmf_err: u32,
+    pub vmf_erraddr: [u8; ETHERADDRL],
+    pub vmf_pad: [u8; 2],
+    pub vmf_addrs: u64,
+}
+
+/// Semantic error codes reported through [`vma_err`]
+/// (`vioc_mac_addr_err_t`).
+///
+/// [`vma_err`]: vioc_mac_addr::vma_err
+pub const VMA_OK: u32 = 0;
+/// Requested address has the group bit set (IEEE 802.3).
+pub const VMA_ERR_NOT_UNICAST: u32 = 1;
+/// MAC layer refused installation of the address.
+pub const VMA_ERR_INSTALL: u32 = 2;
+/// A multicast filter could not be reinstalled after the swap.
+pub const VMA_ERR_MCAST_RESTORE: u32 = 3;
+
+/// For [`VNA_IOC_SET_MAC_ADDR`], [`vma_addr`] carries the unicast address
+/// to install in place of the current one, with [`vma_err`] copied out on
+/// failure and [`vma_present`] reporting whether an address remains
+/// installed, as a failed restoration leaves none.
+///
+/// For [`VNA_IOC_GET_MAC_ADDR`], [`vma_addr`] is copied out with the
+/// active unicast address of the client, and [`vma_present`] is nonzero
+/// when one is installed.
+///
+/// [`vma_addr`]: vioc_mac_addr::vma_addr
+/// [`vma_present`]: vioc_mac_addr::vma_present
+/// [`vma_err`]: vioc_mac_addr::vma_err
+#[repr(C)]
+#[derive(Default)]
+pub struct vioc_mac_addr {
+    pub vma_addr: [u8; ETHERADDRL],
+    pub vma_present: u8,
+    pub vma_pad: u8,
+    pub vma_err: u32,
+}
+
 #[repr(C)]
 #[derive(Default)]
 pub struct vioc_get_params {
@@ -154,7 +252,7 @@ pub struct vioc_set_params {
 /// This is the viona interface version which viona_api expects to operate
 /// 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;
 
 /// Maximum size of packed nvlists used in viona parameter ioctls
 pub const VIONA_MAX_PARAM_NVLIST_SZ: usize = 4096;

crates/viona-api/src/lib.rs

@@ -197,6 +197,10 @@ fn minor(meta: &std::fs::Metadata) -> u32 {
 #[repr(u32)]
 #[derive(Copy, Clone)]
 pub enum ApiVersion {
+    /// Adds support for installing MAC address filter tables on the
+    /// underlying MAC client.
+    V7 = 7,
+
     /// Adds multi-queue support and change the data structure for per-queue
     /// interrupt polling to a compact bitmap.
     V6 = 6,
@@ -218,7 +222,7 @@ pub enum ApiVersion {
 }
 impl ApiVersion {
     pub const fn current() -> Self {
-        Self::V6
+        Self::V7
     }
 }
 impl PartialEq<ApiVersion> for u32 {

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

@@ -24,6 +24,7 @@ pub const VIRTIO_NET_F_CTRL_VLAN: u64 = 1 << 19;
 pub const VIRTIO_NET_F_CTRL_RX_EXTRA: u64 = 1 << 20;
 pub const VIRTIO_NET_F_GUEST_ANNOUNCE: u64 = 1 << 21;
 pub const VIRTIO_NET_F_MQ: u64 = 1 << 22;
+pub const VIRTIO_NET_F_CTRL_MAC_ADDR: u64 = 1 << 23;
 
 // virtio-block feature bits
 pub const VIRTIO_BLK_F_SIZE_MAX: u64 = 1 << 1;

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

@@ -227,6 +227,16 @@ pub trait VirtioDevice: Send + Sync + 'static + Lifecycle {
     ) -> Result<(), ()> {
         Ok(())
     }
+
+    /// Notification that the virtio portion of the device has been reset.
+    ///
+    /// This fires for guest-initiated status resets as well as full device
+    /// resets, after the generic virtio state and virtqueues have been reset.
+    ///
+    /// Devices should use this to restore any state tied to feature
+    /// negotiation, since the next driver may negotiate a different feature
+    /// set.
+    fn device_reset(&self) {}
 }
 
 pub trait VirtioIntr: Send + 'static {

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

@@ -1190,6 +1190,10 @@ impl PciVirtioState {
         self.reset_queues_locked(dev, &mut state);
         state.reset();
         let _ = self.isr_state.read_clear();
+        // Release the state lock before calling into the device so that its
+        // reset handling may query generic virtio state.
+        drop(state);
+        dev.device_reset();
     }
 
     pub fn reset<D>(&self, dev: &D)

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

@@ -348,27 +348,16 @@ impl PciVirtioSoftNpuPort {
 
             let pkt = packet_in::new(&frame[..n]);
 
-            let mut pipeline = match self.pipeline.lock() {
-                Ok(pipe) => pipe,
-                Err(e) => {
-                    error!(self.log, "failed to lock pipeline: {}", e);
-                    break;
-                }
-            };
-            let pl: &mut Box<dyn Pipeline> = match &mut *pipeline {
-                Some(ref mut x) => &mut x.1,
-                None => {
-                    // This just means no P4 program has been set by the guest.
-                    break;
-                }
-            };
-
-            PacketHandler::process_guest_packet(
+            if PacketHandler::process_guest_packet(
                 pkt,
                 &self.data_handles,
-                pl,
+                &self.pipeline,
                 &self.log,
-            );
+            )
+            .is_err()
+            {
+                break;
+            }
         }
 
         if push_used {
@@ -446,6 +435,10 @@ impl VirtioDevice for PciVirtioSoftNpuPort {
     }
 }
 
+/// Error indicating the pipeline cannot process guest packets because no
+/// P4 program has been loaded by the guest.
+struct NoP4Program;
+
 struct PacketHandler {}
 
 impl PacketHandler {
@@ -504,21 +497,13 @@ impl PacketHandler {
             //
             // process packet with loaded P4 program
             //
-
-            // TODO pipeline should not need to be mutable for packet handling?
             let pkt = packet_in::new(&msg[..n]);
-            let mut p = pipeline.lock().unwrap();
-            let pl = match &mut *p {
-                Some(ref mut pl) => &mut pl.1,
-                None => continue, // no program is loaded
-            };
-
             Self::process_external_packet(
                 index + 1,
                 pkt,
                 &data_handles,
                 &virtio,
-                pl,
+                &pipeline,
                 &log,
             )
         }
@@ -531,12 +516,26 @@ impl PacketHandler {
         mut pkt: packet_in<'_>,
         data_handles: &Vec<dlpi::DlpiHandle>,
         virtio: &Arc<PortVirtioState>,
-        pipeline: &mut Box<dyn Pipeline>,
+        pipeline: &Mutex<Option<LoadedP4Program>>,
         log: &Logger,
     ) {
-        for (mut out_pkt, port) in
+        // We hold the pipeline lock only while the pipeline computes the
+        // egress set. Every port worker serializes on this lock, and the
+        // dlpi and virtio egress I/O can block. Holding the lock across
+        // that I/O stalls the other workers, which is especially costly
+        // for multicast where one input frame fans out to several ports.
+        // The egress packets borrow from `pkt` rather than the pipeline,
+        // so they remain valid after the lock is released.
+        let outputs = {
+            let mut guard = pipeline.lock().unwrap();
+            let pipeline = match &mut *guard {
+                Some(ref mut loaded) => &mut loaded.1,
+                None => return, // no program is loaded
+            };
             pipeline.process_packet(index as u16, &mut pkt)
-        {
+        };
+
+        for (mut out_pkt, port) in outputs {
             // packet is going to CPU port
             if port == 0 {
                 Self::send_packet_to_cpu_port(&mut out_pkt, virtio, &log);
@@ -555,20 +554,34 @@ impl PacketHandler {
 
     /// Run a packet coming into the ASIC from the guest pci port through the
     /// loaded pipeline and forward it on to its destination.
+    ///
+    /// Errors indicate the pipeline cannot process packets, and the caller
+    /// should stop reading frames.
     fn process_guest_packet(
         mut pkt: packet_in<'_>,
         data_handles: &Vec<dlpi::DlpiHandle>,
-        pipeline: &mut Box<dyn Pipeline>,
+        pipeline: &Mutex<Option<LoadedP4Program>>,
         log: &Logger,
-    ) {
-        for (mut out_pkt, port) in pipeline.process_packet(0, &mut pkt) {
+    ) -> std::result::Result<(), NoP4Program> {
+        // We hold the pipeline lock only for pipeline processing, not the
+        // egress I/O (see `process_external_packet` for rationale).
+        let outputs = {
+            let mut guard = pipeline.lock().unwrap();
+            let pipeline = match &mut *guard {
+                Some(ref mut loaded) => &mut loaded.1,
+                None => return Err(NoP4Program),
+            };
+            pipeline.process_packet(0, &mut pkt)
+        };
+
+        for (mut out_pkt, port) in outputs {
             if port == 0 {
                 // no looping packets back to the guest
-                return;
+                return Ok(());
             }
             if port == SOFTNPU_CPU_AUX_PORT {
                 // we are not currently emulating this port type
-                return;
+                return Ok(());
             }
             Self::send_packet_to_ext_port(
                 &mut out_pkt,
@@ -577,6 +590,7 @@ impl PacketHandler {
                 &log,
             );
         }
+        Ok(())
     }
 
     /// Send a packet out an external port using dlpi.