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

Jump to bottom

virtio-device: extend VirtioDeviceActions Trait #291

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

virtio-device: extend VirtioDeviceActions Trait #291

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
98 changes: 68 additions & 30 deletions virtio-device/src/virtio_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use std::sync::Arc;

use log::error;

use crate::status::{ACKNOWLEDGE, DRIVER, FEATURES_OK};
use crate::{VirtioDevice, WithDriverSelect};
use virtio_queue::{Queue, QueueT};

Expand Down Expand Up @@ -76,7 +77,7 @@ pub trait VirtioDeviceType {
/// Helper trait that can be implemented for objects which represent virtio devices. Together
/// with `VirtioDeviceType`, it enables an automatic `VirtioDevice` implementation for objects
/// that also implement `BorrowMut<VirtioConfig>`.
pub trait VirtioDeviceActions {
pub trait VirtioDeviceActions: BorrowMut<VirtioConfig<Queue>> {
/// Type of the error that can be returned by `activate` and `reset`.
type E;

Expand All @@ -85,6 +86,58 @@ pub trait VirtioDeviceActions {

/// Invoke the logic associated with resetting this device.
fn reset(&mut self) -> result::Result<(), Self::E>;

/// Invoke the logic associated with reading from the device configuration space.
///
/// A default implementation is provided as we cannot expect all backends to implement
/// this function.
fn read_config(&self, offset: usize, data: &mut [u8]) {
let config_space = &self.borrow().config_space;
let config_len = config_space.len();
if offset >= config_len {
error!("Failed to read from config space");
return;
}

// TODO: Are partial reads ok?
let end = cmp::min(offset.saturating_add(data.len()), config_len);
let read_len = end - offset;
// Cannot fail because the lengths are identical and we do bounds checking beforehand.
data[..read_len].copy_from_slice(&config_space[offset..end])
}

/// Invoke the logic associated with writing to the device configuration space.
///
/// A default implementation is provided as we cannot expect all backends to implement
/// this function.
fn write_config(&mut self, offset: usize, data: &[u8]) {
let config_space = &mut self.borrow_mut().config_space;
let config_len = config_space.len();
if offset >= config_len {
error!("Failed to write to config space");
return;
}

// TODO: Are partial writes ok?
let end = cmp::min(offset.saturating_add(data.len()), config_len);
let write_len = end - offset;
// Cannot fail because the lengths are identical and we do bounds checking beforehand.
config_space[offset..end].copy_from_slice(&data[..write_len]);
}

/// Invoke the logic associated with negotiating the driver features.
///
/// A default implementation is provided as we cannot expect all backends to implement
/// this function.
fn negotiate_driver_features(&mut self) {}

/// Invoke the logic associated with the device interrupt status.
///
/// A default implementation is provided as we cannot expect all backends to implement
/// this function.
fn interrupt_status(&self) -> &Arc<AtomicU8> {
&self.borrow().interrupt_status
}
}

// We can automatically implement the `VirtioDevice` trait for objects that only explicitly
Expand Down Expand Up @@ -139,6 +192,9 @@ where

fn set_device_status(&mut self, status: u8) {
self.borrow_mut().device_status = status;
if self.borrow_mut().device_status == (ACKNOWLEDGE | DRIVER | FEATURES_OK) {
<Self as VirtioDeviceActions>::negotiate_driver_features(self);
}
}

fn activate(&mut self) -> Result<(), Self::E> {
Expand All @@ -150,41 +206,19 @@ where
}

fn interrupt_status(&self) -> &Arc<AtomicU8> {
&self.borrow().interrupt_status
<Self as VirtioDeviceActions>::interrupt_status(self)
}

fn config_generation(&self) -> u8 {
self.borrow().config_generation
}

fn read_config(&self, offset: usize, data: &mut [u8]) {
let config_space = &self.borrow().config_space;
let config_len = config_space.len();
if offset >= config_len {
error!("Failed to read from config space");
return;
}

// TODO: Are partial reads ok?
let end = cmp::min(offset.saturating_add(data.len()), config_len);
let read_len = end - offset;
// Cannot fail because the lengths are identical and we do bounds checking beforehand.
data[..read_len].copy_from_slice(&config_space[offset..end])
<Self as VirtioDeviceActions>::read_config(self, offset, data)
}
Comment on lines -161 to 218
Copy link
Member

Choose a reason for hiding this comment

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

Now read_config is delegated to VirtioDeviceActions, but that means T must implement it now, whereas it did it not have to before. Or did I miss something? It seems like you're taking away functionality here, not adding.

Copy link
Author

Choose a reason for hiding this comment

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

@epilys You are right. I have already placed the default implementations within the VirtioDeviceActions trait. Only the devices that need to invoke some additional logic must implement it. For example, for the vhost-user devices and when the driver intends to read from or write to the device configuration space, if the protocol feature CONFIG is negotiated, dedicated logic is necessary to handle these operations (e.g. GET_CONFIG/SET_CONFIG requests).


fn write_config(&mut self, offset: usize, data: &[u8]) {
let config_space = &mut self.borrow_mut().config_space;
let config_len = config_space.len();
if offset >= config_len {
error!("Failed to write to config space");
return;
}

// TODO: Are partial writes ok?
let end = cmp::min(offset.saturating_add(data.len()), config_len);
let write_len = end - offset;
// Cannot fail because the lengths are identical and we do bounds checking beforehand.
config_space[offset..end].copy_from_slice(&data[..write_len]);
<Self as VirtioDeviceActions>::write_config(self, offset, data)
}
}

Expand Down Expand Up @@ -222,6 +256,7 @@ pub(crate) mod tests {
use super::*;
use crate::mmio::VirtioMmioDevice;
use std::borrow::Borrow;
use std::sync::atomic::Ordering;

pub struct Dummy {
pub cfg: VirtioConfig<Queue>,
Expand Down Expand Up @@ -320,10 +355,10 @@ pub(crate) mod tests {
let mut v2 = vec![0u8; len];

// Offset to large to read anything.
d.read_config(len, v2.as_mut_slice());
VirtioDevice::read_config(&d, len, v2.as_mut_slice());
assert_eq!(v1, v2);

d.read_config(len / 2, v2.as_mut_slice());
VirtioDevice::read_config(&d, len / 2, v2.as_mut_slice());
for i in 0..len {
if i < len / 2 {
assert_eq!(v2[i], config_space[len / 2 + i]);
Expand All @@ -333,10 +368,10 @@ pub(crate) mod tests {
}

// Offset too large to overwrite anything.
d.write_config(len, v1.as_slice());
VirtioDevice::write_config(&mut d, len, v1.as_slice());
assert_eq!(d.cfg.config_space, config_space);

d.write_config(len / 2, v1.as_slice());
VirtioDevice::write_config(&mut d, len / 2, v1.as_slice());
for (i, &value) in config_space.iter().enumerate().take(len) {
if i < len / 2 {
assert_eq!(d.cfg.config_space[i], value);
Expand All @@ -345,6 +380,9 @@ pub(crate) mod tests {
}
}

d.cfg.interrupt_status.fetch_or(1, Ordering::SeqCst);
assert_eq!(VirtioDevice::interrupt_status(&d).load(Ordering::SeqCst), 1);

// Let's test the `WithDriverSelect` auto impl now.
assert_eq!(d.queue_select(), 0);
d.set_queue_select(1);
Expand Down