-
Notifications
You must be signed in to change notification settings - Fork 86
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
virtio-vsock: spec violation about the number of descriptors per packet #204
Comments
I would like to fix it before we release the new driver (not before Linux 6.2). |
Yes, the implementation was Linux specific at that point, it is mentioned in a doc comment as well. I agree this should be fixed, but other things were prioritised. Out of curiosity, does it fail with the error you mentioned when having multiple descriptors for the data or? |
I didn't know that, thanks for pointing it out!
Great! Thanks, let me know if I can help in some way.
Nope, in this case we have a single descriptor for header and data (in RX) |
Aaaah yes that is possible since for the vsock device the header and the data have the same type (device readable or writable), so you can keep them in a single descriptor, I forgot about that for a second. Thanks for the clarification. |
@stefano-garzarella I had a better look at the code, it should be doable with the current abstraction (but didn't do any POC yet). This would probably be a good time to invest in reviewing/updating #33 since the Reader and Writer abstractions should simplify this problem (which we ultimately have in each device implementation). |
@lauralt thank you for the helpful information! I'll see if I can next week, but if we merge the patch into Linux, I think we should at least have a workaround right away and then maybe refine the implementation. |
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 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: rust-vmm#204 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@lauralt I pushed #207. So I thought for now to just support the single descriptor case to run the device with Linux v6.2 without changing API (we might release virtio-vsock 0.2.1). And then think better about your suggestion and change the API. What do you think? |
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 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: rust-vmm#204 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@stefano-garzarella |
@lauralt yep, sure! |
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 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: rust-vmm#204 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
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 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: rust-vmm#204 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
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 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: rust-vmm#204 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
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 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: rust-vmm#204 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
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>
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>
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 some tests working with this change and adding new tests to cover the single descriptor scenario. Fixes: #204 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
In
crates/devices/virtio-vsock/src/packet.rs
for both the TX and RX queue, we assume that each packet always consists of 2 descriptors (returningDescriptorChainTooShort
error otherwise), but this is not in the specification.It was an implementation detail true until Linux 6.1, but now it's about to change.
Testing vhost-user-vsock (which uses this crate) with the proposed Linux patch, I noticed that it doesn't work, printing this error:
In the vhost-vsock device available in Linux, we don't assume that, so I think we should do the same here to respect the virtio specification.
The text was updated successfully, but these errors were encountered: