From b0c16f11fd5dd5b0c74b1fb4b0c3d23365eac650 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sun, 12 Jul 2026 17:51:29 +0000 Subject: [PATCH 1/4] [multicast] dep updates Pin softnpu and p4rs to their zl/multicast branches for multicast support in the emulated ASIC, and bump oxide-tokio-rt to 0.1.4. --- Cargo.lock | 4 ++-- Cargo.toml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dd1c5fa8b..0a54e16fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5774,7 +5774,7 @@ dependencies = [ [[package]] name = "p4rs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/p4?branch=main#33d646041f13062cc39c6f38725d8c95d4024a2f" +source = "git+https://github.com/oxidecomputer/p4?branch=zl/multicast#0e8a28a2edce0a96dd0ac3a3df95af3d58cee839" dependencies = [ "bitvec", "num", @@ -8809,7 +8809,7 @@ dependencies = [ [[package]] name = "softnpu" version = "0.2.0" -source = "git+https://github.com/oxidecomputer/softnpu#3828225fd5719dc985a300e36850d25a0405eba6" +source = "git+https://github.com/oxidecomputer/softnpu?branch=zl/multicast#284c6830722548714128e63ea04bcca78ee27154" dependencies = [ "p4rs", "serde", diff --git a/Cargo.toml b/Cargo.toml index f5f8d36b7..a8ea457f4 100644 --- a/Cargo.toml +++ b/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" From 637ef544fbb51cef1102b9db2053ffe24805404c Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sun, 12 Jul 2026 17:51:35 +0000 Subject: [PATCH 2/4] [viona] full CTRL_RX and CTRL_MAC receive filtering This work extends the minimal `VIRTIO_NET_F_CTRL_RX` support from [1125](https://github.com/oxidecomputer/propolis/pull/1125) to the full control-queue receive filtering surface, including promiscuity levels and guest-supplied unicast/multicast MAC tables being applied to the in-kernel device via the new [VNA_IOC_SET_MAC_FILTERS](https://code.oxide.computer/c/illumos-gate/+/775) ioctl. Filter state is cleared on device reset and carried across live migration via `VionaStateV1`. The payload is exported unconditionally, matching the import behavior on master since #1125, which takes the payload without consulting negotiated features. Import still tolerates a missing payload from sources predating VionaStateV1. The device narrows to classified delivery only once the guest installs a non-empty multicast table. An empty table holds the device at its all-multicast default, where some drivers (illumos vioif among them) negotiate `CTRL_RX` but never manage a filter table, and narrowing on an empty table would silently drop their multicast. The virtio spec permits the resulting unrequested traffic, but not the loss. On falcon links, a host without ALL_VLAN support now falls back to full promiscuity rather than all-multicast, since classified unicast delivery would drop transit frames for the emulated fabric. When viona interface version 7 (kernel-side MAC filter table support in st.louis) is available as per https://code.oxide.computer/c/illumos-gate/+/775 (in review), then this is fully lit up. --- .github/buildomat/jobs/check-headers.sh | 14 +- crates/viona-api/src/ffi.rs | 46 +- crates/viona-api/src/lib.rs | 6 +- lib/propolis/src/hw/virtio/mod.rs | 10 + lib/propolis/src/hw/virtio/pci.rs | 4 + lib/propolis/src/hw/virtio/viona.rs | 755 ++++++++++++++++++++++-- tools/check_headers | 10 +- 7 files changed, 783 insertions(+), 62 deletions(-) diff --git a/.github/buildomat/jobs/check-headers.sh b/.github/buildomat/jobs/check-headers.sh index f961c2d7d..5339fb6ea 100644 --- a/.github/buildomat/jobs/check-headers.sh +++ b/.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 diff --git a/crates/viona-api/src/ffi.rs b/crates/viona-api/src/ffi.rs index e67f97d11..3e1cfc41f 100644 --- a/crates/viona-api/src/ffi.rs +++ b/crates/viona-api/src/ffi.rs @@ -39,6 +39,7 @@ 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); /// VirtIO 1.2 queue pair support. pub const VNA_IOC_GET_PAIRS: i32 = vna_ioc(0x30); @@ -135,6 +136,49 @@ pub const VIONA_PROMISC_ALL: i32 = 2; #[cfg(feature = "falcon")] pub const VIONA_PROMISC_ALL_VLAN: i32 = 3; +/// Number of bytes in a filterable MAC address. +pub const VIONA_MAC_FILTER_ADDRL: usize = 6; + +/// Maximum number of unicast MAC address filters accepted by viona. +/// +/// No cross-device convention exists for a separate unicast table. QEMU +/// shares one 64-entry table across both address classes (see +/// [`VIONA_MAX_MCAST_FILTERS`]). Half the multicast table is generous for +/// entries that are rejected unless they match the primary MAC. +pub const VIONA_MAX_UNICAST_FILTERS: usize = 32; + +/// Maximum number of multicast MAC address filters accepted by viona. +/// +/// Matches the 64-entry table convention of other virtio-net devices, per +/// QEMU's [`MAC_TABLE_ENTRIES`]. +/// +/// [`MAC_TABLE_ENTRIES`]: https://github.com/qemu/qemu/blob/f893c46c3931b3684d235d221bf8b7844ddbf1d7/include/hw/virtio/virtio-net.h +pub const VIONA_MAX_MCAST_FILTERS: usize = 64; + +/// Complete unicast and multicast MAC filter tables, replacing any tables +/// previously installed on the link. Counts of zero clear all filters. +/// +/// Viona installs only multicast filters on the underlying MAC client. The +/// unicast table exists for shape parity with `VIRTIO_NET_CTRL_MAC_TABLE_SET`, +/// and entries other than the primary MAC of the link are rejected. +#[repr(C)] +pub struct vioc_mac_filters { + pub vmf_nucast: u32, + pub vmf_nmcast: u32, + pub vmf_ucast: [[u8; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_UNICAST_FILTERS], + pub vmf_mcast: [[u8; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_MCAST_FILTERS], +} +impl Default for vioc_mac_filters { + fn default() -> Self { + Self { + vmf_nucast: 0, + vmf_nmcast: 0, + vmf_ucast: [[0; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_UNICAST_FILTERS], + vmf_mcast: [[0; VIONA_MAC_FILTER_ADDRL]; VIONA_MAX_MCAST_FILTERS], + } + } +} + #[repr(C)] #[derive(Default)] pub struct vioc_get_params { @@ -154,7 +198,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; diff --git a/crates/viona-api/src/lib.rs b/crates/viona-api/src/lib.rs index d8535972c..6572aaacf 100644 --- a/crates/viona-api/src/lib.rs +++ b/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 for u32 { diff --git a/lib/propolis/src/hw/virtio/mod.rs b/lib/propolis/src/hw/virtio/mod.rs index 88894c759..1ed96ec8d 100644 --- a/lib/propolis/src/hw/virtio/mod.rs +++ b/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 { diff --git a/lib/propolis/src/hw/virtio/pci.rs b/lib/propolis/src/hw/virtio/pci.rs index 4c17d5c9a..957dbdbb0 100644 --- a/lib/propolis/src/hw/virtio/pci.rs +++ b/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(&self, dev: &D) diff --git a/lib/propolis/src/hw/virtio/viona.rs b/lib/propolis/src/hw/virtio/viona.rs index 4f2dc8ea6..246f8e2cc 100644 --- a/lib/propolis/src/hw/virtio/viona.rs +++ b/lib/propolis/src/hw/virtio/viona.rs @@ -205,6 +205,17 @@ pub mod control { return Err(()); } + // Bound the driver's entry count before it reaches an allocation. + // The limit sits well above viona's filter limits so that oversized + // tables still arrive intact and trigger the fallback to multicast + // promiscuity. Shipping guest drivers (Linux virtio_net, FreeBSD + // vtnet, Windows NetKVM) request all-multicast on their own long + // before this size. + const MAC_TABLE_MAX_ENTRIES: u32 = 1024; + if entry_count > MAC_TABLE_MAX_ENTRIES { + return Err(()); + } + chain .read_many_owned(mem, entry_count as usize) .map_err(|_| ()) @@ -323,6 +334,19 @@ bitflags! { } } +/// The kernel-side Rx configuration required to satisfy a guest's filter +/// state: a promiscuity level, optionally paired with an explicit multicast +/// filter table. +/// +/// `install_filters` is only set when `promisc` is [`PromiscLevel::None`]. +/// At any promiscuity level above `None`, the kernel already delivers all +/// multicast traffic, so no table is installed. +#[derive(Copy, Clone, Eq, PartialEq)] +struct RxConfig { + promisc: PromiscLevel, + install_filters: bool, +} + struct Inner { poller: Option, iop_state: Option, @@ -333,6 +357,9 @@ struct Inner { filter: FilterState, unicast_mac_filters: Box<[MacAddr]>, multicast_mac_filters: Box<[MacAddr]>, + /// Whether a (possibly partial) multicast filter table is installed on + /// the in-kernel MAC client via `VNA_IOC_SET_MAC_FILTERS`. + mac_filters_installed: bool, } impl Inner { fn new(max_queues: usize, promisc: PromiscLevel) -> Self { @@ -349,6 +376,7 @@ impl Inner { filter: FilterState::empty(), unicast_mac_filters: Box::new([]), multicast_mac_filters: Box::new([]), + mac_filters_installed: false, } } @@ -483,9 +511,16 @@ impl From<[u8; ETHERADDRL]> for MacAddr { } impl MacAddr { + /// Returns true if the group bit (IEEE 802 I/G, the least significant + /// bit of the first octet) is clear. pub const fn is_unicast(&self) -> bool { (self.0[0] & 0b1) == 0 } + + /// Returns true for the Ethernet broadcast address. + pub const fn is_broadcast(&self) -> bool { + matches!(self.0, [0xff, 0xff, 0xff, 0xff, 0xff, 0xff]) + } } /// Represents a connection to the kernel's Viona (VirtIO Network Adapter) @@ -498,6 +533,15 @@ pub struct PciVirtioViona { dev_features: u64, mac_addr: MacAddr, mtu: Option, + /// Whether the viona device supports `VNA_IOC_SET_MAC_FILTERS` + /// ([`viona_api::ApiVersion::V7`]). + kernel_mac_filters: bool, + /// Promiscuity pinned at construction (ALL_VLAN, or ALL where the host + /// lacks that support). Falcon links carry traffic for the entire + /// emulated fabric, which classified delivery would drop, so no guest + /// or migration state may downgrade below this level. + #[cfg(feature = "falcon")] + pinned_promisc: PromiscLevel, hdl: VionaHdl, inner: Mutex, } @@ -538,13 +582,18 @@ impl PciVirtioViona { let promisc_level = match hdl.set_promisc(PromiscLevel::AllVlan) { Ok(()) => PromiscLevel::AllVlan, Err(e) => { - // Until/unless this support is integrated into stlouis/illumos, - // this is an expected failure. This is needed to use vlans, - // but shouldn't affect any other use case. + // ALL_VLAN support only exists on the viona_vlans branch of + // illumos-gate, so this failure is expected elsewhere. + // + // We fall back to full promiscuity rather than all-multicast: + // falcon links carry traffic for the entire emulated fabric, + // and classified or multicast-only reception would drop + // transit frames. eprintln!( - "failed to enable promisc mode on {vnic_name}: {e:?}" + "failed to enable ALL_VLAN promisc on {vnic_name}: {e:?}" ); - PromiscLevel::AllMulti + hdl.set_promisc(PromiscLevel::All)?; + PromiscLevel::All } }; @@ -552,9 +601,11 @@ impl PciVirtioViona { vp.set(&hdl)?; } + let api_version = hdl.api_version()?; + // Do in-kernel configuration of device MTU if let Some(mtu) = info.mtu { - if hdl.api_version().unwrap() >= viona_api::ApiVersion::V4 { + if api_version >= viona_api::ApiVersion::V4 { hdl.set_mtu(mtu)?; } else if mtu != 1500 { // Squawk about MTUs not matching the default of 1500 @@ -599,6 +650,9 @@ impl PciVirtioViona { dev_features, mac_addr: info.mac_addr.into(), mtu: info.mtu, + kernel_mac_filters: api_version >= viona_api::ApiVersion::V7, + #[cfg(feature = "falcon")] + pinned_promisc: promisc_level, hdl, inner: Mutex::new(Inner::new(nqueues, promisc_level)), }; @@ -952,10 +1006,17 @@ impl PciVirtioViona { let old_filter = state.filter; state.filter.set(filter, active); - match self.set_promisc(self.needed_promisc(&state), &mut state) { + match self.apply_rx_config(&mut state) { Ok(()) => Ok(()), Err(()) => { state.filter = old_filter; + // The failure may have left a partial filter table on the + // MAC client, which would silently drop traffic at + // PromiscLevel::None. Reconcile the kernel with the restored + // state, and force a reset if that fails as well. + if self.apply_rx_config(&mut state).is_err() { + self.virtio_state.set_needs_reset(self); + } Err(()) } } @@ -984,11 +1045,16 @@ impl PciVirtioViona { std::mem::swap(&mut unicast, &mut state.unicast_mac_filters); std::mem::swap(&mut multicast, &mut state.multicast_mac_filters); - match self.set_promisc(self.needed_promisc(&state), &mut state) { + match self.apply_rx_config(&mut state) { Ok(()) => Ok(()), Err(()) => { state.unicast_mac_filters = unicast; state.multicast_mac_filters = multicast; + // Reconcile the kernel with the restored tables, as in + // set_filter_state above. + if self.apply_rx_config(&mut state).is_err() { + self.virtio_state.set_needs_reset(self); + } Err(()) } } @@ -1019,29 +1085,55 @@ impl PciVirtioViona { } } - /// Compute whether the driver requires us to move into promiscuous mode - /// based on its MAC filters and explicit filter mode. - fn needed_promisc(&self, state: &Inner) -> PromiscLevel { - // The VLAN tag workaround, if requested, always wins and cannot - // be downgraded. + /// The promiscuity level pinned at construction, if any. + /// + /// See the `pinned_promisc` field. This accessor exists so that callers + /// need no `cfg` gates of their own. + fn pinned_promisc(&self) -> Option { #[cfg(feature = "falcon")] - if state.promisc == PromiscLevel::AllVlan { - return PromiscLevel::AllVlan; + { + Some(self.pinned_promisc) + } + + #[cfg(not(feature = "falcon"))] + { + None + } + } + + /// Compute the Rx configuration required to satisfy the driver's MAC + /// filters and explicit filter mode. + /// + /// This is computed solely from the guest's filter state and device + /// capabilities, with no side effects. Effecting the result on the + /// in-kernel device is left to [`Self::apply_rx_config`]. + fn rx_config(&self, state: &Inner) -> RxConfig { + // A pinned link ignores the guest's filter state entirely. The pin + // supersets any level the guest could request, so at worst the + // guest receives traffic it did not ask for. + if let Some(pinned) = self.pinned_promisc() { + return RxConfig { promisc: pinned, install_filters: false }; } // We don't yet have any mechanism to account for ALL_UNICAST or // related filters from VIRTIO_NET_F_CTRL_RX_EXTRA, and the guest will // not request them. - // We don't have an ioctl yet for viona to explicitly install a set of - // filters. For now, we need to apply some level of promiscuous mode - // to give the guest what it asks for. + // An explicit multicast table can be installed on the in-kernel MAC + // client (VNA_IOC_SET_MAC_FILTERS) when the kernel supports it + // (viona_api::ApiVersion::V7) and the table fits. Otherwise + // multicast promiscuity covers the request. // - // Even though the guest can't yet use its parent feature, we can still - // account for NO_MULTICAST here. - let need_mcast = (state.filter.contains(FilterState::ALL_MULTICAST) + // NO_MULTICAST is gated behind VIRTIO_NET_F_CTRL_RX_EXTRA, which we + // do not offer, but the derivation accounts for it regardless. + let mcast_filterable = self.kernel_mac_filters + && state.multicast_mac_filters.len() + <= viona_api::VIONA_MAX_MCAST_FILTERS; + let need_mcast_promisc = (state + .filter + .contains(FilterState::ALL_MULTICAST) && !state.filter.contains(FilterState::NO_MULTICAST)) - || !state.multicast_mac_filters.is_empty(); + || (!state.multicast_mac_filters.is_empty() && !mcast_filterable); // Don't inflict promiscuous mode on drivers which request only their // own MAC address. Most guests *should not pass us any unicast @@ -1053,13 +1145,79 @@ impl PciVirtioViona { let need_promisc = state.filter.contains(FilterState::PROMISCUOUS) || !filter_is_self; - if need_promisc { + // A guest whose multicast table is empty has not demonstrated that + // it manages its own filtering. illumos vioif negotiates CTRL_RX and + // forwards promiscuity toggles over the control queue, but its + // multicast entry point is a noop, so narrowing to classified + // delivery after a promiscuity cycle (snoop on/off) would silently + // drop its multicast traffic. Unrequested traffic is permitted by + // the virtio spec, so hold the all-multicast lower bound until the + // table is populated. + let promisc = if need_promisc { PromiscLevel::All - } else if need_mcast { + } else if need_mcast_promisc || state.multicast_mac_filters.is_empty() { PromiscLevel::AllMulti } else { PromiscLevel::None + }; + + RxConfig { + promisc, + // The empty-table check is implied by the all-multicast lower + // bound above, but kept so an empty table is never installed if + // the bound is ever relaxed. + install_filters: promisc == PromiscLevel::None + && !state.multicast_mac_filters.is_empty(), + } + } + + /// Reconcile the in-kernel Rx configuration with the guest's filter + /// state, as computed by [`Self::rx_config`]. + /// + /// Ordering follows the contract documented for + /// `VNA_IOC_SET_MAC_FILTERS`: a filter table is installed before + /// promiscuity is lowered, and promiscuity is raised before a table is + /// cleared. Across the transition, delivery is at least once rather + /// than at most once. + fn apply_rx_config(&self, state: &mut Inner) -> Result<(), ()> { + let target = self.rx_config(state); + + let promisc = if target.install_filters { + // The table is reissued even when only the filter bits have + // changed. The kernel applies it differentially, leaving unchanged entries + // untouched, so the reissue is idempotent and cheap at these + // table sizes. + match self.hdl.set_mac_filters(&state.multicast_mac_filters) { + Ok(()) => { + state.mac_filters_installed = true; + target.promisc + } + // If the table could not be installed, cover the guest's + // request with multicast promiscuity instead. Unrequested + // traffic is permitted by the virtio spec, but loss is not. + // A failed install may leave a partial table on the MAC client, + // so we record it as installed to guarantee a later clearing. + Err(_) => { + state.mac_filters_installed = true; + PromiscLevel::AllMulti + } + } + } else { + target.promisc + }; + + self.set_promisc(promisc, state)?; + + // Promiscuity now meets or exceeds the required level, so a + // lingering table is redundant rather than harmful; clearing it is + // best-effort. + if !target.install_filters + && state.mac_filters_installed + && self.hdl.set_mac_filters(&[]).is_ok() + { + state.mac_filters_installed = false; } + Ok(()) } } impl VirtioDevice for PciVirtioViona { @@ -1123,11 +1281,6 @@ impl VirtioDevice for PciVirtioViona { VIRTIO_NO_MQ_CTRL_Q_INDEX }; - if (feat & VIRTIO_NET_F_CTRL_RX) != 0 { - let mut state = self.inner.lock().unwrap(); - self.set_promisc(PromiscLevel::None, &mut state)?; - } - Some(ctl_q_idx.try_into().expect("queue index must be a valid u16")) }; @@ -1213,6 +1366,35 @@ impl VirtioDevice for PciVirtioViona { } Ok(()) } + + fn device_reset(&self) { + // The next driver may not negotiate `VIRTIO_NET_F_CTRL_RX`, so any + // filter state configured by the previous driver must not outlive the + // reset. This runs for guest status resets as well as full device + // resets. + let mut state = self.inner.lock().unwrap(); + state.filter = FilterState::empty(); + state.unicast_mac_filters = Box::new([]); + state.multicast_mac_filters = Box::new([]); + + // Restore the pre-CTRL_RX default of multicast promiscuity (or the + // pinned level) before clearing any installed filter table so that + // no delivery gap opens. + // + // A guest reaches this path with a status write, so a failed restore + // must not panic. The table is left in place to keep covering delivery + // and the device is flagged as needing reset. + let restore = self.pinned_promisc().unwrap_or(PromiscLevel::AllMulti); + if self.set_promisc(restore, &mut state).is_err() { + self.virtio_state.set_needs_reset(self); + return; + } + + if state.mac_filters_installed && self.hdl.set_mac_filters(&[]).is_ok() + { + state.mac_filters_installed = false; + } + } } impl Lifecycle for PciVirtioViona { fn type_name(&self) -> &'static str { @@ -1227,12 +1409,6 @@ impl Lifecycle for PciVirtioViona { self.set_use_pairs(1).expect("can set viona back to one queue pair"); self.hdl.set_pairs(1).expect("can set viona back to one queue pair"); self.virtio_state.queues.reset_peak(); - - let mut state = self.inner.lock().unwrap(); - state.unicast_mac_filters = Box::new([]); - state.multicast_mac_filters = Box::new([]); - self.set_promisc(PromiscLevel::AllMulti, &mut state) - .expect("can reset viona promiscuous state"); } fn start(&self) -> anyhow::Result<()> { self.run(); @@ -1307,6 +1483,8 @@ impl MigrateMulti for PciVirtioViona { ) -> Result<(), MigrateStateError> { ::export(self, output, ctx)?; + // Exported unconditionally, as importers take this payload without + // consulting negotiated features. let viona_state = { let state = self.inner.lock().unwrap(); @@ -1358,7 +1536,18 @@ impl MigrateMulti for PciVirtioViona { )) })?; - let input: migrate::VionaStateV1 = offer.take()?; + let input: migrate::VionaStateV1 = match offer.take() { + Ok(input) => input, + // Only a source predating this payload sends nothing. Such a + // source never offered CTRL_RX, so the construction defaults + // already match its state. + Err(MigrateStateError::DataMissing) + if (feat & VIRTIO_NET_F_CTRL_RX) == 0 => + { + return Ok(()); + } + Err(e) => return Err(e), + }; let mut state = self.inner.lock().unwrap(); state.filter = @@ -1370,12 +1559,33 @@ impl MigrateMulti for PciVirtioViona { })?; state.unicast_mac_filters = input.unicast_mac_filters.into(); state.multicast_mac_filters = input.multicast_mac_filters.into(); - self.set_promisc(input.promisc, &mut state).map_err(|_| { - MigrateStateError::ImportFailed(format!( - "Could not move device promisc level to {:?}.", - input.promisc - )) - }) + match input.promisc { + // The source could only reach PromiscLevel::None through the + // guest's own filter state, so recompute the Rx configuration + // here rather than blindly restoring the level. This installs + // any needed multicast table on this host, or falls back to + // multicast promiscuity if it cannot be installed. + PromiscLevel::None => { + self.apply_rx_config(&mut state).map_err(|_| { + MigrateStateError::ImportFailed( + "Could not apply imported viona Rx configuration." + .to_string(), + ) + }) + } + // Any other level is either the pre-CTRL_RX default or already + // covers the guest's filters without a table. + level => { + // The pin is a property of this host's link, so an imported + // level must not downgrade it (see rx_config). + let level = self.pinned_promisc().unwrap_or(level); + self.set_promisc(level, &mut state).map_err(|_| { + MigrateStateError::ImportFailed(format!( + "Could not move device promisc level to {level:?}.", + )) + }) + } + } } } @@ -1674,6 +1884,36 @@ impl VionaHdl { self.0.ioctl_usize(viona_api::VNA_IOC_SET_PROMISC, usize::from(p))?; Ok(()) } + + /// Replace the explicit multicast MAC filter table on the underlying + /// device. An empty table clears any installed filters. + /// + /// Broadcast entries are omitted. The kernel drops them from the table, + /// as a MAC client is joined to broadcast for the lifetime of its unicast + /// address, so passing them would only waste table slots. + /// + /// The device's primary unicast address is always programmed on the + /// in-kernel MAC client, so no unicast entries are passed and `vmf_nucast` + /// remains zero. + /// + /// This requires [viona_api::ApiVersion::V7] or greater. + fn set_mac_filters(&self, multicast: &[MacAddr]) -> io::Result<()> { + assert!(multicast.len() <= viona_api::VIONA_MAX_MCAST_FILTERS); + + let entries = multicast + .iter() + .filter(|mac| !mac.is_broadcast()) + .map(|mac| mac.0) + .collect::>(); + + let mut vmf = viona_api::vioc_mac_filters::default(); + vmf.vmf_nmcast = entries.len() as u32; + vmf.vmf_mcast[..entries.len()].copy_from_slice(&entries); + unsafe { + self.0.ioctl(viona_api::VNA_IOC_SET_MAC_FILTERS, &mut vmf)?; + } + Ok(()) + } } impl AsRawFd for VionaHdl { @@ -1913,10 +2153,11 @@ mod test { use crate::hw::pci::Bdf; use crate::hw::virtio::pci::Status; use crate::hw::virtio::viona::{ + control, FilterState, MacAddr, PromiscLevel, VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VQ, VIRTIO_NET_F_MAC, VIRTIO_NET_F_MQ, VIRTIO_NET_F_STATUS, }; - use crate::hw::virtio::PciVirtioViona; + use crate::hw::virtio::{PciVirtioViona, VirtioDevice}; use crate::lifecycle::Lifecycle; use crate::migrate::{ MigrateCtx, MigrateMulti, PayloadOffer, PayloadOffers, PayloadOutputs, @@ -2218,6 +2459,18 @@ mod test { struct DriverState { max_pairs: Option, next_queue_gpa: u64, + queue_locs: std::collections::BTreeMap, + } + + /// Guest-physical layout of a virtqueue as programmed by `init_queue`, + /// retained so tests can build descriptor chains on it later. + #[derive(Copy, Clone)] + struct QueueLoc { + desc_gpa: u64, + avail_gpa: u64, + size: u16, + /// The next available-ring index the driver will publish. + avail_idx: u16, } impl DriverState { @@ -2226,6 +2479,7 @@ mod test { max_pairs: None, // Start virtio-nic queues somewhere other than address 0. next_queue_gpa: 2 * MB as u64, + queue_locs: std::collections::BTreeMap::new(), } } } @@ -2357,7 +2611,14 @@ mod test { self.common_config .write_le64(common_cfg::queue_desc, descriptor_table_gpa); - let avail_gpa = descriptor_table_gpa.next_multiple_of(page_u64); + // Give each ring a page of its own by stepping a full page per + // ring. + // + // Aligning with `next_multiple_of` would not work here: + // `next_queue_gpa` is already page-aligned, so it would be a + // noop and leave the descriptor table, available ring, and used + // ring all at the same address. + let avail_gpa = descriptor_table_gpa + page_u64; // First, flags. // > If the VIRTIO_F_EVENT_IDX feature bit is not negotiated: // > * The driver MUST set flags to 0 or 1. @@ -2370,15 +2631,25 @@ mod test { // negotiated VIRTIO_F_EVENT_IDX so no `used_event` for now. self.common_config.write_le64(common_cfg::queue_driver, avail_gpa); - let used_gpa = avail_gpa.next_multiple_of(page_u64); + let used_gpa = avail_gpa + page_u64; self.common_config.write_le64(common_cfg::queue_device, used_gpa); self.common_config.write_le16(common_cfg::queue_enable, 1); - // Finally, round up so the next queue (if there is one) starts - // page-aligned like it should. - self.state.next_queue_gpa = used_gpa.next_multiple_of(page_u64); + // Skip past the used ring so the next allocation (another queue, + // or a control message scratch buffer) does not share its page. + self.state.next_queue_gpa = used_gpa + page_u64; + + self.state.queue_locs.insert( + queue, + QueueLoc { + desc_gpa: descriptor_table_gpa, + avail_gpa, + size: queue_size.min(chosen_size), + avail_idx: 0, + }, + ); let msi_vector = 0x100 + queue; self.common_config @@ -2550,6 +2821,111 @@ mod test { // thinks everything is OK... assert!(self.status_ok()); } + + /// Submit a command on the control queue and return the device's ack + /// byte. + /// + /// The message is laid out as one device-readable descriptor holding + /// the header and payload, followed by one device-writable descriptor + /// for the ack, mirroring how guest drivers structure control commands. + fn send_ctrl_cmd( + &mut self, + class: u8, + command: u8, + payload: &[u8], + ) -> u8 { + const VIRTQ_DESC_F_NEXT: u16 = 1; + const VIRTQ_DESC_F_WRITE: u16 = 2; + + let qidx = self.ctl_qidx().expect("device has a control queue"); + let loc = *self + .state + .queue_locs + .get(&qidx) + .expect("control queue was initialized"); + + // Reserve a scratch page for the message buffers. Descriptors and + // rings are rewritten on every submission, so this also works on + // the fresh (0'ed) guest memory of a migration target. + let buf_gpa = self.state.next_queue_gpa; + self.state.next_queue_gpa += PAGE_SIZE as u64; + + let acc_mem = + self.machine.acc_mem.access().expect("can access memory"); + + let mut msg = vec![class, command]; + msg.extend_from_slice(payload); + assert_eq!( + acc_mem.write_from(GuestAddr(buf_gpa), &msg, msg.len()), + Some(msg.len()) + ); + + let ack_gpa = buf_gpa + msg.len() as u64; + // Seed the ack with a value the device will never write. + assert!(acc_mem.write::(GuestAddr(ack_gpa), &0xaa)); + + // Descriptor 0: header and payload, device-readable. + let d0 = loc.desc_gpa; + assert!(acc_mem.write::(GuestAddr(d0), &buf_gpa)); + assert!( + acc_mem.write::(GuestAddr(d0 + 8), &(msg.len() as u32)) + ); + assert!( + acc_mem.write::(GuestAddr(d0 + 12), &VIRTQ_DESC_F_NEXT) + ); + assert!(acc_mem.write::(GuestAddr(d0 + 14), &1u16)); + + // Descriptor 1: ack byte, device-writable. + let d1 = loc.desc_gpa + 16; + assert!(acc_mem.write::(GuestAddr(d1), &ack_gpa)); + assert!(acc_mem.write::(GuestAddr(d1 + 8), &1u32)); + assert!( + acc_mem.write::(GuestAddr(d1 + 12), &VIRTQ_DESC_F_WRITE) + ); + assert!(acc_mem.write::(GuestAddr(d1 + 14), &0u16)); + + // Publish the chain head in the available ring and bump the index. + let slot = u64::from(loc.avail_idx % loc.size); + assert!(acc_mem + .write::(GuestAddr(loc.avail_gpa + 4 + 2 * slot), &0u16)); + let new_idx = loc.avail_idx.wrapping_add(1); + assert!( + acc_mem.write::(GuestAddr(loc.avail_gpa + 2), &new_idx) + ); + self.state.queue_locs.get_mut(&qidx).unwrap().avail_idx = new_idx; + + let vq = self + .dev + .virtio_state + .queues + .get(qidx) + .expect("control queue exists") + .clone(); + self.dev.queue_notify(&vq); + + *acc_mem.read::(GuestAddr(ack_gpa)).expect("can read ack byte") + } + + fn ctrl_rx_cmd(&mut self, cmd: control::RxCmd, enable: bool) -> u8 { + self.send_ctrl_cmd(0, cmd as u8, &[u8::from(enable)]) + } + + fn ctrl_mac_table_set( + &mut self, + unicast: &[MacAddr], + multicast: &[MacAddr], + ) -> u8 { + let mut payload = Vec::new(); + payload.extend_from_slice(&(unicast.len() as u32).to_le_bytes()); + for mac in unicast { + payload.extend_from_slice(&mac.0); + } + payload.extend_from_slice(&(multicast.len() as u32).to_le_bytes()); + for mac in multicast { + payload.extend_from_slice(&mac.0); + } + self.send_ctrl_cmd(1, control::MacCmd::TableSet as u8, &payload) + } } fn test_device_status_writes(test_ctx: TestCtx) -> TestCtx { @@ -2629,7 +3005,7 @@ mod test { // try using it or setting any interesting features. // First, we have a fresh device on a fresh VM. The test is playing the - // role of the first use of the device by OVMF, an intiial bootloader, + // role of the first use of the device by OVMF, an initial bootloader, // or maybe the actual guest OS. let mut driver = test_ctx.create_driver(); driver.modern_device_init(expected_feats); @@ -2734,7 +3110,7 @@ mod test { let mut driver = test_ctx.create_driver(); // `basic_operation_multiqueue()` talks about why it's an interesting - // test to shink max pairs. + // test to shrink max pairs. driver.set_max_pairs(Some(4)); driver.modern_device_init(expected_feats | VIRTIO_NET_F_MQ); @@ -2789,7 +3165,7 @@ mod test { driver.modern_device_init(expected_feats | VIRTIO_NET_F_MQ); let mut driver = test_ctx.create_driver(); // `basic_operation_multiqueue()` talks about why it's an interesting - // test to shink max pairs. + // test to shrink max pairs. driver.set_max_pairs(Some(4)); driver.modern_device_init(expected_feats | VIRTIO_NET_F_MQ); @@ -2814,6 +3190,276 @@ mod test { test_ctx } + /// Drive the `VIRTIO_NET_F_CTRL_RX` command set, including the explicit MAC + /// filter tables, class-wide filter flags, and the promiscuity fallbacks + /// they demand from the device. + fn control_rx_filtering(test_ctx: TestCtx) -> TestCtx { + if cfg!(feature = "falcon") { + // Falcon links pin their promiscuity for the emulated fabric, + // which makes guest-driven Rx filtering a noop. + return test_ctx; + } + + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC + | VIRTIO_NET_F_STATUS + | VIRTIO_NET_F_CTRL_VQ + | VIRTIO_NET_F_CTRL_RX, + ); + + let kernel_filters = test_ctx.dev.kernel_mac_filters; + let self_mac = test_ctx.dev.mac_addr; + + // Negotiating CTRL_RX alone leaves the device at its all-multicast + // default. Filtering only engages once a non-empty multicast table + // is installed, protecting drivers that negotiate the feature but + // never manage a filter table (illumos vioif among them). + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + + // A table of our own MAC + a small multicast set narrows the + // device to classified delivery. V7 kernels take the explicit + // table, older kernels are covered by multicast promiscuity. + let mcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 1]), + MacAddr([0x33, 0x33, 0, 0, 0, 2]), + ]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &mcast); + + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + if kernel_filters { + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } else { + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + } + + // A broadcast entry is kept in the guest-visible table but omitted + // from the kernel filter set, as the MAC client is already joined + // to broadcast. Installation must still succeed. + let with_bcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 1]), + MacAddr([0xff, 0xff, 0xff, 0xff, 0xff, 0xff]), + ]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &with_bcast); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(&*state.multicast_mac_filters, &with_bcast[..]); + if kernel_filters { + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } else { + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + } + + // A multicast table larger than viona's filter maximum must fall + // back to multicast promiscuity rather than losing traffic. The + // limit is judged on the guest's table before broadcast omission, + // so a broadcast entry does not bring an over-limit table back + // under the maximum. + let mut big: Vec = (0..viona_api::VIONA_MAX_MCAST_FILTERS) + .map(|i| MacAddr([0x33, 0x33, 0, 0, 0, i as u8])) + .collect(); + big.push(MacAddr([0xff, 0xff, 0xff, 0xff, 0xff, 0xff])); + let ack = driver.ctrl_mac_table_set(&[], &big); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + + // An entry count beyond the parser's bound is rejected outright, + // leaving the previous table untouched. + let mut payload = 0u32.to_le_bytes().to_vec(); + payload.extend_from_slice(&2048u32.to_le_bytes()); + let ack = + driver.send_ctrl_cmd(1, control::MacCmd::TableSet as u8, &payload); + assert_eq!(ack, control::Ack::Err as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.multicast_mac_filters.len(), big.len()); + } + + // A unicast entry other than the device's own MAC cannot be + // classified and forces full promiscuity. + let foreign = MacAddr([0x02, 0, 0, 0, 0, 0x77]); + let ack = driver.ctrl_mac_table_set(&[foreign], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::All); + } + + // Clearing the tables returns to the all-multicast lower bound, and not + // classified delivery. An empty table gives no evidence the guest + // manages its own filtering. + let ack = driver.ctrl_mac_table_set(&[], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().promisc, + PromiscLevel::AllMulti + ); + + // Class-wide flags: ALL_MULTICAST raises multicast promiscuity, + // PROMISCUOUS raises full promiscuity, and disabling each falls + // back to the empty-table lower bound rather than classified + // delivery. + for (cmd, level) in [ + (control::RxCmd::AllMulticast, PromiscLevel::AllMulti), + (control::RxCmd::Promisc, PromiscLevel::All), + ] { + let ack = driver.ctrl_rx_cmd(cmd, true); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!(test_ctx.dev.inner.lock().unwrap().promisc, level); + let ack = driver.ctrl_rx_cmd(cmd, false); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().promisc, + PromiscLevel::AllMulti + ); + } + + // A malformed enable byte is rejected. + let ack = driver.send_ctrl_cmd(0, control::RxCmd::Promisc as u8, &[2]); + assert_eq!(ack, control::Ack::Err as u8); + + // CTRL_RX_EXTRA commands are not advertised and must be rejected. + let ack = driver.ctrl_rx_cmd(control::RxCmd::NoMulticast, true); + assert_eq!(ack, control::Ack::Err as u8); + + test_ctx + } + + /// A guest-initiated status reset must not leak the previous driver's + /// filter state to the next driver, which may not negotiate CTRL_RX at + /// all. + fn control_rx_reset_clears_filters(test_ctx: TestCtx) -> TestCtx { + if cfg!(feature = "falcon") { + // Falcon links pin their promiscuity, see `control_rx_filtering`. + return test_ctx; + } + + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC + | VIRTIO_NET_F_STATUS + | VIRTIO_NET_F_CTRL_VQ + | VIRTIO_NET_F_CTRL_RX, + ); + + let self_mac = test_ctx.dev.mac_addr; + let mcast = [MacAddr([0x01, 0x00, 0x5e, 0, 0, 0xfb])]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &mcast); + assert_eq!(ack, control::Ack::Ok as u8); + assert!(!test_ctx + .dev + .inner + .lock() + .unwrap() + .multicast_mac_filters + .is_empty()); + + // Reset via device status, as a guest reboot would. All filter state + // must revert to the pre-negotiation default of multicast + // promiscuity. + driver.write_status(Status::RESET); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.filter.is_empty()); + assert!(state.unicast_mac_filters.is_empty()); + assert!(state.multicast_mac_filters.is_empty()); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + + // The next driver comes up without CTRL_RX and keeps that default. + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | VIRTIO_NET_F_CTRL_VQ, + ); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().promisc, + PromiscLevel::AllMulti + ); + + test_ctx + } + + /// Filter state must survive migration: the guest-visible state (filter + /// flags and MAC tables) is carried in the payload and the target + /// re-derives the kernel configuration from it. + fn control_rx_migration(test_ctx: TestCtx) -> TestCtx { + if cfg!(feature = "falcon") { + // Falcon links pin their promiscuity, see `control_rx_filtering`. + return test_ctx; + } + + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC + | VIRTIO_NET_F_STATUS + | VIRTIO_NET_F_CTRL_VQ + | VIRTIO_NET_F_CTRL_RX, + ); + + let self_mac = test_ctx.dev.mac_addr; + let mcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 3]), + MacAddr([0x33, 0x33, 0, 0, 0, 4]), + ]; + let ack = driver.ctrl_mac_table_set(&[self_mac], &mcast); + assert_eq!(ack, control::Ack::Ok as u8); + let ack = driver.ctrl_rx_cmd(control::RxCmd::AllMulticast, true); + assert_eq!(ack, control::Ack::Ok as u8); + + let drv_state = driver.export(); + let test_ctx = test_ctx.migrate(); + let mut driver = VirtioNetDriver::import( + &test_ctx.machine, + &test_ctx.dev, + drv_state, + ); + assert!(driver.status_ok()); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.filter.contains(FilterState::ALL_MULTICAST)); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + } + + // The imported state must be live, not just carried data. Dropping + // ALL_MULTICAST on the target must re-derive the table-backed + // config. + let kernel_filters = test_ctx.dev.kernel_mac_filters; + let ack = driver.ctrl_rx_cmd(control::RxCmd::AllMulticast, false); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + if kernel_filters { + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } else { + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + } + + test_ctx + } + // Bears an uncanny resemblance to `phd-test`... struct TestCase { name: &'static str, @@ -2846,8 +3492,8 @@ mod test { } // We'll actually create and destroy some vnics so not only do we need - // `dladm`, we need a recent enough viona and everything.. this test is only - // meaningful on an illumos host:; + // `dladm`, we need a recent enough viona and everything. This test is only + // meaningful on an illumos host. #[test] #[cfg_attr(not(target_os = "illumos"), ignore)] fn run_viona_tests() { @@ -2869,6 +3515,9 @@ mod test { testcase!(multiqueue_to_singlequeue_to_multiqueue), testcase!(multiqueue_migration), testcase!(multiqueue_migration_after_boot), + testcase!(control_rx_filtering), + testcase!(control_rx_reset_clears_filters), + testcase!(control_rx_migration), ]; let underlying_nic = match std::env::var("VIONA_TEST_NIC") { diff --git a/tools/check_headers b/tools/check_headers index ee3cbca6d..a0b630de0 100755 --- a/tools/check_headers +++ b/tools/check_headers @@ -8,7 +8,15 @@ set -e # if your changes to Propolis track changes in the OS as well. # # As a default this ref should probably not change. -HEADER_CHECK_REF="stlouis" +# +# This is temporarily pinned to the viona MAC filter table change (stlouis#986, +# https://code.oxide.computer/c/illumos-gate/+/775), which adds the +# VNA_IOC_SET_MAC_FILTERS ioctl and struct vioc_mac_filters checked by the +# viona-api header-check. +# +# Restore to "stlouis" once that change integrates. Note: this ref names a +# specific patchset and must be bumped if a new patchset is uploaded. +HEADER_CHECK_REF="refs/changes/75/775/3" # Directories with `ctest2`-based `header-check` crates. This list should track # the similar exclusions in `Cargo.toml`, and are described more there. From cd0e2618eac4d80bbfcdbcbd546cb27fd62caa90 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sun, 12 Jul 2026 17:51:04 +0000 Subject: [PATCH 3/4] [softnpu] release pipeline lock before egress I/O Every port worker and the guest Tx path serialize on the pipeline mutex. Previously, the guard was held across both the dlpi and virtio egress, meaning that one worker blocked in egress I/O stalled the others. This is especially costly for multicast, where a single input frame fans out to several ports for replication. This change makes the lock scoped to pipeline evaluation only. The egress packets borrow from the input frame rather than the pipeline, so they remain valid after the guard is dropped. Lock acquisition moves from the callers into `process_external_packet` and `process_guest_packet`, which now take the mutex directly. The latter function returns a bool so the guest read loop can stop early when no P4 program is loaded or the lock is poisoned, preserving the prior break semantics. --- lib/propolis/src/hw/virtio/softnpu.rs | 88 ++++++++++++++++----------- 1 file changed, 53 insertions(+), 35 deletions(-) diff --git a/lib/propolis/src/hw/virtio/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs index 66546c543..3aca8d580 100644 --- a/lib/propolis/src/hw/virtio/softnpu.rs +++ b/lib/propolis/src/hw/virtio/softnpu.rs @@ -348,27 +348,14 @@ 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 = 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, - ); + ) { + break; + } } if push_used { @@ -504,21 +491,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 +510,26 @@ impl PacketHandler { mut pkt: packet_in<'_>, data_handles: &Vec, virtio: &Arc, - pipeline: &mut Box, + pipeline: &Mutex>, 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 +548,44 @@ 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. + /// + /// Returns false if no P4 program is loaded or the pipeline lock is + /// poisoned, in which case the caller should stop reading frames. fn process_guest_packet( mut pkt: packet_in<'_>, data_handles: &Vec, - pipeline: &mut Box, + pipeline: &Mutex>, log: &Logger, - ) { - for (mut out_pkt, port) in pipeline.process_packet(0, &mut pkt) { + ) -> bool { + // 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 = match pipeline.lock() { + Ok(guard) => guard, + Err(e) => { + error!(log, "failed to lock pipeline: {e}"); + return false; + } + }; + let pipeline = match &mut *guard { + Some(ref mut loaded) => &mut loaded.1, + None => { + // This just means no P4 program has been set by the + // guest. + return false; + } + }; + 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 true; } if port == SOFTNPU_CPU_AUX_PORT { // we are not currently emulating this port type - return; + return true; } Self::send_packet_to_ext_port( &mut out_pkt, @@ -577,6 +594,7 @@ impl PacketHandler { &log, ); } + true } /// Send a packet out an external port using dlpi. From dcad23754c05c74644c9bb0954ca7ed13dca8381 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Mon, 20 Jul 2026 23:09:48 +0000 Subject: [PATCH 4/4] [review] address CTRL_RX/viona filtering feedback Here are the follow-ups from review of the CTRL_RX/CTRL_MAC receive filtering work: - Withhold classified delivery until the first accepted MAC_TABLE_SET since reset. illumos vioif negotiates CTRL_RX but never programs the multicast table, so the all-multicast lower bound holds until the driver demonstrates that it can manage its own filtering. - We now carry that marker across migration as an optional payload field (multicast_table_managed). Older sources omit it and the target infers from non-empty tables, while older targets ignore it. - Track table installation independently of promiscuity (mac_filters_dirty), so guest promiscuity toggles no longer unplumb and replumb the MAC client's classified flows. Failed installs and clearings stay dirty and are reissued on a later application. - We scope the falcon promiscuity pin to softnpu-attached links rather than forcing every falcon VM promiscuous. - Return `Result<(), NoP4Program>` from softnpu's `process_guest_packet` instead of just a bool. - Write MAC filter entries directly into `vmf_mcast` rather than collecting them into an intermediate vector. - Bump HEADER_CHECK_REF to gerrit patchset 4. --- lib/propolis/src/hw/virtio/softnpu.rs | 36 +- lib/propolis/src/hw/virtio/viona.rs | 578 ++++++++++++++++++-------- tools/check_headers | 2 +- 3 files changed, 413 insertions(+), 203 deletions(-) diff --git a/lib/propolis/src/hw/virtio/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs index 3aca8d580..8804f0d68 100644 --- a/lib/propolis/src/hw/virtio/softnpu.rs +++ b/lib/propolis/src/hw/virtio/softnpu.rs @@ -348,12 +348,14 @@ impl PciVirtioSoftNpuPort { let pkt = packet_in::new(&frame[..n]); - if !PacketHandler::process_guest_packet( + if PacketHandler::process_guest_packet( pkt, &self.data_handles, &self.pipeline, &self.log, - ) { + ) + .is_err() + { break; } } @@ -433,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 { @@ -549,31 +555,21 @@ 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. /// - /// Returns false if no P4 program is loaded or the pipeline lock is - /// poisoned, in which case the caller should stop reading frames. + /// 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, pipeline: &Mutex>, log: &Logger, - ) -> bool { + ) -> 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 = match pipeline.lock() { - Ok(guard) => guard, - Err(e) => { - error!(log, "failed to lock pipeline: {e}"); - return false; - } - }; + let mut guard = pipeline.lock().unwrap(); let pipeline = match &mut *guard { Some(ref mut loaded) => &mut loaded.1, - None => { - // This just means no P4 program has been set by the - // guest. - return false; - } + None => return Err(NoP4Program), }; pipeline.process_packet(0, &mut pkt) }; @@ -581,11 +577,11 @@ impl PacketHandler { for (mut out_pkt, port) in outputs { if port == 0 { // no looping packets back to the guest - return true; + return Ok(()); } if port == SOFTNPU_CPU_AUX_PORT { // we are not currently emulating this port type - return true; + return Ok(()); } Self::send_packet_to_ext_port( &mut out_pkt, @@ -594,7 +590,7 @@ impl PacketHandler { &log, ); } - true + Ok(()) } /// Send a packet out an external port using dlpi. diff --git a/lib/propolis/src/hw/virtio/viona.rs b/lib/propolis/src/hw/virtio/viona.rs index 246f8e2cc..42e0e6e53 100644 --- a/lib/propolis/src/hw/virtio/viona.rs +++ b/lib/propolis/src/hw/virtio/viona.rs @@ -192,10 +192,26 @@ pub mod control { Ack = 0, } + /// Parse bound for a driver-provided MAC filter table. + /// + /// This bounds what a driver-controlled entry count can demand of the + /// device before it reaches an allocation. It sits well above viona's + /// filter limits, so a table that saturates it still exceeds those + /// limits and triggers the fallback to multicast promiscuity. + pub(super) const MAC_TABLE_MAX_ENTRIES: u32 = 1024; + /// Read a MAC filter table from a control queue. /// - /// These are encoded as a `u32` followed by that number of - /// 6-byte MAC addresses. + /// These are encoded as a `u32` followed by that number of 6-byte MAC + /// addresses. A table larger than [`MAC_TABLE_MAX_ENTRIES`] is truncated + /// to that bound, with the remaining entries skipped so that a list + /// following this one in the message still parses. The spec permits + /// a device given too many addresses to silently switch to all-multicast + /// or promiscuous mode, and refusing the table instead would leave the + /// previously installed one enforcing stale filters. See "Setting MAC + /// Address Filtering", footnote 10, in the [virtio spec]. + /// + /// [virtio spec]: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#fn10x5 pub(super) fn read_mac_list( chain: &mut super::Chain, mem: &super::MemCtx, @@ -205,21 +221,35 @@ pub mod control { return Err(()); } - // Bound the driver's entry count before it reaches an allocation. - // The limit sits well above viona's filter limits so that oversized - // tables still arrive intact and trigger the fallback to multicast - // promiscuity. Shipping guest drivers (Linux virtio_net, FreeBSD - // vtnet, Windows NetKVM) request all-multicast on their own long - // before this size. - const MAC_TABLE_MAX_ENTRIES: u32 = 1024; - if entry_count > MAC_TABLE_MAX_ENTRIES { + // Driver behavior varies above the device's table limits: FreeBSD + // vtnet caps its own table at 128 entries and falls back to + // all-multicast itself, while Linux virtio_net sends the full + // multicast list untruncated and relies on the device to degrade. + let read_count = entry_count.min(MAC_TABLE_MAX_ENTRIES); + let table = chain + .read_many_owned::(mem, read_count as usize) + .map_err(|_| ())?; + + let excess = (entry_count - read_count) as usize + * std::mem::size_of::(); + if excess > 0 && !skip_readable(chain, excess) { + // The driver declared more entries than the message carries, + // which is malformed rather than merely oversized. return Err(()); } - chain - .read_many_owned(mem, entry_count as usize) - .map_err(|_| ()) - .map(Vec::into_boxed_slice) + Ok(table.into_boxed_slice()) + } + + /// Advance the chain's read position by `len` bytes without copying. + fn skip_readable(chain: &mut super::Chain, len: usize) -> bool { + let mut remain = len; + let skipped = chain.for_remaining_type(true, |_addr, buf_len| { + let consumed = usize::min(buf_len, remain); + remain -= consumed; + (consumed, remain > 0) + }); + skipped == len } } @@ -233,6 +263,13 @@ pub mod migrate { pub filter: u8, pub unicast_mac_filters: Vec, pub multicast_mac_filters: Vec, + /// Whether the source had accepted a `MAC_TABLE_SET` since its last + /// reset. + /// + /// This is optional for compatibility, as older sources omit it and + /// older destinations ignore it. + #[serde(default)] + pub multicast_table_managed: Option, } impl Schema<'_> for VionaStateV1 { @@ -338,9 +375,10 @@ bitflags! { /// state: a promiscuity level, optionally paired with an explicit multicast /// filter table. /// -/// `install_filters` is only set when `promisc` is [`PromiscLevel::None`]. -/// At any promiscuity level above `None`, the kernel already delivers all -/// multicast traffic, so no table is installed. +/// `install_filters` records whether a filterable non-empty table should +/// remain installed. It is independent of `promisc`, so guest-requested +/// promiscuity changes do not unplumb and replumb the MAC client's +/// classified flows. #[derive(Copy, Clone, Eq, PartialEq)] struct RxConfig { promisc: PromiscLevel, @@ -357,9 +395,20 @@ struct Inner { filter: FilterState, unicast_mac_filters: Box<[MacAddr]>, multicast_mac_filters: Box<[MacAddr]>, - /// Whether a (possibly partial) multicast filter table is installed on - /// the in-kernel MAC client via `VNA_IOC_SET_MAC_FILTERS`. + /// Whether (or not) a (possibly partial) multicast filter table is + /// installed on the in-kernel MAC client via `VNA_IOC_SET_MAC_FILTERS`. mac_filters_installed: bool, + /// Whether (or not) the next installable multicast table must be written + /// because the guest changed it or a previous installation may have been + /// partial. + mac_filters_dirty: bool, + /// Whether (or not) the driver has issued an accepted `MAC_TABLE_SET` since + /// the last reset. + /// + /// An accepted table, including an empty one, demonstrates that the driver + /// manages its own filtering, which releases the all-multicast lower bound + /// in [`PciVirtioViona::rx_config`]. + mac_table_set: bool, } impl Inner { fn new(max_queues: usize, promisc: PromiscLevel) -> Self { @@ -377,6 +426,8 @@ impl Inner { unicast_mac_filters: Box::new([]), multicast_mac_filters: Box::new([]), mac_filters_installed: false, + mac_filters_dirty: false, + mac_table_set: false, } } @@ -536,12 +587,13 @@ pub struct PciVirtioViona { /// Whether the viona device supports `VNA_IOC_SET_MAC_FILTERS` /// ([`viona_api::ApiVersion::V7`]). kernel_mac_filters: bool, - /// Promiscuity pinned at construction (ALL_VLAN, or ALL where the host - /// lacks that support). Falcon links carry traffic for the entire - /// emulated fabric, which classified delivery would drop, so no guest - /// or migration state may downgrade below this level. + /// Promiscuity pinned at construction, when the host supports ALL_VLAN. + /// Falcon links may carry VLAN-tagged frames for the emulated fabric, + /// which classified delivery would drop, so no guest or migration state + /// may downgrade below the pin. Hosts without ALL_VLAN support leave the + /// link unpinned, with guest-managed filtering as on non-falcon builds. #[cfg(feature = "falcon")] - pinned_promisc: PromiscLevel, + pinned_promisc: Option, hdl: VionaHdl, inner: Mutex, } @@ -579,23 +631,27 @@ impl PciVirtioViona { #[cfg(not(feature = "falcon"))] let promisc_level = PromiscLevel::AllMulti; #[cfg(feature = "falcon")] - let promisc_level = match hdl.set_promisc(PromiscLevel::AllVlan) { - Ok(()) => PromiscLevel::AllVlan, - Err(e) => { - // ALL_VLAN support only exists on the viona_vlans branch of - // illumos-gate, so this failure is expected elsewhere. - // - // We fall back to full promiscuity rather than all-multicast: - // falcon links carry traffic for the entire emulated fabric, - // and classified or multicast-only reception would drop - // transit frames. - eprintln!( - "failed to enable ALL_VLAN promisc on {vnic_name}: {e:?}" - ); - hdl.set_promisc(PromiscLevel::All)?; - PromiscLevel::All - } - }; + let (promisc_level, pinned_promisc) = + match hdl.set_promisc(PromiscLevel::AllVlan) { + Ok(()) => (PromiscLevel::AllVlan, Some(PromiscLevel::AllVlan)), + Err(e) => { + // ALL_VLAN support only exists on the viona_vlans branch + // of illumos-gate, so this failure is expected elsewhere. + // + // Fall back to the stock all-multicast default with + // guest-managed filtering, leaving the link unpinned. + // The fabric traffic that needs more than a guest would + // request (VLAN-tagged sidecar frames) only exists on + // hosts running the ALL_VLAN-capable kernel, and pinning + // here would force every falcon guest into full + // promiscuity permanently. + eprintln!( + "failed to enable ALL_VLAN promisc on {vnic_name}: \ + {e:?}" + ); + (PromiscLevel::AllMulti, None) + } + }; if let Some(vp) = viona_params { vp.set(&hdl)?; @@ -630,7 +686,7 @@ impl PciVirtioViona { // purpose. let queues = VirtQueues::new_with_len(2, &queue_sizes); let nqueues = queues.max_capacity(); - hdl.set_pairs(1).unwrap(); + hdl.set_pairs(1)?; // Add one for config space. let msix_count = Some(1 + nqueues as u16); @@ -652,7 +708,7 @@ impl PciVirtioViona { mtu: info.mtu, kernel_mac_filters: api_version >= viona_api::ApiVersion::V7, #[cfg(feature = "falcon")] - pinned_promisc: promisc_level, + pinned_promisc, hdl, inner: Mutex::new(Inner::new(nqueues, promisc_level)), }; @@ -779,7 +835,7 @@ impl PciVirtioViona { if nqueues == self.virtio_state.queues.len() { return Ok(()); } - self.hdl.set_usepairs(requested).unwrap(); + self.hdl.set_usepairs(requested).map_err(|_| ())?; self.virtio_state.queues.set_len(nqueues).expect("num queue pairs"); Ok(()) } @@ -1014,7 +1070,9 @@ impl PciVirtioViona { // MAC client, which would silently drop traffic at // PromiscLevel::None. Reconcile the kernel with the restored // state, and force a reset if that fails as well. - if self.apply_rx_config(&mut state).is_err() { + let needs_reset = self.apply_rx_config(&mut state).is_err(); + drop(state); + if needs_reset { self.virtio_state.set_needs_reset(self); } Err(()) @@ -1044,15 +1102,24 @@ impl PciVirtioViona { std::mem::swap(&mut unicast, &mut state.unicast_mac_filters); std::mem::swap(&mut multicast, &mut state.multicast_mac_filters); + let prior_table_set = state.mac_table_set; + state.mac_table_set = true; + state.mac_filters_dirty = true; match self.apply_rx_config(&mut state) { Ok(()) => Ok(()), Err(()) => { state.unicast_mac_filters = unicast; state.multicast_mac_filters = multicast; + state.mac_table_set = prior_table_set; + // The failed attempt may have installed all or part of the + // new table, so the restored table must be reissued. + state.mac_filters_dirty = true; // Reconcile the kernel with the restored tables, as in // set_filter_state above. - if self.apply_rx_config(&mut state).is_err() { + let needs_reset = self.apply_rx_config(&mut state).is_err(); + drop(state); + if needs_reset { self.virtio_state.set_needs_reset(self); } Err(()) @@ -1066,10 +1133,6 @@ impl PciVirtioViona { level: PromiscLevel, state: &mut Inner, ) -> Result<(), ()> { - if level == state.promisc { - return Ok(()); - } - match self.hdl.set_promisc(level) { Ok(_) => { state.promisc = level; @@ -1077,9 +1140,7 @@ impl PciVirtioViona { } Err(_) => { // Ensure that we return to the old level of promisc. - if self.hdl.set_promisc(state.promisc).is_err() { - self.virtio_state.set_needs_reset(self); - } + let _ = self.hdl.set_promisc(state.promisc); Err(()) } } @@ -1092,7 +1153,7 @@ impl PciVirtioViona { fn pinned_promisc(&self) -> Option { #[cfg(feature = "falcon")] { - Some(self.pinned_promisc) + self.pinned_promisc } #[cfg(not(feature = "falcon"))] @@ -1140,22 +1201,32 @@ impl PciVirtioViona { // addresses*, as the config-space MAC is assumed to be included by // default. `.all()` will return `true` for such an empty list. This is // defensive programming against an otherwise well-meaning guest. - let filter_is_self = - state.unicast_mac_filters.iter().all(|mac| mac == &self.mac_addr); + // + // A list that saturates the parse bound may have been truncated, so + // it cannot be shown to contain only the device's own MAC and is + // treated as if it were naming foreign addresses. + let filter_is_self = (state.unicast_mac_filters.len() as u32) + < control::MAC_TABLE_MAX_ENTRIES + && state + .unicast_mac_filters + .iter() + .all(|mac| mac == &self.mac_addr); let need_promisc = state.filter.contains(FilterState::PROMISCUOUS) || !filter_is_self; - // A guest whose multicast table is empty has not demonstrated that - // it manages its own filtering. illumos vioif negotiates CTRL_RX and - // forwards promiscuity toggles over the control queue, but its - // multicast entry point is a noop, so narrowing to classified - // delivery after a promiscuity cycle (snoop on/off) would silently - // drop its multicast traffic. Unrequested traffic is permitted by - // the virtio spec, so hold the all-multicast lower bound until the - // table is populated. + // A guest that has never issued an accepted MAC_TABLE_SET has not + // demonstrated that it manages its own filtering. illumos vioif + // negotiates CTRL_RX and forwards promiscuity toggles over the + // control queue, but its multicast entry point is a noop, so + // narrowing to classified delivery after a promiscuity cycle + // (snoop on/off) would silently drop its multicast traffic. + // + // Unrequested traffic is permitted by the virtio spec, so we hold the + // all-multicast lower bound until the first accepted table (empty or) + // not. let promisc = if need_promisc { PromiscLevel::All - } else if need_mcast_promisc || state.multicast_mac_filters.is_empty() { + } else if need_mcast_promisc || !state.mac_table_set { PromiscLevel::AllMulti } else { PromiscLevel::None @@ -1163,10 +1234,18 @@ impl PciVirtioViona { RxConfig { promisc, - // The empty-table check is implied by the all-multicast lower - // bound above, but kept so an empty table is never installed if - // the bound is ever relaxed. - install_filters: promisc == PromiscLevel::None + // Installation tracks the table alone, not the promiscuity + // level, meaning a guest promiscuity cycle does not unplumb and + // replumb the table's classified flows (which quiesces the + // link). Viona keeps the classified callback installed: under + // ALL it drops classified copies, while under MULTI it drops + // classified multicast copies and delivers multicast through + // the promiscuous callback. + // + // An empty table is never installed. Classified delivery with + // no table covers the device MAC and broadcast, which is + // exactly what an empty table requests. + install_filters: mcast_filterable && !state.multicast_mac_filters.is_empty(), } } @@ -1177,29 +1256,35 @@ impl PciVirtioViona { /// Ordering follows the contract documented for /// `VNA_IOC_SET_MAC_FILTERS`: a filter table is installed before /// promiscuity is lowered, and promiscuity is raised before a table is - /// cleared. Across the transition, delivery is at least once rather - /// than at most once. + /// cleared, so new arrivals always have a live delivery path across the + /// transition, with in-kernel dedup suppressing the overlap's duplicate + /// copies. Packets already queued for classified delivery may still be + /// dropped under the new mode. fn apply_rx_config(&self, state: &mut Inner) -> Result<(), ()> { let target = self.rx_config(state); - let promisc = if target.install_filters { - // The table is reissued even when only the filter bits have - // changed. The kernel applies it differentially, leaving unchanged entries - // untouched, so the reissue is idempotent and cheap at these - // table sizes. + let update_filters = target.install_filters + && (!state.mac_filters_installed || state.mac_filters_dirty); + let promisc = if update_filters { match self.hdl.set_mac_filters(&state.multicast_mac_filters) { Ok(()) => { state.mac_filters_installed = true; + state.mac_filters_dirty = false; target.promisc } // If the table could not be installed, cover the guest's - // request with multicast promiscuity instead. Unrequested - // traffic is permitted by the virtio spec, but loss is not. - // A failed install may leave a partial table on the MAC client, - // so we record it as installed to guarantee a later clearing. + // multicast request with multicast promiscuity instead. A + // stronger promiscuity request must remain in force. A failed + // install may leave a partial table on the MAC client, so + // record it as installed and dirty to guarantee later + // replacement or clearing. Err(_) => { state.mac_filters_installed = true; - PromiscLevel::AllMulti + state.mac_filters_dirty = true; + match target.promisc { + PromiscLevel::None => PromiscLevel::AllMulti, + covered => covered, + } } } } else { @@ -1208,14 +1293,20 @@ impl PciVirtioViona { self.set_promisc(promisc, state)?; - // Promiscuity now meets or exceeds the required level, so a - // lingering table is redundant rather than harmful; clearing it is - // best-effort. - if !target.install_filters - && state.mac_filters_installed - && self.hdl.set_mac_filters(&[]).is_ok() - { - state.mac_filters_installed = false; + // No table is required when it is empty, cannot be installed, or the + // link is pinned. Promiscuity has already been raised where needed, + // so clearing a lingering table is best-effort. + if !target.install_filters { + if state.mac_filters_installed + && self.hdl.set_mac_filters(&[]).is_err() + { + // The failed clear may leave entries behind, so a later + // apply retries it. + state.mac_filters_dirty = true; + } else { + state.mac_filters_installed = false; + state.mac_filters_dirty = false; + } } Ok(()) } @@ -1376,6 +1467,8 @@ impl VirtioDevice for PciVirtioViona { state.filter = FilterState::empty(); state.unicast_mac_filters = Box::new([]); state.multicast_mac_filters = Box::new([]); + state.mac_table_set = false; + state.mac_filters_dirty = false; // Restore the pre-CTRL_RX default of multicast promiscuity (or the // pinned level) before clearing any installed filter table so that @@ -1386,13 +1479,18 @@ impl VirtioDevice for PciVirtioViona { // and the device is flagged as needing reset. let restore = self.pinned_promisc().unwrap_or(PromiscLevel::AllMulti); if self.set_promisc(restore, &mut state).is_err() { + state.mac_filters_dirty = state.mac_filters_installed; + drop(state); self.virtio_state.set_needs_reset(self); return; } - if state.mac_filters_installed && self.hdl.set_mac_filters(&[]).is_ok() - { - state.mac_filters_installed = false; + if state.mac_filters_installed { + if self.hdl.set_mac_filters(&[]).is_ok() { + state.mac_filters_installed = false; + } else { + state.mac_filters_dirty = true; + } } } } @@ -1406,8 +1504,15 @@ impl Lifecycle for PciVirtioViona { MqSetPairsCause::Reset as u8, 1 )); - self.set_use_pairs(1).expect("can set viona back to one queue pair"); - self.hdl.set_pairs(1).expect("can set viona back to one queue pair"); + // SET_PAIRS can fail while restoring the kernel's receive callbacks, + // and SET_USEPAIRS can fail independently. Attempt both restorations, + // and on failure surface NEEDS_RESET to the guest rather than + // panicking. + let use_pairs = self.set_use_pairs(1); + let pairs = self.hdl.set_pairs(1); + if use_pairs.is_err() || pairs.is_err() { + self.virtio_state.set_needs_reset(self); + } self.virtio_state.queues.reset_peak(); } fn start(&self) -> anyhow::Result<()> { @@ -1493,6 +1598,7 @@ impl MigrateMulti for PciVirtioViona { filter: state.filter.bits(), unicast_mac_filters: state.unicast_mac_filters.to_vec(), multicast_mac_filters: state.multicast_mac_filters.to_vec(), + multicast_table_managed: Some(state.mac_table_set), } }; @@ -1515,7 +1621,12 @@ impl MigrateMulti for PciVirtioViona { let has_ctl_queue = (feat & VIRTIO_NET_F_CTRL_VQ) != 0; if (feat & VIRTIO_NET_F_MQ) != 0 { - self.hdl.set_pairs(PROPOLIS_MAX_MQ_PAIRS).unwrap(); + self.hdl.set_pairs(PROPOLIS_MAX_MQ_PAIRS).map_err(|e| { + MigrateStateError::ImportFailed(format!( + "error while restoring viona queue pairs \ + ({PROPOLIS_MAX_MQ_PAIRS}): {e:?}" + )) + })?; } // Queue count is a NonZeroU16; hence `get` and -1 will not underflow. let io_queues = @@ -1549,43 +1660,50 @@ impl MigrateMulti for PciVirtioViona { Err(e) => return Err(e), }; + let migrate::VionaStateV1 { + // The source's promisc level is a host mechanism, derived from + // the guest state below plus the source's own pin and kernel + // capabilities. It is not restored directly: doing so could + // demand a level this kernel does not support (ALL_VLAN on a + // stock host) or hold a level this host's pin does not require. + promisc: _, + filter, + unicast_mac_filters, + multicast_mac_filters, + multicast_table_managed, + } = input; + let mut state = self.inner.lock().unwrap(); - state.filter = - FilterState::from_bits(input.filter).ok_or_else(|| { - MigrateStateError::ImportFailed(format!( - "unrecognised flags in filter state: {:x}", - input.filter & !FilterState::all().bits() - )) - })?; - state.unicast_mac_filters = input.unicast_mac_filters.into(); - state.multicast_mac_filters = input.multicast_mac_filters.into(); - match input.promisc { - // The source could only reach PromiscLevel::None through the - // guest's own filter state, so recompute the Rx configuration - // here rather than blindly restoring the level. This installs - // any needed multicast table on this host, or falls back to - // multicast promiscuity if it cannot be installed. - PromiscLevel::None => { - self.apply_rx_config(&mut state).map_err(|_| { - MigrateStateError::ImportFailed( - "Could not apply imported viona Rx configuration." - .to_string(), - ) - }) - } - // Any other level is either the pre-CTRL_RX default or already - // covers the guest's filters without a table. - level => { - // The pin is a property of this host's link, so an imported - // level must not downgrade it (see rx_config). - let level = self.pinned_promisc().unwrap_or(level); - self.set_promisc(level, &mut state).map_err(|_| { - MigrateStateError::ImportFailed(format!( - "Could not move device promisc level to {level:?}.", - )) - }) - } - } + state.filter = FilterState::from_bits(filter).ok_or_else(|| { + MigrateStateError::ImportFailed(format!( + "unrecognised flags in filter state: {:x}", + filter & !FilterState::all().bits() + )) + })?; + state.unicast_mac_filters = unicast_mac_filters.into(); + state.multicast_mac_filters = multicast_mac_filters.into(); + state.mac_filters_dirty = true; + + // Older sources do not carry whether they had accepted a + // MAC_TABLE_SET. Absent this marker, a non-empty table proves that + // one arrived, while a guest whose accepted table was empty + // conservatively returns to the all-multicast lower bound until its + // next command. + state.mac_table_set = multicast_table_managed.unwrap_or( + !state.unicast_mac_filters.is_empty() + || !state.multicast_mac_filters.is_empty(), + ); + + // Recompute the Rx configuration from the imported guest state. + // + // This installs any needed multicast table on this host or falls + // back to multicast promiscuity if it cannot be installed, and a + // local pin supersedes the result (see rx_config). + self.apply_rx_config(&mut state).map_err(|_| { + MigrateStateError::ImportFailed( + "Could not apply imported viona Rx configuration.".to_string(), + ) + }) } } @@ -1900,15 +2018,13 @@ impl VionaHdl { fn set_mac_filters(&self, multicast: &[MacAddr]) -> io::Result<()> { assert!(multicast.len() <= viona_api::VIONA_MAX_MCAST_FILTERS); - let entries = multicast - .iter() - .filter(|mac| !mac.is_broadcast()) - .map(|mac| mac.0) - .collect::>(); - let mut vmf = viona_api::vioc_mac_filters::default(); - vmf.vmf_nmcast = entries.len() as u32; - vmf.vmf_mcast[..entries.len()].copy_from_slice(&entries); + let mut count = 0; + for mac in multicast.iter().filter(|mac| !mac.is_broadcast()) { + vmf.vmf_mcast[count] = mac.0; + count += 1; + } + vmf.vmf_nmcast = count as u32; unsafe { self.0.ioctl(viona_api::VNA_IOC_SET_MAC_FILTERS, &mut vmf)?; } @@ -2844,17 +2960,19 @@ mod test { .get(&qidx) .expect("control queue was initialized"); - // Reserve a scratch page for the message buffers. Descriptors and - // rings are rewritten on every submission, so this also works on - // the fresh (0'ed) guest memory of a migration target. + let mut msg = vec![class, command]; + msg.extend_from_slice(payload); + + // Reserve scratch pages for the message and ack byte. + // Descriptors and rings are rewritten on every submission, so + // this also works on the fresh (0'ed) guest memory of a + // migration target. let buf_gpa = self.state.next_queue_gpa; - self.state.next_queue_gpa += PAGE_SIZE as u64; + self.state.next_queue_gpa += + (msg.len() + 1).next_multiple_of(PAGE_SIZE) as u64; let acc_mem = self.machine.acc_mem.access().expect("can access memory"); - - let mut msg = vec![class, command]; - msg.extend_from_slice(payload); assert_eq!( acc_mem.write_from(GuestAddr(buf_gpa), &msg, msg.len()), Some(msg.len()) @@ -3194,9 +3312,9 @@ mod test { /// filter tables, class-wide filter flags, and the promiscuity fallbacks /// they demand from the device. fn control_rx_filtering(test_ctx: TestCtx) -> TestCtx { - if cfg!(feature = "falcon") { - // Falcon links pin their promiscuity for the emulated fabric, - // which makes guest-driven Rx filtering a noop. + if test_ctx.dev.pinned_promisc().is_some() { + // A link pinned for the emulated fabric ignores guest filter + // state, which makes guest-driven Rx filtering a noop. return test_ctx; } @@ -3212,13 +3330,28 @@ mod test { let self_mac = test_ctx.dev.mac_addr; // Negotiating CTRL_RX alone leaves the device at its all-multicast - // default. Filtering only engages once a non-empty multicast table - // is installed, protecting drivers that negotiate the feature but - // never manage a filter table (illumos vioif among them). + // default. Filtering only engages once the driver issues an + // accepted MAC_TABLE_SET, protecting drivers that negotiate the + // feature but never manage a filter table (illumos vioif among + // them). { let state = test_ctx.dev.inner.lock().unwrap(); assert_eq!(state.promisc, PromiscLevel::AllMulti); assert!(!state.mac_filters_installed); + assert!(!state.mac_table_set); + } + + // An accepted table releases the bound even when empty: the driver + // has demonstrated that it manages filtering, and an empty table + // requests only the device MAC and broadcast. No kernel table is + // needed for that, so this holds on pre-V7 kernels as well. + let ack = driver.ctrl_mac_table_set(&[], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::None); + assert!(!state.mac_filters_installed); + assert!(state.mac_table_set); } // A table of our own MAC + a small multicast set narrows the @@ -3264,6 +3397,35 @@ mod test { } } + // Guest requested promiscuity changes leave an installed table in + // place, avoiding needless MAC flow plumbing and link quiescing. + // Dropping each flag returns directly to table-backed delivery. + let filtered_level = if kernel_filters { + PromiscLevel::None + } else { + PromiscLevel::AllMulti + }; + for (cmd, level) in [ + (control::RxCmd::AllMulticast, PromiscLevel::AllMulti), + (control::RxCmd::Promisc, PromiscLevel::All), + ] { + let ack = driver.ctrl_rx_cmd(cmd, true); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, level); + assert_eq!(state.mac_filters_installed, kernel_filters); + } + + let ack = driver.ctrl_rx_cmd(cmd, false); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, filtered_level); + assert_eq!(state.mac_filters_installed, kernel_filters); + } + } + // A multicast table larger than viona's filter maximum must fall // back to multicast promiscuity rather than losing traffic. The // limit is judged on the guest's table before broadcast omission, @@ -3279,10 +3441,47 @@ mod test { let state = test_ctx.dev.inner.lock().unwrap(); assert_eq!(state.promisc, PromiscLevel::AllMulti); assert!(!state.mac_filters_installed); + assert!(state.mac_table_set); + } + + // An entry count above the parse bound is not refused. The table is + // truncated to the bound, the remainder of the body is skipped, and + // the oversized result falls back to multicast promiscuity. + let huge_mc: Vec = (0..=control::MAC_TABLE_MAX_ENTRIES) + .map(|i| MacAddr([0x33, 0x33, 0, 0, (i >> 8) as u8, i as u8])) + .collect(); + let ack = driver.ctrl_mac_table_set(&[], &huge_mc); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + assert_eq!( + state.multicast_mac_filters.len(), + control::MAC_TABLE_MAX_ENTRIES as usize + ); } - // An entry count beyond the parser's bound is rejected outright, - // leaving the previous table untouched. + // Skipping an oversized unicast list leaves the read position at the + // multicast list that follows it, which must still parse. A unicast + // list that saturates the parse bound may have been truncated, so it + // cannot be shown to contain only the device's own MAC and forces + // full promiscuity even though every entry sent matches it. + let huge_uc = + vec![self_mac; control::MAC_TABLE_MAX_ENTRIES as usize + 1]; + let ack = driver.ctrl_mac_table_set(&huge_uc, &mcast); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.promisc, PromiscLevel::All); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + // The fitting multicast table stays installed under full + // promiscuity, where it has no delivery effect. + assert_eq!(state.mac_filters_installed, kernel_filters); + } + + // A count declared without a matching body is malformed and is still + // rejected, leaving the previous tables untouched. let mut payload = 0u32.to_le_bytes().to_vec(); payload.extend_from_slice(&2048u32.to_le_bytes()); let ack = @@ -3290,7 +3489,7 @@ mod test { assert_eq!(ack, control::Ack::Err as u8); { let state = test_ctx.dev.inner.lock().unwrap(); - assert_eq!(state.multicast_mac_filters.len(), big.len()); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); } // A unicast entry other than the device's own MAC cannot be @@ -3301,37 +3500,22 @@ mod test { { let state = test_ctx.dev.inner.lock().unwrap(); assert_eq!(state.promisc, PromiscLevel::All); + // The now-empty multicast table is cleared from the kernel + // even while promiscuity holds. + assert!(!state.mac_filters_installed); } - // Clearing the tables returns to the all-multicast lower bound, and not - // classified delivery. An empty table gives no evidence the guest - // manages its own filtering. + // Clearing both tables narrows delivery back to classified: the + // accepted table update demonstrates that the guest manages + // filtering, and an empty table requests only the device MAC and + // broadcast. let ack = driver.ctrl_mac_table_set(&[], &[]); assert_eq!(ack, control::Ack::Ok as u8); assert_eq!( test_ctx.dev.inner.lock().unwrap().promisc, - PromiscLevel::AllMulti + PromiscLevel::None ); - // Class-wide flags: ALL_MULTICAST raises multicast promiscuity, - // PROMISCUOUS raises full promiscuity, and disabling each falls - // back to the empty-table lower bound rather than classified - // delivery. - for (cmd, level) in [ - (control::RxCmd::AllMulticast, PromiscLevel::AllMulti), - (control::RxCmd::Promisc, PromiscLevel::All), - ] { - let ack = driver.ctrl_rx_cmd(cmd, true); - assert_eq!(ack, control::Ack::Ok as u8); - assert_eq!(test_ctx.dev.inner.lock().unwrap().promisc, level); - let ack = driver.ctrl_rx_cmd(cmd, false); - assert_eq!(ack, control::Ack::Ok as u8); - assert_eq!( - test_ctx.dev.inner.lock().unwrap().promisc, - PromiscLevel::AllMulti - ); - } - // A malformed enable byte is rejected. let ack = driver.send_ctrl_cmd(0, control::RxCmd::Promisc as u8, &[2]); assert_eq!(ack, control::Ack::Err as u8); @@ -3347,8 +3531,8 @@ mod test { /// filter state to the next driver, which may not negotiate CTRL_RX at /// all. fn control_rx_reset_clears_filters(test_ctx: TestCtx) -> TestCtx { - if cfg!(feature = "falcon") { - // Falcon links pin their promiscuity, see `control_rx_filtering`. + if test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, see `control_rx_filtering`. return test_ctx; } @@ -3383,6 +3567,7 @@ mod test { assert!(state.multicast_mac_filters.is_empty()); assert_eq!(state.promisc, PromiscLevel::AllMulti); assert!(!state.mac_filters_installed); + assert!(!state.mac_table_set); } // The next driver comes up without CTRL_RX and keeps that default. @@ -3402,8 +3587,8 @@ mod test { /// flags and MAC tables) is carried in the payload and the target /// re-derives the kernel configuration from it. fn control_rx_migration(test_ctx: TestCtx) -> TestCtx { - if cfg!(feature = "falcon") { - // Falcon links pin their promiscuity, see `control_rx_filtering`. + if test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, see `control_rx_filtering`. return test_ctx; } @@ -3438,6 +3623,7 @@ mod test { assert!(state.filter.contains(FilterState::ALL_MULTICAST)); assert_eq!(&*state.multicast_mac_filters, &mcast[..]); assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(state.mac_table_set); } // The imported state must be live, not just carried data. Dropping @@ -3457,6 +3643,34 @@ mod test { } } + // An accepted empty table narrows delivery without leaving any + // evidence within the tables themselves, so only the payload marker + // distinguishes this guest from one that never issued a + // MAC_TABLE_SET. + let ack = driver.ctrl_mac_table_set(&[], &[]); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().promisc, + PromiscLevel::None + ); + + let drv_state = driver.export(); + let test_ctx = test_ctx.migrate(); + let driver = VirtioNetDriver::import( + &test_ctx.machine, + &test_ctx.dev, + drv_state, + ); + assert!(driver.status_ok()); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.unicast_mac_filters.is_empty()); + assert!(state.multicast_mac_filters.is_empty()); + assert!(state.mac_table_set); + assert_eq!(state.promisc, PromiscLevel::None); + assert!(!state.mac_filters_installed); + } + test_ctx } diff --git a/tools/check_headers b/tools/check_headers index a0b630de0..316441866 100755 --- a/tools/check_headers +++ b/tools/check_headers @@ -16,7 +16,7 @@ set -e # # Restore to "stlouis" once that change integrates. Note: this ref names a # specific patchset and must be bumped if a new patchset is uploaded. -HEADER_CHECK_REF="refs/changes/75/775/3" +HEADER_CHECK_REF="refs/changes/75/775/4" # Directories with `ctest2`-based `header-check` crates. This list should track # the similar exclusions in `Cargo.toml`, and are described more there.