Virtio tweaks

src/utils/src/lib.rs

@@ -41,6 +41,15 @@ pub const fn u64_to_usize(num: u64) -> usize {
     num as usize
 }
 
+/// Safely converts a usize value to a u64 value.
+/// This bypasses the Clippy lint check because we only support 64-bit platforms.
+#[cfg(target_pointer_width = "64")]
+#[inline]
+#[allow(clippy::cast_possible_truncation)]
+pub const fn usize_to_u64(num: usize) -> u64 {
+    num as u64
+}
+
 /// Converts a usize into a wrapping u32.
 #[inline]
 pub const fn wrap_usize_to_u32(num: usize) -> Wrapping<u32> {

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

@@ -264,7 +264,11 @@
         &self.tx_rate_limiter
     }
 
-    fn signal_used_queue(&mut self, queue_type: NetQueue) -> Result<(), DeviceError> {
+    /// Trigger queue notification for the guest if we used enough descriptors
+    /// for the notification to be enabled.
+    /// https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-320005
+    /// 2.6.7.1 Driver Requirements: Used Buffer Notification Suppression
+    fn try_signal_queue(&mut self, queue_type: NetQueue) -> Result<(), DeviceError> {
         // This is safe since we checked in the event handler that the device is activated.
         let mem = self.device_state.mem().unwrap();
 
@@ -552,9 +556,7 @@
             }
         }
 
-        // At this point we processed as many Rx frames as possible.
-        // We have to wake the guest if at least one descriptor chain has been used.
-        self.signal_used_queue(NetQueue::Rx)
+        self.try_signal_queue(NetQueue::Rx)
     }
 
     // Process the deferred frame first, then continue reading from tap.
@@ -566,7 +568,7 @@
             return self.process_rx();
         }
 
-        self.signal_used_queue(NetQueue::Rx)
+        self.try_signal_queue(NetQueue::Rx)
     }
 
     fn resume_rx(&mut self) -> Result<(), DeviceError> {
@@ -647,7 +649,7 @@
             self.metrics.no_tx_avail_buffer.inc();
         }
 
-        self.signal_used_queue(NetQueue::Tx)?;
+        self.try_signal_queue(NetQueue::Tx)?;
 
         // An incoming frame for the MMDS may trigger the transmission of a new message.
         if process_rx_for_mmds {

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

@@ -312,7 +312,7 @@ pub(crate) fn inject_tap_tx_frame(net: &Net, len: usize) -> Vec<u8> {
 pub fn write_element_in_queue(net: &Net, idx: u16, val: u64) -> Result<(), DeviceError> {
     if idx as usize > net.queue_evts.len() {
         return Err(DeviceError::QueueError(QueueError::DescIndexOutOfBounds(
-            idx,
+            u32::from(idx),
         )));
     }
     net.queue_evts[idx as usize].write(val).unwrap();
@@ -322,7 +322,7 @@ pub fn write_element_in_queue(net: &Net, idx: u16, val: u64) -> Result<(), Devic
 pub fn get_element_from_queue(net: &Net, idx: u16) -> Result<u64, DeviceError> {
     if idx as usize > net.queue_evts.len() {
         return Err(DeviceError::QueueError(QueueError::DescIndexOutOfBounds(
-            idx,
+            u32::from(idx),
         )));
     }
     Ok(u64::try_from(net.queue_evts[idx as usize].as_raw_fd()).unwrap())

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

@@ -9,6 +9,8 @@
 use std::num::Wrapping;
 use std::sync::atomic::{fence, Ordering};
 
+use utils::usize_to_u64;
+
 use crate::logger::error;
 use crate::vstate::memory::{
     Address, ByteValued, Bytes, GuestAddress, GuestMemory, GuestMemoryMmap,
@@ -31,12 +33,15 @@
 #[derive(Debug, thiserror::Error, displaydoc::Display)]
 pub enum QueueError {
     /// Descriptor index out of bounds: {0}.
-    DescIndexOutOfBounds(u16),
+    DescIndexOutOfBounds(u32),
     /// Failed to write value into the virtio queue used ring: {0}
     UsedRing(#[from] vm_memory::GuestMemoryError),
 }
 
 /// A virtio descriptor constraints with C representative.
+/// Taken from Virtio spec:
+/// https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-430008
+/// 2.6.5 The Virtqueue Descriptor Table
 #[repr(C)]
 #[derive(Default, Clone, Copy)]
 struct Descriptor {
@@ -49,6 +54,20 @@
 // SAFETY: `Descriptor` is a POD and contains no padding.
 unsafe impl ByteValued for Descriptor {}
 
+/// A virtio used element in the used ring.
+/// Taken from Virtio spec:
+/// https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-430008
+/// 2.6.8 The Virtqueue Used Ring
+#[repr(C)]
+#[derive(Default, Clone, Copy)]
+struct UsedElement {
+    id: u32,
+    len: u32,
+}
+
+// SAFETY: `UsedElement` is a POD and contains no padding.
+unsafe impl ByteValued for UsedElement {}
+
 /// A virtio descriptor chain.
 #[derive(Debug)]
 pub struct DescriptorChain<'a, M: GuestMemory = GuestMemoryMmap> {
@@ -333,7 +352,10 @@
             // We are choosing to interrupt execution since this could be a potential malicious
             // driver scenario. This way we also eliminate the risk of repeatedly
             // logging and potentially clogging the microVM through the log system.
-            panic!("The number of available virtio descriptors is greater than queue size!");
+            panic!(
+                "The number of available virtio descriptors {len} is greater than queue size: {}!",
+                self.actual_size()
+            );
         }
 
         if len == 0 {
@@ -427,31 +449,51 @@
     ) -> Result<(), QueueError> {
         debug_assert!(self.is_layout_valid(mem));
 
-        if desc_index >= self.actual_size() {
-            error!(
-                "attempted to add out of bounds descriptor to used ring: {}",
-                desc_index
-            );
-            return Err(QueueError::DescIndexOutOfBounds(desc_index));
-        }
-
-        let used_ring = self.used_ring;
-        let next_used = u64::from(self.next_used.0 % self.actual_size());
-        let used_elem = used_ring.unchecked_add(4 + next_used * 8);
-
-        mem.write_obj(u32::from(desc_index), used_elem)?;
-
-        let len_addr = used_elem.unchecked_add(4);
-        mem.write_obj(len, len_addr)?;
+        let next_used = self.next_used.0 % self.actual_size();
+        let used_element = UsedElement {
+            id: u32::from(desc_index),
+            len,
+        };
+        self.write_used_ring(mem, next_used, used_element)?;
 
         self.num_added += Wrapping(1);
         self.next_used += Wrapping(1);
 
         // This fence ensures all descriptor writes are visible before the index update is.
         fence(Ordering::Release);
 
-        let next_used_addr = used_ring.unchecked_add(2);
-        mem.write_obj(self.next_used.0, next_used_addr)
+        self.set_used_ring_idx(self.next_used.0, mem);
+        Ok(())
+    }
+
+    fn write_used_ring<M: GuestMemory>(
+        &self,
+        mem: &M,
+        index: u16,
+        used_element: UsedElement,
+    ) -> Result<(), QueueError> {
+        if used_element.id >= u32::from(self.actual_size()) {
+            error!(
+                "attempted to add out of bounds descriptor to used ring: {}",
+                used_element.id
+            );
+            return Err(QueueError::DescIndexOutOfBounds(used_element.id));
+        }
+
+        // Used ring has layout:
+        // struct UsedRing {
+        //     flags: u16,
+        //     idx: u16,
+        //     ring: [UsedElement; <queue size>],
+        //     avail_event: u16,
+        // }
+        // We calculate offset into `ring` field.
+        let used_ring_offset = std::mem::size_of::<u16>()
+            + std::mem::size_of::<u16>()
+            + std::mem::size_of::<UsedElement>() * usize::from(index);
+        let used_element_address = self.used_ring.unchecked_add(usize_to_u64(used_ring_offset));
+
+        mem.write_obj(used_element, used_element_address)
             .map_err(QueueError::UsedRing)
     }
 
@@ -481,15 +523,45 @@
         Wrapping(mem.read_obj::<u16>(used_event_addr).unwrap())
     }
 
-    /// Helper method that writes `val` to the `avail_event` field of the used ring.
-    fn set_avail_event<M: GuestMemory>(&mut self, val: u16, mem: &M) {
+    /// Helper method that writes to the `avail_event` field of the used ring.
+    #[inline(always)]
+    fn set_used_ring_avail_event<M: GuestMemory>(&mut self, avail_event: u16, mem: &M) {
         debug_assert!(self.is_layout_valid(mem));
 
+        // Used ring has layout:
+        // struct UsedRing {
+        //     flags: u16,
+        //     idx: u16,
+        //     ring: [UsedElement; <queue size>],
+        //     avail_event: u16,
+        // }
+        // We calculate offset into `avail_event` field.
+        let avail_event_offset = std::mem::size_of::<u16>()
+            + std::mem::size_of::<u16>()
+            + std::mem::size_of::<UsedElement>() * usize::from(self.actual_size());
         let avail_event_addr = self
             .used_ring
-            .unchecked_add(u64::from(4 + 8 * self.actual_size()));
+            .unchecked_add(usize_to_u64(avail_event_offset));
 
-        mem.write_obj(val, avail_event_addr).unwrap();
+        mem.write_obj(avail_event, avail_event_addr).unwrap();
+    }
+
+    /// Helper method that writes to the `idx` field of the used ring.
+    #[inline(always)]
+    fn set_used_ring_idx<M: GuestMemory>(&mut self, next_used: u16, mem: &M) {
+        debug_assert!(self.is_layout_valid(mem));
+
+        // Used ring has layout:
+        // struct UsedRing {
+        //     flags: u16,
+        //     idx: u16,
+        //     ring: [UsedElement; <queue size>],
+        //     avail_event: u16,
+        // }
+        // We calculate offset into `idx` field.
+        let idx_offset = std::mem::size_of::<u16>();
+        let next_used_addr = self.used_ring.unchecked_add(usize_to_u64(idx_offset));
+        mem.write_obj(next_used, next_used_addr).unwrap();
     }
 
     /// Try to enable notification events from the guest driver. Returns true if notifications were
@@ -514,15 +586,19 @@
                 // driver scenario. This way we also eliminate the risk of
                 // repeatedly logging and potentially clogging the microVM through
                 // the log system.
-                panic!("The number of available virtio descriptors is greater than queue size!");
+                panic!(
+                    "The number of available virtio descriptors {len} is greater than queue size: \
+                     {}!",
+                    self.actual_size()
+                );
             }
             return false;
         }
 
         // Set the next expected avail_idx as avail_event.
-        self.set_avail_event(self.next_avail.0, mem);
+        self.set_used_ring_avail_event(self.next_avail.0, mem);
 
-        // Make sure all subsequent reads are performed after `set_avail_event`.
+        // Make sure all subsequent reads are performed after `set_used_ring_avail_event`.
         fence(Ordering::SeqCst);
 
         // If the actual avail_idx is different than next_avail one or more descriptors can still
@@ -827,14 +903,14 @@
     mod stubs {
         use super::*;
 
-        // Calls to set_avail_event tend to cause memory to grow unboundedly during verification.
-        // The function writes to the `avail_event` of the virtio queue, which is not read
-        // from by the device. It is only intended to be used by guest. Therefore, it does not
-        // affect any device functionality (e.g. its only call site, try_enable_notification,
-        // will behave independently of what value was written here). Thus we can stub it out
-        // with a no-op. Note that we have a separate harness for set_avail_event, to ensure
-        // the function itself is sound.
-        fn set_avail_event<M: GuestMemory>(_self: &mut Queue, _val: u16, _mem: &M) {
+        // Calls to set_used_ring_avail_event tend to cause memory to grow unboundedly during
+        // verification. The function writes to the `avail_event` of the virtio queue, which
+        // is not read from by the device. It is only intended to be used by guest.
+        // Therefore, it does not affect any device functionality (e.g. its only call site,
+        // try_enable_notification, will behave independently of what value was written
+        // here). Thus we can stub it out with a no-op. Note that we have a separate harness
+        // for set_used_ring_avail_event, to ensure the function itself is sound.
+        fn set_used_ring_avail_event<M: GuestMemory>(_self: &mut Queue, _val: u16, _mem: &M) {
             // do nothing
         }
     }
@@ -967,10 +1043,10 @@
 
     #[kani::proof]
     #[kani::unwind(0)]
-    fn verify_set_avail_event() {
+    fn verify_set_used_ring_avail_event() {
         let ProofContext(mut queue, mem) = ProofContext::bounded_queue();
 
-        queue.set_avail_event(kani::any(), &mem);
+        queue.set_used_ring_avail_event(kani::any(), &mem);
     }
 
     #[kani::proof]
@@ -1016,7 +1092,7 @@
 
     #[kani::proof]
     #[kani::unwind(0)]
-    #[kani::stub(Queue::set_avail_event, stubs::set_avail_event)]
+    #[kani::stub(Queue::set_used_ring_avail_event, stubs::set_used_ring_avail_event)]
     fn verify_try_enable_notification() {
         let ProofContext(mut queue, mem) = ProofContext::bounded_queue();
 
@@ -1292,7 +1368,7 @@
 
     #[test]
     #[should_panic(
-        expected = "The number of available virtio descriptors is greater than queue size!"
+        expected = "The number of available virtio descriptors 5 is greater than queue size: 4!"
     )]
     fn test_invalid_avail_idx_no_notification() {
         // This test ensures constructing a descriptor chain succeeds
@@ -1337,7 +1413,7 @@
 
     #[test]
     #[should_panic(
-        expected = "The number of available virtio descriptors is greater than queue size!"
+        expected = "The number of available virtio descriptors 6 is greater than queue size: 4!"
     )]
     fn test_invalid_avail_idx_with_notification() {
         // This test ensures constructing a descriptor chain succeeds
@@ -1407,17 +1483,17 @@
     }
 
     #[test]
-    fn test_set_avail_event() {
+    fn test_set_used_ring_avail_event() {
         let m = &default_mem();
         let vq = VirtQueue::new(GuestAddress(0), m, 16);
 
         let mut q = vq.create_queue();
         assert_eq!(vq.used.event.get(), 0);
 
-        q.set_avail_event(10, m);
+        q.set_used_ring_avail_event(10, m);
         assert_eq!(vq.used.event.get(), 10);
 
-        q.set_avail_event(u16::MAX, m);
+        q.set_used_ring_avail_event(u16::MAX, m);
         assert_eq!(vq.used.event.get(), u16::MAX);
     }