From 41dff350ee543dfb56cdab48269b7ddd585cf98f Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Mon, 28 Oct 2024 15:35:05 +0000 Subject: [PATCH 1/2] Add methods to Transport to read and write config space, rather than returning a pointer. The config space is not necessarily memory mapped, some other transports require the config space to be accessed via other IO operations such as hypercalls. --- src/device/blk.rs | 13 ++++---- src/device/console.rs | 28 ++++++----------- src/device/gpu.rs | 18 +++++------ src/device/input.rs | 61 +++++++++++++++++++------------------- src/device/net/dev_raw.rs | 21 +++++-------- src/device/net/mod.rs | 7 +++-- src/device/socket/vsock.rs | 13 ++++---- src/device/sound.rs | 16 ++++------ src/transport/fake.rs | 25 +++++++++++----- src/transport/mmio.rs | 39 ++++++++++++++++++------ src/transport/mod.rs | 12 +++++--- src/transport/pci.rs | 53 ++++++++++++++++++++++----------- 12 files changed, 169 insertions(+), 137 deletions(-) diff --git a/src/device/blk.rs b/src/device/blk.rs index 53f4bb53..71e18b99 100644 --- a/src/device/blk.rs +++ b/src/device/blk.rs @@ -3,9 +3,10 @@ use crate::hal::Hal; use crate::queue::VirtQueue; use crate::transport::Transport; -use crate::volatile::{volread, Volatile}; +use crate::volatile::Volatile; use crate::{Error, Result}; use bitflags::bitflags; +use core::mem::offset_of; use log::info; use zerocopy::{FromBytes, Immutable, IntoBytes, KnownLayout}; @@ -55,12 +56,10 @@ impl VirtIOBlk { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); // Read configuration space. - let config = transport.config_space::()?; - info!("config: {:?}", config); - // Safe because config is a valid pointer to the device configuration space. - let capacity = unsafe { - volread!(config, capacity_low) as u64 | (volread!(config, capacity_high) as u64) << 32 - }; + let capacity = transport.read_config_space::(offset_of!(BlkConfig, capacity_low)) + as u64 + | (transport.read_config_space::(offset_of!(BlkConfig, capacity_high)) as u64) + << 32; info!("found a block device of size {}KB", capacity / 2); let queue = VirtQueue::new( diff --git a/src/device/console.rs b/src/device/console.rs index ad7431b7..5f5ff087 100644 --- a/src/device/console.rs +++ b/src/device/console.rs @@ -6,14 +6,12 @@ mod embedded_io; use crate::hal::Hal; use crate::queue::VirtQueue; use crate::transport::Transport; -use crate::volatile::{volread, volwrite, ReadOnly, WriteOnly}; +use crate::volatile::{ReadOnly, WriteOnly}; use crate::{Error, Result, PAGE_SIZE}; use alloc::boxed::Box; use bitflags::bitflags; -use core::{ - fmt::{self, Display, Formatter, Write}, - ptr::NonNull, -}; +use core::fmt::{self, Display, Formatter, Write}; +use core::mem::offset_of; use log::error; const QUEUE_RECEIVEQ_PORT_0: u16 = 0; @@ -51,7 +49,6 @@ const SUPPORTED_FEATURES: Features = Features::RING_EVENT_IDX pub struct VirtIOConsole { transport: T, negotiated_features: Features, - config_space: NonNull, receiveq: VirtQueue, transmitq: VirtQueue, queue_buf_rx: Box<[u8; PAGE_SIZE]>, @@ -94,7 +91,6 @@ impl VirtIOConsole { /// Creates a new VirtIO console driver. pub fn new(mut transport: T) -> Result { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); - let config_space = transport.config_space::()?; let receiveq = VirtQueue::new( &mut transport, QUEUE_RECEIVEQ_PORT_0, @@ -117,7 +113,6 @@ impl VirtIOConsole { let mut console = VirtIOConsole { transport, negotiated_features, - config_space, receiveq, transmitq, queue_buf_rx, @@ -132,13 +127,10 @@ impl VirtIOConsole { /// Returns the size of the console, if the device supports reporting this. pub fn size(&self) -> Option { if self.negotiated_features.contains(Features::SIZE) { - // SAFETY: self.config_space is a valid pointer to the device configuration space. - unsafe { - Some(Size { - columns: volread!(self.config_space, cols), - rows: volread!(self.config_space, rows), - }) - } + Some(Size { + columns: self.transport.read_config_space(offset_of!(Config, cols)), + rows: self.transport.read_config_space(offset_of!(Config, rows)), + }) } else { None } @@ -246,10 +238,8 @@ impl VirtIOConsole { /// Returns an error if the device doesn't support emergency write. pub fn emergency_write(&mut self, chr: u8) -> Result<()> { if self.negotiated_features.contains(Features::EMERG_WRITE) { - // SAFETY: `self.config_space` is a valid pointer to the device configuration space. - unsafe { - volwrite!(self.config_space, emerg_wr, chr.into()); - } + self.transport + .write_config_space::(offset_of!(Config, emerg_wr), chr.into()); Ok(()) } else { Err(Error::Unsupported) diff --git a/src/device/gpu.rs b/src/device/gpu.rs index 6c1cd7db..6db57e9f 100644 --- a/src/device/gpu.rs +++ b/src/device/gpu.rs @@ -3,10 +3,11 @@ use crate::hal::{BufferDirection, Dma, Hal}; use crate::queue::VirtQueue; use crate::transport::Transport; -use crate::volatile::{volread, ReadOnly, Volatile, WriteOnly}; +use crate::volatile::{ReadOnly, Volatile, WriteOnly}; use crate::{pages, Error, Result, PAGE_SIZE}; use alloc::boxed::Box; use bitflags::bitflags; +use core::mem::offset_of; use log::info; use zerocopy::{FromBytes, FromZeros, Immutable, IntoBytes, KnownLayout}; @@ -43,15 +44,12 @@ impl VirtIOGpu { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); // read configuration space - let config_space = transport.config_space::()?; - unsafe { - let events_read = volread!(config_space, events_read); - let num_scanouts = volread!(config_space, num_scanouts); - info!( - "events_read: {:#x}, num_scanouts: {:#x}", - events_read, 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 + ); let control_queue = VirtQueue::new( &mut transport, diff --git a/src/device/input.rs b/src/device/input.rs index 4891c9f4..8f115693 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -4,12 +4,11 @@ use super::common::Feature; use crate::hal::Hal; use crate::queue::VirtQueue; use crate::transport::Transport; -use crate::volatile::{volread, volwrite, ReadOnly, VolatileReadable, WriteOnly}; +use crate::volatile::{ReadOnly, WriteOnly}; use crate::Error; use alloc::{boxed::Box, string::String}; use core::cmp::min; -use core::mem::size_of; -use core::ptr::{addr_of, NonNull}; +use core::mem::{offset_of, size_of}; use zerocopy::{FromBytes, FromZeros, Immutable, IntoBytes, KnownLayout}; /// Virtual human interface devices such as keyboards, mice and tablets. @@ -22,7 +21,6 @@ pub struct VirtIOInput { event_queue: VirtQueue, status_queue: VirtQueue, event_buf: Box<[InputEvent; 32]>, - config: NonNull, } impl VirtIOInput { @@ -32,8 +30,6 @@ impl VirtIOInput { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); - let config = transport.config_space::()?; - let mut event_queue = VirtQueue::new( &mut transport, QUEUE_EVENT, @@ -62,7 +58,6 @@ impl VirtIOInput { event_queue, status_queue, event_buf, - config, }) } @@ -108,17 +103,19 @@ impl VirtIOInput { subsel: u8, out: &mut [u8], ) -> u8 { - let size; + self.transport + .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)); // Safe because config points to a valid MMIO region for the config space. - unsafe { - volwrite!(self.config, select, select as u8); - volwrite!(self.config, subsel, subsel); - size = volread!(self.config, size); - 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 = addr_of!((*self.config.as_ptr()).data[i]).vread(); - } + 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::()); } + size } @@ -129,20 +126,24 @@ impl VirtIOInput { select: InputConfigSelect, subsel: u8, ) -> Result, Error> { - // Safe because config points to a valid MMIO region for the config space. - unsafe { - volwrite!(self.config, select, select as u8); - volwrite!(self.config, subsel, subsel); - let size = usize::from(volread!(self.config, size)); - if size > CONFIG_DATA_MAX_LENGTH { - return Err(Error::IoError); - } - let mut buf = <[u8]>::new_box_zeroed_with_elems(size).unwrap(); - for i in 0..size { - buf[i] = addr_of!((*self.config.as_ptr()).data[i]).vread(); - } - Ok(buf) + self.transport + .write_config_space(offset_of!(Config, select), select as u8); + self.transport + .write_config_space(offset_of!(Config, subsel), subsel); + let size = usize::from( + self.transport + .read_config_space::(offset_of!(Config, size)), + ); + if size > CONFIG_DATA_MAX_LENGTH { + return Err(Error::IoError); + } + let mut buf = <[u8]>::new_box_zeroed_with_elems(size).unwrap(); + for i in 0..size { + buf[i] = self + .transport + .read_config_space(offset_of!(Config, data) + i * size_of::()); } + Ok(buf) } /// Queries a specific piece of information by `select` and `subsel` into a newly-allocated @@ -322,7 +323,7 @@ mod tests { }, }; use alloc::{sync::Arc, vec}; - use core::convert::TryInto; + use core::{convert::TryInto, ptr::NonNull}; use std::sync::Mutex; #[test] diff --git a/src/device/net/dev_raw.rs b/src/device/net/dev_raw.rs index 1f4b4c81..0c7f0ce4 100644 --- a/src/device/net/dev_raw.rs +++ b/src/device/net/dev_raw.rs @@ -1,10 +1,11 @@ use super::{Config, EthernetAddress, Features, VirtioNetHdr}; use super::{MIN_BUFFER_LEN, NET_HDR_SIZE, QUEUE_RECEIVE, QUEUE_TRANSMIT, SUPPORTED_FEATURES}; +use crate::device::net::Status; use crate::hal::Hal; use crate::queue::VirtQueue; use crate::transport::Transport; -use crate::volatile::volread; use crate::{Error, Result}; +use core::mem::offset_of; use log::{debug, info, warn}; use zerocopy::IntoBytes; @@ -28,18 +29,12 @@ impl VirtIONetRaw Result { let negotiated_features = transport.begin_init(SUPPORTED_FEATURES); info!("negotiated_features {:?}", negotiated_features); - // read configuration space - let config = transport.config_space::()?; - let mac; - // Safe because config points to a valid MMIO region for the config space. - unsafe { - mac = volread!(config, mac); - debug!( - "Got MAC={:02x?}, status={:?}", - mac, - volread!(config, status) - ); - } + + // Read configuration space. + 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( &mut transport, QUEUE_TRANSMIT, diff --git a/src/device/net/mod.rs b/src/device/net/mod.rs index d40a2d4f..d55a93b2 100644 --- a/src/device/net/mod.rs +++ b/src/device/net/mod.rs @@ -81,9 +81,12 @@ bitflags! { } } +#[derive(Copy, Clone, Debug, Default, Eq, FromBytes, Immutable, KnownLayout, PartialEq)] +#[repr(transparent)] +struct Status(u16); + bitflags! { - #[derive(Copy, Clone, Debug, Default, Eq, PartialEq)] - struct Status: u16 { + impl Status: u16 { const LINK_UP = 1; const ANNOUNCE = 2; } diff --git a/src/device/socket/vsock.rs b/src/device/socket/vsock.rs index a90ee13f..5c301164 100644 --- a/src/device/socket/vsock.rs +++ b/src/device/socket/vsock.rs @@ -9,9 +9,8 @@ use super::DEFAULT_RX_BUFFER_SIZE; use crate::hal::Hal; use crate::queue::{owning::OwningQueue, VirtQueue}; use crate::transport::Transport; -use crate::volatile::volread; use crate::Result; -use core::mem::size_of; +use core::mem::{offset_of, size_of}; use log::debug; use zerocopy::{FromBytes, IntoBytes}; @@ -248,12 +247,12 @@ impl VirtIOSocket()?; - debug!("config: {:?}", config); // Safe because config is a valid pointer to the device configuration space. - let guest_cid = unsafe { - volread!(config, guest_cid_low) as u64 | (volread!(config, 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 b055d280..e5b10a6f 100644 --- a/src/device/sound.rs +++ b/src/device/sound.rs @@ -7,7 +7,7 @@ use super::common::Feature; use crate::{ queue::{owning::OwningQueue, VirtQueue}, transport::Transport, - volatile::{volread, ReadOnly}, + volatile::ReadOnly, Error, Hal, Result, PAGE_SIZE, }; use alloc::{boxed::Box, collections::BTreeMap, vec, vec::Vec}; @@ -16,7 +16,7 @@ use core::{ array, fmt::{self, Debug, Display, Formatter}, hint::spin_loop, - mem::size_of, + mem::{offset_of, size_of}, ops::RangeInclusive, }; use enumn::N; @@ -96,15 +96,9 @@ impl VirtIOSound { )?; // read configuration space - let config_ptr = transport.config_space::()?; - // SAFETY: config_ptr is a valid pointer to the device configuration space. - let (jacks, streams, chmaps) = unsafe { - ( - volread!(config_ptr, jacks), - volread!(config_ptr, streams), - volread!(config_ptr, 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 77ed8f9b..a23d7120 100644 --- a/src/transport/fake.rs +++ b/src/transport/fake.rs @@ -1,11 +1,10 @@ use super::{DeviceStatus, DeviceType, Transport}; use crate::{ queue::{fake_read_write_queue, Descriptor}, - PhysAddr, Result, + PhysAddr, }; use alloc::{sync::Arc, vec::Vec}; use core::{ - any::TypeId, ptr::NonNull, sync::atomic::{AtomicBool, Ordering}, time::Duration, @@ -97,11 +96,23 @@ impl Transport for FakeTransport { pending } - fn config_space(&self) -> Result> { - if TypeId::of::() == TypeId::of::() { - Ok(self.config_space.cast()) - } else { - panic!("Unexpected config space type."); + fn read_config_space(&self, offset: usize) -> T { + 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() } + } + + fn write_config_space(&mut self, offset: usize, value: T) { + 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); } } } diff --git a/src/transport/mmio.rs b/src/transport/mmio.rs index 434060e3..2370feb9 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}, - Error, PhysAddr, PAGE_SIZE, + PhysAddr, PAGE_SIZE, }; use core::{ convert::{TryFrom, TryInto}, @@ -484,15 +484,36 @@ impl Transport for MmioTransport { } } - fn config_space(&self) -> Result, Error> { - if align_of::() > 4 { - // Panic as this should only happen if the driver is written incorrectly. - panic!( - "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", - align_of::() - ); + fn read_config_space(&self, offset: usize) -> T { + 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 + .cast::() + .byte_add(CONFIG_SPACE_OFFSET) + .byte_add(offset) + .read_volatile() + } + } + + fn write_config_space(&mut self, offset: usize, value: T) { + 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 + .cast::() + .byte_add(CONFIG_SPACE_OFFSET) + .byte_add(offset) + .write_volatile(value); } - Ok(NonNull::new((self.header.as_ptr() as usize + CONFIG_SPACE_OFFSET) as _).unwrap()) } } diff --git a/src/transport/mod.rs b/src/transport/mod.rs index 69234750..beaab6fd 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -5,10 +5,11 @@ pub mod fake; pub mod mmio; pub mod pci; -use crate::{PhysAddr, Result, PAGE_SIZE}; +use crate::{PhysAddr, PAGE_SIZE}; use bitflags::{bitflags, Flags}; -use core::{fmt::Debug, ops::BitAnd, ptr::NonNull}; +use core::{fmt::Debug, ops::BitAnd}; use log::debug; +use zerocopy::{FromBytes, IntoBytes}; /// A VirtIO transport layer. pub trait Transport { @@ -98,8 +99,11 @@ pub trait Transport { ); } - /// Gets the pointer to the config space. - fn config_space(&self) -> Result>; + /// Reads a value from the device config space. + fn read_config_space(&self, offset: usize) -> T; + + /// Writes a value to the device config space. + fn write_config_space(&mut self, offset: usize, value: T); } bitflags! { diff --git a/src/transport/pci.rs b/src/transport/pci.rs index fbbb9524..cf39e445 100644 --- a/src/transport/pci.rs +++ b/src/transport/pci.rs @@ -12,7 +12,6 @@ use crate::{ volatile::{ volread, volwrite, ReadOnly, Volatile, VolatileReadable, VolatileWritable, WriteOnly, }, - Error, }; use core::{ mem::{align_of, size_of}, @@ -325,23 +324,41 @@ impl Transport for PciTransport { isr_status & 0x3 != 0 } - fn config_space(&self) -> Result, Error> { - if let Some(config_space) = self.config_space { - if size_of::() > config_space.len() * size_of::() { - Err(Error::ConfigSpaceTooSmall) - } else if align_of::() > 4 { - // Panic as this should only happen if the driver is written incorrectly. - panic!( - "Driver expected config space alignment of {} bytes, but VirtIO only guarantees 4 byte alignment.", - align_of::() - ); - } else { - // TODO: Use NonNull::as_non_null_ptr once it is stable. - let config_space_ptr = NonNull::new(config_space.as_ptr() as *mut u32).unwrap(); - Ok(config_space_ptr.cast()) - } - } else { - Err(Error::ConfigSpaceMissing) + fn read_config_space(&self, offset: usize) -> T { + 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() + } + } + + fn write_config_space(&mut self, offset: usize, value: T) { + 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); } } } From 3bcb7214102c9dfda2c57a1ca021b863ee4416fc Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Mon, 28 Oct 2024 15:49:39 +0000 Subject: [PATCH 2/2] Return Result from config space read/write methods. This also makes some device methods return a Result. --- examples/aarch64/src/main.rs | 2 +- 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 ++++++++++++++++++++---------------- 12 files changed, 97 insertions(+), 76 deletions(-) diff --git a/examples/aarch64/src/main.rs b/examples/aarch64/src/main.rs index 0158d686..2d7ad046 100644 --- a/examples/aarch64/src/main.rs +++ b/examples/aarch64/src/main.rs @@ -209,7 +209,7 @@ fn virtio_net(transport: T) { fn virtio_console(transport: T) { let mut console = VirtIOConsole::::new(transport).expect("Failed to create console driver"); - if let Some(size) = console.size() { + if let Some(size) = console.size().unwrap() { info!("VirtIO console {}", size); } for &c in b"Hello world on console!\n" { diff --git a/src/device/blk.rs b/src/device/blk.rs index 71e18b99..1ac4803b 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 6db57e9f..d299a990 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 8f115693..6259e391 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_mut_bytes()); + let size = self.query_config_select(InputConfigSelect::IdDevids, 0, ids.as_mut_bytes())?; 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_mut_bytes()); + let size = + self.query_config_select(InputConfigSelect::AbsInfo, axis, info.as_mut_bytes())?; 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 0c7f0ce4..8f478673 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 5c301164..0566b2e7 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 e5b10a6f..1bd73e8c 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 2370feb9..1d00f751 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}, @@ -484,27 +484,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 { @@ -514,6 +517,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 beaab6fd..f166ac3c 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 cf39e445..5ac41145 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::{ mem::{align_of, size_of}, @@ -324,41 +325,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(()) } } }