Return Result from config space read/write methods.

qwandor · qwandor · commit 16c1e6523e61 · 2024-10-28T15:50:40.000Z
This also makes some device methods return a Result.
diff --git a/src/device/blk.rs b/src/device/blk.rs
@@ -56,9 +56,9 @@ impl<H: Hal, T: Transport> VirtIOBlk<H, T> {
         let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);
 
         // Read configuration space.
-        let capacity = transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_low))
+        let capacity = transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_low))?
             as u64
-            | (transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_high)) as u64)
+            | (transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_high))? as u64)
                 << 32;
         info!("found a block device of size {}KB", capacity / 2);
 
diff --git a/src/device/console.rs b/src/device/console.rs
@@ -34,7 +34,7 @@ const SUPPORTED_FEATURES: Features = Features::RING_EVENT_IDX
 /// # fn example<HalImpl: Hal, T: Transport>(transport: T) -> Result<(), Error> {
 /// let mut console = VirtIOConsole::<HalImpl, _>::new(transport)?;
 ///
-/// let size = console.size().unwrap();
+/// let size = console.size().unwrap().unwrap();
 /// println!("VirtIO console {}x{}", size.rows, size.columns);
 ///
 /// for &c in b"Hello console!\n" {
@@ -125,14 +125,14 @@ impl<H: Hal, T: Transport> VirtIOConsole<H, T> {
     }
 
     /// Returns the size of the console, if the device supports reporting this.
-    pub fn size(&self) -> Option<Size> {
+    pub fn size(&self) -> Result<Option<Size>> {
         if self.negotiated_features.contains(Features::SIZE) {
-            Some(Size {
-                columns: self.transport.read_config_space(offset_of!(Config, cols)),
-                rows: self.transport.read_config_space(offset_of!(Config, rows)),
-            })
+            Ok(Some(Size {
+                columns: self.transport.read_config_space(offset_of!(Config, cols))?,
+                rows: self.transport.read_config_space(offset_of!(Config, rows))?,
+            }))
         } else {
-            None
+            Ok(None)
         }
     }
 
@@ -239,7 +239,7 @@ impl<H: Hal, T: Transport> VirtIOConsole<H, T> {
     pub fn emergency_write(&mut self, chr: u8) -> Result<()> {
         if self.negotiated_features.contains(Features::EMERG_WRITE) {
             self.transport
-                .write_config_space::<u32>(offset_of!(Config, emerg_wr), chr.into());
+                .write_config_space::<u32>(offset_of!(Config, emerg_wr), chr.into())?;
             Ok(())
         } else {
             Err(Error::Unsupported)
@@ -333,7 +333,7 @@ mod tests {
         };
         let console = VirtIOConsole::<FakeHal, FakeTransport<Config>>::new(transport).unwrap();
 
-        assert_eq!(console.size(), None);
+        assert_eq!(console.size(), Ok(None));
     }
 
     #[test]
@@ -359,10 +359,10 @@ mod tests {
 
         assert_eq!(
             console.size(),
-            Some(Size {
+            Ok(Some(Size {
                 columns: 80,
                 rows: 42
-            })
+            }))
         );
     }
 
diff --git a/src/device/gpu.rs b/src/device/gpu.rs
@@ -44,8 +44,8 @@ impl<H: Hal, T: Transport> VirtIOGpu<H, T> {
         let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);
 
         // read configuration space
-        let events_read = transport.read_config_space::<u32>(offset_of!(Config, events_read));
-        let num_scanouts = transport.read_config_space::<u32>(offset_of!(Config, num_scanouts));
+        let events_read = transport.read_config_space::<u32>(offset_of!(Config, events_read))?;
+        let num_scanouts = transport.read_config_space::<u32>(offset_of!(Config, num_scanouts))?;
         info!(
             "events_read: {:#x}, num_scanouts: {:#x}",
             events_read, num_scanouts
diff --git a/src/device/input.rs b/src/device/input.rs
@@ -102,21 +102,21 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
         select: InputConfigSelect,
         subsel: u8,
         out: &mut [u8],
-    ) -> u8 {
+    ) -> Result<u8, Error> {
         self.transport
-            .write_config_space(offset_of!(Config, select), select as u8);
+            .write_config_space(offset_of!(Config, select), select as u8)?;
         self.transport
-            .write_config_space(offset_of!(Config, subsel), subsel);
-        let size: u8 = self.transport.read_config_space(offset_of!(Config, size));
+            .write_config_space(offset_of!(Config, subsel), subsel)?;
+        let size: u8 = self.transport.read_config_space(offset_of!(Config, size))?;
         // Safe because config points to a valid MMIO region for the config space.
         let size_to_copy = min(usize::from(size), out.len());
         for (i, out_item) in out.iter_mut().take(size_to_copy).enumerate() {
             *out_item = self
                 .transport
-                .read_config_space(offset_of!(Config, data) + i * size_of::<u8>());
+                .read_config_space(offset_of!(Config, data) + i * size_of::<u8>())?;
         }
 
-        size
+        Ok(size)
     }
 
     /// Queries a specific piece of information by `select` and `subsel`, allocates a sufficiently
@@ -127,12 +127,12 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
         subsel: u8,
     ) -> Result<Box<[u8]>, Error> {
         self.transport
-            .write_config_space(offset_of!(Config, select), select as u8);
+            .write_config_space(offset_of!(Config, select), select as u8)?;
         self.transport
-            .write_config_space(offset_of!(Config, subsel), subsel);
+            .write_config_space(offset_of!(Config, subsel), subsel)?;
         let size = usize::from(
             self.transport
-                .read_config_space::<u8>(offset_of!(Config, size)),
+                .read_config_space::<u8>(offset_of!(Config, size))?,
         );
         if size > CONFIG_DATA_MAX_LENGTH {
             return Err(Error::IoError);
@@ -141,7 +141,7 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
         for i in 0..size {
             buf[i] = self
                 .transport
-                .read_config_space(offset_of!(Config, data) + i * size_of::<u8>());
+                .read_config_space(offset_of!(Config, data) + i * size_of::<u8>())?;
         }
         Ok(buf)
     }
@@ -173,7 +173,7 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
     /// Queries and returns the ID information of the device.
     pub fn ids(&mut self) -> Result<DevIDs, Error> {
         let mut ids = DevIDs::default();
-        let size = self.query_config_select(InputConfigSelect::IdDevids, 0, ids.as_bytes_mut());
+        let size = self.query_config_select(InputConfigSelect::IdDevids, 0, ids.as_bytes_mut())?;
         if usize::from(size) == size_of::<DevIDs>() {
             Ok(ids)
         } else {
@@ -196,7 +196,8 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
     /// Queries and returns information about the given axis of the device.
     pub fn abs_info(&mut self, axis: u8) -> Result<AbsInfo, Error> {
         let mut info = AbsInfo::default();
-        let size = self.query_config_select(InputConfigSelect::AbsInfo, axis, info.as_bytes_mut());
+        let size =
+            self.query_config_select(InputConfigSelect::AbsInfo, axis, info.as_bytes_mut())?;
         if usize::from(size) == size_of::<AbsInfo>() {
             Ok(info)
         } else {
diff --git a/src/device/net/dev_raw.rs b/src/device/net/dev_raw.rs
@@ -31,8 +31,8 @@ impl<H: Hal, T: Transport, const QUEUE_SIZE: usize> VirtIONetRaw<H, T, QUEUE_SIZ
         info!("negotiated_features {:?}", negotiated_features);
 
         // Read configuration space.
-        let mac = transport.read_config_space(offset_of!(Config, mac));
-        let status = transport.read_config_space::<Status>(offset_of!(Config, status));
+        let mac = transport.read_config_space(offset_of!(Config, mac))?;
+        let status = transport.read_config_space::<Status>(offset_of!(Config, status))?;
         debug!("Got MAC={:02x?}, status={:?}", mac, status);
 
         let send_queue = VirtQueue::new(
diff --git a/src/device/socket/vsock.rs b/src/device/socket/vsock.rs
@@ -248,11 +248,12 @@ impl<H: Hal, T: Transport, const RX_BUFFER_SIZE: usize> VirtIOSocket<H, T, RX_BU
         let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);
 
         // Safe because config is a valid pointer to the device configuration space.
-        let guest_cid =
-            transport.read_config_space::<u32>(offset_of!(VirtioVsockConfig, guest_cid_low)) as u64
-                | (transport.read_config_space::<u32>(offset_of!(VirtioVsockConfig, guest_cid_high))
-                    as u64)
-                    << 32;
+        let guest_cid = transport
+            .read_config_space::<u32>(offset_of!(VirtioVsockConfig, guest_cid_low))?
+            as u64
+            | (transport.read_config_space::<u32>(offset_of!(VirtioVsockConfig, guest_cid_high))?
+                as u64)
+                << 32;
         debug!("guest cid: {guest_cid:?}");
 
         let rx = VirtQueue::new(
diff --git a/src/device/sound.rs b/src/device/sound.rs
@@ -96,9 +96,9 @@ impl<H: Hal, T: Transport> VirtIOSound<H, T> {
         )?;
 
         // read configuration space
-        let jacks = transport.read_config_space(offset_of!(VirtIOSoundConfig, jacks));
-        let streams = transport.read_config_space(offset_of!(VirtIOSoundConfig, streams));
-        let chmaps = transport.read_config_space(offset_of!(VirtIOSoundConfig, chmaps));
+        let jacks = transport.read_config_space(offset_of!(VirtIOSoundConfig, jacks))?;
+        let streams = transport.read_config_space(offset_of!(VirtIOSoundConfig, streams))?;
+        let chmaps = transport.read_config_space(offset_of!(VirtIOSoundConfig, chmaps))?;
         info!(
             "[sound device] config: jacks: {}, streams: {}, chmaps: {}",
             jacks, streams, chmaps
diff --git a/src/transport/fake.rs b/src/transport/fake.rs
@@ -1,7 +1,7 @@
 use super::{DeviceStatus, DeviceType, Transport};
 use crate::{
     queue::{fake_read_write_queue, Descriptor},
-    PhysAddr,
+    Error, PhysAddr,
 };
 use alloc::{sync::Arc, vec::Vec};
 use core::{
@@ -96,23 +96,32 @@ impl<C> Transport for FakeTransport<C> {
         pending
     }
 
-    fn read_config_space<T>(&self, offset: usize) -> T {
+    fn read_config_space<T>(&self, offset: usize) -> Result<T, Error> {
         assert!(align_of::<T>() <= 4,
             "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
             align_of::<T>());
         assert!(offset % align_of::<T>() == 0);
-        assert!(offset + size_of::<T>() <= size_of::<C>());
-        unsafe { self.config_space.cast::<T>().byte_add(offset).read() }
+
+        if size_of::<C>() < offset + size_of::<T>() {
+            Err(Error::ConfigSpaceTooSmall)
+        } else {
+            unsafe { Ok(self.config_space.cast::<T>().byte_add(offset).read()) }
+        }
     }
 
-    fn write_config_space<T>(&mut self, offset: usize, value: T) {
+    fn write_config_space<T>(&mut self, offset: usize, value: T) -> Result<(), Error> {
         assert!(align_of::<T>() <= 4,
             "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
             align_of::<T>());
         assert!(offset % align_of::<T>() == 0);
-        assert!(offset + size_of::<T>() <= size_of::<C>());
-        unsafe {
-            self.config_space.cast::<T>().byte_add(offset).write(value);
+
+        if size_of::<C>() < offset + size_of::<T>() {
+            Err(Error::ConfigSpaceTooSmall)
+        } else {
+            unsafe {
+                self.config_space.cast::<T>().byte_add(offset).write(value);
+            }
+            Ok(())
         }
     }
 }
diff --git a/src/transport/mmio.rs b/src/transport/mmio.rs
@@ -5,7 +5,7 @@ use crate::{
     align_up,
     queue::Descriptor,
     volatile::{volread, volwrite, ReadOnly, Volatile, WriteOnly},
-    PhysAddr, PAGE_SIZE,
+    Error, PhysAddr, PAGE_SIZE,
 };
 use core::{
     convert::{TryFrom, TryInto},
@@ -498,27 +498,30 @@ impl Transport for MmioTransport {
         }
     }
 
-    fn read_config_space<T>(&self, offset: usize) -> T {
+    fn read_config_space<T>(&self, offset: usize) -> Result<T, Error> {
         assert!(align_of::<T>() <= 4,
             "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
             align_of::<T>());
         assert!(offset % align_of::<T>() == 0);
+
         // SAFETY: The caller of `MmioTransport::new` guaranteed that the header pointer was valid,
         // which includes the config space.
         unsafe {
-            self.header
+            Ok(self
+                .header
                 .cast::<T>()
                 .byte_add(CONFIG_SPACE_OFFSET)
                 .byte_add(offset)
-                .read_volatile()
+                .read_volatile())
         }
     }
 
-    fn write_config_space<T>(&mut self, offset: usize, value: T) {
+    fn write_config_space<T>(&mut self, offset: usize, value: T) -> Result<(), Error> {
         assert!(align_of::<T>() <= 4,
             "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
             align_of::<T>());
         assert!(offset % align_of::<T>() == 0);
+
         // SAFETY: The caller of `MmioTransport::new` guaranteed that the header pointer was valid,
         // which includes the config space.
         unsafe {
@@ -528,6 +531,7 @@ impl Transport for MmioTransport {
                 .byte_add(offset)
                 .write_volatile(value);
         }
+        Ok(())
     }
 }
 
diff --git a/src/transport/mod.rs b/src/transport/mod.rs
@@ -5,7 +5,7 @@ pub mod fake;
 pub mod mmio;
 pub mod pci;
 
-use crate::{PhysAddr, PAGE_SIZE};
+use crate::{PhysAddr, Result, PAGE_SIZE};
 use bitflags::{bitflags, Flags};
 use core::{fmt::Debug, ops::BitAnd};
 use log::debug;
@@ -100,10 +100,10 @@ pub trait Transport {
     }
 
     /// Reads a value from the device config space.
-    fn read_config_space<T: FromBytes>(&self, offset: usize) -> T;
+    fn read_config_space<T: FromBytes>(&self, offset: usize) -> Result<T>;
 
     /// Writes a value to the device config space.
-    fn write_config_space<T: AsBytes>(&mut self, offset: usize, value: T);
+    fn write_config_space<T: AsBytes>(&mut self, offset: usize, value: T) -> Result<()>;
 }
 
 bitflags! {
diff --git a/src/transport/pci.rs b/src/transport/pci.rs
@@ -12,6 +12,7 @@ use crate::{
     volatile::{
         volread, volwrite, ReadOnly, Volatile, VolatileReadable, VolatileWritable, WriteOnly,
     },
+    Error,
 };
 use core::{
     fmt::{self, Display, Formatter},
@@ -325,41 +326,46 @@ impl Transport for PciTransport {
         isr_status & 0x3 != 0
     }
 
-    fn read_config_space<T>(&self, offset: usize) -> T {
+    fn read_config_space<T>(&self, offset: usize) -> Result<T, Error> {
         assert!(align_of::<T>() <= 4,
             "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
             align_of::<T>());
         assert_eq!(offset % align_of::<T>(), 0);
 
-        let config_space: NonNull<[u32]> = self.config_space.unwrap();
-        assert!(offset + size_of::<T>() <= config_space.len() * size_of::<u32>());
-
-        // SAFETY: If we have a config space pointer it must be valid for its length, and we just
-        // checked that the offset and size of the access was within the length.
-        unsafe {
-            // TODO: Use NonNull::as_non_null_ptr once it is stable.
-            (config_space.as_ptr() as *mut T)
-                .byte_add(offset)
-                .read_volatile()
+        let config_space = self.config_space.ok_or(Error::ConfigSpaceMissing)?;
+        if config_space.len() * size_of::<u32>() < offset + size_of::<T>() {
+            Err(Error::ConfigSpaceTooSmall)
+        } else {
+            // SAFETY: If we have a config space pointer it must be valid for its length, and we just
+            // checked that the offset and size of the access was within the length.
+            unsafe {
+                // TODO: Use NonNull::as_non_null_ptr once it is stable.
+                Ok((config_space.as_ptr() as *mut T)
+                    .byte_add(offset)
+                    .read_volatile())
+            }
         }
     }
 
-    fn write_config_space<T>(&mut self, offset: usize, value: T) {
+    fn write_config_space<T>(&mut self, offset: usize, value: T) -> Result<(), Error> {
         assert!(align_of::<T>() <= 4,
             "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.",
             align_of::<T>());
         assert_eq!(offset % align_of::<T>(), 0);
 
-        let config_space = self.config_space.unwrap();
-        assert!(offset + size_of::<T>() <= config_space.len() * size_of::<u32>());
-
-        // SAFETY: If we have a config space pointer it must be valid for its length, and we just
-        // checked that the offset and size of the access was within the length.
-        unsafe {
-            // TODO: Use NonNull::as_non_null_ptr once it is stable.
-            (config_space.as_ptr() as *mut T)
-                .byte_add(offset)
-                .write_volatile(value);
+        let config_space = self.config_space.ok_or(Error::ConfigSpaceMissing)?;
+        if config_space.len() * size_of::<u32>() < offset + size_of::<T>() {
+            Err(Error::ConfigSpaceTooSmall)
+        } else {
+            // SAFETY: If we have a config space pointer it must be valid for its length, and we just
+            // checked that the offset and size of the access was within the length.
+            unsafe {
+                // TODO: Use NonNull::as_non_null_ptr once it is stable.
+                (config_space.as_ptr() as *mut T)
+                    .byte_add(offset)
+                    .write_volatile(value);
+            }
+            Ok(())
         }
     }
 }
