kvm-ioctls: Fix a bug in enable_dirty_log_ring and add test

Fixes a bug in enable_dirty_log_ring that incorrectly checks if the size
passed is a multiple of the element size. Adds a example / doctest that checks
basic functionality of enable_dirty_log_ring

Signed-off-by: David Kleymann <[email protected]>

Author: davidkleymann

kvm-ioctls/src/ioctls/mod.rs

@@ -44,12 +44,6 @@ pub(crate) struct KvmDirtyLogRing {
     use_acq_rel: bool,
 }
 
-// // SAFETY: For each vcpu we only allow creating a single instance of the KvmDirtyLogRing,
-// // therefore ownership can safely be transferred between threads (`Send`).
-// unsafe impl Send for KvmDirtyLogRing {}
-
-// // SAFETY: TBD
-// unsafe impl Sync for KvmDirtyLogRing {}
 impl KvmDirtyLogRing {
     /// Maps the KVM dirty log ring from the vCPU file descriptor.
     ///
@@ -94,14 +88,14 @@ impl KvmDirtyLogRing {
                 offset as i64,
             ) as *mut kvm_dirty_gfn)
             .filter(|addr| addr.as_ptr() != libc::MAP_FAILED as *mut kvm_dirty_gfn)
-            .ok_or_else(|| errno::Error::last())?
+            .ok_or_else(errno::Error::last)?
         };
-        return Ok(Self {
+        Ok(Self {
             next_dirty: 0,
             gfns,
             mask: (slots - 1) as u64,
             use_acq_rel,
-        });
+        })
     }
 }
 

kvm-ioctls/src/ioctls/vcpu.rs

@@ -2202,7 +2202,7 @@ pub fn new_vcpu(
         vcpu,
         kvm_run_ptr,
         coalesced_mmio_ring: None,
-        dirty_log_ring: dirty_log_ring,
+        dirty_log_ring,
     }
 }
 
@@ -2874,6 +2874,144 @@ mod tests {
         }
     }
 
+    #[cfg(target_arch = "x86_64")]
+    #[test]
+    fn test_run_code_dirty_log_ring() {
+        use std::io::Write;
+
+        let kvm = Kvm::new().unwrap();
+        let mut vm = kvm.create_vm().unwrap();
+
+        // Enable dirty log ring
+        let need_bitmap = vm.enable_dirty_log_ring(None).unwrap();
+
+        // This example is based on https://lwn.net/Articles/658511/
+        #[rustfmt::skip]
+        let code = [
+            0xba, 0xf8, 0x03, /* mov $0x3f8, %dx */
+            0x00, 0xd8, /* add %bl, %al */
+            0x04, b'0', /* add $'0', %al */
+            0xee, /* out %al, %dx */
+            0xec, /* in %dx, %al */
+            0xc6, 0x06, 0x00, 0x80, 0x00, /* movl $0, (0x8000); This generates a MMIO Write.*/
+            0x8a, 0x16, 0x00, 0x80, /* movl (0x8000), %dl; This generates a MMIO Read.*/
+            0xc6, 0x06, 0x00, 0x20, 0x00, /* movl $0, (0x2000); Dirty one page in guest mem. */
+            0xf4, /* hlt */
+        ];
+        let expected_rips: [u64; 3] = [0x1003, 0x1005, 0x1007];
+
+        let mem_size = 0x4000;
+        let load_addr = mmap_anonymous(mem_size).as_ptr();
+        let guest_addr: u64 = 0x1000;
+        let slot: u32 = 0;
+        let mem_region = kvm_userspace_memory_region {
+            slot,
+            guest_phys_addr: guest_addr,
+            memory_size: mem_size as u64,
+            userspace_addr: load_addr as u64,
+            flags: KVM_MEM_LOG_DIRTY_PAGES,
+        };
+        unsafe {
+            vm.set_user_memory_region(mem_region).unwrap();
+        }
+
+        unsafe {
+            // Get a mutable slice of `mem_size` from `load_addr`.
+            // This is safe because we mapped it before.
+            let mut slice = std::slice::from_raw_parts_mut(load_addr, mem_size);
+            slice.write_all(&code).unwrap();
+        }
+
+        let mut vcpu_fd = vm.create_vcpu(0).unwrap();
+
+        let mut vcpu_sregs = vcpu_fd.get_sregs().unwrap();
+        assert_ne!(vcpu_sregs.cs.base, 0);
+        assert_ne!(vcpu_sregs.cs.selector, 0);
+        vcpu_sregs.cs.base = 0;
+        vcpu_sregs.cs.selector = 0;
+        vcpu_fd.set_sregs(&vcpu_sregs).unwrap();
+
+        let mut vcpu_regs = vcpu_fd.get_regs().unwrap();
+        // Set the Instruction Pointer to the guest address where we loaded the code.
+        vcpu_regs.rip = guest_addr;
+        vcpu_regs.rax = 2;
+        vcpu_regs.rbx = 3;
+        vcpu_regs.rflags = 2;
+        vcpu_fd.set_regs(&vcpu_regs).unwrap();
+
+        let mut debug_struct = kvm_guest_debug {
+            control: KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP,
+            pad: 0,
+            arch: kvm_guest_debug_arch {
+                debugreg: [0, 0, 0, 0, 0, 0, 0, 0],
+            },
+        };
+        vcpu_fd.set_guest_debug(&debug_struct).unwrap();
+
+        let mut instr_idx = 0;
+        loop {
+            match vcpu_fd.run().expect("run failed") {
+                VcpuExit::IoIn(addr, data) => {
+                    assert_eq!(addr, 0x3f8);
+                    assert_eq!(data.len(), 1);
+                }
+                VcpuExit::IoOut(addr, data) => {
+                    assert_eq!(addr, 0x3f8);
+                    assert_eq!(data.len(), 1);
+                    assert_eq!(data[0], b'5');
+                }
+                VcpuExit::MmioRead(addr, data) => {
+                    assert_eq!(addr, 0x8000);
+                    assert_eq!(data.len(), 1);
+                }
+                VcpuExit::MmioWrite(addr, data) => {
+                    assert_eq!(addr, 0x8000);
+                    assert_eq!(data.len(), 1);
+                    assert_eq!(data[0], 0);
+                }
+                VcpuExit::Debug(debug) => {
+                    if instr_idx == expected_rips.len() - 1 {
+                        // Disabling debugging/single-stepping
+                        debug_struct.control = 0;
+                        vcpu_fd.set_guest_debug(&debug_struct).unwrap();
+                    } else if instr_idx >= expected_rips.len() {
+                        unreachable!();
+                    }
+                    let vcpu_regs = vcpu_fd.get_regs().unwrap();
+                    assert_eq!(vcpu_regs.rip, expected_rips[instr_idx]);
+                    assert_eq!(debug.exception, 1);
+                    assert_eq!(debug.pc, expected_rips[instr_idx]);
+                    // Check first 15 bits of DR6
+                    let mask = (1 << 16) - 1;
+                    assert_eq!(debug.dr6 & mask, 0b100111111110000);
+                    // Bit 10 in DR7 is always 1
+                    assert_eq!(debug.dr7, 1 << 10);
+                    instr_idx += 1;
+                }
+                VcpuExit::Hlt => {
+                    // The code snippet dirties 2 pages:
+                    // * one when the code itself is loaded in memory;
+                    // * and one more from the `movl` that writes to address 0x8000
+
+                    let dirty_pages: u32 =
+                        u32::try_from(vcpu_fd.dirty_log_ring_iter().unwrap().count()).unwrap()
+                            + if need_bitmap {
+                                let dirty_pages_bitmap = vm.get_dirty_log(slot, mem_size).unwrap();
+                                dirty_pages_bitmap
+                                    .into_iter()
+                                    .map(|page| page.count_ones())
+                                    .sum()
+                            } else {
+                                0
+                            };
+                    assert_eq!(dirty_pages, 2);
+                    break;
+                }
+                r => panic!("unexpected exit reason: {:?}", r),
+            }
+        }
+    }
+
     #[test]
     #[cfg(target_arch = "aarch64")]
     fn test_get_preferred_target() {

kvm-ioctls/src/ioctls/vm.rs

@@ -1947,18 +1947,34 @@ impl VmFd {
     /// Enables KVM's dirty log ring for new vCPUs created on this VM. Checks required capabilities and returns
     /// a boolean `use_bitmap` as a result. `use_bitmap` is `true` if the ring needs to be used
     /// together with a backup bitmap `KVM_GET_DIRTY_LOG`. Takes optional dirty ring size as bytes, if not supplied, will
-    /// use maximum supported dirty ring size. Enabling the dirty log ring is only allowed before any vCPU was
-    /// created on the VmFd.
+    /// use maximum supported dirty ring size. The size needs to be multiple of `std::mem::size_of::<kvm_dirty_gfn>()`
+    /// and power of two. Enabling the dirty log ring is only allowed before any vCPU was created on the VmFd.
+    ///
     /// # Arguments
     ///
-    /// * `bytes` - Size of the dirty log ring in bytes. Needs to be multiple of `std::mem::size_of::<kvm_dirty_gfn>()`
-    /// and power of two.
+    /// * `bytes` - Size of the dirty log ring in bytes.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # extern crate kvm_bindings;
+    /// # extern crate kvm_ioctls;
+    /// # use kvm_ioctls::Kvm;
+    /// # use kvm_bindings::{ KVM_CAP_DIRTY_LOG_RING, };
+    ///
+    /// let kvm = Kvm::new().unwrap();
+    /// let mut vm = kvm.create_vm().unwrap();
+    /// let max_supported_size = vm.check_extension_raw(KVM_CAP_DIRTY_LOG_RING.into());
+    /// vm.enable_dirty_log_ring(Some(max_supported_size));
+    /// // Create one vCPU with the ID=0 to cause a dirty log ring to be mapped.
+    /// let vcpu = vm.create_vcpu(0);
+    /// ```
     pub fn enable_dirty_log_ring(&mut self, bytes: Option<i32>) -> Result<bool> {
         // Check if requested size is larger than 0
         if let Some(sz) = bytes {
             if sz <= 0
                 || !(sz as u32).is_power_of_two()
-                || (sz as usize % std::mem::size_of::<kvm_dirty_gfn>() == 0)
+                || (sz as usize % std::mem::size_of::<kvm_dirty_gfn>() != 0)
             {
                 return Err(errno::Error::new(libc::EINVAL));
             }
@@ -2034,13 +2050,14 @@ impl VmFd {
     /// # use kvm_ioctls::{Cap, Kvm};
     /// let kvm = Kvm::new().unwrap();
     /// let mut vm = kvm.create_vm().unwrap();
-    /// vm.enable_dirty_log_ring(None).unwrap();
     /// if kvm.check_extension(Cap::DirtyLogRing) {
+    ///     vm.enable_dirty_log_ring(None).unwrap();
+    ///     // Create one vCPU with the ID=0 to cause a dirty log ring to be mapped.
+    ///     let vcpu = vm.create_vcpu(0);
     ///     vm.reset_dirty_rings().unwrap();
     /// }
     /// ```
     ///
-    #[cfg(target_arch = "x86_64")]
     pub fn reset_dirty_rings(&self) -> Result<c_int> {
         // SAFETY: Safe because we know that our file is a KVM fd and that the request is one of
         // the ones defined by kernel.
@@ -3049,4 +3066,73 @@ mod tests {
         vm.has_device_attr(&dist_attr).unwrap();
         vm.set_device_attr(&dist_attr).unwrap();
     }
+
+    #[test]
+    fn test_enable_dirty_log_rings() {
+        let kvm = Kvm::new().unwrap();
+        let mut vm = kvm.create_vm().unwrap();
+        if kvm.check_extension(Cap::DirtyLogRing) {
+            vm.enable_dirty_log_ring(None).unwrap();
+            // Create two vCPUs to cause two dirty log rings to be mapped.
+            let _0 = vm.create_vcpu(0).unwrap();
+            let _1 = vm.create_vcpu(1).unwrap();
+            vm.reset_dirty_rings().unwrap();
+        }
+    }
+
+    #[test]
+    fn test_enable_dirty_log_rings_sized() {
+        let kvm = Kvm::new().unwrap();
+        let mut vm = kvm.create_vm().unwrap();
+        if kvm.check_extension(Cap::DirtyLogRing) {
+            let max_supported_size = vm.check_extension_raw(KVM_CAP_DIRTY_LOG_RING.into());
+            let size = std::cmp::max(max_supported_size / 2, size_of::<kvm_dirty_gfn>() as i32);
+            vm.enable_dirty_log_ring(Some(size)).unwrap();
+            // Create two vCPUs to cause two dirty log rings to be mapped.
+            let _0 = vm.create_vcpu(0).unwrap();
+            let _1 = vm.create_vcpu(1).unwrap();
+            vm.reset_dirty_rings().unwrap();
+        }
+    }
+
+    #[test]
+    #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))]
+    fn test_enable_dirty_log_rings_acq_rel() {
+        let kvm = Kvm::new().unwrap();
+        let mut vm = kvm.create_vm().unwrap();
+        if kvm.check_extension(Cap::DirtyLogRing) {
+            vm.enable_dirty_log_ring(None).unwrap();
+
+            // Manually enable Acq/Rel
+            vm.dirty_log_ring_info = vm
+                .dirty_log_ring_info
+                .map(|i| DirtyLogRingInfo { acq_rel: true, ..i });
+
+            // Create two vCPUs to cause two dirty log rings to be mapped.
+            let _0 = vm.create_vcpu(0).unwrap();
+            let _1 = vm.create_vcpu(1).unwrap();
+
+            // Reset dirty rings
+            vm.reset_dirty_rings().unwrap();
+        }
+    }
+
+    #[test]
+    fn test_illegal_dirty_ring() {
+        let kvm = Kvm::new().unwrap();
+        let mut vm = kvm.create_vm().unwrap();
+        if kvm.check_extension(Cap::DirtyLogRing) {
+            // Create one vCPU without dirty log ring
+            let _0 = vm.create_vcpu(0).unwrap();
+
+            // Not allowed after vCPU has been created
+            assert!(vm.enable_dirty_log_ring(None).is_err());
+
+            // Create another vCPU
+            let _1 = vm.create_vcpu(1).unwrap();
+
+            // Dirty ring should not be enabled
+            assert!(vm.reset_dirty_rings().is_err());
+        }
+    }
 }