[RFC] Make mmap_unix and mmap_xen compatible #317
Conversation
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
roypat
force-pushed
the
confluence
branch
4 times, most recently
from
dcb79f7 to
d1df9ba
Compare
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
This trait implementation of GuestMemoryRegion for `GuestRegionMmap`does not actually make use of the specifics of `GuestRegionMmap`, so can be completely generic in terms of `GuestMemoryRegion`. This allows us to move it to guest_memory.rs, eliminating one further instance of code depending on exactly one of mmap_unix.rs and mmap_xen.rs being compiled in. Replace .unwrap() with error propagation via `?`, as we no longer know that we are dealing with a specific GuestMemoryRegion impl that always implements as_volatile_slice(). Signed-off-by: Patrick Roy <[email protected]>
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 <[email protected]>
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 <[email protected]>
This avoids duplicating all the variants across the unix/xen specific error types. Signed-off-by: Patrick Roy <[email protected]>
Currently, on xen the guest base in tracked in two different places. GuestRegionMmap, defined in mmap.rs, and (almost) each of the implemented of XenMmapTrait. As a first step for eliminating this duplication, add getters so that the guest base stored in the XenMmapTrait trait object stored in MmapXen can be extracted. Signed-off-by: Patrick Roy <[email protected]>
Xen's MmapRegion struct already stores a `GuestAddress` inside of it, so there's no need to wrap it into a tuple of `(MmapRegion, GuestAddress)` to implement this trait on (which is essentially what happens in `mmap.rs`) - in fact, duplicating the GuestAddress seems wrong to be because I don't see any mechanism for keeping them in-sync. Do not duplication over the functions defined on mmap::GuestRegionMmap, because there already exist functions with colliding names in mmap_xen. Signed-off-by: Patrick Roy <[email protected]>
This is a xen specific version of `GuestMemoryMmap`. This makes the xen module independent of the cfg'd types in mmap.rs, meaning we are almost ready to allow enabling both Mmap and Xen support at the same time. Signed-off-by: Patrick Roy <[email protected]>
Currently, these tests are run either with the Xen backend, or the mmap backend, depending on which cargo features are enabled. But since the xen feature disables mmap_unix, effectively this means if we do cargo test --all-features, only the xen backend is tested. Refactor our testing story to support running tests with _all_ available backends, so that in a world where both the xen and non-xen backends can coexist, tests will be run for both if --all-features is passed. This won't win beauty contests, but it works for now. Signed-off-by: Patrick Roy <[email protected]>
`test_atomic_accesses` in `guest_memory.rs` is exactly the same as `test_atomic_accesses` in `mmap.rs`, so delete the former. Signed-off-by: Patrick Roy <[email protected]>
There are a lot of backend-mmap dependent tests in guest_memory.rs, so have them also use the new capabilities to run with all supported backends. Signed-off-by: Patrick Roy <[email protected]>
This is some duplication, but it once and for all eliminates all cfg fuckery and code that expects precisely one module to be defined. Admittedly, for windows/unix we could have stuck with the cfg stuff, but I wanted a clean slate for now. The windows parts are compile-tested only, as I do not have a windows system available. The handling of various from_range functions is based on the assumption that these were only used in test code and nowhere else (hence all the preceding refactors of test code). In unit tests, their use has been eliminated, and doc tests now got a cfg(unix. not(feature="xen")) so that they only run when mmap_unix is available. This is fine, because they are examples of how to use specific functions, and not any serious tests (e.g. dont need to run for all supported backends). Signed-off-by: Patrick Roy <[email protected]>
Add a specific error type for the GuestMemoryMmap related functions in mmap_unix.rs, to remove them from the global GuestRegionError. This was the last blocker to Xen/Mmap coexistence. Signed-off-by: Patrick Roy <[email protected]>
Now that all tension between these two modules is resolves (e.g. no more code depends on exactly one of these being defined, and tests can support iterating over multiple supported backends), drop all the `cfg(not(feature = "xen"))` from across the code base. Signed-off-by: Patrick Roy <[email protected]>
Now that --all-features actually includes all possible vm-memory code, cargo test --all-features also runs all possible tests. So drop CI steps that were running with a subset of features, as they just re-run subsets of tests now. Signed-off-by: Patrick Roy <[email protected]>
It seems that now with all our cargo features compatible, clippy is checking more code in CI. Fix a warning about an error variant being suffixed with the enum name in mmap_xen.rs that only popped up now. Signed-off-by: Patrick Roy <[email protected]>
Now that all features are additive, collect coverage with the xen feature enabled. Signed-off-by: Patrick Roy <[email protected]>
Apparently enabling the xen feature made it worse, not better :( Signed-off-by: Patrick Roy <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR enables the use of structs from the modules
mmap_unixandmmap_xenat the same time, resolving their mutual exclusivity. Particularly, one can now use both xen and non-xen backends at the same time (e.g. no longer is there a need to make a decision at compile time).If the xen feature is enabled, all unit tests run for both xen and non-xen backends.
The windows bits are only compile tested (there's a separate compiler issue for windows in the bitmap module,
I'll submit a separate patch for that on Mondaywhich'll get fixed by #318).There's probably a lot that can be improved still, e.g. there's more duplication between unix and windows support now than probably needed, but as a proposal it's good enough I think. I also probably messed up re-exports every now and then - I'll try to compile some downstreams against this on Monday as well :)
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.