Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Set the member variables of the virtio queue to volatile. #127

Closed
wants to merge 1 commit into from
Closed

fix: Set the member variables of the virtio queue to volatile. #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 51 additions & 21 deletions src/queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

use crate::hal::{BufferDirection, Dma, Hal, PhysAddr};
use crate::transport::Transport;
use crate::volatile::Volatile;
use crate::{align_up, nonnull_slice_from_raw_parts, pages, Error, Result, PAGE_SIZE};
#[cfg(feature = "alloc")]
use alloc::boxed::Box;
use bitflags::bitflags;
#[cfg(test)]
use core::cmp::min;
use core::fmt::Debug;
use core::hint::spin_loop;
use core::mem::{size_of, take};
#[cfg(test)]
Expand Down Expand Up @@ -71,7 +73,7 @@
pub fn new<T: Transport>(
transport: &mut T,
idx: u16,
indirect: bool,

Check warning on line 76 in src/queue.rs

View workflow job for this annotation

GitHub Actions / build 

unused variable: `indirect`

Check warning on line 76 in src/queue.rs

View workflow job for this annotation

GitHub Actions / build 

unused variable: `indirect`
event_idx: bool,
) -> Result<Self> {
if transport.queue_used(idx) {
Expand Down Expand Up @@ -192,7 +194,7 @@
self.avail_idx = self.avail_idx.wrapping_add(1);
// Safe because self.avail is properly aligned, dereferenceable and initialised.
unsafe {
(*self.avail.as_ptr()).idx = self.avail_idx;
(*self.avail.as_ptr()).idx.write_volatile(self.avail_idx);
}

// Write barrier so that device can see change to available index after this method returns.
Expand Down Expand Up @@ -324,7 +326,11 @@
if !self.event_idx {
// Safe because self.avail points to a valid, aligned, initialised, dereferenceable, readable
// instance of AvailRing.
unsafe { (*self.avail.as_ptr()).flags = avail_ring_flags }
unsafe {
(*self.avail.as_ptr())
.flags
.write_volatile(avail_ring_flags)
}
}
// Write barrier so that device can see change to available index after this method returns.
fence(Ordering::SeqCst);
Expand All @@ -341,12 +347,12 @@
if self.event_idx {
// Safe because self.used points to a valid, aligned, initialised, dereferenceable, readable
// instance of UsedRing.
let avail_event = unsafe { (*self.used.as_ptr()).avail_event };
let avail_event = unsafe { (*self.used.as_ptr()).avail_event.read_volatile() };
self.avail_idx >= avail_event.wrapping_add(1)
} else {
// Safe because self.used points to a valid, aligned, initialised, dereferenceable, readable
// instance of UsedRing.
unsafe { (*self.used.as_ptr()).flags & 0x0001 == 0 }
unsafe { (*self.used.as_ptr()).flags.read_volatile() & 0x0001 == 0 }
}
}

Expand All @@ -368,7 +374,7 @@

// 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 }
self.last_used_idx != unsafe { (*self.used.as_ptr()).idx.read_volatile() }
}

/// Returns the descriptor index (a.k.a. token) of the next used element without popping it, or
Expand Down Expand Up @@ -532,7 +538,9 @@

if self.event_idx {
unsafe {
(*self.avail.as_ptr()).used_event = self.last_used_idx;
(*self.avail.as_ptr())
.used_event
.write_volatile(self.last_used_idx);
}
}

Expand Down Expand Up @@ -759,26 +767,36 @@
/// each ring entry refers to the head of a descriptor chain.
/// It is only written by the driver and read by the device.
#[repr(C)]
#[derive(Debug)]
struct AvailRing<const SIZE: usize> {
flags: u16,
flags: Volatile<u16>,
/// A driver MUST NOT decrement the idx.
idx: u16,
idx: Volatile<u16>,
ring: [u16; SIZE],
/// Only used if `VIRTIO_F_EVENT_IDX` is negotiated.
used_event: u16,
used_event: Volatile<u16>,
}

impl<const SIZE: usize> Debug for AvailRing<SIZE> {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
f.debug_struct("AvailRing").finish()
}
}

/// The used ring is where the device returns buffers once it is done with them:
/// it is only written to by the device, and read by the driver.
#[repr(C)]
#[derive(Debug)]
struct UsedRing<const SIZE: usize> {
flags: u16,
idx: u16,
flags: Volatile<u16>,
idx: Volatile<u16>,
ring: [UsedElem; SIZE],
/// Only used if `VIRTIO_F_EVENT_IDX` is negotiated.
avail_event: u16,
avail_event: Volatile<u16>,
}

impl<const SIZE: usize> Debug for UsedRing<SIZE> {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
f.debug_struct("UsedRing").finish()
}
}

#[repr(C)]
Expand Down Expand Up @@ -847,10 +865,13 @@
// nothing else accesses them during this block.
unsafe {
// Make sure there is actually at least one descriptor available to read from.
assert_ne!((*available_ring).idx, (*used_ring).idx);
assert_ne!(
(*available_ring).idx.read_volatile(),
(*used_ring).idx.read_volatile()
);
// 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 & (QUEUE_SIZE as u16 - 1);
let next_slot = (*used_ring).idx.read_volatile() & (QUEUE_SIZE as u16 - 1);
let head_descriptor_index = (*available_ring).ring[next_slot as usize];
let mut descriptor = &(*descriptors)[head_descriptor_index as usize];

Expand Down Expand Up @@ -1156,17 +1177,26 @@
let mut queue = VirtQueue::<FakeHal, 4>::new(&mut transport, 0, false, false).unwrap();

// Check that the avail ring's flag is zero by default.
assert_eq!(unsafe { (*queue.avail.as_ptr()).flags }, 0x0);
assert_eq!(
unsafe { (*queue.avail.as_ptr()).flags.read_volatile() },
0x0
);

queue.set_dev_notify(false);

// Check that the avail ring's flag is 1 after `disable_dev_notify`.
assert_eq!(unsafe { (*queue.avail.as_ptr()).flags }, 0x1);
assert_eq!(
unsafe { (*queue.avail.as_ptr()).flags.read_volatile() },
0x1
);

queue.set_dev_notify(true);

// Check that the avail ring's flag is 0 after `enable_dev_notify`.
assert_eq!(unsafe { (*queue.avail.as_ptr()).flags }, 0x0);
assert_eq!(
unsafe { (*queue.avail.as_ptr()).flags.read_volatile() },
0x0
);
}

/// Tests that the queue notifies the device about added buffers, if it hasn't suppressed
Expand Down Expand Up @@ -1197,7 +1227,7 @@
// initialised, and nothing else is accessing them at the same time.
unsafe {
// Suppress notifications.
(*queue.used.as_ptr()).flags = 0x01;
(*queue.used.as_ptr()).flags.write_volatile(0x01);
}

// Check that the transport would not be notified.
Expand Down Expand Up @@ -1232,7 +1262,7 @@
// initialised, and nothing else is accessing them at the same time.
unsafe {
// Suppress notifications.
(*queue.used.as_ptr()).avail_event = 1;
(*queue.used.as_ptr()).avail_event.write_volatile(1);
}

// Check that the transport would not be notified.
Expand Down
56 changes: 56 additions & 0 deletions src/volatile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,59 @@ impl<T: Copy> Volatile<T> {
pub fn new(value: T) -> Self {
Self(value)
}

#[inline]
fn self_ptr(&self) -> NonNull<T> {
NonNull::from(&self.0)
}

#[inline(always)]
pub fn read_volatile(&self) -> T {
unsafe { self.self_ptr().as_ptr().read_volatile() }
}

#[inline(always)]
pub fn write_volatile(&mut self, value: T) {
unsafe { self.self_ptr().as_ptr().write_volatile(value) }
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods don't make sense. If a memory location must be accessed through volatile reads or writes, e.g. because it is MMIO, then it's not sound to hold a reference to it, because the Rust memory model allows the compiler to dereference references whenever it wants. So the only case where these methods can be called is when Volatile is being used incorrectly. The intention of this type is that it is only ever referred to via a pointer, and read and written via the VolatileReadable and VolatileWritable traits, possibly via the volread! and volwrite! macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do not implement this method, then when changing the volatile variables of itself in the used ring and avail ring, I would also need to implement a method similar to self_ref(). However, that would lead to rather redundant code, so that's why I wrote it like this.

}

macro_rules! impl_volatile_ops {
($($t:ty),*) => {
$(
impl AddAssign<$t> for Volatile<$t> {
fn add_assign(&mut self, rhs: $t) {
self.write_volatile(self.read_volatile() + rhs);
}
}

impl Add<$t> for Volatile<$t> {
type Output = $t;

fn add(self, rhs: $t) -> $t {
self.read_volatile() + rhs
}
}

impl SubAssign<$t> for Volatile<$t> {
fn sub_assign(&mut self, rhs: $t) {
self.write_volatile(self.read_volatile() - rhs);
}
}

impl Sub<$t> for Volatile<$t> {
type Output = $t;

fn sub(self, rhs: $t) -> $t {
self.read_volatile() - rhs
}
}
)*
};
}

impl_volatile_ops!(u8, u16, u32, u64, usize, i8, i16, i32, i64, isize);

/// A trait implemented by MMIO registers which may be read from.
pub trait VolatileReadable<T> {
/// Performs a volatile read from the MMIO register.
Expand Down Expand Up @@ -104,5 +155,10 @@ macro_rules! volwrite {
};
}

use core::{
ops::{Add, AddAssign, Sub, SubAssign},
ptr::NonNull,
};

pub(crate) use volread;
pub(crate) use volwrite;
Loading