Re: [virtio-dev] [PATCH v9] virtio-net: add Max MTU configuration field

[Aaron Conole <aconole@redhat.com>]

Cornelia Huck <cornelia.huck@de.ibm.com> writes:

> On Tue,  6 Sep 2016 10:31:01 -0400
> Aaron Conole <aconole@redhat.com> wrote:
>
> <sorry for being late to the party, but I just got around to looking at
> this as this is up for vote>
>
>> diff --git a/content.tex b/content.tex
>> index 4b45678..b90cbad 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -3049,6 +3049,14 @@ features.
>>  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
>>          reconfiguration support.
>> 
>> +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
>> +    offered by the device, device advises driver about the value of
>> +    MTU to be used. If negotiated, the driver uses \field{mtu} as
>> +    the maximum MTU value supplied to the operating system.
>> +
>> +    Note: many operating systems override the MTU value provided by the
>> +    driver.
>
> I'm wondering: Do we need to distinguish between what the _driver_ does
> and what the _guest_ does, generally speaking?
>
>> +
>>  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
>> 
>>  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
>> @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
>>  and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
>>  is negotiated.
>> 
>> +The following driver-read-only field, \field{mtu} only exists if
>> +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
>> +use.
>> +
>>  \begin{lstlisting}
>>  struct virtio_net_config {
>>          u8 mac[6];
>>          le16 status;
>>          le16 max_virtqueue_pairs;
>> +        le16 mtu;
>>  };
>>  \end{lstlisting}
>> 
>> @@ -3153,6 +3166,18 @@ struct virtio_net_config {
>>  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
>>  if it offers VIRTIO_NET_F_MQ.
>> 
>> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
>> +if it offers VIRTIO_NET_F_MTU.
>> +
>> +The device MUST NOT modify \field{mtu} once it has been set.
>
> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
> successfully negotiated"? I don't think the driver should assume
> anything until after feature negotiation is complete.

I agree; that was always the intent, but the wording does not convey
that.  Apologies.

>> +
>> +The device MUST NOT pass received packets that exceed \field{mtu} size
>> +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
>
> I don't think it should be the offer that is the deciding factor, but
> rather the final negotiation. But maybe we can make this "SHOULD NOT"?

It really needs to be MUST NOT, but as you note above this needs to be
based on the successful negotiation of the VIRTIO_NET_F_MTU feature,
rather than just the offer.

>> +
>> +The device MUST forward transmitted packets of up to MTU size with
>> +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
>> +offers VIRTIO_NET_F_MTU.
>> +
>>  \drivernormative{\subsubsection}{Device configuration
>> layout}{Device Types / Network Device / Device configuration layout}
>> 
>>  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
>> @@ -3165,6 +3190,15 @@ If the driver does not negotiate the
>> VIRTIO_NET_F_STATUS feature, it SHOULD
>>  assume the link is active, otherwise it SHOULD read the link status from
>>  the bottom bit of \field{status}.
>> 
>> +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
>> +buffers of size \field{mtu} to be able to receive at least one receive
>> +packet with \field{gso_type} NONE or ECN.
>
> Again, this should be the final negotiation instead of the offer,
> especially as...
>
>> +
>> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
>
> ...this is SHOULD and not MUST. The driver should be able to fail
> negotiating the feature if it cannot fulfill the condition above.

Agreed with both points.

>> +
>> +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets of
>> +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
>> +
>>  \subsubsection{Legacy Interface: Device configuration
>> layout}\label{sec:Device Types / Network Device / Device
>> configuration layout / Legacy Interface: Device configuration
>> layout}
>>  \label{sec:Device Types / Block Device / Feature bits / Device
>> configuration layout / Legacy Interface: Device configuration
>> layout}
>>  When using the legacy interface, transitional devices and drivers
>
> Again, sorry for being late with comments, but I'll vote with no for
> now. I can still change the vote if you can convince me, though.

No problem, thanks so much for the review.  I'm hoping you would be
convinced to change your vote to yes with the adjusted wording - I'll
publish a v10 next week (to give folks enough time to respond).

-Aaron