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

[Laura Loghin <lauralg@amazon.com>]

On 4/15/22 18:23, Laura Loghin wrote:
> On 4/15/22 16:06, 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 Wed, Apr 13, 2022 at 01:01:40PM +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 | 27 +++++++++++++++++++++++++++
> > > 1 file changed, 27 insertions(+)
> > >
> > > diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > > index d79984d..e6903bd 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,28 @@ \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.
> > > +
> > > +\devicenormative{\subsubsection}{Device configuration 
> > > layout}{Device Types / Socket Device / Device configuration layout}
> > > +
> > > +The device MUST NOT change the value exposed through 
> > > \field{data_max_size}.
> > > +
> > > +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.
> >
> > Sorry, I thought we were going to delete this sentence like we did the
> > one above, maybe I didn't get the meaning right though.
> >
> > What do you mean here by "forward"?
> >
> > Thanks,
> > Stefano
>
> Sorry for the confusion. Now I got super confused as well :D. So actually
> "MUST NOT pass received packets" was about the packets received from 
> the driver,
> and "MUST forward transmitted packets" about the packets transmitted 
> from the
> backend to the driver. The "receive/transmit" terminology was from the 
> device
> perspective here. So your initial comment applied to this sentence 
> actually, not the
> initial one, but I forgot then that I wrote this section from the 
> device perspective.
> Both sentences are already implied by their corresponding ones from 
> the driver
> section, so I think I can delete both actually.
> Thanks for raising this!
>
> Laura

Hi,


Both sentences are removed now, and I also updated a bit the second
requirement from the driver section. Before it was a bit unclear if
data_max_size is the limit for each buffer or for their sum (for
implementations that allow more than 2 buffers for a single packet).
I (hopefully) made it clearer in
https://lists.oasis-open.org/archives/virtio-comment/202204/msg00089.html.
Please, have a look!

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.





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.