virtio-nic: in-repo tests and a better device state FSM

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

@@ -256,4 +256,6 @@ mod probes {
     fn virtio_viona_mq_set_use_pairs(cause: u8, npairs: u16) {}
 
     fn virtio_device_needs_reset() {}
+    fn virtio_set_status(value: u8) {}
+    fn virtio_state_reset() {}
 }

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

@@ -658,11 +658,11 @@ impl PciVirtioState {
                 state.queue_select = wo.read_u16();
             }
             CommonConfigReg::QueueSize => {
-                let state = self.state.lock().unwrap();
+                let mut state = self.state.lock().unwrap();
                 match VqSize::try_from(wo.read_u16()) {
                     Err(_) => {
                         // Bad queue size.
-                        self.set_needs_reset(dev);
+                        self.needs_reset_locked(dev, &mut state);
                     }
                     Ok(offered) => {
                         let qs = state.queue_select;
@@ -1051,19 +1051,65 @@ impl PciVirtioState {
     }
 
     fn set_status(&self, dev: &dyn VirtioDevice, value: u8) {
+        probes::virtio_set_status!(|| value);
         let mut state = self.state.lock().unwrap();
         let status = Status::from_bits_truncate(value);
         if status == Status::RESET && state.status != Status::RESET {
             self.virtio_reset(dev, state);
         } else {
-            // XXX: better device status FSM
-            state.status = status;
-            if status.contains(Status::FEATURES_OK) {
-                if dev.set_features(state.negotiated_features).is_err() {
-                    self.needs_reset_locked(dev, &mut state);
-                }
+            self.apply_status(dev, &mut state, status);
+        }
+    }
+
+    fn apply_status(
+        &self,
+        dev: &dyn VirtioDevice,
+        state: &mut MutexGuard<VirtioState>,
+        status: Status,
+    ) {
+        if status == state.status {
+            // No actual difference, bail out early.
+            return;
+        }
+
+        if !status.contains(state.status) {
+            // The driver has disregarded VirtIO 1.2 section 2.1.2:
+            //
+            // > The driver MUST NOT clear a device status bit.
+            //
+            // If we allowed such a thing then the guest might toggle
+            // FEATURES_OK and violate the expectation that `set_features`
+            // is called only once when setting up a device. Instead, the
+            // guest driver is in the wrong and we'll set NEEDS_RESET.
+            self.needs_reset_locked(dev, state);
+            return;
+        }
+
+        // Any bits here are being set at most once since the last device reset.
+        let new_bits = status.difference(state.status);
+
+        if new_bits.contains(Status::FEATURES_OK) {
+            // From VirtIO 1.2 section 2.1:
+            //
+            // > FEATURES_OK (8) Indicates that the driver has acknowledged
+            // > all the features it understands, and feature negotiation
+            // > is complete.
+            //
+            // So, at this point if the guest sets additional features, we don't
+            // have to care about them; renegotiation requires a device reset
+            // ("The only way to renegotiate is to reset the device."). The
+            // features provided are the ones we should enable.
+            if dev.set_features(state.negotiated_features) == Err(()) {
+                // Those requested features were not tolerable. We *must not*
+                // reflect FEATURES_OK in status. Additionally, set NEEDS_RESET
+                // in the hopes that the guset might see the issue and attempt
+                // operating in a less-featureful mode.
+                self.needs_reset_locked(dev, state);
+                return;
             }
         }
+
+        state.status = status;
     }
 
     /// Set the "Needs Reset" state on the VirtIO device
@@ -1130,6 +1176,7 @@ impl PciVirtioState {
         dev: &dyn VirtioDevice,
         mut state: MutexGuard<VirtioState>,
     ) {
+        probes::virtio_state_reset!(|| ());
         self.reset_queues_locked(dev, &mut state);
         state.reset();
         let _ = self.isr_state.read_clear();
@@ -1514,6 +1561,11 @@ const LEGACY_REG_QUEUE_NOTIFY_OFFSET: usize = 0x10;
 const COMMON_REG_OFFSET: usize = 0;
 const COMMON_REG_SIZE: usize =
     4 + 4 + 4 + 4 + 2 + 2 + 1 + 1 + 2 + 2 + 2 + 2 + 2 + 8 + 8 + 8 + 2 + 2;
+// Some tests want to poke at the common config registers, but before doing so use the total
+// common_cfg struct size to spot-check that the layout is correct. Ideally the register map we
+// build here could go both ways and either be public, or public for tests.
+#[cfg(test)]
+pub const COMMON_REG_SIZE_TEST: usize = COMMON_REG_SIZE;
 const DEVICE_REG_OFFSET: usize = PAGE_SIZE;
 const NOTIFY_REG_OFFSET: usize = 2 * PAGE_SIZE;
 pub const NOTIFY_REG_SIZE: usize = 4;

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

@@ -1015,10 +1015,11 @@ 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 viona, which always puts the
-        // control queue at the end of queue vector.  None of the other
-        // devices currently handle queues specially in this way, but we
-        // should come up with some better mechanism here.
+        // 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 {