[virtio-comment] vsock: device expects at least 4 KB buffers in the rx virtqueue

[virtio-comment] vsock: device expects at least 4 KB buffers in the rx virtqueue

[Stefano Garzarella]

Hi,
following the discussion with Michael [1], the device emulation
implemented in vhost-vsock made an assumption on buffers at least 4 KB
in the rx virtqueue.

This patch [2] removed the assumption unintentionally, but vhost-vsock
modules released before Linux v5.4 still have this limitation.

The current implementation continues to use 4 KB buffers, so for now, we
are backward compatible, but we should add something in the
specifications.

I'm not sure how to proceed, my ideas are the following:

1. Add a note like this:
    diff --git a/virtio-vsock.tex b/virtio-vsock.tex
    index da7e641..4c8f65d 100644
    --- a/virtio-vsock.tex
    +++ b/virtio-vsock.tex
    @@ -198,6 +198,9 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De
     Virtqueue buffers for outgoing packets are read-only. Virtqueue buffers for
     incoming packets are write-only.
     
    +Note: The device implementation (vhost-vsock) in Linux prior to v5.4 requires
    +that the receive buffers in the rx virtqueue MUST be at least 4KBytes.
    +
     \drivernormative{\paragraph}{Device Operation: Receive and Transmit}{Device Types / Socket Device / Device Operation / Receive and Transmit}
     
     The \field{guest_cid} configuration field MUST be used as the source CID when

2. Add a new feature to tell that the device supports any buffer size in
   the rx queue

3. Add a new feature to support mergeable buffers (in my plans),
   avoiding the feature at point 2

Any suggestion?

Thanks,
Stefano

[1] https://lkml.org/lkml/2019/9/2/676
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6dbd3e66e7785a2f055bf84d98de9b8fd31ff3f5 


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: vsock: device expects at least 4 KB buffers in the rx virtqueue

[Michael S. Tsirkin]

On Fri, Nov 15, 2019 at 12:31:17PM +0100, Stefano Garzarella wrote:
> Hi,
> following the discussion with Michael [1], the device emulation
> implemented in vhost-vsock made an assumption on buffers at least 4 KB
> in the rx virtqueue.
> 
> This patch [2] removed the assumption unintentionally, but vhost-vsock
> modules released before Linux v5.4 still have this limitation.
> 
> The current implementation continues to use 4 KB buffers, so for now, we
> are backward compatible, but we should add something in the
> specifications.
> 
> I'm not sure how to proceed, my ideas are the following:
> 
> 1. Add a note like this:
>     diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>     index da7e641..4c8f65d 100644
>     --- a/virtio-vsock.tex
>     +++ b/virtio-vsock.tex
>     @@ -198,6 +198,9 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De
>      Virtqueue buffers for outgoing packets are read-only. Virtqueue buffers for
>      incoming packets are write-only.
>      
>     +Note: The device implementation (vhost-vsock) in Linux prior to v5.4 requires
>     +that the receive buffers in the rx virtqueue MUST be at least 4KBytes.
>     +

Question is how big a fix it is for all devices?
It is attractive to fix it in the correct place, which is host kernel.
If we are to fix it I'd start with putting the patch in stable,
and red hat kernels over which we have control.

Also, there are implementations outside vhost. Can you check these
please? Did they make the 4K buffer assumption too?

>      \drivernormative{\paragraph}{Device Operation: Receive and Transmit}{Device Types / Socket Device / Device Operation / Receive and Transmit}
>      
>      The \field{guest_cid} configuration field MUST be used as the source CID when
> 
> 2. Add a new feature to tell that the device supports any buffer size in
>    the rx queue
> 
> 3. Add a new feature to support mergeable buffers (in my plans),
>    avoiding the feature at point 2

Assuming we make it a new feature, I think either 2 or 3.


> Any suggestion?
> 
> Thanks,
> Stefano
> 
> [1] https://lkml.org/lkml/2019/9/2/676
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6dbd3e66e7785a2f055bf84d98de9b8fd31ff3f5 


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: vsock: device expects at least 4 KB buffers in the rx virtqueue

[Stefano Garzarella]

On Fri, Nov 15, 2019 at 09:06:36AM -0500, Michael S. Tsirkin wrote:
> On Fri, Nov 15, 2019 at 12:31:17PM +0100, Stefano Garzarella wrote:
> > Hi,
> > following the discussion with Michael [1], the device emulation
> > implemented in vhost-vsock made an assumption on buffers at least 4 KB
> > in the rx virtqueue.
> > 
> > This patch [2] removed the assumption unintentionally, but vhost-vsock
> > modules released before Linux v5.4 still have this limitation.
> > 
> > The current implementation continues to use 4 KB buffers, so for now, we
> > are backward compatible, but we should add something in the
> > specifications.
> > 
> > I'm not sure how to proceed, my ideas are the following:
> > 
> > 1. Add a note like this:
> >     diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> >     index da7e641..4c8f65d 100644
> >     --- a/virtio-vsock.tex
> >     +++ b/virtio-vsock.tex
> >     @@ -198,6 +198,9 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De
> >      Virtqueue buffers for outgoing packets are read-only. Virtqueue buffers for
> >      incoming packets are write-only.
> >      
> >     +Note: The device implementation (vhost-vsock) in Linux prior to v5.4 requires
> >     +that the receive buffers in the rx virtqueue MUST be at least 4KBytes.
> >     +
> 
> Question is how big a fix it is for all devices?

The only device affected seems to be the vhost-vsock implementation (the
other implementation that I found is the Firecracker emulation, see
later) and the fix [2] should easily be applied on stable branches, since
that code hasn't changed much since the introduction.

This is the diffstat of the patch:
 drivers/vhost/vsock.c                   | 66 +++++++++++++++++++++++++++++++++++-------------
 net/vmw_vsock/virtio_transport_common.c | 15 ++++++++---
 2 files changed, 60 insertions(+), 21 deletions(-)

> It is attractive to fix it in the correct place, which is host kernel.
> If we are to fix it I'd start with putting the patch in stable,
> and red hat kernels over which we have control.

Okay, I'll ask Dave to consider the patch also for stable branches, and
I'll look for RH kernels.

> 
> Also, there are implementations outside vhost. Can you check these
> please? Did they make the 4K buffer assumption too?

The other sole implementation that I found is in Firecracker [3] and looking
at the code [4], they don't do any assumption and look at the buffer
size taken from the RX virtqueue.

> 
> >      \drivernormative{\paragraph}{Device Operation: Receive and Transmit}{Device Types / Socket Device / Device Operation / Receive and Transmit}
> >      
> >      The \field{guest_cid} configuration field MUST be used as the source CID when
> > 
> > 2. Add a new feature to tell that the device supports any buffer size in
> >    the rx queue
> > 
> > 3. Add a new feature to support mergeable buffers (in my plans),
> >    avoiding the feature at point 2
> 
> Assuming we make it a new feature, I think either 2 or 3.
> 

Okay, maybe 3 would be cleaner and less related to implementation in
vhost-vsock.

Thanks,
Stefano

> [1] https://lkml.org/lkml/2019/9/2/676
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6dbd3e66e7785a2f055bf84d98de9b8fd31ff3f5
[3] https://github.com/firecracker-microvm/firecracker/tree/master/devices/src/virtio/vsock
[4] https://github.com/firecracker-microvm/firecracker/blob/81592a1e21084c4f8e02bfafc9b1122c9db4254e/devices/src/virtio/vsock/csm/connection.rs#L217


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/