Skip to content

Commit 7a911a4

Browse files
roypatJonathanWoollett-Light
roypat
authored and
JonathanWoollett-Light
committed
Add read_volatile_from/write_volatile_to to GuestMemory
These are the equivalent of the old read_from and write_to functions from Bytes. They have the same semantics in the sense that they correctly handle reads/writes spanning multiple guest memory regions. Note that no equivalent will be introduced in Bytes itself, as that those usecases are completely met by turning whatever Bytes-impl one has to a VolatileSlice, and then using the traits directly. This also has the advantage of streamlining the bounds checking to the functions that return VolatileSlices. Furthermore updates the tests to no longer use the deprecated functions. Note that some negative tests are removed due to bounds checking happening in get_slice instead of read_from/write_to now. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
1 parent 9c3e6aa commit 7a911a4

File tree

6 files changed

+189
-43
lines changed

6 files changed

+189
-43
lines changed

‎benches/mmap/mod.rs‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ extern crate criterion;
88
extern crate vm_memory;
99

1010
use std::fs::{File, OpenOptions};
11-
use std::io::Cursor;
1211
use std::mem::size_of;
1312
use std::path::Path;
1413

@@ -105,23 +104,23 @@ pub fn benchmark_for_mmap(c: &mut Criterion) {
105104
c.bench_function(format!("read_from_{:#0X}", offset).as_str(), |b| {
106105
b.iter(|| {
107106
black_box(&memory)
108-
.read_from(address, &mut Cursor::new(&image), ACCESS_SIZE)
107+
.read_volatile_from(address, &mut image.as_slice(), ACCESS_SIZE)
109108
.unwrap()
110109
})
111110
});
112111

113112
c.bench_function(format!("read_from_file_{:#0X}", offset).as_str(), |b| {
114113
b.iter(|| {
115114
black_box(&memory)
116-
.read_from(address, &mut file, ACCESS_SIZE)
115+
.read_volatile_from(address, &mut file, ACCESS_SIZE)
117116
.unwrap()
118117
})
119118
});
120119

121120
c.bench_function(format!("read_exact_from_{:#0X}", offset).as_str(), |b| {
122121
b.iter(|| {
123122
black_box(&memory)
124-
.read_exact_from(address, &mut Cursor::new(&mut image), ACCESS_SIZE)
123+
.read_exact_volatile_from(address, &mut image.as_slice(), ACCESS_SIZE)
125124
.unwrap()
126125
})
127126
});
@@ -154,23 +153,23 @@ pub fn benchmark_for_mmap(c: &mut Criterion) {
154153
c.bench_function(format!("write_to_{:#0X}", offset).as_str(), |b| {
155154
b.iter(|| {
156155
black_box(&memory)
157-
.write_to(address, &mut Cursor::new(&mut image), ACCESS_SIZE)
156+
.write_volatile_to(address, &mut image.as_mut_slice(), ACCESS_SIZE)
158157
.unwrap()
159158
})
160159
});
161160

162161
c.bench_function(format!("write_to_file_{:#0X}", offset).as_str(), |b| {
163162
b.iter(|| {
164163
black_box(&memory)
165-
.write_to(address, &mut file_to_write, ACCESS_SIZE)
164+
.write_volatile_to(address, &mut file_to_write, ACCESS_SIZE)
166165
.unwrap()
167166
})
168167
});
169168

170169
c.bench_function(format!("write_exact_to_{:#0X}", offset).as_str(), |b| {
171170
b.iter(|| {
172171
black_box(&memory)
173-
.write_all_to(address, &mut Cursor::new(&mut image), ACCESS_SIZE)
172+
.write_all_volatile_to(address, &mut image.as_mut_slice(), ACCESS_SIZE)
174173
.unwrap()
175174
})
176175
});

‎src/bitmap/mod.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@ pub(crate) mod tests {
267267
dirty_offset += step;
268268

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

279280
// Test `read_exact_from`.
281+
#[allow(deprecated)] // test of deprecated functions
280282
h.test_access(bytes, dirty_offset, BUF_SIZE, |m, addr| {
281283
m.read_exact_from(addr, &mut Cursor::new(&buf), BUF_SIZE)
282284
.unwrap()

‎src/bytes.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ pub trait Bytes<A> {
281281
/// * `src` - Copy from `src` into the container.
282282
/// * `count` - Copy `count` bytes from `src` into the container.
283283
#[deprecated(
284-
note = "Use `.read_from_volatile` or the functions of the `ReadVolatile` trait instead"
284+
note = "Use `.read_volatile_from` or the functions of the `ReadVolatile` trait instead"
285285
)]
286286
fn read_from<F>(&self, addr: A, src: &mut F, count: usize) -> Result<usize, Self::E>
287287
where
@@ -299,7 +299,7 @@ pub trait Bytes<A> {
299299
/// * `src` - Copy from `src` into the container.
300300
/// * `count` - Copy exactly `count` bytes from `src` into the container.
301301
#[deprecated(
302-
note = "Use `.read_exact_from_volatile` or the functions of the `ReadVolatile` trait instead"
302+
note = "Use `.read_exact_volatile_from` or the functions of the `ReadVolatile` trait instead"
303303
)]
304304
fn read_exact_from<F>(&self, addr: A, src: &mut F, count: usize) -> Result<(), Self::E>
305305
where
@@ -314,7 +314,7 @@ pub trait Bytes<A> {
314314
/// * `dst` - Copy from the container to `dst`.
315315
/// * `count` - Copy `count` bytes from the container to `dst`.
316316
#[deprecated(
317-
note = "Use `.write_to_volatile` or the functions of the `WriteVolatile` trait instead"
317+
note = "Use `.write_volatile_to` or the functions of the `WriteVolatile` trait instead"
318318
)]
319319
fn write_to<F>(&self, addr: A, dst: &mut F, count: usize) -> Result<usize, Self::E>
320320
where
@@ -332,7 +332,7 @@ pub trait Bytes<A> {
332332
/// * `dst` - Copy from the container to `dst`.
333333
/// * `count` - Copy exactly `count` bytes from the container to `dst`.
334334
#[deprecated(
335-
note = "Use `.write_all_to_volatile` or the functions of the `WriteVolatile` trait instead"
335+
note = "Use `.write_all_volatile_to` or the functions of the `WriteVolatile` trait instead"
336336
)]
337337
fn write_all_to<F>(&self, addr: A, dst: &mut F, count: usize) -> Result<(), Self::E>
338338
where

‎src/guest_memory.rs‎

Lines changed: 152 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,9 @@ use std::sync::Arc;
5252
use crate::address::{Address, AddressValue};
5353
use crate::bitmap::{Bitmap, BS, MS};
5454
use crate::bytes::{AtomicAccess, Bytes};
55+
use crate::io::{ReadVolatile, WriteVolatile};
5556
use crate::volatile_memory::{self, VolatileSlice};
57+
use crate::GuestMemoryError;
5658

5759
static MAX_ACCESS_CHUNK: usize = 4096;
5860

@@ -665,6 +667,146 @@ pub trait GuestMemory {
665667
}
666668
}
667669

670+
/// Reads up to `count` bytes from an object and writes them into guest memory at `addr`.
671+
///
672+
/// Returns the number of bytes written into guest memory.
673+
///
674+
/// # Arguments
675+
/// * `addr` - Begin writing at this address.
676+
/// * `src` - Copy from `src` into the container.
677+
/// * `count` - Copy `count` bytes from `src` into the container.
678+
///
679+
/// # Examples
680+
///
681+
/// * Read bytes from /dev/urandom (uses the `backend-mmap` feature)
682+
///
683+
/// ```
684+
/// # #[cfg(feature = "backend-mmap")]
685+
/// # {
686+
/// # use vm_memory::{Address, GuestMemory, Bytes, GuestAddress, GuestMemoryMmap};
687+
/// # use std::fs::File;
688+
/// # use std::path::Path;
689+
/// #
690+
/// # let start_addr = GuestAddress(0x1000);
691+
/// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)])
692+
/// # .expect("Could not create guest memory");
693+
/// # let addr = GuestAddress(0x1010);
694+
/// # let mut file = if cfg!(unix) {
695+
/// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom");
696+
/// # file
697+
/// # } else {
698+
/// # File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe"))
699+
/// # .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe")
700+
/// # };
701+
///
702+
/// gm.read_volatile_from(addr, &mut file, 128)
703+
/// .expect("Could not read from /dev/urandom into guest memory");
704+
///
705+
/// let read_addr = addr.checked_add(8).expect("Could not compute read address");
706+
/// let rand_val: u32 = gm
707+
/// .read_obj(read_addr)
708+
/// .expect("Could not read u32 val from /dev/urandom");
709+
/// # }
710+
/// ```
711+
fn read_volatile_from<F>(&self, addr: GuestAddress, src: &mut F, count: usize) -> Result<usize>
712+
where
713+
F: ReadVolatile,
714+
{
715+
self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
716+
// Check if something bad happened before doing unsafe things.
717+
assert!(offset <= count);
718+
719+
let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
720+
721+
let mut vslice = region.get_slice(caddr, len)?;
722+
723+
src.read_volatile(&mut vslice)
724+
.map_err(GuestMemoryError::from)
725+
})
726+
}
727+
728+
/// Reads up to `count` bytes from the container at `addr` and writes them it into guest memory.
729+
///
730+
/// Returns the number of bytes written into guest memory.
731+
///
732+
/// # Arguments
733+
/// * `addr` - Begin reading from this address.
734+
/// * `dst` - Copy from guest memory to `dst`.
735+
/// * `count` - Copy `count` bytes from guest memory to `dst`.
736+
fn write_volatile_to<F>(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<usize>
737+
where
738+
F: WriteVolatile,
739+
{
740+
self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
741+
// Check if something bad happened before doing unsafe things.
742+
assert!(offset <= count);
743+
744+
let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
745+
let vslice = region.get_slice(caddr, len)?;
746+
747+
// For a non-RAM region, reading could have side effects, so we
748+
// must use write_all().
749+
dst.write_all_volatile(&vslice)?;
750+
751+
Ok(len)
752+
})
753+
}
754+
755+
/// Reads exactly `count` bytes from an object and writes them into guest memory at `addr`.
756+
///
757+
/// # Errors
758+
///
759+
/// Returns an error if `count` bytes couldn't have been copied from `src` to guest memory.
760+
/// Part of the data may have been copied nevertheless.
761+
///
762+
/// # Arguments
763+
/// * `addr` - Begin writing at this address.
764+
/// * `src` - Copy from `src` into guest memory.
765+
/// * `count` - Copy exactly `count` bytes from `src` into guest memory.
766+
fn read_exact_volatile_from<F>(
767+
&self,
768+
addr: GuestAddress,
769+
src: &mut F,
770+
count: usize,
771+
) -> Result<()>
772+
where
773+
F: ReadVolatile,
774+
{
775+
let res = self.read_volatile_from(addr, src, count)?;
776+
if res != count {
777+
return Err(Error::PartialBuffer {
778+
expected: count,
779+
completed: res,
780+
});
781+
}
782+
Ok(())
783+
}
784+
785+
/// Reads exactly `count` bytes from the container at `addr` and writes them into guest memory.
786+
///
787+
/// # Errors
788+
///
789+
/// Returns an error if `count` bytes couldn't have been copied from guest memory to `dst`.
790+
/// Part of the data may have been copied nevertheless.
791+
///
792+
/// # Arguments
793+
/// * `addr` - Begin reading from this address.
794+
/// * `dst` - Copy from guest memory to `dst`.
795+
/// * `count` - Copy exactly `count` bytes from guest memory to `dst`.
796+
fn write_all_volatile_to<F>(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<()>
797+
where
798+
F: WriteVolatile,
799+
{
800+
let res = self.write_volatile_to(addr, dst, count)?;
801+
if res != count {
802+
return Err(Error::PartialBuffer {
803+
expected: count,
804+
completed: res,
805+
});
806+
}
807+
Ok(())
808+
}
809+
668810
/// Get the host virtual address corresponding to the guest address.
669811
///
670812
/// Some [`GuestMemory`](trait.GuestMemory.html) implementations, like `GuestMemoryMmap`,
@@ -985,8 +1127,6 @@ mod tests {
9851127
#[cfg(feature = "backend-mmap")]
9861128
use crate::GuestAddress;
9871129
#[cfg(feature = "backend-mmap")]
988-
use std::io::Cursor;
989-
#[cfg(feature = "backend-mmap")]
9901130
use std::time::{Duration, Instant};
9911131

9921132
use vmm_sys_util::tempfile::TempFile;
@@ -1026,7 +1166,7 @@ mod tests {
10261166
let count: usize = 0x20;
10271167
assert_eq!(
10281168
0x20_usize,
1029-
mem.read_from(offset, &mut Cursor::new(&image), count)
1169+
mem.read_volatile_from(offset, &mut image.as_slice(), count)
10301170
.unwrap()
10311171
);
10321172
}
@@ -1181,19 +1321,24 @@ mod tests {
11811321
assert!(mem.write_obj(obj, addr).is_ok());
11821322
assert!(mem.read_obj::<ZeroSizedStruct>(addr).is_ok());
11831323

1184-
assert_eq!(mem.read_from(addr, &mut Cursor::new(&image), 0).unwrap(), 0);
1324+
assert_eq!(
1325+
mem.read_volatile_from(addr, &mut image.as_slice(), 0)
1326+
.unwrap(),
1327+
0
1328+
);
11851329

11861330
assert!(mem
1187-
.read_exact_from(addr, &mut Cursor::new(&image), 0)
1331+
.read_exact_volatile_from(addr, &mut image.as_slice(), 0)
11881332
.is_ok());
11891333

11901334
assert_eq!(
1191-
mem.write_to(addr, &mut Cursor::new(&mut image), 0).unwrap(),
1335+
mem.write_volatile_to(addr, &mut image.as_mut_slice(), 0)
1336+
.unwrap(),
11921337
0
11931338
);
11941339

11951340
assert!(mem
1196-
.write_all_to(addr, &mut Cursor::new(&mut image), 0)
1341+
.write_all_volatile_to(addr, &mut image.as_mut_slice(), 0)
11971342
.is_ok());
11981343
}
11991344

‎src/mmap.rs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,7 @@ impl<B: Bitmap> Bytes<MemoryRegionAddress> for GuestRegionMmap<B> {
273273
F: Read,
274274
{
275275
let maddr = addr.raw_value() as usize;
276+
#[allow(deprecated)] // function itself is deprecated
276277
self.as_volatile_slice()
277278
.unwrap()
278279
.read_from::<F>(maddr, src, count)
@@ -318,6 +319,7 @@ impl<B: Bitmap> Bytes<MemoryRegionAddress> for GuestRegionMmap<B> {
318319
F: Read,
319320
{
320321
let maddr = addr.raw_value() as usize;
322+
#[allow(deprecated)] // function itself is deprecated
321323
self.as_volatile_slice()
322324
.unwrap()
323325
.read_exact_from::<F>(maddr, src, count)
@@ -363,6 +365,7 @@ impl<B: Bitmap> Bytes<MemoryRegionAddress> for GuestRegionMmap<B> {
363365
F: Write,
364366
{
365367
let maddr = addr.raw_value() as usize;
368+
#[allow(deprecated)] // function itself is deprecated
366369
self.as_volatile_slice()
367370
.unwrap()
368371
.write_to::<F>(maddr, dst, count)
@@ -408,6 +411,7 @@ impl<B: Bitmap> Bytes<MemoryRegionAddress> for GuestRegionMmap<B> {
408411
F: Write,
409412
{
410413
let maddr = addr.raw_value() as usize;
414+
#[allow(deprecated)] // function itself is deprecated
411415
self.as_volatile_slice()
412416
.unwrap()
413417
.write_all_to::<F>(maddr, dst, count)
@@ -1177,7 +1181,7 @@ mod tests {
11771181
File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")).unwrap()
11781182
};
11791183
gm.write_obj(!0u32, addr).unwrap();
1180-
gm.read_exact_from(addr, &mut file, mem::size_of::<u32>())
1184+
gm.read_exact_volatile_from(addr, &mut file, mem::size_of::<u32>())
11811185
.unwrap();
11821186
let value: u32 = gm.read_obj(addr).unwrap();
11831187
if cfg!(unix) {
@@ -1186,8 +1190,8 @@ mod tests {
11861190
assert_eq!(value, 0x0090_5a4d);
11871191
}
11881192

1189-
let mut sink = Vec::new();
1190-
gm.write_all_to(addr, &mut sink, mem::size_of::<u32>())
1193+
let mut sink = vec![0; mem::size_of::<u32>()];
1194+
gm.write_all_volatile_to(addr, &mut sink.as_mut_slice(), mem::size_of::<u32>())
11911195
.unwrap();
11921196
if cfg!(unix) {
11931197
assert_eq!(sink, vec![0; mem::size_of::<u32>()]);

0 commit comments

Comments
 (0)