devices: convert virtio features to a u64

The virtio specification only defines feature bits in the 0-63 range
currently, so we can represent the features as a u64.  The Linux kernel
makes the same simplifying assumption, and very few features have been
defined beyond the first 32 bits, so this is probably safe for a while.

This allows the device models to be simplified, since they no longer
need to deal with the features paging mechanism (it is handled by the
generic virtio transport code).

BUG=None
TEST=build_test; boot termina on kevin

Change-Id: I6fd86907b2bdf494466c205e85072ebfeb7f5b73
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1313012
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>

10 files changed, 63 insertions, 150 deletions

diff --git a/devices/src/virtio/balloon.rs b/devices/src/virtio/balloon.rs
index cc6a7e6..ffb7aab 100644
--- a/devices/src/virtio/balloon.rs
+++ b/devices/src/virtio/balloon.rs
@@ -220,7 +220,7 @@ impl Worker {
 pub struct Balloon {
     command_socket: Option<UnixDatagram>,
     config: Arc<BalloonConfig>,
-    features: u32,
+    features: u64,
     kill_evt: Option<EventFd>,
 }
 
@@ -295,18 +295,12 @@ impl VirtioDevice for Balloon {
             .store(new_actual as usize, Ordering::Relaxed);
     }
 
-    fn features(&self, page: u32) -> u32 {
-        match page {
-            0 => 1 << VIRTIO_BALLOON_F_MUST_TELL_HOST | 1 << VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
-            _ => 0u32,
-        }
+    fn features(&self) -> u64 {
+        1 << VIRTIO_BALLOON_F_MUST_TELL_HOST | 1 << VIRTIO_BALLOON_F_DEFLATE_ON_OOM
     }
 
-    fn ack_features(&mut self, page: u32, value: u32) {
-        match page {
-            0 => self.features = self.features & value,
-            _ => (),
-        };
+    fn ack_features(&mut self, value: u64) {
+        self.features = self.features & value;
     }
 
     fn activate(
diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs
index c1e7be8..f2ddb2c 100644
--- a/devices/src/virtio/block.rs
+++ b/devices/src/virtio/block.rs
@@ -686,12 +686,8 @@ impl<T: 'static + AsRawFd + DiskFile + Send> VirtioDevice for Block<T> {
         keep_fds
     }
 
-    fn features(&self, page: u32) -> u32 {
-        match page {
-            0 => self.avail_features as u32,
-            1 => (self.avail_features >> 32) as u32,
-            _ => 0,
-        }
+    fn features(&self) -> u64 {
+        self.avail_features
     }
 
     fn device_type(&self) -> u32 {
@@ -803,7 +799,7 @@ mod tests {
             let b = Block::new(f, false).unwrap();
             // writable device should set VIRTIO_BLK_F_FLUSH + VIRTIO_BLK_F_DISCARD
             // + VIRTIO_BLK_F_WRITE_ZEROES
-            assert_eq!(0x6200, b.features(0));
+            assert_eq!(0x6200, b.features());
         }
 
         // read-only block device
@@ -811,7 +807,7 @@ mod tests {
             let f = File::create(&path).unwrap();
             let b = Block::new(f, true).unwrap();
             // read-only device should set VIRTIO_BLK_F_FLUSH and VIRTIO_BLK_F_RO
-            assert_eq!(0x220, b.features(0));
+            assert_eq!(0x220, b.features());
         }
     }
 }
diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs
index 5ef90c9..5c1806b 100644
--- a/devices/src/virtio/gpu/mod.rs
+++ b/devices/src/virtio/gpu/mod.rs
@@ -632,17 +632,11 @@ impl VirtioDevice for Gpu {
         QUEUE_SIZES
     }
 
-    fn features(&self, page: u32) -> u32 {
-        let avail_features: u64 = 1 << VIRTIO_GPU_F_VIRGL | 1 << VIRTIO_F_VERSION_1;
-        match page {
-            0 => avail_features as u32,
-            1 => (avail_features >> 32) as u32,
-            _ => 0,
-        }
+    fn features(&self) -> u64 {
+        1 << VIRTIO_GPU_F_VIRGL | 1 << VIRTIO_F_VERSION_1
     }
 
-    fn ack_features(&mut self, page: u32, value: u32) {
-        let _ = page;
+    fn ack_features(&mut self, value: u64) {
         let _ = value;
     }
 
diff --git a/devices/src/virtio/net.rs b/devices/src/virtio/net.rs
index 97feb4c..c127946 100644
--- a/devices/src/virtio/net.rs
+++ b/devices/src/virtio/net.rs
@@ -399,29 +399,12 @@ where
         QUEUE_SIZES
     }
 
-    fn features(&self, page: u32) -> u32 {
-        match page {
-            0 => self.avail_features as u32,
-            1 => (self.avail_features >> 32) as u32,
-            _ => {
-                warn!("net: virtio net got request for 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 => value as u64,
-            1 => (value as u64) << 32,
-            _ => {
-                warn!(
-                    "net: virtio net device cannot ack unknown feature 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;
diff --git a/devices/src/virtio/p9.rs b/devices/src/virtio/p9.rs
index 2237b0d..1984096 100644
--- a/devices/src/virtio/p9.rs
+++ b/devices/src/virtio/p9.rs
@@ -357,34 +357,17 @@ impl VirtioDevice for P9 {
         QUEUE_SIZES
     }
 
-    fn features(&self, page: u32) -> u32 {
-        match page {
-            0 => self.avail_features as u32,
-            1 => (self.avail_features >> 32) as u32,
-            _ => {
-                warn!("virtio_9p got request for 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 => value as u64,
-            1 => (value as u64) << 32,
-            _ => {
-                warn!("virtio_9p device cannot ack unknown feature 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 {
-            warn!(
-                "virtio_9p got unknown feature ack: {:x}, page = {}, value = {:x}",
-                v, page, value
-            );
+            warn!("virtio_9p got unknown feature ack: {:x}", v);
 
             // Don't count these features as acked.
             v &= !unrequested_features;
diff --git a/devices/src/virtio/vhost/net.rs b/devices/src/virtio/vhost/net.rs
index 96c8a27..d95a7db 100644
--- a/devices/src/virtio/vhost/net.rs
+++ b/devices/src/virtio/vhost/net.rs
@@ -141,29 +141,12 @@ where
         QUEUE_SIZES
     }
 
-    fn features(&self, page: u32) -> u32 {
-        match page {
-            0 => self.avail_features as u32,
-            1 => (self.avail_features >> 32) as u32,
-            _ => {
-                warn!("net: virtio net got request for 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 => value as u64,
-            1 => (value as u64) << 32,
-            _ => {
-                warn!(
-                    "net: virtio net device cannot ack unknown feature 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;
@@ -272,17 +255,15 @@ pub mod tests {
     #[test]
     fn features() {
         let net = create_net_common();
-        assert_eq!(net.features(0), 822135939);
-        assert_eq!(net.features(1), 1);
-        assert_eq!(net.features(2), 0);
+        assert_eq!(net.features(), 5117103235);
     }
 
     #[test]
     fn ack_features() {
         let mut net = create_net_common();
         // Just testing that we don't panic, for now
-        net.ack_features(0, 1);
-        net.ack_features(1, 1);
+        net.ack_features(1);
+        net.ack_features(1 << 32);
     }
 
     #[test]
diff --git a/devices/src/virtio/vhost/vsock.rs b/devices/src/virtio/vhost/vsock.rs
index 31dd61e..4b8fe85 100644
--- a/devices/src/virtio/vhost/vsock.rs
+++ b/devices/src/virtio/vhost/vsock.rs
@@ -111,20 +111,8 @@ impl VirtioDevice for Vsock {
         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!(
-                    "vsock: virtio-vsock got request for features page: {}",
-                    page
-                );
-                0u32
-            }
-        }
+    fn features(&self) -> u64 {
+        self.avail_features
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {
@@ -142,18 +130,8 @@ impl VirtioDevice for Vsock {
         }
     }
 
-    fn ack_features(&mut self, page: u32, value: u32) {
-        let mut v = match page {
-            0 => value as u64,
-            1 => (value as u64) << 32,
-            _ => {
-                warn!(
-                    "vsock: virtio-vsock device cannot ack unknown feature 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;
@@ -238,16 +216,16 @@ mod tests {
         assert_eq!(acked_features, vsock.acked_features());
 
         acked_features |= 1 << 2;
-        vsock.ack_features(0, (acked_features & 0xffffffff) as u32);
+        vsock.ack_features(acked_features);
         assert_eq!(acked_features, vsock.acked_features());
 
         acked_features |= 1 << 49;
-        vsock.ack_features(1, (acked_features >> 32) as u32);
+        vsock.ack_features(acked_features);
         assert_eq!(acked_features, vsock.acked_features());
 
         acked_features |= 1 << 60;
         unavailable_features |= 1 << 60;
-        vsock.ack_features(1, (acked_features >> 32) as u32);
+        vsock.ack_features(acked_features);
         assert_eq!(
             acked_features & !unavailable_features,
             vsock.acked_features()
@@ -255,7 +233,7 @@ mod tests {
 
         acked_features |= 1 << 1;
         unavailable_features |= 1 << 1;
-        vsock.ack_features(0, (acked_features & 0xffffffff) as u32);
+        vsock.ack_features(acked_features);
         assert_eq!(
             acked_features & !unavailable_features,
             vsock.acked_features()
@@ -290,9 +268,6 @@ mod tests {
         let features: u64 = 0xfc195ae8db88cff9;
 
         let vsock = Vsock::new_for_testing(cid, features);
-        assert_eq!((features & 0xffffffff) as u32, vsock.features(0));
-        assert_eq!((features >> 32) as u32, vsock.features(1));
-        assert_eq!(0, vsock.features(559));
-        assert_eq!(0, vsock.features(3));
+        assert_eq!(features, vsock.features());
     }
 }
diff --git a/devices/src/virtio/virtio_device.rs b/devices/src/virtio/virtio_device.rs
index 8f790d9..a7896de 100644
--- a/devices/src/virtio/virtio_device.rs
+++ b/devices/src/virtio/virtio_device.rs
@@ -27,15 +27,13 @@ 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) {
-        let _ = page;
+    fn ack_features(&mut self, value: u64) {
         let _ = value;
     }
 
diff --git a/devices/src/virtio/virtio_pci_common_config.rs b/devices/src/virtio/virtio_pci_common_config.rs
index 8596563..efbfbe0 100644
--- a/devices/src/virtio/virtio_pci_common_config.rs
+++ b/devices/src/virtio/virtio_pci_common_config.rs
@@ -138,10 +138,13 @@ impl VirtioPciCommonConfig {
                 // TODO(dverkamp): This hack (copied from MmioDevice) unconditionally
                 // reports support for VIRTIO_F_VERSION_1; once all devices have been
                 // fixed to report VIRTIO_F_VERSION_1, remove this workaround.
-                device.features(self.device_feature_select) | if self.device_feature_select == 1 {
-                    0x1
+                let features = device.features() | 1 << VIRTIO_F_VERSION_1;
+                // 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 {
+                    (features >> (self.device_feature_select * 32)) as u32
                 } else {
-                    0x0
+                    0
                 }
             }
             0x08 => self.driver_feature_select,
@@ -167,7 +170,16 @@ impl VirtioPciCommonConfig {
         match offset {
             0x00 => self.device_feature_select = value,
             0x08 => self.driver_feature_select = value,
-            0x0c => device.ack_features(self.driver_feature_select, value),
+            0x0c => {
+                if self.driver_feature_select < 2 {
+                    device.ack_features((value as u64) << (self.driver_feature_select * 32));
+                } else {
+                    warn!(
+                        "invalid ack_features (page {}, value 0x{:x})",
+                        self.driver_feature_select, value
+                    );
+                }
+            }
             0x20 => self.with_queue_mut(queues, |q| lo(&mut q.desc_table, value)),
             0x24 => self.with_queue_mut(queues, |q| hi(&mut q.desc_table, value)),
             0x28 => self.with_queue_mut(queues, |q| lo(&mut q.avail_ring, value)),
@@ -221,7 +233,7 @@ mod tests {
     struct DummyDevice(u32);
     const QUEUE_SIZE: u16 = 256;
     const QUEUE_SIZES: &'static [u16] = &[QUEUE_SIZE];
-    const DUMMY_FEATURES: u32 = 0x5555_aaaa;
+    const DUMMY_FEATURES: u64 = 0x5555_aaaa;
     impl VirtioDevice for DummyDevice {
         fn keep_fds(&self) -> Vec<RawFd> {
             Vec::new()
@@ -242,7 +254,7 @@ mod tests {
             _queue_evts: Vec<EventFd>,
         ) {
         }
-        fn features(&self, _page: u32) -> u32 {
+        fn features(&self) -> u64 {
             DUMMY_FEATURES
         }
     }
@@ -276,7 +288,7 @@ mod tests {
         regs.write(0x04, &[0, 0, 0, 0], &mut queues, &mut dev);
         let mut read_back = vec![0, 0, 0, 0];
         regs.read(0x04, &mut read_back, &mut queues, &mut dev);
-        assert_eq!(LittleEndian::read_u32(&read_back), DUMMY_FEATURES);
+        assert_eq!(LittleEndian::read_u32(&read_back), DUMMY_FEATURES as u32);
 
         // Feature select registers are read/write.
         regs.write(0x00, &[1, 2, 3, 4], &mut queues, &mut dev);
diff --git a/devices/src/virtio/wl.rs b/devices/src/virtio/wl.rs
index 7b9ffc8..98f741f 100644
--- a/devices/src/virtio/wl.rs
+++ b/devices/src/virtio/wl.rs
@@ -1601,15 +1601,12 @@ impl VirtioDevice for Wl {
         QUEUE_SIZES
     }
 
-    fn features(&self, page: u32) -> u32 {
-        match page {
-            0 => (1 << VIRTIO_WL_F_TRANS_FLAGS),
-            _ => 0x0,
-        }
+    fn features(&self) -> u64 {
+        1 << VIRTIO_WL_F_TRANS_FLAGS
     }
 
-    fn ack_features(&mut self, page: u32, value: u32) {
-        if page == 0 && value & (1 << VIRTIO_WL_F_TRANS_FLAGS) != 0 {
+    fn ack_features(&mut self, value: u64) {
+        if value & (1 << VIRTIO_WL_F_TRANS_FLAGS) != 0 {
             self.use_transition_flags = true;
         }
     }