Re: [virtio-dev] Re: [PATCH v3] virtio-net: add mtu configuration field

[Aaron Conole <aconole@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Fri, Jan 29, 2016 at 01:48:55PM -0500, Aaron Conole wrote:
>> Sometimes it is essential for libvirt to be able to configure MTU
>> on guest's NICs to a value different from 1500.
>> 
>> The change adds a new field to configuration area of network
>> devices. It will be used to pass initial MTU from the device to
>> the driver.
>> 
>> In addition, in order to support backward and forward
>> compatibility, we introduce a new feature bit called
>> VIRTIO_NET_F_DEFAULT_MTU.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> Cc: Victor Kaplansky <victork@redhat.com>
>> ---
>> v1:
>> This is an attempt at continuing the work done by Victor Kaplansky on
>> mtu negiotiation for virtio-net devices. It attempts to pick up from
>> https://lists.oasis-open.org/archives/virtio-dev/201508/msg00007.html
>> and is just a minor blurb from the first patch along with the 2nd patch
>> from the series, and some of the feedback integrated.
>> 
>> v2:
>> Rephrase and provide a mechanism for guest->host and host->guest
>> communication through a driver read-only and driver write-only field.
>> 
>> v3:
>> Converted to just support initial MTU. Guest->host and Host->guest MTU
>> changes are outside the scope of this change.
>> 
>>  content.tex | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>> 
>> diff --git a/content.tex b/content.tex
>> index d989d98..0115982 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -3078,6 +3078,11 @@ features.
>>  
>>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>      channel.
>> +
>> +\item[VIRTIO_NET_F_MTU(24)] Negotiated MTU is supported. If
>> +    offered by the device, device advises driver about initial
>> +    MTU to be used. If negotiated, the driver uses \field{mtu} as
>> +    an initial value.
>>  \end{description}
>>  
>>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
>> @@ -3132,11 +3137,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 initial 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}
>>  
>> @@ -3145,6 +3155,11 @@ 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.
>>  
>> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>> +
>> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
>> +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.
>> @@ -3157,6 +3172,10 @@ 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}.
>>  
>> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.  If the
>> +driver negotiates the VIRTIO_NET_F_MTU feature, the driver MUST use \field{mtu}
>> +as the initial value for MTU.
>> +
>
> What is this last sentence in aid of?  Are we just trying to figure out
> whether the guest is new and supports getting MTU from host?
> Because using as initial value is something we can not check,
> we can check driver reads a field.
>
> So it looks like we are saying that drivers must not acknowledge feature
> if they do not read MTU, then let's write it that way.
> Same thing but I think it's clearer.

Okay, but I'm wondering how to write that up. 'MUST NOT' is difficult to
design a test around, usually. Would it be better to take out the
'initial' part? I realize we cannot enforce anything about where in the
lifecycle of guest we are, so is the objection just to the word
'initial'? Would a second sentence saying something like:

Drivers MUST ignore the VIRTIO_NET_F_MTU feature if they do not use
\field{mtu} as the value for MTU

be acceptable, along with remove the word 'initial'?

>>  \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
>> -- 
>> 2.5.0
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org