Skip to content
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

Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions #312

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
### Added

- \[[#311](https://github.com/rust-vmm/vm-memory/pull/311)\] Allow compiling without the ReadVolatile and WriteVolatile implementations
- \[[#312](https://github.com/rust-vmm/vm-memory/pull/312)\] `GuestRegionContainer`, a generic container of `GuestMemoryRegion`s, generalizing `GuestMemoryMmap` (which
is now a type alias for `GuestRegionContainer<GuestRegionMmap>`)

### Changed

Expand Down
167 changes: 166 additions & 1 deletion src/guest_memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,10 @@ pub trait GuestMemory {
fn num_regions(&self) -> usize;

/// Returns the region containing the specified address or `None`.
fn find_region(&self, addr: GuestAddress) -> Option<&Self::R>;
fn find_region(&self, addr: GuestAddress) -> Option<&Self::R> {
self.iter()
.find(|region| addr >= region.start_addr() && addr <= region.last_addr())
}

/// Gets an iterator over the entries in the collection.
///
Expand Down Expand Up @@ -584,6 +587,168 @@ pub trait GuestMemory {
}
}

/// Errors that can occur when creating a memory map.
#[derive(Debug, thiserror::Error)]
pub enum GuestRegionCollectionError {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole GuestRegionCollection looks like it can be moved into it's own module. Also maybe rename to just Regions? I assume there are no other regions other than memory regions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, I'd be open to having it in its own module. What should we call it though?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just regions.rs?

/// Adding the guest base address to the length of the underlying mapping resulted
/// in an overflow.
#[error("Adding the guest base address to the length of the underlying mapping resulted in an overflow")]
#[cfg(feature = "backend-mmap")]
InvalidGuestRegion,
/// Error creating a `MmapRegion` object.
#[error("{0}")]
#[cfg(feature = "backend-mmap")]
MmapRegion(crate::mmap::MmapRegionError),
/// No memory region found.
#[error("No memory region found")]
NoMemoryRegion,
/// Some of the memory regions intersect with each other.
#[error("Some of the memory regions intersect with each other")]
MemoryRegionOverlap,
/// The provided memory regions haven't been sorted.
#[error("The provided memory regions haven't been sorted")]
UnsortedMemoryRegions,
}

/// [`GuestMemory`](trait.GuestMemory.html) implementation based on a homogeneous collection
/// of [`GuestMemoryRegion`] implementations.
///
/// Represents a sorted set of non-overlapping physical guest memory regions.
#[derive(Debug)]
pub struct GuestRegionCollection<R: GuestMemoryRegion> {
regions: Vec<Arc<R>>,
}

impl<R: GuestMemoryRegion> Default for GuestRegionCollection<R> {
fn default() -> Self {
Self {
regions: Vec::new(),
}
}
}

impl<R: GuestMemoryRegion> Clone for GuestRegionCollection<R> {
fn clone(&self) -> Self {
GuestRegionCollection {
regions: self.regions.iter().map(Arc::clone).collect(),
}
}
}

impl<R: GuestMemoryRegion> GuestRegionCollection<R> {
/// Creates an empty `GuestMemoryMmap` instance.
pub fn new() -> Self {
Self::default()
}

/// Creates a new [`GuestRegionCollection`] from a vector of regions.
///
/// # Arguments
///
/// * `regions` - The vector of regions.
/// The regions shouldn't overlap, and they should be sorted
/// by the starting address.
pub fn from_regions(
mut regions: Vec<R>,
) -> std::result::Result<Self, GuestRegionCollectionError> {
Self::from_arc_regions(regions.drain(..).map(Arc::new).collect())
}

/// Creates a new [`GuestRegionCollection`] from a vector of Arc regions.
///
/// Similar to the constructor `from_regions()` as it returns a
/// [`GuestRegionCollection`]. The need for this constructor is to provide a way for
/// consumer of this API to create a new [`GuestRegionCollection`] based on existing
/// regions coming from an existing [`GuestRegionCollection`] instance.
///
/// # Arguments
///
/// * `regions` - The vector of `Arc` regions.
/// The regions shouldn't overlap and they should be sorted
/// by the starting address.
pub fn from_arc_regions(
regions: Vec<Arc<R>>,
) -> std::result::Result<Self, GuestRegionCollectionError> {
if regions.is_empty() {
return Err(GuestRegionCollectionError::NoMemoryRegion);
}
Comment on lines +672 to +674

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really an error to provide no regions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is just moved from mmap.rs without changes. There might be improvements that can be made to the actual API, but that's out of scope here I'd say, because ideally we'd keep backwards compatibility here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


for window in regions.windows(2) {
let prev = &window[0];
let next = &window[1];

if prev.start_addr() > next.start_addr() {
return Err(GuestRegionCollectionError::UnsortedMemoryRegions);
}

if prev.last_addr() >= next.start_addr() {
return Err(GuestRegionCollectionError::MemoryRegionOverlap);
}
}

Ok(Self { regions })
}

/// Insert a region into the `GuestMemoryMmap` object and return a new `GuestMemoryMmap`.
///
/// # Arguments
/// * `region`: the memory region to insert into the guest memory object.
pub fn insert_region(
&self,
region: Arc<R>,
) -> std::result::Result<GuestRegionCollection<R>, GuestRegionCollectionError> {
let mut regions = self.regions.clone();
regions.push(region);
regions.sort_by_key(|x| x.start_addr());

Self::from_arc_regions(regions)
}
Comment on lines +696 to +705

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just take &mut self and mutate in place. The check for region intersection can be moved into a separate function and used here and in the from_arc_regions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is just moved from mmap.rs without changes. There might be improvements that can be made to the actual API, but that's out of scope here I'd say, because ideally we'd keep backwards compatibility here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


/// Remove a region from the [`GuestRegionCollection`] object and return a new `GuestRegionCollection`
/// on success, together with the removed region.
///
/// # Arguments
/// * `base`: base address of the region to be removed
/// * `size`: size of the region to be removed
pub fn remove_region(
&self,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why not &mut self

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is just moved from mmap.rs without changes. There might be improvements that can be made to the actual API, but that's out of scope here I'd say, because ideally we'd keep backwards compatibility here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

base: GuestAddress,
size: GuestUsize,
) -> std::result::Result<(GuestRegionCollection<R>, Arc<R>), GuestRegionCollectionError> {
if let Ok(region_index) = self.regions.binary_search_by_key(&base, |x| x.start_addr()) {
if self.regions.get(region_index).unwrap().len() == size {
let mut regions = self.regions.clone();
let region = regions.remove(region_index);
return Ok((Self { regions }, region));
}
}

Err(GuestRegionCollectionError::NoMemoryRegion)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an error to remove a non existing region? Maybe return type should be (Self, Option<Arc<R>>) where optional is the removed region if it existed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is just moved from mmap.rs without changes. There might be improvements that can be made to the actual API, but that's out of scope here I'd say, because ideally we'd keep backwards compatibility here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}
}

impl<R: GuestMemoryRegion> GuestMemory for GuestRegionCollection<R> {
type R = R;

fn num_regions(&self) -> usize {
self.regions.len()
}

fn find_region(&self, addr: GuestAddress) -> Option<&R> {
let index = match self.regions.binary_search_by_key(&addr, |x| x.start_addr()) {
Ok(x) => Some(x),
// Within the closest region with starting address < addr
Err(x) if (x > 0 && addr <= self.regions[x - 1].last_addr()) => Some(x - 1),
_ => None,
};
index.map(|x| self.regions[x].as_ref())
Comment on lines +738 to +744

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many regions there can possibly be? I think a simple for loop where we check if address is in the region will be as fast in general case and more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is also just moved from mmap.rs, so I also err'd on the side of keeping things as they are. The default implementation for this trait function indeed just uses a simple loop.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}

fn iter(&self) -> impl Iterator<Item = &Self::R> {
self.regions.iter().map(AsRef::as_ref)
}
}

impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
type E = Error;

Expand Down
5 changes: 3 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ pub use endian::{Be16, Be32, Be64, BeSize, Le16, Le32, Le64, LeSize};
pub mod guest_memory;
pub use guest_memory::{
Error as GuestMemoryError, FileOffset, GuestAddress, GuestAddressSpace, GuestMemory,
GuestMemoryRegion, GuestUsize, MemoryRegionAddress, Result as GuestMemoryResult,
GuestMemoryRegion, GuestRegionCollection, GuestRegionCollectionError as Error, GuestUsize,
MemoryRegionAddress, Result as GuestMemoryResult,
};

pub mod io;
Expand All @@ -66,7 +67,7 @@ mod mmap_windows;
pub mod mmap;

#[cfg(feature = "backend-mmap")]
pub use mmap::{Error, GuestMemoryMmap, GuestRegionMmap, MmapRegion};
pub use mmap::{GuestMemoryMmap, GuestRegionMmap, MmapRegion};
#[cfg(all(feature = "backend-mmap", feature = "xen", unix))]
pub use mmap::{MmapRange, MmapXenFlags};

Expand Down
Loading