pci: match pci cap structs with linux/virtio_pci.h

VirtioPciCap omits the `cap_vndr` and `cap_next` fields from it's
definition, deferring the instantiation of these bytes to the
add_capability method in PCI configuration. There is even a
comment on add_capability that mentions this omission.

Unfortunately, comments tend not to be read, and mismatches between
the linux headers and crosvm structures can result in some subtle
and tricky to debug bugs, especially when implementing other types
of virtio capabilties that subsume VirtioPciCap.

Case in point, when implementing the VirtioPciShmCap (used by
virtio-fs), this subtle mismatch resulted in a bug where an
additional 2 bytes of padding were inserted between the `cap` member
and the `offset_hi` member (see CL:1493014 for the exact struct).
Since the cap_len field was instantiated using mem::sizeof Self, the
additional padding just-so-happened to be the perfect ammount to sneak
past the sanity checks in add_capabilities. The bug manifested itself by
shifting over the length_hi field by 16 bits, resulting in much larger
than expected cache sizes.

This CL brings the VirtioPciCap structures in-line with their
linux/virtio_pci.h counterparts, marking the structures as repr(C) (as
opposed to repr(packed)) and leaving the cap_vndr and cap_next members
in the struct, noting that they will be automatically populated in
add_capability.

BUG=chromium:936567
TEST=cargo test -p devices, boot vm

Change-Id: Ia360e532b58070372a52346e85dd4e30e81ace7a
Reviewed-on: https://chromium-review.googlesource.com/1540397
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>

2 files changed, 26 insertions, 15 deletions

diff --git a/devices/src/pci/pci_configuration.rs b/devices/src/pci/pci_configuration.rs
index 532faef..a8f9b65 100644
--- a/devices/src/pci/pci_configuration.rs
+++ b/devices/src/pci/pci_configuration.rs
@@ -419,14 +419,11 @@ impl PciConfiguration {
     }
 
     /// Adds the capability `cap_data` to the list of capabilities.
-    /// `cap_data` should not include the two-byte PCI capability header (type, next);
-    /// it will be generated automatically based on `cap_data.id()`.
+    /// `cap_data` should include the two-byte PCI capability header (type, next),
+    /// but not populate it. Correct values will be generated automatically based
+    /// on `cap_data.id()`.
     pub fn add_capability(&mut self, cap_data: &PciCapability) -> Result<usize> {
-        let total_len = cap_data
-            .bytes()
-            .len()
-            .checked_add(2)
-            .ok_or(Error::CapabilityLengthInvalid(cap_data.bytes().len()))?;
+        let total_len = cap_data.bytes().len();
         // Check that the length is valid.
         if cap_data.bytes().is_empty() {
             return Err(Error::CapabilityEmpty);
@@ -445,8 +442,8 @@ impl PciConfiguration {
         self.write_byte_internal(tail_offset, cap_offset as u8, false);
         self.write_byte_internal(cap_offset, cap_data.id() as u8, false);
         self.write_byte_internal(cap_offset + 1, 0, false); // Next pointer.
-        for (i, byte) in cap_data.bytes().iter().enumerate() {
-            self.write_byte_internal(cap_offset + i + 2, *byte, false);
+        for (i, byte) in cap_data.bytes().iter().enumerate().skip(2) {
+            self.write_byte_internal(cap_offset + i, *byte, false);
         }
         self.last_capability = Some((cap_offset, total_len));
         Ok(cap_offset)
@@ -517,6 +514,8 @@ mod tests {
     #[derive(Clone, Copy)]
     #[allow(dead_code)]
     struct TestCap {
+        _vndr: u8,
+        _next: u8,
         len: u8,
         foo: u8,
     }
@@ -548,11 +547,18 @@ mod tests {
         );
 
         // Add two capabilities with different contents.
-        let cap1 = TestCap { len: 4, foo: 0xAA };
+        let cap1 = TestCap {
+            _vndr: 0,
+            _next: 0,
+            len: 4,
+            foo: 0xAA,
+        };
         let cap1_offset = cfg.add_capability(&cap1).unwrap();
         assert_eq!(cap1_offset % 4, 0);
 
         let cap2 = TestCap {
+            _vndr: 0,
+            _next: 0,
             len: 0x04,
             foo: 0x55,
         };
diff --git a/devices/src/virtio/virtio_pci_device.rs b/devices/src/virtio/virtio_pci_device.rs
index 071c7d6..4ba5437 100644
--- a/devices/src/virtio/virtio_pci_device.rs
+++ b/devices/src/virtio/virtio_pci_device.rs
@@ -29,9 +29,12 @@ pub enum PciCapabilityType {
 }
 
 #[allow(dead_code)]
-#[repr(packed)]
+#[repr(C)]
 #[derive(Clone, Copy)]
 struct VirtioPciCap {
+    // _cap_vndr and _cap_next are autofilled based on id() in pci configuration
+    _cap_vndr: u8,    // Generic PCI field: PCI_CAP_ID_VNDR
+    _cap_next: u8,    // Generic PCI field: next ptr
     cap_len: u8,      // Generic PCI field: capability length
     cfg_type: u8,     // Identifies the structure.
     bar: u8,          // Where to find it.
@@ -52,12 +55,12 @@ impl PciCapability for VirtioPciCap {
     }
 }
 
-const VIRTIO_PCI_CAPABILITY_BYTES: u8 = 16;
-
 impl VirtioPciCap {
     pub fn new(cfg_type: PciCapabilityType, bar: u8, offset: u32, length: u32) -> Self {
         VirtioPciCap {
-            cap_len: VIRTIO_PCI_CAPABILITY_BYTES,
+            _cap_vndr: 0,
+            _cap_next: 0,
+            cap_len: std::mem::size_of::<VirtioPciCap>() as u8,
             cfg_type: cfg_type as u8,
             bar,
             padding: [0; 3],
@@ -68,7 +71,7 @@ impl VirtioPciCap {
 }
 
 #[allow(dead_code)]
-#[repr(packed)]
+#[repr(C)]
 #[derive(Clone, Copy)]
 pub struct VirtioPciNotifyCap {
     cap: VirtioPciCap,
@@ -97,6 +100,8 @@ impl VirtioPciNotifyCap {
     ) -> Self {
         VirtioPciNotifyCap {
             cap: VirtioPciCap {
+                _cap_vndr: 0,
+                _cap_next: 0,
                 cap_len: std::mem::size_of::<VirtioPciNotifyCap>() as u8,
                 cfg_type: cfg_type as u8,
                 bar,