Make Viona more robust in the face of errors

When the Viona driver was initially drafted, it was made to work well
enough on the "happy path", but lacked any sort of resilience in the
face of uncommon or unexpected conditions.

Fixes #583

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

@@ -218,8 +218,9 @@ impl VirtioDevice for PciVirtioBlock {
         }
         feat
     }
-    fn set_features(&self, _feat: u32) {
+    fn set_features(&self, _feat: u32) -> Result<(), ()> {
         // XXX: real features
+        Ok(())
     }
 
     fn queue_notify(&self, _vq: &Arc<VirtQueue>) {

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

@@ -25,16 +25,30 @@ pub use viona::PciVirtioViona;
 pub trait VirtioDevice: Send + Sync + 'static + Entity {
     /// Read/write device-specific virtio configuration space
     fn cfg_rw(&self, ro: RWOp);
+
     /// Get the device-specific virtio feature bits
     fn get_features(&self) -> u32;
+
     /// Set the device-specific virtio feature bits
-    fn set_features(&self, feat: u32);
+    ///
+    /// Returns `Err` if an error occurred while setting the features.  Doing so
+    /// will transition the device to the Failed state.
+    fn set_features(&self, feat: u32) -> Result<(), ()>;
+
     /// Service driver notification for a given virtqueue
     fn queue_notify(&self, vq: &Arc<VirtQueue>);
 
-    #[allow(unused_variables)]
     /// Notification of virtqueue configuration change
-    fn queue_change(&self, vq: &Arc<VirtQueue>, change: VqChange) {}
+    ///
+    /// Returns `Err` if an error occurred while handling the specified
+    /// `VqChange`.  Doing so will transition the device to the Failed state.
+    fn queue_change(
+        &self,
+        _vq: &Arc<VirtQueue>,
+        _change: VqChange,
+    ) -> Result<(), ()> {
+        Ok(())
+    }
 }
 
 pub trait VirtioIntr: Send + 'static {

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

@@ -117,7 +117,9 @@ impl VirtioDevice for PciVirtio9pfs {
         VIRTIO_9P_F_MOUNT_TAG
     }
 
-    fn set_features(&self, _feat: u32) {}
+    fn set_features(&self, _feat: u32) -> Result<(), ()> {
+        Ok(())
+    }
 
     fn queue_notify(&self, vq: &Arc<VirtQueue>) {
         self.handler.handle_req(vq);

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

@@ -144,19 +144,24 @@ impl<D: PciVirtio + Send + Sync + 'static> pci::Device for D {
             // avoid deadlock while modify per-VQ interrupt config
             drop(state);
 
-            match info {
+            let result = match info {
                 pci::MsiUpdate::MaskAll | pci::MsiUpdate::UnmaskAll
                     if val != VIRTIO_MSI_NO_VECTOR =>
                 {
-                    self.queue_change(vq, VqChange::IntrCfg);
+                    self.queue_change(vq, VqChange::IntrCfg)
                 }
                 pci::MsiUpdate::Modify(idx) if val == idx => {
-                    self.queue_change(vq, VqChange::IntrCfg);
+                    self.queue_change(vq, VqChange::IntrCfg)
                 }
-                _ => {}
-            }
+                _ => Ok(()),
+            };
 
             state = vs.state.lock().unwrap();
+            if result.is_err() {
+                // An error updating the VQ interrupt config should set the
+                // device in a failed state.
+                vs.set_failed(self, &mut state);
+            }
         }
         state.intr_mode_updating = false;
         vs.state_cv.notify_all();
@@ -260,8 +265,8 @@ impl PciVirtioState {
             LegacyReg::QueuePfn => {
                 let state = self.state.lock().unwrap();
                 if let Some(queue) = self.queues.get(state.queue_sel) {
-                    let state = queue.get_state();
-                    let addr = state.mapping.desc_addr;
+                    let qs = queue.get_state();
+                    let addr = qs.mapping.desc_addr;
                     ro.write_u32((addr >> PAGE_SHIFT) as u32);
                 } else {
                     // bogus queue
@@ -310,20 +315,28 @@ impl PciVirtioState {
             LegacyReg::FeatDriver => {
                 let nego = wo.read_u32() & self.features_supported(dev);
                 let mut state = self.state.lock().unwrap();
-                state.nego_feat = nego;
-                dev.set_features(nego);
+                match dev.set_features(nego) {
+                    Ok(_) => {
+                        state.nego_feat = nego;
+                    }
+                    Err(_) => {
+                        self.set_failed(dev, &mut state);
+                    }
+                }
             }
             LegacyReg::QueuePfn => {
                 let mut state = self.state.lock().unwrap();
-                let mut success = false;
                 let pfn = wo.read_u32();
                 if let Some(queue) = self.queues.get(state.queue_sel) {
-                    success = queue.map_legacy((pfn as u64) << PAGE_SHIFT);
-                    dev.queue_change(queue, VqChange::Address);
-                }
-                if !success {
-                    // XXX: interrupt needed?
-                    state.status |= Status::FAILED;
+                    let qs_old = queue.get_state();
+                    let new_addr = (pfn as u64) << PAGE_SHIFT;
+                    queue.map_legacy(new_addr);
+
+                    if qs_old.mapping.desc_addr != new_addr {
+                        if dev.queue_change(queue, VqChange::Address).is_err() {
+                            self.set_failed(dev, &mut state);
+                        }
+                    }
                 }
             }
             LegacyReg::QueueSelect => {
@@ -366,7 +379,9 @@ impl PciVirtioState {
 
                         // With the MSI configuration updated for the virtqueue,
                         // notify the device of the change
-                        dev.queue_change(queue, VqChange::IntrCfg);
+                        if dev.queue_change(queue, VqChange::IntrCfg).is_err() {
+                            self.set_failed(dev, &mut state);
+                        }
 
                         state.intr_mode_updating = false;
                         self.state_cv.notify_all();
@@ -395,6 +410,22 @@ impl PciVirtioState {
             state.status = val;
         }
     }
+
+    /// Set the VirtIO device in a Failed state.
+    ///
+    /// This will require the guest OS to reset the device before it can use it
+    /// for anything further.
+    fn set_failed(
+        &self,
+        _dev: &dyn VirtioDevice,
+        state: &mut MutexGuard<VirtioState>,
+    ) {
+        if !state.status.contains(Status::FAILED) {
+            state.status.insert(Status::FAILED);
+            // XXX: interrupt needed?
+        }
+    }
+
     fn queue_notify(&self, dev: &dyn VirtioDevice, queue: u16) {
         probes::virtio_vq_notify!(|| (
             dev as *const dyn VirtioDevice as *const c_void as u64,
@@ -416,7 +447,9 @@ impl PciVirtioState {
     ) {
         for queue in self.queues.iter() {
             queue.reset();
-            dev.queue_change(queue, VqChange::Reset);
+            if dev.queue_change(queue, VqChange::Reset).is_err() {
+                self.set_failed(dev, &mut state);
+            }
         }
         state.reset();
         let _ = self.isr_state.read_clear();

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

@@ -198,7 +198,7 @@ impl VirtQueue {
     /// using legacy-style split virtqueue layout.
     ///
     /// `addr` must be aligned to 4k per the legacy requirements
-    pub fn map_legacy(&self, addr: u64) -> bool {
+    pub fn map_legacy(&self, addr: u64) {
         assert_eq!(addr & (LEGACY_QALIGN - 1), 0);
 
         let size = self.size as usize;
@@ -218,8 +218,6 @@ impl VirtQueue {
         used.map_split(used_addr);
         avail.valid = true;
         used.valid = true;
-
-        true
     }
     pub fn get_state(&self) -> Info {
         let avail = self.avail.lock().unwrap();

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

@@ -436,7 +436,9 @@ impl VirtioDevice for PciVirtioSoftNpuPort {
         VIRTIO_NET_F_MAC
     }
 
-    fn set_features(&self, _feat: u32) {}
+    fn set_features(&self, _feat: u32) -> std::result::Result<(), ()> {
+        Ok(())
+    }
 
     fn queue_notify(&self, vq: &Arc<VirtQueue>) {
         self.handle_guest_virtio_request(vq);