Add vhost-device I2C support #1

vireshk · 2021-05-26T11:48:26Z

Earlier discussion: rust-vmm/community#106

I have tried to migrate my existing i2c work over to this repository and did whatever made sense to me (in respect to the workspace thing). There are high chances that I might have made some mistakes here.

I know there would be a lot of things that would require improvement in the i2c code as well as I am really a newbie to the rust world, apologies in advance for reviewing not so great code.

Thanks

andreeaflorescu · 2021-05-26T11:56:32Z

i2c/Cargo.toml

+authors = ["Viresh Kumar <viresh.kumar@linaro.org>"]
+description = "vhost i2c backend device"
+repository = "https://github.com/rust-vmm/vhost-device"
+edition = "2018"


Can you add all the fields that were in the root Cargo.toml?

andreeaflorescu · 2021-05-26T11:57:49Z

i2c/Cargo.toml

@@ -9,3 +9,11 @@ edition = "2018"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [dependencies]
+clap = { version = "2.33.3",  features = ["yaml"] }
+epoll = "4.3"


Can you use vmm-sys-util instead? We have an epoll wrapper there that is relatively safer than the epoll as it checks the results of unsafe functions.

Can you use vmm-sys-util instead? We have an epoll wrapper there that is relatively safer than the epoll as it checks the results of unsafe functions.

I used epoll instead as https://github.com/rust-vmm/vhost-user-backend uses the same and my repo depends on a function prototype from there.

Is that fine @andreeaflorescu ?

Oh, I see. Okay.

andreeaflorescu · 2021-05-26T11:58:26Z

We have to setup the repo & the CI as well.

andreeaflorescu · 2021-05-26T14:04:50Z

We have to setup the repo & the CI as well.

I've setup the Buildkite CI. On the next push we should be able to see the results of running the pipeline.

README.md

andreeaflorescu · 2021-05-26T14:06:24Z

README.md

- What are the main components of the crate? How do they interact which each
-  other?
+This repository hosts various 'vhost-user' device backends in their own crates.
+See their individual README.md files for specific information about those


How about also mentioning all these devices (which right now is just one 😆) here as well, and have a link that takes you to their readme?

andreeaflorescu · 2021-05-26T14:06:50Z

i2c/README.md

+work with any virtual machine monitor (VMM) that supports vhost-user. See the
+Examples section below.
+
+##Synopsis


nit: need space after ##.

andreeaflorescu · 2021-05-27T07:14:56Z

@vireshk this is the first time we run the tests for a workspace, so there might be problems that we discover along the way and that we might need to address.

It looks like one potential problem is that the coverage seems to be 0%. This means that it is not correctly computed by test_coverage. We should look into that as well, and maybe open separate issues in rust-vmm-ci for the problems we discover along the way.

vireshk · 2021-05-27T07:19:51Z

@vireshk this is the first time we run the tests for a workspace, so there might be problems that we discover along the way and that we might need to address.

It looks like one potential problem is that the coverage seems to be 0%. This means that it is not correctly computed by test_coverage. We should look into that as well, and maybe open separate issues in rust-vmm-ci for the problems we discover along the way.

Okay, meanwhile I am trying to fix all the problems reported by the bot and commented on by you.

Few doubts though:

Are frequent pushes to my master branch fine ? I see that the pull requests mentions them and so thought..
What should I do once I have worked up on the stuff and want them to get reviewed again ? The same pull request will work or a new one needs to be created ?

andreeaflorescu · 2021-05-27T07:40:42Z

Are frequent pushes to my master branch fine ? I see that the pull requests mentions them and so thought..

Yes, that's okay. That's the development method that we are following.

What should I do once I have worked up on the stuff and want them to get reviewed again ? The same pull request will work or a new one needs to be created ?

You can just ping people when you want to look at your code again. Some folks are automatically following all repositories created in rust-vmm, but you can also ask on Slack or ping people here on GitHub. We do not have a predefined process for this.

vireshk · 2021-05-27T12:05:06Z

It looks like one potential problem is that the coverage seems to be 0%. This means that it is not correctly computed by test_coverage. We should look into that as well, and maybe open separate issues in rust-vmm-ci for the problems we discover along the way.

@andreeaflorescu I was able to get rid of all other errors and warnings, and only two are leftL coverage-x86 and cargo-audit and I do not understand how to fix them. If there is something wrong in my repo or it is because of the workspace thing you mentioned. will it be possible for you to look at it?

vireshk · 2021-05-27T12:06:01Z

I have tried to migrate my existing i2c work over to this repository and did whatever made sense to me (in respect to the workspace thing). There are high chances that I might have made some mistakes here.

@andreeaflorescu I have updated the code based on your feedback and all the errors generated by the CI. Please give it another glance once you get a chance. Thanks.

andreeaflorescu · 2021-05-27T12:24:54Z

It looks like you're pulling a dependency on a vulnerable crate: https://buildkite.com/rust-vmm/vhost-device-ci/builds/12#ede8f33d-722f-4da2-a7ff-57b9e2d218f7. Can you update that?

Can you try some configurations with the workspace such that the coverage is correctly computed? To do that, you'll need to follow the instructions from here: https://github.com/rust-vmm/rust-vmm-ci#adaptive-coverage to run the coverage test. The latest container version is 11. To check the coverage result, you'll need to run the command with --no-cleanup:

pytest  rust-vmm-ci/integration_tests/test_coverage.py --no-cleanup

You will find the aggregated coverage results in kcov_output/index.html.

andreeaflorescu · 2021-05-27T13:25:13Z

Can you try some configurations with the workspace such that the coverage is correctly computed? To do that, you'll need to follow the instructions from here: https://github.com/rust-vmm/rust-vmm-ci#adaptive-coverage to run the coverage test. The latest container version is 11. To check the coverage result, you'll need to run the command with --no-cleanup:
pytest  rust-vmm-ci/integration_tests/test_coverage.py --no-cleanup
You will find the aggregated coverage results in kcov_output/index.html.

LE: it looks like this PR adds no unit tests. Can you add some? Since this crate exports a binary, it might make sense to also have an integration test.

andreeaflorescu · 2021-05-27T13:26:29Z

Cargo.toml


-[dependencies]
+members = [
+    "i2c",


Can you move this under the src/ directory? Otherwise we are going to have lots of things in the root of the repository as we add more devices.

Can you move this under the src/ directory? Otherwise we are going to have lots of things in the root of the repository as we add more devices.

done

vireshk · 2021-05-27T15:16:39Z

It looks like you're pulling a dependency on a vulnerable crate: https://buildkite.com/rust-vmm/vhost-device-ci/builds/12#ede8f33d-722f-4da2-a7ff-57b9e2d218f7. Can you update that?

I have this in my dependencies:

clap = { version = "2.33.3", features = ["yaml"] }

This clap crate is fetching yaml-rust (I am not doing that directly) and in there somehow the versioning thing is getting screwed up.

Here is clap's dependency, which looks just fine:
https://github.com/clap-rs/clap/blob/master/Cargo.toml#L70

yaml-rust = { version = "0.4.1", optional = true }

What should I be doing here to fix this ?

vireshk · 2021-05-28T07:57:12Z

What should I be doing here to fix this ?

I have raised an issue for this with clap, something is wrong.

clap-rs/clap#2506

vireshk · 2021-08-03T07:35:54Z

@andreeaflorescu is there anything else you would like me to fix here ?

@andreeaflorescu gentle reminder!

vireshk · 2021-08-17T06:15:00Z

@jiangliu I have lost your review by mistake. Ideally I should have just added a patch on top, but instead did force-push by mistake. By the time I realized I made the mistake, I coulnd't do anything to get your review back.

Here is what changed since the time you reviewed this: https://pastebin.com/PDQqvS4G

vireshk · 2021-08-17T07:02:52Z

@andreeaflorescu Clap requires rustc >= 1.54, how do I fix that to make the build work ?

andreeaflorescu · 2021-08-17T07:10:42Z

Do you need the latest version of clap? Can you fix the dependency to a version that doesn't need the latest version?

andreeaflorescu · 2021-08-17T07:16:58Z

@vireshk I am not following this repository. If you need my help, can you ping me on Slack/email?

This adds the boilerplate code and its dependencies to get the basic structure for the i2c crate. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This patch implements the low level I2C helpers responsible for transferring data and parsing the adapters and their clients. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This patch implements process_queue() to process incoming requests. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This updates the main README and adds a specific one for i2c crate. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

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

This adds unit tests for the i2c workspace. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

vireshk · 2021-08-17T07:21:43Z

Do you need the latest version of clap? Can you fix the dependency to a version that doesn't need the latest version?

Done.

vireshk · 2021-08-17T07:23:00Z

@vireshk I am not following this repository. If you need my help, can you ping me on Slack/email?

Sure.

Though this stuff is ready to be merged. @jiangliu has already given his review-approval for this. Can you give yours too if everything looks okay ?

Reduce the test coverage score as the files apart from i2c.rs are mostly boilerplate code and it is difficult to test them as well as it may not be worth it. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

andreeaflorescu · 2021-08-17T08:05:00Z

I will not be able to review the code as we are unlikely to use it in the near future, and I currently have other priorities. I can give a formal approve so you can merge the code. It would be good to find another crate owner that can help with reviewing subsequent PRs. We require 2 approvals for PRs to be merged.

I think @sboeuf had some concerns related to publishing this crate before the driver changes are merged in upstream Linux. Are those now merged?

Before publishing this crate to crates.io, can you please check the requirements for publishing? I am mostly concerned about the coverage which is still very low compared to the other rust-vmm crates. We can merge this initial PR first, and work on incremental changes to make sure we keep the same quality bar as for existing components. Does that make sense?

vireshk · 2021-08-17T09:49:49Z

I will not be able to review the code as we are unlikely to use it in the near future, and I currently have other priorities. I can give a formal approve so you can merge the code. It would be good to find another crate owner that can help with reviewing subsequent PRs. We require 2 approvals for PRs to be merged.

Sure.

I think @sboeuf had some concerns related to publishing this crate before the driver changes are merged in upstream Linux. Are those now merged?

It is already accepted by the I2C maintainer, and is waiting for a dependency patch to land in first.
https://lore.kernel.org/linux-i2c/YQEkWL3ZQzbiMVl6@kunai/

Before publishing this crate to crates.io, can you please check the requirements for publishing? I am mostly concerned about the coverage which is still very low compared to the other rust-vmm crates.

The part which is missing the testing is vhu_i2c.rs, which is mostly the implementation of vhost-user-backend device and I am not sure how to add testing code for it, since the same is missing here: https://github.com/rust-vmm/vhost-user-backend

We can merge this initial PR first, and work on incremental changes to make sure we keep the same quality bar as for existing components. Does that make sense?

Sure.

vireshk · 2021-08-18T03:29:21Z

I think @sboeuf had some concerns related to publishing this crate before the driver changes are merged in upstream Linux. Are those now merged?

And it is merged finally.

https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=8fb12751ac78d0a4ba3c604496ffc8dcd1bd6c31

sboeuf · 2021-08-18T07:27:45Z

And it is merged finally.

Cool! Let's proceed with this PR then :)

virtio_sound.rs: Added device configuration and common definitions

andreeaflorescu reviewed May 26, 2021

View reviewed changes

andreeaflorescu mentioned this pull request May 26, 2021

New crate for vhost-user-i2c rust-vmm/community#106

Closed

vireshk force-pushed the master branch 3 times, most recently from d941338 to 04bd4bd Compare May 27, 2021 05:21

vireshk force-pushed the master branch 8 times, most recently from ef87941 to e349285 Compare May 27, 2021 11:01

andreeaflorescu reviewed May 27, 2021

View reviewed changes

vireshk force-pushed the master branch 3 times, most recently from 9f5a8e5 to b5d906d Compare May 28, 2021 07:49

vireshk force-pushed the master branch from b5d906d to 0aefbb2 Compare May 28, 2021 09:12

vireshk dismissed jiangliu’s stale review via b7359ad August 17, 2021 06:08

vireshk force-pushed the master branch 3 times, most recently from 58050ad to b7359ad Compare August 17, 2021 06:12

vireshk added 6 commits August 17, 2021 12:49

i2c: Add boilerplate code

8048e0c

This adds the boilerplate code and its dependencies to get the basic structure for the i2c crate. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

i2c: Implement low level I2C helpers

5906d1e

This patch implements the low level I2C helpers responsible for transferring data and parsing the adapters and their clients. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

i2c: Process virtio requests

f55afba

This patch implements process_queue() to process incoming requests. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

i2c: Update README.md and add one for i2c

cb612da

This updates the main README and adds a specific one for i2c crate. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

i2c: Update CODEOWNERS

9e6bf43

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

i2c: Add unit tests

cde8baa

This adds unit tests for the i2c workspace. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

vireshk force-pushed the master branch from b7359ad to 81d0d26 Compare August 17, 2021 07:19

i2c: Reduce test coverage score

09f4b76

Reduce the test coverage score as the files apart from i2c.rs are mostly boilerplate code and it is difficult to test them as well as it may not be worth it. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

vireshk force-pushed the master branch from 81d0d26 to 09f4b76 Compare August 17, 2021 07:30

sboeuf mentioned this pull request Aug 17, 2021

Implement vTPM support cloud-hypervisor/cloud-hypervisor#2343

Closed

jiangliu approved these changes Aug 18, 2021

View reviewed changes

andreeaflorescu approved these changes Aug 19, 2021

View reviewed changes

andreeaflorescu merged commit 019e755 into rust-vmm:master Aug 19, 2021

vireshk deleted the master branch August 25, 2021 07:11

stsquad pushed a commit to stsquad/vhost-device that referenced this pull request Jul 20, 2023

Merge pull request rust-vmm#1 from dorindabassey/d_sound

c3e2f6a

virtio_sound.rs: Added device configuration and common definitions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vhost-device I2C support #1

Add vhost-device I2C support #1

vireshk commented May 26, 2021

andreeaflorescu May 26, 2021

vireshk May 27, 2021

andreeaflorescu May 26, 2021

vireshk May 27, 2021

vireshk May 27, 2021

andreeaflorescu May 27, 2021

andreeaflorescu commented May 26, 2021

andreeaflorescu commented May 26, 2021

andreeaflorescu May 26, 2021

vireshk May 27, 2021

andreeaflorescu May 26, 2021

vireshk May 27, 2021

andreeaflorescu commented May 27, 2021

vireshk commented May 27, 2021

andreeaflorescu commented May 27, 2021

vireshk commented May 27, 2021

vireshk commented May 27, 2021

andreeaflorescu commented May 27, 2021

andreeaflorescu commented May 27, 2021

andreeaflorescu May 27, 2021

vireshk May 27, 2021

vireshk commented May 27, 2021

vireshk commented May 28, 2021

vireshk commented Aug 3, 2021

vireshk commented Aug 17, 2021

vireshk commented Aug 17, 2021

andreeaflorescu commented Aug 17, 2021

andreeaflorescu commented Aug 17, 2021

vireshk commented Aug 17, 2021

vireshk commented Aug 17, 2021

andreeaflorescu commented Aug 17, 2021

vireshk commented Aug 17, 2021

vireshk commented Aug 18, 2021

sboeuf commented Aug 18, 2021

Add vhost-device I2C support #1

Add vhost-device I2C support #1

Conversation

vireshk commented May 26, 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

andreeaflorescu commented May 26, 2021

andreeaflorescu commented May 26, 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

andreeaflorescu commented May 27, 2021

vireshk commented May 27, 2021

andreeaflorescu commented May 27, 2021

vireshk commented May 27, 2021

vireshk commented May 27, 2021

andreeaflorescu commented May 27, 2021

andreeaflorescu commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vireshk commented May 27, 2021

vireshk commented May 28, 2021

vireshk commented Aug 3, 2021

vireshk commented Aug 17, 2021

vireshk commented Aug 17, 2021

andreeaflorescu commented Aug 17, 2021

andreeaflorescu commented Aug 17, 2021

vireshk commented Aug 17, 2021

vireshk commented Aug 17, 2021

andreeaflorescu commented Aug 17, 2021

vireshk commented Aug 17, 2021

vireshk commented Aug 18, 2021

sboeuf commented Aug 18, 2021