From a78ec87e1349ecb0e0533096e53055ad652ae10e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kornel=20Lesi=C5=84ski?= Date: Mon, 11 Dec 2023 15:22:11 +0100 Subject: [PATCH] vqueue encapsulation: make virtio.Queue fields private, expose as functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Kornel Lesiński --- src/vmm/src/devices/virtio/mmio.rs | 73 ++++++++++-------- src/vmm/src/devices/virtio/net/device.rs | 8 +- src/vmm/src/devices/virtio/persist.rs | 14 ++-- src/vmm/src/devices/virtio/queue.rs | 96 ++++++++++++++++++------ src/vmm/src/devices/virtio/test_utils.rs | 10 +-- src/vmm/src/devices/virtio/vhost_user.rs | 14 ++-- 6 files changed, 136 insertions(+), 79 deletions(-) diff --git a/src/vmm/src/devices/virtio/mmio.rs b/src/vmm/src/devices/virtio/mmio.rs index cd9c1a5e033..1d53210ab9f 100644 --- a/src/vmm/src/devices/virtio/mmio.rs +++ b/src/vmm/src/devices/virtio/mmio.rs @@ -158,7 +158,7 @@ impl MmioTransport { // 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()); } } @@ -243,8 +243,8 @@ impl MmioTransport { } 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` @@ -319,20 +319,20 @@ impl MmioTransport { } 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); } @@ -479,18 +479,24 @@ pub(crate) mod tests { 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()[d.queue_select as usize].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()[d.queue_select as usize].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()); } @@ -667,42 +673,45 @@ pub(crate) mod tests { 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()[0].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()[0].size(), 16); - assert!(!d.locked_device().queues()[0].ready); + assert!(!d.locked_device().queues()[0].ready()); write_le_u32(&mut buf[..], 1); d.bus_write(0x44, &buf[..]); - assert!(d.locked_device().queues()[0].ready); + assert!(d.locked_device().queues()[0].ready()); - assert_eq!(d.locked_device().queues()[0].desc_table.0, 0); + assert_eq!(d.locked_device().queues()[0].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()[0].desc_table().0, 123); d.bus_write(0x84, &buf[..]); assert_eq!( - d.locked_device().queues()[0].desc_table.0, + d.locked_device().queues()[0].desc_table().0, 123 + (123 << 32) ); - assert_eq!(d.locked_device().queues()[0].avail_ring.0, 0); + assert_eq!(d.locked_device().queues()[0].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()[0].avail_ring().0, 124); d.bus_write(0x94, &buf[..]); assert_eq!( - d.locked_device().queues()[0].avail_ring.0, + d.locked_device().queues()[0].avail_ring().0, 124 + (124 << 32) ); - assert_eq!(d.locked_device().queues()[0].used_ring.0, 0); + assert_eq!(d.locked_device().queues()[0].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()[0].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()[0].used_ring().0, + 125 + (125 << 32) + ); set_device_status( &mut d, diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index cc11ad42b23..798134f0218 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -2009,8 +2009,8 @@ pub mod tests { // 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!(queues[RX_INDEX].size(), th.rxq.size()); + assert_eq!(queues[TX_INDEX].size(), th.txq.size()); // Test corresponding queues events. assert_eq!(net.queue_events().len(), NET_QUEUE_SIZES.len()); @@ -2029,7 +2029,7 @@ pub mod tests { let net = th.net(); let queues = net.queues(); - assert!(queues[RX_INDEX].uses_notif_suppression); - assert!(queues[TX_INDEX].uses_notif_suppression); + assert!(queues[RX_INDEX].uses_notif_suppression()); + assert!(queues[TX_INDEX].uses_notif_suppression()); } } diff --git a/src/vmm/src/devices/virtio/persist.rs b/src/vmm/src/devices/virtio/persist.rs index dba9ba1bc41..31a83513dba 100644 --- a/src/vmm/src/devices/virtio/persist.rs +++ b/src/vmm/src/devices/virtio/persist.rs @@ -61,12 +61,12 @@ impl Persist<'_> for Queue { fn save(&self) -> Self::State { QueueState { - max_size: self.max_size, - size: self.size, - ready: self.ready, - desc_table: self.desc_table.0, - avail_ring: self.avail_ring.0, - used_ring: self.used_ring.0, + max_size: self.max_size(), + size: self.size(), + ready: self.ready(), + desc_table: self.desc_table().0, + avail_ring: self.avail_ring().0, + used_ring: self.used_ring().0, next_avail: self.next_avail, next_used: self.next_used, num_added: self.num_added, @@ -161,7 +161,7 @@ impl VirtioDeviceState { for q in &queues { // Sanity check queue size and queue max size. - if q.max_size != expected_queue_max_size || q.size > expected_queue_max_size { + if q.max_size() != expected_queue_max_size || q.size() > expected_queue_max_size { return Err(PersistError::InvalidInput); } // Snapshot can happen at any time, including during device configuration/activation diff --git a/src/vmm/src/devices/virtio/queue.rs b/src/vmm/src/devices/virtio/queue.rs index 45cf6349f7e..0be48b07449 100644 --- a/src/vmm/src/devices/virtio/queue.rs +++ b/src/vmm/src/devices/virtio/queue.rs @@ -183,19 +183,19 @@ pub struct Queue { pub(crate) max_size: u16, /// The queue size in elements the driver selected - pub size: u16, + pub(crate) size: u16, /// Indicates if the queue is finished with configuration - pub ready: bool, + pub(crate) ready: bool, /// Guest physical address of the descriptor table - pub desc_table: GuestAddress, + pub(crate) desc_table: GuestAddress, /// Guest physical address of the available ring - pub avail_ring: GuestAddress, + pub(crate) avail_ring: GuestAddress, /// Guest physical address of the used ring - pub used_ring: GuestAddress, + pub(crate) used_ring: GuestAddress, pub(crate) next_avail: Wrapping, pub(crate) next_used: Wrapping, @@ -225,10 +225,54 @@ impl Queue { } /// Maximum size of the queue. - pub fn get_max_size(&self) -> u16 { + pub fn max_size(&self) -> u16 { self.max_size } + pub fn size(&self) -> u16 { + self.size + } + + pub fn ready(&self) -> bool { + self.ready + } + + pub fn desc_table(&self) -> GuestAddress { + self.desc_table + } + + pub fn avail_ring(&self) -> GuestAddress { + self.avail_ring + } + + pub fn used_ring(&self) -> GuestAddress { + self.used_ring + } + + pub fn desc_table_mut(&mut self) -> &mut GuestAddress { + &mut self.desc_table + } + + pub fn avail_ring_mut(&mut self) -> &mut GuestAddress { + &mut self.avail_ring + } + + pub fn used_ring_mut(&mut self) -> &mut GuestAddress { + &mut self.used_ring + } + + pub fn set_max_size(&mut self, val: u16) { + self.max_size = val; + } + + pub fn set_size(&mut self, val: u16) { + self.size = val; + } + + pub fn set_ready(&mut self, val: bool) { + self.ready = val; + } + /// Return the actual size of the queue, as the driver may not set up a /// queue as big as the device allows. pub fn actual_size(&self) -> u16 { @@ -347,7 +391,7 @@ impl Queue { &mut self, mem: &'b M, ) -> Option> { - if !self.uses_notif_suppression { + if !self.uses_notif_suppression() { return self.pop(mem); } @@ -499,7 +543,7 @@ impl Queue { // If the device doesn't use notification suppression, we'll continue to get notifications // no matter what. - if !self.uses_notif_suppression { + if !self.uses_notif_suppression() { return true; } @@ -533,6 +577,10 @@ impl Queue { self.uses_notif_suppression = true; } + pub fn uses_notif_suppression(&self) -> bool { + self.uses_notif_suppression + } + /// Check if we need to kick the guest. /// /// Please note this method has side effects: once it returns `true`, it considers the @@ -544,7 +592,7 @@ impl Queue { debug_assert!(self.is_layout_valid(mem)); // If the device doesn't use notification suppression, always return true - if !self.uses_notif_suppression { + if !self.uses_notif_suppression() { return true; } @@ -959,8 +1007,8 @@ mod verification { fn verify_actual_size() { let ProofContext(queue, _) = kani::any(); - assert!(queue.actual_size() <= queue.get_max_size()); - assert!(queue.actual_size() <= queue.size); + assert!(queue.actual_size() <= queue.max_size()); + assert!(queue.actual_size() <= queue.size()); } #[kani::proof] @@ -1075,7 +1123,7 @@ mod tests { impl Queue { fn avail_event(&self, mem: &GuestMemoryMmap) -> u16 { let avail_event_addr = self - .used_ring + .used_ring() .unchecked_add(u64::from(4 + 8 * self.actual_size())); mem.read_obj::(avail_event_addr).unwrap() @@ -1171,7 +1219,7 @@ mod tests { assert!(!q.is_valid(m)); m.write_obj::(5_u16, q.avail_ring.unchecked_add(2)) .unwrap(); - q.max_size = 2; + q.set_max_size(2); assert!(!q.is_valid(m)); // reset dirtied values @@ -1182,23 +1230,23 @@ mod tests { // or if the various addresses are off - q.desc_table = GuestAddress(0xffff_ffff); + *q.desc_table_mut() = GuestAddress(0xffff_ffff); assert!(!q.is_valid(m)); - q.desc_table = GuestAddress(0x1001); + *q.desc_table_mut() = GuestAddress(0x1001); assert!(!q.is_valid(m)); - q.desc_table = vq.dtable_start(); + *q.desc_table_mut() = vq.dtable_start(); - q.avail_ring = GuestAddress(0xffff_ffff); + *q.avail_ring_mut() = GuestAddress(0xffff_ffff); assert!(!q.is_valid(m)); - q.avail_ring = GuestAddress(0x1001); + *q.avail_ring_mut() = GuestAddress(0x1001); assert!(!q.is_valid(m)); - q.avail_ring = vq.avail_start(); + *q.avail_ring_mut() = vq.avail_start(); - q.used_ring = GuestAddress(0xffff_ffff); + *q.used_ring_mut() = GuestAddress(0xffff_ffff); assert!(!q.is_valid(m)); - q.used_ring = GuestAddress(0x1001); + *q.used_ring_mut() = GuestAddress(0x1001); assert!(!q.is_valid(m)); - q.used_ring = vq.used_start(); + *q.used_ring_mut() = vq.used_start(); } #[test] @@ -1351,7 +1399,7 @@ mod tests { let vq = VirtQueue::new(GuestAddress(0), m, 4); let mut q = vq.create_queue(); - q.uses_notif_suppression = true; + q.enable_notif_suppression(); // Create 1 descriptor chain of 4. for j in 0..4 { @@ -1476,7 +1524,7 @@ mod tests { let vq = VirtQueue::new(GuestAddress(0), m, 16); let mut q = vq.create_queue(); - q.ready = true; + q.set_ready(true); // We create a simple descriptor chain vq.dtable[0].set(0x1000_u64, 0x1000, 0, 0); diff --git a/src/vmm/src/devices/virtio/test_utils.rs b/src/vmm/src/devices/virtio/test_utils.rs index 96971574c3d..7b96eb62e2e 100644 --- a/src/vmm/src/devices/virtio/test_utils.rs +++ b/src/vmm/src/devices/virtio/test_utils.rs @@ -294,11 +294,11 @@ impl<'a> VirtQueue<'a> { pub fn create_queue(&self) -> Queue { let mut q = Queue::new(self.size()); - q.size = self.size(); - q.ready = true; - q.desc_table = self.dtable_start(); - q.avail_ring = self.avail_start(); - q.used_ring = self.used_start(); + q.set_size(self.size()); + q.set_ready(true); + *q.desc_table_mut() = self.dtable_start(); + *q.avail_ring_mut() = self.avail_start(); + *q.used_ring_mut() = self.used_start(); q } diff --git a/src/vmm/src/devices/virtio/vhost_user.rs b/src/vmm/src/devices/virtio/vhost_user.rs index bdb12015fcd..a6a1af8b68d 100644 --- a/src/vmm/src/devices/virtio/vhost_user.rs +++ b/src/vmm/src/devices/virtio/vhost_user.rs @@ -417,18 +417,18 @@ impl VhostUserHandleImpl { for (queue_index, queue, queue_evt) in queues.iter() { let config_data = VringConfigData { - queue_max_size: queue.get_max_size(), + queue_max_size: queue.max_size(), queue_size: queue.actual_size(), flags: 0u32, desc_table_addr: mem - .get_host_address(queue.desc_table) + .get_host_address(queue.desc_table()) .map_err(VhostUserError::DescriptorTableAddress)? as u64, used_ring_addr: mem - .get_host_address(queue.used_ring) + .get_host_address(queue.used_ring()) .map_err(VhostUserError::UsedAddress)? as u64, avail_ring_addr: mem - .get_host_address(queue.avail_ring) + .get_host_address(queue.avail_ring()) .map_err(VhostUserError::AvailAddress)? as u64, log_addr: None, }; @@ -910,9 +910,9 @@ mod tests { queue_max_size: 69, queue_size: 0, flags: 0, - desc_table_addr: guest_memory.get_host_address(queue.desc_table).unwrap() as u64, - used_ring_addr: guest_memory.get_host_address(queue.used_ring).unwrap() as u64, - avail_ring_addr: guest_memory.get_host_address(queue.avail_ring).unwrap() as u64, + desc_table_addr: guest_memory.get_host_address(queue.desc_table()).unwrap() as u64, + used_ring_addr: guest_memory.get_host_address(queue.used_ring()).unwrap() as u64, + avail_ring_addr: guest_memory.get_host_address(queue.avail_ring()).unwrap() as u64, log_addr: None, }, base: queue.avail_idx(&guest_memory).0,