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

Xen/mmapv2 #160

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Xen/mmapv2 #160

merged 5 commits into from
Jul 5, 2023

Conversation

vireshk
Copy link
Contributor

@vireshk vireshk commented May 19, 2023

vhost: Add support for generic memory mappings

vm-memory crate now supports Xen specific memory mappings via a special
feature: "xen". Modify all vhost crates to work with the new feature.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Copy link
Collaborator

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

@vireshk I have not found anything that jumped to my eye. However, I find it a bit hard to follow what is happening. I think a bit more detail in the commit messages and a few more details in the doc strings could be helpful to follow what happens here :).

crates/vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved
crates/vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved
crates/vhost/src/vhost_user/message.rs Show resolved Hide resolved
crates/vhost-user-backend/tests/vhost-user-server.rs Outdated Show resolved Hide resolved
@vireshk
Copy link
Contributor Author

vireshk commented Jun 30, 2023

Waiting for rust-vmm/vm-virtio#250 to get merged.

@vireshk vireshk force-pushed the xen/mmapv2 branch 4 times, most recently from a863253 to c65ea35 Compare July 4, 2023 05:23
Copy link
Collaborator

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

just some nit-picking...

crates/vhost/src/backend.rs Outdated Show resolved Hide resolved
crates/vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved
@vireshk
Copy link
Contributor Author

vireshk commented Jul 4, 2023

Fixes: #169

Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

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

The thing that is not clear to me is: if we enable xen then we will always only handle xen guests, and we could never have the case of handling both?

If this is the case, then we should document this compile-time feature like: if you enable it you can only handle xen guests.

crates/vhost/src/vhost_user/message.rs Outdated Show resolved Hide resolved
crates/vhost/src/vhost_user/message.rs Show resolved Hide resolved
crates/vhost/src/vhost_user/message.rs Show resolved Hide resolved
crates/vhost-user-backend/src/handler.rs Outdated Show resolved Hide resolved
crates/vhost-user-backend/CHANGELOG.md Outdated Show resolved Hide resolved
crates/vhost-user-backend/CHANGELOG.md Outdated Show resolved Hide resolved
@vireshk
Copy link
Contributor Author

vireshk commented Jul 4, 2023

The thing that is not clear to me is: if we enable xen then we will always only handle xen guests, and we could never have the case of handling both?

This is based on the decision taken in the vm-memory crate. In the very first version, I tried to support both kvm and Xen mappings together in a single build, but that was rejected in favor of simplifying the code and selecting one at compile time only.

And so yes, this is either Xen or kvm, but not both currently. Though it may change later, but current code is selected at build time only. Will document this.

The vm-memory crate now supports Xen specific memory mappings via a
special feature: "xen".

Add a corresponding feature for vhost crate and add support for Xen
memory regions. Update various dependencies to align to the same version
of vm-memory crate.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
@vireshk
Copy link
Contributor Author

vireshk commented Jul 4, 2023

@stefano-garzarella updated code, hopefully covered all your concerns apart from #160 (comment).

Automatically enable the VhostUserProtocolFeatures::XEN_MMAP feature for
backends for Xen specific builds. With these the backends don't need to
enable this feature and can directly support Xen.

Suggested-by: Erik Schilling <erik.schilling@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Release a new version with Xen support.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Migrate to a newer version of the vhost and other dependencies and add
support for xen memory mappings. Add a corresponding xen feature for
vhost-user-backend crate.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
@vireshk
Copy link
Contributor Author

vireshk commented Jul 5, 2023

Hmm, I have pushed my branch again: https://github.com/vireshk/vhost/tree/xen/mmapv2 , but somehow this pull request isn't fetching the changes automatically anymore. Any idea ?

@stefano-garzarella @jiangliu

Release a new version with Xen support.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
@vireshk
Copy link
Contributor Author

vireshk commented Jul 5, 2023

Hmm, I have pushed my branch again: https://github.com/vireshk/vhost/tree/xen/mmapv2 , but somehow this pull request isn't fetching the changes automatically anymore. Any idea ?

Okay, it worked now :)

@jiangliu jiangliu merged commit 8783a8d into rust-vmm:main Jul 5, 2023
23 checks passed
@vireshk vireshk deleted the xen/mmapv2 branch July 5, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants