-
Notifications
You must be signed in to change notification settings - Fork 110
Preliminary virtual memory work #339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
XanClic
wants to merge
3
commits into
rust-vmm:main
Choose a base branch
from
XanClic:iommu-preliminary-work
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+201
−13
Open
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,6 +455,93 @@ pub trait GuestMemory { | |
.ok_or(Error::InvalidGuestAddress(addr)) | ||
.and_then(|(r, addr)| r.get_slice(addr, count)) | ||
} | ||
|
||
/// Returns an iterator over [`VolatileSlice`](struct.VolatileSlice.html)s, together covering | ||
/// `count` bytes starting at `addr`. | ||
/// | ||
/// Iterating in this way is necessary because the given address range may be fragmented across | ||
/// multiple [`GuestMemoryRegion`]s. | ||
/// | ||
/// The iterator’s items are wrapped in [`Result`], i.e. errors are reported on individual | ||
/// items. If there is no such error, the cumulative length of all items will be equal to | ||
/// `count`. If `count` is 0, an empty iterator will be returned. | ||
fn get_slices<'a>( | ||
&'a self, | ||
addr: GuestAddress, | ||
count: usize, | ||
) -> GuestMemorySliceIterator<'a, Self> { | ||
GuestMemorySliceIterator { | ||
mem: self, | ||
addr, | ||
count, | ||
} | ||
} | ||
} | ||
|
||
/// Iterates over [`VolatileSlice`]s that together form a guest memory area. | ||
/// | ||
/// Returned by [`GuestMemory::get_slices()`]. | ||
#[derive(Debug)] | ||
pub struct GuestMemorySliceIterator<'a, M: GuestMemory + ?Sized> { | ||
roypat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// Underlying memory | ||
mem: &'a M, | ||
/// Next address in the guest memory area | ||
addr: GuestAddress, | ||
/// Remaining bytes in the guest memory area | ||
count: usize, | ||
} | ||
|
||
impl<'a, M: GuestMemory + ?Sized> GuestMemorySliceIterator<'a, M> { | ||
/// Helper function for [`<Self as Iterator>::next()`]. | ||
/// | ||
/// Get the next slice (i.e. the one starting from `self.addr` with a length up to | ||
/// `self.count`) and update the internal state. | ||
/// | ||
/// # Safety | ||
/// | ||
/// This function does not reset to `self.count` to 0 in case of error, i.e. will not stop | ||
/// iterating. Actual behavior after an error is ill-defined, so the caller must check the | ||
/// return value, and in case of an error, reset `self.count` to 0. | ||
/// | ||
/// (This is why this function exists, so this resetting can be done in a single central | ||
/// location.) | ||
unsafe fn do_next(&mut self) -> Option<Result<VolatileSlice<'a, MS<'a, M>>>> { | ||
if self.count == 0 { | ||
return None; | ||
} | ||
|
||
let Some((region, start)) = self.mem.to_region_addr(self.addr) else { | ||
return Some(Err(Error::InvalidGuestAddress(self.addr))); | ||
}; | ||
|
||
let cap = region.len() - start.raw_value(); | ||
let len = std::cmp::min(cap, self.count as GuestUsize); | ||
|
||
self.count -= len as usize; | ||
self.addr = match self.addr.overflowing_add(len as GuestUsize) { | ||
(x @ GuestAddress(0), _) | (x, false) => x, | ||
(_, true) => return Some(Err(Error::GuestAddressOverflow)), | ||
}; | ||
|
||
Some(region.get_slice(start, len as usize)) | ||
} | ||
} | ||
|
||
impl<'a, M: GuestMemory + ?Sized> Iterator for GuestMemorySliceIterator<'a, M> { | ||
type Item = Result<VolatileSlice<'a, MS<'a, M>>>; | ||
|
||
fn next(&mut self) -> Option<Self::Item> { | ||
// SAFETY: | ||
// We reset `self.count` to 0 on error | ||
match unsafe { self.do_next() } { | ||
Some(Ok(slice)) => Some(Ok(slice)), | ||
other => { | ||
// On error (or end), reset to 0 so iteration remains stopped | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right! |
||
self.count = 0; | ||
other | ||
} | ||
} | ||
} | ||
} | ||
|
||
impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T { | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reimplement
try_access
in terms ofget_slices
, to avoid the duplication of the iteration implementation? Or even deprecatetry_access
in favor ofget_slices
, since it seems to me to be the more powerful of the two?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can be deprecated, I don’t have a stake in that game. :)
try_access()
is slightly different in that it passes theGuestMemoryRegion
plus the address in that region, not the slice, so I don’t think it can be implemented viaget_slices()
. However, at least some (if not most)try_access()
users only really use that region to generate a slice, so those could indeed be replaced byget_slices()
.So we can definitely try and see whether the
try_access()
calls across rust-vmm (vm-memory, vm-virtio, vhost) could all be replaced withget_slices()
, and if so, indeed deprecate it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a try for vm-memory, and turns out the problem is less operating on VolatileSlice instead of GuestRegion, but more than the iterator doesn't give us the
offset
for free, and the fact that the iterator yieldsResult
. But nothing that isn't solved using try_fold (compile-tested only):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it helps, we could of course add that, e.g. as part of
Iterator::Item
, though I’m happy with thetry_fold()
solution, too.