From 16c1e6523e6175b31cf02ca7ff1c124e052426e8 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Mon, 28 Oct 2024 15:49:39 +0000 Subject: [PATCH] Return Result from config space read/write methods. This also makes some device methods return a Result. --- src/device/blk.rs | 4 +-- src/device/console.rs | 22 ++++++++--------- src/device/gpu.rs | 4 +-- src/device/input.rs | 25 ++++++++++--------- src/device/net/dev_raw.rs | 4 +-- src/device/socket/vsock.rs | 11 +++++---- src/device/sound.rs | 6 ++--- src/transport/fake.rs | 25 +++++++++++++------ src/transport/mmio.rs | 14 +++++++---- src/transport/mod.rs | 6 ++--- src/transport/pci.rs | 50 +++++++++++++++++++++----------------- 11 files changed, 96 insertions(+), 75 deletions(-) diff --git a/src/device/blk.rs b/src/device/blk.rs index 8786d0bb..0fea1064 100644 --- a/src/device/blk.rs +++ b/src/device/blk.rs @@ -56,9 +56,9 @@ impl VirtIOBlk { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); // Read configuration space. - let capacity = transport.read_config_space::(offset_of!(BlkConfig, capacity_low)) + let capacity = transport.read_config_space::(offset_of!(BlkConfig, capacity_low))? as u64 - | (transport.read_config_space::(offset_of!(BlkConfig, capacity_high)) as u64) + | (transport.read_config_space::(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 index 5f5ff087..37585517 100644 --- a/src/device/console.rs +++ b/src/device/console.rs @@ -34,7 +34,7 @@ const SUPPORTED_FEATURES: Features = Features::RING_EVENT_IDX /// # fn example(transport: T) -> Result<(), Error> { /// let mut console = VirtIOConsole::::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 VirtIOConsole { } /// Returns the size of the console, if the device supports reporting this. - pub fn size(&self) -> Option { + pub fn size(&self) -> Result> { 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 VirtIOConsole { pub fn emergency_write(&mut self, chr: u8) -> Result<()> { if self.negotiated_features.contains(Features::EMERG_WRITE) { self.transport - .write_config_space::(offset_of!(Config, emerg_wr), chr.into()); + .write_config_space::(offset_of!(Config, emerg_wr), chr.into())?; Ok(()) } else { Err(Error::Unsupported) @@ -333,7 +333,7 @@ mod tests { }; let console = VirtIOConsole::>::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 index 0087a0d2..057e7eb2 100644 --- a/src/device/gpu.rs +++ b/src/device/gpu.rs @@ -44,8 +44,8 @@ impl VirtIOGpu { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); // read configuration space - let events_read = transport.read_config_space::(offset_of!(Config, events_read)); - let num_scanouts = transport.read_config_space::(offset_of!(Config, num_scanouts)); + let events_read = transport.read_config_space::(offset_of!(Config, events_read))?; + let num_scanouts = transport.read_config_space::(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 index 9e7c8310..d111bb08 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -102,21 +102,21 @@ impl VirtIOInput { select: InputConfigSelect, subsel: u8, out: &mut [u8], - ) -> u8 { + ) -> Result { 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::()); + .read_config_space(offset_of!(Config, data) + i * size_of::())?; } - size + Ok(size) } /// Queries a specific piece of information by `select` and `subsel`, allocates a sufficiently @@ -127,12 +127,12 @@ impl VirtIOInput { subsel: u8, ) -> Result, 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::(offset_of!(Config, size)), + .read_config_space::(offset_of!(Config, size))?, ); if size > CONFIG_DATA_MAX_LENGTH { return Err(Error::IoError); @@ -141,7 +141,7 @@ impl VirtIOInput { for i in 0..size { buf[i] = self .transport - .read_config_space(offset_of!(Config, data) + i * size_of::()); + .read_config_space(offset_of!(Config, data) + i * size_of::())?; } Ok(buf) } @@ -173,7 +173,7 @@ impl VirtIOInput { /// Queries and returns the ID information of the device. pub fn ids(&mut self) -> Result { 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::() { Ok(ids) } else { @@ -196,7 +196,8 @@ impl VirtIOInput { /// Queries and returns information about the given axis of the device. pub fn abs_info(&mut self, axis: u8) -> Result { 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::() { Ok(info) } else { diff --git a/src/device/net/dev_raw.rs b/src/device/net/dev_raw.rs index ca88a9b2..d5e43611 100644 --- a/src/device/net/dev_raw.rs +++ b/src/device/net/dev_raw.rs @@ -31,8 +31,8 @@ impl VirtIONetRaw(offset_of!(Config, status)); + let mac = transport.read_config_space(offset_of!(Config, mac))?; + let status = transport.read_config_space::(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 index e93d7333..654e0086 100644 --- a/src/device/socket/vsock.rs +++ b/src/device/socket/vsock.rs @@ -248,11 +248,12 @@ impl VirtIOSocket(offset_of!(VirtioVsockConfig, guest_cid_low)) as u64 - | (transport.read_config_space::(offset_of!(VirtioVsockConfig, guest_cid_high)) - as u64) - << 32; + let guest_cid = transport + .read_config_space::(offset_of!(VirtioVsockConfig, guest_cid_low))? + as u64 + | (transport.read_config_space::(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 index 4ec0e753..3c2fc4ef 100644 --- a/src/device/sound.rs +++ b/src/device/sound.rs @@ -96,9 +96,9 @@ impl VirtIOSound { )?; // 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 index a23d7120..c0aa39b6 100644 --- 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 Transport for FakeTransport { pending } - fn read_config_space(&self, offset: usize) -> T { + fn read_config_space(&self, offset: usize) -> Result { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", align_of::()); assert!(offset % align_of::() == 0); - assert!(offset + size_of::() <= size_of::()); - unsafe { self.config_space.cast::().byte_add(offset).read() } + + if size_of::() < offset + size_of::() { + Err(Error::ConfigSpaceTooSmall) + } else { + unsafe { Ok(self.config_space.cast::().byte_add(offset).read()) } + } } - fn write_config_space(&mut self, offset: usize, value: T) { + fn write_config_space(&mut self, offset: usize, value: T) -> Result<(), Error> { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", align_of::()); assert!(offset % align_of::() == 0); - assert!(offset + size_of::() <= size_of::()); - unsafe { - self.config_space.cast::().byte_add(offset).write(value); + + if size_of::() < offset + size_of::() { + Err(Error::ConfigSpaceTooSmall) + } else { + unsafe { + self.config_space.cast::().byte_add(offset).write(value); + } + Ok(()) } } } diff --git a/src/transport/mmio.rs b/src/transport/mmio.rs index 49622a60..07d8a861 100644 --- 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(&self, offset: usize) -> T { + fn read_config_space(&self, offset: usize) -> Result { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", align_of::()); assert!(offset % align_of::() == 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::() .byte_add(CONFIG_SPACE_OFFSET) .byte_add(offset) - .read_volatile() + .read_volatile()) } } - fn write_config_space(&mut self, offset: usize, value: T) { + fn write_config_space(&mut self, offset: usize, value: T) -> Result<(), Error> { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", align_of::()); assert!(offset % align_of::() == 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 index 319d0d85..83d329a4 100644 --- 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(&self, offset: usize) -> T; + fn read_config_space(&self, offset: usize) -> Result; /// Writes a value to the device config space. - fn write_config_space(&mut self, offset: usize, value: T); + fn write_config_space(&mut self, offset: usize, value: T) -> Result<()>; } bitflags! { diff --git a/src/transport/pci.rs b/src/transport/pci.rs index 442a84a3..9d310fdc 100644 --- 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(&self, offset: usize) -> T { + fn read_config_space(&self, offset: usize) -> Result { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", align_of::()); assert_eq!(offset % align_of::(), 0); - let config_space: NonNull<[u32]> = self.config_space.unwrap(); - assert!(offset + size_of::() <= config_space.len() * size_of::()); - - // 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::() < offset + size_of::() { + 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(&mut self, offset: usize, value: T) { + fn write_config_space(&mut self, offset: usize, value: T) -> Result<(), Error> { assert!(align_of::() <= 4, "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", align_of::()); assert_eq!(offset % align_of::(), 0); - let config_space = self.config_space.unwrap(); - assert!(offset + size_of::() <= config_space.len() * size_of::()); - - // 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::() < offset + size_of::() { + 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(()) } } }