Use pointers instead of slices when operating on guest memory

benches/volatile.rs

@@ -4,19 +4,22 @@
 
 pub use criterion::{black_box, Criterion};
 use vm_memory::volatile_memory::VolatileMemory;
+use vm_memory::VolatileSlice;
 
 pub fn benchmark_for_volatile(c: &mut Criterion) {
     let mut a = [0xa5u8; 1024];
-    let a_ref = &mut a[..];
-    let v_ref8 = a_ref.get_slice(0, a_ref.len()).unwrap();
-    let v_ref16 = a_ref.get_slice(0, a_ref.len() / 2).unwrap();
+    let vslice = VolatileSlice::from(&mut a[..]);
+    let v_ref8 = vslice.get_slice(0, vslice.len()).unwrap();
     let mut d8 = [0u8; 1024];
-    let mut d16 = [0u16; 512];
 
     // Check performance for read operations.
     c.bench_function("VolatileSlice::copy_to_u8", |b| {
         b.iter(|| v_ref8.copy_to(black_box(&mut d8[..])))
     });
+
+    let v_ref16 = vslice.get_slice(0, vslice.len() / 2).unwrap();
+    let mut d16 = [0u16; 512];
+
     c.bench_function("VolatileSlice::copy_to_u16", |b| {
         b.iter(|| v_ref16.copy_to(black_box(&mut d16[..])))
     });
@@ -33,11 +36,11 @@ pub fn benchmark_for_volatile(c: &mut Criterion) {
 
 fn benchmark_volatile_copy_to_volatile_slice(c: &mut Criterion) {
     let mut a = [0xa5u8; 10240];
-    let a_ref = &mut a[..];
-    let a_slice = a_ref.get_slice(0, a_ref.len()).unwrap();
+    let vslice = VolatileSlice::from(&mut a[..]);
+    let a_slice = vslice.get_slice(0, vslice.len()).unwrap();
     let mut d = [0u8; 10240];
-    let d_ref = &mut d[..];
-    let d_slice = d_ref.get_slice(0, d_ref.len()).unwrap();
+    let vslice2 = VolatileSlice::from(&mut d[..]);
+    let d_slice = vslice2.get_slice(0, vslice2.len()).unwrap();
 
     c.bench_function("VolatileSlice::copy_to_volatile_slice", |b| {
         b.iter(|| black_box(a_slice).copy_to_volatile_slice(d_slice))

coverage_config_x86_64.json

@@ -1,5 +1,5 @@
 {
-  "coverage_score": 87.1,
+  "coverage_score": 88.9,
   "exclude_path": "mmap_windows.rs",
   "crate_features": "backend-mmap,backend-atomic,backend-bitmap"
 }

src/bytes.rs

@@ -120,7 +120,7 @@ pub unsafe trait ByteValued: Copy + Default + Send + Sync {
     /// be no other accesses).
     fn as_bytes(&mut self) -> VolatileSlice {
         // SAFETY: This is safe because the lifetime is the same as self
-        unsafe { VolatileSlice::new(self as *mut Self as usize as *mut _, size_of::<Self>()) }
+        unsafe { VolatileSlice::new(self as *mut Self as *mut _, size_of::<Self>()) }
     }
 }
 

src/guest_memory.rs

@@ -250,6 +250,8 @@ pub trait GuestMemoryRegion: Bytes<MemoryRegionAddress, E = Error> {
     /// # Safety
     ///
     /// Unsafe because of possible aliasing.
+    #[deprecated = "It is impossible to use this function for accessing memory of a running virtual \
+    machine without violating aliasing rules "]
     unsafe fn as_slice(&self) -> Option<&[u8]> {
         None
     }
@@ -263,6 +265,8 @@ pub trait GuestMemoryRegion: Bytes<MemoryRegionAddress, E = Error> {
     /// Unsafe because of possible aliasing. Mutable accesses performed through the
     /// returned slice are not visible to the dirty bitmap tracking functionality of
     /// the region, and must be manually recorded using the associated bitmap object.
+    #[deprecated = "It is impossible to use this function for accessing memory of a running virtual \
+    machine without violating aliasing rules "]
     unsafe fn as_mut_slice(&self) -> Option<&mut [u8]> {
         None
     }
@@ -848,38 +852,22 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
         self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
             // Check if something bad happened before doing unsafe things.
             assert!(offset <= count);
-            // SAFETY: Safe because we are checking the offset.
-            if let Some(dst) = unsafe { region.as_mut_slice() } {
-                // This is safe cause `start` and `len` are within the `region`, and we manually
-                // record the dirty status of the written range below.
-                let start = caddr.raw_value() as usize;
-                let end = start + len;
-                let bytes_read = loop {
-                    match src.read(&mut dst[start..end]) {
-                        Ok(n) => break n,
-                        Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
-                        Err(e) => return Err(Error::IOError(e)),
-                    }
-                };
-
-                region.bitmap().mark_dirty(start, bytes_read);
-                Ok(bytes_read)
-            } else {
-                let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
-                let mut buf = vec![0u8; len].into_boxed_slice();
-                loop {
-                    match src.read(&mut buf[..]) {
-                        Ok(bytes_read) => {
-                            // We don't need to update the dirty bitmap manually here because it's
-                            // expected to be handled by the logic within the `Bytes`
-                            // implementation for the region object.
-                            let bytes_written = region.write(&buf[0..bytes_read], caddr)?;
-                            assert_eq!(bytes_written, bytes_read);
-                            break Ok(bytes_read);
-                        }
-                        Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
-                        Err(e) => break Err(Error::IOError(e)),
+
+            let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
+            let mut buf = vec![0u8; len].into_boxed_slice();
+
+            loop {
+                match src.read(&mut buf[..]) {
+                    Ok(bytes_read) => {
+                        // We don't need to update the dirty bitmap manually here because it's
+                        // expected to be handled by the logic within the `Bytes`
+                        // implementation for the region object.
+                        let bytes_written = region.write(&buf[0..bytes_read], caddr)?;
+                        assert_eq!(bytes_written, bytes_read);
+                        break Ok(bytes_read);
                     }
+                    Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
+                    Err(e) => break Err(Error::IOError(e)),
                 }
             }
         })
@@ -936,32 +924,15 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
         self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
             // Check if something bad happened before doing unsafe things.
             assert!(offset <= count);
-            // SAFETY: Safe because we are checking the offset is valid.
-            if let Some(src) = unsafe { region.as_slice() } {
-                // This is safe cause `start` and `len` are within the `region`.
-                let start = caddr.raw_value() as usize;
-                let end = start + len;
-                loop {
-                    // It is safe to read from volatile memory. Accessing the guest
-                    // memory as a slice should be OK as long as nothing assumes another
-                    // thread won't change what is loaded; however, we may want to introduce
-                    // VolatileRead and VolatileWrite traits in the future.
-                    match dst.write(&src[start..end]) {
-                        Ok(n) => break Ok(n),
-                        Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
-                        Err(e) => break Err(Error::IOError(e)),
-                    }
-                }
-            } else {
-                let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
-                let mut buf = vec![0u8; len].into_boxed_slice();
-                let bytes_read = region.read(&mut buf, caddr)?;
-                assert_eq!(bytes_read, len);
-                // For a non-RAM region, reading could have side effects, so we
-                // must use write_all().
-                dst.write_all(&buf).map_err(Error::IOError)?;
-                Ok(len)
-            }
+
+            let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
+            let mut buf = vec![0u8; len].into_boxed_slice();
+            let bytes_read = region.read(&mut buf, caddr)?;
+            assert_eq!(bytes_read, len);
+            // For a non-RAM region, reading could have side effects, so we
+            // must use write_all().
+            dst.write_all(&buf).map_err(Error::IOError)?;
+            Ok(len)
         })
     }
 

src/mmap.rs

@@ -50,26 +50,6 @@ impl NewBitmap for () {
     fn with_len(_len: usize) -> Self {}
 }
 
-/// Trait implemented by the underlying `MmapRegion`.
-pub(crate) trait AsSlice {
-    /// Returns a slice corresponding to the data in the underlying `MmapRegion`.
-    ///
-    /// # Safety
-    ///
-    /// This is unsafe because of possible aliasing.
-    unsafe fn as_slice(&self) -> &[u8];
-
-    /// Returns a mutable slice corresponding to the data in the underlying `MmapRegion`.
-    ///
-    /// # Safety
-    ///
-    /// This is unsafe because of possible aliasing. Accesses done through the resulting slice
-    /// are not visible to dirty bitmap tracking functionality (when present), and have to be
-    /// explicitly accounted for.
-    #[allow(clippy::mut_from_ref)]
-    unsafe fn as_mut_slice(&self) -> &mut [u8];
-}
-
 /// Errors that can occur when creating a memory map.
 #[derive(Debug)]
 pub enum Error {
@@ -486,14 +466,6 @@ impl<B: Bitmap> GuestMemoryRegion for GuestRegionMmap<B> {
         self.mapping.file_offset()
     }
 
-    unsafe fn as_slice(&self) -> Option<&[u8]> {
-        Some(self.mapping.as_slice())
-    }
-
-    unsafe fn as_mut_slice(&self) -> Option<&mut [u8]> {
-        Some(self.mapping.as_mut_slice())
-    }
-
     fn get_slice(
         &self,
         offset: MemoryRegionAddress,

src/mmap_unix.rs

@@ -19,8 +19,8 @@ use std::result;
 
 use crate::bitmap::{Bitmap, BS};
 use crate::guest_memory::FileOffset;
-use crate::mmap::{check_file_offset, AsSlice, NewBitmap};
-use crate::volatile_memory::{self, compute_offset, VolatileMemory, VolatileSlice};
+use crate::mmap::{check_file_offset, NewBitmap};
+use crate::volatile_memory::{self, VolatileMemory, VolatileSlice};
 
 /// Error conditions that may arise when creating a new `MmapRegion` object.
 #[derive(Debug)]
@@ -401,21 +401,6 @@ impl<B: Bitmap> MmapRegion<B> {
     }
 }
 
-impl<B> AsSlice for MmapRegion<B> {
-    // SAFETY: This is safe because we mapped the area at addr ourselves, so this slice will not
-    // overflow. However, it is possible to alias.
-    unsafe fn as_slice(&self) -> &[u8] {
-        std::slice::from_raw_parts(self.addr, self.size)
-    }
-
-    // SAFETY: This is safe because we mapped the area at addr ourselves, so this slice will not
-    // overflow. However, it is possible to alias.
-    #[allow(clippy::mut_from_ref)]
-    unsafe fn as_mut_slice(&self) -> &mut [u8] {
-        std::slice::from_raw_parts_mut(self.addr, self.size)
-    }
-}
-
 impl<B: Bitmap> VolatileMemory for MmapRegion<B> {
     type B = B;
 
@@ -428,17 +413,14 @@ impl<B: Bitmap> VolatileMemory for MmapRegion<B> {
         offset: usize,
         count: usize,
     ) -> volatile_memory::Result<VolatileSlice<BS<B>>> {
-        let end = compute_offset(offset, count)?;
-        if end > self.size {
-            return Err(volatile_memory::Error::OutOfBounds { addr: end });
-        }
+        let _ = self.compute_end_offset(offset, count)?;
 
         Ok(
             // SAFETY: Safe because we checked that offset + count was within our range and we only
             // ever hand out volatile accessors.
             unsafe {
                 VolatileSlice::with_bitmap(
-                    (self.addr as usize + offset) as *mut _,
+                    self.addr.add(offset),
                     count,
                     self.bitmap.slice_at(offset),
                 )

src/mmap_windows.rs

@@ -14,7 +14,7 @@ use winapi::um::errhandlingapi::GetLastError;
 
 use crate::bitmap::{Bitmap, BS};
 use crate::guest_memory::FileOffset;
-use crate::mmap::{AsSlice, NewBitmap};
+use crate::mmap::NewBitmap;
 use crate::volatile_memory::{self, compute_offset, VolatileMemory, VolatileSlice};
 
 #[allow(non_snake_case)]
@@ -190,21 +190,6 @@ impl<B: Bitmap> MmapRegion<B> {
     }
 }
 
-impl<B> AsSlice for MmapRegion<B> {
-    unsafe fn as_slice(&self) -> &[u8] {
-        // This is safe because we mapped the area at addr ourselves, so this slice will not
-        // overflow. However, it is possible to alias.
-        std::slice::from_raw_parts(self.addr, self.size)
-    }
-
-    #[allow(clippy::mut_from_ref)]
-    unsafe fn as_mut_slice(&self) -> &mut [u8] {
-        // This is safe because we mapped the area at addr ourselves, so this slice will not
-        // overflow. However, it is possible to alias.
-        std::slice::from_raw_parts_mut(self.addr, self.size)
-    }
-}
-
 impl<B: Bitmap> VolatileMemory for MmapRegion<B> {
     type B = B;
 
@@ -225,11 +210,7 @@ impl<B: Bitmap> VolatileMemory for MmapRegion<B> {
         // Safe because we checked that offset + count was within our range and we only ever hand
         // out volatile accessors.
         Ok(unsafe {
-            VolatileSlice::with_bitmap(
-                (self.addr as usize + offset) as *mut _,
-                count,
-                self.bitmap.slice_at(offset),
-            )
+            VolatileSlice::with_bitmap(self.addr.add(offset), count, self.bitmap.slice_at(offset))
         })
     }
 }