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/Cargo.lock b/Cargo.lock index 9efee6dcb..e6f5e29e1 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", @@ -8806,7 +8806,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 2ade0febf..a527ad5a7 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" } @@ -148,7 +148,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" diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 0da580375..2608429dd 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -1032,6 +1032,8 @@ impl MachineInitializer<'_> { // - Geneve: 8–16 (due to options) // - (and then round up to nearest 8) header_pad: 80, + // Remains "off" until Nexus supplies an allowed-MAC policy. + allow_guest_mac_change: false, }); let viona = virtio::PciVirtioViona::new( diff --git a/bin/propolis-standalone/src/config.rs b/bin/propolis-standalone/src/config.rs index 2280a6399..ee54740e0 100644 --- a/bin/propolis-standalone/src/config.rs +++ b/bin/propolis-standalone/src/config.rs @@ -154,6 +154,7 @@ struct MemAsyncConfig { pub struct VionaDeviceParams { tx_copy_data: Option, tx_header_pad: Option, + allow_guest_mac_change: Option, } impl VionaDeviceParams { pub fn from_opts( @@ -164,12 +165,16 @@ impl VionaDeviceParams { let parsed: Self = opt_deser(opts)?; let out = if parsed.tx_copy_data.is_some() || parsed.tx_header_pad.is_some() + || parsed.allow_guest_mac_change.is_some() { let default = DeviceParams::default(); Some(DeviceParams { copy_data: parsed.tx_copy_data.unwrap_or(default.copy_data), header_pad: parsed.tx_header_pad.unwrap_or(default.header_pad), + allow_guest_mac_change: parsed + .allow_guest_mac_change + .unwrap_or(default.allow_guest_mac_change), }) } else { None diff --git a/crates/viona-api/src/ffi.rs b/crates/viona-api/src/ffi.rs index e67f97d11..611cdd098 100644 --- a/crates/viona-api/src/ffi.rs +++ b/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,114 @@ 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 a compact array of +/// [`vmf_nmcast`] Ethernet addresses, [`ETHERADDRL`] bytes per entry +/// with no padding between them. +/// +/// Both ioctls return zero whenever this result struct is copied out, +/// with success or failure reported through [`vmf_err`]. A nonzero +/// return means no result was returned at all. +/// +/// 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. A count of zero queries the installed count alone, with the +/// buffer left unread. +/// +/// [`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. The ioctl returns zero +/// whenever this result struct is copied out, with success or failure +/// reported through [`vma_err`], and [`vma_present`] recording whether +/// an address remains installed, as a failed restoration leaves none. A +/// nonzero return means no result was returned at all. +/// +/// Viona performs the swap as directed. The consumer must validate the +/// requested address against host policy before exposing the swap to a +/// guest, as neither the ability to issue the ioctl nor link protections +/// such as [`mac-nospoof`] stand in for that validation. +/// +/// 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 +/// [`mac-nospoof`]: https://github.com/illumos/illumos-gate/blob/d8b08b811c1375eb45accfc2aa105f39f967d364/usr/src/lib/libdladm/common/linkprop.c#L469-L474 +#[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 +266,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/bits.rs b/lib/propolis/src/hw/virtio/bits.rs index a7372886a..2f3938348 100644 --- a/lib/propolis/src/hw/virtio/bits.rs +++ b/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; diff --git a/lib/propolis/src/hw/virtio/mod.rs b/lib/propolis/src/hw/virtio/mod.rs index de50df68f..7b9687d1a 100644 --- a/lib/propolis/src/hw/virtio/mod.rs +++ b/lib/propolis/src/hw/virtio/mod.rs @@ -228,6 +228,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/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs index 66546c543..8804f0d68 100644 --- a/lib/propolis/src/hw/virtio/softnpu.rs +++ b/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 = 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, 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 +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, - pipeline: &mut Box, + pipeline: &Mutex>, 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. diff --git a/lib/propolis/src/hw/virtio/viona.rs b/lib/propolis/src/hw/virtio/viona.rs index 4f2dc8ea6..81546c741 100644 --- a/lib/propolis/src/hw/virtio/viona.rs +++ b/lib/propolis/src/hw/virtio/viona.rs @@ -75,6 +75,19 @@ enum MqSetPairsCause { mod probes { fn virtio_viona_mq_set_use_pairs(cause: u8, npairs: u16) {} fn virtio_viona_cq_request(class: u8, command: u8) {} + /// A failed `VNA_IOC_SET_MAC_FILTERS`, with the semantic code from + /// `vmf_err`, the offending address (`vmf_erraddr`, low 48 bits), and + /// the copied-out entry count (the device capacity on `VMF_ERR_COUNT`). + /// + /// A code of zero (`VMF_OK`) means the ioctl itself failed without + /// returning a result, leaving the other arguments as submitted. + fn virtio_viona_mac_filters_err(err: u32, addr: u64, nmcast: u32) {} + /// A failed `VNA_IOC_SET_MAC_ADDR`, with the semantic code from + /// `vma_err` and whether a unicast address remains installed. + /// + /// A code of zero (`VMA_OK`) means the ioctl itself failed without + /// returning a result. + fn virtio_viona_mac_swap_err(err: u32, present: u8) {} } /// Types and so forth for supporting the control queue. @@ -192,10 +205,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,10 +234,35 @@ pub mod control { return Err(()); } - chain - .read_many_owned(mem, entry_count as usize) - .map_err(|_| ()) - .map(Vec::into_boxed_slice) + // 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(()); + } + + 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 } } @@ -222,6 +276,24 @@ 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, + /// The unicast MAC address installed by the driver in place of the + /// nominal one the device was created with, if any + /// (`VIRTIO_NET_CTRL_MAC_ADDR_SET`). + /// + /// This is optional for compatibility, as older sources omit it. + /// + /// An older destination ignores it and reverts config space to the + /// nominal address, while the guest continues using the replaced + /// one, which that host will not classify. + #[serde(default)] + pub mac_addr: Option, } impl Schema<'_> for VionaStateV1 { @@ -323,6 +395,20 @@ 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` 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, + install_filters: bool, +} + struct Inner { poller: Option, iop_state: Option, @@ -333,6 +419,24 @@ struct Inner { filter: FilterState, unicast_mac_filters: Box<[MacAddr]>, multicast_mac_filters: Box<[MacAddr]>, + /// 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, + /// The unicast MAC address installed by the driver through + /// `VIRTIO_NET_CTRL_MAC_ADDR_SET`, when it has replaced the nominal + /// [`PciVirtioViona::mac_addr`] the device was created with. + mac_override: Option, } impl Inner { fn new(max_queues: usize, promisc: PromiscLevel) -> Self { @@ -349,6 +453,10 @@ impl Inner { filter: FilterState::empty(), unicast_mac_filters: Box::new([]), multicast_mac_filters: Box::new([]), + mac_filters_installed: false, + mac_filters_dirty: false, + mac_table_set: false, + mac_override: None, } } @@ -383,6 +491,15 @@ pub struct DeviceParams { /// This parameter requires [viona_api::ApiVersion::V3] or greater. This is /// before Propolis' minimum viona API version and can always be set. pub header_pad: u16, + + /// Permit the guest to replace the device's unicast MAC address with a + /// unicast address of its choosing (`VIRTIO_NET_CTRL_MAC_ADDR_SET`). + /// + /// This is a propolis-side policy knob rather than a kernel parameter, as + /// viona performs the swap as directed and leaves validating the requested + /// address against host policy to its consumer. When unset, the device does + /// not advertise `VIRTIO_NET_F_CTRL_MAC_ADDR`. + pub allow_guest_mac_change: bool, } impl DeviceParams { #[cfg(target_os = "illumos")] @@ -412,8 +529,9 @@ impl DeviceParams { impl Default for DeviceParams { fn default() -> Self { // Viona (as of V3) allocs/copies entire packet by default, with no - // padding added to the header. - Self { copy_data: true, header_pad: 0 } + // padding added to the header. Guest MAC replacement requires an + // explicit opt-in. + Self { copy_data: true, header_pad: 0, allow_guest_mac_change: false } } } @@ -425,7 +543,9 @@ pub enum PromiscLevel { /// The device should receive only packets for its installed MAC /// filters. /// - /// Today this allows solely [`PciVirtioViona::mac_addr`]. + /// That is, the active unicast address (nominally + /// [`PciVirtioViona::mac_addr`], unless replaced by the driver) and any + /// installed multicast filter table. #[default] None, /// The device should receive all multicast traffic in addition @@ -483,9 +603,35 @@ 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]) + } +} + +/// Outcome of a failed `VNA_IOC_SET_MAC_ADDR` swap, decoded from the +/// semantic error code and address presence copied out through +/// `vioc_mac_addr`. +#[derive(Copy, Clone, Debug)] +enum MacSwapError { + /// The new address is active, but the multicast filter table behind it + /// was only partially reinstalled (`VMA_ERR_MCAST_RESTORE`). + McastRestore, + /// The swap failed with the previous address left installed, either + /// reported outright (`vma_present` set) or inferred from an ioctl + /// error with no result returned, meaning the kernel processed + /// nothing at all. The kernel may have shed entries from any + /// installed filter table along the way. + Unchanged, + /// The swap failed and no unicast address remains on the client, a + /// state only full promiscuity can cover. + NoUnicast, } /// Represents a connection to the kernel's Viona (VirtIO Network Adapter) @@ -498,6 +644,21 @@ pub struct PciVirtioViona { dev_features: u64, mac_addr: MacAddr, mtu: Option, + /// Whether the viona device supports the MAC filtering ioctls of + /// [`viona_api::ApiVersion::V7`]: `VNA_IOC_SET_MAC_FILTERS` and + /// `VNA_IOC_SET_MAC_ADDR`. + kernel_mac_filters: bool, + /// Policy gate for guest-initiated MAC replacement. + /// + /// See [`DeviceParams::allow_guest_mac_change`]. + allow_guest_mac_change: bool, + /// 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: Option, hdl: VionaHdl, inner: Mutex, } @@ -535,26 +696,37 @@ 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) => { - // 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. - eprintln!( - "failed to enable promisc mode on {vnic_name}: {e:?}" - ); - PromiscLevel::AllMulti - } - }; + 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)?; } + 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 @@ -579,7 +751,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); @@ -599,6 +771,11 @@ impl PciVirtioViona { dev_features, mac_addr: info.mac_addr.into(), mtu: info.mtu, + kernel_mac_filters: api_version >= viona_api::ApiVersion::V7, + allow_guest_mac_change: viona_params + .is_some_and(|vp| vp.allow_guest_mac_change), + #[cfg(feature = "falcon")] + pinned_promisc, hdl, inner: Mutex::new(Inner::new(nqueues, promisc_level)), }; @@ -711,8 +888,13 @@ impl PciVirtioViona { self.set_mac_filters(unicast, multicast) } - // We do not advertise `VIRTIO_NET_F_CTRL_MAC_ADDR` - MacCmd::AddrSet => return Err(()), + MacCmd::AddrSet => { + let mut mac = MacAddr::default(); + if !chain.read(&mut mac, mem) { + return Err(()); + } + self.set_mac_override(mac) + } } } @@ -725,7 +907,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(()) } @@ -758,7 +940,10 @@ impl PciVirtioViona { fn net_cfg_read(&self, id: &NetReg, ro: &mut ReadOp) { match id { - NetReg::Mac => ro.write_bytes(&self.mac_addr.0), + NetReg::Mac => { + let state = self.inner.lock().unwrap(); + ro.write_bytes(&self.current_mac(&state).0); + } NetReg::Status => { // Always report link up ro.write_u16(VIRTIO_NET_S_LINK_UP); @@ -952,10 +1137,19 @@ 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. + let needs_reset = self.apply_rx_config(&mut state).is_err(); + drop(state); + if needs_reset { + self.virtio_state.set_needs_reset(self); + } Err(()) } } @@ -983,12 +1177,119 @@ 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.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; + 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. + let needs_reset = self.apply_rx_config(&mut state).is_err(); + drop(state); + if needs_reset { + self.virtio_state.set_needs_reset(self); + } + Err(()) + } + } + } + + /// Replace the device's active unicast MAC address on behalf of a + /// driver's `VIRTIO_NET_CTRL_MAC_ADDR_SET` command. + /// + /// The comments below distinguish the nominal address, the + /// [`PciVirtioViona::mac_addr`] the device was created with, from a + /// driver-installed override. + fn set_mac_override(&self, mac: MacAddr) -> Result<(), ()> { + // The policy is checked directly rather than through feature + // negotiation alone: the bit cannot be negotiated without the + // grant, but the swap must not depend on that indirection. + if !self.allow_guest_mac_change + || (self.virtio_state.negotiated_features() + & VIRTIO_NET_F_CTRL_MAC_ADDR) + == 0 + || !mac.is_unicast() + { + return Err(()); + } + + let mut state = self.inner.lock().unwrap(); + if mac == self.current_mac(&state) { + return Ok(()); + } + + // The kernel removes the current address (and with it, the installed + // multicast filters) before installing the replacement, meaning that + // full promiscuity (or the superset pin) must cover delivery across the + // swap. + let cover = self.pinned_promisc().unwrap_or(PromiscLevel::All); + if self.set_promisc(cover, &mut state).is_err() { + return Err(()); + } + + let res = match self.hdl.set_mac_addr(mac) { + // A clean swap reinstalled the multicast table as well. Only + // the promiscuity level remains to be reconciled below. + Ok(()) => { + state.mac_override = Some(mac); + Ok(()) + } + // The new address is active, but the table behind it was only + // partially reinstalled. We mark it dirty so the reconciliation + // below reissues it. + Err(MacSwapError::McastRestore) => { + state.mac_override = Some(mac); + state.mac_filters_dirty = state.mac_filters_installed; + Ok(()) + } + // The command failed and the previous address, nominal or + // override, is still installed, so `mac_override` is left + // as-is. + // + // A failed swap can still shed table entries, as the kernel + // removes and reinstalls the table around restoring the + // previous address, and entries refused during reinstallation + // are dropped without report. + // + // Hence, we reissue the table. + Err(MacSwapError::Unchanged) => { + state.mac_filters_dirty = state.mac_filters_installed; + Err(()) + } + // The client holds no unicast address, which only the full + // promiscuity raised above covers, and config space now names + // an address the kernel lost. + // + // We record the override even though the kernel holds nothing: the + // restore path of device_reset engages only when one is tracked, and + // reissuing the nominal address there recovers a client without + // one. + // + // Leave the raised level in force and demand a reset. + Err(MacSwapError::NoUnicast) => { + state.mac_override = Some(mac); + state.mac_filters_dirty = state.mac_filters_installed; + drop(state); + self.virtio_state.set_needs_reset(self); + return Err(()); + } + }; + + // Reconcile promiscuity (and any dirtied table) with the filter + // state. + match self.apply_rx_config(&mut state) { + Ok(()) => res, + Err(()) => { + drop(state); + self.virtio_state.set_needs_reset(self); Err(()) } } @@ -1000,10 +1301,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; @@ -1011,56 +1308,182 @@ 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(()) } } } - /// 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 active unicast MAC address: the override installed by the guest + /// driver, or the nominal address the device was created with. + fn current_mac(&self, state: &Inner) -> MacAddr { + state.mac_override.unwrap_or(self.mac_addr) + } + + /// 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; + { + 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 // 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.current_mac(state)); let need_promisc = state.filter.contains(FilterState::PROMISCUOUS) || !filter_is_self; - if need_promisc { + // 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 { + } else if need_mcast_promisc || !state.mac_table_set { PromiscLevel::AllMulti } else { PromiscLevel::None + }; + + RxConfig { + promisc, + // 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(), } } + + /// 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, 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 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 + // 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; + state.mac_filters_dirty = true; + match target.promisc { + PromiscLevel::None => PromiscLevel::AllMulti, + covered => covered, + } + } + } + } else { + target.promisc + }; + + self.set_promisc(promisc, state)?; + + // 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(()) + } } impl VirtioDevice for PciVirtioViona { fn rw_dev_config(&self, mut rwo: RWOp) { @@ -1071,8 +1494,8 @@ impl VirtioDevice for PciVirtioViona { // // Technically while we are in either `Mode::Transitional` // or `Mode::Legacy` the driver may write to `NetReg::Mac` - // in lieu of sending a `MacCmd::AddrSet`. We don't support - // changing the MAC address today. + // in lieu of sending a `MacCmd::AddrSet`. We support only + // the control-queue `AddrSet` today. } }); } @@ -1095,6 +1518,17 @@ impl VirtioDevice for PciVirtioViona { } feat |= self.dev_features; + // The address swap behind `MacCmd::AddrSet` requires + // `VNA_IOC_SET_MAC_ADDR` (viona API V7) and an explicit policy + // grant (`allow_guest_mac_change`). Apply the policy after merging + // kernel-advertised features so a future kernel capability cannot + // bypass the propolis-side opt-out. + if self.kernel_mac_filters && self.allow_guest_mac_change { + feat |= VIRTIO_NET_F_CTRL_MAC_ADDR; + } else { + feat &= !VIRTIO_NET_F_CTRL_MAC_ADDR; + } + feat } @@ -1123,11 +1557,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 +1642,70 @@ 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([]); + state.mac_table_set = false; + state.mac_filters_dirty = false; + + // A driver-installed unicast address must not outlive the reset + // either. As in set_mac_override, the swap back to the nominal address + // runs under full promiscuity (or the superset pin), which the + // restore below then lowers. A failure can leave the client with no + // unicast address, so we keep the raised level active and flag the + // device, as with a failed restore. + if state.mac_override.is_some() { + let cover = self.pinned_promisc().unwrap_or(PromiscLevel::All); + // A partially reinstalled table (McastRestore) still leaves the + // nominal address active, and the table is cleared below anyway. + let restored = self.set_promisc(cover, &mut state).is_ok() + && !matches!( + self.hdl.set_mac_addr(self.mac_addr), + Err(MacSwapError::Unchanged | MacSwapError::NoUnicast) + ); + if !restored { + // The nominal address may not be installed. `mac_override` is + // kept, as the kernel most likely still holds the override, + // and the raised promiscuity covers delivery either way + // until the demanded reset lands. + state.mac_filters_dirty = state.mac_filters_installed; + drop(state); + self.virtio_state.set_needs_reset(self); + return; + } + state.mac_override = None; + } + + // 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() { + state.mac_filters_dirty = state.mac_filters_installed; + drop(state); + self.virtio_state.set_needs_reset(self); + return; + } + + if state.mac_filters_installed { + if self.hdl.set_mac_filters(&[]).is_ok() { + state.mac_filters_installed = false; + } else { + state.mac_filters_dirty = true; + } + } + } } impl Lifecycle for PciVirtioViona { fn type_name(&self) -> &'static str { @@ -1224,15 +1717,16 @@ 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(); - - 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 +1801,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(); @@ -1315,6 +1811,8 @@ 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), + mac_addr: state.mac_override, } }; @@ -1329,6 +1827,20 @@ impl MigrateMulti for PciVirtioViona { ::import(self, offer, ctx)?; let feat = self.virtio_state.negotiated_features(); + + // A source that negotiated CTRL_MAC_ADDR granted its guest MAC + // replacement, a policy this host must extend as well because the + // guest keeps the negotiated bit and may issue further swaps here. + if (feat & VIRTIO_NET_F_CTRL_MAC_ADDR) != 0 + && !self.allow_guest_mac_change + { + return Err(MigrateStateError::ImportFailed( + "source negotiated VIRTIO_NET_F_CTRL_MAC_ADDR but guest \ + MAC replacement is not permitted on this host" + .to_string(), + )); + } + self.hdl.set_features(feat).map_err(|e| { MigrateStateError::ImportFailed(format!( "error while setting viona features ({feat:x}): {e:?}" @@ -1337,7 +1849,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 = @@ -1358,23 +1875,110 @@ 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 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, + mac_addr, + } = 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(); - self.set_promisc(input.promisc, &mut state).map_err(|_| { + state.filter = FilterState::from_bits(filter).ok_or_else(|| { MigrateStateError::ImportFailed(format!( - "Could not move device promisc level to {:?}.", - input.promisc + "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(), + ); + + // A driver-installed unicast address must be active on this host + // as well. As in set_mac_override, full promiscuity (or the superset + // pin) covers the swap, and the reconciliation below settles the + // final level. + if let Some(mac) = mac_addr { + if !self.kernel_mac_filters { + return Err(MigrateStateError::ImportFailed( + "cannot restore driver-installed MAC address without \ + VNA_IOC_SET_MAC_ADDR support" + .to_string(), + )); + } + // An override can outlive the negotiated bit: a failed nominal + // restore during reset retains it after the features are + // cleared. + // + // The feature gate above therefore does not cover this + // payload, so the policy applies to it directly. + if !self.allow_guest_mac_change { + return Err(MigrateStateError::ImportFailed( + "source carried a driver-installed MAC address but \ + guest MAC replacement is not permitted on this host" + .to_string(), + )); + } + let cover = self.pinned_promisc().unwrap_or(PromiscLevel::All); + self.set_promisc(cover, &mut state).map_err(|_| { + MigrateStateError::ImportFailed( + "could not raise promiscuity to cover MAC address \ + replacement" + .to_string(), + ) + })?; + + self.hdl.set_mac_addr(mac).or_else(|e| match e { + // McastRestore still installs the address, as the table behind + // it was only partially reinstalled, and the reconciliation + // below reissues it from the imported (already dirty) state. + MacSwapError::McastRestore => Ok(()), + e => Err(MigrateStateError::ImportFailed(format!( + "error while restoring driver-installed MAC \ + address: {e:?}" + ))), + })?; + state.mac_override = Some(mac); + } + + // 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(), + ) }) } } @@ -1674,6 +2278,142 @@ 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 also 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 table is passed out-of-band through `vmf_addrs`, so the backing + /// array must outlive the ioctl. + /// + /// The semantic code copied out through `vmf_err` is checked against + /// `VMF_OK` to detect failure, but the individual codes are + /// deliberately not distinguished. The count is checked against the + /// device capacity before this call, non-multicast entries are + /// rejected at the control queue, and every remaining failure is + /// answered the same way: by falling back to multicast promiscuity in + /// [`PciVirtioViona::apply_rx_config`]. + /// + /// Diagnosis stays with the `virtio_viona_mac_filters_err` probe below, + /// which carries the code and the offending address. + /// + /// 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 addrs: Vec = multicast + .iter() + .filter(|mac| !mac.is_broadcast()) + .copied() + .collect(); + + let mut vmf = viona_api::vioc_mac_filters { + vmf_nmcast: addrs.len() as u32, + // A count of zero clears the table without the kernel reading + // the buffer. We pass no pointer rather than a dangling one. + vmf_addrs: if addrs.is_empty() { + 0 + } else { + addrs.as_bytes().as_ptr() as u64 + }, + ..Default::default() + }; + + // A zero return carries the semantic outcome in vmf_err; a nonzero + // return means the kernel never processed the request and copied + // nothing out. Either way, a failure. + unsafe { self.0.ioctl(viona_api::VNA_IOC_SET_MAC_FILTERS, &mut vmf) } + .and_then(|rv| match vmf.vmf_err { + viona_api::VMF_OK => Ok(rv), + _ => Err(io::ErrorKind::InvalidInput.into()), + }) + .inspect_err(|_| { + probes::virtio_viona_mac_filters_err!(|| { + let mut addr = [0u8; size_of::()]; + addr[size_of::() - ETHERADDRL..] + .copy_from_slice(&vmf.vmf_erraddr); + (vmf.vmf_err, u64::from_be_bytes(addr), vmf.vmf_nmcast) + }); + })?; + Ok(()) + } + + /// Replace the unicast MAC address of the underlying MAC client, as + /// requested by a `VIRTIO_NET_CTRL_MAC_ADDR_SET` command. + /// + /// The caller is expected to hold `VIONA_PROMISC_ALL` across the + /// replacement, as the kernel may remove the current address (and any + /// installed multicast filters) before installing the new one, leaving a + /// classified delivery gap until the swap completes. + /// + /// Updating the `mac` field of virtio config space remains the device's + /// responsibility. + /// + /// This requires [viona_api::ApiVersion::V7] or greater. + fn set_mac_addr(&self, mac: MacAddr) -> Result<(), MacSwapError> { + let mut vma = + viona_api::vioc_mac_addr { vma_addr: mac.0, ..Default::default() }; + // A zero return carries the semantic outcome in vma_err; a nonzero + // return means the kernel never processed the request and copied + // nothing out. Either way, a failure. + unsafe { self.0.ioctl(viona_api::VNA_IOC_SET_MAC_ADDR, &mut vma) } + .and_then(|rv| match vma.vma_err { + viona_api::VMA_OK => Ok(rv), + _ => Err(io::ErrorKind::InvalidInput.into()), + }) + .map_err(|_| { + probes::virtio_viona_mac_swap_err!(|| ( + vma.vma_err, + vma.vma_present + )); + match vma.vma_err { + // vma is a stack object the kernel writes in full on + // every processed request, so VMA_OK surviving a + // failed ioctl means the request was never processed + // and the address stands as is. + viona_api::VMA_OK => MacSwapError::Unchanged, + viona_api::VMA_ERR_MCAST_RESTORE => { + MacSwapError::McastRestore + } + _ if vma.vma_present != 0 => MacSwapError::Unchanged, + _ => MacSwapError::NoUnicast, + } + })?; + Ok(()) + } + + /// Read the active unicast MAC address of the underlying MAC client, if + /// one is installed. + /// + /// This requires [viona_api::ApiVersion::V7] or greater. + #[cfg(test)] + fn get_mac_addr(&self) -> io::Result> { + let mut vma = viona_api::vioc_mac_addr::default(); + unsafe { self.0.ioctl(viona_api::VNA_IOC_GET_MAC_ADDR, &mut vma) }?; + Ok((vma.vma_present != 0).then_some(MacAddr(vma.vma_addr))) + } + + /// Read the installed multicast filter table. + /// + /// This requires [viona_api::ApiVersion::V7] or greater. + #[cfg(test)] + fn get_mac_filters(&self) -> io::Result> { + let mut addrs = + vec![MacAddr::default(); viona_api::VIONA_MAX_MCAST_FILTERS]; + let mut vmf = viona_api::vioc_mac_filters { + vmf_nmcast: addrs.len() as u32, + vmf_addrs: addrs.as_mut_bytes().as_mut_ptr() as u64, + ..Default::default() + }; + unsafe { self.0.ioctl(viona_api::VNA_IOC_GET_MAC_FILTERS, &mut vmf) }?; + // The kernel rewrites the count with the installed total, which its + // capacity keeps within the buffer above. + addrs.truncate(vmf.vmf_nmcast as usize); + Ok(addrs) + } } impl AsRawFd for VionaHdl { @@ -1913,13 +2653,15 @@ mod test { use crate::hw::pci::Bdf; use crate::hw::virtio::pci::Status; use crate::hw::virtio::viona::{ - VIRTIO_NET_F_CTRL_VQ, VIRTIO_NET_F_MAC, VIRTIO_NET_F_MQ, - VIRTIO_NET_F_STATUS, + control, DeviceParams, FilterState, MacAddr, PromiscLevel, + VIRTIO_NET_F_CTRL_MAC_ADDR, 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, + MigrateCtx, MigrateMulti, MigrateStateError, PayloadOffer, + PayloadOffers, PayloadOutputs, }; use crate::Machine; use std::env::VarError; @@ -1932,6 +2674,7 @@ mod test { vnic_name: String, machine: Machine, dev: Arc, + params: Option, } impl Drop for TestCtx { @@ -1947,6 +2690,19 @@ mod test { } fn migrate(self) -> TestCtx { + let params = self.params; + let (ctx, res) = self.migrate_to(params); + res.expect("can import PciVirtioViona"); + ctx + } + + /// Export this device, tear it down, and import the payload into a + /// fresh device built with `params`. The target is returned alongside + /// the import result so its state can be inspected after a rejection. + fn migrate_to( + self, + params: Option, + ) -> (TestCtx, Result<(), MigrateStateError>) { let mut dev_payloads = PayloadOutputs::new(); let acc_mem = self.machine.acc_mem.access().expect("machine has memory"); @@ -1993,18 +2749,23 @@ mod test { create_vnic(&underlying_nic, &vnic_name); let new_ctx = - create_test_ctx(test_name, &underlying_nic, &vnic_name); + create_test_ctx(test_name, &underlying_nic, &vnic_name, params); let acc_mem = new_ctx .machine .acc_mem .access() .expect("new machine has memory"); let new_migrate = MigrateCtx { mem: &acc_mem }; - ::import(&new_ctx.dev, &mut offers, &new_migrate) - .expect("can import PciVirtioViona"); + let res = ::import( + &new_ctx.dev, + &mut offers, + &new_migrate, + ); + // Start the target either way: a rejected import leaves the + // kernel state untouched, so it comes up as a cold boot would. Lifecycle::start(new_ctx.dev.as_ref()) .expect("can start viona device"); - new_ctx + (new_ctx, res) } } @@ -2012,6 +2773,7 @@ mod test { test_name: &'static str, underlying_nic: &str, vnic_name: &str, + params: Option, ) -> TestCtx { // Create the VM with `force: true`: if we're running tests concurrently // this will trample an existing test (which should then fail!). We do @@ -2047,7 +2809,7 @@ mod test { enable_pcie: false, }, ); - let viona_dev = PciVirtioViona::new(vnic_name, &machine.hdl, None) + let viona_dev = PciVirtioViona::new(vnic_name, &machine.hdl, params) .expect("can create test vnic"); chipset_hb.pci_attach(i440fx::DEFAULT_HB_BDF, chipset_hb.clone(), None); @@ -2064,6 +2826,7 @@ mod test { test_name, underlying_nic: underlying_nic.to_owned(), vnic_name: vnic_name.to_owned(), + params, } } @@ -2218,6 +2981,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 +3001,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 +3133,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 +3153,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 +3343,124 @@ 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"); + + 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 += + (msg.len() + 1).next_multiple_of(PAGE_SIZE) as u64; + + let acc_mem = + self.machine.acc_mem.access().expect("can access memory"); + 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 ctrl_mac_addr_set(&mut self, mac: MacAddr) -> u8 { + self.send_ctrl_cmd(1, control::MacCmd::AddrSet as u8, &mac.0) + } + + /// Read the MAC address the device presents in its config space. + fn config_mac(&self) -> MacAddr { + let mut buf = [0u8; 6]; + self.device_config.read(net_config::mac, &mut buf); + MacAddr(buf) + } } fn test_device_status_writes(test_ctx: TestCtx) -> TestCtx { @@ -2629,7 +3540,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 +3645,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 +3700,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 +3725,622 @@ 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 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; + } + + 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 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 + // 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, and the kernel compacts duplicate entries on its + // own. + // + // Installation must still succeed, however. + let with_bcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 1]), + MacAddr([0xff, 0xff, 0xff, 0xff, 0xff, 0xff]), + MacAddr([0x33, 0x33, 0, 0, 0, 1]), + ]; + 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); + // Only the first entry survives compaction: broadcast is + // omitted before the ioctl and the kernel drops the + // duplicate. + assert_eq!( + test_ctx.dev.hdl.get_mac_filters().unwrap(), + &with_bcast[..1] + ); + } else { + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + } + + // A unicast entry in the multicast list is refused at the control + // queue, as is a multicast entry in the unicast list, leaving the + // accepted table in place. + for (unicast, multicast) in + [(vec![], vec![self_mac]), (vec![mcast[0]], vec![])] + { + let ack = driver.ctrl_mac_table_set(&unicast, &multicast); + assert_eq!(ack, control::Ack::Err as u8); + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(&*state.multicast_mac_filters, &with_bcast[..]); + } + + // 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, + // 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); + 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 + ); + } + + // 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 = + 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, &mcast[..]); + } + + // 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); + // The now-empty multicast table is cleared from the kernel + // even while promiscuity holds. + assert!(!state.mac_filters_installed); + } + + // 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::None + ); + + // 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 test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, 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); + assert!(!state.mac_table_set); + } + + // 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 test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, 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); + assert!(state.mac_table_set); + } + + // 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); + } + } + + // 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 + } + + /// `VIRTIO_NET_CTRL_MAC_ADDR_SET` replaces the active unicast address + /// for the life of the driver: config space follows the replacement, + /// filter classification tracks it, and a reset restores the nominal + /// address. + fn control_mac_addr_replacement(test_ctx: TestCtx) -> TestCtx { + if test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, see `control_rx_filtering`. + return test_ctx; + } + + let nominal = test_ctx.dev.mac_addr; + + // Without CTRL_MAC_ADDR negotiated the command must be rejected, + // regardless of kernel support. + let mut driver = test_ctx.create_driver(); + driver.modern_device_init( + VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | VIRTIO_NET_F_CTRL_VQ, + ); + let replacement = MacAddr([0x02, 0x08, 0x20, 0xac, 0x70, 0x55]); + let ack = driver.ctrl_mac_addr_set(replacement); + assert_eq!(ack, control::Ack::Err as u8); + assert_eq!(driver.config_mac(), nominal); + assert!(test_ctx.dev.inner.lock().unwrap().mac_override.is_none()); + + // The feature is only advertised with kernel support for the + // address swap and the policy grant (supplied by the test harness), + // so the remainder needs a V7 kernel. + if !test_ctx.dev.kernel_mac_filters { + 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 + | VIRTIO_NET_F_CTRL_MAC_ADDR, + ); + + // A multicast address can never be a unicast replacement. + let ack = driver.ctrl_mac_addr_set(MacAddr([0x33, 0x33, 0, 0, 0, 1])); + assert_eq!(ack, control::Ack::Err as u8); + assert!(test_ctx.dev.inner.lock().unwrap().mac_override.is_none()); + + // Requesting the already-active address is a successful noop and + // installs no override. + let ack = driver.ctrl_mac_addr_set(nominal); + assert_eq!(ack, control::Ack::Ok as u8); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.mac_override.is_none()); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + } + + // A real replacement takes effect in config space, and the + // reconciliation settles back at the pre-table lower bound. + let ack = driver.ctrl_mac_addr_set(replacement); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!(driver.config_mac(), replacement); + // The kernel view must match the tracked state, not just the ack. + assert_eq!(test_ctx.dev.hdl.get_mac_addr().unwrap(), Some(replacement)); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.mac_override, Some(replacement)); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + } + + // Filter classification follows the replacement: an empty unicast + // table plus a fitting multicast table narrows to classified + // delivery against the new address. + let mcast = [ + MacAddr([0x33, 0x33, 0, 0, 0, 5]), + MacAddr([0x33, 0x33, 0, 0, 0, 6]), + ]; + let ack = driver.ctrl_mac_table_set(&[], &mcast); + 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_eq!(test_ctx.dev.hdl.get_mac_filters().unwrap(), mcast); + + // A further swap with a table installed keeps the table active + // behind the new address. + let replacement2 = MacAddr([0x02, 0x08, 0x20, 0xac, 0x70, 0x66]); + let ack = driver.ctrl_mac_addr_set(replacement2); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!(driver.config_mac(), replacement2); + assert_eq!( + test_ctx.dev.hdl.get_mac_addr().unwrap(), + Some(replacement2) + ); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.mac_override, Some(replacement2)); + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } + assert_eq!(test_ctx.dev.hdl.get_mac_filters().unwrap(), mcast); + + // A guest-initiated reset restores the nominal address along with + // the rest of the filter state. + driver.write_status(Status::RESET); + assert_eq!(driver.config_mac(), nominal); + assert_eq!(test_ctx.dev.hdl.get_mac_addr().unwrap(), Some(nominal)); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert!(state.mac_override.is_none()); + assert_eq!(state.promisc, PromiscLevel::AllMulti); + assert!(!state.mac_filters_installed); + } + assert!(test_ctx.dev.hdl.get_mac_filters().unwrap().is_empty()); + + test_ctx + } + + /// A driver-installed unicast address must survive migration: the + /// payload carries the override and the target installs it in the + /// kernel before reapplying the Rx configuration. + fn control_mac_addr_migration(test_ctx: TestCtx) -> TestCtx { + if test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, see `control_rx_filtering`. + return test_ctx; + } + if !test_ctx.dev.kernel_mac_filters { + // See `control_mac_addr_replacement`. + return test_ctx; + } + + let nominal = test_ctx.dev.mac_addr; + 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 + | VIRTIO_NET_F_CTRL_MAC_ADDR, + ); + + let replacement = MacAddr([0x02, 0x08, 0x20, 0xac, 0x70, 0x77]); + let ack = driver.ctrl_mac_addr_set(replacement); + assert_eq!(ack, control::Ack::Ok as u8); + let mcast = [MacAddr([0x33, 0x33, 0, 0, 0, 7])]; + let ack = driver.ctrl_mac_table_set(&[], &mcast); + 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()); + assert_eq!(driver.config_mac(), replacement); + // The target kernel must hold the override and the table, not just + // track them. + assert_eq!(test_ctx.dev.hdl.get_mac_addr().unwrap(), Some(replacement)); + assert_eq!(test_ctx.dev.hdl.get_mac_filters().unwrap(), mcast); + { + let state = test_ctx.dev.inner.lock().unwrap(); + assert_eq!(state.mac_override, Some(replacement)); + assert_eq!(&*state.multicast_mac_filters, &mcast[..]); + assert_eq!(state.promisc, PromiscLevel::None); + assert!(state.mac_filters_installed); + } + + // The imported override must be live: the guest can still swap + // back, which installs the nominal address as an explicit override. + let ack = driver.ctrl_mac_addr_set(nominal); + assert_eq!(ack, control::Ack::Ok as u8); + assert_eq!(driver.config_mac(), nominal); + assert_eq!( + test_ctx.dev.inner.lock().unwrap().mac_override, + Some(nominal) + ); + + test_ctx + } + + /// A host that does not grant guest MAC replacement neither advertises + /// `VIRTIO_NET_F_CTRL_MAC_ADDR` nor imports a source that negotiated + /// it. Instead, the guest would keep the bit and could issue further + /// swaps. + fn control_mac_addr_policy_denied(test_ctx: TestCtx) -> TestCtx { + if test_ctx.dev.pinned_promisc().is_some() { + // Pinned link, see `control_rx_filtering`. + return test_ctx; + } + + if !test_ctx.dev.kernel_mac_filters { + // See `control_mac_addr_replacement`. + return test_ctx; + } + + // The source (with the grant) negotiates the bit and installs an + // override. + 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 + | VIRTIO_NET_F_CTRL_MAC_ADDR, + ); + let replacement = MacAddr([0x02, 0x08, 0x20, 0xac, 0x70, 0x88]); + let ack = driver.ctrl_mac_addr_set(replacement); + assert_eq!(ack, control::Ack::Ok as u8); + + // A target without the grant must reject the import. + let no_grant = DeviceParams { + allow_guest_mac_change: false, + ..Default::default() + }; + let (test_ctx, res) = test_ctx.migrate_to(Some(no_grant)); + let err = res.expect_err("import must fail without the policy grant"); + assert!(matches!(err, MigrateStateError::ImportFailed(_))); + + // Nor does such a host advertise the feature in the first place, + // (kernel support notwithstanding). + assert_eq!(test_ctx.dev.features() & VIRTIO_NET_F_CTRL_MAC_ADDR, 0); + + test_ctx + } + // Bears an uncanny resemblance to `phd-test`... struct TestCase { name: &'static str, @@ -2846,8 +4373,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 +4396,12 @@ 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), + testcase!(control_mac_addr_replacement), + testcase!(control_mac_addr_migration), + testcase!(control_mac_addr_policy_denied), ]; let underlying_nic = match std::env::var("VIONA_TEST_NIC") { @@ -2907,8 +4440,18 @@ mod test { create_vnic(&underlying_nic, TEST_VNIC); let res = std::panic::catch_unwind(move || { - let test_ctx = - create_test_ctx(test.name, &underlying_nic, TEST_VNIC); + // Grant guest MAC replacement so the `control_mac_addr_*` + // tests can negotiate `VIRTIO_NET_F_CTRL_MAC_ADDR` + let params = DeviceParams { + allow_guest_mac_change: true, + ..Default::default() + }; + let test_ctx = create_test_ctx( + test.name, + &underlying_nic, + TEST_VNIC, + Some(params), + ); Lifecycle::start(test_ctx.dev.as_ref()) .expect("can start viona device"); let test_ctx = (test.test_fn)(test_ctx); diff --git a/scripts/viona.d b/scripts/viona.d index 04a9a5e3a..6a7fb8fbb 100755 --- a/scripts/viona.d +++ b/scripts/viona.d @@ -37,6 +37,10 @@ #define VNA_IOC_SET_MTU (VNA_IOC | 0x28) #define VNA_IOC_SET_NOTIFY_MMIO (VNA_IOC | 0x29) #define VNA_IOC_INTR_POLL_MQ (VNA_IOC | 0x2a) +#define VNA_IOC_SET_MAC_FILTERS (VNA_IOC | 0x2b) +#define VNA_IOC_GET_MAC_FILTERS (VNA_IOC | 0x2c) +#define VNA_IOC_SET_MAC_ADDR (VNA_IOC | 0x2d) +#define VNA_IOC_GET_MAC_ADDR (VNA_IOC | 0x2e) #define VNA_IOC_GET_PAIRS (VNA_IOC | 0x30) #define VNA_IOC_SET_PAIRS (VNA_IOC | 0x31) @@ -219,6 +223,57 @@ viona_ioctl:entry/arg1 == VNA_IOC_SET_NOTIFY_MMIO && arg2/ { printf("%s %x+%x\n", self->cmd, mmio->vim_address, mmio->vim_size); } +viona_ioctl:entry/arg1 == VNA_IOC_SET_MAC_FILTERS/ { + self->cmd = "SET_MAC_FILTERS"; + vmf = (vioc_mac_filters_t *)copyin(arg2, sizeof (vioc_mac_filters_t)); + printf("%s n=%u\n", self->cmd, vmf->vmf_nmcast); + self->pending = arg1; +} + +/* + * For both SET_MAC ioctls, semantic failures are reported through the + * result struct on a zero return, bypassing the generic nonzero-return + * clause at the bottom of this script. Print the copied-out result on + * every return instead. + */ +viona_ioctl:return/self->pending == VNA_IOC_SET_MAC_FILTERS/ { + self->pending = 0; + vmfr = (vioc_mac_filters_t *)copyin(self->dptr, + sizeof (vioc_mac_filters_t)); + printf("SET_MAC_FILTERS result err=%u n=%u ", + vmfr->vmf_err, vmfr->vmf_nmcast); + printf("addr=%02x:%02x:%02x:%02x:%02x:%02x\n", + vmfr->vmf_erraddr[0], vmfr->vmf_erraddr[1], vmfr->vmf_erraddr[2], + vmfr->vmf_erraddr[3], vmfr->vmf_erraddr[4], vmfr->vmf_erraddr[5]); +} + +viona_ioctl:entry/arg1 == VNA_IOC_GET_MAC_FILTERS/ { + self->cmd = "GET_MAC_FILTERS"; + printf("%s\n", self->cmd); +} + +viona_ioctl:entry/arg1 == VNA_IOC_SET_MAC_ADDR/ { + self->cmd = "SET_MAC_ADDR"; + vma = (vioc_mac_addr_t *)copyin(arg2, sizeof (vioc_mac_addr_t)); + printf("%s %02x:%02x:%02x:%02x:%02x:%02x\n", self->cmd, + vma->vma_addr[0], vma->vma_addr[1], vma->vma_addr[2], + vma->vma_addr[3], vma->vma_addr[4], vma->vma_addr[5]); + self->pending = arg1; +} + +viona_ioctl:return/self->pending == VNA_IOC_SET_MAC_ADDR/ { + self->pending = 0; + vmar = (vioc_mac_addr_t *)copyin(self->dptr, + sizeof (vioc_mac_addr_t)); + printf("SET_MAC_ADDR result err=%u present=%u\n", + vmar->vma_err, vmar->vma_present); +} + +viona_ioctl:entry/arg1 == VNA_IOC_GET_MAC_ADDR/ { + self->cmd = "GET_MAC_ADDR"; + printf("%s\n", self->cmd); +} + viona_ioctl:entry/arg1 == VNA_IOC_GET_PAIRS/ { self->cmd = "GET_PAIRS"; printf("%s\n", self->cmd); diff --git a/tools/check_headers b/tools/check_headers index ee3cbca6d..02ec4dcab 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/8" # Directories with `ctest2`-based `header-check` crates. This list should track # the similar exclusions in `Cargo.toml`, and are described more there.