Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions #312

roypat · 2025-02-10T17:18:46Z

Summary of the PR

Add the concept of a GuestRegionCollection, which just manages a list
of some GuestMemoryRegion impls. Functionality wise, it offers the
same implementations as GuestMemoryMmap. As a result, turn
GuestMemoryMmap into a type alias for GuestRegionCollection with a
fixed R = GuestRegionMmap.

Long term, this can be a first step towards resolving the incompatibility between the unix and xen features (currently, the code in mmap.rs only works if exactly one of the modules mmap_windows, mmap_xen and mmap_unix exist, since they all define roughly the same types which are then imported by the mmap module without renames)

The last commit is only tangentially related, but I ran into it when moving around the error type, and I like having PRs that remove more lines than they add, so I included it here xP

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR have Signed-Off-By trailers (with
git commit -s), and the commit message has max 60 characters for the
summary and max 75 characters for each description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

This function can be default-implemented in terms of `GuestMemory::iter()`. Downstream impls can overwrite this more specialized and efficient versions of course (such as GuestMemoryMmap using a binary search). Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Add the concept of a `GuestRegionCollection`, which just manages a list of some `GuestMemoryRegion` impls. Functionality wise, it offers the same implementations as `GuestMemoryMmap`. As a result, turn `GuestMemoryMmap` into a type alias for `GuestRegionCollection` with a fixed `R = GuestRegionMmap`. The error type handling is a bit wack, but this is needed to preserve backwards compatibility: The error type of GuestRegionCollection must match what GuestMemoryMmap historically returned, so the error type needs to be lifted from mmap.rs - however, it contains specific variants that are only relevant to GuestMemoryMmap, so cfg those behind the `backend-mmap` feature flag (as to why this specific error type gets be privilege of just being reexported as `Error` from this crate: No idea, but its pre-existing, and changing it would be a breaking change). Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Some tests that were explicitly testing for error conditions used converted errors to strings to determine whether two errors are the same (by saying they're only the same if their string representation was identical). Replace this with more roboust assertions on `matches!`. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

ShadowCurse · 2025-02-19T19:09:17Z

src/guest_memory.rs

+        if regions.is_empty() {
+            return Err(GuestRegionCollectionError::NoMemoryRegion);
+        }


Is it really an error to provide no regions?

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.

ShadowCurse · 2025-02-19T19:11:38Z

src/guest_memory.rs

+    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)
+    }


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

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.

ShadowCurse · 2025-02-19T19:12:11Z

src/guest_memory.rs

+    /// * `base`: base address of the region to be removed
+    /// * `size`: size of the region to be removed
+    pub fn remove_region(
+        &self,


Same here, why not &mut self

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.

ShadowCurse · 2025-02-19T19:14:10Z

src/guest_memory.rs

+            }
+        }
+
+        Err(GuestRegionCollectionError::NoMemoryRegion)


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?

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.

ShadowCurse · 2025-02-19T19:15:54Z

src/guest_memory.rs

+        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())


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.

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.

ShadowCurse · 2025-02-19T19:17:49Z

src/guest_memory.rs

@@ -586,6 +587,168 @@ pub trait GuestMemory {
    }
 }

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


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.

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

Just regions.rs?

roypat requested review from alexandruag, bonzini and jiangliu as code owners February 10, 2025 17:18

roypat force-pushed the generic-region-container branch from 99d1a51 to df86175 Compare February 10, 2025 17:19

roypat force-pushed the generic-region-container branch from df86175 to c76a6ca Compare February 10, 2025 17:20

ShadowCurse reviewed Feb 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions #312

Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions #312

roypat commented Feb 10, 2025 •

edited

Loading

ShadowCurse Feb 19, 2025

roypat Feb 20, 2025

ShadowCurse Feb 20, 2025

ShadowCurse Feb 19, 2025

roypat Feb 20, 2025

ShadowCurse Feb 20, 2025

ShadowCurse Feb 19, 2025

roypat Feb 20, 2025

ShadowCurse Feb 20, 2025

ShadowCurse Feb 19, 2025

roypat Feb 20, 2025

ShadowCurse Feb 20, 2025

ShadowCurse Feb 19, 2025

roypat Feb 20, 2025

ShadowCurse Feb 20, 2025

ShadowCurse Feb 19, 2025

roypat Feb 20, 2025

ShadowCurse Feb 20, 2025

Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions #312

Are you sure you want to change the base?

Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions #312

Conversation

roypat commented Feb 10, 2025 • edited Loading

Summary of the PR

Requirements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roypat commented Feb 10, 2025 •

edited

Loading