From 8de8330f41e44b8e292e31361892251a194271be Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 27 Nov 2024 13:52:00 +0000 Subject: [PATCH 1/7] Add macros to read and write config space in a type-safe way. --- src/device/blk.rs | 11 +++------- src/device/console.rs | 10 ++++----- src/device/gpu.rs | 7 +++---- src/device/input.rs | 21 +++++++------------ src/device/net/dev_raw.rs | 9 +++----- src/device/socket/vsock.rs | 12 ++++------- src/device/sound.rs | 10 ++++----- src/transport/mod.rs | 43 ++++++++++++++++++++++++++++++++++++++ src/volatile.rs | 2 +- 9 files changed, 73 insertions(+), 52 deletions(-) diff --git a/src/device/blk.rs b/src/device/blk.rs index 6fc9d252..d8e8d029 100644 --- a/src/device/blk.rs +++ b/src/device/blk.rs @@ -2,11 +2,10 @@ use crate::hal::Hal; use crate::queue::VirtQueue; -use crate::transport::Transport; +use crate::transport::{read_config, Transport}; 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}; @@ -57,12 +56,8 @@ impl VirtIOBlk { // Read configuration space. let capacity = transport.read_consistent(|| { - Ok( - transport.read_config_space::(offset_of!(BlkConfig, capacity_low))? as u64 - | (transport.read_config_space::(offset_of!(BlkConfig, capacity_high))? - as u64) - << 32, - ) + Ok(read_config!(transport, BlkConfig, capacity_low)? as u64 + | (read_config!(transport, 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 a8d6095a..8aaa520d 100644 --- a/src/device/console.rs +++ b/src/device/console.rs @@ -5,13 +5,12 @@ mod embedded_io; use crate::hal::Hal; use crate::queue::VirtQueue; -use crate::transport::Transport; +use crate::transport::{read_config, write_config, Transport}; 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}; -use core::mem::offset_of; use log::error; const QUEUE_RECEIVEQ_PORT_0: u16 = 0; @@ -129,8 +128,8 @@ impl VirtIOConsole { if self.negotiated_features.contains(Features::SIZE) { self.transport.read_consistent(|| { Ok(Some(Size { - columns: self.transport.read_config_space(offset_of!(Config, cols))?, - rows: self.transport.read_config_space(offset_of!(Config, rows))?, + columns: read_config!(self.transport, Config, cols)?, + rows: read_config!(self.transport, Config, rows)?, })) }) } else { @@ -240,8 +239,7 @@ 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) { - self.transport - .write_config_space::(offset_of!(Config, emerg_wr), chr.into())?; + write_config!(self.transport, Config, emerg_wr, chr.into())?; Ok(()) } else { Err(Error::Unsupported) diff --git a/src/device/gpu.rs b/src/device/gpu.rs index d299a990..4dc8344a 100644 --- a/src/device/gpu.rs +++ b/src/device/gpu.rs @@ -2,12 +2,11 @@ use crate::hal::{BufferDirection, Dma, Hal}; use crate::queue::VirtQueue; -use crate::transport::Transport; +use crate::transport::{read_config, Transport}; 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}; @@ -44,8 +43,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 = read_config!(transport, Config, events_read)?; + let num_scanouts = read_config!(transport, 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 6259e391..fedb220c 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -3,7 +3,7 @@ use super::common::Feature; use crate::hal::Hal; use crate::queue::VirtQueue; -use crate::transport::Transport; +use crate::transport::{read_config, write_config, Transport}; use crate::volatile::{ReadOnly, WriteOnly}; use crate::Error; use alloc::{boxed::Box, string::String}; @@ -103,11 +103,9 @@ impl VirtIOInput { subsel: u8, out: &mut [u8], ) -> Result { - 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))?; + write_config!(self.transport, Config, select, select as u8)?; + write_config!(self.transport, Config, subsel, subsel)?; + let size: u8 = read_config!(self.transport, 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() { @@ -126,14 +124,9 @@ impl VirtIOInput { select: InputConfigSelect, subsel: u8, ) -> Result, Error> { - 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))?, - ); + write_config!(self.transport, Config, select, select as u8)?; + write_config!(self.transport, Config, subsel, subsel)?; + let size = usize::from(read_config!(self.transport, Config, size)?); if size > CONFIG_DATA_MAX_LENGTH { return Err(Error::IoError); } diff --git a/src/device/net/dev_raw.rs b/src/device/net/dev_raw.rs index 58d30d8d..f74eeba7 100644 --- a/src/device/net/dev_raw.rs +++ b/src/device/net/dev_raw.rs @@ -1,11 +1,9 @@ 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::transport::{read_config, Transport}; use crate::{Error, Result}; -use core::mem::offset_of; use log::{debug, info, warn}; use zerocopy::IntoBytes; @@ -31,9 +29,8 @@ impl VirtIONetRaw(offset_of!(Config, status))?; + let mac = transport.read_consistent(|| read_config!(transport, Config, mac))?; + let status = read_config!(transport, 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 5270e82e..bc99add7 100644 --- a/src/device/socket/vsock.rs +++ b/src/device/socket/vsock.rs @@ -8,9 +8,9 @@ use super::protocol::{ use super::DEFAULT_RX_BUFFER_SIZE; use crate::hal::Hal; use crate::queue::{owning::OwningQueue, VirtQueue}; -use crate::transport::Transport; +use crate::transport::{read_config, Transport}; use crate::Result; -use core::mem::{offset_of, size_of}; +use core::mem::size_of; use log::debug; use zerocopy::{FromBytes, IntoBytes}; @@ -249,12 +249,8 @@ impl VirtIOSocket(offset_of!(VirtioVsockConfig, guest_cid_low))? - as u64 - | (transport - .read_config_space::(offset_of!(VirtioVsockConfig, guest_cid_high))? - as u64) - << 32, + read_config!(transport, VirtioVsockConfig, guest_cid_low)? as u64 + | (read_config!(transport, VirtioVsockConfig, guest_cid_high)? as u64) << 32, ) })?; debug!("guest cid: {guest_cid:?}"); diff --git a/src/device/sound.rs b/src/device/sound.rs index 1bd73e8c..07218044 100644 --- a/src/device/sound.rs +++ b/src/device/sound.rs @@ -6,7 +6,7 @@ mod fake; use super::common::Feature; use crate::{ queue::{owning::OwningQueue, VirtQueue}, - transport::Transport, + transport::{read_config, Transport}, volatile::ReadOnly, Error, Hal, Result, PAGE_SIZE, }; @@ -16,7 +16,7 @@ use core::{ array, fmt::{self, Debug, Display, Formatter}, hint::spin_loop, - mem::{offset_of, size_of}, + mem::size_of, ops::RangeInclusive, }; use enumn::N; @@ -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 = read_config!(transport, VirtIOSoundConfig, jacks)?; + let streams = read_config!(transport, VirtIOSoundConfig, streams)?; + let chmaps = read_config!(transport, VirtIOSoundConfig, chmaps)?; info!( "[sound device] config: jacks: {}, streams: {}, chmaps: {}", jacks, streams, chmaps diff --git a/src/transport/mod.rs b/src/transport/mod.rs index b9fb7dfa..2f66a916 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -231,3 +231,46 @@ impl From for DeviceType { u32::from(virtio_device_id).into() } } + +#[inline(always)] +pub(crate) fn read_help( + transport: &T, + offset: usize, + _dummy_v: Option, +) -> Result { + transport.read_config_space(offset) +} + +#[inline(always)] +pub(crate) fn write_help( + transport: &mut T, + offset: usize, + value: V, + _dummy_v: Option, +) -> Result<()> { + transport.write_config_space(offset, value) +} + +macro_rules! read_config { + ($transport:expr, $struct:ty, $field:ident) => {{ + let dummy_struct: Option<$struct> = None; + let dummy_v = dummy_struct.map(|s| s.$field.0); + crate::transport::read_help(&$transport, core::mem::offset_of!($struct, $field), dummy_v) + }}; +} + +macro_rules! write_config { + ($transport:expr, $struct:ty, $field:ident, $value:expr) => {{ + let dummy_struct: Option<$struct> = None; + let dummy_v = dummy_struct.map(|s| s.$field.0); + crate::transport::write_help( + &mut $transport, + core::mem::offset_of!($struct, $field), + $value, + dummy_v, + ) + }}; +} + +pub(crate) use read_config; +pub(crate) use write_config; diff --git a/src/volatile.rs b/src/volatile.rs index 74f527b3..0acbad24 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -18,7 +18,7 @@ pub struct WriteOnly(pub(crate) T); /// An MMIO register which may be both read and written. #[derive(Default)] #[repr(transparent)] -pub struct Volatile(T); +pub struct Volatile(pub(crate) T); impl Volatile { /// Construct a new instance for testing. From 8d19ccf91dda94266370701516c229330f16847e Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 27 Nov 2024 16:17:35 +0000 Subject: [PATCH 2/7] Add doc comments. --- src/transport/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/transport/mod.rs b/src/transport/mod.rs index 2f66a916..f04fe61f 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -232,6 +232,8 @@ impl From for DeviceType { } } +/// Wrapper for Transport::read_config_space with an extra dummy parameter to force the correct type +/// to be inferred. #[inline(always)] pub(crate) fn read_help( transport: &T, @@ -241,6 +243,8 @@ pub(crate) fn read_help( transport.read_config_space(offset) } +/// Wrapper for Transport::write_config_space with an extra dummy parameter to force the correct +/// type to be inferred. #[inline(always)] pub(crate) fn write_help( transport: &mut T, @@ -251,6 +255,7 @@ pub(crate) fn write_help( transport.write_config_space(offset, value) } +/// Reads the given field of the given struct from the device config space via the given transport. macro_rules! read_config { ($transport:expr, $struct:ty, $field:ident) => {{ let dummy_struct: Option<$struct> = None; @@ -259,6 +264,7 @@ macro_rules! read_config { }}; } +/// Writes the given field of the given struct from the device config space via the given transport. macro_rules! write_config { ($transport:expr, $struct:ty, $field:ident, $value:expr) => {{ let dummy_struct: Option<$struct> = None; From 12316293bc372c097cef29bfdd713eaa7ac6bb56 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 27 Nov 2024 16:23:08 +0000 Subject: [PATCH 3/7] Only read or write config fields which are readable or writable. --- src/transport/mod.rs | 39 ++++++++++++++++++++++++++------------- src/volatile.rs | 2 +- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/transport/mod.rs b/src/transport/mod.rs index f04fe61f..7d456764 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -6,7 +6,10 @@ pub mod mmio; pub mod pci; mod some; -use crate::{PhysAddr, Result, PAGE_SIZE}; +use crate::{ + volatile::{VolatileReadable, VolatileWritable}, + PhysAddr, Result, PAGE_SIZE, +}; use bitflags::{bitflags, Flags}; use core::{fmt::Debug, ops::BitAnd}; use log::debug; @@ -235,23 +238,29 @@ impl From for DeviceType { /// Wrapper for Transport::read_config_space with an extra dummy parameter to force the correct type /// to be inferred. #[inline(always)] -pub(crate) fn read_help( - transport: &T, - offset: usize, - _dummy_v: Option, -) -> Result { +pub(crate) fn read_help(transport: &T, offset: usize, _dummy_r: Option) -> Result +where + T: Transport, + V: FromBytes, + *const R: VolatileReadable, +{ transport.read_config_space(offset) } /// Wrapper for Transport::write_config_space with an extra dummy parameter to force the correct /// type to be inferred. #[inline(always)] -pub(crate) fn write_help( +pub(crate) fn write_help( transport: &mut T, offset: usize, value: V, - _dummy_v: Option, -) -> Result<()> { + _dummy_w: Option, +) -> Result<()> +where + T: Transport, + V: Immutable + IntoBytes, + *mut W: VolatileWritable, +{ transport.write_config_space(offset, value) } @@ -259,8 +268,12 @@ pub(crate) fn write_help( macro_rules! read_config { ($transport:expr, $struct:ty, $field:ident) => {{ let dummy_struct: Option<$struct> = None; - let dummy_v = dummy_struct.map(|s| s.$field.0); - crate::transport::read_help(&$transport, core::mem::offset_of!($struct, $field), dummy_v) + let dummy_field = dummy_struct.map(|s| s.$field); + crate::transport::read_help( + &$transport, + core::mem::offset_of!($struct, $field), + dummy_field, + ) }}; } @@ -268,12 +281,12 @@ macro_rules! read_config { macro_rules! write_config { ($transport:expr, $struct:ty, $field:ident, $value:expr) => {{ let dummy_struct: Option<$struct> = None; - let dummy_v = dummy_struct.map(|s| s.$field.0); + let dummy_field = dummy_struct.map(|s| s.$field); crate::transport::write_help( &mut $transport, core::mem::offset_of!($struct, $field), $value, - dummy_v, + dummy_field, ) }}; } diff --git a/src/volatile.rs b/src/volatile.rs index 0acbad24..74f527b3 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -18,7 +18,7 @@ pub struct WriteOnly(pub(crate) T); /// An MMIO register which may be both read and written. #[derive(Default)] #[repr(transparent)] -pub struct Volatile(pub(crate) T); +pub struct Volatile(T); impl Volatile { /// Construct a new instance for testing. From 65b8c6fe333fe9c08100ccffd92e7ce81c924883 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 27 Nov 2024 18:29:07 +0000 Subject: [PATCH 4/7] Correct writability of some config space registers. --- src/device/blk.rs | 146 +++++++++++++++++++++++----------------------- src/device/gpu.rs | 4 +- 2 files changed, 75 insertions(+), 75 deletions(-) diff --git a/src/device/blk.rs b/src/device/blk.rs index d8e8d029..baf87ca1 100644 --- a/src/device/blk.rs +++ b/src/device/blk.rs @@ -3,7 +3,7 @@ use crate::hal::Hal; use crate::queue::VirtQueue; use crate::transport::{read_config, Transport}; -use crate::volatile::Volatile; +use crate::volatile::ReadOnly; use crate::{Error, Result}; use bitflags::bitflags; use log::info; @@ -391,18 +391,18 @@ impl Drop for VirtIOBlk { #[repr(C)] struct BlkConfig { /// Number of 512 Bytes sectors - capacity_low: Volatile, - capacity_high: Volatile, - size_max: Volatile, - seg_max: Volatile, - cylinders: Volatile, - heads: Volatile, - sectors: Volatile, - blk_size: Volatile, - physical_block_exp: Volatile, - alignment_offset: Volatile, - min_io_size: Volatile, - opt_io_size: Volatile, + capacity_low: ReadOnly, + capacity_high: ReadOnly, + size_max: ReadOnly, + seg_max: ReadOnly, + cylinders: ReadOnly, + heads: ReadOnly, + sectors: ReadOnly, + blk_size: ReadOnly, + physical_block_exp: ReadOnly, + alignment_offset: ReadOnly, + min_io_size: ReadOnly, + opt_io_size: ReadOnly, // ... ignored } @@ -565,18 +565,18 @@ mod tests { #[test] fn config() { let mut config_space = BlkConfig { - capacity_low: Volatile::new(0x42), - capacity_high: Volatile::new(0x02), - size_max: Volatile::new(0), - seg_max: Volatile::new(0), - cylinders: Volatile::new(0), - heads: Volatile::new(0), - sectors: Volatile::new(0), - blk_size: Volatile::new(0), - physical_block_exp: Volatile::new(0), - alignment_offset: Volatile::new(0), - min_io_size: Volatile::new(0), - opt_io_size: Volatile::new(0), + capacity_low: ReadOnly::new(0x42), + capacity_high: ReadOnly::new(0x02), + size_max: ReadOnly::new(0), + seg_max: ReadOnly::new(0), + cylinders: ReadOnly::new(0), + heads: ReadOnly::new(0), + sectors: ReadOnly::new(0), + blk_size: ReadOnly::new(0), + physical_block_exp: ReadOnly::new(0), + alignment_offset: ReadOnly::new(0), + min_io_size: ReadOnly::new(0), + opt_io_size: ReadOnly::new(0), }; let state = Arc::new(Mutex::new(State { queues: vec![QueueStatus::default()], @@ -598,18 +598,18 @@ mod tests { #[test] fn read() { let mut config_space = BlkConfig { - capacity_low: Volatile::new(66), - capacity_high: Volatile::new(0), - size_max: Volatile::new(0), - seg_max: Volatile::new(0), - cylinders: Volatile::new(0), - heads: Volatile::new(0), - sectors: Volatile::new(0), - blk_size: Volatile::new(0), - physical_block_exp: Volatile::new(0), - alignment_offset: Volatile::new(0), - min_io_size: Volatile::new(0), - opt_io_size: Volatile::new(0), + capacity_low: ReadOnly::new(66), + capacity_high: ReadOnly::new(0), + size_max: ReadOnly::new(0), + seg_max: ReadOnly::new(0), + cylinders: ReadOnly::new(0), + heads: ReadOnly::new(0), + sectors: ReadOnly::new(0), + blk_size: ReadOnly::new(0), + physical_block_exp: ReadOnly::new(0), + alignment_offset: ReadOnly::new(0), + min_io_size: ReadOnly::new(0), + opt_io_size: ReadOnly::new(0), }; let state = Arc::new(Mutex::new(State { queues: vec![QueueStatus::default()], @@ -668,18 +668,18 @@ mod tests { #[test] fn write() { let mut config_space = BlkConfig { - capacity_low: Volatile::new(66), - capacity_high: Volatile::new(0), - size_max: Volatile::new(0), - seg_max: Volatile::new(0), - cylinders: Volatile::new(0), - heads: Volatile::new(0), - sectors: Volatile::new(0), - blk_size: Volatile::new(0), - physical_block_exp: Volatile::new(0), - alignment_offset: Volatile::new(0), - min_io_size: Volatile::new(0), - opt_io_size: Volatile::new(0), + capacity_low: ReadOnly::new(66), + capacity_high: ReadOnly::new(0), + size_max: ReadOnly::new(0), + seg_max: ReadOnly::new(0), + cylinders: ReadOnly::new(0), + heads: ReadOnly::new(0), + sectors: ReadOnly::new(0), + blk_size: ReadOnly::new(0), + physical_block_exp: ReadOnly::new(0), + alignment_offset: ReadOnly::new(0), + min_io_size: ReadOnly::new(0), + opt_io_size: ReadOnly::new(0), }; let state = Arc::new(Mutex::new(State { queues: vec![QueueStatus::default()], @@ -743,18 +743,18 @@ mod tests { #[test] fn flush() { let mut config_space = BlkConfig { - capacity_low: Volatile::new(66), - capacity_high: Volatile::new(0), - size_max: Volatile::new(0), - seg_max: Volatile::new(0), - cylinders: Volatile::new(0), - heads: Volatile::new(0), - sectors: Volatile::new(0), - blk_size: Volatile::new(0), - physical_block_exp: Volatile::new(0), - alignment_offset: Volatile::new(0), - min_io_size: Volatile::new(0), - opt_io_size: Volatile::new(0), + capacity_low: ReadOnly::new(66), + capacity_high: ReadOnly::new(0), + size_max: ReadOnly::new(0), + seg_max: ReadOnly::new(0), + cylinders: ReadOnly::new(0), + heads: ReadOnly::new(0), + sectors: ReadOnly::new(0), + blk_size: ReadOnly::new(0), + physical_block_exp: ReadOnly::new(0), + alignment_offset: ReadOnly::new(0), + min_io_size: ReadOnly::new(0), + opt_io_size: ReadOnly::new(0), }; let state = Arc::new(Mutex::new(State { queues: vec![QueueStatus::default()], @@ -810,18 +810,18 @@ mod tests { #[test] fn device_id() { let mut config_space = BlkConfig { - capacity_low: Volatile::new(66), - capacity_high: Volatile::new(0), - size_max: Volatile::new(0), - seg_max: Volatile::new(0), - cylinders: Volatile::new(0), - heads: Volatile::new(0), - sectors: Volatile::new(0), - blk_size: Volatile::new(0), - physical_block_exp: Volatile::new(0), - alignment_offset: Volatile::new(0), - min_io_size: Volatile::new(0), - opt_io_size: Volatile::new(0), + capacity_low: ReadOnly::new(66), + capacity_high: ReadOnly::new(0), + size_max: ReadOnly::new(0), + seg_max: ReadOnly::new(0), + cylinders: ReadOnly::new(0), + heads: ReadOnly::new(0), + sectors: ReadOnly::new(0), + blk_size: ReadOnly::new(0), + physical_block_exp: ReadOnly::new(0), + alignment_offset: ReadOnly::new(0), + min_io_size: ReadOnly::new(0), + opt_io_size: ReadOnly::new(0), }; let state = Arc::new(Mutex::new(State { queues: vec![QueueStatus::default()], diff --git a/src/device/gpu.rs b/src/device/gpu.rs index 4dc8344a..30145b69 100644 --- a/src/device/gpu.rs +++ b/src/device/gpu.rs @@ -3,7 +3,7 @@ use crate::hal::{BufferDirection, Dma, Hal}; use crate::queue::VirtQueue; use crate::transport::{read_config, Transport}; -use crate::volatile::{ReadOnly, Volatile, WriteOnly}; +use crate::volatile::{ReadOnly, WriteOnly}; use crate::{pages, Error, Result, PAGE_SIZE}; use alloc::boxed::Box; use bitflags::bitflags; @@ -302,7 +302,7 @@ struct Config { /// Specifies the maximum number of scanouts supported by the device. /// /// Minimum value is 1, maximum value is 16. - num_scanouts: Volatile, + num_scanouts: ReadOnly, } /// Display configuration has changed. From a8bd90b42677510370c1357fa534dc66473da74a Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 27 Nov 2024 18:21:44 +0000 Subject: [PATCH 5/7] Use different types for config space vs. MMIO. Also moved helper macros to new module. --- src/config.rs | 114 +++++++++++++++++++++++++ src/device/blk.rs | 4 +- src/device/console.rs | 4 +- src/device/gpu.rs | 4 +- src/device/input.rs | 4 +- src/device/net/dev_raw.rs | 3 +- src/device/net/mod.rs | 2 +- src/device/socket/connectionmanager.rs | 2 +- src/device/socket/protocol.rs | 2 +- src/device/socket/vsock.rs | 5 +- src/device/sound.rs | 6 +- src/device/sound/fake.rs | 2 +- src/lib.rs | 1 + src/transport/mod.rs | 64 +------------- 14 files changed, 136 insertions(+), 81 deletions(-) create mode 100644 src/config.rs diff --git a/src/config.rs b/src/config.rs new file mode 100644 index 00000000..5461e78f --- /dev/null +++ b/src/config.rs @@ -0,0 +1,114 @@ +//! Types and macros for VirtIO device configuration space. + +use crate::{transport::Transport, Error}; +use zerocopy::{FromBytes, Immutable, IntoBytes}; + +/// A configuration space register from which the driver can only read. +#[derive(Default, FromBytes, Immutable, IntoBytes)] +#[repr(transparent)] +pub struct ReadOnly(pub T); + +impl ReadOnly { + /// Constructs a new instance for testing. + pub const fn new(value: T) -> Self { + Self(value) + } +} + +/// A configuration space register to which the driver can only write. +#[derive(Default, FromBytes, Immutable, IntoBytes)] +#[repr(transparent)] +pub struct WriteOnly(pub T); + +impl WriteOnly { + /// Constructs a new instance for testing. + pub const fn new(value: T) -> Self { + Self(value) + } +} + +/// A configuration space register which the driver may both read and write. +#[derive(Default, FromBytes, Immutable, IntoBytes)] +#[repr(transparent)] +pub struct ReadWrite(pub T); + +impl ReadWrite { + /// Constructs a new instance for testing. + pub const fn new(value: T) -> Self { + Self(value) + } +} + +/// Marker trait for configuration space registers from which the driver may read. +pub trait ConfigReadable {} + +/// Marker trait for configuration space registers to which the driver may write. +pub trait ConfigWritable {} + +impl ConfigReadable for ReadOnly {} +impl ConfigReadable for ReadWrite {} +impl ConfigWritable for ReadWrite {} +impl ConfigWritable for WriteOnly {} + +/// Wrapper for `Transport::read_config_space`` with an extra dummy parameter to force the correct +/// type to be inferred. +#[inline(always)] +pub(crate) fn read_help( + transport: &T, + offset: usize, + _dummy_r: Option, +) -> Result +where + T: Transport, + V: FromBytes, + R: ConfigReadable, +{ + transport.read_config_space(offset) +} + +/// Wrapper for Transport::write_config_space with an extra dummy parameter to force the correct +/// type to be inferred. +#[inline(always)] +pub(crate) fn write_help( + transport: &mut T, + offset: usize, + value: V, + _dummy_w: Option, +) -> Result<(), Error> +where + T: Transport, + V: Immutable + IntoBytes, + W: ConfigWritable, +{ + transport.write_config_space(offset, value) +} + +/// Reads the given field of the given struct from the device config space via the given transport. +macro_rules! read_config { + ($transport:expr, $struct:ty, $field:ident) => {{ + let dummy_struct: Option<$struct> = None; + let dummy_field = dummy_struct.map(|s| s.$field); + crate::config::read_help( + &$transport, + core::mem::offset_of!($struct, $field), + dummy_field, + ) + }}; +} + +/// Writes the given field of the given struct from the device config space via the given transport. +macro_rules! write_config { + ($transport:expr, $struct:ty, $field:ident, $value:expr) => {{ + let dummy_struct: Option<$struct> = None; + let dummy_field = dummy_struct.map(|s| s.$field); + crate::config::write_help( + &mut $transport, + core::mem::offset_of!($struct, $field), + $value, + dummy_field, + ) + }}; +} + +pub(crate) use read_config; +pub(crate) use write_config; diff --git a/src/device/blk.rs b/src/device/blk.rs index baf87ca1..bc9ceba6 100644 --- a/src/device/blk.rs +++ b/src/device/blk.rs @@ -1,9 +1,9 @@ //! Driver for VirtIO block devices. +use crate::config::{read_config, ReadOnly}; use crate::hal::Hal; use crate::queue::VirtQueue; -use crate::transport::{read_config, Transport}; -use crate::volatile::ReadOnly; +use crate::transport::Transport; use crate::{Error, Result}; use bitflags::bitflags; use log::info; diff --git a/src/device/console.rs b/src/device/console.rs index 8aaa520d..261effe2 100644 --- a/src/device/console.rs +++ b/src/device/console.rs @@ -3,10 +3,10 @@ #[cfg(feature = "embedded-io")] mod embedded_io; +use crate::config::{read_config, write_config, ReadOnly, WriteOnly}; use crate::hal::Hal; use crate::queue::VirtQueue; -use crate::transport::{read_config, write_config, Transport}; -use crate::volatile::{ReadOnly, WriteOnly}; +use crate::transport::Transport; use crate::{Error, Result, PAGE_SIZE}; use alloc::boxed::Box; use bitflags::bitflags; diff --git a/src/device/gpu.rs b/src/device/gpu.rs index 30145b69..75a5fefa 100644 --- a/src/device/gpu.rs +++ b/src/device/gpu.rs @@ -1,9 +1,9 @@ //! Driver for VirtIO GPU devices. +use crate::config::{read_config, ReadOnly, WriteOnly}; use crate::hal::{BufferDirection, Dma, Hal}; use crate::queue::VirtQueue; -use crate::transport::{read_config, Transport}; -use crate::volatile::{ReadOnly, WriteOnly}; +use crate::transport::Transport; use crate::{pages, Error, Result, PAGE_SIZE}; use alloc::boxed::Box; use bitflags::bitflags; diff --git a/src/device/input.rs b/src/device/input.rs index fedb220c..7b699d2b 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -1,10 +1,10 @@ //! Driver for VirtIO input devices. use super::common::Feature; +use crate::config::{read_config, write_config, ReadOnly, WriteOnly}; use crate::hal::Hal; use crate::queue::VirtQueue; -use crate::transport::{read_config, write_config, Transport}; -use crate::volatile::{ReadOnly, WriteOnly}; +use crate::transport::Transport; use crate::Error; use alloc::{boxed::Box, string::String}; use core::cmp::min; diff --git a/src/device/net/dev_raw.rs b/src/device/net/dev_raw.rs index f74eeba7..ca6fd21d 100644 --- a/src/device/net/dev_raw.rs +++ b/src/device/net/dev_raw.rs @@ -1,8 +1,9 @@ use super::{Config, EthernetAddress, Features, VirtioNetHdr}; use super::{MIN_BUFFER_LEN, NET_HDR_SIZE, QUEUE_RECEIVE, QUEUE_TRANSMIT, SUPPORTED_FEATURES}; +use crate::config::read_config; use crate::hal::Hal; use crate::queue::VirtQueue; -use crate::transport::{read_config, Transport}; +use crate::transport::Transport; use crate::{Error, Result}; use log::{debug, info, warn}; use zerocopy::IntoBytes; diff --git a/src/device/net/mod.rs b/src/device/net/mod.rs index d55a93b2..6adc7abf 100644 --- a/src/device/net/mod.rs +++ b/src/device/net/mod.rs @@ -10,7 +10,7 @@ pub use self::dev_raw::VirtIONetRaw; #[cfg(feature = "alloc")] pub use self::{dev::VirtIONet, net_buf::RxBuffer, net_buf::TxBuffer}; -use crate::volatile::ReadOnly; +use crate::config::ReadOnly; use bitflags::bitflags; use zerocopy::{FromBytes, Immutable, IntoBytes, KnownLayout}; diff --git a/src/device/socket/connectionmanager.rs b/src/device/socket/connectionmanager.rs index 96c7cd24..d38dc95e 100644 --- a/src/device/socket/connectionmanager.rs +++ b/src/device/socket/connectionmanager.rs @@ -415,6 +415,7 @@ impl RingBuffer { mod tests { use super::*; use crate::{ + config::ReadOnly, device::socket::{ protocol::{ SocketType, StreamShutdown, VirtioVsockConfig, VirtioVsockHdr, VirtioVsockOp, @@ -426,7 +427,6 @@ mod tests { fake::{FakeTransport, QueueStatus, State}, DeviceType, }, - volatile::ReadOnly, }; use alloc::{sync::Arc, vec}; use core::{mem::size_of, ptr::NonNull}; diff --git a/src/device/socket/protocol.rs b/src/device/socket/protocol.rs index af5b85df..8df924fc 100644 --- a/src/device/socket/protocol.rs +++ b/src/device/socket/protocol.rs @@ -1,7 +1,7 @@ //! This module defines the socket device protocol according to the virtio spec v1.1 5.10 Socket Device use super::error::{self, SocketError}; -use crate::volatile::ReadOnly; +use crate::config::ReadOnly; use bitflags::bitflags; use core::{ convert::{TryFrom, TryInto}, diff --git a/src/device/socket/vsock.rs b/src/device/socket/vsock.rs index bc99add7..63f7956f 100644 --- a/src/device/socket/vsock.rs +++ b/src/device/socket/vsock.rs @@ -6,9 +6,10 @@ use super::protocol::{ Feature, StreamShutdown, VirtioVsockConfig, VirtioVsockHdr, VirtioVsockOp, VsockAddr, }; use super::DEFAULT_RX_BUFFER_SIZE; +use crate::config::read_config; use crate::hal::Hal; use crate::queue::{owning::OwningQueue, VirtQueue}; -use crate::transport::{read_config, Transport}; +use crate::transport::Transport; use crate::Result; use core::mem::size_of; use log::debug; @@ -461,12 +462,12 @@ fn read_header_and_body(buffer: &[u8]) -> Result<(VirtioVsockHdr, &[u8])> { mod tests { use super::*; use crate::{ + config::ReadOnly, hal::fake::FakeHal, transport::{ fake::{FakeTransport, QueueStatus, State}, DeviceType, }, - volatile::ReadOnly, }; use alloc::{sync::Arc, vec}; use core::ptr::NonNull; diff --git a/src/device/sound.rs b/src/device/sound.rs index 07218044..d1b44f3a 100644 --- a/src/device/sound.rs +++ b/src/device/sound.rs @@ -5,9 +5,9 @@ mod fake; use super::common::Feature; use crate::{ + config::{read_config, ReadOnly}, queue::{owning::OwningQueue, VirtQueue}, - transport::{read_config, Transport}, - volatile::ReadOnly, + transport::Transport, Error, Hal, Result, PAGE_SIZE, }; use alloc::{boxed::Box, collections::BTreeMap, vec, vec::Vec}; @@ -1558,12 +1558,12 @@ impl Display for VirtIOSndChmapInfo { mod tests { use super::*; use crate::{ + config::ReadOnly, hal::fake::FakeHal, transport::{ fake::{FakeTransport, QueueStatus, State}, DeviceType, }, - volatile::ReadOnly, }; use alloc::{sync::Arc, vec}; use core::ptr::NonNull; diff --git a/src/device/sound/fake.rs b/src/device/sound/fake.rs index 15d81ad3..ab516514 100644 --- a/src/device/sound/fake.rs +++ b/src/device/sound/fake.rs @@ -6,12 +6,12 @@ use super::{ QUEUE_SIZE, TX_QUEUE_IDX, }; use crate::{ + config::ReadOnly, device::sound::{VirtIOSndPcmHdr, VirtIOSndPcmSetParams}, transport::{ fake::{FakeTransport, QueueStatus, State}, DeviceType, }, - volatile::ReadOnly, }; use alloc::{sync::Arc, vec}; use core::{ diff --git a/src/lib.rs b/src/lib.rs index aa3737df..0d38c94b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -49,6 +49,7 @@ #[cfg(any(feature = "alloc", test))] extern crate alloc; +mod config; pub mod device; #[cfg(feature = "embedded-io")] mod embedded_io; diff --git a/src/transport/mod.rs b/src/transport/mod.rs index 7d456764..b9fb7dfa 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -6,10 +6,7 @@ pub mod mmio; pub mod pci; mod some; -use crate::{ - volatile::{VolatileReadable, VolatileWritable}, - PhysAddr, Result, PAGE_SIZE, -}; +use crate::{PhysAddr, Result, PAGE_SIZE}; use bitflags::{bitflags, Flags}; use core::{fmt::Debug, ops::BitAnd}; use log::debug; @@ -234,62 +231,3 @@ impl From for DeviceType { u32::from(virtio_device_id).into() } } - -/// Wrapper for Transport::read_config_space with an extra dummy parameter to force the correct type -/// to be inferred. -#[inline(always)] -pub(crate) fn read_help(transport: &T, offset: usize, _dummy_r: Option) -> Result -where - T: Transport, - V: FromBytes, - *const R: VolatileReadable, -{ - transport.read_config_space(offset) -} - -/// Wrapper for Transport::write_config_space with an extra dummy parameter to force the correct -/// type to be inferred. -#[inline(always)] -pub(crate) fn write_help( - transport: &mut T, - offset: usize, - value: V, - _dummy_w: Option, -) -> Result<()> -where - T: Transport, - V: Immutable + IntoBytes, - *mut W: VolatileWritable, -{ - transport.write_config_space(offset, value) -} - -/// Reads the given field of the given struct from the device config space via the given transport. -macro_rules! read_config { - ($transport:expr, $struct:ty, $field:ident) => {{ - let dummy_struct: Option<$struct> = None; - let dummy_field = dummy_struct.map(|s| s.$field); - crate::transport::read_help( - &$transport, - core::mem::offset_of!($struct, $field), - dummy_field, - ) - }}; -} - -/// Writes the given field of the given struct from the device config space via the given transport. -macro_rules! write_config { - ($transport:expr, $struct:ty, $field:ident, $value:expr) => {{ - let dummy_struct: Option<$struct> = None; - let dummy_field = dummy_struct.map(|s| s.$field); - crate::transport::write_help( - &mut $transport, - core::mem::offset_of!($struct, $field), - $value, - dummy_field, - ) - }}; -} - -pub(crate) use read_config; -pub(crate) use write_config; From 2d052c336e41ce966c0259bd5fc417e2b9d44872 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 27 Nov 2024 18:01:15 +0000 Subject: [PATCH 6/7] Move fake config space to State for safe access. --- src/device/blk.rs | 58 +++++++++----------- src/device/console.rs | 60 ++++++++++---------- src/device/input.rs | 76 +++++++++++++++++--------- src/device/socket/connectionmanager.rs | 24 ++++---- src/device/socket/protocol.rs | 1 + src/device/socket/vsock.rs | 12 ++-- src/device/sound.rs | 13 ++--- src/device/sound/fake.rs | 14 ++--- src/queue.rs | 21 +------ src/transport/fake.rs | 60 +++++++++++++++----- 10 files changed, 183 insertions(+), 156 deletions(-) diff --git a/src/device/blk.rs b/src/device/blk.rs index bc9ceba6..dc4a6c52 100644 --- a/src/device/blk.rs +++ b/src/device/blk.rs @@ -388,6 +388,7 @@ impl Drop for VirtIOBlk { } } +#[derive(FromBytes, Immutable, IntoBytes)] #[repr(C)] struct BlkConfig { /// Number of 512 Bytes sectors @@ -559,12 +560,12 @@ mod tests { }, }; use alloc::{sync::Arc, vec}; - use core::{mem::size_of, ptr::NonNull}; + use core::mem::size_of; use std::{sync::Mutex, thread}; #[test] fn config() { - let mut config_space = BlkConfig { + let config_space = BlkConfig { capacity_low: ReadOnly::new(0x42), capacity_high: ReadOnly::new(0x02), size_max: ReadOnly::new(0), @@ -578,15 +579,14 @@ mod tests { min_io_size: ReadOnly::new(0), opt_io_size: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: QUEUE_SIZE.into(), device_features: BlkFeature::RO.bits(), - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let blk = VirtIOBlk::>::new(transport).unwrap(); @@ -597,7 +597,7 @@ mod tests { #[test] fn read() { - let mut config_space = BlkConfig { + let config_space = BlkConfig { capacity_low: ReadOnly::new(66), capacity_high: ReadOnly::new(0), size_max: ReadOnly::new(0), @@ -611,15 +611,14 @@ mod tests { min_io_size: ReadOnly::new(0), opt_io_size: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: QUEUE_SIZE.into(), device_features: BlkFeature::RING_INDIRECT_DESC.bits(), - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut blk = VirtIOBlk::>::new(transport).unwrap(); @@ -667,7 +666,7 @@ mod tests { #[test] fn write() { - let mut config_space = BlkConfig { + let config_space = BlkConfig { capacity_low: ReadOnly::new(66), capacity_high: ReadOnly::new(0), size_max: ReadOnly::new(0), @@ -681,15 +680,14 @@ mod tests { min_io_size: ReadOnly::new(0), opt_io_size: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: QUEUE_SIZE.into(), device_features: BlkFeature::RING_INDIRECT_DESC.bits(), - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut blk = VirtIOBlk::>::new(transport).unwrap(); @@ -742,7 +740,7 @@ mod tests { #[test] fn flush() { - let mut config_space = BlkConfig { + let config_space = BlkConfig { capacity_low: ReadOnly::new(66), capacity_high: ReadOnly::new(0), size_max: ReadOnly::new(0), @@ -756,15 +754,14 @@ mod tests { min_io_size: ReadOnly::new(0), opt_io_size: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: QUEUE_SIZE.into(), device_features: (BlkFeature::RING_INDIRECT_DESC | BlkFeature::FLUSH).bits(), - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut blk = VirtIOBlk::>::new(transport).unwrap(); @@ -809,7 +806,7 @@ mod tests { #[test] fn device_id() { - let mut config_space = BlkConfig { + let config_space = BlkConfig { capacity_low: ReadOnly::new(66), capacity_high: ReadOnly::new(0), size_max: ReadOnly::new(0), @@ -823,15 +820,14 @@ mod tests { min_io_size: ReadOnly::new(0), opt_io_size: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: QUEUE_SIZE.into(), device_features: BlkFeature::RING_INDIRECT_DESC.bits(), - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut blk = VirtIOBlk::>::new(transport).unwrap(); diff --git a/src/device/console.rs b/src/device/console.rs index 261effe2..c8c49d19 100644 --- a/src/device/console.rs +++ b/src/device/console.rs @@ -12,6 +12,7 @@ use alloc::boxed::Box; use bitflags::bitflags; use core::fmt::{self, Display, Formatter, Write}; use log::error; +use zerocopy::{FromBytes, Immutable, IntoBytes}; const QUEUE_RECEIVEQ_PORT_0: u16 = 0; const QUEUE_TRANSMITQ_PORT_0: u16 = 1; @@ -265,6 +266,7 @@ impl Drop for VirtIOConsole { } } +#[derive(FromBytes, Immutable, IntoBytes)] #[repr(C)] struct Config { cols: ReadOnly, @@ -309,26 +311,24 @@ mod tests { }, }; use alloc::{sync::Arc, vec}; - use core::ptr::NonNull; use std::{sync::Mutex, thread}; #[test] fn config_info_no_features() { - let mut config_space = Config { + let config_space = Config { cols: ReadOnly::new(80), rows: ReadOnly::new(42), max_nr_ports: ReadOnly::new(0), emerg_wr: WriteOnly::default(), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default(), QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default(), QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Console, max_queue_size: 2, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let console = VirtIOConsole::>::new(transport).unwrap(); @@ -338,21 +338,20 @@ mod tests { #[test] fn config_info() { - let mut config_space = Config { + let config_space = Config { cols: ReadOnly::new(80), rows: ReadOnly::new(42), max_nr_ports: ReadOnly::new(0), emerg_wr: WriteOnly::default(), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default(), QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default(), QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Console, max_queue_size: 2, device_features: 0x07, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let console = VirtIOConsole::>::new(transport).unwrap(); @@ -368,46 +367,44 @@ mod tests { #[test] fn emergency_write() { - let mut config_space = Config { + let config_space = Config { cols: ReadOnly::new(0), rows: ReadOnly::new(0), max_nr_ports: ReadOnly::new(0), emerg_wr: WriteOnly::default(), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default(), QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default(), QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Console, max_queue_size: 2, device_features: 0x07, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut console = VirtIOConsole::>::new(transport).unwrap(); console.emergency_write(42).unwrap(); - assert_eq!(config_space.emerg_wr.0, 42); + assert_eq!(state.lock().unwrap().config_space.emerg_wr.0, 42); } #[test] fn receive() { - let mut config_space = Config { + let config_space = Config { cols: ReadOnly::new(0), rows: ReadOnly::new(0), max_nr_ports: ReadOnly::new(0), emerg_wr: WriteOnly::default(), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default(), QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default(), QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Console, max_queue_size: 2, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut console = VirtIOConsole::>::new(transport).unwrap(); @@ -438,21 +435,20 @@ mod tests { #[test] fn send() { - let mut config_space = Config { + let config_space = Config { cols: ReadOnly::new(0), rows: ReadOnly::new(0), max_nr_ports: ReadOnly::new(0), emerg_wr: WriteOnly::default(), }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default(), QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default(), QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Console, max_queue_size: 2, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut console = VirtIOConsole::>::new(transport).unwrap(); diff --git a/src/device/input.rs b/src/device/input.rs index 7b699d2b..1947497a 100644 --- a/src/device/input.rs +++ b/src/device/input.rs @@ -247,6 +247,7 @@ pub enum InputConfigSelect { AbsInfo = 0x12, } +#[derive(FromBytes, Immutable, IntoBytes)] #[repr(C)] struct Config { select: WriteOnly, @@ -317,41 +318,52 @@ mod tests { }, }; use alloc::{sync::Arc, vec}; - use core::{convert::TryInto, ptr::NonNull}; + use core::convert::TryInto; use std::sync::Mutex; #[test] fn config() { const DEFAULT_DATA: ReadOnly = ReadOnly::new(0); - let mut config_space = Config { + let config_space = Config { select: WriteOnly::default(), subsel: WriteOnly::default(), size: ReadOnly::new(0), _reserved: Default::default(), data: [DEFAULT_DATA; 128], }; - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default(), QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new( + vec![QueueStatus::default(), QueueStatus::default()], + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: QUEUE_SIZE.try_into().unwrap(), device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut input = VirtIOInput::>::new(transport).unwrap(); - set_data(&mut config_space, "Test input device".as_bytes()); + set_data( + &mut state.lock().unwrap().config_space, + "Test input device".as_bytes(), + ); assert_eq!(input.name().unwrap(), "Test input device"); - assert_eq!(config_space.select.0, InputConfigSelect::IdName as u8); - assert_eq!(config_space.subsel.0, 0); - - set_data(&mut config_space, "Serial number".as_bytes()); + assert_eq!( + state.lock().unwrap().config_space.select.0, + InputConfigSelect::IdName as u8 + ); + assert_eq!(state.lock().unwrap().config_space.subsel.0, 0); + + set_data( + &mut state.lock().unwrap().config_space, + "Serial number".as_bytes(), + ); assert_eq!(input.serial_number().unwrap(), "Serial number"); - assert_eq!(config_space.select.0, InputConfigSelect::IdSerial as u8); - assert_eq!(config_space.subsel.0, 0); + assert_eq!( + state.lock().unwrap().config_space.select.0, + InputConfigSelect::IdSerial as u8 + ); + assert_eq!(state.lock().unwrap().config_space.subsel.0, 0); let ids = DevIDs { bustype: 0x4242, @@ -359,20 +371,29 @@ mod tests { vendor: 0x1234, version: 0x4321, }; - set_data(&mut config_space, ids.as_bytes()); + set_data(&mut state.lock().unwrap().config_space, ids.as_bytes()); assert_eq!(input.ids().unwrap(), ids); - assert_eq!(config_space.select.0, InputConfigSelect::IdDevids as u8); - assert_eq!(config_space.subsel.0, 0); + assert_eq!( + state.lock().unwrap().config_space.select.0, + InputConfigSelect::IdDevids as u8 + ); + assert_eq!(state.lock().unwrap().config_space.subsel.0, 0); - set_data(&mut config_space, &[0x12, 0x34, 0x56]); + set_data(&mut state.lock().unwrap().config_space, &[0x12, 0x34, 0x56]); assert_eq!(input.prop_bits().unwrap().as_ref(), &[0x12, 0x34, 0x56]); - assert_eq!(config_space.select.0, InputConfigSelect::PropBits as u8); - assert_eq!(config_space.subsel.0, 0); + assert_eq!( + state.lock().unwrap().config_space.select.0, + InputConfigSelect::PropBits as u8 + ); + assert_eq!(state.lock().unwrap().config_space.subsel.0, 0); - set_data(&mut config_space, &[0x42, 0x66]); + set_data(&mut state.lock().unwrap().config_space, &[0x42, 0x66]); assert_eq!(input.ev_bits(3).unwrap().as_ref(), &[0x42, 0x66]); - assert_eq!(config_space.select.0, InputConfigSelect::EvBits as u8); - assert_eq!(config_space.subsel.0, 3); + assert_eq!( + state.lock().unwrap().config_space.select.0, + InputConfigSelect::EvBits as u8 + ); + assert_eq!(state.lock().unwrap().config_space.subsel.0, 3); let abs_info = AbsInfo { min: 12, @@ -381,10 +402,13 @@ mod tests { flat: 10, res: 2, }; - set_data(&mut config_space, abs_info.as_bytes()); + set_data(&mut state.lock().unwrap().config_space, abs_info.as_bytes()); assert_eq!(input.abs_info(5).unwrap(), abs_info); - assert_eq!(config_space.select.0, InputConfigSelect::AbsInfo as u8); - assert_eq!(config_space.subsel.0, 5); + assert_eq!( + state.lock().unwrap().config_space.select.0, + InputConfigSelect::AbsInfo as u8 + ); + assert_eq!(state.lock().unwrap().config_space.subsel.0, 5); } fn set_data(config_space: &mut Config, value: &[u8]) { diff --git a/src/device/socket/connectionmanager.rs b/src/device/socket/connectionmanager.rs index d38dc95e..8e823eb7 100644 --- a/src/device/socket/connectionmanager.rs +++ b/src/device/socket/connectionmanager.rs @@ -429,7 +429,7 @@ mod tests { }, }; use alloc::{sync::Arc, vec}; - use core::{mem::size_of, ptr::NonNull}; + use core::mem::size_of; use std::{sync::Mutex, thread}; use zerocopy::{FromBytes, IntoBytes}; @@ -446,23 +446,22 @@ mod tests { let hello_from_guest = "Hello from guest"; let hello_from_host = "Hello from host"; - let mut config_space = VirtioVsockConfig { + let config_space = VirtioVsockConfig { guest_cid_low: ReadOnly::new(66), guest_cid_high: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![ + let state = Arc::new(Mutex::new(State::new( + vec![ QueueStatus::default(), QueueStatus::default(), QueueStatus::default(), ], - ..Default::default() - })); + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Socket, max_queue_size: 32, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut socket = VsockConnectionManager::new( @@ -661,23 +660,22 @@ mod tests { port: host_port, }; - let mut config_space = VirtioVsockConfig { + let config_space = VirtioVsockConfig { guest_cid_low: ReadOnly::new(66), guest_cid_high: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![ + let state = Arc::new(Mutex::new(State::new( + vec![ QueueStatus::default(), QueueStatus::default(), QueueStatus::default(), ], - ..Default::default() - })); + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Socket, max_queue_size: 32, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut socket = VsockConnectionManager::new( diff --git a/src/device/socket/protocol.rs b/src/device/socket/protocol.rs index 8df924fc..baffadd7 100644 --- a/src/device/socket/protocol.rs +++ b/src/device/socket/protocol.rs @@ -32,6 +32,7 @@ impl From for U16 { } /// VirtioVsockConfig is the vsock device configuration space. +#[derive(FromBytes, Immutable, IntoBytes)] #[repr(C)] pub struct VirtioVsockConfig { /// The guest_cid field contains the guest’s context ID, which uniquely identifies diff --git a/src/device/socket/vsock.rs b/src/device/socket/vsock.rs index 63f7956f..8a95ee49 100644 --- a/src/device/socket/vsock.rs +++ b/src/device/socket/vsock.rs @@ -470,28 +470,26 @@ mod tests { }, }; use alloc::{sync::Arc, vec}; - use core::ptr::NonNull; use std::sync::Mutex; #[test] fn config() { - let mut config_space = VirtioVsockConfig { + let config_space = VirtioVsockConfig { guest_cid_low: ReadOnly::new(66), guest_cid_high: ReadOnly::new(0), }; - let state = Arc::new(Mutex::new(State { - queues: vec![ + let state = Arc::new(Mutex::new(State::new( + vec![ QueueStatus::default(), QueueStatus::default(), QueueStatus::default(), ], - ..Default::default() - })); + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Socket, max_queue_size: 32, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let socket = diff --git a/src/device/sound.rs b/src/device/sound.rs index d1b44f3a..f5f71ee6 100644 --- a/src/device/sound.rs +++ b/src/device/sound.rs @@ -1012,6 +1012,7 @@ impl From for u8 { } } +#[derive(FromBytes, Immutable, IntoBytes)] #[repr(C)] struct VirtIOSoundConfig { jacks: ReadOnly, @@ -1566,31 +1567,29 @@ mod tests { }, }; use alloc::{sync::Arc, vec}; - use core::ptr::NonNull; use fake::FakeSoundDevice; use std::sync::Mutex; #[test] fn config() { - let mut config_space = VirtIOSoundConfig { + let config_space = VirtIOSoundConfig { jacks: ReadOnly::new(3), streams: ReadOnly::new(4), chmaps: ReadOnly::new(2), }; - let state = Arc::new(Mutex::new(State { - queues: vec![ + let state = Arc::new(Mutex::new(State::new( + vec![ QueueStatus::default(), QueueStatus::default(), QueueStatus::default(), QueueStatus::default(), ], - ..Default::default() - })); + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Sound, max_queue_size: 32, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let sound = diff --git a/src/device/sound/fake.rs b/src/device/sound/fake.rs index ab516514..d698824f 100644 --- a/src/device/sound/fake.rs +++ b/src/device/sound/fake.rs @@ -17,7 +17,6 @@ use alloc::{sync::Arc, vec}; use core::{ convert::{TryFrom, TryInto}, mem::size_of, - ptr::NonNull, time::Duration, }; use std::{ @@ -32,7 +31,7 @@ use zerocopy::{FromBytes, IntoBytes}; #[derive(Clone, Debug)] pub struct FakeSoundDevice { - pub state: Arc>, + pub state: Arc>>, pub terminate: Arc, /// The paramaters set for each stream, if any. pub params: Arc>>>, @@ -49,25 +48,24 @@ impl FakeSoundDevice { pcm_infos: Vec, chmap_infos: Vec, ) -> (Self, FakeTransport) { - let mut config_space = VirtIOSoundConfig { + let config_space = VirtIOSoundConfig { jacks: ReadOnly::new(jack_infos.len().try_into().unwrap()), streams: ReadOnly::new(pcm_infos.len().try_into().unwrap()), chmaps: ReadOnly::new(chmap_infos.len().try_into().unwrap()), }; - let state = Arc::new(Mutex::new(State { - queues: vec![ + let state = Arc::new(Mutex::new(State::new( + vec![ QueueStatus::default(), QueueStatus::default(), QueueStatus::default(), QueueStatus::default(), ], - ..Default::default() - })); + config_space, + ))); let transport = FakeTransport { device_type: DeviceType::Socket, max_queue_size: 32, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let params = repeat_with(|| None).take(pcm_infos.len()).collect(); diff --git a/src/queue.rs b/src/queue.rs index b21d8429..a4e01548 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -1164,16 +1164,11 @@ mod tests { /// Tests that the queue advises the device that notifications are needed. #[test] fn set_dev_notify() { - let mut config_space = (); - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new(vec![QueueStatus::default()], ()))); let mut transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: 4, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut queue = VirtQueue::::new(&mut transport, 0, false, false).unwrap(); @@ -1205,16 +1200,11 @@ mod tests { /// notifications. #[test] fn add_notify() { - let mut config_space = (); - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new(vec![QueueStatus::default()], ()))); let mut transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: 4, device_features: 0, - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut queue = VirtQueue::::new(&mut transport, 0, false, false).unwrap(); @@ -1240,16 +1230,11 @@ mod tests { /// notifications with the `avail_event` index. #[test] fn add_notify_event_idx() { - let mut config_space = (); - let state = Arc::new(Mutex::new(State { - queues: vec![QueueStatus::default()], - ..Default::default() - })); + let state = Arc::new(Mutex::new(State::new(vec![QueueStatus::default()], ()))); let mut transport = FakeTransport { device_type: DeviceType::Block, max_queue_size: 4, device_features: Feature::RING_EVENT_IDX.bits(), - config_space: NonNull::from(&mut config_space), state: state.clone(), }; let mut queue = VirtQueue::::new(&mut transport, 0, false, true).unwrap(); diff --git a/src/transport/fake.rs b/src/transport/fake.rs index 0fc00dc7..090a7f8a 100644 --- a/src/transport/fake.rs +++ b/src/transport/fake.rs @@ -5,23 +5,23 @@ use crate::{ }; use alloc::{sync::Arc, vec::Vec}; use core::{ - ptr::NonNull, + fmt::{self, Debug, Formatter}, sync::atomic::{AtomicBool, Ordering}, time::Duration, }; use std::{sync::Mutex, thread}; +use zerocopy::{FromBytes, Immutable, IntoBytes}; /// A fake implementation of [`Transport`] for unit tests. #[derive(Debug)] -pub struct FakeTransport { +pub struct FakeTransport { pub device_type: DeviceType, pub max_queue_size: u32, pub device_features: u64, - pub config_space: NonNull, - pub state: Arc>, + pub state: Arc>>, } -impl Transport for FakeTransport { +impl Transport for FakeTransport { fn device_type(&self) -> DeviceType { self.device_type } @@ -100,7 +100,7 @@ impl Transport for FakeTransport { self.state.lock().unwrap().config_generation } - fn read_config_space(&self, offset: usize) -> Result { + 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::()); @@ -109,11 +109,17 @@ impl Transport for FakeTransport { if size_of::() < offset + size_of::() { Err(Error::ConfigSpaceTooSmall) } else { - unsafe { Ok(self.config_space.cast::().byte_add(offset).read()) } + let state = self.state.lock().unwrap(); + let bytes = &state.config_space.as_bytes()[offset..offset + size_of::()]; + Ok(T::read_from_bytes(bytes).unwrap()) } } - fn write_config_space(&mut self, offset: usize, value: T) -> Result<(), Error> { + 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::()); @@ -122,25 +128,51 @@ impl Transport for FakeTransport { if size_of::() < offset + size_of::() { Err(Error::ConfigSpaceTooSmall) } else { - unsafe { - self.config_space.cast::().byte_add(offset).write(value); - } + let mut state = self.state.lock().unwrap(); + let bytes = &mut state.config_space.as_mut_bytes()[offset..offset + size_of::()]; + value.write_to(bytes).unwrap(); Ok(()) } } } -#[derive(Debug, Default)] -pub struct State { +pub struct State { pub status: DeviceStatus, pub driver_features: u64, pub guest_page_size: u32, pub interrupt_pending: bool, pub queues: Vec, pub config_generation: u32, + pub config_space: C, +} + +impl Debug for State { + fn fmt(&self, f: &mut Formatter) -> fmt::Result { + f.debug_struct("State") + .field("status", &self.status) + .field("driver_features", &self.driver_features) + .field("guest_page_size", &self.guest_page_size) + .field("interrupt_pending", &self.interrupt_pending) + .field("queues", &self.queues) + .field("config_generation", &self.config_generation) + .field("config_space", &"...") + .finish() + } } -impl State { +impl State { + pub const fn new(queues: Vec, config_space: C) -> Self { + Self { + status: DeviceStatus::empty(), + driver_features: 0, + guest_page_size: 0, + interrupt_pending: false, + queues, + config_generation: 0, + config_space, + } + } + /// Simulates the device writing to the given queue. /// /// The fake device always uses descriptors in order. From f4605c90428ea0e1a606fe1249a9403b05e9b4c6 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 4 Dec 2024 09:15:43 +0000 Subject: [PATCH 7/7] Add Rustdoc comments for fake transport. --- src/transport/fake.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/transport/fake.rs b/src/transport/fake.rs index 090a7f8a..dd7ecd37 100644 --- a/src/transport/fake.rs +++ b/src/transport/fake.rs @@ -15,9 +15,13 @@ use zerocopy::{FromBytes, Immutable, IntoBytes}; /// A fake implementation of [`Transport`] for unit tests. #[derive(Debug)] pub struct FakeTransport { + /// The type of device which the transport should claim to be for. pub device_type: DeviceType, + /// The maximum queue size supported by the transport. pub max_queue_size: u32, + /// The device features which should be reported by the transport. pub device_features: u64, + /// The mutable state of the transport. pub state: Arc>>, } @@ -136,13 +140,21 @@ impl Transport for FakeTransport { } } +/// The mutable state of a fake transport. pub struct State { + /// The status of the fake device. pub status: DeviceStatus, + /// The features which the driver says it supports. pub driver_features: u64, + /// The guest page size set by the driver. pub guest_page_size: u32, + /// Whether the transport has an interrupt pending. pub interrupt_pending: bool, + /// The state of the transport's queues. pub queues: Vec, + /// The config generation which the transport should report. pub config_generation: u32, + /// The state of the transport's VirtIO configuration space. pub config_space: C, }