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

Add methods to Transport to read and write config space, rather than returning a pointer. #163

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Add methods to Transport to read and write config space, rather than returning a pointer. #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41dff35
Add methods to Transport to read and write config space, rather than
qwandor Oct 28, 2024
3bcb721
Return Result from config space read/write methods.
qwandor Oct 28, 2024
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
2 changes: 1 addition & 1 deletion examples/aarch64/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ fn virtio_net<T: Transport>(transport: T) {
fn virtio_console<T: Transport>(transport: T) {
let mut console =
VirtIOConsole::<HalImpl, T>::new(transport).expect("Failed to create console driver");
if let Some(size) = console.size() {
if let Some(size) = console.size().unwrap() {
info!("VirtIO console {}", size);
}
for &c in b"Hello world on console!\n" {
Expand Down
13 changes: 6 additions & 7 deletions src/device/blk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
use crate::hal::Hal;
use crate::queue::VirtQueue;
use crate::transport::Transport;
use crate::volatile::{volread, Volatile};
use crate::volatile::Volatile;
use crate::{Error, Result};
use bitflags::bitflags;
use core::mem::offset_of;
use log::info;
use zerocopy::{FromBytes, Immutable, IntoBytes, KnownLayout};

Expand Down Expand Up @@ -55,12 +56,10 @@ impl<H: Hal, T: Transport> VirtIOBlk<H, T> {
let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);

// Read configuration space.
let config = transport.config_space::<BlkConfig>()?;
info!("config: {:?}", config);
// Safe because config is a valid pointer to the device configuration space.
let capacity = unsafe {
volread!(config, capacity_low) as u64 | (volread!(config, capacity_high) as u64) << 32
};
let capacity = transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_low))?
Copy link
Contributor

Choose a reason for hiding this comment

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

In the new code, the you have to be careful about keeping the template param and the field types in sync at every call point. It looks easy to mess up. I wonder if we could have a macro like read_config_space!(transport, BlkConfig, capacity_low) that handles the offset and makes sure the types line up properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same thought, but can't find any way to do so as there doesn't seem to be any macro to get the type of a struct field from its name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a trick

macro_rules! offset_and_size {
    ($t:ty, $f:ident) => {{
        let v: $t = Default::default();
        (std::mem::offset_of!($t, $f), std::mem::size_of_val(&v.$f))
    }};
}

Seems to optimize out https://godbolt.org/z/hzWGjqxY1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think getting the size helps, the issue is to get the type. But I think I have found a way around that, I'll send another PR in a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Example at #171

Whoops, it seems I only half read your comment, didn't see that you were going to send a PR until after I sent that one just now.

as u64
| (transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_high))? as u64)
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR, but technically you should use the config generation field when doing multiple part reads like this: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-230001

(FWIW, the crosvm VMM currently always has config_generation == 0)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I missed that. Done in #169.

<< 32;
info!("found a block device of size {}KB", capacity / 2);

let queue = VirtQueue::new(
Expand Down
40 changes: 15 additions & 25 deletions src/device/console.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,12 @@ mod embedded_io;
use crate::hal::Hal;
use crate::queue::VirtQueue;
use crate::transport::Transport;
use crate::volatile::{volread, volwrite, ReadOnly, WriteOnly};
use crate::volatile::{ReadOnly, WriteOnly};
use crate::{Error, Result, PAGE_SIZE};
use alloc::boxed::Box;
use bitflags::bitflags;
use core::{
fmt::{self, Display, Formatter, Write},
ptr::NonNull,
};
use core::fmt::{self, Display, Formatter, Write};
use core::mem::offset_of;
use log::error;

const QUEUE_RECEIVEQ_PORT_0: u16 = 0;
Expand All @@ -36,7 +34,7 @@ const SUPPORTED_FEATURES: Features = Features::RING_EVENT_IDX
/// # fn example<HalImpl: Hal, T: Transport>(transport: T) -> Result<(), Error> {
/// let mut console = VirtIOConsole::<HalImpl, _>::new(transport)?;
///
/// let size = console.size().unwrap();
/// let size = console.size().unwrap().unwrap();
/// println!("VirtIO console {}x{}", size.rows, size.columns);
///
/// for &c in b"Hello console!\n" {
Expand All @@ -51,7 +49,6 @@ const SUPPORTED_FEATURES: Features = Features::RING_EVENT_IDX
pub struct VirtIOConsole<H: Hal, T: Transport> {
transport: T,
negotiated_features: Features,
config_space: NonNull<Config>,
receiveq: VirtQueue<H, QUEUE_SIZE>,
transmitq: VirtQueue<H, QUEUE_SIZE>,
queue_buf_rx: Box<[u8; PAGE_SIZE]>,
Expand Down Expand Up @@ -94,7 +91,6 @@ impl<H: Hal, T: Transport> VirtIOConsole<H, T> {
/// Creates a new VirtIO console driver.
pub fn new(mut transport: T) -> Result<Self> {
let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);
let config_space = transport.config_space::<Config>()?;
let receiveq = VirtQueue::new(
&mut transport,
QUEUE_RECEIVEQ_PORT_0,
Expand All @@ -117,7 +113,6 @@ impl<H: Hal, T: Transport> VirtIOConsole<H, T> {
let mut console = VirtIOConsole {
transport,
negotiated_features,
config_space,
receiveq,
transmitq,
queue_buf_rx,
Expand All @@ -130,17 +125,14 @@ impl<H: Hal, T: Transport> VirtIOConsole<H, T> {
}

/// Returns the size of the console, if the device supports reporting this.
pub fn size(&self) -> Option<Size> {
pub fn size(&self) -> Result<Option<Size>> {
if self.negotiated_features.contains(Features::SIZE) {
// SAFETY: self.config_space is a valid pointer to the device configuration space.
unsafe {
Some(Size {
columns: volread!(self.config_space, cols),
rows: volread!(self.config_space, rows),
})
}
Ok(Some(Size {
columns: self.transport.read_config_space(offset_of!(Config, cols))?,
rows: self.transport.read_config_space(offset_of!(Config, rows))?,
}))
} else {
None
Ok(None)
}
}

Expand Down Expand Up @@ -246,10 +238,8 @@ impl<H: Hal, T: Transport> VirtIOConsole<H, T> {
/// Returns an error if the device doesn't support emergency write.
pub fn emergency_write(&mut self, chr: u8) -> Result<()> {
if self.negotiated_features.contains(Features::EMERG_WRITE) {
// SAFETY: `self.config_space` is a valid pointer to the device configuration space.
unsafe {
volwrite!(self.config_space, emerg_wr, chr.into());
}
self.transport
.write_config_space::<u32>(offset_of!(Config, emerg_wr), chr.into())?;
Ok(())
} else {
Err(Error::Unsupported)
Expand Down Expand Up @@ -343,7 +333,7 @@ mod tests {
};
let console = VirtIOConsole::<FakeHal, FakeTransport<Config>>::new(transport).unwrap();

assert_eq!(console.size(), None);
assert_eq!(console.size(), Ok(None));
}

#[test]
Expand All @@ -369,10 +359,10 @@ mod tests {

assert_eq!(
console.size(),
Some(Size {
Ok(Some(Size {
columns: 80,
rows: 42
})
}))
);
}

Expand Down
18 changes: 8 additions & 10 deletions src/device/gpu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
use crate::hal::{BufferDirection, Dma, Hal};
use crate::queue::VirtQueue;
use crate::transport::Transport;
use crate::volatile::{volread, ReadOnly, Volatile, WriteOnly};
use crate::volatile::{ReadOnly, Volatile, WriteOnly};
use crate::{pages, Error, Result, PAGE_SIZE};
use alloc::boxed::Box;
use bitflags::bitflags;
use core::mem::offset_of;
use log::info;
use zerocopy::{FromBytes, FromZeros, Immutable, IntoBytes, KnownLayout};

Expand Down Expand Up @@ -43,15 +44,12 @@
let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);

// read configuration space
let config_space = transport.config_space::<Config>()?;
unsafe {
let events_read = volread!(config_space, events_read);
let num_scanouts = volread!(config_space, num_scanouts);
info!(
"events_read: {:#x}, num_scanouts: {:#x}",
events_read, num_scanouts
);
}
let events_read = transport.read_config_space::<u32>(offset_of!(Config, events_read))?;
let num_scanouts = transport.read_config_space::<u32>(offset_of!(Config, num_scanouts))?;
info!(
"events_read: {:#x}, num_scanouts: {:#x}",
events_read, num_scanouts
);

let control_queue = VirtQueue::new(
&mut transport,
Expand Down Expand Up @@ -255,16 +253,16 @@
rsp.check_type(Command::OK_NODATA)
}

fn update_cursor(
&mut self,
resource_id: u32,
scanout_id: u32,
pos_x: u32,
pos_y: u32,
hot_x: u32,
hot_y: u32,
is_move: bool,
) -> Result {

Check warning on line 265 in src/device/gpu.rs

View workflow job for this annotation

GitHub Actions / clippy

this function has too many arguments (8/7) 

warning: this function has too many arguments (8/7)
   --> src/device/gpu.rs:256:5
    |
256 | /     fn update_cursor(
257 | |         &mut self,
258 | |         resource_id: u32,
259 | |         scanout_id: u32,
...   |
264 | |         is_move: bool,
265 | |     ) -> Result {
    | |_______________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
    = note: `#[warn(clippy::too_many_arguments)]` on by default
self.cursor_request(UpdateCursor {
header: if is_move {
CtrlHeader::with_type(Command::MOVE_CURSOR)
Expand Down
70 changes: 36 additions & 34 deletions src/device/input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@ use super::common::Feature;
use crate::hal::Hal;
use crate::queue::VirtQueue;
use crate::transport::Transport;
use crate::volatile::{volread, volwrite, ReadOnly, VolatileReadable, WriteOnly};
use crate::volatile::{ReadOnly, WriteOnly};
use crate::Error;
use alloc::{boxed::Box, string::String};
use core::cmp::min;
use core::mem::size_of;
use core::ptr::{addr_of, NonNull};
use core::mem::{offset_of, size_of};
use zerocopy::{FromBytes, FromZeros, Immutable, IntoBytes, KnownLayout};

/// Virtual human interface devices such as keyboards, mice and tablets.
Expand All @@ -22,7 +21,6 @@ pub struct VirtIOInput<H: Hal, T: Transport> {
event_queue: VirtQueue<H, QUEUE_SIZE>,
status_queue: VirtQueue<H, QUEUE_SIZE>,
event_buf: Box<[InputEvent; 32]>,
config: NonNull<Config>,
}

impl<H: Hal, T: Transport> VirtIOInput<H, T> {
Expand All @@ -32,8 +30,6 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {

let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);

let config = transport.config_space::<Config>()?;

let mut event_queue = VirtQueue::new(
&mut transport,
QUEUE_EVENT,
Expand Down Expand Up @@ -62,7 +58,6 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
event_queue,
status_queue,
event_buf,
config,
})
}

Expand Down Expand Up @@ -107,19 +102,21 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
select: InputConfigSelect,
subsel: u8,
out: &mut [u8],
) -> u8 {
let size;
) -> Result<u8, 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: u8 = self.transport.read_config_space(offset_of!(Config, size))?;
// Safe because config points to a valid MMIO region for the config space.
unsafe {
volwrite!(self.config, select, select as u8);
volwrite!(self.config, subsel, subsel);
size = volread!(self.config, size);
let size_to_copy = min(usize::from(size), out.len());
for (i, out_item) in out.iter_mut().take(size_to_copy).enumerate() {
*out_item = addr_of!((*self.config.as_ptr()).data[i]).vread();
}
let size_to_copy = min(usize::from(size), out.len());
for (i, out_item) in out.iter_mut().take(size_to_copy).enumerate() {
*out_item = self
.transport
.read_config_space(offset_of!(Config, data) + i * size_of::<u8>())?;
}
size

Ok(size)
}

/// Queries a specific piece of information by `select` and `subsel`, allocates a sufficiently
Expand All @@ -129,20 +126,24 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
select: InputConfigSelect,
subsel: u8,
) -> Result<Box<[u8]>, Error> {
// Safe because config points to a valid MMIO region for the config space.
unsafe {
volwrite!(self.config, select, select as u8);
volwrite!(self.config, subsel, subsel);
let size = usize::from(volread!(self.config, size));
if size > CONFIG_DATA_MAX_LENGTH {
return Err(Error::IoError);
}
let mut buf = <[u8]>::new_box_zeroed_with_elems(size).unwrap();
for i in 0..size {
buf[i] = addr_of!((*self.config.as_ptr()).data[i]).vread();
}
Ok(buf)
self.transport
.write_config_space(offset_of!(Config, select), select as u8)?;
self.transport
.write_config_space(offset_of!(Config, subsel), subsel)?;
let size = usize::from(
self.transport
.read_config_space::<u8>(offset_of!(Config, size))?,
);
if size > CONFIG_DATA_MAX_LENGTH {
return Err(Error::IoError);
}
let mut buf = <[u8]>::new_box_zeroed_with_elems(size).unwrap();
for i in 0..size {
buf[i] = self
.transport
.read_config_space(offset_of!(Config, data) + i * size_of::<u8>())?;
}
Ok(buf)
}

/// Queries a specific piece of information by `select` and `subsel` into a newly-allocated
Expand Down Expand Up @@ -172,7 +173,7 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
/// Queries and returns the ID information of the device.
pub fn ids(&mut self) -> Result<DevIDs, Error> {
let mut ids = DevIDs::default();
let size = self.query_config_select(InputConfigSelect::IdDevids, 0, ids.as_mut_bytes());
let size = self.query_config_select(InputConfigSelect::IdDevids, 0, ids.as_mut_bytes())?;
if usize::from(size) == size_of::<DevIDs>() {
Ok(ids)
} else {
Expand All @@ -195,7 +196,8 @@ impl<H: Hal, T: Transport> VirtIOInput<H, T> {
/// Queries and returns information about the given axis of the device.
pub fn abs_info(&mut self, axis: u8) -> Result<AbsInfo, Error> {
let mut info = AbsInfo::default();
let size = self.query_config_select(InputConfigSelect::AbsInfo, axis, info.as_mut_bytes());
let size =
self.query_config_select(InputConfigSelect::AbsInfo, axis, info.as_mut_bytes())?;
if usize::from(size) == size_of::<AbsInfo>() {
Ok(info)
} else {
Expand Down Expand Up @@ -322,7 +324,7 @@ mod tests {
},
};
use alloc::{sync::Arc, vec};
use core::convert::TryInto;
use core::{convert::TryInto, ptr::NonNull};
use std::sync::Mutex;

#[test]
Expand Down
21 changes: 8 additions & 13 deletions src/device/net/dev_raw.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use super::{Config, EthernetAddress, Features, VirtioNetHdr};
use super::{MIN_BUFFER_LEN, NET_HDR_SIZE, QUEUE_RECEIVE, QUEUE_TRANSMIT, SUPPORTED_FEATURES};
use crate::device::net::Status;
use crate::hal::Hal;
use crate::queue::VirtQueue;
use crate::transport::Transport;
use crate::volatile::volread;
use crate::{Error, Result};
use core::mem::offset_of;
use log::{debug, info, warn};
use zerocopy::IntoBytes;

Expand All @@ -28,18 +29,12 @@ impl<H: Hal, T: Transport, const QUEUE_SIZE: usize> VirtIONetRaw<H, T, QUEUE_SIZ
pub fn new(mut transport: T) -> Result<Self> {
let negotiated_features = transport.begin_init(SUPPORTED_FEATURES);
info!("negotiated_features {:?}", negotiated_features);
// read configuration space
let config = transport.config_space::<Config>()?;
let mac;
// Safe because config points to a valid MMIO region for the config space.
unsafe {
mac = volread!(config, mac);
debug!(
"Got MAC={:02x?}, status={:?}",
mac,
volread!(config, status)
);
}

// Read configuration space.
let mac = transport.read_config_space(offset_of!(Config, mac))?;
let status = transport.read_config_space::<Status>(offset_of!(Config, status))?;
debug!("Got MAC={:02x?}, status={:?}", mac, status);

let send_queue = VirtQueue::new(
&mut transport,
QUEUE_TRANSMIT,
Expand Down
7 changes: 5 additions & 2 deletions src/device/net/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,12 @@ bitflags! {
}
}

#[derive(Copy, Clone, Debug, Default, Eq, FromBytes, Immutable, KnownLayout, PartialEq)]
#[repr(transparent)]
struct Status(u16);

bitflags! {
#[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
struct Status: u16 {
impl Status: u16 {
const LINK_UP = 1;
const ANNOUNCE = 2;
}
Expand Down
Loading
Loading