add vhost-user-vsock device emulation #216

stefano-garzarella · 2022-09-28T12:53:47Z

This PR is based on #7.

@harshanavkis implemented a vhost-user-vsock device for a GSoC 2021 project mentored by me and @fidencio.

His PR was in a great state, I just rebased and completed the following items to be able to merge this project that could be very useful:

updated some dependencies (e.g. vm-memory, etc.)
replaced the deprecated App::from_yaml()
fixed some new warnings from clippy
addressed @stsquad comments (don't panic when CONNECT 1234 is wrong, use virtio-vsock 0.1.0 crate from @lauralt )
avoided sending credit updates when the credit hasn't changed
added SPDX-License-Identifier in each vsock file

stefano-garzarella · 2022-09-29T07:51:44Z

v2:

fixed more clippy warnings (I run the nightly clippy to be sure I cover also the new ones)
fixed commit title length issue

TODO:

extend test coverage (this PR drops the coverage by 20%, would be nice to extend the tests. I'll try to do my best in the next days, but if someone wants to help, it will be great)

stefano-garzarella · 2022-09-29T08:29:00Z

v3:

fixed another commit length issue
reverted a change that I added by mistake in v2

TODO:

extend test coverage (this PR drops the coverage by 20%, would be nice to extend the tests. I'll try to do my best in the next days, but if someone wants to help, it will be great)

stefano-garzarella · 2022-09-30T07:59:31Z

v4:

added more test to reach ~70% of coverage (it was ~55% before the new tests).
removed some unwrap() and fixed little issues discovered while writing tests

I hope the coverage is not that bad, and we can merge this. In the future we can try to increase it, adding more moks.

stsquad

I've tested the daemon on my setup and everything looks pretty good to me. I have added a few comments about removing commented out code and cleaning up the println -> info!,warn! and debug! macros. I think the remaining TODOs and further increasing the coverage can be done once this is merged.

vsock/src/thread_backend.rs

vsock/src/main.rs

vsock/src/vhu_vsock_thread.rs

stefano-garzarella · 2022-09-30T15:06:21Z

v5:

replaced all println!() with dbg!(), info!(), or error!()
initialized env_logger in main()
removed old comments, useless code, and code commented
added test in main.rs to check that VhostUserDaemon's workers and VhostUserVsockBackend's threads are the same (they should be always the same if the code is correct)

stefano-garzarella · 2022-09-30T15:08:05Z

@stsquad thanks for testing and reviews! I just addressed all your comments and pushed the new version

mathieupoirier · 2022-10-11T01:11:40Z

I will start looking into this pull request in the coming days. Given its sheer size I expect the review process to take several days. I will clearly let you know when I am done.

stefano-garzarella · 2022-10-11T07:45:35Z

I will start looking into this pull request in the coming days. Given its sheer size I expect the review process to take several days. I will clearly let you know when I am done.

@mathieupoirier thank you very much! Yep, take your time. I have taken up Harsha's work, so some part may be unknown to me too ;-)

vireshk

About code coverage, I looked at kcov_output/ to see what all is missed and there are things we can easily cover I think. For example main.rs::start_backend_server() and a lot of other code in vhu_vsock_thread.rs which is not covered as of now in tests.

vireshk · 2022-10-11T05:35:09Z

vsock/src/main.rs

@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: Apache-2.0 or BSD-3-Clause


During the reviews of I2C crate, a lot of feedback was received, mostly from Andreea. I am mostly going to pass that on here. I (and maybe @mathieupoirier too) would like to keep the same kind of formatting / parsing / layout for all crates in here, which makes going through them much easier when they look alike.

For example for the current crates, we only mentioned Apache-2.0. If we want to add BSD too, then maybe add it to all of them ?

I'm not the original author of this code, so I just added the SPDX-License-Identifier following what Harsha specified in his Cargo.toml.

I'm open to change on Apache-2.0 only, but we need an approval from @harshanavkis.

Fine by me.

@harshanavkis thanks for your ACK, I'll prepare a PR to re-license vhost-user-vsock as Apache-2.0 only.

vireshk · 2022-10-11T05:41:35Z

vsock/src/main.rs

+use vhost::{vhost_user, vhost_user::Listener};
+use vhost_user_backend::VhostUserDaemon;
+use vhu_vsock::{VhostUserVsockBackend, VsockArgs, VsockConfig};
+use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap};


We have separated the headers intro three groups in existing crates, first one for standard headers, second one for external ones (like vhost), and last one for vsock local ones. All groups separated by blank lines.

In i2c and gpio I see use log... in the first group. IIUC log is an external crate. Should it be in the second group?

vsock/src/main.rs

vsock/src/thread_backend.rs

vsock/src/vhu_vsock.rs

stefano-garzarella · 2022-10-12T07:47:29Z

About code coverage, I looked at kcov_output/ to see what all is missed and there are things we can easily cover I think. For example main.rs::start_backend_server() and a lot of other code in vhu_vsock_thread.rs which is not covered as of now in tests.

I agree, but can we postpone?
I already spent a lot of time to rebase Harsha's work, so my plan will be to fix the simple things and open issues to postpone the rest. (maybe some student can start with tests or refactoring, or I can do it when I'll have more time)
Is that okay?

vireshk · 2022-10-12T08:26:00Z

I agree, but can we postpone? I already spent a lot of time to rebase Harsha's work, so my plan will be to fix the simple things and open issues to postpone the rest. (maybe some student can start with tests or refactoring, or I can do it when I'll have more time) Is that okay?

It is always better / easier to fix things at review time as they are lost otherwise. But I do get what you are saying here.

Please do whatever changes you can to the current patches, based on current comments, and we will get it merged. I can also try to spend some cycles on this then and raise issues too.

stefano-garzarella · 2022-10-12T08:38:59Z

It is always better / easier to fix things at review time as they are lost otherwise. But I do get what you are saying here.

I totally agree with you, but since I'm not the author, it's like doing a review every time to see if what I'm testing or changing makes sense :-)

Please do whatever changes you can to the current patches, based on current comments, and we will get it merged. I can also try to spend some cycles on this then and raise issues too.

Thank you for understanding, I will do my best.
If you need help maintaining this workspace, I can allocate some time to help and add myself to CODEOWNERS.

This commit introduces a vhost-user-vsock device that enables communicaton between an application running in the guest i.e inside a VM and an application running on the host i.e outside the VM. The device exposes unix sockets to which the VMM and host-side applications connect to. Applications in the guest communicate over VM sockets. Applicaitons on the host connect to the unix socket i.e communicate over AF_UNIX sockets. Signed-off-by: Harshavardhan Unnibhavi <harshanavkis@gmail.com> [sgarzare: rebased, updated Cargo.lock, updated clap version to avoid build issues, and fixed clap issues with the new version] Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

clippy complains of some minor issues, probably because the new version has been improved. Let's fix them. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Update the dependencies aligning with the other crates in this workspace. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

clap::App::from_yaml() is deprecated since clap-3.0.0, let's switch to clap::Parser. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Replace our VsockPacket with the new one implemented in the virtio-vsock crate. Based on Alex Bennée and Laura Loghin commit: stsquad@6752266608dd To achieve this the following changes where made: - deleted the internal packet.rs - convert the send_pkt/recv_pkt helpers to be BitmapSlice aware - update push from LocalTxBuf - tweak a few API calls due to minor diffs Fixed tests and moved some helpers from the removed vsock/src/packet.rs file. Co-developed-by: Laura Loghin <lauralg@amazon.com> Co-developed-by: Alex Bennée <alex.bennee@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Harsha specified `license = "Apache-2.0 OR BSD-3-Clause"` in vsock/Cargo.toml, so let's add a properly SPDX tag in the vsock files. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Remove old comments, useless code, and code commented. Avoid println!() to print messages and replaced with dbg!(), info!(), or error!(). Also init `env_logger` like other daemons in the main(). Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Instead of panic if the local peer doesn't send "connect PORT\n" correctly, let's print an error, ignoring the command. Print an error message also when allocating local port fails. Reported-by: Alex Bennée <alex.bennee@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

If we don't remove any bytes from the buffer, it's useless to send a credit update since the credit hasn't changed. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Instead of panic, let's propagate errors or print warnings where we can't. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

VIRTIO spec defines `guest_cid` in LE byte order. Let's introduce VirtioVsockConfig based on the virtio-spec 1.2. For now it contains only `guest_cid`, but in the future it could contain other fields. Let's also handle correctly `offset` and `size`, returning an empty Vec if they are not valid. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Add several tests to get good coverage. Unfortunately, it's a lot of new code and not easy to test everything. We may add more mocks and tests in the future. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

This is a binary crate and most of the types shouldn't be visible out of this crate. Change also documentation with ///. If we would like to export some types as part of a library in the future, we will adjust the visibility accordingly. Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

VsockArgs is used only by main.rs, so we can move its definition there. Let's also move the test. Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

In several places the "vsock_" prefix doesn't add much and makes names longer, let's remove it where it's not needed. Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Following the other crates, reorder the use declaration in 3 groups: 1. standard library 2. external crates 3. local crates Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

stefano-garzarella · 2022-10-12T13:23:04Z

v6:

addressed most of @vireshk requests
- reordered use declarations
- removed "vsock_" prefix where is redundant
- moved VsockArgs in main.rs
- used pub(crate) for types used only by our binary
- and other small changes merged on previous commits

TODO:

extend test coverage (I prefer as a follow up in a new PR, I'll open an issue to track)

stefano-garzarella · 2022-10-12T13:33:02Z

@vireshk @stsquad @mathieupoirier I just pressed the button to request Alex's review and GH messed up other requests. Sorry about that ;-)

mathieupoirier · 2022-10-12T14:57:32Z

I see that Viresh has beaten me to the punch on reviewing this pull request. There is no need to do things twice, especially with a patchset this big, and as such will not continue with this work.

vireshk · 2022-10-12T15:03:34Z

I see that Viresh has beaten me to the punch on reviewing this pull request. There is no need to do things twice, especially with a patchset this big, and as such will not continue with this work.

You can approve it for now, I have already done so and we will continue to modify it to make it similar to other crates and fix any more issues we find later.

stefano-garzarella requested review from mathieupoirier, stsquad and vireshk as code owners September 28, 2022 12:53

stefano-garzarella mentioned this pull request Sep 30, 2022

vhost-user-vsock implementation #7

Closed

stsquad requested changes Sep 30, 2022

View reviewed changes

stsquad previously approved these changes Oct 3, 2022

View reviewed changes

vireshk requested changes Oct 11, 2022

View reviewed changes

vsock/src/vhu_vsock.rs Show resolved Hide resolved

harshanavkis and others added 12 commits October 12, 2022 14:21

vsock: fix clippy warnings

69882e1

clippy complains of some minor issues, probably because the new version has been improved. Let's fix them. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: update Cargo.toml dependencies

a1daeec

Update the dependencies aligning with the other crates in this workspace. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: replace the deprecated App::from_yaml

896d346

clap::App::from_yaml() is deprecated since clap-3.0.0, let's switch to clap::Parser. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: add SPDX-License-Identifier in each file

8fa597e

Harsha specified `license = "Apache-2.0 OR BSD-3-Clause"` in vsock/Cargo.toml, so let's add a properly SPDX tag in the vsock files. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: send credit update only when needed

1605f54

If we don't remove any bytes from the buffer, it's useless to send a credit update since the credit hasn't changed. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: try to avoid panic as much as we can

167206d

Instead of panic, let's propagate errors or print warnings where we can't. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: add more tests and update coverage_score

a6d157f

Add several tests to get good coverage. Unfortunately, it's a lot of new code and not easy to test everything. We may add more mocks and tests in the future. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

stefano-garzarella added 4 commits October 12, 2022 15:17

vsock: move VsockArgs in main.rs

c452d1c

VsockArgs is used only by main.rs, so we can move its definition there. Let's also move the test. Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: remove "vsock_" prefix where is redundant

a76f9eb

In several places the "vsock_" prefix doesn't add much and makes names longer, let's remove it where it's not needed. Suggested-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

vsock: reorder use declarations

e6a85ff

Following the other crates, reorder the use declaration in 3 groups: 1. standard library 2. external crates 3. local crates Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

stefano-garzarella dismissed stsquad’s stale review via e6a85ff October 12, 2022 13:22

stefano-garzarella requested review from vireshk and stsquad and removed request for mathieupoirier, vireshk and stsquad October 12, 2022 13:30

vireshk approved these changes Oct 12, 2022

View reviewed changes

vireshk enabled auto-merge October 12, 2022 15:02

mathieupoirier approved these changes Oct 12, 2022

View reviewed changes

vireshk merged commit 1f77465 into rust-vmm:main Oct 12, 2022

This was referenced Oct 13, 2022

Improve code coverage of vsock crate #229

Closed

vsock: re-license as Apache-2.0 only #234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add vhost-user-vsock device emulation #216

add vhost-user-vsock device emulation #216

stefano-garzarella commented Sep 28, 2022

stefano-garzarella commented Sep 29, 2022

stefano-garzarella commented Sep 29, 2022

stefano-garzarella commented Sep 30, 2022

stsquad left a comment

stefano-garzarella commented Sep 30, 2022

stefano-garzarella commented Sep 30, 2022

mathieupoirier commented Oct 11, 2022

stefano-garzarella commented Oct 11, 2022

vireshk left a comment

vireshk Oct 11, 2022

stefano-garzarella Oct 12, 2022

harshanavkis Oct 13, 2022

stefano-garzarella Oct 13, 2022

vireshk Oct 11, 2022

stefano-garzarella Oct 12, 2022

stefano-garzarella Oct 12, 2022

stefano-garzarella commented Oct 12, 2022

vireshk commented Oct 12, 2022

stefano-garzarella commented Oct 12, 2022

stefano-garzarella commented Oct 12, 2022

stefano-garzarella commented Oct 12, 2022

mathieupoirier commented Oct 12, 2022

vireshk commented Oct 12, 2022

		@@ -0,0 +1,111 @@
		// SPDX-License-Identifier: Apache-2.0 or BSD-3-Clause

add vhost-user-vsock device emulation #216

add vhost-user-vsock device emulation #216

Conversation

stefano-garzarella commented Sep 28, 2022

stefano-garzarella commented Sep 29, 2022

stefano-garzarella commented Sep 29, 2022

stefano-garzarella commented Sep 30, 2022

stsquad left a comment

Choose a reason for hiding this comment

stefano-garzarella commented Sep 30, 2022

stefano-garzarella commented Sep 30, 2022

mathieupoirier commented Oct 11, 2022

stefano-garzarella commented Oct 11, 2022

vireshk left a comment

Choose a reason for hiding this comment

vireshk Oct 11, 2022

Choose a reason for hiding this comment

stefano-garzarella Oct 12, 2022

Choose a reason for hiding this comment

harshanavkis Oct 13, 2022

Choose a reason for hiding this comment

stefano-garzarella Oct 13, 2022

Choose a reason for hiding this comment

vireshk Oct 11, 2022

Choose a reason for hiding this comment

stefano-garzarella Oct 12, 2022

Choose a reason for hiding this comment

stefano-garzarella Oct 12, 2022

Choose a reason for hiding this comment

stefano-garzarella commented Oct 12, 2022

vireshk commented Oct 12, 2022

stefano-garzarella commented Oct 12, 2022

stefano-garzarella commented Oct 12, 2022

stefano-garzarella commented Oct 12, 2022

mathieupoirier commented Oct 12, 2022

vireshk commented Oct 12, 2022