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

[virtio-comment] [PATCH] 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.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
---
 virtio-vsock.tex | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index d79984d..a6950cc 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -23,6 +23,10 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
 \begin{description}
 \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
 \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
+\item[VIRTIO_VSOCK_F_SIZE_MAX (2)] Maximum size of the packet payload is in
+	\field{data_max_size}. If offered by the device, device advises driver
+	about the value of its maximum payload size. If negotiated, the driver uses
+	\field{data_max_size} as the maximum packet payload size value.
 \end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
@@ -32,6 +36,7 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
 \begin{lstlisting}
 struct virtio_vsock_config {
 	le64 guest_cid;
+	le32 data_max_size;
 };
 \end{lstlisting}
 
@@ -57,6 +62,35 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
 \hline
 \end{tabular}
 
+The following driver-read-only field, \field{data_max_size} only exists if
+VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum packet payload
+size for the driver to use.
+
+\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Socket Device / Device configuration layout}
+
+The device MUST set \field{data_max_size} to between 0 and 65536 inclusive, if
+it offers VIRTIO_VSOCK_F_SIZE_MAX.
+
+The device MUST NOT modify \field{data_max_size} once it has been set.
+
+The device MUST NOT pass received packets that exceed \field{data_max_size}
+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been successfully
+negotiated.
+
+The device MUST forward transmitted packets of up to \field{data_max_size}
+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been successfully
+negotiated.
+
+\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Socket Device / Device configuration layout}
+
+A driver SHOULD negotiate VIRTIO_VSOCK_F_SIZE_MAX if the device offers it.
+
+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST supply receive
+buffers that don't exceed the size \field{data_max_size} (plus header length).
+
+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST  NOT transmit packets
+of size exceeding the value of \field{data_max_size} (plus header length).
+
 \subsection{Device Initialization}\label{sec:Device Types / Socket Device / Device Initialization}
 
 \begin{enumerate}
-- 
2.17.1




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] virtio-vsock: add max payload size config field

[Cornelia Huck]

On Mon, Apr 04 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.
>
> Signed-off-by: Laura Loghin <lauralg@amazon.com>
> ---
>  virtio-vsock.tex | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

(...)

> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Socket Device / Device configuration layout}
> +
> +The device MUST set \field{data_max_size} to between 0 and 65536 inclusive, if
> +it offers VIRTIO_VSOCK_F_SIZE_MAX.
> +
> +The device MUST NOT modify \field{data_max_size} once it has been set.

I'm not sure this is the best way to frame this. Maybe

"The device MUST NOT change the value exposed through
\field{data_max_size}."

?


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] virtio-vsock: add max payload size config field

[Stefano Garzarella]

On Mon, Apr 04, 2022 at 04:49:31PM +0300, Laura Loghin wrote:
>Added a new field to the vsock device config space that
>is limiting the size of the packet payload. This way
>the driver is not allowed to allocate huge buffers, and
>potentially fill up the entire memory.
>Also defined a new feature bit for this, VIRTIO_VSOCK_F_SIZE_MAX,
>in order to keep backwards compatibility.
>
>Signed-off-by: Laura Loghin <lauralg@amazon.com>
>---
> virtio-vsock.tex | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
>diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>index d79984d..a6950cc 100644
>--- a/virtio-vsock.tex
>+++ b/virtio-vsock.tex
>@@ -23,6 +23,10 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
> \begin{description}
> \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
> \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
>+\item[VIRTIO_VSOCK_F_SIZE_MAX (2)] Maximum size of the packet payload is in
>+	\field{data_max_size}. If offered by the device, device advises driver
>+	about the value of its maximum payload size. If negotiated, the driver uses
>+	\field{data_max_size} as the maximum packet payload size value.
> \end{description}
>
> \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
>@@ -32,6 +36,7 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
> \begin{lstlisting}
> struct virtio_vsock_config {
> 	le64 guest_cid;
>+	le32 data_max_size;
> };
> \end{lstlisting}
>
>@@ -57,6 +62,35 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
> \hline
> \end{tabular}
>
>+The following driver-read-only field, \field{data_max_size} only exists if
>+VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum packet payload
>+size for the driver to use.

Should we also define the maximum payload size when the feature is not 
negotiated?
We never defined it, but the current implementation does not use 
payloads larger than 64k.

>+
>+\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Socket Device / Device configuration layout}
>+
>+The device MUST set \field{data_max_size} to between 0 and 65536 

IIUC, this value is the maximum the device can manage, but then the 
driver can always allocate smaller buffers both for TX and RX, so maybe 
we could not put a limit, at the end the device can say what it wants, 
the driver must only avoid to exceed that value.

>inclusive, if
>+it offers VIRTIO_VSOCK_F_SIZE_MAX.
>+
>+The device MUST NOT modify \field{data_max_size} once it has been set.
>+
>+The device MUST NOT pass received packets that exceed \field{data_max_size}
>+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been successfully
>+negotiated.

For the device the limit is anyway the buffer (allocated by the driver) 
it finds in RX, so I don't know if this sentence is useful.

>+
>+The device MUST forward transmitted packets of up to \field{data_max_size}
>+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been successfully
>+negotiated.
>+
>+\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Socket Device / Device configuration layout}
>+
>+A driver SHOULD negotiate VIRTIO_VSOCK_F_SIZE_MAX if the device offers it.
>+
>+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST supply receive
>+buffers that don't exceed the size \field{data_max_size} (plus header length).
>+
>+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST  NOT transmit packets
>+of size exceeding the value of \field{data_max_size} (plus header length).
>+
> \subsection{Device Initialization}\label{sec:Device Types / Socket Device / Device Initialization}
>
> \begin{enumerate}
>-- 2.17.1

I don't know if it is useful to define also a minimum size for the RX 
buffers. It could be useful for example when we will support datagram, 
we could use it to make driver allocate buffers that can contain entire 
datagram.

For now since we only support socket stream maybe it's not useful, so we 
can postpone it. And maybe it's not useful even with datagram if we 
support mergable buffers.

Thanks,
Stefano


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] virtio-vsock: add max payload size config field

[Laura Loghin]

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

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

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



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

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

[Stefano Garzarella]

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

For the driver, I suppose the linux could be the "standard".

>driver/device
>implementation accepted payloads larger than 64k before, then they will be

I'm not sure, IIRC we were limited to 4K, then we increased to 64K for 
the TX:
0038ff357f05 vsock/virtio: change the maximum packet size allowed
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0038ff357f05fee242aa4e5ec5e75f83fa1ed28c

>forced now to negotiate `data_max_size` because of the fixed default value?
>Or that implementation was considered as not being compliant to the spec?
>I'm not exactly sure how backwards compatibility works here.

Yes, I think we can avoid that. When the flag is not negotiated, the 
driver sets the buffer it wants.

>>
>>>+
>>>+\devicenormative{\subsubsection}{Device configuration 
>>>layout}{Device Types / Socket Device / Device configuration 
>>>layout}
>>>+
>>>+The device MUST set \field{data_max_size} to between 0 and 65536
>>
>>IIUC, this value is the maximum the device can manage, but then the
>>driver can always allocate smaller buffers both for TX and RX, so maybe
>>we could not put a limit, at the end the device can say what it wants,
>>the driver must only avoid to exceed that value.
>True, I was thinking that a hard limit might be useful here, but I 
>don't have
>a strong preference. I can remove this.
>>
>>>inclusive, if
>>>+it offers VIRTIO_VSOCK_F_SIZE_MAX.
>>>+
>>>+The device MUST NOT modify \field{data_max_size} once it has been set.
>>>+
>>>+The device MUST NOT pass received packets that exceed 
>>>\field{data_max_size}
>>>+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been 
>>>successfully
>>>+negotiated.
>>
>>For the device the limit is anyway the buffer (allocated by the driver)
>>it finds in RX, so I don't know if this sentence is useful.
>>
>Yes, I don't think this sentence is needed, I'll remove it.
>>>+
>>>+The device MUST forward transmitted packets of up to 
>>>\field{data_max_size}
>>>+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been 
>>>successfully
>>>+negotiated.
>>>+
>>>+\drivernormative{\subsubsection}{Device configuration 
>>>layout}{Device Types / Socket Device / Device configuration 
>>>layout}
>>>+
>>>+A driver SHOULD negotiate VIRTIO_VSOCK_F_SIZE_MAX if the device 
>>>offers it.
>>>+
>>>+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST supply 
>>>receive
>>>+buffers that don't exceed the size \field{data_max_size} (plus 
>>>header length).
>>>+
>>>+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST  NOT 
>>>transmit packets
>>>+of size exceeding the value of \field{data_max_size} (plus header 
>>>length).
>>>+
>>>\subsection{Device Initialization}\label{sec:Device Types / Socket 
>>>Device / Device Initialization}
>>>
>>>\begin{enumerate}
>>>-- 2.17.1
>>
>>I don't know if it is useful to define also a minimum size for the RX
>>buffers.
>
>The reason for that would be to not work with a large number of
>descriptor chains for a packet?

Yes, exactly. I don't know if it makes sense to have it in the 
configuration, though.

Thanks,
Stefano


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] virtio-vsock: add max payload size config field

[Laura Loghin]

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

On 4/6/22 11:59, Cornelia Huck wrote:
> The device MUST NOT change the value exposed through
> \field{data_max_size}.

I updated it in v2:https://lists.oasis-open.org/archives/virtio-comment/202204/msg00062.html.

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: 883 bytes --]

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

[Laura Loghin]

On 4/12/22 17:13, 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 Fri, Apr 08, 2022 at 12:57:19PM +0300, Laura Loghin wrote:
>> On 4/7/22 15:03, Stefano Garzarella wrote:
>>> CAUTION: This email originated from outside of the organization. Do
>>> not click links or open attachments unless you can confirm the
>>> sender and know the content is safe.
>>>
>>>
>>>
>>> On Mon, Apr 04, 2022 at 04:49:31PM +0300, Laura Loghin wrote:
>>>> Added a new field to the vsock device config space that
>>>> is limiting the size of the packet payload. This way
>>>> the driver is not allowed to allocate huge buffers, and
>>>> potentially fill up the entire memory.
>>>> Also defined a new feature bit for this, VIRTIO_VSOCK_F_SIZE_MAX,
>>>> in order to keep backwards compatibility.
>>>>
>>>> Signed-off-by: Laura Loghin <lauralg@amazon.com>
>>>> ---
>>>> virtio-vsock.tex | 34 ++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>>>> index d79984d..a6950cc 100644
>>>> --- a/virtio-vsock.tex
>>>> +++ b/virtio-vsock.tex
>>>> @@ -23,6 +23,10 @@ \subsection{Feature bits}\label{sec:Device
>>>> Types / Socket Device / Feature bits}
>>>> \begin{description}
>>>> \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
>>>> \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is 
>>>> supported.
>>>> +\item[VIRTIO_VSOCK_F_SIZE_MAX (2)] Maximum size of the packet
>>>> payload is in
>>>> +      \field{data_max_size}. If offered by the device, device
>>>> advises driver
>>>> +      about the value of its maximum payload size. If negotiated,
>>>> the driver uses
>>>> +      \field{data_max_size} as the maximum packet payload size value.
>>>> \end{description}
>>>>
>>>> \subsection{Device configuration layout}\label{sec:Device Types /
>>>> Socket Device / Device configuration layout}
>>>> @@ -32,6 +36,7 @@ \subsection{Device configuration
>>>> layout}\label{sec:Device Types / Socket Device
>>>> \begin{lstlisting}
>>>> struct virtio_vsock_config {
>>>>       le64 guest_cid;
>>>> +      le32 data_max_size;
>>>> };
>>>> \end{lstlisting}
>>>>
>>>> @@ -57,6 +62,35 @@ \subsection{Device configuration
>>>> layout}\label{sec:Device Types / Socket Device
>>>> \hline
>>>> \end{tabular}
>>>>
>>>> +The following driver-read-only field, \field{data_max_size} only
>>>> exists if
>>>> +VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum
>>>> packet payload
>>>> +size for the driver to use.
>>>
>>> Should we also define the maximum payload size when the feature is not
>>> negotiated?
>>> We never defined it, but the current implementation does not use
>>> payloads larger than 64k.
>>
>> Hmm I don't know exactly how the spec applies to the vsock. So do we
>> consider the linux implementation as the "standard" one? In case the
>
> For the driver, I suppose the linux could be the "standard".
>
>> driver/device
>> implementation accepted payloads larger than 64k before, then they 
>> will be
>
> I'm not sure, IIRC we were limited to 4K, then we increased to 64K for
> the TX:
> 0038ff357f05 vsock/virtio: change the maximum packet size allowed
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0038ff357f05fee242aa4e5ec5e75f83fa1ed28c 
>
>
>> forced now to negotiate `data_max_size` because of the fixed default 
>> value?
>> Or that implementation was considered as not being compliant to the 
>> spec?
>> I'm not exactly sure how backwards compatibility works here.
>
> Yes, I think we can avoid that. When the flag is not negotiated, the
> driver sets the buffer it wants.
>
>>>
>>>> +
>>>> +\devicenormative{\subsubsection}{Device configuration
>>>> layout}{Device Types / Socket Device / Device configuration
>>>> layout}
>>>> +
>>>> +The device MUST set \field{data_max_size} to between 0 and 65536
>>>
>>> IIUC, this value is the maximum the device can manage, but then the
>>> driver can always allocate smaller buffers both for TX and RX, so maybe
>>> we could not put a limit, at the end the device can say what it wants,
>>> the driver must only avoid to exceed that value.
>> True, I was thinking that a hard limit might be useful here, but I
>> don't have
>> a strong preference. I can remove this.
>>>
>>>> inclusive, if
>>>> +it offers VIRTIO_VSOCK_F_SIZE_MAX.
>>>> +
>>>> +The device MUST NOT modify \field{data_max_size} once it has been 
>>>> set.
>>>> +
>>>> +The device MUST NOT pass received packets that exceed
>>>> \field{data_max_size}
>>>> +(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been
>>>> successfully
>>>> +negotiated.
>>>
>>> For the device the limit is anyway the buffer (allocated by the driver)
>>> it finds in RX, so I don't know if this sentence is useful.
>>>
>> Yes, I don't think this sentence is needed, I'll remove it.
>>>> +
>>>> +The device MUST forward transmitted packets of up to
>>>> \field{data_max_size}
>>>> +(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been
>>>> successfully
>>>> +negotiated.
>>>> +
>>>> +\drivernormative{\subsubsection}{Device configuration
>>>> layout}{Device Types / Socket Device / Device configuration
>>>> layout}
>>>> +
>>>> +A driver SHOULD negotiate VIRTIO_VSOCK_F_SIZE_MAX if the device
>>>> offers it.
>>>> +
>>>> +If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST supply
>>>> receive
>>>> +buffers that don't exceed the size \field{data_max_size} (plus
>>>> header length).
>>>> +
>>>> +If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST NOT
>>>> transmit packets
>>>> +of size exceeding the value of \field{data_max_size} (plus header
>>>> length).
>>>> +
>>>> \subsection{Device Initialization}\label{sec:Device Types / Socket
>>>> Device / Device Initialization}
>>>>
>>>> \begin{enumerate}
>>>> -- 2.17.1
>>>
>>> I don't know if it is useful to define also a minimum size for the RX
>>> buffers.
>>
>> The reason for that would be to not work with a large number of
>> descriptor chains for a packet?
>
> Yes, exactly. I don't know if it makes sense to have it in the
> configuration, though.
>

Right now I don't see it there as well.


> Thanks,
> Stefano

I addressed your comments in v2:
https://lists.oasis-open.org/archives/virtio-comment/202204/msg00062.html.
Please have a look!


>
>
> 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/
>




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] virtio-vsock: add max payload size config field

[Stefan Hajnoczi]

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

On Mon, Apr 04, 2022 at 04:49:31PM +0300, Laura Loghin wrote:
> @@ -57,6 +62,35 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
>  \hline
>  \end{tabular}
>  
> +The following driver-read-only field, \field{data_max_size} only exists if
> +VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum packet payload
> +size for the driver to use.

This wording can be misinterpreted but can be kept for now. This wording
should be updated when the next field is added in the future, indicating
that the field is only *used* when VIRTIO_VSOCK_F_SIZE_MAX has been
negotiated. Further fields will be located after data_max_size so the
field certainly *exists* even when VIRTIO_VSOCK_F_SIZE_MAX is not set.

If someone misinterprets this they might think the config struct layout
depends on the combination of negotiated feature bits. That's not the
case, the layout is always the same. The size of the struct depends on
the features offered by the device.

Similar wording is used elsewhere in the spec, so we may want to clarify
it for the entire spec.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Laura Loghin]

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

On 6/15/22 17:11, Stefan Hajnoczi wrote:
> On Mon, Apr 04, 2022 at 04:49:31PM +0300, Laura Loghin wrote:
>> @@ -57,6 +62,35 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
>>   \hline
>>   \end{tabular}
>>   
>> +The following driver-read-only field, \field{data_max_size} only exists if
>> +VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum packet payload
>> +size for the driver to use.
> This wording can be misinterpreted but can be kept for now. This wording
> should be updated when the next field is added in the future, indicating
> that the field is only *used* when VIRTIO_VSOCK_F_SIZE_MAX has been
> negotiated. Further fields will be located after data_max_size so the
> field certainly *exists* even when VIRTIO_VSOCK_F_SIZE_MAX is not set.
>
> If someone misinterprets this they might think the config struct layout
> depends on the combination of negotiated feature bits. That's not the
> case, the layout is always the same. The size of the struct depends on
> the features offered by the device.
>
> Similar wording is used elsewhere in the spec, so we may want to clarify
> it for the entire spec.
>
> Stefan

Yes, I agree with you. In fact when I added this section I had a look at
the corresponding ones from other devices, and got confused by the wording.
My understanding actually was that the field won't exist, but indeed I
found it pretty weird, so it would be great if we can update all the spec
sections that are propagating this misinterpretation. For now, I will
update the section in this patch.

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: 2251 bytes --]

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

[Stefan Hajnoczi]

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

On Thu, Jun 23, 2022 at 05:27:28PM +0300, Laura Loghin wrote:
> On 6/15/22 17:11, Stefan Hajnoczi wrote:
> > On Mon, Apr 04, 2022 at 04:49:31PM +0300, Laura Loghin wrote:
> > > @@ -57,6 +62,35 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
> > >   \hline
> > >   \end{tabular}
> > > +The following driver-read-only field, \field{data_max_size} only exists if
> > > +VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum packet payload
> > > +size for the driver to use.
> > This wording can be misinterpreted but can be kept for now. This wording
> > should be updated when the next field is added in the future, indicating
> > that the field is only *used* when VIRTIO_VSOCK_F_SIZE_MAX has been
> > negotiated. Further fields will be located after data_max_size so the
> > field certainly *exists* even when VIRTIO_VSOCK_F_SIZE_MAX is not set.
> > 
> > If someone misinterprets this they might think the config struct layout
> > depends on the combination of negotiated feature bits. That's not the
> > case, the layout is always the same. The size of the struct depends on
> > the features offered by the device.
> > 
> > Similar wording is used elsewhere in the spec, so we may want to clarify
> > it for the entire spec.
> > 
> > Stefan
> 
> Yes, I agree with you. In fact when I added this section I had a look at
> the corresponding ones from other devices, and got confused by the wording.
> My understanding actually was that the field won't exist, but indeed I
> found it pretty weird, so it would be great if we can update all the spec
> sections that are propagating this misinterpretation. For now, I will
> update the section in this patch.

Thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]