Improve vqueue encapsulation: make virtio.Queue fields private, expose as functions

Signed-off-by: Kornel Lesiński <[email protected]>

Author: kornelski

src/vmm/src/devices/virtio/mmio.rs

@@ -158,7 +158,7 @@ impl MmioTransport {
         //   eventfds, but nothing will happen other than supurious wakeups.
         // . Do not reset config_generation and keep it monotonically increasing
         for queue in self.locked_device().queues_mut() {
-            *queue = Queue::new(queue.get_max_size());
+            *queue = Queue::new(queue.max_size());
         }
     }
 
@@ -243,8 +243,8 @@ impl MmioTransport {
                         }
                         features
                     }
-                    0x34 => self.with_queue(0, |q| u32::from(q.get_max_size())),
-                    0x44 => self.with_queue(0, |q| u32::from(q.ready)),
+                    0x34 => self.with_queue(0, |q| u32::from(q.max_size())),
+                    0x44 => self.with_queue(0, |q| u32::from(q.ready())),
                     0x60 => {
                         // For vhost-user backed devices we need some additional
                         // logic to differentiate between `VIRTIO_MMIO_INT_VRING`
@@ -319,20 +319,20 @@ impl MmioTransport {
                     }
                     0x24 => self.acked_features_select = v,
                     0x30 => self.queue_select = v,
-                    0x38 => self.update_queue_field(|q| q.size = (v & 0xffff) as u16),
-                    0x44 => self.update_queue_field(|q| q.ready = v == 1),
+                    0x38 => self.update_queue_field(|q| q.set_size(v as u16)),
+                    0x44 => self.update_queue_field(|q| q.set_ready(v == 1)),
                     0x64 => {
                         if self.check_device_status(device_status::DRIVER_OK, 0) {
                             self.interrupt_status.fetch_and(!v, Ordering::SeqCst);
                         }
                     }
                     0x70 => self.set_device_status(v),
-                    0x80 => self.update_queue_field(|q| lo(&mut q.desc_table, v)),
-                    0x84 => self.update_queue_field(|q| hi(&mut q.desc_table, v)),
-                    0x90 => self.update_queue_field(|q| lo(&mut q.avail_ring, v)),
-                    0x94 => self.update_queue_field(|q| hi(&mut q.avail_ring, v)),
-                    0xa0 => self.update_queue_field(|q| lo(&mut q.used_ring, v)),
-                    0xa4 => self.update_queue_field(|q| hi(&mut q.used_ring, v)),
+                    0x80 => self.update_queue_field(|q| lo(q.desc_table_mut(), v)),
+                    0x84 => self.update_queue_field(|q| hi(q.desc_table_mut(), v)),
+                    0x90 => self.update_queue_field(|q| lo(q.avail_ring_mut(), v)),
+                    0x94 => self.update_queue_field(|q| hi(q.avail_ring_mut(), v)),
+                    0xa0 => self.update_queue_field(|q| lo(q.used_ring_mut(), v)),
+                    0xa4 => self.update_queue_field(|q| hi(q.used_ring_mut(), v)),
                     _ => {
                         warn!("unknown virtio mmio register write: 0x{:x}", offset);
                     }
@@ -479,18 +479,24 @@ pub(crate) mod tests {
         assert!(!d.are_queues_valid());
 
         d.queue_select = 0;
-        assert_eq!(d.with_queue(0, Queue::get_max_size), 16);
-        assert!(d.with_queue_mut(|q| q.size = 16));
-        assert_eq!(d.locked_device().queues()[d.queue_select as usize].size, 16);
+        assert_eq!(d.with_queue(0, |q| q.max_size()), 16);
+        assert!(d.with_queue_mut(|q| q.set_size(16)));
+        assert_eq!(
+            d.locked_device().queues()[d.queue_select as usize].size(),
+            16
+        );
 
         d.queue_select = 1;
-        assert_eq!(d.with_queue(0, Queue::get_max_size), 32);
-        assert!(d.with_queue_mut(|q| q.size = 16));
-        assert_eq!(d.locked_device().queues()[d.queue_select as usize].size, 16);
+        assert_eq!(d.with_queue(0, |q| q.max_size()), 32);
+        assert!(d.with_queue_mut(|q| q.set_size(16)));
+        assert_eq!(
+            d.locked_device().queues()[d.queue_select as usize].size(),
+            16
+        );
 
         d.queue_select = 2;
-        assert_eq!(d.with_queue(0, Queue::get_max_size), 0);
-        assert!(!d.with_queue_mut(|q| q.size = 16));
+        assert_eq!(d.with_queue(0, |q| q.max_size()), 0);
+        assert!(!d.with_queue_mut(|q| q.set_size(16)));
 
         assert!(!d.are_queues_valid());
     }
@@ -667,42 +673,45 @@ pub(crate) mod tests {
         assert_eq!(d.queue_select, 3);
 
         d.queue_select = 0;
-        assert_eq!(d.locked_device().queues()[0].size, 0);
+        assert_eq!(d.locked_device().queues()[0].size(), 0);
         write_le_u32(&mut buf[..], 16);
         d.bus_write(0x38, &buf[..]);
-        assert_eq!(d.locked_device().queues()[0].size, 16);
+        assert_eq!(d.locked_device().queues()[0].size(), 16);
 
-        assert!(!d.locked_device().queues()[0].ready);
+        assert!(!d.locked_device().queues()[0].ready());
         write_le_u32(&mut buf[..], 1);
         d.bus_write(0x44, &buf[..]);
-        assert!(d.locked_device().queues()[0].ready);
+        assert!(d.locked_device().queues()[0].ready());
 
-        assert_eq!(d.locked_device().queues()[0].desc_table.0, 0);
+        assert_eq!(d.locked_device().queues()[0].desc_table().0, 0);
         write_le_u32(&mut buf[..], 123);
         d.bus_write(0x80, &buf[..]);
-        assert_eq!(d.locked_device().queues()[0].desc_table.0, 123);
+        assert_eq!(d.locked_device().queues()[0].desc_table().0, 123);
         d.bus_write(0x84, &buf[..]);
         assert_eq!(
-            d.locked_device().queues()[0].desc_table.0,
+            d.locked_device().queues()[0].desc_table().0,
             123 + (123 << 32)
         );
 
-        assert_eq!(d.locked_device().queues()[0].avail_ring.0, 0);
+        assert_eq!(d.locked_device().queues()[0].avail_ring().0, 0);
         write_le_u32(&mut buf[..], 124);
         d.bus_write(0x90, &buf[..]);
-        assert_eq!(d.locked_device().queues()[0].avail_ring.0, 124);
+        assert_eq!(d.locked_device().queues()[0].avail_ring().0, 124);
         d.bus_write(0x94, &buf[..]);
         assert_eq!(
-            d.locked_device().queues()[0].avail_ring.0,
+            d.locked_device().queues()[0].avail_ring().0,
             124 + (124 << 32)
         );
 
-        assert_eq!(d.locked_device().queues()[0].used_ring.0, 0);
+        assert_eq!(d.locked_device().queues()[0].used_ring().0, 0);
         write_le_u32(&mut buf[..], 125);
         d.bus_write(0xa0, &buf[..]);
-        assert_eq!(d.locked_device().queues()[0].used_ring.0, 125);
+        assert_eq!(d.locked_device().queues()[0].used_ring().0, 125);
         d.bus_write(0xa4, &buf[..]);
-        assert_eq!(d.locked_device().queues()[0].used_ring.0, 125 + (125 << 32));
+        assert_eq!(
+            d.locked_device().queues()[0].used_ring().0,
+            125 + (125 << 32)
+        );
 
         set_device_status(
             &mut d,

src/vmm/src/devices/virtio/net/device.rs

@@ -2009,8 +2009,8 @@ pub mod tests {
         // Test queues count (TX and RX).
         let queues = net.queues();
         assert_eq!(queues.len(), NET_QUEUE_SIZES.len());
-        assert_eq!(queues[RX_INDEX].size, th.rxq.size());
-        assert_eq!(queues[TX_INDEX].size, th.txq.size());
+        assert_eq!(queues[RX_INDEX].size(), th.rxq.size());
+        assert_eq!(queues[TX_INDEX].size(), th.txq.size());
 
         // Test corresponding queues events.
         assert_eq!(net.queue_events().len(), NET_QUEUE_SIZES.len());
@@ -2029,7 +2029,7 @@ pub mod tests {
 
         let net = th.net();
         let queues = net.queues();
-        assert!(queues[RX_INDEX].uses_notif_suppression);
-        assert!(queues[TX_INDEX].uses_notif_suppression);
+        assert!(queues[RX_INDEX].uses_notif_suppression());
+        assert!(queues[TX_INDEX].uses_notif_suppression());
     }
 }

src/vmm/src/devices/virtio/persist.rs

@@ -61,12 +61,12 @@ impl Persist<'_> for Queue {
 
     fn save(&self) -> Self::State {
         QueueState {
-            max_size: self.max_size,
-            size: self.size,
-            ready: self.ready,
-            desc_table: self.desc_table.0,
-            avail_ring: self.avail_ring.0,
-            used_ring: self.used_ring.0,
+            max_size: self.max_size(),
+            size: self.size(),
+            ready: self.ready(),
+            desc_table: self.desc_table().0,
+            avail_ring: self.avail_ring().0,
+            used_ring: self.used_ring().0,
             next_avail: self.next_avail,
             next_used: self.next_used,
             num_added: self.num_added,
@@ -161,7 +161,7 @@ impl VirtioDeviceState {
 
         for q in &queues {
             // Sanity check queue size and queue max size.
-            if q.max_size != expected_queue_max_size || q.size > expected_queue_max_size {
+            if q.max_size() != expected_queue_max_size || q.size() > expected_queue_max_size {
                 return Err(PersistError::InvalidInput);
             }
             // Snapshot can happen at any time, including during device configuration/activation

src/vmm/src/devices/virtio/queue.rs

@@ -183,19 +183,19 @@ pub struct Queue {
     pub(crate) max_size: u16,
 
     /// The queue size in elements the driver selected
-    pub size: u16,
+    pub(crate) size: u16,
 
     /// Indicates if the queue is finished with configuration
-    pub ready: bool,
+    pub(crate) ready: bool,
 
     /// Guest physical address of the descriptor table
-    pub desc_table: GuestAddress,
+    pub(crate) desc_table: GuestAddress,
 
     /// Guest physical address of the available ring
-    pub avail_ring: GuestAddress,
+    pub(crate) avail_ring: GuestAddress,
 
     /// Guest physical address of the used ring
-    pub used_ring: GuestAddress,
+    pub(crate) used_ring: GuestAddress,
 
     pub(crate) next_avail: Wrapping<u16>,
     pub(crate) next_used: Wrapping<u16>,
@@ -225,10 +225,54 @@ impl Queue {
     }
 
     /// Maximum size of the queue.
-    pub fn get_max_size(&self) -> u16 {
+    pub fn max_size(&self) -> u16 {
         self.max_size
     }
 
+    pub fn size(&self) -> u16 {
+        self.size
+    }
+
+    pub fn ready(&self) -> bool {
+        self.ready
+    }
+
+    pub fn desc_table(&self) -> GuestAddress {
+        self.desc_table
+    }
+
+    pub fn avail_ring(&self) -> GuestAddress {
+        self.avail_ring
+    }
+
+    pub fn used_ring(&self) -> GuestAddress {
+        self.used_ring
+    }
+
+    pub fn desc_table_mut(&mut self) -> &mut GuestAddress {
+        &mut self.desc_table
+    }
+
+    pub fn avail_ring_mut(&mut self) -> &mut GuestAddress {
+        &mut self.avail_ring
+    }
+
+    pub fn used_ring_mut(&mut self) -> &mut GuestAddress {
+        &mut self.used_ring
+    }
+
+    pub fn set_max_size(&mut self, val: u16) {
+        self.max_size = val;
+    }
+
+    pub fn set_size(&mut self, val: u16) {
+        self.size = val;
+    }
+
+    pub fn set_ready(&mut self, val: bool) {
+        self.ready = val;
+    }
+
     /// Return the actual size of the queue, as the driver may not set up a
     /// queue as big as the device allows.
     pub fn actual_size(&self) -> u16 {
@@ -347,7 +391,7 @@ impl Queue {
         &mut self,
         mem: &'b M,
     ) -> Option<DescriptorChain<'b, M>> {
-        if !self.uses_notif_suppression {
+        if !self.uses_notif_suppression() {
             return self.pop(mem);
         }
 
@@ -499,7 +543,7 @@ impl Queue {
 
         // If the device doesn't use notification suppression, we'll continue to get notifications
         // no matter what.
-        if !self.uses_notif_suppression {
+        if !self.uses_notif_suppression() {
             return true;
         }
 
@@ -533,6 +577,10 @@ impl Queue {
         self.uses_notif_suppression = true;
     }
 
+    pub fn uses_notif_suppression(&self) -> bool {
+        self.uses_notif_suppression
+    }
+
     /// Check if we need to kick the guest.
     ///
     /// Please note this method has side effects: once it returns `true`, it considers the
@@ -544,7 +592,7 @@ impl Queue {
         debug_assert!(self.is_layout_valid(mem));
 
         // If the device doesn't use notification suppression, always return true
-        if !self.uses_notif_suppression {
+        if !self.uses_notif_suppression() {
             return true;
         }
 
@@ -959,8 +1007,8 @@ mod verification {
     fn verify_actual_size() {
         let ProofContext(queue, _) = kani::any();
 
-        assert!(queue.actual_size() <= queue.get_max_size());
-        assert!(queue.actual_size() <= queue.size);
+        assert!(queue.actual_size() <= queue.max_size());
+        assert!(queue.actual_size() <= queue.size());
     }
 
     #[kani::proof]
@@ -1075,7 +1123,7 @@ mod tests {
     impl Queue {
         fn avail_event(&self, mem: &GuestMemoryMmap) -> u16 {
             let avail_event_addr = self
-                .used_ring
+                .used_ring()
                 .unchecked_add(u64::from(4 + 8 * self.actual_size()));
 
             mem.read_obj::<u16>(avail_event_addr).unwrap()
@@ -1171,7 +1219,7 @@ mod tests {
         assert!(!q.is_valid(m));
         m.write_obj::<u16>(5_u16, q.avail_ring.unchecked_add(2))
             .unwrap();
-        q.max_size = 2;
+        q.set_max_size(2);
         assert!(!q.is_valid(m));
 
         // reset dirtied values
@@ -1182,23 +1230,23 @@ mod tests {
 
         // or if the various addresses are off
 
-        q.desc_table = GuestAddress(0xffff_ffff);
+        *q.desc_table_mut() = GuestAddress(0xffff_ffff);
         assert!(!q.is_valid(m));
-        q.desc_table = GuestAddress(0x1001);
+        *q.desc_table_mut() = GuestAddress(0x1001);
         assert!(!q.is_valid(m));
-        q.desc_table = vq.dtable_start();
+        *q.desc_table_mut() = vq.dtable_start();
 
-        q.avail_ring = GuestAddress(0xffff_ffff);
+        *q.avail_ring_mut() = GuestAddress(0xffff_ffff);
         assert!(!q.is_valid(m));
-        q.avail_ring = GuestAddress(0x1001);
+        *q.avail_ring_mut() = GuestAddress(0x1001);
         assert!(!q.is_valid(m));
-        q.avail_ring = vq.avail_start();
+        *q.avail_ring_mut() = vq.avail_start();
 
-        q.used_ring = GuestAddress(0xffff_ffff);
+        *q.used_ring_mut() = GuestAddress(0xffff_ffff);
         assert!(!q.is_valid(m));
-        q.used_ring = GuestAddress(0x1001);
+        *q.used_ring_mut() = GuestAddress(0x1001);
         assert!(!q.is_valid(m));
-        q.used_ring = vq.used_start();
+        *q.used_ring_mut() = vq.used_start();
     }
 
     #[test]
@@ -1351,7 +1399,7 @@ mod tests {
         let vq = VirtQueue::new(GuestAddress(0), m, 4);
         let mut q = vq.create_queue();
 
-        q.uses_notif_suppression = true;
+        q.enable_notif_suppression();
 
         // Create 1 descriptor chain of 4.
         for j in 0..4 {
@@ -1476,7 +1524,7 @@ mod tests {
         let vq = VirtQueue::new(GuestAddress(0), m, 16);
         let mut q = vq.create_queue();
 
-        q.ready = true;
+        q.set_ready(true);
 
         // We create a simple descriptor chain
         vq.dtable[0].set(0x1000_u64, 0x1000, 0, 0);