Generify parts of mmap.rs #312

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

Summary of the PR

Mainly, 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.

While we're at it, generalize the impl Bytes for GuestRegionMmap into impl<R: GuestMemoryRegion> Bytes for R, because really that impls didn't actually make use of any specifics of the GuestRegionMmap struct.

This PR is the backward-compatible subset of #317. In that context, it is 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). However, short-term is is also a required step for enabling things like #315 without necessarily requiring changes in core vm-memory code.

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.

src/guest_memory.rs

roypat · 2025-03-27T12:52:32Z

Hey @bonzini, since you chimed in over at #321 the other day, any thoughts on this one? :o

bonzini · 2025-03-27T13:19:00Z

While we're at it, generalize the impl Bytes for GuestRegionMmap into impl<R: GuestMemoryRegion> Bytes for R, because really that impls didn't actually make use of any specifics of the GuestRegionMmap struct.

I would need to double check that. IIRC if QEMU was ever going to use vm-memory, which actually could happen now, it needed something custom to handle access to an assigned device's BAR.

roypat · 2025-03-27T13:28:37Z

While we're at it, generalize the impl Bytes for GuestRegionMmap into impl<R: GuestMemoryRegion> Bytes for R, because really that impls didn't actually make use of any specifics of the GuestRegionMmap struct.

I would need to double check that. IIRC if QEMU was ever going to use vm-memory, which actually could happen now, it needed something custom to handle access to an assigned device's BAR.

Ah, that's good to know, thanks! I can rework it, so that an additional empty marker trait is required to be implemented, e.g. GuestMemoryRegionBytes: GuestMemoryRegion and then only have impl<R: GuestMemoryRegionBytes> Bytes for R { ... }?

roypat · 2025-03-27T13:49:39Z

While we're at it, generalize the impl Bytes for GuestRegionMmap into impl<R: GuestMemoryRegion> Bytes for R, because really that impls didn't actually make use of any specifics of the GuestRegionMmap struct.

I would need to double check that. IIRC if QEMU was ever going to use vm-memory, which actually could happen now, it needed something custom to handle access to an assigned device's BAR.

Ah, that's good to know, thanks! I can rework it, so that an additional empty marker trait is required to be implemented, e.g. GuestMemoryRegionBytes: GuestMemoryRegion and then only have impl<R: GuestMemoryRegionBytes> Bytes for R { ... }?

Done :)

bonzini · 2025-03-27T14:05:14Z

That doesn't hurt, yes :) I will tell you tomorrow anyway!

andreeaflorescu · 2025-07-01T09:04:11Z

The error type handling is a bit wack, but this is needed to preserve
backwards compatibility

Do we really want to keep backwards compatibility at all costs? I am afraid that we'll end up with even more technical debt. How hard would it be to update to the new vm-memory version if we were NOT to keep the backwards compatibility?

andreeaflorescu · 2025-06-03T09:04:10Z

src/guest_memory.rs

@@ -408,7 +408,9 @@ 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> {


What is the motivation behind adding the default implementation? Mostly curious, but it would be good to also mention it in the commit message.

Mainly because it's possible, and it means that downstream impls of the trait need to do less (in my case, I was implementing a mock for some testing and got a bit annoyed at having to unneccessarily write out things that I didn't very much care about in that scenario). Looking at it now, num_regions can also get a default impl, so I'll add that too haha

Can you also add a comment in the commit message that this can be useful when writing unit tests?

Yup, done! (sorry took me a while longer to push because I got lost trying to untangle the error types and found some other unrelated issues that was making my cargo check-all-features compile testing fail)

roypat · 2025-07-01T09:29:47Z

The error type handling is a bit wack, but this is needed to preserve
backwards compatibility

Do we really want to keep backwards compatibility at all costs? I am afraid that we'll end up with even more technical debt. How hard would it be to update to the new vm-memory version if we were NOT to keep the backwards compatibility?

Honestly, the backward compatibility went out the window a few revisions ago... I'll just make this change and clean it up properly. We already have quite a few breaking changes accumulated on main anyway, and this should be a fairly minor one.

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). Mainly a QoL improvement to make mock impls of this trait for tests less verbose. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

roypat · 2025-07-01T12:55:15Z

(ik you dont like the coverage update being its own commit, but the test only started failing after the rebase on main, so am not sure what exact commit (or combination thereof) is causing the change)

Similar to find_region(), we can provide an inefficient default impl to make downstream impls of the trait less verbose in scenarios that really do not care (e.g. mock impls for tests). Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

This modules is intended for all functionality that relates to contiguous regions of guest memory. This differentiates it from `guest_memory`, as that is about a holistic view of guest memory, and from `mmap`, which is specifically about guest memory regions backed by mmap VMAs. 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`. Error types get cleaned up in a follow up commit, the messy state of GuestRegionError containing both collection and region constrution specific errors is transitory. 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>

Move some tests that are all about the invariants of GuestRegionCollection constructors to region.rs, where they can be run without the need for the backend-mmap feature (by instead using a mock memory region). While we're at it, fix these tests calling from_regions twice, but from_arc_regions never. Remove some test cases that are superfluous, because since the `regions` field of `GuestRegionCollection` is private, all construction needs to go through `from_regions`/`from_arc_regions`, and testing that wrappers around these functions uphold the invariants of the wrapped functions is not very useful. test_memory and create_vec_with_regions were the same test, so deduplicate while moving to region.rs. Generally, most of these tests could be moved to region.rs, given sufficient mocking of the memory region. I've somewhat arbitrarily drawn the line at "only transfer tests where the mock only needs GuestAddress and length", which roughly translates to "move tests where we are testing the default implementations of the GuestMemory and GuestMemoryRegion traits, which are not overwritten in the mmap-based implementations". Of course, we could write a mock that implements actual allocation of memory via std::alloc::alloc, but at that point we'd be testing the mock more than actual vm-memory code (and start loosing coverage of the mmap implementations). Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

These tests only test properties of the GuestMemoryAtomic implementation that are independent of the actual M: GuestMemory being used. So simplify it to drop the dependency on backend-mmap. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

There is only a single error condition, so we can indicate the error case with an Option. First step towards untangling the region specific errors from the region collection specific errors. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Turn the error type defined in region.rs into one specifically to GuestRegionCollections, and move the non-collection related errors into a new error type that is specific to the `from_ranges*` family of functions (these are the only functions that can return both a region construction error, as well as a collection error). The `from_ranges*` family of functions is publicly exported, but in reality only used in tests, so breakage of this should be limited. Since we're breaking things anyway, remove the reexport of GuestRegionCollectionError as `vm_memory::Error`. It made no sense for this one to have this special designation. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

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

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

ShadowCurse reviewed Feb 19, 2025

View reviewed changes

This was referenced Mar 10, 2025

Misc cleanups: move mmap files and replace cfg unix/windows with target_family #314

Merged

GuestRegionMmap: extend for supporting guest_memfd #315

Open

roypat force-pushed the generic-region-container branch from c76a6ca to ae4c4f6 Compare March 17, 2025 11:12

roypat changed the title ~~Generalize GuestMemoryMmap to arbitrary GuestMemoryRegions~~ Generify parts of mmap.rs Mar 17, 2025

roypat requested review from ShadowCurse and andreeaflorescu and removed request for ShadowCurse March 17, 2025 15:04

ShadowCurse previously approved these changes Mar 19, 2025

View reviewed changes

roypat force-pushed the generic-region-container branch from ae4c4f6 to 5e4e1d4 Compare March 21, 2025 11:12

roypat force-pushed the generic-region-container branch from e7d74ea to bc4e643 Compare March 27, 2025 13:49

roypat dismissed ShadowCurse’s stale review via f3ce283 April 11, 2025 09:44

roypat force-pushed the generic-region-container branch from f3ce283 to 04b4509 Compare April 16, 2025 06:17

roypat force-pushed the generic-region-container branch 2 times, most recently from d05a37e to 1c6454f Compare May 22, 2025 13:19

roypat mentioned this pull request May 30, 2025

Prepare v0.17.0 release #326

Closed

4 tasks

andreeaflorescu reviewed Jul 1, 2025

View reviewed changes

roypat force-pushed the generic-region-container branch from 1c6454f to 2cbbee1 Compare July 1, 2025 09:55

roypat force-pushed the generic-region-container branch 2 times, most recently from bb6f105 to 6e84b7f Compare July 1, 2025 11:50

roypat requested a review from andreeaflorescu July 1, 2025 11:50

roypat force-pushed the generic-region-container branch 2 times, most recently from 257d1cd to abe773d Compare July 1, 2025 12:54

roypat force-pushed the generic-region-container branch from abe773d to 784c535 Compare July 1, 2025 13:42

roypat added 9 commits July 2, 2025 07:47

add naive default impl for GuestMemory::num_regions

c1f9a01

Similar to find_region(), we can provide an inefficient default impl to make downstream impls of the trait less verbose in scenarios that really do not care (e.g. mock impls for tests). Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

chore: update coverage

9f31516

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

roypat force-pushed the generic-region-container branch from 784c535 to 9f31516 Compare July 2, 2025 06:59

andreeaflorescu approved these changes Jul 3, 2025

View reviewed changes

ShadowCurse approved these changes Jul 3, 2025

View reviewed changes

roypat merged commit b03b103 into rust-vmm:main Jul 3, 2025
2 checks passed

roypat mentioned this pull request Jul 3, 2025

fix up #[cfg(...)] related to test code that uses dirty bitmaps #331

Merged

4 tasks

Generify parts of mmap.rs #312

Generify parts of mmap.rs #312

Uh oh!

Conversation

roypat commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the PR

Requirements

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roypat commented Mar 27, 2025

Uh oh!

bonzini commented Mar 27, 2025

Uh oh!

roypat commented Mar 27, 2025

Uh oh!

roypat commented Mar 27, 2025

Uh oh!

bonzini commented Mar 27, 2025

Uh oh!

andreeaflorescu commented Jul 1, 2025

Uh oh!

andreeaflorescu Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

roypat Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

andreeaflorescu Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

roypat Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

roypat commented Jul 1, 2025

Uh oh!

roypat commented Jul 1, 2025

Uh oh!

Uh oh!

Uh oh!

roypat commented Feb 10, 2025 •

edited

Loading