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.