virtio-nic: in-repo tests and a better device state FSM (#1064)

VirtIO 1.2 states clearly that a driver MUST NOT clear bits from
device_status (except, implicitly, by writing RESET aka 0 to
device_status to.. do a device reset). Since this is the only way to
clear a bit, bits that *are* set can be set at most once. Our old FSM
accepted whatever status a guest happened to provide and stored that; a
guest could intentionally or unintentionally clear NEEDS_RESET even,
though it would be very much in error at that point.

Instead of this very lax treatment of device_status, be more clear that
FEATURES_OK can be toggled to set only once (which fixes a bug on its
own), and if a guest tries clearing a status bit by any means other than
reset, set NEEDS_RESET and demand a reset outright.

Along with this, `viona.rs` gets a very simple driver to exercise some
usage patterns we've seen in starting up virtio NIC devices.

`basic_operation_multiqueue` fails without the device state FSM
improvements, a reflection of the bug mentioned above; since FEATURES_OK
is set before DEVICE_OK, the final status write including DEVICE_OK
would cause a second attempt to apply the negotiated features. We would
then try to set up all the virtqueues again, which fails because the
virtqueues were just taken out of the RESET state when the driver
initialized them, and we come to rest with the NIC in NEEDS_RESET.

Linux (and BSDs, and illumos) clearly do not care about this, as such
instances proceed to network seemingly without issue. Windows seems to
have a periodic task that notices the NEEDS_RESET bit and obliges. It
resets the NIC, which fails to ever come up correctly, and dutifully
stops networking (#1048).

Commenting out parts of prior patches (957f5c4, 1df49a4, 45af0f7) also
cause these new tests to fail; they are collectively as necessary as it
seemed for correct operation in the face of resets.

Finally, one of the new tests verifies that a device in NEEDS_RESET
cannot have the bit inappropriately cleared without reset. This tripped
over a deadlock if a virtqueue is set with an inappropriate size. That
is fixed in this patch as well.

Author: iximeow

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

@@ -263,4 +263,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 {