devices: virtio: remove Clone from Queue

Replace the automatically derived Clone implementation with an
activate() function. For now, this just manually performs the same
action as the derived clone(), but it prevents accidental cloning of
queues, and it gives us a place to put additional checks that need to
occur at the point where a queue is enabled.

BUG=b:201119859
TEST=tools/presubmit --all

Change-Id: Ie72c0c7c90d819ec7ce586eca0b69d58f546b390
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4094294
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>

crosvm-fuzz/virtqueue_fuzzer.rs

@@ -73,9 +73,11 @@ fuzz_target!(|data: &[u8]| {
     q.set_ready(true);
 
     GUEST_MEM.with(|mem| {
-        if !q.ready() {
+        let mut q = if let Ok(q) = q.activate() {
+            q
+        } else {
             return;
-        }
+        };
 
         // First zero out all of the memory.
         let vs = mem

devices/src/virtio/queue.rs

@@ -296,14 +296,11 @@ impl<'a, 'b> Iterator for AvailIter<'a, 'b> {
     }
 }
 
-#[derive(Clone)]
 /// A virtio queue's parameters.
-///
-/// WARNING: it is NOT safe to clone and then use n>1 Queue(s) to interact with the same virtqueue.
-/// That will prevent descriptor index tracking from being accurate, which can cause incorrect
-/// interrupt masking.
-/// TODO(b/201119859) drop Clone from this struct.
 pub struct Queue {
+    /// Whether this queue has already been activated.
+    activated: bool,
+
     /// The maximal size in elements offered by the device
     max_size: u16,
 
@@ -365,6 +362,7 @@ impl Queue {
     pub fn new(max_size: u16) -> Queue {
         assert!(max_size.is_power_of_two());
         Queue {
+            activated: false,
             max_size,
             size: max_size,
             ready: false,
@@ -453,6 +451,39 @@ impl Queue {
         self.ready = enable;
     }
 
+    /// Convert the queue configuration into an active queue.
+    pub fn activate(&mut self) -> Result<Queue> {
+        if !self.ready {
+            bail!("attempted to activate a non-ready queue");
+        }
+
+        if self.activated {
+            bail!("queue is already activated");
+        }
+
+        self.activated = true;
+
+        let queue = Queue {
+            activated: self.activated,
+            max_size: self.max_size,
+            size: self.size,
+            ready: self.ready,
+            vector: self.vector,
+            desc_table: self.desc_table,
+            avail_ring: self.avail_ring,
+            used_ring: self.used_ring,
+            next_avail: self.next_avail,
+            next_used: self.next_used,
+            features: self.features,
+            last_used: self.last_used,
+            iommu: self.iommu.as_ref().map(Arc::clone),
+            exported_desc_table: self.exported_desc_table.clone(),
+            exported_avail_ring: self.exported_avail_ring.clone(),
+            exported_used_ring: self.exported_used_ring.clone(),
+        };
+        Ok(queue)
+    }
+
     // Return `index` modulo the currently configured queue size.
     fn wrap_queue_index(&self, index: Wrapping<u16>) -> u16 {
         // We know that `self.size` is a power of two (enforced by `set_size()`), so the modulus can
@@ -463,6 +494,7 @@ impl Queue {
 
     /// Reset queue to a clean state
     pub fn reset(&mut self) {
+        self.activated = false;
         self.ready = false;
         self.size = self.max_size;
         self.vector = VIRTIO_MSI_NO_VECTOR;

devices/src/virtio/vhost/user/device/handler.rs

@@ -569,7 +569,14 @@ impl<O: VhostUserPlatformOps> VhostUserSlaveReqHandlerMut for DeviceRequestHandl
         vring.queue.ack_features(self.backend.acked_features());
         vring.queue.set_ready(true);
 
-        let queue = vring.queue.clone();
+        let queue = match vring.queue.activate() {
+            Ok(queue) => queue,
+            Err(e) => {
+                error!("failed to activate vring: {:#}", e);
+                return Err(VhostError::SlaveInternalError);
+            }
+        };
+
         let doorbell = vring.doorbell.clone().ok_or(VhostError::InvalidOperation)?;
         let mem = self
             .mem

devices/src/virtio/virtio_mmio_device.rs

@@ -152,12 +152,12 @@ impl VirtioMmioDevice {
         // Use ready queues and their events.
         let queues = self
             .queues
-            .iter()
+            .iter_mut()
             .zip(self.queue_evts.iter())
             .filter(|(q, _)| q.ready())
             .map(|(queue, evt)| {
                 Ok((
-                    queue.clone(),
+                    queue.activate().context("failed to activate queue")?,
                     evt.try_clone().context("failed to clone queue_evt")?,
                 ))
             })

devices/src/virtio/virtio_pci_device.rs

@@ -491,12 +491,12 @@ impl VirtioPciDevice {
         // Use ready queues and their events.
         let queues = self
             .queues
-            .iter()
+            .iter_mut()
             .zip(self.queue_evts.iter())
             .filter(|(q, _)| q.ready())
             .map(|(queue, evt)| {
                 Ok((
-                    queue.clone(),
+                    queue.activate().context("failed to activate queue")?,
                     evt.try_clone().context("failed to clone queue_evt")?,
                 ))
             })