viona: support (enough) VIRTIO_NET_F_CTRL_RX to leave all-multicast mode (#1125)

* viona: support (enough) `VIRTIO_NET_F_CTRL_RX` to remove promisc

Closes #1122.

* Actually get it all working.

Well, that was an adventure.

Closes 1127.

* Actually move filter tables and associated state during migration.

* Review feedback.

* Clippy..

* Debugging...

* Got it.

* Ensure we unset the control queue if we re-init without CTRL_VQ

* do reset is_control with the rest of the queue

having `is_control` survive across reset is benign but might be
confusing for debugging: after a device reset the previous control
queue(s[^1]) remains "is_control: true" all the way until a VirtIO
driver negotiates features and has prompted us to set new control
queues. In the case of viona, if we tried to manage (say, pause) queues
before feature negotiation was complete, we've left a more complicated
situation to think through!

instead, we can clear `is_control` on reset, but doing so exposes a bug.
reset_queues_locked() would drive the queue state change *after*
clearing queue state, but queue_change() itself uses queue state to
operate correctly. if queue_change() wanted to tear down Propolis-side
bookkeeping (or debugging, logging, ...) we'll have removed necessary
data before giving it a chance to do its job.

so, `VirtQueue::reset` should come *after* a successful queue_change().
this has the mild benefit(?) of if we've failed to reset the queue we'll
leave it as it was. might be helpful for debugging the erroneous case?

* drivers which request their own MAC and others do need promisc

* `needed_promisc` should treat an empty unicast list as non-promisc.

* Missed a multiqueue bug...

* Fixup iopair calculation during import.

* Test that `num_queues` is static, device control queue is correct

---------

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

Author: FelixMcFelix

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

@@ -21,6 +21,8 @@ pub const VIRTIO_NET_F_STATUS: u64 = 1 << 16;
 pub const VIRTIO_NET_F_CTRL_VQ: u64 = 1 << 17;
 pub const VIRTIO_NET_F_CTRL_RX: u64 = 1 << 18;
 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;
 
 // virtio-block feature bits

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

@@ -530,7 +530,14 @@ impl PciVirtioState {
                 ro.write_u16(state.msix_cfg_vec);
             }
             CommonConfigReg::NumQueues => {
-                ro.write_u16(self.queues.count().get());
+                // This is the maximum count of *non-administration* virtqueues
+                // supported by the device. As we do not support
+                // `VIRTIO_F_ADMIN_VQ`, this is all of the virtqueues we have
+                // allocated.
+                ro.write_u16(
+                    u16::try_from(self.queues.max_capacity())
+                        .expect("must have fewer than u16::MAX queues"),
+                );
             }
             CommonConfigReg::DeviceStatus => {
                 let state = self.state.lock().unwrap();
@@ -1161,10 +1168,13 @@ impl PciVirtioState {
         state: &mut MutexGuard<VirtioState>,
     ) {
         for queue in self.queues.iter_all() {
-            queue.reset();
+            // We must queue_change() before the reset: the device may use
+            // information from the queue in handling the state transition, so
+            // give it an opportunity to use that data as part of a reset.
             if dev.queue_change(queue, VqChange::Reset).is_err() {
                 self.needs_reset_locked(dev, state);
             }
+            queue.reset();
         }
     }
 

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

@@ -2,6 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
+use std::io::Error as IoError;
 use std::mem;
 use std::num::{NonZeroU16, Wrapping};
 use std::sync::atomic::{fence, AtomicBool, AtomicUsize, Ordering};
@@ -192,10 +193,11 @@ impl VqUsed {
         mem.write(self.gpa_flags, &value);
     }
 
-    /// Disables notifications on this queue; returns the previous state.
+    /// Disables notifications on this queue; returns whether notfications were
+    /// enabled before.
     fn disable_notify(&self, mem: &MemCtx) -> bool {
         let flags = self.flags(mem);
-        let current = flags.contains(UsedFlags::NO_NOTIFY);
+        let current = !flags.contains(UsedFlags::NO_NOTIFY);
         self.set_flags(flags | UsedFlags::NO_NOTIFY, mem);
         current
     }
@@ -326,6 +328,7 @@ impl VirtQueue {
         used.reset();
         self.live.store(false, Ordering::Release);
         self.enabled.store(false, Ordering::Release);
+        self.is_control.store(false, Ordering::Release);
     }
 
     pub(super) fn enable(&self) {
@@ -348,8 +351,8 @@ impl VirtQueue {
         self.is_control.load(Ordering::Acquire)
     }
 
-    pub(super) fn set_control(&self) {
-        self.is_control.store(true, Ordering::Release);
+    fn set_control(&self, val: bool) {
+        self.is_control.store(val, Ordering::Release);
     }
 
     #[inline(always)]
@@ -564,7 +567,9 @@ impl VirtQueue {
         used.interrupt.as_ref().map(|x| x.read())
     }
 
-    /// Disables interrupts (notifications) on the `Used` ring
+    /// Disables interrupts (notifications) on the `Used` ring.
+    ///
+    /// Returns `true` if notifications were previously enabled.
     pub(super) fn disable_intr(&self, mem: &MemCtx) -> bool {
         let used = self.used.lock().unwrap();
         used.disable_notify(mem)
@@ -786,6 +791,66 @@ impl Chain {
         });
         total == item_sz
     }
+
+    pub fn read_many_owned<T: Copy + FromBytes>(
+        &mut self,
+        mem: &MemCtx,
+        n_elements: usize,
+    ) -> Result<Vec<T>, IoError> {
+        let required_sz = mem::size_of::<T>() * n_elements;
+        if (self.read_stat.bytes_remain as usize) < required_sz {
+            return Err(IoError::new(
+                std::io::ErrorKind::InvalidData,
+                "Buffer too small",
+            ));
+        }
+
+        let mut vec = Vec::with_capacity(n_elements);
+
+        if n_elements == 0 {
+            return Ok(vec);
+        }
+
+        // SAFETY: a `u8` has no alignment requirement so it is safe to
+        // construct a byte slice from another Vec. These bytes will not be read
+        // before initialisation.
+        let raw = unsafe {
+            std::slice::from_raw_parts_mut(
+                vec.spare_capacity_mut().as_ptr() as *mut u8,
+                required_sz,
+            )
+        };
+
+        let mut done = 0;
+        let total = self.for_remaining_type(true, |addr, len| {
+            let mut remain = GuestData::from(&mut raw[done..]);
+            if let Some(copied) = mem.read_into(addr, &mut remain, len) {
+                let need_more = copied != remain.len();
+
+                done += copied;
+                (copied, need_more)
+            } else {
+                // Copy failed, so do not attempt anything else
+                (0, false)
+            }
+        });
+
+        if total == required_sz {
+            // SAFETY: read_many() was successful and just initialized all
+            // `n_elements` of the vector.
+            unsafe {
+                vec.set_len(n_elements);
+            }
+
+            Ok(vec)
+        } else {
+            Err(IoError::new(
+                std::io::ErrorKind::UnexpectedEof,
+                "Insufficient bytes to complete read",
+            ))
+        }
+    }
+
     /// Fetch a string of readable guest regions from the chain, provided there
     /// are enough to cover a specified length.
     pub fn readable_bufs(&mut self, len: usize) -> Option<Vec<GuestRegion>> {
@@ -808,6 +873,7 @@ impl Chain {
         assert_eq!(remain, 0);
         Some(bufs)
     }
+
     pub fn write<T: Copy + IntoBytes>(
         &mut self,
         item: &T,
@@ -1030,8 +1096,19 @@ impl VirtQueues {
         Ok(())
     }
 
+    pub fn set_ctl_queues(&self, indices: &[u16]) -> Result<(), ()> {
+        for vq in &self.queues {
+            vq.set_control(false);
+        }
+        for i in indices {
+            let vq = self.queues.get(usize::from(*i)).ok_or(())?;
+            vq.set_control(true);
+        }
+        Ok(())
+    }
+
     pub fn count(&self) -> NonZeroU16 {
-        NonZeroU16::try_from(self.len() as u16)
+        NonZeroU16::try_from(self.iter().count() as u16)
             .expect("queue count already validated")
     }
 
@@ -1055,34 +1132,31 @@ impl VirtQueues {
     pub fn get(&self, qid: u16) -> Option<&Arc<VirtQueue>> {
         let len = self.len();
         let qid = usize::from(qid);
-        // XXX: This special case is for the virtio network device, which always
-        // puts the control queue at the end of queue vector (see VirtIO 1.2
-        // section 5.1.2).  None of the other devices currently handle queues
-        // specially in this way, but we should come up with some better
-        // mechanism here.
-        if qid + 1 == len {
-            Some(self.get_control())
-        } else {
-            self.queues[..len].get(qid)
-        }
-    }
 
-    fn get_control(&self) -> &Arc<VirtQueue> {
-        &self.queues[self.max_capacity() - 1]
+        // Control queues may be placed almost arbitrarily depending on the
+        // device type -- they may be mixed in with other queues, or placed at
+        // the very end after preallocated but unused queues.
+        self.queues.get(qid).filter(|v| v.is_control() || qid < len)
     }
 
     pub fn iter(&self) -> impl std::iter::Iterator<Item = &Arc<VirtQueue>> {
-        let len = self.len() - 1;
-        self.queues[..len].iter().chain([self.get_control()])
+        let len = self.len();
+        self.queues
+            .iter()
+            .enumerate()
+            .filter_map(move |(i, v)| (i < len || v.is_control()).then_some(v))
     }
 
     /// Iterate all queues the device may have used; the current number of
     /// VirtQueues may be lower than a previous high watermark, but in cases
     /// like device reset and teardown we must manage all viona rings
     /// corresponding to ever-active VirtQueues.
     pub fn iter_all(&self) -> impl std::iter::Iterator<Item = &Arc<VirtQueue>> {
-        let peak = self.peak() - 1;
-        self.queues[..peak].iter().chain([self.get_control()])
+        let peak = self.peak();
+        self.queues
+            .iter()
+            .enumerate()
+            .filter_map(move |(i, v)| (i < peak || v.is_control()).then_some(v))
     }
 
     pub fn export(&self) -> migrate::VirtQueuesV1 {