Moves specification-required checks to setters

Moves the mandatory checks, according to the virtio specification,
from Queue::is_valid() to the corresponding Queue setters.
closes: rust-vmm#94

Signed-off-by: German Maglione <[email protected]>

crates/virtio-device/src/mmio.rs

@@ -10,10 +10,10 @@ use std::convert::TryInto;
 use std::sync::atomic::Ordering;
 
 use log::warn;
-use vm_memory::GuestAddressSpace;
+use vm_memory::{GuestAddress, GuestAddressSpace};
 
 use crate::{status, WithDriverSelect};
-use virtio_queue::Queue;
+use virtio_queue::{Error, Queue};
 
 // Required by the Virtio MMIO device register layout at offset 0 from base. Turns out this
 // is actually the ASCII sequence for "virt" (in little endian ordering).
@@ -31,15 +31,31 @@ const VENDOR_ID: u32 = 0;
 // a `VirtioDevice`, provided the status check is successful.
 // TODO: This function and its uses will likely have to be updated when we start offering
 // packed virtqueue support as well.
+const DEFAULT_VIRTQ_ADDRESS: GuestAddress = GuestAddress(0);
 fn update_queue_field<M, D, F>(device: &mut D, f: F)
 where
     M: GuestAddressSpace,
     D: WithDriverSelect<M> + ?Sized,
-    F: FnOnce(&mut Queue<M>),
+    F: FnOnce(&mut Queue<M>) -> Result<(), Error>,
 {
     if device.check_device_status(status::FEATURES_OK, status::DRIVER_OK | status::FAILED) {
         if let Some(queue) = device.selected_queue_mut() {
-            f(queue);
+            if let Err(e) = f(queue) {
+                match e {
+                    Error::InvalidQueueSize => queue.state.size = queue.max_size(),
+                    Error::InvalidDescriptorTableAddressAlignment => {
+                        queue.state.desc_table = DEFAULT_VIRTQ_ADDRESS
+                    }
+                    Error::InvalidAvailableRingAddressAlignment => {
+                        queue.state.avail_ring = DEFAULT_VIRTQ_ADDRESS
+                    }
+                    Error::InvalidUsedRingAddressAlignment => {
+                        queue.state.used_ring = DEFAULT_VIRTQ_ADDRESS
+                    }
+                    _ => (),
+                }
+                warn!("invalid queue update: {}", e);
+            }
         } else {
             warn!("update invalid virtio queue");
         }
@@ -150,7 +166,10 @@ pub trait VirtioMmioDevice<M: GuestAddressSpace>: WithDriverSelect<M> {
                     // for now).
                     0x30 => self.set_queue_select(v as u16),
                     0x38 => update_queue_field(self, |q| q.set_size(v as u16)),
-                    0x44 => update_queue_field(self, |q| q.set_ready(v == 1)),
+                    0x44 => update_queue_field(self, |q| {
+                        q.set_ready(v == 1);
+                        Ok(())
+                    }),
                     0x50 => self.queue_notify(v),
                     0x64 => {
                         if self.check_device_status(status::DRIVER_OK, 0) {
@@ -259,6 +278,10 @@ mod tests {
         d.write(0x38, &32u32.to_le_bytes());
         assert_eq!(d.cfg.queues[0].state.size, 32);
 
+        // Let's try to set an invalid size
+        d.write(0x38, &257u32.to_le_bytes());
+        assert_eq!(d.cfg.queues[0].state.size, d.cfg.queues[0].state.max_size);
+
         // The queue in `Dummy` is not ready yet.
         assert_eq!(mmio_read(&d, 0x44), 0);
 
@@ -272,22 +295,31 @@ mod tests {
         assert_eq!(d.last_queue_notify, 2);
 
         assert_eq!(d.cfg.queues[0].state.desc_table.0, 0);
-        d.write(0x80, &1u32.to_le_bytes());
-        assert_eq!(d.cfg.queues[0].state.desc_table.0, 1);
+        d.write(0x80, &16u32.to_le_bytes());
+        assert_eq!(d.cfg.queues[0].state.desc_table.0, 16);
         d.write(0x84, &2u32.to_le_bytes());
-        assert_eq!(d.cfg.queues[0].state.desc_table.0, (2 << 32) + 1);
+        assert_eq!(d.cfg.queues[0].state.desc_table.0, (2 << 32) + 16);
+        // Let's try to set a misaligned description table address
+        d.write(0x80, &1u32.to_le_bytes());
+        assert_eq!(d.cfg.queues[0].state.desc_table.0, 0);
 
         assert_eq!(d.cfg.queues[0].state.avail_ring.0, 0);
-        d.write(0x90, &1u32.to_le_bytes());
-        assert_eq!(d.cfg.queues[0].state.avail_ring.0, 1);
+        d.write(0x90, &2u32.to_le_bytes());
+        assert_eq!(d.cfg.queues[0].state.avail_ring.0, 2);
         d.write(0x94, &2u32.to_le_bytes());
-        assert_eq!(d.cfg.queues[0].state.avail_ring.0, (2 << 32) + 1);
+        assert_eq!(d.cfg.queues[0].state.avail_ring.0, (2 << 32) + 2);
+        // Let's try to set a misaligned available ring address
+        d.write(0x90, &1u32.to_le_bytes());
+        assert_eq!(d.cfg.queues[0].state.avail_ring.0, 0);
 
         assert_eq!(d.cfg.queues[0].state.used_ring.0, 0);
-        d.write(0xa0, &1u32.to_le_bytes());
-        assert_eq!(d.cfg.queues[0].state.used_ring.0, 1);
+        d.write(0xa0, &4u32.to_le_bytes());
+        assert_eq!(d.cfg.queues[0].state.used_ring.0, 4);
         d.write(0xa4, &2u32.to_le_bytes());
-        assert_eq!(d.cfg.queues[0].state.used_ring.0, (2 << 32) + 1);
+        assert_eq!(d.cfg.queues[0].state.used_ring.0, (2 << 32) + 4);
+        // Let's try to set a misaligned used ring address
+        d.write(0xa0, &1u32.to_le_bytes());
+        assert_eq!(d.cfg.queues[0].state.used_ring.0, 0);
 
         // Let's select a non-existent queue.
         d.write(0x30, &1u32.to_le_bytes());

crates/virtio-device/src/virtio_config.rs

@@ -251,8 +251,9 @@ pub(crate) mod tests {
 
     impl Dummy {
         pub fn new(device_type: u32, features: u64, config_space: Vec<u8>) -> Self {
-            let mem =
-                Arc::new(GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x1000_0000)]).unwrap());
+            let mem = Arc::new(
+                GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x10_0000_0000)]).unwrap(),
+            );
             let queue = Queue::new(mem, 256);
 
             let cfg = VirtioConfig::new(features, vec![queue], config_space);