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.

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

Improve vqueue encapsulation - prerequisite for vhost work #4312

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions src/vmm/src/device_manager/mmio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ mod tests {

use super::*;
use crate::devices::virtio::device::VirtioDevice;
use crate::devices::virtio::queue::Queue;
use crate::devices::virtio::queue::{Queue, QueueIter, QueueIterMut};
use crate::devices::virtio::ActivateError;
use crate::vstate::memory::{GuestAddress, GuestMemoryExtension, GuestMemoryMmap};
use crate::{builder, Vm};
Expand Down Expand Up @@ -535,12 +535,12 @@ mod tests {
0
}

fn queues(&self) -> &[Queue] {
&self.queues
fn queues(&self) -> QueueIter {
self.queues.iter()
}

fn queues_mut(&mut self) -> &mut [Queue] {
&mut self.queues
fn queues_mut(&mut self) -> QueueIterMut {
self.queues.iter_mut()
}

fn queue_events(&self) -> &[EventFd] {
Expand Down
10 changes: 5 additions & 5 deletions src/vmm/src/devices/virtio/balloon/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
use utils::u64_to_usize;

use super::super::device::{DeviceState, VirtioDevice};
use super::super::queue::Queue;
use super::super::{ActivateError, TYPE_BALLOON};
use super::metrics::METRICS;
use super::util::{compact_page_frame_numbers, remove_range};
Expand All @@ -30,6 +29,7 @@
use crate::devices::virtio::balloon::BalloonError;
use crate::devices::virtio::device::{IrqTrigger, IrqType};
use crate::devices::virtio::gen::virtio_blk::VIRTIO_F_VERSION_1;
use crate::devices::virtio::queue::{Queue, QueueIter, QueueIterMut};
use crate::logger::IncMetric;
use crate::vstate::memory::{Address, ByteValued, Bytes, GuestAddress, GuestMemoryMmap};

Expand Down Expand Up @@ -573,12 +573,12 @@
TYPE_BALLOON
}

fn queues(&self) -> &[Queue] {
&self.queues
fn queues(&self) -> QueueIter {
self.queues.iter()
}

fn queues_mut(&mut self) -> &mut [Queue] {
&mut self.queues
fn queues_mut(&mut self) -> QueueIterMut {
self.queues.iter_mut()

Check warning on line 581 in src/vmm/src/devices/virtio/balloon/device.rs

View check run for this annotation

Codecov / codecov/patch

src/vmm/src/devices/virtio/balloon/device.rs#L580-L581

Added lines #L580 - L581 were not covered by tests
}

fn queue_events(&self) -> &[EventFd] {
Expand Down
2 changes: 1 addition & 1 deletion src/vmm/src/devices/virtio/balloon/persist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ mod tests {
assert_eq!(restored_balloon.acked_features, balloon.acked_features);
assert_eq!(restored_balloon.avail_features, balloon.avail_features);
assert_eq!(restored_balloon.config_space, balloon.config_space);
assert_eq!(restored_balloon.queues(), balloon.queues());
assert!(restored_balloon.queues().eq(balloon.queues()));
assert_eq!(
restored_balloon.interrupt_status().load(Ordering::Relaxed),
balloon.interrupt_status().load(Ordering::Relaxed)
Expand Down
11 changes: 6 additions & 5 deletions src/vmm/src/devices/virtio/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ use std::sync::Arc;
use utils::eventfd::EventFd;

use super::mmio::{VIRTIO_MMIO_INT_CONFIG, VIRTIO_MMIO_INT_VRING};
use super::queue::Queue;
use super::ActivateError;
use crate::devices::virtio::queue::{QueueIter, QueueIterMut};
use crate::devices::virtio::AsAny;
use crate::logger::{error, warn};
use crate::vstate::memory::GuestMemoryMmap;
Expand Down Expand Up @@ -111,10 +111,10 @@ pub trait VirtioDevice: AsAny + Send {
fn device_type(&self) -> u32;

/// Returns the device queues.
fn queues(&self) -> &[Queue];
fn queues(&self) -> QueueIter;

/// Returns a mutable reference to the device queues.
fn queues_mut(&mut self) -> &mut [Queue];
fn queues_mut(&mut self) -> QueueIterMut;

/// Returns the device queues event fds.
fn queue_events(&self) -> &[EventFd];
Expand Down Expand Up @@ -190,6 +190,7 @@ impl fmt::Debug for dyn VirtioDevice {
#[cfg(test)]
pub(crate) mod tests {
use super::*;
use crate::devices::virtio::queue::{QueueIter, QueueIterMut};

impl IrqTrigger {
pub fn has_pending_irq(&self, irq_type: IrqType) -> bool {
Expand Down Expand Up @@ -254,11 +255,11 @@ pub(crate) mod tests {
todo!()
}

fn queues(&self) -> &[Queue] {
fn queues(&self) -> QueueIter {
todo!()
}

fn queues_mut(&mut self) -> &mut [Queue] {
fn queues_mut(&mut self) -> QueueIterMut {
todo!()
}

Expand Down
108 changes: 66 additions & 42 deletions src/vmm/src/devices/virtio/mmio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,7 @@
}

fn are_queues_valid(&self) -> bool {
self.locked_device()
.queues()
.iter()
.all(|q| q.is_valid(&self.mem))
self.locked_device().queues().all(|q| q.is_valid(&self.mem))
}

fn with_queue<U, F>(&self, d: U, f: F) -> U
Expand All @@ -111,7 +108,7 @@
match self
.locked_device()
.queues()
.get(self.queue_select as usize)
.nth(self.queue_select as usize)
{
Some(queue) => f(queue),
None => d,
Expand All @@ -122,7 +119,7 @@
if let Some(queue) = self
.locked_device()
.queues_mut()
.get_mut(self.queue_select as usize)
.nth(self.queue_select as usize)
{
f(queue);
true
Expand Down Expand Up @@ -158,7 +155,7 @@
// eventfds, but nothing will happen other than supurious wakeups.
// . Do not reset config_generation and keep it monotonically increasing
for queue in self.locked_device().queues_mut() {
*queue = Queue::new(queue.get_max_size());
*queue = Queue::new(queue.max_size());

Check warning on line 158 in src/vmm/src/devices/virtio/mmio.rs

View check run for this annotation

Codecov / codecov/patch

src/vmm/src/devices/virtio/mmio.rs#L158

Added line #L158 was not covered by tests
}
}

Expand Down Expand Up @@ -243,8 +240,8 @@
}
features
}
0x34 => self.with_queue(0, |q| u32::from(q.get_max_size())),
0x44 => self.with_queue(0, |q| u32::from(q.ready)),
0x34 => self.with_queue(0, |q| u32::from(q.max_size())),
0x44 => self.with_queue(0, |q| u32::from(q.ready())),
0x60 => {
// For vhost-user backed devices we need some additional
// logic to differentiate between `VIRTIO_MMIO_INT_VRING`
Expand Down Expand Up @@ -319,20 +316,20 @@
}
0x24 => self.acked_features_select = v,
0x30 => self.queue_select = v,
0x38 => self.update_queue_field(|q| q.size = (v & 0xffff) as u16),
0x44 => self.update_queue_field(|q| q.ready = v == 1),
0x38 => self.update_queue_field(|q| q.set_size(v as u16)),
0x44 => self.update_queue_field(|q| q.set_ready(v == 1)),
0x64 => {
if self.check_device_status(device_status::DRIVER_OK, 0) {
self.interrupt_status.fetch_and(!v, Ordering::SeqCst);
}
}
0x70 => self.set_device_status(v),
0x80 => self.update_queue_field(|q| lo(&mut q.desc_table, v)),
0x84 => self.update_queue_field(|q| hi(&mut q.desc_table, v)),
0x90 => self.update_queue_field(|q| lo(&mut q.avail_ring, v)),
0x94 => self.update_queue_field(|q| hi(&mut q.avail_ring, v)),
0xa0 => self.update_queue_field(|q| lo(&mut q.used_ring, v)),
0xa4 => self.update_queue_field(|q| hi(&mut q.used_ring, v)),
0x80 => self.update_queue_field(|q| lo(q.desc_table_mut(), v)),
0x84 => self.update_queue_field(|q| hi(q.desc_table_mut(), v)),
0x90 => self.update_queue_field(|q| lo(q.avail_ring_mut(), v)),
0x94 => self.update_queue_field(|q| hi(q.avail_ring_mut(), v)),
0xa0 => self.update_queue_field(|q| lo(q.used_ring_mut(), v)),
0xa4 => self.update_queue_field(|q| hi(q.used_ring_mut(), v)),
_ => {
warn!("unknown virtio mmio register write: 0x{:x}", offset);
}
Expand Down Expand Up @@ -363,6 +360,7 @@
use utils::u64_to_usize;

use super::*;
use crate::devices::virtio::queue::{Queue, QueueIter, QueueIterMut};
use crate::devices::virtio::ActivateError;
use crate::vstate::memory::{GuestMemoryExtension, GuestMemoryMmap};

Expand Down Expand Up @@ -417,12 +415,12 @@
123
}

fn queues(&self) -> &[Queue] {
&self.queues
fn queues(&self) -> QueueIter {
self.queues.iter()
}

fn queues_mut(&mut self) -> &mut [Queue] {
&mut self.queues
fn queues_mut(&mut self) -> QueueIterMut {
self.queues.iter_mut()
}

fn queue_events(&self) -> &[EventFd] {
Expand Down Expand Up @@ -479,18 +477,32 @@
assert!(!d.are_queues_valid());

d.queue_select = 0;
assert_eq!(d.with_queue(0, Queue::get_max_size), 16);
assert!(d.with_queue_mut(|q| q.size = 16));
assert_eq!(d.locked_device().queues()[d.queue_select as usize].size, 16);
assert_eq!(d.with_queue(0, |q| q.max_size()), 16);
assert!(d.with_queue_mut(|q| q.set_size(16)));
assert_eq!(
d.locked_device()
.queues()
.nth(d.queue_select as usize)
.unwrap()
.size(),
16
);

d.queue_select = 1;
assert_eq!(d.with_queue(0, Queue::get_max_size), 32);
assert!(d.with_queue_mut(|q| q.size = 16));
assert_eq!(d.locked_device().queues()[d.queue_select as usize].size, 16);
assert_eq!(d.with_queue(0, |q| q.max_size()), 32);
assert!(d.with_queue_mut(|q| q.set_size(16)));
assert_eq!(
d.locked_device()
.queues()
.nth(d.queue_select as usize)
.unwrap()
.size(),
16
);

d.queue_select = 2;
assert_eq!(d.with_queue(0, Queue::get_max_size), 0);
assert!(!d.with_queue_mut(|q| q.size = 16));
assert_eq!(d.with_queue(0, |q| q.max_size()), 0);
assert!(!d.with_queue_mut(|q| q.set_size(16)));

assert!(!d.are_queues_valid());
}
Expand Down Expand Up @@ -667,42 +679,54 @@
assert_eq!(d.queue_select, 3);

d.queue_select = 0;
assert_eq!(d.locked_device().queues()[0].size, 0);
assert_eq!(d.locked_device().queues().nth(0).unwrap().size(), 0);
write_le_u32(&mut buf[..], 16);
d.bus_write(0x38, &buf[..]);
assert_eq!(d.locked_device().queues()[0].size, 16);
assert_eq!(d.locked_device().queues().nth(0).unwrap().size(), 16);

assert!(!d.locked_device().queues()[0].ready);
assert!(!d.locked_device().queues().nth(0).unwrap().ready());
write_le_u32(&mut buf[..], 1);
d.bus_write(0x44, &buf[..]);
assert!(d.locked_device().queues()[0].ready);
assert!(d.locked_device().queues().nth(0).unwrap().ready());

assert_eq!(d.locked_device().queues()[0].desc_table.0, 0);
assert_eq!(d.locked_device().queues().nth(0).unwrap().desc_table().0, 0);
write_le_u32(&mut buf[..], 123);
d.bus_write(0x80, &buf[..]);
assert_eq!(d.locked_device().queues()[0].desc_table.0, 123);
assert_eq!(
d.locked_device().queues().nth(0).unwrap().desc_table().0,
123
);
d.bus_write(0x84, &buf[..]);
assert_eq!(
d.locked_device().queues()[0].desc_table.0,
d.locked_device().queues().nth(0).unwrap().desc_table().0,
123 + (123 << 32)
);

assert_eq!(d.locked_device().queues()[0].avail_ring.0, 0);
assert_eq!(d.locked_device().queues().nth(0).unwrap().avail_ring().0, 0);
write_le_u32(&mut buf[..], 124);
d.bus_write(0x90, &buf[..]);
assert_eq!(d.locked_device().queues()[0].avail_ring.0, 124);
assert_eq!(
d.locked_device().queues().nth(0).unwrap().avail_ring().0,
124
);
d.bus_write(0x94, &buf[..]);
assert_eq!(
d.locked_device().queues()[0].avail_ring.0,
d.locked_device().queues().nth(0).unwrap().avail_ring().0,
124 + (124 << 32)
);

assert_eq!(d.locked_device().queues()[0].used_ring.0, 0);
assert_eq!(d.locked_device().queues().nth(0).unwrap().used_ring().0, 0);
write_le_u32(&mut buf[..], 125);
d.bus_write(0xa0, &buf[..]);
assert_eq!(d.locked_device().queues()[0].used_ring.0, 125);
assert_eq!(
d.locked_device().queues().nth(0).unwrap().used_ring().0,
125
);
d.bus_write(0xa4, &buf[..]);
assert_eq!(d.locked_device().queues()[0].used_ring.0, 125 + (125 << 32));
assert_eq!(
d.locked_device().queues().nth(0).unwrap().used_ring().0,
125 + (125 << 32)
);

set_device_status(
&mut d,
Expand Down
24 changes: 11 additions & 13 deletions src/vmm/src/devices/virtio/net/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use crate::devices::virtio::net::tap::Tap;
use crate::devices::virtio::net::{
gen, NetError, NetQueue, MAX_BUFFER_SIZE, NET_QUEUE_SIZES, RX_INDEX, TX_INDEX,
};
use crate::devices::virtio::queue::{DescriptorChain, Queue};
use crate::devices::virtio::queue::{DescriptorChain, Queue, QueueIter, QueueIterMut};
use crate::devices::virtio::{ActivateError, TYPE_NET};
use crate::devices::{report_net_event_fail, DeviceError};
use crate::dumbo::pdu::arp::ETH_IPV4_FRAME_LEN;
Expand Down Expand Up @@ -787,12 +787,12 @@ impl VirtioDevice for Net {
TYPE_NET
}

fn queues(&self) -> &[Queue] {
&self.queues
fn queues(&self) -> QueueIter {
self.queues.iter()
}

fn queues_mut(&mut self) -> &mut [Queue] {
&mut self.queues
fn queues_mut(&mut self) -> QueueIterMut {
self.queues.iter_mut()
}

fn queue_events(&self) -> &[EventFd] {
Expand Down Expand Up @@ -845,7 +845,7 @@ impl VirtioDevice for Net {
fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> {
let event_idx = self.has_feature(u64::from(VIRTIO_RING_F_EVENT_IDX));
if event_idx {
for queue in &mut self.queues {
for queue in self.queues_mut() {
queue.enable_notif_suppression();
}
}
Expand Down Expand Up @@ -2007,10 +2007,9 @@ pub mod tests {
let net = th.net.lock().unwrap();

// Test queues count (TX and RX).
let queues = net.queues();
assert_eq!(queues.len(), NET_QUEUE_SIZES.len());
assert_eq!(queues[RX_INDEX].size, th.rxq.size());
assert_eq!(queues[TX_INDEX].size, th.txq.size());
assert_eq!(net.queues().count(), NET_QUEUE_SIZES.len());
assert_eq!(net.queues().nth(RX_INDEX).unwrap().size(), th.rxq.size());
assert_eq!(net.queues().nth(TX_INDEX).unwrap().size(), th.txq.size());

// Test corresponding queues events.
assert_eq!(net.queue_events().len(), NET_QUEUE_SIZES.len());
Expand All @@ -2028,8 +2027,7 @@ pub mod tests {
th.activate_net();

let net = th.net();
let queues = net.queues();
assert!(queues[RX_INDEX].uses_notif_suppression);
assert!(queues[TX_INDEX].uses_notif_suppression);
assert!(net.queues().nth(RX_INDEX).unwrap().uses_notif_suppression());
assert!(net.queues().nth(TX_INDEX).unwrap().uses_notif_suppression());
}
}
Loading
Loading