Re: [virtio-comment] [PATCH] virtio-vsock: add max payload size config field

[Laura Loghin <lauralg@amazon.com>]

On 4/7/22 15:03, Stefano Garzarella wrote:
> CAUTION: This email originated from outside of the organization. Do 
> not click links or open attachments unless you can confirm the sender 
> and know the content is safe.
>
>
>
> On Mon, Apr 04, 2022 at 04:49:31PM +0300, Laura Loghin wrote:
> > Added a new field to the vsock device config space that
> > is limiting the size of the packet payload. This way
> > the driver is not allowed to allocate huge buffers, and
> > potentially fill up the entire memory.
> > Also defined a new feature bit for this, VIRTIO_VSOCK_F_SIZE_MAX,
> > in order to keep backwards compatibility.
> >
> > Signed-off-by: Laura Loghin <lauralg@amazon.com>
> > ---
> > virtio-vsock.tex | 34 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > index d79984d..a6950cc 100644
> > --- a/virtio-vsock.tex
> > +++ b/virtio-vsock.tex
> > @@ -23,6 +23,10 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > Socket Device / Feature bits}
> > \begin{description}
> > \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
> > \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
> > +\item[VIRTIO_VSOCK_F_SIZE_MAX (2)] Maximum size of the packet 
> > payload is in
> > +ÂÂÂÂÂ \field{data_max_size}. If offered by the device, device 
> > advises driver
> > +ÂÂÂÂÂ about the value of its maximum payload size. If negotiated, 
> > the driver uses
> > +ÂÂÂÂÂ \field{data_max_size} as the maximum packet payload size value.
> > \end{description}
> >
> > \subsection{Device configuration layout}\label{sec:Device Types / 
> > Socket Device / Device configuration layout}
> > @@ -32,6 +36,7 @@ \subsection{Device configuration 
> > layout}\label{sec:Device Types / Socket Device
> > \begin{lstlisting}
> > struct virtio_vsock_config {
> > ÂÂÂÂÂ le64 guest_cid;
> > +ÂÂÂÂÂ le32 data_max_size;
> > };
> > \end{lstlisting}
> >
> > @@ -57,6 +62,35 @@ \subsection{Device configuration 
> > layout}\label{sec:Device Types / Socket Device
> > \hline
> > \end{tabular}
> >
> > +The following driver-read-only field, \field{data_max_size} only 
> > exists if
> > +VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum 
> > packet payload
> > +size for the driver to use.
>
> Should we also define the maximum payload size when the feature is not
> negotiated?
> We never defined it, but the current implementation does not use
> payloads larger than 64k.

Hmm I don't know exactly how the spec applies to the vsock. So do we
consider the linux implementation as the "standard" one? In case the 
driver/device
implementation accepted payloads larger than 64k before, then they will be
forced now to negotiate `data_max_size` because of the fixed default value?
Or that implementation was considered as not being compliant to the spec?
I'm not exactly sure how backwards compatibility works here.
> > +
> > +\devicenormative{\subsubsection}{Device configuration layout}{Device 
> > Types / Socket Device / Device configuration layout}
> > +
> > +The device MUST set \field{data_max_size} to between 0 and 65536
>
> IIUC, this value is the maximum the device can manage, but then the
> driver can always allocate smaller buffers both for TX and RX, so maybe
> we could not put a limit, at the end the device can say what it wants,
> the driver must only avoid to exceed that value.
True, I was thinking that a hard limit might be useful here, but I don't 
have
a strong preference. I can remove this.
> > inclusive, if
> > +it offers VIRTIO_VSOCK_F_SIZE_MAX.
> > +
> > +The device MUST NOT modify \field{data_max_size} once it has been set.
> > +
> > +The device MUST NOT pass received packets that exceed 
> > \field{data_max_size}
> > +(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been 
> > successfully
> > +negotiated.
>
> For the device the limit is anyway the buffer (allocated by the driver)
> it finds in RX, so I don't know if this sentence is useful.
Yes, I don't think this sentence is needed, I'll remove it.
> > +
> > +The device MUST forward transmitted packets of up to 
> > \field{data_max_size}
> > +(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been 
> > successfully
> > +negotiated.
> > +
> > +\drivernormative{\subsubsection}{Device configuration layout}{Device 
> > Types / Socket Device / Device configuration layout}
> > +
> > +A driver SHOULD negotiate VIRTIO_VSOCK_F_SIZE_MAX if the device 
> > offers it.
> > +
> > +If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST supply 
> > receive
> > +buffers that don't exceed the size \field{data_max_size} (plus 
> > header length).
> > +
> > +If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUSTÂ NOT 
> > transmit packets
> > +of size exceeding the value of \field{data_max_size} (plus header 
> > length).
> > +
> > \subsection{Device Initialization}\label{sec:Device Types / Socket 
> > Device / Device Initialization}
> >
> > \begin{enumerate}
> > -- 2.17.1
>
> I don't know if it is useful to define also a minimum size for the RX
> buffers.

The reason for that would be to not work with a large number of
descriptor chains for a packet?
> It could be useful for example when we will support datagram,
> we could use it to make driver allocate buffers that can contain entire
> datagram.
>
> For now since we only support socket stream maybe it's not useful, so we
> can postpone it. And maybe it's not useful even with datagram if we
> support mergable buffers.
>
> Thanks,
> Stefano
Thanks,
Laura



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.