From 14eddf72b4f4ce9b74e2a50608c8fe0bde1da521 Mon Sep 17 00:00:00 2001 From: Cathy Zhang Date: Thu, 23 Jan 2020 18:14:38 +0800 Subject: [PATCH] vm-virtio: Simplify virtio feature handling Remove duplicated code across the different devices by handling the virtio feature pages in VirtioDevice itself rather than in the backends. This works as no virtio devices use feature bits beyond 64-bits. Signed-off-by: Cathy Zhang --- vm-virtio/src/block.rs | 25 ++---------- vm-virtio/src/console.rs | 25 ++---------- vm-virtio/src/device.rs | 9 +++-- vm-virtio/src/iommu.rs | 25 ++---------- vm-virtio/src/net.rs | 25 ++---------- vm-virtio/src/pmem.rs | 25 ++---------- vm-virtio/src/rng.rs | 25 ++---------- vm-virtio/src/transport/mmio.rs | 26 +++++++++---- vm-virtio/src/transport/pci_common_config.rs | 11 +++--- vm-virtio/src/vhost_user/blk.rs | 23 ++--------- vm-virtio/src/vhost_user/fs.rs | 25 ++---------- vm-virtio/src/vhost_user/net.rs | 23 ++--------- vm-virtio/src/vsock/device.rs | 40 +++++--------------- 13 files changed, 76 insertions(+), 231 deletions(-) diff --git a/vm-virtio/src/block.rs b/vm-virtio/src/block.rs index de6169294..58b1df139 100755 --- a/vm-virtio/src/block.rs +++ b/vm-virtio/src/block.rs @@ -893,29 +893,12 @@ impl VirtioDevice for Block { self.queue_size.as_slice() } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page."); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page."); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/console.rs b/vm-virtio/src/console.rs index 87270c95d..e08f1f6a2 100755 --- a/vm-virtio/src/console.rs +++ b/vm-virtio/src/console.rs @@ -417,29 +417,12 @@ impl VirtioDevice for Console { QUEUE_SIZES } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page."); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page."); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/device.rs b/vm-virtio/src/device.rs index 7a1fa0050..1a372eea4 100644 --- a/vm-virtio/src/device.rs +++ b/vm-virtio/src/device.rs @@ -62,14 +62,15 @@ pub trait VirtioDevice: Send { /// The maximum size of each queue that this device supports. fn queue_max_sizes(&self) -> &[u16]; - /// The set of feature bits shifted by `page * 32`. - fn features(&self, page: u32) -> u32 { - let _ = page; + /// The set of feature bits that this device supports. + fn features(&self) -> u64 { 0 } /// Acknowledges that this set of features should be enabled. - fn ack_features(&mut self, page: u32, value: u32); + fn ack_features(&mut self, value: u64) { + let _ = value; + } /// Reads this device configuration space at `offset`. fn read_config(&self, offset: u64, data: &mut [u8]); diff --git a/vm-virtio/src/iommu.rs b/vm-virtio/src/iommu.rs index e40ef39b8..6343731d0 100644 --- a/vm-virtio/src/iommu.rs +++ b/vm-virtio/src/iommu.rs @@ -897,29 +897,12 @@ impl VirtioDevice for Iommu { QUEUE_SIZES } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page."); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page."); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/net.rs b/vm-virtio/src/net.rs index c03a245ae..c0c6d2dd2 100644 --- a/vm-virtio/src/net.rs +++ b/vm-virtio/src/net.rs @@ -388,29 +388,12 @@ impl VirtioDevice for Net { &self.queue_size.as_slice() } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page: {}", page); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page: {}", page); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/pmem.rs b/vm-virtio/src/pmem.rs index 63974ab12..f09e48158 100644 --- a/vm-virtio/src/pmem.rs +++ b/vm-virtio/src/pmem.rs @@ -368,29 +368,12 @@ impl VirtioDevice for Pmem { QUEUE_SIZES } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page."); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page."); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/rng.rs b/vm-virtio/src/rng.rs index 0b5727eb5..4c4062bea 100755 --- a/vm-virtio/src/rng.rs +++ b/vm-virtio/src/rng.rs @@ -230,29 +230,12 @@ impl VirtioDevice for Rng { QUEUE_SIZES } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page."); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page."); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/transport/mmio.rs b/vm-virtio/src/transport/mmio.rs index 993afe2cb..259142faf 100644 --- a/vm-virtio/src/transport/mmio.rs +++ b/vm-virtio/src/transport/mmio.rs @@ -189,8 +189,12 @@ impl BusDevice for MmioDevice { 0x08 => self.device.lock().unwrap().device_type(), 0x0c => VENDOR_ID, // vendor id 0x10 => { - self.device.lock().unwrap().features(self.features_select) - | if self.features_select == 1 { 0x1 } else { 0x0 } + if self.features_select < 2 { + (self.device.lock().unwrap().features() >> (self.features_select * 32)) + as u32 + } else { + 0 + } } 0x34 => self.with_queue(0, |q| u32::from(q.get_max_size())), 0x44 => self.with_queue(0, |q| q.ready as u32), @@ -234,11 +238,19 @@ impl BusDevice for MmioDevice { let v = LittleEndian::read_u32(data); match offset { 0x14 => self.features_select = v, - 0x20 => self - .device - .lock() - .unwrap() - .ack_features(self.acked_features_select, v), + 0x20 => { + if self.acked_features_select < 2 { + self.device + .lock() + .unwrap() + .ack_features(u64::from(v) << (self.acked_features_select * 32)); + } else { + warn!( + "invalid ack_features (page {}, value 0x{:x})", + self.acked_features_select, v + ); + } + } 0x24 => self.acked_features_select = v, 0x30 => self.queue_select = v, 0x38 => mut_q = self.with_queue_mut(|q| q.size = v as u16), diff --git a/vm-virtio/src/transport/pci_common_config.rs b/vm-virtio/src/transport/pci_common_config.rs index d6cf39995..16e88ed92 100644 --- a/vm-virtio/src/transport/pci_common_config.rs +++ b/vm-virtio/src/transport/pci_common_config.rs @@ -164,7 +164,7 @@ impl VirtioPciCommonConfig { // Only 64 bits of features (2 pages) are defined for now, so limit // device_feature_select to avoid shifting by 64 or more bits. if self.device_feature_select < 2 { - locked_device.features(self.device_feature_select) + (locked_device.features() >> (self.device_feature_select * 32)) as u32 } else { 0 } @@ -199,7 +199,8 @@ impl VirtioPciCommonConfig { 0x0c => { if self.driver_feature_select < 2 { let mut locked_device = device.lock().unwrap(); - locked_device.ack_features(self.driver_feature_select, value); + locked_device + .ack_features(u64::from(value) << (self.driver_feature_select * 32)); } else { warn!( "invalid ack_features (page {}, value 0x{:x})", @@ -280,11 +281,11 @@ mod tests { Ok(()) } - fn features(&self, _page: u32) -> u32 { - DUMMY_FEATURES as u32 + fn features(&self) -> u64 { + DUMMY_FEATURES } - fn ack_features(&mut self, _page: u32, _value: u32) {} + fn ack_features(&mut self, _value: u64) {} fn read_config(&self, _offset: u64, _data: &mut [u8]) {} diff --git a/vm-virtio/src/vhost_user/blk.rs b/vm-virtio/src/vhost_user/blk.rs index 080045ce7..d53a21ac9 100644 --- a/vm-virtio/src/vhost_user/blk.rs +++ b/vm-virtio/src/vhost_user/blk.rs @@ -171,27 +171,12 @@ impl VirtioDevice for Blk { &self.queue_sizes } - fn features(&self, page: u32) -> u32 { - match page { - 0 => self.avail_features as u32, - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page: {}", page); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page: {}", page); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/vhost_user/fs.rs b/vm-virtio/src/vhost_user/fs.rs index 04099ff7a..28cd7a730 100644 --- a/vm-virtio/src/vhost_user/fs.rs +++ b/vm-virtio/src/vhost_user/fs.rs @@ -284,29 +284,12 @@ impl VirtioDevice for Fs { &self.queue_sizes.as_slice() } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("fs: Received request for unknown features page: {}", page); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("fs: Cannot acknowledge unknown features page: {}", page); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/vhost_user/net.rs b/vm-virtio/src/vhost_user/net.rs index 5fc730d8f..954db0865 100644 --- a/vm-virtio/src/vhost_user/net.rs +++ b/vm-virtio/src/vhost_user/net.rs @@ -177,27 +177,12 @@ impl VirtioDevice for Net { &self.queue_sizes } - fn features(&self, page: u32) -> u32 { - match page { - 0 => self.avail_features as u32, - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page: {}", page); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page: {}", page); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { diff --git a/vm-virtio/src/vsock/device.rs b/vm-virtio/src/vsock/device.rs index a84daed3d..634a45915 100644 --- a/vm-virtio/src/vsock/device.rs +++ b/vm-virtio/src/vsock/device.rs @@ -439,29 +439,12 @@ where QUEUE_SIZES } - fn features(&self, page: u32) -> u32 { - match page { - // Get the lower 32-bits of the features bitfield. - 0 => self.avail_features as u32, - // Get the upper 32-bits of the features bitfield. - 1 => (self.avail_features >> 32) as u32, - _ => { - warn!("Received request for unknown features page."); - 0u32 - } - } + fn features(&self) -> u64 { + self.avail_features } - fn ack_features(&mut self, page: u32, value: u32) { - let mut v = match page { - 0 => u64::from(value), - 1 => u64::from(value) << 32, - _ => { - warn!("Cannot acknowledge unknown features page."); - 0u64 - } - }; - + fn ack_features(&mut self, value: u64) { + let mut v = value; // Check if the guest is ACK'ing a feature that we didn't claim to have. let unrequested_features = v & !self.avail_features; if unrequested_features != 0 { @@ -619,18 +602,15 @@ mod tests { VirtioDeviceType::TYPE_VSOCK as u32 ); assert_eq!(ctx.device.queue_max_sizes(), QUEUE_SIZES); - assert_eq!(ctx.device.features(0), device_pages[0]); - assert_eq!(ctx.device.features(1), device_pages[1]); - assert_eq!(ctx.device.features(2), 0); + assert_eq!((ctx.device.features() >> (0 * 32)) as u32, device_pages[0]); + assert_eq!((ctx.device.features() >> (1 * 32)) as u32, device_pages[1]); // Ack device features, page 0. - ctx.device.ack_features(0, driver_pages[0]); + ctx.device + .ack_features(u64::from(driver_pages[0]) << (0 * 32)); // Ack device features, page 1. - ctx.device.ack_features(1, driver_pages[1]); - // Ack some bogus page (i.e. 2). This should have no side effect. - ctx.device.ack_features(2, 0); - // Attempt to un-ack the first feature page. This should have no side effect. - ctx.device.ack_features(0, !driver_pages[0]); + ctx.device + .ack_features(u64::from(driver_pages[1]) << (1 * 32)); // Check that no side effect are present, and that the acked features are exactly the same // as the device features. assert_eq!(ctx.device.acked_features, device_features & driver_features);