fix(virtq): address code review

Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>

Author: andreiltd

src/hyperlight_common/src/virtq/access.rs

@@ -47,14 +47,10 @@ pub trait MemOps {
     /// * `addr` - Guest physical address to read from
     /// * `dst` - Destination buffer to fill
     ///
-    /// # Returns
-    ///
-    /// Number of bytes actually read (should equal `dst.len()` on success).
-    ///
     /// # Safety
     ///
     /// The caller must ensure `addr` is valid and points to at least `dst.len()` bytes.
-    fn read(&self, addr: u64, dst: &mut [u8]) -> Result<usize, Self::Error>;
+    fn read(&self, addr: u64, dst: &mut [u8]) -> Result<(), Self::Error>;
 
     /// Write bytes to physical memory.
     ///
@@ -63,14 +59,10 @@ pub trait MemOps {
     /// * `addr` - address to write to
     /// * `src` - Source data to write
     ///
-    /// # Returns
-    ///
-    /// Number of bytes actually written (should equal `src.len()` on success).
-    ///
     /// # Safety
     ///
     /// The caller must ensure `addr` is valid and points to at least `src.len()` bytes.
-    fn write(&self, addr: u64, src: &[u8]) -> Result<usize, Self::Error>;
+    fn write(&self, addr: u64, src: &[u8]) -> Result<(), Self::Error>;
 
     /// Load a u16 with acquire semantics.
     ///
@@ -140,11 +132,11 @@ pub trait MemOps {
 impl<T: MemOps> MemOps for Arc<T> {
     type Error = T::Error;
 
-    fn read(&self, addr: u64, dst: &mut [u8]) -> Result<usize, Self::Error> {
+    fn read(&self, addr: u64, dst: &mut [u8]) -> Result<(), Self::Error> {
         (**self).read(addr, dst)
     }
 
-    fn write(&self, addr: u64, src: &[u8]) -> Result<usize, Self::Error> {
+    fn write(&self, addr: u64, src: &[u8]) -> Result<(), Self::Error> {
         (**self).write(addr, src)
     }
 

src/hyperlight_common/src/virtq/mod.rs

@@ -92,18 +92,31 @@ impl Layout {
     /// Create a Layout from a base address and number of descriptors.
     ///
     /// The base address must be aligned to `Descriptor::ALIGN`.
+    /// The number of descriptors must be a power of 2.
     /// The memory region starting at `base` must be at least `Layout::query_size(num_descs)` bytes.
     ///
     /// # Safety
     /// - `base` must be valid for `Layout::query_size(num_descs)` bytes.
     /// - `base` must be aligned to `Descriptor::ALIGN`.
     /// - Memory must remain valid for the lifetime of the ring.
     pub const unsafe fn from_base(base: u64, num_descs: NonZeroU16) -> Result<Self, RingError> {
+        let num_descs = num_descs.get() as usize;
+        if !num_descs.is_power_of_two() {
+            return Err(RingError::InvalidLayout);
+        }
+
         if !base.is_multiple_of(Descriptor::ALIGN as u64) {
             return Err(RingError::InvalidLayout);
         }
 
-        let desc_size = num_descs.get() as usize * Descriptor::SIZE;
+        if base
+            .checked_add(Layout::query_size(num_descs) as u64)
+            .is_none()
+        {
+            return Err(RingError::InvalidLayout);
+        }
+
+        let desc_size = num_descs * Descriptor::SIZE;
         let event_size = EventSuppression::SIZE;
         let event_align = EventSuppression::ALIGN;
 
@@ -112,7 +125,7 @@ impl Layout {
 
         Ok(Self {
             desc_table_addr: base,
-            desc_table_len: num_descs.get(),
+            desc_table_len: num_descs as u16,
             drv_evt_addr: base + drv_evt_offset as u64,
             dev_evt_addr: base + dev_evt_offset as u64,
         })
@@ -170,6 +183,10 @@ const _: () = {
         assert!(base + expected_size as u64 == layout_end);
     }
 
+    unsafe {
+        assert!(Layout::from_base(u64::MAX, NonZeroU16::new(1).unwrap()).is_err());
+    }
+
     verify_layout(1);
     verify_layout(2);
     verify_layout(4);

src/hyperlight_common/src/virtq/ring.rs

@@ -288,7 +288,7 @@ impl BufferChainBuilder<Writable> {
 ///
 /// Contains a scatter-gather list of [`BufferElement`]s, divided into
 /// readable (driver->device) and writable (device->driver) sections.
-#[derive(Debug, Default, Clone)]
+#[derive(Debug, Clone)]
 pub struct BufferChain {
     /// All buffer elements (readable followed by writable)
     elems: SmallVec<[BufferElement; 16]>,
@@ -1350,20 +1350,20 @@ pub(crate) mod tests {
     impl MemOps for TestMem {
         type Error = core::convert::Infallible;
 
-        fn read(&self, addr: u64, dst: &mut [u8]) -> Result<usize, Self::Error> {
+        fn read(&self, addr: u64, dst: &mut [u8]) -> Result<(), Self::Error> {
             let src = self.ptr_for_addr(addr);
             unsafe {
                 ptr::copy_nonoverlapping(src, dst.as_mut_ptr(), dst.len());
             }
-            Ok(dst.len())
+            Ok(())
         }
 
-        fn write(&self, addr: u64, src: &[u8]) -> Result<usize, Self::Error> {
+        fn write(&self, addr: u64, src: &[u8]) -> Result<(), Self::Error> {
             let dst = self.ptr_for_addr(addr);
             unsafe {
                 ptr::copy_nonoverlapping(src.as_ptr(), dst, src.len());
             }
-            Ok(src.len())
+            Ok(())
         }
 
         fn read_val<T: Pod>(&self, addr: u64) -> Result<T, Self::Error> {
@@ -1842,18 +1842,6 @@ pub(crate) mod tests {
         ));
     }
 
-    #[test]
-    fn test_empty_chain_rejected() {
-        let chain = BufferChain::default();
-        assert_eq!(chain.len(), 0);
-
-        let ring = make_ring(4);
-        let mut producer = make_producer(&ring);
-
-        let result = producer.submit_available(&chain);
-        assert!(matches!(result, Err(RingError::EmptyChain)));
-    }
-
     #[test]
     fn test_wrap_stress() {
         let ring = make_ring(4);
@@ -2491,7 +2479,7 @@ pub(crate) mod tests {
     // Out-of-order multi-length explicit
     #[test]
     fn test_out_of_order_multi_length() {
-        let ring = make_ring(12);
+        let ring = make_ring(16);
         let mut producer = make_producer(&ring);
         let mut consumer = make_consumer(&ring);
 
@@ -3311,7 +3299,7 @@ mod fuzz {
 
     impl Arbitrary for Scenario {
         fn arbitrary(g: &mut Gen) -> Self {
-            let table_size = usize::arbitrary(g) % MAX_RING + 1;
+            let table_size = (usize::arbitrary(g) % MAX_RING + 1).next_power_of_two();
             let num_ops = usize::arbitrary(g) % MAX_OPS + 1;
 
             let ops = (0..num_ops).map(|_| Op::arbitrary(g)).collect();