Re: vsock: device expects at least 4 KB buffers in the rx virtqueue

[Stefano Garzarella <sgarzare@redhat.com>]

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