Skip to content

enhance virtio_queue to better support multi-threading #76

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

Merged
merged 6 commits into from
Oct 15, 2021
Merged

enhance virtio_queue to better support multi-threading #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8c76c8a
queue: split out `struct QueueState`
jiangliu Aug 5, 2021
f0d534f
queue: move go_to_previous_position() to AvailIter
jiangliu Aug 6, 2021
ca9f381
queue: introduce traits queue state operations
jiangliu Aug 5, 2021
3c4170c
queue: add QueueStateSync for multi-threading
jiangliu Aug 5, 2021
be4f85d
queue: use atomic to access queue head field
jiangliu Aug 18, 2021
9b985ef
queue: refine documentation a bit
jiangliu Aug 18, 2021
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 coverage_config_x86_64.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"coverage_score": 90.6,
"coverage_score": 91.0,
"exclude_path": "",
"crate_features": "virtio-blk/backend-stdio"
}
4 changes: 2 additions & 2 deletions crates/virtio-device/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ mod tests {
assert_eq!(d.cfg.device_features & (1 << VIRTIO_F_RING_EVENT_IDX), 0);

for q in d.cfg.queues.iter() {
assert_eq!(q.event_idx_enabled, false);
assert_eq!(q.state.event_idx_enabled, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as a mention, the config in devices should start holding QueueState objects instead of Queuess, but we can apply this change later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, let's cover it by a dedicated MR, so we could keep this MR easy to review:)

}

// Revert status.
Expand All @@ -302,7 +302,7 @@ mod tests {
assert_eq!(d.cfg.device_status, status);

for q in d.cfg.queues.iter() {
assert_eq!(q.event_idx_enabled, true);
assert_eq!(q.state.event_idx_enabled, true);
}
}

Expand Down
54 changes: 22 additions & 32 deletions crates/virtio-device/src/mmio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::convert::TryInto;
use std::sync::atomic::Ordering;

use log::warn;
use vm_memory::{GuestAddress, GuestAddressSpace};
use vm_memory::GuestAddressSpace;

use crate::{status, WithDriverSelect};
use virtio_queue::Queue;
Expand Down Expand Up @@ -51,16 +51,6 @@ where
}
}

// Helper function that rewrites the most significant 4 bytes of the provided `GuestAddress`.
fn set_high(v: &mut GuestAddress, hi: u32) {
*v = (*v & 0xffff_ffff) | (u64::from(hi) << 32)
}

// Helper function that rewrites the least significant 4 bytes of the provided `GuestAddress`.
fn set_low(v: &mut GuestAddress, lo: u32) {
*v = (*v & !0xffff_ffff) | u64::from(lo)
}

/// A common interface for Virtio devices that use the MMIO transport, which also provides a
/// default implementation of read and write operations from/to the device registers and
/// configuration space.
Expand Down Expand Up @@ -99,7 +89,7 @@ pub trait VirtioMmioDevice<M: GuestAddressSpace>: WithDriverSelect<M> {
.into(),
0x44 => self
.selected_queue()
.map(|q| q.ready)
.map(|q| q.ready())
.unwrap_or(false)
.into(),
0x60 => self.interrupt_status().load(Ordering::SeqCst).into(),
Expand Down Expand Up @@ -159,8 +149,8 @@ pub trait VirtioMmioDevice<M: GuestAddressSpace>: WithDriverSelect<M> {
// data type specified by the virtio standard (we simply use `as` conversion
// for now).
0x30 => self.set_queue_select(v as u16),
0x38 => update_queue_field(self, |q| q.size = v as u16),
0x44 => update_queue_field(self, |q| q.ready = v == 1),
0x38 => update_queue_field(self, |q| q.set_size(v as u16)),
0x44 => update_queue_field(self, |q| q.set_ready(v == 1)),
0x50 => self.queue_notify(v),
0x64 => {
if self.check_device_status(status::DRIVER_OK, 0) {
Expand All @@ -169,12 +159,12 @@ pub trait VirtioMmioDevice<M: GuestAddressSpace>: WithDriverSelect<M> {
}
}
0x70 => self.ack_device_status(v as u8),
0x80 => update_queue_field(self, |q| set_low(&mut q.desc_table, v)),
0x84 => update_queue_field(self, |q| set_high(&mut q.desc_table, v)),
0x90 => update_queue_field(self, |q| set_low(&mut q.avail_ring, v)),
0x94 => update_queue_field(self, |q| set_high(&mut q.avail_ring, v)),
0xa0 => update_queue_field(self, |q| set_low(&mut q.used_ring, v)),
0xa4 => update_queue_field(self, |q| set_high(&mut q.used_ring, v)),
0x80 => update_queue_field(self, |q| q.set_desc_table_address(Some(v), None)),
0x84 => update_queue_field(self, |q| q.set_desc_table_address(None, Some(v))),
0x90 => update_queue_field(self, |q| q.set_avail_ring_address(Some(v), None)),
0x94 => update_queue_field(self, |q| q.set_avail_ring_address(None, Some(v))),
0xa0 => update_queue_field(self, |q| q.set_used_ring_address(Some(v), None)),
0xa4 => update_queue_field(self, |q| q.set_used_ring_address(None, Some(v))),
_ => {
warn!("unknown virtio mmio register write: 0x{:x}", offset);
}
Expand Down Expand Up @@ -258,16 +248,16 @@ mod tests {
// The max size for the queue in `Dummy` is 256.
assert_eq!(mmio_read(&d, 0x34), 256);

assert_eq!(d.cfg.queues[0].size, 256);
assert_eq!(d.cfg.queues[0].state.size, 256);
d.write(0x38, &32u32.to_le_bytes());
// Updating the queue field has no effect due to invalid device status.
assert_eq!(d.cfg.queues[0].size, 256);
assert_eq!(d.cfg.queues[0].state.size, 256);

d.cfg.device_status |= status::FEATURES_OK;

// Let's try the update again.
d.write(0x38, &32u32.to_le_bytes());
assert_eq!(d.cfg.queues[0].size, 32);
assert_eq!(d.cfg.queues[0].state.size, 32);

// The queue in `Dummy` is not ready yet.
assert_eq!(mmio_read(&d, 0x44), 0);
Expand All @@ -281,23 +271,23 @@ mod tests {
d.write(0x50, &2u32.to_le_bytes());
assert_eq!(d.last_queue_notify, 2);

assert_eq!(d.cfg.queues[0].desc_table.0, 0);
assert_eq!(d.cfg.queues[0].state.desc_table.0, 0);
d.write(0x80, &1u32.to_le_bytes());
assert_eq!(d.cfg.queues[0].desc_table.0, 1);
assert_eq!(d.cfg.queues[0].state.desc_table.0, 1);
d.write(0x84, &2u32.to_le_bytes());
assert_eq!(d.cfg.queues[0].desc_table.0, (2 << 32) + 1);
assert_eq!(d.cfg.queues[0].state.desc_table.0, (2 << 32) + 1);

assert_eq!(d.cfg.queues[0].avail_ring.0, 0);
assert_eq!(d.cfg.queues[0].state.avail_ring.0, 0);
d.write(0x90, &1u32.to_le_bytes());
assert_eq!(d.cfg.queues[0].avail_ring.0, 1);
assert_eq!(d.cfg.queues[0].state.avail_ring.0, 1);
d.write(0x94, &2u32.to_le_bytes());
assert_eq!(d.cfg.queues[0].avail_ring.0, (2 << 32) + 1);
assert_eq!(d.cfg.queues[0].state.avail_ring.0, (2 << 32) + 1);

assert_eq!(d.cfg.queues[0].used_ring.0, 0);
assert_eq!(d.cfg.queues[0].state.used_ring.0, 0);
d.write(0xa0, &1u32.to_le_bytes());
assert_eq!(d.cfg.queues[0].used_ring.0, 1);
assert_eq!(d.cfg.queues[0].state.used_ring.0, 1);
d.write(0xa4, &2u32.to_le_bytes());
assert_eq!(d.cfg.queues[0].used_ring.0, (2 << 32) + 1);
assert_eq!(d.cfg.queues[0].state.used_ring.0, (2 << 32) + 1);

// Let's select a non-existent queue.
d.write(0x30, &1u32.to_le_bytes());
Expand Down
Loading