From 32fc595affcd56a29a9cc3606d0aaf719f2a9966 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Tue, 6 Sep 2022 17:15:23 +0100 Subject: [PATCH 1/8] First cut at sound volatile MMIO. --- src/blk.rs | 2 +- src/console.rs | 2 +- src/gpu.rs | 2 +- src/input.rs | 2 +- src/lib.rs | 1 + src/net.rs | 2 +- src/queue.rs | 2 +- src/transport/mmio.rs | 139 +++++++++++++++++++++++------------------- src/volatile.rs | 76 +++++++++++++++++++++++ 9 files changed, 160 insertions(+), 68 deletions(-) create mode 100644 src/volatile.rs diff --git a/src/blk.rs b/src/blk.rs index de666d2f..5dce9cb2 100644 --- a/src/blk.rs +++ b/src/blk.rs @@ -1,10 +1,10 @@ use super::*; use crate::queue::VirtQueue; use crate::transport::Transport; +use ::volatile::Volatile; use bitflags::*; use core::hint::spin_loop; use log::*; -use volatile::Volatile; /// The virtio block device is a simple virtual block device (ie. disk). /// diff --git a/src/console.rs b/src/console.rs index 55741af9..8ec95e8c 100644 --- a/src/console.rs +++ b/src/console.rs @@ -1,10 +1,10 @@ use super::*; use crate::queue::VirtQueue; use crate::transport::Transport; +use ::volatile::{ReadOnly, WriteOnly}; use bitflags::*; use core::{fmt, hint::spin_loop}; use log::*; -use volatile::{ReadOnly, WriteOnly}; const QUEUE_RECEIVEQ_PORT_0: usize = 0; const QUEUE_TRANSMITQ_PORT_0: usize = 1; diff --git a/src/gpu.rs b/src/gpu.rs index 9714eae7..d621f6bb 100644 --- a/src/gpu.rs +++ b/src/gpu.rs @@ -1,10 +1,10 @@ use super::*; use crate::queue::VirtQueue; use crate::transport::Transport; +use ::volatile::{ReadOnly, Volatile, WriteOnly}; use bitflags::*; use core::{fmt, hint::spin_loop}; use log::*; -use volatile::{ReadOnly, Volatile, WriteOnly}; /// A virtio based graphics adapter. /// diff --git a/src/input.rs b/src/input.rs index 2254a010..1f4072df 100644 --- a/src/input.rs +++ b/src/input.rs @@ -1,9 +1,9 @@ use super::*; use crate::transport::Transport; +use ::volatile::{ReadOnly, WriteOnly}; use alloc::boxed::Box; use bitflags::*; use log::*; -use volatile::{ReadOnly, WriteOnly}; /// Virtual human interface devices such as keyboards, mice and tablets. /// diff --git a/src/lib.rs b/src/lib.rs index 49dd295b..291af7be 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -15,6 +15,7 @@ mod input; mod net; mod queue; mod transport; +mod volatile; pub use self::blk::{BlkResp, RespStatus, VirtIOBlk}; pub use self::console::VirtIOConsole; diff --git a/src/net.rs b/src/net.rs index c36d8719..111526a6 100644 --- a/src/net.rs +++ b/src/net.rs @@ -2,10 +2,10 @@ use core::mem::{size_of, MaybeUninit}; use super::*; use crate::transport::Transport; +use ::volatile::{ReadOnly, Volatile}; use bitflags::*; use core::hint::spin_loop; use log::*; -use volatile::{ReadOnly, Volatile}; /// The virtio network device is a virtual ethernet card. /// diff --git a/src/queue.rs b/src/queue.rs index 9a31104d..15a733aa 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -8,7 +8,7 @@ use super::*; use crate::transport::Transport; use bitflags::*; -use volatile::Volatile; +use ::volatile::Volatile; /// The mechanism for bulk data transport on virtio devices. /// diff --git a/src/transport/mmio.rs b/src/transport/mmio.rs index ac9a262b..9b6a49c8 100644 --- a/src/transport/mmio.rs +++ b/src/transport/mmio.rs @@ -1,12 +1,16 @@ use super::{DeviceStatus, DeviceType, Transport}; -use crate::{align_up, queue::Descriptor, PhysAddr, PAGE_SIZE}; +use crate::{ + align_up, + queue::Descriptor, + volatile::{volread, volwrite, ReadOnly, Volatile, WriteOnly}, + PhysAddr, PAGE_SIZE, +}; use core::{ convert::{TryFrom, TryInto}, fmt::{self, Display, Formatter}, mem::size_of, ptr::NonNull, }; -use volatile::{ReadOnly, Volatile, WriteOnly}; const MAGIC_VALUE: u32 = 0x7472_6976; pub(crate) const LEGACY_VERSION: u32 = 1; @@ -281,14 +285,14 @@ impl MmioTransport { /// `header` must point to a properly aligned valid VirtIO MMIO region, which must remain valid /// for the lifetime of the transport that is returned. pub unsafe fn new(header: NonNull) -> Result { - let magic = header.as_ref().magic.read(); + let magic = volread!(header, magic); if magic != MAGIC_VALUE { return Err(MmioError::BadMagic(magic)); } - if header.as_ref().device_id.read() == 0 { + if volread!(header, device_id) == 0 { return Err(MmioError::ZeroDeviceId); } - let version = header.as_ref().version.read().try_into()?; + let version = volread!(header, version).try_into()?; Ok(Self { header, version }) } @@ -299,61 +303,67 @@ impl MmioTransport { /// Gets the vendor ID. pub fn vendor_id(&self) -> u32 { - self.header().vendor_id.read() - } - - fn header(&self) -> &VirtIOHeader { - unsafe { self.header.as_ref() } - } - - fn header_mut(&mut self) -> &mut VirtIOHeader { - unsafe { self.header.as_mut() } + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { volread!(self.header, vendor_id) } } } impl Transport for MmioTransport { fn device_type(&self) -> DeviceType { - match self.header().device_id.read() { + // Safe because self.header points to a valid VirtIO MMIO region. + match unsafe { volread!(self.header, device_id) } { x @ 1..=13 | x @ 16..=24 => unsafe { core::mem::transmute(x as u8) }, _ => DeviceType::Invalid, } } fn read_device_features(&mut self) -> u64 { - let header = self.header_mut(); - header.device_features_sel.write(0); // device features [0, 32) - let mut device_features_bits = header.device_features.read().into(); - header.device_features_sel.write(1); // device features [32, 64) - device_features_bits += (header.device_features.read() as u64) << 32; - device_features_bits + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, device_features_sel, 0); // device features [0, 32) + let mut device_features_bits = volread!(self.header, device_features).into(); + volwrite!(self.header, device_features_sel, 1); // device features [32, 64) + device_features_bits += (volread!(self.header, device_features) as u64) << 32; + device_features_bits + } } fn write_driver_features(&mut self, driver_features: u64) { - let header = self.header_mut(); - header.driver_features_sel.write(0); // driver features [0, 32) - header.driver_features.write(driver_features as u32); - header.driver_features_sel.write(1); // driver features [32, 64) - header.driver_features.write((driver_features >> 32) as u32); + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, driver_features_sel, 0); // driver features [0, 32) + volwrite!(self.header, driver_features, driver_features as u32); + volwrite!(self.header, driver_features_sel, 1); // driver features [32, 64) + volwrite!(self.header, driver_features, (driver_features >> 32) as u32); + } } fn max_queue_size(&self) -> u32 { - self.header().queue_num_max.read() + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { volread!(self.header, queue_num_max) } } fn notify(&mut self, queue: u32) { - self.header_mut().queue_notify.write(queue); + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, queue_notify, queue); + } } fn set_status(&mut self, status: DeviceStatus) { - self.header_mut().status.write(status); + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, status, status); + } } fn set_guest_page_size(&mut self, guest_page_size: u32) { match self.version { MmioVersion::Legacy => { - self.header_mut() - .legacy_guest_page_size - .write(guest_page_size); + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, legacy_guest_page_size, guest_page_size); + } } MmioVersion::Modern => { // No-op, modern devices don't care. @@ -385,47 +395,52 @@ impl Transport for MmioTransport { let align = PAGE_SIZE as u32; let pfn = (descriptors / PAGE_SIZE) as u32; assert_eq!(pfn as usize * PAGE_SIZE, descriptors); - self.header_mut().queue_sel.write(queue); - self.header_mut().queue_num.write(size); - self.header_mut().legacy_queue_align.write(align); - self.header_mut().legacy_queue_pfn.write(pfn); + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, queue_sel, queue); + volwrite!(self.header, queue_num, size); + volwrite!(self.header, legacy_queue_align, align); + volwrite!(self.header, legacy_queue_pfn, pfn); + } } MmioVersion::Modern => { - self.header_mut().queue_sel.write(queue); - self.header_mut().queue_num.write(size); - self.header_mut().queue_desc_low.write(descriptors as u32); - self.header_mut() - .queue_desc_high - .write((descriptors >> 32) as u32); - self.header_mut().queue_driver_low.write(driver_area as u32); - self.header_mut() - .queue_driver_high - .write((driver_area >> 32) as u32); - self.header_mut().queue_device_low.write(device_area as u32); - self.header_mut() - .queue_device_high - .write((device_area >> 32) as u32); - self.header_mut().queue_ready.write(1); + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, queue_sel, queue); + volwrite!(self.header, queue_num, size); + volwrite!(self.header, queue_desc_low, descriptors as u32); + volwrite!(self.header, queue_desc_high, (descriptors >> 32) as u32); + volwrite!(self.header, queue_driver_low, driver_area as u32); + volwrite!(self.header, queue_driver_high, (driver_area >> 32) as u32); + volwrite!(self.header, queue_device_low, device_area as u32); + volwrite!(self.header, queue_device_high, (device_area >> 32) as u32); + volwrite!(self.header, queue_ready, 1); + } } } } fn queue_used(&mut self, queue: u32) -> bool { - self.header_mut().queue_sel.write(queue); - match self.version { - MmioVersion::Legacy => self.header().legacy_queue_pfn.read() != 0, - MmioVersion::Modern => self.header().queue_ready.read() != 0, + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + volwrite!(self.header, queue_sel, queue); + match self.version { + MmioVersion::Legacy => volread!(self.header, legacy_queue_pfn) != 0, + MmioVersion::Modern => volread!(self.header, queue_ready) != 0, + } } } fn ack_interrupt(&mut self) -> bool { - let header = self.header_mut(); - let interrupt = header.interrupt_status.read(); - if interrupt != 0 { - header.interrupt_ack.write(interrupt); - true - } else { - false + // Safe because self.header points to a valid VirtIO MMIO region. + unsafe { + let interrupt = volread!(self.header, interrupt_status); + if interrupt != 0 { + volwrite!(self.header, interrupt_ack, interrupt); + true + } else { + false + } } } diff --git a/src/volatile.rs b/src/volatile.rs new file mode 100644 index 00000000..11168056 --- /dev/null +++ b/src/volatile.rs @@ -0,0 +1,76 @@ +/// An MMIO register which can only be read from. +#[derive(Debug, Default)] +#[repr(transparent)] +pub struct ReadOnly(T); + +impl ReadOnly { + pub const fn new(value: T) -> Self { + Self(value) + } +} + +/// An MMIO register which can only be written to. +#[derive(Debug, Default)] +#[repr(transparent)] +pub struct WriteOnly(T); + +/// An MMIO register which may be both read and written. +#[derive(Debug, Default)] +#[repr(transparent)] +pub struct Volatile(T); + +impl Volatile { + pub const fn new(value: T) -> Self { + Self(value) + } +} + +pub trait VolatileReadable { + unsafe fn vread(self) -> T; +} + +impl VolatileReadable for *const ReadOnly { + unsafe fn vread(self) -> T { + self.read_volatile().0 + } +} + +impl VolatileReadable for *const Volatile { + unsafe fn vread(self) -> T { + self.read_volatile().0 + } +} + +pub trait VolatileWritable { + unsafe fn vwrite(self, value: T); +} + +impl VolatileWritable for *mut WriteOnly { + unsafe fn vwrite(self, value: T) { + (self as *mut T).write_volatile(value) + } +} + +impl VolatileWritable for *mut Volatile { + unsafe fn vwrite(self, value: T) { + (self as *mut T).write_volatile(value) + } +} + +macro_rules! volread { + ($nonnull:expr, $field:ident) => { + $crate::volatile::VolatileReadable::vread(core::ptr::addr_of!((*$nonnull.as_ptr()).$field)) + }; +} + +macro_rules! volwrite { + ($nonnull:expr, $field:ident, $value:expr) => { + $crate::volatile::VolatileWritable::vwrite( + core::ptr::addr_of_mut!((*$nonnull.as_ptr()).$field), + $value, + ) + }; +} + +pub(crate) use volread; +pub(crate) use volwrite; From 787a203d0e28d5d580db6f43984abc23d9b03947 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 7 Sep 2022 13:22:42 +0000 Subject: [PATCH 2/8] Use new volatile module for blk too. --- src/blk.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/blk.rs b/src/blk.rs index 5dce9cb2..754dd5fc 100644 --- a/src/blk.rs +++ b/src/blk.rs @@ -1,7 +1,7 @@ use super::*; use crate::queue::VirtQueue; use crate::transport::Transport; -use ::volatile::Volatile; +use crate::volatile::{volread, Volatile}; use bitflags::*; use core::hint::spin_loop; use log::*; @@ -28,13 +28,11 @@ impl VirtIOBlk<'_, H, T> { }); // read configuration space - let config_space = transport.config_space().cast::(); - let config = unsafe { config_space.as_ref() }; + let config = transport.config_space().cast::(); info!("config: {:?}", config); - info!( - "found a block device of size {}KB", - config.capacity.read() / 2 - ); + // Safe because config is a valid pointer to the device configuration space. + let capacity = unsafe { volread!(config, capacity) }; + info!("found a block device of size {}KB", capacity / 2); let queue = VirtQueue::new(&mut transport, 0, 16)?; transport.finish_init(); @@ -42,7 +40,7 @@ impl VirtIOBlk<'_, H, T> { Ok(VirtIOBlk { transport, queue, - capacity: config.capacity.read() as usize, + capacity: capacity as usize, }) } From 430726aef5c4d60684873398d9dbc95f96a02239 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 7 Sep 2022 13:22:57 +0000 Subject: [PATCH 3/8] Backwards compatibility for old rustc in example. --- src/volatile.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/volatile.rs b/src/volatile.rs index 11168056..39d57be7 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -4,7 +4,7 @@ pub struct ReadOnly(T); impl ReadOnly { - pub const fn new(value: T) -> Self { + pub fn new(value: T) -> Self { Self(value) } } @@ -20,7 +20,7 @@ pub struct WriteOnly(T); pub struct Volatile(T); impl Volatile { - pub const fn new(value: T) -> Self { + pub fn new(value: T) -> Self { Self(value) } } From 78834c5962008179292e43d14053e0814d2e1216 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 7 Sep 2022 15:31:26 +0000 Subject: [PATCH 4/8] Document volatile module. --- src/volatile.rs | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/volatile.rs b/src/volatile.rs index 39d57be7..01d97e2c 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -4,6 +4,7 @@ pub struct ReadOnly(T); impl ReadOnly { + /// Construct a new instance for testing. pub fn new(value: T) -> Self { Self(value) } @@ -20,12 +21,15 @@ pub struct WriteOnly(T); pub struct Volatile(T); impl Volatile { + /// Construct a new instance for testing. pub fn new(value: T) -> Self { Self(value) } } +/// A trait implemented by MMIO registers which may be read from. pub trait VolatileReadable { + /// Performs a volatile read from the MMIO register. unsafe fn vread(self) -> T; } @@ -41,7 +45,9 @@ impl VolatileReadable for *const Volatile { } } +/// A trait implemented by MMIO registers which may be written to. pub trait VolatileWritable { + /// Performs a volatile write to the MMIO register. unsafe fn vwrite(self, value: T); } @@ -57,12 +63,38 @@ impl VolatileWritable for *mut Volatile { } } +/// Performs a volatile read from the given field of pointer to a struct representing an MMIO region. +/// +/// # Usage +/// ```compile_fail +/// # use core::ptr::NonNull; +/// # use virtio_drivers::volatile::{ReadOnly, volread}; +/// struct MmioDevice { +/// field: ReadOnly, +/// } +/// +/// let device: NonNull = NonNull::new(0x1234 as *mut MmioDevice).unwrap(); +/// let value = unsafe { volread!(device, field) }; +/// ``` macro_rules! volread { ($nonnull:expr, $field:ident) => { $crate::volatile::VolatileReadable::vread(core::ptr::addr_of!((*$nonnull.as_ptr()).$field)) }; } +/// Performs a volatile write to the given field of pointer to a struct representing an MMIO region. +/// +/// # Usage +/// ```compile_fail +/// # use core::ptr::NonNull; +/// # use virtio_drivers::volatile::{WriteOnly, volread}; +/// struct MmioDevice { +/// field: WriteOnly, +/// } +/// +/// let device: NonNull = NonNull::new(0x1234 as *mut MmioDevice).unwrap(); +/// unsafe { volwrite!(device, field, 42); } +/// ``` macro_rules! volwrite { ($nonnull:expr, $field:ident, $value:expr) => { $crate::volatile::VolatileWritable::vwrite( From b11a599dc3c830a345e02cd27e6aff54a6759ed6 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 7 Sep 2022 15:46:05 +0000 Subject: [PATCH 5/8] Use volatile module for remaining devices. --- src/console.rs | 4 ++-- src/gpu.rs | 2 +- src/input.rs | 18 +++++++++++------- src/net.rs | 13 ++++++++----- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/console.rs b/src/console.rs index 8ec95e8c..50b62d2a 100644 --- a/src/console.rs +++ b/src/console.rs @@ -1,7 +1,7 @@ use super::*; use crate::queue::VirtQueue; use crate::transport::Transport; -use ::volatile::{ReadOnly, WriteOnly}; +use crate::volatile::{ReadOnly, WriteOnly}; use bitflags::*; use core::{fmt, hint::spin_loop}; use log::*; @@ -161,7 +161,7 @@ mod tests { cols: ReadOnly::new(0), rows: ReadOnly::new(0), max_nr_ports: ReadOnly::new(0), - emerg_wr: WriteOnly::new(0), + emerg_wr: WriteOnly::default(), }; let state = Arc::new(Mutex::new(State { status: DeviceStatus::empty(), diff --git a/src/gpu.rs b/src/gpu.rs index d621f6bb..68153b89 100644 --- a/src/gpu.rs +++ b/src/gpu.rs @@ -1,7 +1,7 @@ use super::*; use crate::queue::VirtQueue; use crate::transport::Transport; -use ::volatile::{ReadOnly, Volatile, WriteOnly}; +use crate::volatile::{ReadOnly, Volatile, WriteOnly}; use bitflags::*; use core::{fmt, hint::spin_loop}; use log::*; diff --git a/src/input.rs b/src/input.rs index 1f4072df..db2966c1 100644 --- a/src/input.rs +++ b/src/input.rs @@ -1,6 +1,6 @@ use super::*; use crate::transport::Transport; -use ::volatile::{ReadOnly, WriteOnly}; +use crate::volatile::{volread, volwrite, ReadOnly, WriteOnly}; use alloc::boxed::Box; use bitflags::*; use log::*; @@ -75,12 +75,16 @@ impl VirtIOInput<'_, H, T> { subsel: u8, out: &mut [u8], ) -> u8 { - let mut config_space = self.transport.config_space().cast::(); - let config = unsafe { config_space.as_mut() }; - config.select.write(select as u8); - config.subsel.write(subsel); - let size = config.size.read(); - let data = config.data.read(); + let config = self.transport.config_space().cast::(); + let size; + let data; + // Safe because config points to a valid MMIO region for the config space. + unsafe { + volwrite!(config, select, select as u8); + volwrite!(config, subsel, subsel); + size = volread!(config, size); + data = volread!(config, data); + } out[..size as usize].copy_from_slice(&data[..size as usize]); size } diff --git a/src/net.rs b/src/net.rs index 111526a6..3eb6b0df 100644 --- a/src/net.rs +++ b/src/net.rs @@ -2,7 +2,7 @@ use core::mem::{size_of, MaybeUninit}; use super::*; use crate::transport::Transport; -use ::volatile::{ReadOnly, Volatile}; +use crate::volatile::{volread, ReadOnly, Volatile}; use bitflags::*; use core::hint::spin_loop; use log::*; @@ -31,10 +31,13 @@ impl VirtIONet<'_, H, T> { (features & supported_features).bits() }); // read configuration space - let config_space = transport.config_space().cast::(); - let config = unsafe { config_space.as_ref() }; - let mac = config.mac.read(); - debug!("Got MAC={:?}, status={:?}", mac, config.status.read()); + let config = transport.config_space().cast::(); + let mac; + // Safe because config points to a valid MMIO region for the config space. + unsafe { + mac = volread!(config, mac); + debug!("Got MAC={:?}, status={:?}", mac, volread!(config, status)); + } let queue_num = 2; // for simplicity let recv_queue = VirtQueue::new(&mut transport, QUEUE_RECEIVE, queue_num)?; From e7331a04274809aea280f002e3a34ff316c116ea Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 7 Sep 2022 15:50:51 +0000 Subject: [PATCH 6/8] UsedRing is only read by the driver. --- src/queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/queue.rs b/src/queue.rs index 15a733aa..521b9062 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -22,7 +22,7 @@ pub struct VirtQueue<'a, H: Hal> { /// Available ring avail: &'a mut AvailRing, /// Used ring - used: &'a mut UsedRing, + used: &'a UsedRing, /// The index of queue queue_idx: u32, From 455bbbef72305f32064cfb376bfb30fcd5d7fd36 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 7 Sep 2022 16:14:51 +0000 Subject: [PATCH 7/8] No need to volatile for queue. The queue is just shared memory, not MMIO memory, so there is no need to use volatile reads and writes. --- Cargo.toml | 1 - src/blk.rs | 6 +- src/console.rs | 4 +- src/gpu.rs | 4 +- src/input.rs | 8 +- src/net.rs | 8 +- src/queue.rs | 268 +++++++++++++++++++++++++++++-------------------- 7 files changed, 172 insertions(+), 127 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 24a185ae..03e6c5e8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,5 @@ description = "VirtIO guest drivers." # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -volatile = "0.3" log = "0.4" bitflags = "1.3" diff --git a/src/blk.rs b/src/blk.rs index 754dd5fc..8c0512a4 100644 --- a/src/blk.rs +++ b/src/blk.rs @@ -10,13 +10,13 @@ use log::*; /// /// Read and write requests (and other exotic requests) are placed in the queue, /// and serviced (probably out of order) by the device except where noted. -pub struct VirtIOBlk<'a, H: Hal, T: Transport> { +pub struct VirtIOBlk { transport: T, - queue: VirtQueue<'a, H>, + queue: VirtQueue, capacity: usize, } -impl VirtIOBlk<'_, H, T> { +impl VirtIOBlk { /// Create a new VirtIO-Blk driver. pub fn new(mut transport: T) -> Result { transport.begin_init(|features| { diff --git a/src/console.rs b/src/console.rs index 50b62d2a..3ba4280b 100644 --- a/src/console.rs +++ b/src/console.rs @@ -14,8 +14,8 @@ const QUEUE_SIZE: u16 = 2; /// Emergency and cols/rows unimplemented. pub struct VirtIOConsole<'a, H: Hal, T: Transport> { transport: T, - receiveq: VirtQueue<'a, H>, - transmitq: VirtQueue<'a, H>, + receiveq: VirtQueue, + transmitq: VirtQueue, queue_buf_dma: DMA, queue_buf_rx: &'a mut [u8], cursor: usize, diff --git a/src/gpu.rs b/src/gpu.rs index 68153b89..26fb4d6a 100644 --- a/src/gpu.rs +++ b/src/gpu.rs @@ -21,9 +21,9 @@ pub struct VirtIOGpu<'a, H: Hal, T: Transport> { /// DMA area of cursor image buffer. cursor_buffer_dma: Option>, /// Queue for sending control commands. - control_queue: VirtQueue<'a, H>, + control_queue: VirtQueue, /// Queue for sending cursor commands. - cursor_queue: VirtQueue<'a, H>, + cursor_queue: VirtQueue, /// Queue buffer DMA queue_buf_dma: DMA, /// Send buffer for queue. diff --git a/src/input.rs b/src/input.rs index db2966c1..aa3de36e 100644 --- a/src/input.rs +++ b/src/input.rs @@ -10,14 +10,14 @@ use log::*; /// An instance of the virtio device represents one such input device. /// Device behavior mirrors that of the evdev layer in Linux, /// making pass-through implementations on top of evdev easy. -pub struct VirtIOInput<'a, H: Hal, T: Transport> { +pub struct VirtIOInput { transport: T, - event_queue: VirtQueue<'a, H>, - status_queue: VirtQueue<'a, H>, + event_queue: VirtQueue, + status_queue: VirtQueue, event_buf: Box<[InputEvent; 32]>, } -impl VirtIOInput<'_, H, T> { +impl VirtIOInput { /// Create a new VirtIO-Input driver. pub fn new(mut transport: T) -> Result { let mut event_buf = Box::new([InputEvent::default(); QUEUE_SIZE]); diff --git a/src/net.rs b/src/net.rs index 3eb6b0df..3641ac88 100644 --- a/src/net.rs +++ b/src/net.rs @@ -14,14 +14,14 @@ use log::*; /// Empty buffers are placed in one virtqueue for receiving packets, and /// outgoing packets are enqueued into another for transmission in that order. /// A third command queue is used to control advanced filtering features. -pub struct VirtIONet<'a, H: Hal, T: Transport> { +pub struct VirtIONet { transport: T, mac: EthernetAddress, - recv_queue: VirtQueue<'a, H>, - send_queue: VirtQueue<'a, H>, + recv_queue: VirtQueue, + send_queue: VirtQueue, } -impl VirtIONet<'_, H, T> { +impl VirtIONet { /// Create a new VirtIO-Net driver. pub fn new(mut transport: T) -> Result { transport.begin_init(|features| { diff --git a/src/queue.rs b/src/queue.rs index 521b9062..69604216 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -1,28 +1,26 @@ +#[cfg(test)] +use core::cmp::min; use core::mem::size_of; -use core::slice; +use core::ptr::{self, addr_of_mut, NonNull}; use core::sync::atomic::{fence, Ordering}; -#[cfg(test)] -use core::{cmp::min, ptr}; use super::*; use crate::transport::Transport; use bitflags::*; -use ::volatile::Volatile; - /// The mechanism for bulk data transport on virtio devices. /// /// Each device can have zero or more virtqueues. #[derive(Debug)] -pub struct VirtQueue<'a, H: Hal> { +pub struct VirtQueue { /// DMA guard dma: DMA, /// Descriptor table - desc: &'a mut [Descriptor], + desc: NonNull<[Descriptor]>, /// Available ring - avail: &'a mut AvailRing, + avail: NonNull, /// Used ring - used: &'a UsedRing, + used: NonNull, /// The index of queue queue_idx: u32, @@ -39,7 +37,7 @@ pub struct VirtQueue<'a, H: Hal> { last_used_idx: u16, } -impl VirtQueue<'_, H> { +impl VirtQueue { /// Create a new VirtQueue. pub fn new(transport: &mut T, idx: usize, size: u16) -> Result { if transport.queue_used(idx as u32) { @@ -60,14 +58,21 @@ impl VirtQueue<'_, H> { dma.paddr() + layout.used_offset, ); - let desc = - unsafe { slice::from_raw_parts_mut(dma.vaddr() as *mut Descriptor, size as usize) }; - let avail = unsafe { &mut *((dma.vaddr() + layout.avail_offset) as *mut AvailRing) }; - let used = unsafe { &mut *((dma.vaddr() + layout.used_offset) as *mut UsedRing) }; + let desc = NonNull::new(ptr::slice_from_raw_parts_mut( + dma.vaddr() as *mut Descriptor, + size as usize, + )) + .unwrap(); + let avail = NonNull::new((dma.vaddr() + layout.avail_offset) as *mut AvailRing).unwrap(); + let used = NonNull::new((dma.vaddr() + layout.used_offset) as *mut UsedRing).unwrap(); // Link descriptors together. for i in 0..(size - 1) { - desc[i as usize].next.write(i + 1); + // Safe because `desc` is properly aligned, dereferenceable, initialised, and the device + // won't access the descriptors for the duration of this unsafe block. + unsafe { + (*desc.as_ptr())[i as usize].next = i + 1; + } } Ok(VirtQueue { @@ -98,47 +103,63 @@ impl VirtQueue<'_, H> { // allocate descriptors from free list let head = self.free_head; let mut last = self.free_head; - for input in inputs.iter() { - let desc = &mut self.desc[self.free_head as usize]; - desc.set_buf::(input); - desc.flags.write(DescFlags::NEXT); - last = self.free_head; - self.free_head = desc.next.read(); - } - for output in outputs.iter() { - let desc = &mut self.desc[self.free_head as usize]; - desc.set_buf::(output); - desc.flags.write(DescFlags::NEXT | DescFlags::WRITE); - last = self.free_head; - self.free_head = desc.next.read(); - } - // set last_elem.next = NULL - { - let desc = &mut self.desc[last as usize]; - let mut flags = desc.flags.read(); - flags.remove(DescFlags::NEXT); - desc.flags.write(flags); + + // Safe because self.desc is properly aligned, dereferenceable and initialised, and nothing + // else reads or writes the free descriptors during this block. + unsafe { + for input in inputs.iter() { + let mut desc = self.desc_ptr(self.free_head); + (*desc).set_buf::(input); + (*desc).flags = DescFlags::NEXT; + last = self.free_head; + self.free_head = (*desc).next; + } + for output in outputs.iter() { + let desc = self.desc_ptr(self.free_head); + (*desc).set_buf::(output); + (*desc).flags = DescFlags::NEXT | DescFlags::WRITE; + last = self.free_head; + self.free_head = (*desc).next; + } + + // set last_elem.next = NULL + (*self.desc_ptr(last)).flags.remove(DescFlags::NEXT); } self.num_used += (inputs.len() + outputs.len()) as u16; let avail_slot = self.avail_idx & (self.queue_size - 1); - self.avail.ring[avail_slot as usize].write(head); + // Safe because self.avail is properly aligned, dereferenceable and initialised. + unsafe { + (*self.avail.as_ptr()).ring[avail_slot as usize] = head; + } // write barrier fence(Ordering::SeqCst); // increase head of avail ring self.avail_idx = self.avail_idx.wrapping_add(1); - self.avail.idx.write(self.avail_idx); + // Safe because self.avail is properly aligned, dereferenceable and initialised. + unsafe { + (*self.avail.as_ptr()).idx = self.avail_idx; + } + Ok(head) } - /// Whether there is a used element that can pop. + /// Returns a non-null pointer to the descriptor at the given index. + fn desc_ptr(&mut self, index: u16) -> *mut Descriptor { + // Safe because self.desc is properly aligned and dereferenceable. + unsafe { addr_of_mut!((*self.desc.as_ptr())[index as usize]) } + } + + /// Returns whether there is a used element that can be popped. pub fn can_pop(&self) -> bool { - self.last_used_idx != self.used.idx.read() + // Safe because self.used points to a valid, aligned, initialised, dereferenceable, readable + // instance of UsedRing. + self.last_used_idx != unsafe { (*self.used.as_ptr()).idx } } - /// The number of free descriptors. + /// Returns the number of free descriptors. pub fn available_desc(&self) -> usize { (self.queue_size - self.num_used) as usize } @@ -147,17 +168,21 @@ impl VirtQueue<'_, H> { /// /// This will push all linked descriptors at the front of the free list. fn recycle_descriptors(&mut self, mut head: u16) { - let origin_free_head = self.free_head; + let original_free_head = self.free_head; self.free_head = head; loop { - let desc = &mut self.desc[head as usize]; - let flags = desc.flags.read(); - self.num_used -= 1; - if flags.contains(DescFlags::NEXT) { - head = desc.next.read(); - } else { - desc.next.write(origin_free_head); - return; + let desc = self.desc_ptr(head); + // Safe because self.desc is properly aligned, dereferenceable and initialised, and + // nothing else reads or writes the descriptor during this block. + unsafe { + let flags = (*desc).flags; + self.num_used -= 1; + if flags.contains(DescFlags::NEXT) { + head = (*desc).next; + } else { + (*desc).next = original_free_head; + return; + } } } } @@ -173,8 +198,14 @@ impl VirtQueue<'_, H> { fence(Ordering::SeqCst); let last_used_slot = self.last_used_idx & (self.queue_size - 1); - let index = self.used.ring[last_used_slot as usize].id.read() as u16; - let len = self.used.ring[last_used_slot as usize].len.read(); + let index; + let len; + // Safe because self.used points to a valid, aligned, initialised, dereferenceable, readable + // instance of UsedRing. + unsafe { + index = (*self.used.as_ptr()).ring[last_used_slot as usize].id as u16; + len = (*self.used.as_ptr()).ring[last_used_slot as usize].len; + } self.recycle_descriptors(index); self.last_used_idx = self.last_used_idx.wrapping_add(1); @@ -218,17 +249,16 @@ impl VirtQueueLayout { #[repr(C, align(16))] #[derive(Debug)] pub(crate) struct Descriptor { - addr: Volatile, - len: Volatile, - flags: Volatile, - next: Volatile, + addr: u64, + len: u32, + flags: DescFlags, + next: u16, } impl Descriptor { fn set_buf(&mut self, buf: &[u8]) { - self.addr - .write(H::virt_to_phys(buf.as_ptr() as usize) as u64); - self.len.write(buf.len() as u32); + self.addr = H::virt_to_phys(buf.as_ptr() as usize) as u64; + self.len = buf.len() as u32; } } @@ -247,11 +277,11 @@ bitflags! { #[repr(C)] #[derive(Debug)] struct AvailRing { - flags: Volatile, + flags: u16, /// A driver MUST NOT decrement the idx. - idx: Volatile, - ring: [Volatile; 32], // actual size: queue_size - used_event: Volatile, // unused + idx: u16, + ring: [u16; 32], // actual size: queue_size + used_event: u16, // unused } /// The used ring is where the device returns buffers once it is done with them: @@ -259,17 +289,17 @@ struct AvailRing { #[repr(C)] #[derive(Debug)] struct UsedRing { - flags: Volatile, - idx: Volatile, - ring: [UsedElem; 32], // actual size: queue_size - avail_event: Volatile, // unused + flags: u16, + idx: u16, + ring: [UsedElem; 32], // actual size: queue_size + avail_event: u16, // unused } #[repr(C)] #[derive(Debug)] struct UsedElem { - id: Volatile, - len: Volatile, + id: u32, + len: u32, } /// Simulates the device writing to a VirtIO queue, for use in tests. @@ -283,37 +313,38 @@ pub(crate) fn fake_write_to_queue( receive_queue_device_area: VirtAddr, data: &[u8], ) { - let descriptors = - unsafe { slice::from_raw_parts(receive_queue_descriptors, queue_size as usize) }; + let descriptors = ptr::slice_from_raw_parts(receive_queue_descriptors, queue_size as usize); let available_ring = receive_queue_driver_area as *const AvailRing; let used_ring = receive_queue_device_area as *mut UsedRing; + // Safe because the various pointers are properly aligned, dereferenceable, initialised, and + // nothing else accesses them during this block. unsafe { // Make sure there is actually at least one descriptor available to write to. - assert_ne!((*available_ring).idx.read(), (*used_ring).idx.read()); + assert_ne!((*available_ring).idx, (*used_ring).idx); // The fake device always uses descriptors in order, like VIRTIO_F_IN_ORDER, so // `used_ring.idx` marks the next descriptor we should take from the available ring. - let next_slot = (*used_ring).idx.read() & (queue_size - 1); - let head_descriptor_index = (*available_ring).ring[next_slot as usize].read(); - let mut descriptor = &descriptors[head_descriptor_index as usize]; + let next_slot = (*used_ring).idx & (queue_size - 1); + let head_descriptor_index = (*available_ring).ring[next_slot as usize]; + let mut descriptor = &(*descriptors)[head_descriptor_index as usize]; // Loop through all descriptors in the chain, writing data to them. let mut remaining_data = data; loop { // Check the buffer and write to it. - let flags = descriptor.flags.read(); + let flags = descriptor.flags; assert!(flags.contains(DescFlags::WRITE)); - let buffer_length = descriptor.len.read() as usize; + let buffer_length = descriptor.len as usize; let length_to_write = min(remaining_data.len(), buffer_length); ptr::copy( remaining_data.as_ptr(), - descriptor.addr.read() as *mut u8, + descriptor.addr as *mut u8, length_to_write, ); remaining_data = &remaining_data[length_to_write..]; if flags.contains(DescFlags::NEXT) { - let next = descriptor.next.read() as usize; - descriptor = &descriptors[next]; + let next = descriptor.next as usize; + descriptor = &(*descriptors)[next]; } else { assert_eq!(remaining_data.len(), 0); break; @@ -321,13 +352,9 @@ pub(crate) fn fake_write_to_queue( } // Mark the buffer as used. - (*used_ring).ring[next_slot as usize] - .id - .write(head_descriptor_index as u32); - (*used_ring).ring[next_slot as usize] - .len - .write(data.len() as u32); - (*used_ring).idx.update(|idx| *idx += 1); + (*used_ring).ring[next_slot as usize].id = head_descriptor_index as u32; + (*used_ring).ring[next_slot as usize].len = data.len() as u32; + (*used_ring).idx += 1; } } @@ -408,30 +435,49 @@ mod tests { assert_eq!(queue.available_desc(), 0); assert!(!queue.can_pop()); - let first_descriptor_index = queue.avail.ring[0].read(); - assert_eq!(first_descriptor_index, token); - assert_eq!(queue.desc[first_descriptor_index as usize].len.read(), 2); - assert_eq!( - queue.desc[first_descriptor_index as usize].flags.read(), - DescFlags::NEXT - ); - let second_descriptor_index = queue.desc[first_descriptor_index as usize].next.read(); - assert_eq!(queue.desc[second_descriptor_index as usize].len.read(), 1); - assert_eq!( - queue.desc[second_descriptor_index as usize].flags.read(), - DescFlags::NEXT - ); - let third_descriptor_index = queue.desc[second_descriptor_index as usize].next.read(); - assert_eq!(queue.desc[third_descriptor_index as usize].len.read(), 2); - assert_eq!( - queue.desc[third_descriptor_index as usize].flags.read(), - DescFlags::NEXT | DescFlags::WRITE - ); - let fourth_descriptor_index = queue.desc[third_descriptor_index as usize].next.read(); - assert_eq!(queue.desc[fourth_descriptor_index as usize].len.read(), 1); - assert_eq!( - queue.desc[fourth_descriptor_index as usize].flags.read(), - DescFlags::WRITE - ); + // Safe because the various parts of the queue are properly aligned, dereferenceable and + // initialised, and nothing else is accessing them at the same time. + unsafe { + let first_descriptor_index = (*queue.avail.as_ptr()).ring[0]; + assert_eq!(first_descriptor_index, token); + assert_eq!( + (*queue.desc.as_ptr())[first_descriptor_index as usize].len, + 2 + ); + assert_eq!( + (*queue.desc.as_ptr())[first_descriptor_index as usize].flags, + DescFlags::NEXT + ); + let second_descriptor_index = + (*queue.desc.as_ptr())[first_descriptor_index as usize].next; + assert_eq!( + (*queue.desc.as_ptr())[second_descriptor_index as usize].len, + 1 + ); + assert_eq!( + (*queue.desc.as_ptr())[second_descriptor_index as usize].flags, + DescFlags::NEXT + ); + let third_descriptor_index = + (*queue.desc.as_ptr())[second_descriptor_index as usize].next; + assert_eq!( + (*queue.desc.as_ptr())[third_descriptor_index as usize].len, + 2 + ); + assert_eq!( + (*queue.desc.as_ptr())[third_descriptor_index as usize].flags, + DescFlags::NEXT | DescFlags::WRITE + ); + let fourth_descriptor_index = + (*queue.desc.as_ptr())[third_descriptor_index as usize].next; + assert_eq!( + (*queue.desc.as_ptr())[fourth_descriptor_index as usize].len, + 1 + ); + assert_eq!( + (*queue.desc.as_ptr())[fourth_descriptor_index as usize].flags, + DescFlags::WRITE + ); + } } } From 8f086e70740b269a090509e784434e4948b0749e Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Wed, 14 Sep 2022 17:07:40 +0100 Subject: [PATCH 8/8] Add fence before reading used ring and after writing available ring. --- src/queue.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/queue.rs b/src/queue.rs index 69604216..264c87ec 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -133,7 +133,8 @@ impl VirtQueue { (*self.avail.as_ptr()).ring[avail_slot as usize] = head; } - // write barrier + // Write barrier so that device sees changes to descriptor table and available ring before + // change to available index. fence(Ordering::SeqCst); // increase head of avail ring @@ -143,6 +144,9 @@ impl VirtQueue { (*self.avail.as_ptr()).idx = self.avail_idx; } + // Write barrier so that device can see change to available index after this method returns. + fence(Ordering::SeqCst); + Ok(head) } @@ -154,6 +158,9 @@ impl VirtQueue { /// Returns whether there is a used element that can be popped. pub fn can_pop(&self) -> bool { + // Read barrier, so we read a fresh value from the device. + fence(Ordering::SeqCst); + // Safe because self.used points to a valid, aligned, initialised, dereferenceable, readable // instance of UsedRing. self.last_used_idx != unsafe { (*self.used.as_ptr()).idx }