add vhost-vdpa support (in-kernel) #33

stefano-garzarella · 2021-03-24T16:19:06Z

This PR add the vhost-vdpa support (in-kernel).
Since it requires IOTLB message support for DMA map/unmap features, the first patches added that support to vhost-kern (VHOST_IOTLB, VHOST_BACKEND, vhost_msg_vs), let me know if you prefer a separated PR for that.

While writing the tests, I found an issue in the kernel. The patches are upstream 1 2 but not landed in stable kernels yet, so I left the vdpa.set_config_call() commented in the test to avoid kernel panics.

I tested with Linux v5.10 and I used vdpa_sim module (net device):

modprobe vhost-vdpa
modprobe vdpa-sim   # vdpa-sim-net on Linux v5.11+

cargo test --features=vhost-kern,vhost-vdpa

Then /dev/vhost-vdpa-0 is available and you can run the tests.

Signed-off-by: Stefano Garzarella [email protected]

stefano-garzarella · 2021-03-24T17:05:07Z

The tests fail because /dev/vhost-vdpa-0 doesn't exist and I should load the kernel modules.

Where is the best place? In the test itself?

jiangliu · 2021-05-18T02:05:27Z

The tests fail because /dev/vhost-vdpa-0 doesn't exist and I should load the kernel modules.

Where is the best place? In the test itself?

@andreeaflorescu Is there anyway to load specific kernel modules on the CI machines? The vhost unit test cases have dependency on vhost-xxx/vdpa drivers.

stefano-garzarella · 2021-05-18T14:04:16Z

@jiangliu if we can't load kernel modules, maybe I can replace the tests without involving the vdpa simulator. But would be cool to use it.

andreeaflorescu · 2021-05-18T14:20:09Z

Can we do this in a reproducible way? The CI machines are started from a fresh image nightly, so it would be nice if this can be done via the CI. Is it enough to call a command before running the tests? Do we need a specific kernel?

stefano-garzarella · 2021-05-18T14:33:46Z

Can we do this in a reproducible way?

If you mean the kernel issue, it is now fixed and landed to stable kernels too.

The CI machines are started from a fresh image nightly, so it would be nice if this can be done via the CI. Is it enough to call a command before running the tests? Do we need a specific kernel?

Yep, we need at least Linux 5.7, but from Linux 5.11 the kernel module changed the name.

So, from Linux 5.7 to Linux 5.10 we should do the following:

modprobe vhost-vdpa
modprobe vdpa-sim

with Linux >= 5.11:

modprobe vhost-vdpa
modprobe vdpa-sim-net

andreeaflorescu · 2021-05-18T15:11:57Z

That's sad, the CI machine runs Linux 5.4:

~$ uname -sr
Linux 5.4.0-1048-aws

It might take a while to upgrade to a newer Linux version because the machines are refreshed nightly, and we would need to recreate the images. I'll create an internal (AWS) ticket to upgrade the images. Is there something that we can do in the meantime to not block this PR?

stefano-garzarella · 2021-05-18T15:36:25Z

That's sad, the CI machine runs Linux 5.4:

:-(

~$ uname -sr
Linux 5.4.0-1048-aws
It might take a while to upgrade to a newer Linux version because the machines are refreshed nightly, and we would need to recreate the images. I'll create an internal (AWS) ticket to upgrade the images. Is there something that we can do in the meantime to not block this PR?

If you agree, I can simply skip the tests if /dev/vhost-vdpa-0 is not found.

stefano-garzarella · 2021-05-20T08:38:36Z

If you agree, I can simply skip the tests if /dev/vhost-vdpa-0 is not found.

@andreeaflorescu @jiangliu what do you think?

We can also add some tests without involving the kernel module, but VhostKernVdpa mainly provides a wrapper around the IOCTLs, so I'm not sure they are useful.

andreeaflorescu · 2021-05-21T08:42:07Z

@stefano-garzarella I synced with @jiangliu on Slack and we decided the following:

for now we skip the tests if /dev/vhost-vdpa-0 is not found (as you suggested)
we open an issue to track the update of the CI machine (I can do that)

Does that sound good?

stefano-garzarella · 2021-05-21T10:04:52Z

@stefano-garzarella I synced with @jiangliu on Slack and we decided the following:
* for now we skip the tests if `/dev/vhost-vdpa-0` is not found (as you suggested)

* we open an issue to track the update of the CI machine (I can do that)
Does that sound good?

@andreeaflorescu yep, I agree on both :-)
I'll update this PR next week.

Thanks for the help!

stefano-garzarella · 2021-05-24T12:48:53Z

@jiangliu @andreeaflorescu just pushed new changes:

uncommented the function that produced the kernel panic (vdpa.set_config_call) since the fixes are merged in all stable branches
skipped tests if /dev/vhost-vdpa-0 is not found (added a single commit for this, so we can revert it when we have a new kernel in the CI)

CI fails with AssertionError: Coverage drops by 0.60%., since the new traits are implemented only for vhost_kern can I patch coverage_config_x86_64.json in this way?

diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json
index 2b2c164..3a95f1c 100644
--- a/coverage_config_x86_64.json
+++ b/coverage_config_x86_64.json
@@ -1 +1 @@
-{"coverage_score": 81.2, "exclude_path": "src/vhost_kern/", "crate_features": "vhost-user-master,vhost-user-slave"}
+{"coverage_score": 80.6, "exclude_path": "src/vhost_kern/", "crate_features": "vhost-user-master,vhost-user-slave"}

jiangliu · 2021-05-25T02:25:51Z

@jiangliu @andreeaflorescu just pushed new changes:

uncommented the function that produced the kernel panic (vdpa.set_config_call) since the fixes are merged in all stable branches

skipped tests if /dev/vhost-vdpa-0 is not found (added a single commit for this, so we can revert it when we have a new kernel in the CI)

CI fails with AssertionError: Coverage drops by 0.60%., since the new traits are implemented only for vhost_kern can I patch coverage_config_x86_64.json in this way?
diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json
index 2b2c164..3a95f1c 100644
--- a/coverage_config_x86_64.json
+++ b/coverage_config_x86_64.json
@@ -1 +1 @@
-{"coverage_score": 81.2, "exclude_path": "src/vhost_kern/", "crate_features": "vhost-user-master,vhost-user-slave"}
+{"coverage_score": 80.6, "exclude_path": "src/vhost_kern/", "crate_features": "vhost-user-master,vhost-user-slave"}

Sure, it's ok to patch the coverage_cofnig_x86_64.json.

stefano-garzarella · 2021-05-25T07:38:34Z

@jiangliu thanks! done :-)

slp

Requires a rebase, but otherwise LGTM.

stefano-garzarella · 2021-06-17T10:22:19Z

Requires a rebase, but otherwise LGTM.

@slp thanks!

The only conflict so far is the coverage.
Should I rebase or can it be fixed during the merge?

slp · 2021-06-17T10:51:40Z

Requires a rebase, but otherwise LGTM.

@slp thanks!

The only conflict so far is the coverage.
Should I rebase or can it be fixed during the merge?

@stefano-garzarella If possible, please do a rebase.

stefano-garzarella · 2021-06-18T08:22:22Z

@jiangliu @andreeaflorescu Do you think it's okay now or do I need to make more changes?
If okay can we merge it?

Thanks! :-)

andreeaflorescu · 2021-06-22T08:47:09Z

@stefano-garzarella we added vdpa support on the CI host, so now I think we can also add a specialized CI pipeline to run the tests you're adding. I can add a POC with how to add the specialized pipeline and we can work together to integrate it. How does that sound?

stefano-garzarella · 2021-06-22T09:49:43Z

@stefano-garzarella we added vdpa support on the CI host, so now I think we can also add a specialized CI pipeline to run the tests you're adding.

@andreeaflorescu yeah!

I can add a POC with how to add the specialized pipeline and we can work together to integrate it. How does that sound?

It sounds great :-) feel free to reach me on rust-vmm slack workspace (user: sgarzare) to sync our works.

VHOST_ACCESS_* and VHOST_IOTLB_* constants are assigned to c_uchar fields and VHOST_IOTLB_MSG to a c_int field. Let's set the proper types to avoid casts when assigning them. Signed-off-by: Stefano Garzarella <[email protected]>

These two new VHOST_IOTLB message types are supported by vhost-vdpa. Signed-off-by: Stefano Garzarella <[email protected]>

Add structs and tests for the C 'struct vhost_msg_v2' type supported by vhost-kern. Signed-off-by: Stefano Garzarella <[email protected]>

vhost-kern support these ioctls to set/get backend features related to IOTLB message support. Signed-off-by: Stefano Garzarella <[email protected]>

Add new VhostKernFeatures trait to handle the backend features supported by the backend and acked by the fronted. Signed-off-by: Stefano Garzarella <[email protected]>

This new VhostIotlbBackend trait will be implemented by the backends that support IOTLB messages. Add also VhostIotlbMsg struct and related enums to handle IOTLB messages properly. Signed-off-by: Stefano Garzarella <[email protected]>

Implement VhostIotlbBackend trait for vhost_kern handling both vhost_msg and vhost_msg_v2 messages according to acked backend features. Signed-off-by: Stefano Garzarella <[email protected]>

Add ioctls, structs, and test needed to support vhost-vdpa. Signed-off-by: Stefano Garzarella <[email protected]>

Add new VhostVdpa trait to handle vhost-vdpa devices and a new vhost-vdpa building feature. vhost-vdpa devices is based on vhost backend, for this reason VhostVdpa trait requires VhostBackend. Signed-off-by: Stefano Garzarella <[email protected]>

Add a vhost-vdpa in-kernel implementation of VhostVdpa trait. Tests are serialized since the device supports only a single user at a time. Signed-off-by: Stefano Garzarella <[email protected]>

Add dma_map/dma_unmap functions in VhostVdpa trait and implement them in VhostKernVdpa using the IOTLB messages support provided by VhostIotlbBackend. Add also a specific test in vhost_kern/vdpa.rs. Signed-off-by: Stefano Garzarella <[email protected]>

vDPA simulators are available since Linux 5.7. The CI may have an older kernel, so to avoid CI failures, for now we skip the tests if we don't find the device. Signed-off-by: Stefano Garzarella <[email protected]>

stefano-garzarella · 2021-09-14T12:33:06Z

CI failures are not related to this PR, but should be fixed after merging #69

slp · 2021-09-15T05:47:49Z

CI failures are not related to this PR, but should be fixed after merging #69

With #69, all tests pass.

jiangliu · 2021-09-15T06:30:10Z

src/backend.rs

+    /// # Arguments
+    /// * `buffer` - Buffer containing the raw data received from the vhost-based backend.
+    /// * `msg` - IOTLB message parsed.
+    fn parse_iotlb_msg<T: Sized + VhostIotlbMsgParser>(


Seems the parse_iotlb_msg() should be common for all vDPA backends, because it parses spec-defined messages. So should we implement it as a public function?

Yep, it is common for all vhost_kern backends, indeed it's implemented in src/vhost_kern/mod.rs.
Do you mean that can be common also for vhost_user backends, so better to implement here?

I feel it's common for all vhost-kern drivers, so it should be implemented as an pub function at vhost-kern module, and no need to implement as a trait method.

Not sure whether it will be used by vhost-user drivers, especially vdpa-user servers.

Ok, I get your point now. I'll fix it.
About vdpa-user, there is an ongoing effort for VDUSE, but it is still under development.
I asked to the author and they plan to publish also Rust API for it in rust-vmm, let's see :-)

jiangliu · 2021-09-15T06:32:41Z

src/vhost_kern/vdpa.rs

+use vmm_sys_util::ioctl::{ioctl_with_mut_ref, ioctl_with_ptr, ioctl_with_ref};
+
+use std::alloc::{alloc, dealloc, Layout};
+use std::mem;


Group all std dependencies together by moving it near to line 9?

Yep, I'll fix.

It would also be great to give a simple introduction about vDPA as the module documentation.

yep sure, I'll add an introduction!

jiangliu · 2021-09-15T06:38:42Z

src/vhost_kern/vdpa.rs

+    fn get_config(&self, offset: u32, buffer: &mut [u8]) -> Result<()> {
+        let buffer_len = buffer.len();
+        let layout =
+            Layout::from_size_align(mem::size_of::<vhost_vdpa_config>() + buffer_len, 1).unwrap();


should we use vmm_sys_util::fam_struct here?

Oh absolutely, I didn't know it. I'll give it a try,

stefano-garzarella · 2021-09-15T07:52:36Z

@jiangliu @slp thanks for merging!
I'll fix the remaining points in another PR

stefano-garzarella force-pushed the vdpa branch from 47c64b0 to a4a3ea3 Compare March 24, 2021 16:47

stefano-garzarella force-pushed the vdpa branch 2 times, most recently from 13c1c2f to 12a74fc Compare May 24, 2021 11:34

stefano-garzarella force-pushed the vdpa branch from 12a74fc to d8802db Compare May 25, 2021 07:25

This was referenced Jun 2, 2021

Expose vhost vdpa device to container rust-vmm/rust-vmm-ci#64

Open

vhost vdpa: expose device to container rust-vmm/rust-vmm-ci#65

Closed

slp previously approved these changes Jun 17, 2021

View reviewed changes

stefano-garzarella dismissed slp’s stale review via a25fea1 June 17, 2021 11:01

stefano-garzarella force-pushed the vdpa branch 2 times, most recently from a25fea1 to e668682 Compare June 17, 2021 11:23

stefano-garzarella requested a review from slp June 17, 2021 12:45

slp previously approved these changes Jun 17, 2021

View reviewed changes

stefano-garzarella dismissed jiangliu’s stale review via 69e7629 September 3, 2021 12:59

stefano-garzarella force-pushed the vdpa branch from 2b98e23 to 69e7629 Compare September 3, 2021 12:59

This was referenced Sep 3, 2021

Draft: vDPA without skip test commit #82

Closed

Prepare for release v0.2.0 #83

Merged

stefano-garzarella force-pushed the vdpa branch from 69e7629 to 37d94f6 Compare September 9, 2021 14:02

stefano-garzarella requested a review from jiangliu September 9, 2021 14:08

stefano-garzarella added 12 commits September 14, 2021 13:51

vhost_kern: set the proper type of constants

15e12e5

VHOST_ACCESS_* and VHOST_IOTLB_* constants are assigned to c_uchar fields and VHOST_IOTLB_MSG to a c_int field. Let's set the proper types to avoid casts when assigning them. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern: add VHOST_IOTLB_BATCH_* constants

69d3316

These two new VHOST_IOTLB message types are supported by vhost-vdpa. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern: add vhost_msg_v2 bindings

b4df295

Add structs and tests for the C 'struct vhost_msg_v2' type supported by vhost-kern. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern: add VHOST_BACKEND ioctls and flags

9323394

vhost-kern support these ioctls to set/get backend features related to IOTLB message support. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern: add VhostKernFeatures trait

50e7f12

Add new VhostKernFeatures trait to handle the backend features supported by the backend and acked by the fronted. Signed-off-by: Stefano Garzarella <[email protected]>

Add VhostIotlbBackend trait

bc4fd9d

This new VhostIotlbBackend trait will be implemented by the backends that support IOTLB messages. Add also VhostIotlbMsg struct and related enums to handle IOTLB messages properly. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern: add IOTLB message support

5c0d236

Implement VhostIotlbBackend trait for vhost_kern handling both vhost_msg and vhost_msg_v2 messages according to acked backend features. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern: add VHOST_VDPA_* ioctls and structs

8b1cf2f

Add ioctls, structs, and test needed to support vhost-vdpa. Signed-off-by: Stefano Garzarella <[email protected]>

Add VhostVdpa trait

9e90134

Add new VhostVdpa trait to handle vhost-vdpa devices and a new vhost-vdpa building feature. vhost-vdpa devices is based on vhost backend, for this reason VhostVdpa trait requires VhostBackend. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern: add VhostKernVdpa struct

2adf66b

Add a vhost-vdpa in-kernel implementation of VhostVdpa trait. Tests are serialized since the device supports only a single user at a time. Signed-off-by: Stefano Garzarella <[email protected]>

vhost_kern/vdpa: skip tests if device is not found

e41a1ca

vDPA simulators are available since Linux 5.7. The CI may have an older kernel, so to avoid CI failures, for now we skip the tests if we don't find the device. Signed-off-by: Stefano Garzarella <[email protected]>

stefano-garzarella force-pushed the vdpa branch from 37d94f6 to e41a1ca Compare September 14, 2021 11:56

Merge branch 'main' into vdpa

a9551dd

slp approved these changes Sep 15, 2021

View reviewed changes

jiangliu reviewed Sep 15, 2021

View reviewed changes

jiangliu approved these changes Sep 15, 2021

View reviewed changes

jiangliu merged commit 354dd56 into rust-vmm:main Sep 15, 2021

stefano-garzarella mentioned this pull request Sep 29, 2021

vhost-vdpa: followup fixes after merging #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add vhost-vdpa support (in-kernel) #33

add vhost-vdpa support (in-kernel) #33

stefano-garzarella commented Mar 24, 2021

stefano-garzarella commented Mar 24, 2021

jiangliu commented May 18, 2021

stefano-garzarella commented May 18, 2021

andreeaflorescu commented May 18, 2021

stefano-garzarella commented May 18, 2021

andreeaflorescu commented May 18, 2021

stefano-garzarella commented May 18, 2021

stefano-garzarella commented May 20, 2021

andreeaflorescu commented May 21, 2021

stefano-garzarella commented May 21, 2021

stefano-garzarella commented May 24, 2021

jiangliu commented May 25, 2021

stefano-garzarella commented May 25, 2021

slp left a comment

stefano-garzarella commented Jun 17, 2021

slp commented Jun 17, 2021

stefano-garzarella commented Jun 18, 2021

andreeaflorescu commented Jun 22, 2021

stefano-garzarella commented Jun 22, 2021

stefano-garzarella commented Sep 14, 2021

slp commented Sep 15, 2021

jiangliu Sep 15, 2021

stefano-garzarella Sep 15, 2021

jiangliu Sep 15, 2021

stefano-garzarella Sep 16, 2021

jiangliu Sep 15, 2021

stefano-garzarella Sep 15, 2021

jiangliu Sep 15, 2021

stefano-garzarella Sep 16, 2021

jiangliu Sep 15, 2021

stefano-garzarella Sep 15, 2021

stefano-garzarella commented Sep 15, 2021

add vhost-vdpa support (in-kernel) #33

add vhost-vdpa support (in-kernel) #33

Conversation

stefano-garzarella commented Mar 24, 2021

stefano-garzarella commented Mar 24, 2021

jiangliu commented May 18, 2021

stefano-garzarella commented May 18, 2021

andreeaflorescu commented May 18, 2021

stefano-garzarella commented May 18, 2021

andreeaflorescu commented May 18, 2021

stefano-garzarella commented May 18, 2021

stefano-garzarella commented May 20, 2021

andreeaflorescu commented May 21, 2021

stefano-garzarella commented May 21, 2021

stefano-garzarella commented May 24, 2021

jiangliu commented May 25, 2021

stefano-garzarella commented May 25, 2021

slp left a comment

Choose a reason for hiding this comment

stefano-garzarella commented Jun 17, 2021

slp commented Jun 17, 2021

stefano-garzarella commented Jun 18, 2021

andreeaflorescu commented Jun 22, 2021

stefano-garzarella commented Jun 22, 2021

stefano-garzarella commented Sep 14, 2021

slp commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefano-garzarella commented Sep 15, 2021