feat(virtq): address code review comments

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

Author: andreiltd

src/hyperlight_common/src/virtq/access.rs

@@ -29,13 +29,17 @@ use bytemuck::Pod;
 /// # Safety
 ///
 /// Implementations must ensure that:
-/// - Pointers passed to methods are valid for the duration of the call
-/// - Memory ordering guarantees are upheld as documented
-/// - Reads and writes don't cause undefined behavior (alignment, validity)
+/// - Addresses accepted by these methods are translated according to the
+///   backend's memory model.
+/// - Invalid or inaccessible addresses are reported with `Self::Error` rather
+///   than causing undefined behavior.
+/// - Memory ordering guarantees are upheld as documented.
+/// - Typed reads/writes and atomic operations honor alignment and initialized
+///   memory requirements for the translated addresses.
 ///
 /// [`RingProducer`]: super::RingProducer
 /// [`RingConsumer`]: super::RingConsumer
-pub trait MemOps {
+pub unsafe trait MemOps {
     type Error;
 
     /// Read bytes from physical memory.
@@ -47,9 +51,8 @@ pub trait MemOps {
     /// * `addr` - Guest physical address to read from
     /// * `dst` - Destination buffer to fill
     ///
-    /// # Safety
-    ///
-    /// The caller must ensure `addr` is valid and points to at least `dst.len()` bytes.
+    /// Implementations must return an error if `addr` cannot be read for
+    /// at least `dst.len()` bytes.
     fn read(&self, addr: u64, dst: &mut [u8]) -> Result<(), Self::Error>;
 
     /// Write bytes to physical memory.
@@ -59,23 +62,20 @@ pub trait MemOps {
     /// * `addr` - address to write to
     /// * `src` - Source data to write
     ///
-    /// # Safety
-    ///
-    /// The caller must ensure `addr` is valid and points to at least `src.len()` bytes.
+    /// Implementations must return an error if `addr` cannot be written for
+    /// at least `src.len()` bytes.
     fn write(&self, addr: u64, src: &[u8]) -> Result<(), Self::Error>;
 
     /// Load a u16 with acquire semantics.
     ///
-    /// # Safety
-    ///
-    /// `addr` must translate to a valid, aligned `AtomicU16` in shared memory.
+    /// Implementations must return an error if `addr` does not translate to a
+    /// valid, aligned `AtomicU16` in shared memory.
     fn load_acquire(&self, addr: u64) -> Result<u16, Self::Error>;
 
     /// Store a u16 with release semantics.
     ///
-    /// # Safety
-    ///
-    /// `addr` must translate to a valid `AtomicU16` in shared memory.
+    /// Implementations must return an error if `addr` does not translate to a
+    /// valid, aligned `AtomicU16` in shared memory.
     fn store_release(&self, addr: u64, val: u16) -> Result<(), Self::Error>;
 
     /// Get a direct read-only slice into shared memory.
@@ -106,9 +106,8 @@ pub trait MemOps {
 
     /// Read a Pod type at the given pointer.
     ///
-    /// # Safety
-    ///
-    /// The caller must ensure `addr` is valid, aligned, and translates to initialized memory.
+    /// Implementations must return an error if `addr` is not valid, aligned,
+    /// and initialized for `T`.
     fn read_val<T: Pod>(&self, addr: u64) -> Result<T, Self::Error> {
         let mut val = T::zeroed();
         let bytes = bytemuck::bytes_of_mut(&mut val);
@@ -119,17 +118,18 @@ pub trait MemOps {
 
     /// Write a Pod type at the given pointer.
     ///
-    /// # Safety
-    ///
-    /// The caller ensures that `ptr` is valid.
+    /// Implementations must return an error if `addr` is not valid and aligned
+    /// for `T`.
     fn write_val<T: Pod>(&self, addr: u64, val: T) -> Result<(), Self::Error> {
         let bytes = bytemuck::bytes_of(&val);
         self.write(addr, bytes)?;
         Ok(())
     }
 }
 
-impl<T: MemOps> MemOps for Arc<T> {
+// SAFETY: Arc delegates all memory operations to the wrapped backend, preserving
+// that backend's MemOps contract.
+unsafe impl<T: MemOps> MemOps for Arc<T> {
     type Error = T::Error;
 
     fn read(&self, addr: u64, dst: &mut [u8]) -> Result<(), Self::Error> {

src/hyperlight_common/src/virtq/desc.rs

@@ -27,6 +27,9 @@ use super::MemOps;
 
 bitflags! {
     /// Descriptor flags as defined by VIRTIO specification.
+    ///
+    /// Note: The implementation never follows the indirect-table interpretation,
+    /// so INDIRECT bit is effectively ignored.
     #[repr(transparent)]
     #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
     pub struct DescFlags: u16 {
@@ -90,7 +93,7 @@ impl Descriptor {
         DescFlags::from_bits_truncate(self.flags)
     }
 
-    /// Did the guest mark this descriptor in the current guest round?
+    /// Did the driver make this descriptor available in the current driver round?
     #[inline]
     pub fn is_avail(&self, wrap: bool) -> bool {
         let f = self.flags();
@@ -99,7 +102,7 @@ impl Descriptor {
         avail == wrap && used != wrap
     }
 
-    /// Did the host mark this descriptor used in the current host round?
+    /// Did the device mark this descriptor used in the current device round?
     #[inline]
     pub fn is_used(&self, wrap: bool) -> bool {
         let f = self.flags();

src/hyperlight_common/src/virtq/event.rs

@@ -53,6 +53,10 @@ const _: () = assert!(EventSuppression::WRAP_OFFSET == 0);
 const _: () = assert!(EventSuppression::FLAGS_OFFSET == 2);
 
 impl EventSuppression {
+    const FLAGS_MASK: u16 = 0x3;
+    const DESC_EVENT_OFF_MASK: u16 = 0x7FFF;
+    const DESC_EVENT_WRAP: u16 = 0x8000;
+
     pub const SIZE: usize = core::mem::size_of::<Self>();
     pub const ALIGN: usize = core::mem::align_of::<Self>();
     pub const WRAP_OFFSET: usize = core::mem::offset_of!(Self, off_wrap);
@@ -68,27 +72,28 @@ impl EventSuppression {
 
     /// Get the event flags.
     pub fn flags(&self) -> EventFlags {
-        EventFlags::from_bits_truncate(self.flags & 0x3)
+        EventFlags::from_bits_truncate(self.flags & Self::FLAGS_MASK)
     }
 
     /// Set the event flags.
     pub fn set_flags(&mut self, flags: EventFlags) {
-        self.flags = (self.flags & !0x3) | (flags.bits() & 0x3);
+        self.flags = (self.flags & !Self::FLAGS_MASK) | (flags.bits() & Self::FLAGS_MASK);
     }
 
     /// Get the descriptor event offset (bits 0-14).
     pub fn desc_event_off(&self) -> u16 {
-        self.off_wrap & 0x7FFF
+        self.off_wrap & Self::DESC_EVENT_OFF_MASK
     }
 
     /// Check if the descriptor event wrap bit (bit 15) is set.
     pub fn desc_event_wrap(&self) -> bool {
-        (self.off_wrap & 0x8000) != 0
+        (self.off_wrap & Self::DESC_EVENT_WRAP) != 0
     }
 
     /// Set the descriptor event offset and wrap bit.
     pub fn set_desc_event(&mut self, off: u16, wrap: bool) {
-        self.off_wrap = (off & 0x7FFF) | if wrap { 0x8000 } else { 0 };
+        self.off_wrap =
+            (off & Self::DESC_EVENT_OFF_MASK) | if wrap { Self::DESC_EVENT_WRAP } else { 0 };
     }
 
     /// Create an `EventSuppression` from a raw pointer with acquire semantics.

src/hyperlight_common/src/virtq/mod.rs

@@ -74,18 +74,18 @@ pub use ring::*;
 #[derive(Clone, Copy, Debug)]
 pub struct Layout {
     /// Packed ring descriptor table base in shared memory.
-    pub desc_table_addr: u64,
+    desc_table_addr: u64,
     /// Number of descriptors (ring size, must be power of 2).
-    pub desc_table_len: u16,
+    desc_table_len: u16,
     /// Driver-written event suppression area in shared memory.
-    pub drv_evt_addr: u64,
+    drv_evt_addr: u64,
     /// Device-written event suppression area in shared memory.
-    pub dev_evt_addr: u64,
+    dev_evt_addr: u64,
 }
 
 #[inline]
 const fn align_up(val: usize, align: usize) -> usize {
-    (val + align - 1) & !(align - 1)
+    val.next_multiple_of(align)
 }
 
 impl Layout {
@@ -131,6 +131,26 @@ impl Layout {
         })
     }
 
+    /// Packed ring descriptor table base in shared memory.
+    pub const fn desc_table_addr(&self) -> u64 {
+        self.desc_table_addr
+    }
+
+    /// Number of descriptors in the ring.
+    pub const fn desc_table_len(&self) -> u16 {
+        self.desc_table_len
+    }
+
+    /// Driver-written event suppression area in shared memory.
+    pub const fn drv_evt_addr(&self) -> u64 {
+        self.drv_evt_addr
+    }
+
+    /// Device-written event suppression area in shared memory.
+    pub const fn dev_evt_addr(&self) -> u64 {
+        self.dev_evt_addr
+    }
+
     /// Calculate the memory size needed for a ring with `num_descs` descriptors,
     /// accounting for alignment requirements.
     pub const fn query_size(num_descs: usize) -> usize {
@@ -160,26 +180,26 @@ const _: () = {
 
         let expected_size = Layout::query_size(num_descs);
 
-        assert!(layout.desc_table_addr == base);
-        assert!(layout.desc_table_len as usize == num_descs);
+        assert!(layout.desc_table_addr() == base);
+        assert!(layout.desc_table_len() as usize == num_descs);
         assert!(
             layout
-                .drv_evt_addr
+                .drv_evt_addr()
                 .is_multiple_of(EventSuppression::ALIGN as u64)
         );
         assert!(
             layout
-                .dev_evt_addr
+                .dev_evt_addr()
                 .is_multiple_of(EventSuppression::ALIGN as u64)
         );
 
         // Events don't overlap with descriptor table
         let desc_end = base + (num_descs * Descriptor::SIZE) as u64;
-        assert!(layout.drv_evt_addr >= desc_end);
-        assert!(layout.dev_evt_addr >= layout.drv_evt_addr + EventSuppression::SIZE as u64);
+        assert!(layout.drv_evt_addr() >= desc_end);
+        assert!(layout.dev_evt_addr() >= layout.drv_evt_addr() + EventSuppression::SIZE as u64);
 
         // Total size from query_size covers entire layout
-        let layout_end = layout.dev_evt_addr + EventSuppression::SIZE as u64;
+        let layout_end = layout.dev_evt_addr() + EventSuppression::SIZE as u64;
         assert!(base + expected_size as u64 == layout_end);
     }