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

[epilys]

Perform some cleanups/refactorings that should not result in functional changes.

The motivation for consolidating mmap stuff together is to perform future work on unifying xen/non-xen interfaces to allow compiling vm-memory with any combination of features.

cfg flags "unix" and "windows" are now aliases to target_family = "unix"
and target_family = "windows" respectively for backwards compatibility,
so use the non-legacy way of targeting those OS families.

Signed-off-by: Manos Pitsidianakis [email protected]

[epilys]

The interfaces don't change in this PR, it's easy to check this by patching the vm-memory dependency in a crate with this PR's branch:

[patch.crates-io]
vm-memory = { git = "https://github.com/epilys/vm-memory.git", branch = "rework-xen-interface" }

And cargo check should execute successfully.

[roypat]

Happy to see someone else interested in making the xen and unix worlds compatible! :D

fwiw, I also started some work towards this in #312, by generifying some of the code in mmap.rs to stop relying on exactly one of mmap_*.rs being define - does that roughly match your vision of how to go about this?

[epilys]

Happy to see someone else interested in making the xen and unix worlds compatible! :D

fwiw, I also started some work towards this in #312, by generifying some of the code in mmap.rs to stop relying on exactly one of mmap_*.rs being define - does that roughly match your vision of how to go about this?

Oops, completely missed your PR (because I had started this branch locally before you posted it and only picked it up again recently). I wouldn't make the memory region type generic, we would want it to be opaque so that it would be the same type under any hypervisor. The actual memory region kind would be a "hidden" implementation detail and depend on how the library consumer/user initializes the vm-memory api, on run-time. At least, that's what my thoughts were.

[roypat]

(preamble: no matter what we do long term, this PR makes sense, so if you do mv mmap.rs mmap/mod.rs we can just merge it imo)

Happy to see someone else interested in making the xen and unix worlds compatible! :D
fwiw, I also started some work towards this in #312, by generifying some of the code in mmap.rs to stop relying on exactly one of mmap_*.rs being define - does that roughly match your vision of how to go about this?

Oops, completely missed your PR (because I had started this branch locally before you posted it and only picked it up again recently). I wouldn't make the memory region type generic, we would want it to be opaque so that it would be the same type under any hypervisor. The actual memory region kind would be a "hidden" implementation detail and depend on how the library consumer/user initializes the vm-memory api, on run-time. At least, that's what my thoughts were.

Mh, interesting! I was actually thinking about it exactly the other way - e.g. I'd even love to separate out the Xen stuff from VolatileSlice, and instead add a second type (VolatileSliceXen or sth that is just a thin wrapper/decorator for VolatileSlice that does the xen mmaps on read/write) and add an associated type to GuestMemoryRegion that determines the volatile slice type. With a very long term vision that VolatileSlice could just become a repr(transparent) wrapper for iovec (which would be nice for doing vectored I/O on virtio descriptor chains for example, which could then be supported directly in vm-memory via the Read/WriteVolatile traits) - that is if there's a way to get rid of the Bitmap field (maybe via a decorator pattern, as Paolo once suggested?).

re: making the memory region type generic: Ideally, I would like vm-memory's API to be in such a way that theoretically, something like Xen support can be easily implemented outside of the core code, and for that goal, generifying things like GuestMemoryMmap (GuestRegionCollection in my PR) still makes sense imo (and also the generification of VolatileSlice would be in line with this goal).

And if downstream users that want to support multiple backends are written generically against trait GuestMemory, then the actual memory region would still be a hidden implementation detail, right?

[epilys]

And if downstream users that want to support multiple backends are written generically against trait GuestMemory, then the actual memory region would still be a hidden implementation detail, right?

By that you mean that vm-memory will hand out trait objects that hide the implementation details?

[roypat]

And if downstream users that want to support multiple backends are written generically against trait GuestMemory, then the actual memory region would still be a hidden implementation detail, right?

By that you mean that vm-memory will hand out trait objects that hide the implementation details?

vm-memory will hand out objects that implement specific traits (e.g. GuestMemory or GuestRegion), and so if a downstream only uses the interfaces of these traits, then the implementation details of which exact backend was initialized will be abstracted away

[epilys]

Right, I think we're talking about the same thing essentially. 🤔

Interface would look like this:

• vm-memory Types cannot be constructed via the public API, except for a "backend" object which is created by passing option on whether you want xen or mmap.
• The "backend" object hands out opaque Box<dyn GuestMemory> trait (example name) objects
• Downstream users can still implement the traits themselves (i.e. we would not Seal them)
• Other rust-vmm crates use trait objects only, not generics or associated types, so that they are hypervisor agnostic

[roypat]

Right, I think we're talking about the same thing essentially. 🤔

Mh, yeah, I think generally we are aligned - why trait objects instead of static dispatch though? Or rather, why do the type erasure between Xen and Mmap backends in vm-memory instead of downstream? (I admittedly also am not sure our traits are dyn compatible/object safe, because they have associated types 🤔)

Interface would look like this:

• vm-memory Types cannot be constructed via the public API, except for a "backend" object which is created by passing option on whether you want xen or mmap.

• The "backend" object hands out opaque Box<dyn GuestMemory> trait (example name) objects

• Downstream users can still implement the traits themselves (i.e. we would not Seal them)

• Other rust-vmm crates use trait objects only, not generics or associated types, so that they are hypervisor agnostic

[epilys]

The reasoning is, to have one build that works on any (compile-time) supported hypervisor, configured at runtime. We're investigating whether this usecase is reasonable to achieve upstream.

[roypat]

The reasoning is, to have one build that works on any (compile-time) supported hypervisor, configured at runtime. We're investigating whether this usecase is reasonable to achieve upstream.

Right, but why isn't that possible with static dispatch? E.g.

fn main() {
    /* parse arguments, determine runtime configuration */

   if (xen) {
        run(setup_xen_memory());
   } else {
        run(setup_mmap_memory());
   }
}

fn setup_xen_memory() -> impl GuestMemory {  /* actual type something like GuestRegionCollection<XenRegion> */
    todo!()
}

fn setup_mmap_memory() -> impl GuestMemory {  /* actual type something like GuestRegionCollection<MmapRegion> */
    todo!()
}

fn run<G: GuestMemory>() {
		todo!()
}

[epilys]

It's possible of course, but adding a new hypervisor means you'd have to change downstream code to handle it, right?

[roypat]

It's possible of course, but adding a new hypervisor means you'd have to change downstream code to handle it, right?

Wouldn't you have to do that either way? At the very least, downstreams would need to be recompiled against the new vm-memory version.

But say today vm-memory was written in terms of type erased trait objects and then it gains support for a new hypervisor (say, Xen). Then, how would a downstream actually make use of vm-memory's new Xen support without any code changes? There would still need to be something downstream that drives whatever vm-memory API hands out Box<dyn GuestMemory> instances, and instruct it to start handing out Xen mappings. And then downstream would still need to propagate APIs of its own (be it CLI arguments or whatever) so that users of the downstream crate can start requesting Xen mappings.

Besides, adding support for new hypervisors is an incredibly rare event 😅 - how much does it make sense to optimize for this, especially if the savings for downstream is just a couple lines of initialization code?

Now, I'm not saying we cannot have a utility API that offers a sort of builder pattern for handing out Box<dyn GuestMemory> ¹, implemented on top of dedicated APIs for creating mappings for specific hypervisors, but I don't think it makes sense to start with this. Because we can build such a layer on top of an API based on static dispatch (and in fact, downstream can do so without any changes in vm-memory), but we cannot do it the other way around.

Footnotes

1. with the asterisk of GuestMemory currently not being dyn compatible/object safe: https://docs.rs/vm-memory/latest/vm_memory/guest_memory/trait.GuestMemory.html#dyn-compatibility - but this seems resolvable by sprinkling some where Self: Sized for specific functions ↩

[stsquad]

The reasoning is, to have one build that works on any (compile-time) supported hypervisor, configured at runtime. We're investigating whether this usecase is reasonable to achieve upstream.

Right, but why isn't that possible with static dispatch? E.g.

fn main() {
    /* parse arguments, determine runtime configuration */

   if (xen) {
        run(setup_xen_memory());
   } else {
        run(setup_mmap_memory());
   }
}

fn setup_xen_memory() -> impl GuestMemory {  /* actual type something like GuestRegionCollection<XenRegion> */
    todo!()
}

fn setup_mmap_memory() -> impl GuestMemory {  /* actual type something like GuestRegionCollection<MmapRegion> */
    todo!()
}

fn run<G: GuestMemory>() {
		todo!()
}

When your referring to the downstream code you mean the consumers of vm-memory? e.g. A vhost-device binary?

The aim is to get automatic support from the vm-memory and vhost-user crates without the binary having to specify anything more then what features it wants enabled.

[roypat]

When your referring to the downstream code you mean the consumers of vm-memory? e.g. A vhost-device binary?

yea

The aim is to get automatic support from the vm-memory and vhost-user crates without the binary having to specify anything more then what features it wants enabled.

Just to clarify, by "features" you don't mean "cargo features", right? Because that would put the choice back at compile time, which I thought was precisely what @epilys did not want.

Could you maybe sketch out roughly how you would like to drive the vm-memory API? I'm probably just missing something, but I don't see how vhost-user binary crates could automatically support a new hypervisor without code changes just because vm-memory adds support for it.

[roypat]

Is this a deserialization thing? The vhost backend gets a description of the memory regions, and just passes that description on to vm-memory to reconstruct its view of the regions, and this should be transparent to the backend binary? E.g. if the binary suddenly receives descriptions for regions for/from a new hypervisor then it just forwards them to vm-memory all the same, and vm-memory then constructs an opaque objects to give back to the vhost binary?

[stsquad]

When your referring to the downstream code you mean the consumers of vm-memory? e.g. A vhost-device binary?

yea

The aim is to get automatic support from the vm-memory and vhost-user crates without the binary having to specify anything more then what features it wants enabled.

Just to clarify, by "features" you don't mean "cargo features", right? Because that would put the choice back at compile time, which I thought was precisely what @epilys did not want.

Currently the cargo features select which mode to support, the idea of this change is to enable multiple modes that can be supported.

Could you maybe sketch out roughly how you would like to drive the vm-memory API? I'm probably just missing something, but I don't see how vhost-user binary crates could automatically support a new hypervisor without code changes just because vm-memory adds support for it.

Its the vhost-user crate which will create the slice in response to the feature negotiation and the memory messages. I'm happy for the logic to live in there. The point is the details of memory mapping should be invisible to the binaries themselves.

[roypat]

Could you maybe sketch out roughly how you would like to drive the vm-memory API? I'm probably just missing something, but I don't see how vhost-user binary crates could automatically support a new hypervisor without code changes just because vm-memory adds support for it.

Its the vhost-user crate which will create the slice in response to the feature negotiation and the memory messages. I'm happy for the logic to live in there. The point is the details of memory mapping should be invisible to the binaries themselves.

Okay, I think I got it now. I was missing the part with the shim layer in vhost-user that'll translate vhost-user messages into vm-memory objects. So in that case let me revise my answer to your earlier question...

When your referring to the downstream code you mean the consumers of vm-memory? e.g. A vhost-device binary?

... to instead be "consumers of vm-memory that have to construct guest memory mappings", e.g. not the binaries, but only the vhost-user library crate.

So the shim layer in vhost-user can then construct GuestMemoryXen or GuestMemoryMmap based on which vhost messages it receives, cram those into a Box¹ or an enum², and pass that off to the binary - new backends would only need to be added to the shim in vhost-user, but the binaries will be non the wiser

Footnotes

1. I'm putting an asterisk on this because I checked and our traits are indeed not object safe/dyn compatible today. It's because of all the generic functions they have. We'd probably need to have a second set of traits that re-implements the functions with generics via dynamic dispatch instead ↩

2. If you really want to make the guest memory impl opaque to the binaries, you can hand them some struct OpaqueMem { inner: MyEnumOverBackends } that only implements GuestMemory and nothing else ↩