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

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

[Laura Loghin]

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.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/140

Signed-off-by: Laura Loghin <lauralg@amazon.com>
---
 conformance.tex  |  2 ++
 virtio-vsock.tex | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index 42f8537..77f7583 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -222,6 +222,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 A socket driver MUST conform to the following normative statements:
 
 \begin{itemize}
+\item \ref{drivernormative:Device Types / Socket Device / Device configuration layout}
 \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Buffer Space Management}
 \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
 \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Device Events}
@@ -481,6 +482,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 A socket device MUST conform to the following normative statements:
 
 \begin{itemize}
+\item \ref{devicenormative:Device Types / Socket Device / Device configuration layout}
 \item \ref{devicenormative:Device Types / Socket Device / Device Operation / Buffer Space Management}
 \item \ref{devicenormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
 \end{itemize}
diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index d79984d..5db6110 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,25 @@ \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}.
+
+\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, the receive buffers it
+supplies for a packet MUST have a total size that doesn'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




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.


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/

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

[Cornelia Huck]

On Mon, Jun 13 2022, Laura Loghin <lauralg@amazon.com> 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.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/140
>
> Signed-off-by: Laura Loghin <lauralg@amazon.com>
> ---
>  conformance.tex  |  2 ++
>  virtio-vsock.tex | 24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)

I think you dropped Stefano's R-b by accident -- Stefano, I guess it
still holds? (I can readd when merging).

Looks good to me now; if nobody else has any comments, we can start voting.

>
> diff --git a/conformance.tex b/conformance.tex
> index 42f8537..77f7583 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -222,6 +222,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  A socket driver MUST conform to the following normative statements:
>  
>  \begin{itemize}
> +\item \ref{drivernormative:Device Types / Socket Device / Device configuration layout}
>  \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Buffer Space Management}
>  \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
>  \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Device Events}
> @@ -481,6 +482,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  A socket device MUST conform to the following normative statements:
>  
>  \begin{itemize}
> +\item \ref{devicenormative:Device Types / Socket Device / Device configuration layout}
>  \item \ref{devicenormative:Device Types / Socket Device / Device Operation / Buffer Space Management}
>  \item \ref{devicenormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
>  \end{itemize}
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index d79984d..5db6110 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,25 @@ \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}.
> +
> +\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, the receive buffers it
> +supplies for a packet MUST have a total size that doesn'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}


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/

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

[Stefano Garzarella]

On Mon, Jun 13, 2022 at 01:50:41PM +0200, Cornelia Huck wrote:
>On Mon, Jun 13 2022, Laura Loghin <lauralg@amazon.com> 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.
>>
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/140
>>
>> Signed-off-by: Laura Loghin <lauralg@amazon.com>
>> ---
>>  conformance.tex  |  2 ++
>>  virtio-vsock.tex | 24 ++++++++++++++++++++++++
>>  2 files changed, 26 insertions(+)
>
>I think you dropped Stefano's R-b by accident -- Stefano, I guess it
>still holds? (I can readd when merging).

Yep, thanks for CCing me!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>Looks good to me now; if nobody else has any comments, we can start voting.
>
>>
>> diff --git a/conformance.tex b/conformance.tex
>> index 42f8537..77f7583 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -222,6 +222,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>  A socket driver MUST conform to the following normative statements:
>>
>>  \begin{itemize}
>> +\item \ref{drivernormative:Device Types / Socket Device / Device configuration layout}
>>  \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Buffer Space Management}
>>  \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
>>  \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Device Events}
>> @@ -481,6 +482,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>  A socket device MUST conform to the following normative statements:
>>
>>  \begin{itemize}
>> +\item \ref{devicenormative:Device Types / Socket Device / Device configuration layout}
>>  \item \ref{devicenormative:Device Types / Socket Device / Device Operation / Buffer Space Management}
>>  \item \ref{devicenormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
>>  \end{itemize}
>> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>> index d79984d..5db6110 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,25 @@ \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}.
>> +
>> +\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, the receive buffers it
>> +supplies for a packet MUST have a total size that doesn'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}
>

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

[Halil Pasic]

On Mon, 13 Jun 2022 13:40:38 +0300
Laura Loghin <lauralg@amazon.com> wrote:

> @@ -57,6 +62,25 @@ \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}.
> +
> +\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, the receive buffers it
> +supplies for a packet MUST have a total size that doesn'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).
> +

Hi and sorry for being late to the party!

I believe I do understand why do we want to put a restriction on the
size of the transmitted packets, but I would appreciate if you could
explain to me why do we want to limit the receive buffer size.

Also I find the wording regarding a little bit ambiguous because
in a networking context it also makes sense to talk about the size of the
receive buffer. I guess hear we are talking about a single virtio buffer
(a descriptor chain described potentially non-continuous (or compact in
the mathematical sense of the word) which is composed from as many
continuous chunks of memory as many descriptors are contained within the
descriptor chain). If we are indeed talking about a single virtio buffer,
I don't understand the plural. If not, I'm not sure what are we talking
about.

Also, do we have some sort of packets may not cross virtio buffer
boundaries, or even a single packet per single viritio buffer rule for
vsock. I did a quick search and could not find any. Did I overlook
something? Should we spell this out?

@Michael, Conny: What do you think? 

Regards,
Halil

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/

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

[Michael S. Tsirkin]

On Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
> On Mon, 13 Jun 2022 13:40:38 +0300
> Laura Loghin <lauralg@amazon.com> wrote:
> 
> > @@ -57,6 +62,25 @@ \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}.
> > +
> > +\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, the receive buffers it
> > +supplies for a packet MUST have a total size that doesn'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).
> > +
> 
> Hi and sorry for being late to the party!
> 
> I believe I do understand why do we want to put a restriction on the
> size of the transmitted packets, but I would appreciate if you could
> explain to me why do we want to limit the receive buffer size.
> 
> Also I find the wording regarding a little bit ambiguous because
> in a networking context it also makes sense to talk about the size of the
> receive buffer. I guess hear we are talking about a single virtio buffer
> (a descriptor chain described potentially non-continuous (or compact in
> the mathematical sense of the word) which is composed from as many
> continuous chunks of memory as many descriptors are contained within the
> descriptor chain). If we are indeed talking about a single virtio buffer,
> I don't understand the plural. If not, I'm not sure what are we talking
> about.

I think I agree here, I don't understand the mix of "buffers" and "a
packet" either.

I voted "no" on the ballot, though if others feel we should apply as
is and fix up later, that is not too bad.


> Also, do we have some sort of packets may not cross virtio buffer
> boundaries, or even a single packet per single viritio buffer rule for
> vsock. I did a quick search and could not find any. Did I overlook
> something? Should we spell this out?
> 
> @Michael, Conny: What do you think? 
> 
> Regards,
> Halil

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

[Cornelia Huck]

On Thu, Jun 16 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
>> On Mon, 13 Jun 2022 13:40:38 +0300
>> Laura Loghin <lauralg@amazon.com> wrote:
>> 
>> > @@ -57,6 +62,25 @@ \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}.
>> > +
>> > +\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, the receive buffers it
>> > +supplies for a packet MUST have a total size that doesn'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).
>> > +
>> 
>> Hi and sorry for being late to the party!
>> 
>> I believe I do understand why do we want to put a restriction on the
>> size of the transmitted packets, but I would appreciate if you could
>> explain to me why do we want to limit the receive buffer size.
>> 
>> Also I find the wording regarding a little bit ambiguous because
>> in a networking context it also makes sense to talk about the size of the
>> receive buffer. I guess hear we are talking about a single virtio buffer
>> (a descriptor chain described potentially non-continuous (or compact in
>> the mathematical sense of the word) which is composed from as many
>> continuous chunks of memory as many descriptors are contained within the
>> descriptor chain). If we are indeed talking about a single virtio buffer,
>> I don't understand the plural. If not, I'm not sure what are we talking
>> about.
>
> I think I agree here, I don't understand the mix of "buffers" and "a
> packet" either.
>
> I voted "no" on the ballot, though if others feel we should apply as
> is and fix up later, that is not too bad.

I now switched to "no" as well; it's not too bad to fix things later,
but it would be good if we had a common understanding before the change
goes in.


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/

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

[Laura Loghin]

[-- Attachment #1: Type: text/plain, Size: 3165 bytes --]

On 6/20/22 12:43, Cornelia Huck 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 Thu, Jun 16 2022, "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>
>> On Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
>>> On Mon, 13 Jun 2022 13:40:38 +0300
>>> Laura Loghin<lauralg@amazon.com>  wrote:
>>>
>>>> @@ -57,6 +62,25 @@ \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}.
>>>> +
>>>> +\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, the receive buffers it
>>>> +supplies for a packet MUST have a total size that doesn'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).
>>>> +
>>> Hi and sorry for being late to the party!
>>>
>>> I believe I do understand why do we want to put a restriction on the
>>> size of the transmitted packets, but I would appreciate if you could
>>> explain to me why do we want to limit the receive buffer size.
>>>
>>> Also I find the wording regarding a little bit ambiguous because
>>> in a networking context it also makes sense to talk about the size of the
>>> receive buffer. I guess hear we are talking about a single virtio buffer
>>> (a descriptor chain described potentially non-continuous (or compact in
>>> the mathematical sense of the word) which is composed from as many
>>> continuous chunks of memory as many descriptors are contained within the
>>> descriptor chain). If we are indeed talking about a single virtio buffer,
>>> I don't understand the plural. If not, I'm not sure what are we talking
>>> about.
>> I think I agree here, I don't understand the mix of "buffers" and "a
>> packet" either.
>>
>> I voted "no" on the ballot, though if others feel we should apply as
>> is and fix up later, that is not too bad.
> I now switched to "no" as well; it's not too bad to fix things later,
> but it would be good if we had a common understanding before the change
> goes in.
>
Hi!

Sorry, I will address your comments this week. Thanks everyone for the feedback!

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.

[-- Attachment #2: Type: text/html, Size: 3954 bytes --]

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

[Michael S. Tsirkin]

On Mon, Jun 20, 2022 at 11:43:57AM +0200, Cornelia Huck wrote:
> On Thu, Jun 16 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
> >> On Mon, 13 Jun 2022 13:40:38 +0300
> >> Laura Loghin <lauralg@amazon.com> wrote:
> >> 
> >> > @@ -57,6 +62,25 @@ \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}.
> >> > +
> >> > +\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, the receive buffers it
> >> > +supplies for a packet MUST have a total size that doesn'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).
> >> > +
> >> 
> >> Hi and sorry for being late to the party!
> >> 
> >> I believe I do understand why do we want to put a restriction on the
> >> size of the transmitted packets, but I would appreciate if you could
> >> explain to me why do we want to limit the receive buffer size.
> >> 
> >> Also I find the wording regarding a little bit ambiguous because
> >> in a networking context it also makes sense to talk about the size of the
> >> receive buffer. I guess hear we are talking about a single virtio buffer
> >> (a descriptor chain described potentially non-continuous (or compact in
> >> the mathematical sense of the word) which is composed from as many
> >> continuous chunks of memory as many descriptors are contained within the
> >> descriptor chain). If we are indeed talking about a single virtio buffer,
> >> I don't understand the plural. If not, I'm not sure what are we talking
> >> about.
> >
> > I think I agree here, I don't understand the mix of "buffers" and "a
> > packet" either.
> >
> > I voted "no" on the ballot, though if others feel we should apply as
> > is and fix up later, that is not too bad.
> 
> I now switched to "no" as well; it's not too bad to fix things later,
> but it would be good if we had a common understanding before the change
> goes in.

If anyone else intends to change their votes, note today is
the last opportunity.

Alternatively, Laura, would you consider to address the issues?
If you want us to withdraw the ballot to consider the options,
that is ok too.

-- 
MST

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

[Laura Loghin]

[-- Attachment #1: Type: text/plain, Size: 3676 bytes --]

On 6/20/22 13:13, Michael S. Tsirkin 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, Jun 20, 2022 at 11:43:57AM +0200, Cornelia Huck wrote:
>> On Thu, Jun 16 2022, "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>>
>>> On Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
>>>> On Mon, 13 Jun 2022 13:40:38 +0300
>>>> Laura Loghin<lauralg@amazon.com>  wrote:
>>>>
>>>>> @@ -57,6 +62,25 @@ \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}.
>>>>> +
>>>>> +\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, the receive buffers it
>>>>> +supplies for a packet MUST have a total size that doesn'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).
>>>>> +
>>>> Hi and sorry for being late to the party!
>>>>
>>>> I believe I do understand why do we want to put a restriction on the
>>>> size of the transmitted packets, but I would appreciate if you could
>>>> explain to me why do we want to limit the receive buffer size.
>>>>
>>>> Also I find the wording regarding a little bit ambiguous because
>>>> in a networking context it also makes sense to talk about the size of the
>>>> receive buffer. I guess hear we are talking about a single virtio buffer
>>>> (a descriptor chain described potentially non-continuous (or compact in
>>>> the mathematical sense of the word) which is composed from as many
>>>> continuous chunks of memory as many descriptors are contained within the
>>>> descriptor chain). If we are indeed talking about a single virtio buffer,
>>>> I don't understand the plural. If not, I'm not sure what are we talking
>>>> about.
>>> I think I agree here, I don't understand the mix of "buffers" and "a
>>> packet" either.
>>>
>>> I voted "no" on the ballot, though if others feel we should apply as
>>> is and fix up later, that is not too bad.
>> I now switched to "no" as well; it's not too bad to fix things later,
>> but it would be good if we had a common understanding before the change
>> goes in.
> If anyone else intends to change their votes, note today is
> the last opportunity.
>
> Alternatively, Laura, would you consider to address the issues?
> If you want us to withdraw the ballot to consider the options,
> that is ok too.
>
> --
> MST
>
Hi,

I sent a message a bit earlier. I plan to address the comments this week. I'm not yet very familiar with the voting process, but if addressing the comments means withdrawing the ballot, that is fine on my side. 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.

[-- Attachment #2: Type: text/html, Size: 4604 bytes --]

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

[Cornelia Huck]

On Mon, Jun 20 2022, Laura Loghin <lauralg@amazon.com> wrote:

> On 6/20/22 13:13, Michael S. Tsirkin wrote:
>> If anyone else intends to change their votes, note today is
>> the last opportunity.
>>
>> Alternatively, Laura, would you consider to address the issues?
>> If you want us to withdraw the ballot to consider the options,
>> that is ok too.
>>
>> --
>> MST
>>
> Hi,
>
> I sent a message a bit earlier. I plan to address the comments this week. I'm not yet very familiar with the voting process, but if addressing the comments means withdrawing the ballot, that is fine on my side. Thanks!

"Withdrawing the ballot" is supposed to be an active action (so it needs
to be requested explicitly IMHO, so I'd prefer to have an explict
statement before doing so).

If you plan to send a new version to be voted upon, please just state
that you'd like the ballot to be withdrawn. Thanks!


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/

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

[Laura Loghin]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

On 6/20/22 14:20, Cornelia Huck 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, Jun 20 2022, Laura Loghin<lauralg@amazon.com>  wrote:
>
>> On 6/20/22 13:13, Michael S. Tsirkin wrote:
>>> If anyone else intends to change their votes, note today is
>>> the last opportunity.
>>>
>>> Alternatively, Laura, would you consider to address the issues?
>>> If you want us to withdraw the ballot to consider the options,
>>> that is ok too.
>>>
>>> --
>>> MST
>>>
>> Hi,
>>
>> I sent a message a bit earlier. I plan to address the comments this week. I'm not yet very familiar with the voting process, but if addressing the comments means withdrawing the ballot, that is fine on my side. Thanks!
> "Withdrawing the ballot" is supposed to be an active action (so it needs
> to be requested explicitly IMHO, so I'd prefer to have an explict
> statement before doing so).
>
> If you plan to send a new version to be voted upon, please just state
> that you'd like the ballot to be withdrawn. Thanks!
>
OK, I'd like my ballot to be withdrawn.

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.

[-- Attachment #2: Type: text/html, Size: 2086 bytes --]

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

[Cornelia Huck]

On Mon, Jun 20 2022, Laura Loghin <lauralg@amazon.com> wrote:

> On 6/20/22 14:20, Cornelia Huck 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, Jun 20 2022, Laura Loghin<lauralg@amazon.com>  wrote:
>>
>>> On 6/20/22 13:13, Michael S. Tsirkin wrote:
>>>> If anyone else intends to change their votes, note today is
>>>> the last opportunity.
>>>>
>>>> Alternatively, Laura, would you consider to address the issues?
>>>> If you want us to withdraw the ballot to consider the options,
>>>> that is ok too.
>>>>
>>>> --
>>>> MST
>>>>
>>> Hi,
>>>
>>> I sent a message a bit earlier. I plan to address the comments this week. I'm not yet very familiar with the voting process, but if addressing the comments means withdrawing the ballot, that is fine on my side. Thanks!
>> "Withdrawing the ballot" is supposed to be an active action (so it needs
>> to be requested explicitly IMHO, so I'd prefer to have an explict
>> statement before doing so).
>>
>> If you plan to send a new version to be voted upon, please just state
>> that you'd like the ballot to be withdrawn. Thanks!
>>
> OK, I'd like my ballot to be withdrawn.

Thanks for the confirmation, I just did that.


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/

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

[Laura Loghin]

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On 6/20/22 16:29, Cornelia Huck 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, Jun 20 2022, Laura Loghin<lauralg@amazon.com>  wrote:
>
>> On 6/20/22 14:20, Cornelia Huck 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, Jun 20 2022, Laura Loghin<lauralg@amazon.com>   wrote:
>>>
>>>> On 6/20/22 13:13, Michael S. Tsirkin wrote:
>>>>> If anyone else intends to change their votes, note today is
>>>>> the last opportunity.
>>>>>
>>>>> Alternatively, Laura, would you consider to address the issues?
>>>>> If you want us to withdraw the ballot to consider the options,
>>>>> that is ok too.
>>>>>
>>>>> --
>>>>> MST
>>>>>
>>>> Hi,
>>>>
>>>> I sent a message a bit earlier. I plan to address the comments this week. I'm not yet very familiar with the voting process, but if addressing the comments means withdrawing the ballot, that is fine on my side. Thanks!
>>> "Withdrawing the ballot" is supposed to be an active action (so it needs
>>> to be requested explicitly IMHO, so I'd prefer to have an explict
>>> statement before doing so).
>>>
>>> If you plan to send a new version to be voted upon, please just state
>>> that you'd like the ballot to be withdrawn. Thanks!
>>>
>> OK, I'd like my ballot to be withdrawn.
> Thanks for the confirmation, I just did that.
>
Hello,

I just sent a new version
https://lists.oasis-open.org/archives/virtio-comment/202206/msg00088.html  in
which I hopefully addressed the outstanding issues. Please have another look!

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.

[-- Attachment #2: Type: text/html, Size: 3073 bytes --]

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

[Laura Loghin]

[-- Attachment #1: Type: text/plain, Size: 3594 bytes --]

On 6/16/22 19:40, Michael S. Tsirkin 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 Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
>> On Mon, 13 Jun 2022 13:40:38 +0300
>> Laura Loghin<lauralg@amazon.com>  wrote:
>>
>>> @@ -57,6 +62,25 @@ \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}.
>>> +
>>> +\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, the receive buffers it
>>> +supplies for a packet MUST have a total size that doesn'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).
>>> +
>> Hi and sorry for being late to the party!
>>
>> I believe I do understand why do we want to put a restriction on the
>> size of the transmitted packets, but I would appreciate if you could
>> explain to me why do we want to limit the receive buffer size.
>>
>> Also I find the wording regarding a little bit ambiguous because
>> in a networking context it also makes sense to talk about the size of the
>> receive buffer. I guess hear we are talking about a single virtio buffer
>> (a descriptor chain described potentially non-continuous (or compact in
>> the mathematical sense of the word) which is composed from as many
>> continuous chunks of memory as many descriptors are contained within the
>> descriptor chain). If we are indeed talking about a single virtio buffer,
>> I don't understand the plural. If not, I'm not sure what are we talking
>> about.
> I think I agree here, I don't understand the mix of "buffers" and "a
> packet" either.

The way I was understanding that while reading the spec is that a buffer
is corresponding to one descriptor, so a packet will correspond to
multiple buffers (like for example in Linux one buffer for the packet
header and one buffer for the payload). I wanted to limit the memory
allocated by the driver for RX buffers and TX buffers, so that's why I
used 'buffers' for RX. Does it make sense or did I misunderstand what
was causing the confusion here?

Thanks,
Laura

>
> I voted "no" on the ballot, though if others feel we should apply as
> is and fix up later, that is not too bad.
>
>
>> Also, do we have some sort of packets may not cross virtio buffer
>> boundaries, or even a single packet per single viritio buffer rule for
>> vsock. I did a quick search and could not find any. Did I overlook
>> something? Should we spell this out?
>>
>> @Michael, Conny: What do you think?
>>
>> Regards,
>> Halil




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.

[-- Attachment #2: Type: text/html, Size: 4488 bytes --]

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

[Michael S. Tsirkin]

On Thu, Jun 23, 2022 at 06:36:50PM +0300, Laura Loghin wrote:
> On 6/16/22 19:40, Michael S. Tsirkin 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 Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
> 
>         On Mon, 13 Jun 2022 13:40:38 +0300
>         Laura Loghin <lauralg@amazon.com> wrote:
> 
> 
>             @@ -57,6 +62,25 @@ \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}.
>             +
>             +\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, the receive buffers it
>             +supplies for a packet MUST have a total size that doesn'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).
>             +
> 
>         Hi and sorry for being late to the party!
> 
>         I believe I do understand why do we want to put a restriction on the
>         size of the transmitted packets, but I would appreciate if you could
>         explain to me why do we want to limit the receive buffer size.
> 
>         Also I find the wording regarding a little bit ambiguous because
>         in a networking context it also makes sense to talk about the size of the
>         receive buffer. I guess hear we are talking about a single virtio buffer
>         (a descriptor chain described potentially non-continuous (or compact in
>         the mathematical sense of the word) which is composed from as many
>         continuous chunks of memory as many descriptors are contained within the
>         descriptor chain). If we are indeed talking about a single virtio buffer,
>         I don't understand the plural. If not, I'm not sure what are we talking
>         about.
> 
>     I think I agree here, I don't understand the mix of "buffers" and "a
>     packet" either.
> 
> The way I was understanding that while reading the spec is that a buffer
> is corresponding to one descriptor,

what gave this impression? buffers can use any number of descriptors.

> so a packet will correspond to
> multiple buffers (like for example in Linux one buffer for the packet
> header and one buffer for the payload). I wanted to limit the memory
> allocated by the driver for RX buffers and TX buffers, so that's why I
> used 'buffers' for RX. Does it make sense or did I misunderstand what
> was causing the confusion here?
> 
> Thanks,
> Laura


As above, a buffer can consist of many descriptors. See e.g.
Descriptor Chaining.
I think there are places in spec when say "descriptor" and we should
fix them to say one or more descriptors.

> 
>     I voted "no" on the ballot, though if others feel we should apply as
>     is and fix up later, that is not too bad.
> 
> 
> 
>         Also, do we have some sort of packets may not cross virtio buffer
>         boundaries, or even a single packet per single viritio buffer rule for
>         vsock. I did a quick search and could not find any. Did I overlook
>         something? Should we spell this out?
> 
>         @Michael, Conny: What do you think?
> 
>         Regards,
>         Halil
> 
> 
> 
> 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.
> 

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

[Laura Loghin]

[-- Attachment #1: Type: text/plain, Size: 5903 bytes --]

On 6/23/22 19:22, Michael S. Tsirkin 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 Thu, Jun 23, 2022 at 06:36:50PM +0300, Laura Loghin wrote:
>> On 6/16/22 19:40, Michael S. Tsirkin 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 Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
>>
>>          On Mon, 13 Jun 2022 13:40:38 +0300
>>          Laura Loghin<lauralg@amazon.com>  wrote:
>>
>>
>>              @@ -57,6 +62,25 @@ \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}.
>>              +
>>              +\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, the receive buffers it
>>              +supplies for a packet MUST have a total size that doesn'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).
>>              +
>>
>>          Hi and sorry for being late to the party!
>>
>>          I believe I do understand why do we want to put a restriction on the
>>          size of the transmitted packets, but I would appreciate if you could
>>          explain to me why do we want to limit the receive buffer size.
>>
>>          Also I find the wording regarding a little bit ambiguous because
>>          in a networking context it also makes sense to talk about the size of the
>>          receive buffer. I guess hear we are talking about a single virtio buffer
>>          (a descriptor chain described potentially non-continuous (or compact in
>>          the mathematical sense of the word) which is composed from as many
>>          continuous chunks of memory as many descriptors are contained within the
>>          descriptor chain). If we are indeed talking about a single virtio buffer,
>>          I don't understand the plural. If not, I'm not sure what are we talking
>>          about.
>>
>>      I think I agree here, I don't understand the mix of "buffers" and "a
>>      packet" either.
>>
>> The way I was understanding that while reading the spec is that a buffer
>> is corresponding to one descriptor,
> what gave this impression? buffers can use any number of descriptors.
>
There are sections in the spec that make you think of a buffer as
corresponding to a descriptor chain, and others that don't, for example:
2.6.5    The Virtqueue Descriptor Table: Each descriptor describes a
buffer which is read-only for the device (“device-readable”) or
write-only for the device (“device-writable”), but a chain of descriptors
can contain both device-readable and device-writable buffers.
I remember while reading the spec I was always confused about the buffer
- descriptor (chain) relation, but in the end concluded that the buffer
is the memory region to which a single descriptor is pointing to (probably
also because when you think of a buffer you would at first assume it is
contiguous in memory).
This doesn't seem to be the right understanding, so I will update the
patch.

>> so a packet will correspond to
>> multiple buffers (like for example in Linux one buffer for the packet
>> header and one buffer for the payload). I wanted to limit the memory
>> allocated by the driver for RX buffers and TX buffers, so that's why I
>> used 'buffers' for RX. Does it make sense or did I misunderstand what
>> was causing the confusion here?
>>
>> Thanks,
>> Laura
>
> As above, a buffer can consist of many descriptors. See e.g.
> Descriptor Chaining.
> I think there are places in spec when say "descriptor" and we should
> fix them to say one or more descriptors.

It would be great if we could fix the spec, so that the relation between
a descriptor and a buffer becomes more clear and doesn't leave room for
interpretations.


Laura

>>      I voted "no" on the ballot, though if others feel we should apply as
>>      is and fix up later, that is not too bad.
>>
>>
>>
>>          Also, do we have some sort of packets may not cross virtio buffer
>>          boundaries, or even a single packet per single viritio buffer rule for
>>          vsock. I did a quick search and could not find any. Did I overlook
>>          something? Should we spell this out?
>>
>>          @Michael, Conny: What do you think?
>>
>>          Regards,
>>          Halil
>>
>>
>>
>> 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.

[-- Attachment #2: Type: text/html, Size: 6804 bytes --]

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

[Halil Pasic]

On Fri, 24 Jun 2022 11:23:31 +0300
Laura Loghin <lauralg@amazon.com> wrote:

> On 6/23/22 19:22, Michael S. Tsirkin 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 Thu, Jun 23, 2022 at 06:36:50PM +0300, Laura Loghin wrote:  
> >> On 6/16/22 19:40, Michael S. Tsirkin 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 Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
> >>
> >>          On Mon, 13 Jun 2022 13:40:38 +0300
> >>          Laura Loghin<lauralg@amazon.com>  wrote:
> >>
> >>
> >>              @@ -57,6 +62,25 @@ \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}.
> >>              +
> >>              +\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, the receive buffers it
> >>              +supplies for a packet MUST have a total size that doesn'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).
> >>              +
> >>
> >>          Hi and sorry for being late to the party!
> >>
> >>          I believe I do understand why do we want to put a restriction on the
> >>          size of the transmitted packets, but I would appreciate if you could
> >>          explain to me why do we want to limit the receive buffer size.
> >>
> >>          Also I find the wording regarding a little bit ambiguous because
> >>          in a networking context it also makes sense to talk about the size of the
> >>          receive buffer. I guess hear we are talking about a single virtio buffer
> >>          (a descriptor chain described potentially non-continuous (or compact in
> >>          the mathematical sense of the word) which is composed from as many
> >>          continuous chunks of memory as many descriptors are contained within the
> >>          descriptor chain). If we are indeed talking about a single virtio buffer,
> >>          I don't understand the plural. If not, I'm not sure what are we talking
> >>          about.
> >>
> >>      I think I agree here, I don't understand the mix of "buffers" and "a
> >>      packet" either.
> >>
> >> The way I was understanding that while reading the spec is that a buffer
> >> is corresponding to one descriptor,  
> > what gave this impression? buffers can use any number of descriptors.
> >  
> There are sections in the spec that make you think of a buffer as
> corresponding to a descriptor chain, and others that don't, for example:
> 2.6.5    The Virtqueue Descriptor Table: Each descriptor describes a
> buffer which is read-only for the device (“device-readable”) or
> write-only for the device (“device-writable”), but a chain of descriptors
> can contain both device-readable and device-writable buffers.
> I remember while reading the spec I was always confused about the buffer
> - descriptor (chain) relation, but in the end concluded that the buffer
> is the memory region to which a single descriptor is pointing to (probably
> also because when you think of a buffer you would at first assume it is
> contiguous in memory).

I think in the distant past I have pointed out that the usage of the word
buffer in the virtio spec is somewhat confusing. But I never took the
time to make a patch which attempts to set it strait.

IMHO eve if we were to use "buffer" consistently in a sense that, it is
a portion of the RAM which may or may not be continuous in the address
space used by virtio (guest physical or DMA depending on ...) where
chained descriptors describe continuous pieces of the same, we would
still have the problem, that in the normal CS world AFAIK a buffer is
usually a continuous piece of memory used for a certain purpose.
https://en.wikipedia.org/wiki/Data_buffer

Well actually our buffers are continuous in a sense that each descriptor
chain defines and has an address space of its own. In a sense that we
can talk about the N-th byte of the L long buffer (N < L), but that
the difference between addresses of the 0-th and the N-th byte of an
virtio buffer described by a descriptor chain in DMA/guest physical
address space isn't guaranteed to be N.

One solution would be to define the terms:
virtqueue buffer: a portion of a RAM that is composed by one or more
primitive buffers and a descriptor chain such that at least one
descriptor in the chain is pointed to each primitive buffer
primitive buffer: a byte array in RAM which is continuous in
the address space used by the spec
and use this terms consistently. That is never use buffer without
a qualifier.


BTW I don't remember if the our primitive buffers are allowed to cross
page boundaries.

> This doesn't seem to be the right understanding, so I will update the
> patch.
> 
> >> so a packet will correspond to
> >> multiple buffers (like for example in Linux one buffer for the packet
> >> header and one buffer for the payload). I wanted to limit the memory
> >> allocated by the driver for RX buffers and TX buffers, so that's why I
> >> used 'buffers' for RX. Does it make sense or did I misunderstand what
> >> was causing the confusion here?
> >>
> >> Thanks,
> >> Laura  
> >
> > As above, a buffer can consist of many descriptors. See e.g.
> > Descriptor Chaining.
> > I think there are places in spec when say "descriptor" and we should
> > fix them to say one or more descriptors.  
> 
> It would be great if we could fix the spec, so that the relation between
> a descriptor and a buffer becomes more clear and doesn't leave room for
> interpretations.

+1

Regards,
Halil

> 
> 
> Laura
> 
> >>      I voted "no" on the ballot, though if others feel we should apply as
> >>      is and fix up later, that is not too bad.
> >>
> >>
> >>
> >>          Also, do we have some sort of packets may not cross virtio buffer
> >>          boundaries, or even a single packet per single viritio buffer rule for
> >>          vsock. I did a quick search and could not find any. Did I overlook
> >>          something? Should we spell this out?
> >>
> >>          @Michael, Conny: What do you think?
> >>
> >>          Regards,
> >>          Halil
> >>
> >>
> >>
> >> 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.

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

[Michael S. Tsirkin]

On Mon, Jun 27, 2022 at 02:59:07PM +0200, Halil Pasic wrote:
> On Fri, 24 Jun 2022 11:23:31 +0300
> Laura Loghin <lauralg@amazon.com> wrote:
> 
> > On 6/23/22 19:22, Michael S. Tsirkin 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 Thu, Jun 23, 2022 at 06:36:50PM +0300, Laura Loghin wrote:  
> > >> On 6/16/22 19:40, Michael S. Tsirkin 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 Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
> > >>
> > >>          On Mon, 13 Jun 2022 13:40:38 +0300
> > >>          Laura Loghin<lauralg@amazon.com>  wrote:
> > >>
> > >>
> > >>              @@ -57,6 +62,25 @@ \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}.
> > >>              +
> > >>              +\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, the receive buffers it
> > >>              +supplies for a packet MUST have a total size that doesn'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).
> > >>              +
> > >>
> > >>          Hi and sorry for being late to the party!
> > >>
> > >>          I believe I do understand why do we want to put a restriction on the
> > >>          size of the transmitted packets, but I would appreciate if you could
> > >>          explain to me why do we want to limit the receive buffer size.
> > >>
> > >>          Also I find the wording regarding a little bit ambiguous because
> > >>          in a networking context it also makes sense to talk about the size of the
> > >>          receive buffer. I guess hear we are talking about a single virtio buffer
> > >>          (a descriptor chain described potentially non-continuous (or compact in
> > >>          the mathematical sense of the word) which is composed from as many
> > >>          continuous chunks of memory as many descriptors are contained within the
> > >>          descriptor chain). If we are indeed talking about a single virtio buffer,
> > >>          I don't understand the plural. If not, I'm not sure what are we talking
> > >>          about.
> > >>
> > >>      I think I agree here, I don't understand the mix of "buffers" and "a
> > >>      packet" either.
> > >>
> > >> The way I was understanding that while reading the spec is that a buffer
> > >> is corresponding to one descriptor,  
> > > what gave this impression? buffers can use any number of descriptors.
> > >  
> > There are sections in the spec that make you think of a buffer as
> > corresponding to a descriptor chain, and others that don't, for example:
> > 2.6.5    The Virtqueue Descriptor Table: Each descriptor describes a
> > buffer which is read-only for the device (“device-readable”) or
> > write-only for the device (“device-writable”), but a chain of descriptors
> > can contain both device-readable and device-writable buffers.
> > I remember while reading the spec I was always confused about the buffer
> > - descriptor (chain) relation, but in the end concluded that the buffer
> > is the memory region to which a single descriptor is pointing to (probably
> > also because when you think of a buffer you would at first assume it is
> > contiguous in memory).

well we have wording like

Device reports the number of bytes it has written to memory for
each buffer it uses. This is referred to as ``used length''.



> I think in the distant past I have pointed out that the usage of the word
> buffer in the virtio spec is somewhat confusing. But I never took the
> time to make a patch which attempts to set it strait.
> 
> IMHO eve if we were to use "buffer" consistently in a sense that, it is
> a portion of the RAM which may or may not be continuous in the address
> space used by virtio (guest physical or DMA depending on ...) where
> chained descriptors describe continuous pieces of the same, we would
> still have the problem, that in the normal CS world AFAIK a buffer is
> usually a continuous piece of memory used for a certain purpose.
> https://en.wikipedia.org/wiki/Data_buffer
> 
> Well actually our buffers are continuous in a sense that each descriptor
> chain defines and has an address space of its own. In a sense that we
> can talk about the N-th byte of the L long buffer (N < L), but that
> the difference between addresses of the 0-th and the N-th byte of an
> virtio buffer described by a descriptor chain in DMA/guest physical
> address space isn't guaranteed to be N.
> 
> One solution would be to define the terms:
> virtqueue buffer: a portion of a RAM that is composed by one or more
> primitive buffers and a descriptor chain such that at least one
> descriptor in the chain is pointed to each primitive buffer
> primitive buffer: a byte array in RAM which is continuous in
> the address space used by the spec
> and use this terms consistently. That is never use buffer without
> a qualifier.

I would be inclined to avoid saying primitive buffer.  saying
virtqueue buffer is ok if a bit verbose.
Let's start by locating places in spec which say buffer
when they mean descriptor though.



> 
> BTW I don't remember if the our primitive buffers are allowed to cross
> page boundaries.


Spec doesn't say they shouldn't.

> > This doesn't seem to be the right understanding, so I will update the
> > patch.
> > 
> > >> so a packet will correspond to
> > >> multiple buffers (like for example in Linux one buffer for the packet
> > >> header and one buffer for the payload). I wanted to limit the memory
> > >> allocated by the driver for RX buffers and TX buffers, so that's why I
> > >> used 'buffers' for RX. Does it make sense or did I misunderstand what
> > >> was causing the confusion here?
> > >>
> > >> Thanks,
> > >> Laura  
> > >
> > > As above, a buffer can consist of many descriptors. See e.g.
> > > Descriptor Chaining.
> > > I think there are places in spec when say "descriptor" and we should
> > > fix them to say one or more descriptors.  
> > 
> > It would be great if we could fix the spec, so that the relation between
> > a descriptor and a buffer becomes more clear and doesn't leave room for
> > interpretations.
> 
> +1
> 
> Regards,
> Halil
> 
> > 
> > 
> > Laura
> > 
> > >>      I voted "no" on the ballot, though if others feel we should apply as
> > >>      is and fix up later, that is not too bad.
> > >>
> > >>
> > >>
> > >>          Also, do we have some sort of packets may not cross virtio buffer
> > >>          boundaries, or even a single packet per single viritio buffer rule for
> > >>          vsock. I did a quick search and could not find any. Did I overlook
> > >>          something? Should we spell this out?
> > >>
> > >>          @Michael, Conny: What do you think?
> > >>
> > >>          Regards,
> > >>          Halil
> > >>
> > >>
> > >>
> > >> 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.

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

[Laura Loghin]

On 6/27/22 16:15, Michael S. Tsirkin 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, Jun 27, 2022 at 02:59:07PM +0200, Halil Pasic wrote:
>> On Fri, 24 Jun 2022 11:23:31 +0300
>> Laura Loghin <lauralg@amazon.com> wrote:
>>
>>> On 6/23/22 19:22, Michael S. Tsirkin 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 Thu, Jun 23, 2022 at 06:36:50PM +0300, Laura Loghin wrote:
>>>>> On 6/16/22 19:40, Michael S. Tsirkin 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 Tue, Jun 14, 2022 at 01:57:34PM +0200, Halil Pasic wrote:
>>>>>
>>>>>           On Mon, 13 Jun 2022 13:40:38 +0300
>>>>>           Laura Loghin<lauralg@amazon.com>  wrote:
>>>>>
>>>>>
>>>>>               @@ -57,6 +62,25 @@ \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}.
>>>>>               +
>>>>>               +\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, the receive buffers it
>>>>>               +supplies for a packet MUST have a total size that doesn'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).
>>>>>               +
>>>>>
>>>>>           Hi and sorry for being late to the party!
>>>>>
>>>>>           I believe I do understand why do we want to put a restriction on the
>>>>>           size of the transmitted packets, but I would appreciate if you could
>>>>>           explain to me why do we want to limit the receive buffer size.
>>>>>
>>>>>           Also I find the wording regarding a little bit ambiguous because
>>>>>           in a networking context it also makes sense to talk about the size of the
>>>>>           receive buffer. I guess hear we are talking about a single virtio buffer
>>>>>           (a descriptor chain described potentially non-continuous (or compact in
>>>>>           the mathematical sense of the word) which is composed from as many
>>>>>           continuous chunks of memory as many descriptors are contained within the
>>>>>           descriptor chain). If we are indeed talking about a single virtio buffer,
>>>>>           I don't understand the plural. If not, I'm not sure what are we talking
>>>>>           about.
>>>>>
>>>>>       I think I agree here, I don't understand the mix of "buffers" and "a
>>>>>       packet" either.
>>>>>
>>>>> The way I was understanding that while reading the spec is that a buffer
>>>>> is corresponding to one descriptor,
>>>> what gave this impression? buffers can use any number of descriptors.
>>>>
>>> There are sections in the spec that make you think of a buffer as
>>> corresponding to a descriptor chain, and others that don't, for example:
>>> 2.6.5    The Virtqueue Descriptor Table: Each descriptor describes a
>>> buffer which is read-only for the device (“device-readable”) or
>>> write-only for the device (“device-writable”), but a chain of descriptors
>>> can contain both device-readable and device-writable buffers.
>>> I remember while reading the spec I was always confused about the buffer
>>> - descriptor (chain) relation, but in the end concluded that the buffer
>>> is the memory region to which a single descriptor is pointing to (probably
>>> also because when you think of a buffer you would at first assume it is
>>> contiguous in memory).
> well we have wording like
>
> Device reports the number of bytes it has written to memory for
> each buffer it uses. This is referred to as ``used length''.

That's true, there are also sections that make you think of a buffer
belonging to the entire descriptor chain. Hopefully once we update the spec,
the confusion will no longer be possible.

>> I think in the distant past I have pointed out that the usage of the word
>> buffer in the virtio spec is somewhat confusing. But I never took the
>> time to make a patch which attempts to set it strait.
>>
>> IMHO eve if we were to use "buffer" consistently in a sense that, it is
>> a portion of the RAM which may or may not be continuous in the address
>> space used by virtio (guest physical or DMA depending on ...) where
>> chained descriptors describe continuous pieces of the same, we would
>> still have the problem, that in the normal CS world AFAIK a buffer is
>> usually a continuous piece of memory used for a certain purpose.
>> https://en.wikipedia.org/wiki/Data_buffer
>>
>> Well actually our buffers are continuous in a sense that each descriptor
>> chain defines and has an address space of its own. In a sense that we
>> can talk about the N-th byte of the L long buffer (N < L), but that
>> the difference between addresses of the 0-th and the N-th byte of an
>> virtio buffer described by a descriptor chain in DMA/guest physical
>> address space isn't guaranteed to be N.
>>
>> One solution would be to define the terms:
>> virtqueue buffer: a portion of a RAM that is composed by one or more
>> primitive buffers and a descriptor chain such that at least one
>> descriptor in the chain is pointed to each primitive buffer
>> primitive buffer: a byte array in RAM which is continuous in
>> the address space used by the spec
>> and use this terms consistently. That is never use buffer without
>> a qualifier.
> I would be inclined to avoid saying primitive buffer.  saying
> virtqueue buffer is ok if a bit verbose.
> Let's start by locating places in spec which say buffer
> when they mean descriptor though.
>
>
>
>> BTW I don't remember if the our primitive buffers are allowed to cross
>> page boundaries.
>
> Spec doesn't say they shouldn't.
>
>>> This doesn't seem to be the right understanding, so I will update the
>>> patch.
>>>
>>>>> so a packet will correspond to
>>>>> multiple buffers (like for example in Linux one buffer for the packet
>>>>> header and one buffer for the payload). I wanted to limit the memory
>>>>> allocated by the driver for RX buffers and TX buffers, so that's why I
>>>>> used 'buffers' for RX. Does it make sense or did I misunderstand what
>>>>> was causing the confusion here?
>>>>>
>>>>> Thanks,
>>>>> Laura
>>>> As above, a buffer can consist of many descriptors. See e.g.
>>>> Descriptor Chaining.
>>>> I think there are places in spec when say "descriptor" and we should
>>>> fix them to say one or more descriptors.
>>> It would be great if we could fix the spec, so that the relation between
>>> a descriptor and a buffer becomes more clear and doesn't leave room for
>>> interpretations.
>> +1
>>
>> Regards,
>> Halil
>>
>>>
>>> Laura
>>>
>>>>>       I voted "no" on the ballot, though if others feel we should apply as
>>>>>       is and fix up later, that is not too bad.
>>>>>
>>>>>
>>>>>
>>>>>           Also, do we have some sort of packets may not cross virtio buffer
>>>>>           boundaries, or even a single packet per single viritio buffer rule for
>>>>>           vsock. I did a quick search and could not find any. Did I overlook
>>>>>           something? Should we spell this out?
>>>>>
>>>>>           @Michael, Conny: What do you think?
>>>>>
>>>>>           Regards,
>>>>>           Halil
>>>>>
>>>>>
>>>>>
>>>>> 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.





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.

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

[Michael S. Tsirkin]

On Mon, Jun 27, 2022 at 04:33:27PM +0300, Laura Loghin wrote:
> > > > There are sections in the spec that make you think of a buffer as
> > > > corresponding to a descriptor chain, and others that don't, for example:
> > > > 2.6.5    The Virtqueue Descriptor Table: Each descriptor describes a
> > > > buffer which is read-only for the device (“device-readable”) or
> > > > write-only for the device (“device-writable”), but a chain of descriptors
> > > > can contain both device-readable and device-writable buffers.

I think this should just refer to memory:

	Each descriptor describes
	memory which is read-only for the device (``device-readable'') or write-only for the device (``device-writable''), but a chain of
	descriptors can contain both device-readable and device-writable memory.

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

[Laura Loghin]

[-- Attachment #1: Type: text/plain, Size: 3745 bytes --]

On 6/14/22 14:57, Halil Pasic 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, 13 Jun 2022 13:40:38 +0300
> Laura Loghin<lauralg@amazon.com>  wrote:
>
>> @@ -57,6 +62,25 @@ \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}.
>> +
>> +\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, the receive buffers it
>> +supplies for a packet MUST have a total size that doesn'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).
>> +
> Hi and sorry for being late to the party!
>
> I believe I do understand why do we want to put a restriction on the
> size of the transmitted packets, but I would appreciate if you could
> explain to me why do we want to limit the receive buffer size.

My initial proposal was having a limit only for the size of the TX
packets, but Stefano suggested to add it for RX as well, and I found
that okay, since both the RX and TX buffers are provided by the driver,
so they are untrusted. This way we would limit the memory allocated for
both types of buffers, and consequently for the packets as well.

>
> Also I find the wording regarding a little bit ambiguous because
> in a networking context it also makes sense to talk about the size of the
> receive buffer. I guess hear we are talking about a single virtio buffer
> (a descriptor chain described potentially non-continuous (or compact in
> the mathematical sense of the word) which is composed from as many
> continuous chunks of memory as many descriptors are contained within the
> descriptor chain). If we are indeed talking about a single virtio buffer,
> I don't understand the plural. If not, I'm not sure what are we talking
> about.

Hmm so my understanding when reading the spec was that a virtio buffer is
corresponding to a single descriptor (not to a descriptor chain).
Did I misunderstand it the whole time? :(

>
> Also, do we have some sort of packets may not cross virtio buffer
> boundaries, or even a single packet per single viritio buffer rule for
> vsock. I did a quick search and could not find any. Did I overlook
> something? Should we spell this out?

I think the rule would be the same as for the other devices, at least
the ones that I know some things about, so I might be wrong with this
assumption: one message or transmission unit (not sure what would be
the best word here). e.g. request for block, packet for net/vsock
is corresponding to exactly one descriptor chain (so one virtio buffer
if you consider a buffer as being a descriptor chain).

>
> @Michael, Conny: What do you think?
>
> Regards,
> Halil

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.

[-- Attachment #2: Type: text/html, Size: 4719 bytes --]