[RFC] ReadVolatile and WriteVolatile traits for efficient data transfer from/to guest memory

CHANGELOG.md

@@ -1,5 +1,21 @@
 # Changelog
 
+## Unreleased
+
+### Added
+- [[#247]](https://github.com/rust-vmm/vm-memory/pull/247) Add `ReadVolatile` and
+  `WriteVolatile` traits which are equivalents of `Read`/`Write` with volatile
+  access semantics.
+
+### Changed
+
+- [[#247]](https://github.com/rust-vmm/vm-memory/pull/247) Deprecate
+  `Bytes::{read_from, read_exact_from, write_to, write_all_to}`. Instead use
+  `ReadVolatile`/`WriteVolatile`, which do not incur the performance penalty
+  of copying to hypervisor memory due to `Read`/`Write` being incompatible
+  with volatile semantics (see also #217).
+
+
 ## [v0.12.2]
 
 ### Fixed

benches/mmap/mod.rs

@@ -8,7 +8,6 @@ extern crate criterion;
 extern crate vm_memory;
 
 use std::fs::{File, OpenOptions};
-use std::io::Cursor;
 use std::mem::size_of;
 use std::path::Path;
 
@@ -105,23 +104,23 @@ pub fn benchmark_for_mmap(c: &mut Criterion) {
         c.bench_function(format!("read_from_{:#0X}", offset).as_str(), |b| {
             b.iter(|| {
                 black_box(&memory)
-                    .read_from(address, &mut Cursor::new(&image), ACCESS_SIZE)
+                    .read_volatile_from(address, &mut image.as_slice(), ACCESS_SIZE)
                     .unwrap()
             })
         });
 
         c.bench_function(format!("read_from_file_{:#0X}", offset).as_str(), |b| {
             b.iter(|| {
                 black_box(&memory)
-                    .read_from(address, &mut file, ACCESS_SIZE)
+                    .read_volatile_from(address, &mut file, ACCESS_SIZE)
                     .unwrap()
             })
         });
 
         c.bench_function(format!("read_exact_from_{:#0X}", offset).as_str(), |b| {
             b.iter(|| {
                 black_box(&memory)
-                    .read_exact_from(address, &mut Cursor::new(&mut image), ACCESS_SIZE)
+                    .read_exact_volatile_from(address, &mut image.as_slice(), ACCESS_SIZE)
                     .unwrap()
             })
         });
@@ -154,23 +153,23 @@ pub fn benchmark_for_mmap(c: &mut Criterion) {
         c.bench_function(format!("write_to_{:#0X}", offset).as_str(), |b| {
             b.iter(|| {
                 black_box(&memory)
-                    .write_to(address, &mut Cursor::new(&mut image), ACCESS_SIZE)
+                    .write_volatile_to(address, &mut image.as_mut_slice(), ACCESS_SIZE)
                     .unwrap()
             })
         });
 
         c.bench_function(format!("write_to_file_{:#0X}", offset).as_str(), |b| {
             b.iter(|| {
                 black_box(&memory)
-                    .write_to(address, &mut file_to_write, ACCESS_SIZE)
+                    .write_volatile_to(address, &mut file_to_write, ACCESS_SIZE)
                     .unwrap()
             })
         });
 
         c.bench_function(format!("write_exact_to_{:#0X}", offset).as_str(), |b| {
             b.iter(|| {
                 black_box(&memory)
-                    .write_all_to(address, &mut Cursor::new(&mut image), ACCESS_SIZE)
+                    .write_all_volatile_to(address, &mut image.as_mut_slice(), ACCESS_SIZE)
                     .unwrap()
             })
         });

coverage_config_x86_64.json

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

src/bitmap/mod.rs

@@ -267,6 +267,7 @@ pub(crate) mod tests {
         dirty_offset += step;
 
         // Test `read_from`.
+        #[allow(deprecated)] // test of deprecated functions
         h.test_access(bytes, dirty_offset, BUF_SIZE, |m, addr| {
             assert_eq!(
                 m.read_from(addr, &mut Cursor::new(&buf), BUF_SIZE).unwrap(),
@@ -277,6 +278,7 @@ pub(crate) mod tests {
         dirty_offset += step;
 
         // Test `read_exact_from`.
+        #[allow(deprecated)] // test of deprecated functions
         h.test_access(bytes, dirty_offset, BUF_SIZE, |m, addr| {
             m.read_exact_from(addr, &mut Cursor::new(&buf), BUF_SIZE)
                 .unwrap()

src/bytes.rs

@@ -280,6 +280,9 @@ pub trait Bytes<A> {
     /// * `addr` - Begin writing at this address.
     /// * `src` - Copy from `src` into the container.
     /// * `count` - Copy `count` bytes from `src` into the container.
+    #[deprecated(
+        note = "Use `.read_volatile_from` or the functions of the `ReadVolatile` trait instead"
+    )]
     fn read_from<F>(&self, addr: A, src: &mut F, count: usize) -> Result<usize, Self::E>
     where
         F: Read;
@@ -295,6 +298,9 @@ pub trait Bytes<A> {
     /// * `addr` - Begin writing at this address.
     /// * `src` - Copy from `src` into the container.
     /// * `count` - Copy exactly `count` bytes from `src` into the container.
+    #[deprecated(
+        note = "Use `.read_exact_volatile_from` or the functions of the `ReadVolatile` trait instead"
+    )]
     fn read_exact_from<F>(&self, addr: A, src: &mut F, count: usize) -> Result<(), Self::E>
     where
         F: Read;
@@ -307,6 +313,9 @@ pub trait Bytes<A> {
     /// * `addr` - Begin reading from this address.
     /// * `dst` - Copy from the container to `dst`.
     /// * `count` - Copy `count` bytes from the container to `dst`.
+    #[deprecated(
+        note = "Use `.write_volatile_to` or the functions of the `WriteVolatile` trait instead"
+    )]
     fn write_to<F>(&self, addr: A, dst: &mut F, count: usize) -> Result<usize, Self::E>
     where
         F: Write;
@@ -322,6 +331,9 @@ pub trait Bytes<A> {
     /// * `addr` - Begin reading from this address.
     /// * `dst` - Copy from the container to `dst`.
     /// * `count` - Copy exactly `count` bytes from the container to `dst`.
+    #[deprecated(
+        note = "Use `.write_all_volatile_to` or the functions of the `WriteVolatile` trait instead"
+    )]
     fn write_all_to<F>(&self, addr: A, dst: &mut F, count: usize) -> Result<(), Self::E>
     where
         F: Write;

src/guest_memory.rs

@@ -52,7 +52,9 @@ use std::sync::Arc;
 use crate::address::{Address, AddressValue};
 use crate::bitmap::{Bitmap, BS, MS};
 use crate::bytes::{AtomicAccess, Bytes};
+use crate::io::{ReadVolatile, WriteVolatile};
 use crate::volatile_memory::{self, VolatileSlice};
+use crate::GuestMemoryError;
 
 static MAX_ACCESS_CHUNK: usize = 4096;
 
@@ -665,6 +667,146 @@ pub trait GuestMemory {
         }
     }
 
+    /// Reads up to `count` bytes from an object and writes them into guest memory at `addr`.
+    ///
+    /// Returns the number of bytes written into guest memory.
+    ///
+    /// # Arguments
+    /// * `addr` - Begin writing at this address.
+    /// * `src` - Copy from `src` into the container.
+    /// * `count` - Copy `count` bytes from `src` into the container.
+    ///
+    /// # Examples
+    ///
+    /// * Read bytes from /dev/urandom (uses the `backend-mmap` feature)
+    ///
+    /// ```
+    /// # #[cfg(feature = "backend-mmap")]
+    /// # {
+    /// # use vm_memory::{Address, GuestMemory, Bytes, GuestAddress, GuestMemoryMmap};
+    /// # use std::fs::File;
+    /// # use std::path::Path;
+    /// #
+    /// # let start_addr = GuestAddress(0x1000);
+    /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)])
+    /// #    .expect("Could not create guest memory");
+    /// # let addr = GuestAddress(0x1010);
+    /// # let mut file = if cfg!(unix) {
+    /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom");
+    /// #   file
+    /// # } else {
+    /// #   File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe"))
+    /// #       .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe")
+    /// # };
+    ///
+    /// gm.read_volatile_from(addr, &mut file, 128)
+    ///     .expect("Could not read from /dev/urandom into guest memory");
+    ///
+    /// let read_addr = addr.checked_add(8).expect("Could not compute read address");
+    /// let rand_val: u32 = gm
+    ///     .read_obj(read_addr)
+    ///     .expect("Could not read u32 val from /dev/urandom");
+    /// # }
+    /// ```
+    fn read_volatile_from<F>(&self, addr: GuestAddress, src: &mut F, count: usize) -> Result<usize>
+    where
+        F: ReadVolatile,
+    {
+        self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
+            // Check if something bad happened before doing unsafe things.
+            assert!(offset <= count);
+
+            let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
+
+            let mut vslice = region.get_slice(caddr, len)?;
+
+            src.read_volatile(&mut vslice)
+                .map_err(GuestMemoryError::from)
+        })
+    }
+
+    /// Reads up to `count` bytes from the container at `addr` and writes them it into guest memory.
+    ///
+    /// Returns the number of bytes written into guest memory.
+    ///
+    /// # Arguments
+    /// * `addr` - Begin reading from this address.
+    /// * `dst` - Copy from guest memory to `dst`.
+    /// * `count` - Copy `count` bytes from guest memory to `dst`.
+    fn write_volatile_to<F>(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<usize>
+    where
+        F: WriteVolatile,
+    {
+        self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
+            // Check if something bad happened before doing unsafe things.
+            assert!(offset <= count);
+
+            let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
+            let vslice = region.get_slice(caddr, len)?;
+
+            // For a non-RAM region, reading could have side effects, so we
+            // must use write_all().
+            dst.write_all_volatile(&vslice)?;
+
+            Ok(len)
+        })
+    }
+
+    /// Reads exactly `count` bytes from an object and writes them into guest memory at `addr`.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if `count` bytes couldn't have been copied from `src` to guest memory.
+    /// Part of the data may have been copied nevertheless.
+    ///
+    /// # Arguments
+    /// * `addr` - Begin writing at this address.
+    /// * `src` - Copy from `src` into guest memory.
+    /// * `count` - Copy exactly `count` bytes from `src` into guest memory.
+    fn read_exact_volatile_from<F>(
+        &self,
+        addr: GuestAddress,
+        src: &mut F,
+        count: usize,
+    ) -> Result<()>
+    where
+        F: ReadVolatile,
+    {
+        let res = self.read_volatile_from(addr, src, count)?;
+        if res != count {
+            return Err(Error::PartialBuffer {
+                expected: count,
+                completed: res,
+            });
+        }
+        Ok(())
+    }
+
+    /// Reads exactly `count` bytes from the container at `addr` and writes them into guest memory.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if `count` bytes couldn't have been copied from guest memory to `dst`.
+    /// Part of the data may have been copied nevertheless.
+    ///
+    /// # Arguments
+    /// * `addr` - Begin reading from this address.
+    /// * `dst` - Copy from guest memory to `dst`.
+    /// * `count` - Copy exactly `count` bytes from guest memory to `dst`.
+    fn write_all_volatile_to<F>(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<()>
+    where
+        F: WriteVolatile,
+    {
+        let res = self.write_volatile_to(addr, dst, count)?;
+        if res != count {
+            return Err(Error::PartialBuffer {
+                expected: count,
+                completed: res,
+            });
+        }
+        Ok(())
+    }
+
     /// Get the host virtual address corresponding to the guest address.
     ///
     /// Some [`GuestMemory`](trait.GuestMemory.html) implementations, like `GuestMemoryMmap`,
@@ -856,6 +998,7 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
     where
         F: Read,
     {
+        #[allow(deprecated)] // this function itself is deprecated
         let res = self.read_from(addr, src, count)?;
         if res != count {
             return Err(Error::PartialBuffer {
@@ -949,6 +1092,7 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
     where
         F: Write,
     {
+        #[allow(deprecated)] // this function itself is deprecated
         let res = self.write_to(addr, dst, count)?;
         if res != count {
             return Err(Error::PartialBuffer {
@@ -983,8 +1127,6 @@ mod tests {
     #[cfg(feature = "backend-mmap")]
     use crate::GuestAddress;
     #[cfg(feature = "backend-mmap")]
-    use std::io::Cursor;
-    #[cfg(feature = "backend-mmap")]
     use std::time::{Duration, Instant};
 
     use vmm_sys_util::tempfile::TempFile;
@@ -1024,7 +1166,7 @@ mod tests {
         let count: usize = 0x20;
         assert_eq!(
             0x20_usize,
-            mem.read_from(offset, &mut Cursor::new(&image), count)
+            mem.read_volatile_from(offset, &mut image.as_slice(), count)
                 .unwrap()
         );
     }
@@ -1179,19 +1321,24 @@ mod tests {
         assert!(mem.write_obj(obj, addr).is_ok());
         assert!(mem.read_obj::<ZeroSizedStruct>(addr).is_ok());
 
-        assert_eq!(mem.read_from(addr, &mut Cursor::new(&image), 0).unwrap(), 0);
+        assert_eq!(
+            mem.read_volatile_from(addr, &mut image.as_slice(), 0)
+                .unwrap(),
+            0
+        );
 
         assert!(mem
-            .read_exact_from(addr, &mut Cursor::new(&image), 0)
+            .read_exact_volatile_from(addr, &mut image.as_slice(), 0)
             .is_ok());
 
         assert_eq!(
-            mem.write_to(addr, &mut Cursor::new(&mut image), 0).unwrap(),
+            mem.write_volatile_to(addr, &mut image.as_mut_slice(), 0)
+                .unwrap(),
             0
         );
 
         assert!(mem
-            .write_all_to(addr, &mut Cursor::new(&mut image), 0)
+            .write_all_volatile_to(addr, &mut image.as_mut_slice(), 0)
             .is_ok());
     }