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

vsock: support header and data on a single descriptor #207

Merged
merged 3 commits into from
Dec 21, 2022

Conversation

stefano-garzarella
Copy link
Member

Summary of the PR

Starting from Linux 6.2 the virtio-vsock driver can use a single descriptor for both header and data:
https://lore.kernel.org/lkml/20221215043645.3545127-1-bobby.eshleman@bytedance.com/

So with this modification the virtio-vsock device can support header and data on a single descriptor or on two. This is just a Linux implementation detail though, for the spec it could be multiple descriptors as well.

For now let's add only the single descriptor support, since it doesn't involve an API change, but in the future we should change the API and support an array of VolatileSlices as suggested by Laura here: #204 (comment)

Let's make the tests work with this change (mainly by always using PKT_HEADER_SIZE for the header descriptor) and adding new tests to cover the single descriptor scenario.

Fixes: #204

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@stefano-garzarella
Copy link
Member Author

About the CI failure on the commit message, should I remove the links?
IMHO they are useful.

@lauralt
Copy link
Contributor

lauralt commented Dec 19, 2022

About the CI failure on the commit message, should I remove the links?

No, we can merge the PR with admin rights when it'll be the time.

@andreeaflorescu
Copy link
Member

@stefano-garzarella The tests that we are now using for generating the fuzz input are failing, can you take a look?
https://buildkite.com/rust-vmm/vm-virtio-ci/builds/728#01852a4d-1e33-4d66-885b-006d851b7344

@stefano-garzarella
Copy link
Member Author

@stefano-garzarella The tests that we are now using for generating the fuzz input are failing, can you take a look? https://buildkite.com/rust-vmm/vm-virtio-ci/builds/728#01852a4d-1e33-4d66-885b-006d851b7344

Ooops, I forgot to adjust the descriptors in the fuzz subfolder.

@andreeaflorescu
Copy link
Member

@stefano-garzarella if you do a rebase on top of main the cargo audit test should now pass, and we shouldn't see any more failures. Besides the commit message test all the other tests should pass to merge this PR.

Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

Didn't look over all the code, but I think it needs some adjustments. Will take a look at the rest tomorrow.

crates/devices/virtio-vsock/src/packet.rs Outdated Show resolved Hide resolved
crates/devices/virtio-vsock/src/packet.rs Outdated Show resolved Hide resolved
crates/devices/virtio-vsock/src/packet.rs Outdated Show resolved Hide resolved
@andreeaflorescu
Copy link
Member

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

@stefano-garzarella
Copy link
Member Author

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

It's fine, but the current branch doesn't include the change that I proposed to avoid the overflow.
Let me push it and IIUC the 1day pipeline will start, right?

@stefano-garzarella
Copy link
Member Author

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

It's fine, but the current branch doesn't include the change that I proposed to avoid the overflow. Let me push it and IIUC the 1day pipeline will start, right?

ah no, I thought was on main. So I'll send the new version and then you can update it.

@andreeaflorescu
Copy link
Member

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

It's fine, but the current branch doesn't include the change that I proposed to avoid the overflow. Let me push it and IIUC the 1day pipeline will start, right?

Yap, that's right! Once it is pushed once, then we can also retrigger it from github. But since the webhook was yet never called, GitHub doesn't know of its existence.

Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

We can probably get rid of some of the duplication in the tx and rx path, but I don't think that makes a lot of sense since we want to do a more generic refactoring anyway.

LGTM.

crates/devices/virtio-vsock/src/packet.rs Outdated Show resolved Hide resolved
@lauralt
Copy link
Contributor

lauralt commented Dec 21, 2022

This documentation needs to be updated.

@lauralt
Copy link
Contributor

lauralt commented Dec 21, 2022

We should probably also add a CHANGELOG entry for this, even if it's not an API change.

@stefano-garzarella
Copy link
Member Author

new version:

lauralt
lauralt previously approved these changes Dec 21, 2022
Copy link
Contributor

@lauralt lauralt left a comment

Choose a reason for hiding this comment

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

Do we publish this as a patch release, in that case we should follow this guide which involves creating a new branch etc.?

crates/devices/virtio-vsock/src/packet.rs Outdated Show resolved Hide resolved
@andreeaflorescu
Copy link
Member

Do we publish this as a patch release, in that case we should follow this guide which involves creating a new branch etc.

I don't think we need a patch release on a new branch unless we want to backport this fix to previous releases. We can just do patch releases from main as well as long as the history is linear, which is what I think is the case here.

@stefano-garzarella
Copy link
Member Author

stefano-garzarella commented Dec 21, 2022

new version:

  • checked also that chain_head.next() is false as @lauralt suggested
  • reverted most of the changes in the tests since we now behave more similarly to before

lauralt
lauralt previously approved these changes Dec 21, 2022
crates/devices/virtio-vsock/src/packet.rs Show resolved Hide resolved
Starting from Linux 6.2 the virtio-vsock driver can use a single
descriptor for both header and data:
https://lore.kernel.org/lkml/20221215043645.3545127-1-bobby.eshleman@bytedance.com/

So with this modification the virtio-vsock device can support header
and data on a single descriptor or on two. This is just a Linux
implementation detail though, for the spec it could be multiple
descriptors as well.

For now let's add only the single descriptor support, since it doesn't
involve an API change, but in the future we should change the API and
support an array of VolatileSlices as suggested by Laura here:
rust-vmm#204 (comment)

Let's make some tests working with this change and adding new tests to
cover the single descriptor scenario.

Fixes: rust-vmm#204

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
When we use `vq.build_desc_chain()` in tests, we usually pass the
entire array of descriptors, so there is no need to create a slice
using a range, we can pass it all directly.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@lauralt lauralt merged commit 64ac388 into rust-vmm:main Dec 21, 2022
stefano-garzarella added a commit to stefano-garzarella/vhost-device that referenced this pull request Jan 16, 2023
The 0.2.1 version of virtio-vsock crate contains a fix [1] needed to
properly work with the virtio-vsock driver provided by Linux v6.3 and
later (originally the new driver was supposed to be in v6.2, but it
was postponed).

The fix was just a quick workaround, but in the future more work will
be needed in the virtio-vsock crate to not have a Linux-only specific
implementation of VsockPacket, as described in this issue [2].

[1] rust-vmm/vm-virtio#207
[2] rust-vmm/vm-virtio#216

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
mathieupoirier pushed a commit to rust-vmm/vhost-device that referenced this pull request Jan 19, 2023
The 0.2.1 version of virtio-vsock crate contains a fix [1] needed to
properly work with the virtio-vsock driver provided by Linux v6.3 and
later (originally the new driver was supposed to be in v6.2, but it
was postponed).

The fix was just a quick workaround, but in the future more work will
be needed in the virtio-vsock crate to not have a Linux-only specific
implementation of VsockPacket, as described in this issue [2].

[1] rust-vmm/vm-virtio#207
[2] rust-vmm/vm-virtio#216

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
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.

virtio-vsock: spec violation about the number of descriptors per packet
3 participants