Re: [PATCH v3] used ring: specify legacy behaviour for len field

[Paolo Bonzini <pbonzini@redhat.com>]

On 20/04/2015 16:24, Michael S. Tsirkin wrote:
> many hypervisors implemented len field incorrectly.
> Document existing bugs in the legacy sections.
> 
> VIRTIO-141
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> As Paolo pointed out, most devices don't really need the used len
> field. Going over QEMU code, I found more devices that seem buggy.
> So let's just document that drivers should ignore len in
> legacy mode, for all devices where this makes sense (i.e.  except
> receive queue for serial and net devices, and the rng).

Looks good.  Indeed in this cases len is used.

Thanks,

Paolo

> changes from v2
>         dropped normative statements from generic section
>         added non-normative text suggesting len is unreliable
>         updated all devices
> 
> 
>  content.tex | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 748c78d..ac14490 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -614,6 +614,17 @@ the constant as VRING_USED_F_NO_NOTIFY, but the layout and value were
>  identical.
>  \end{note}
>  
> +\subsubsection{Legacy Interface: The Virtqueue Used
> +Ring}\label{sec:Basic Facilities of a Virtio Device / Virtqueues
> +/ The Virtqueue Used Ring/ Legacy Interface: The Virtqueue Used
> +Ring}
> +
> +Historically, many drivers ignored the \field{len} value, as a
> +result, many devices set \field{len} incorrectly.  Thus, when
> +using the legacy interface, it is generally a good idea to ignore
> +the \field{len} value in used ring entries if possible.  Specific
> +known issues are listed per device type.
> +
>  \devicenormative{\subsubsection}{The Virtqueue Used Ring}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>  
>  The device MUST set \field{len} prior to updating the used \field{idx}.
> @@ -3236,6 +3247,15 @@ The legacy driver only presented \field{num_buffers} in the struct virtio_net_hd
>  when VIRTIO_NET_F_MRG_RXBUF was not negotiated; without that feature the
>  structure was 2 bytes shorter.
>  
> +When using the legacy interface, the driver SHOULD ignore the
> +\field{len} value in used ring entries for the transmit queues
> +and the controlq queue.
> +\begin{note}
> +Historically, some devices put
> +the total descriptor length there, even though no data was
> +actually written.
> +\end{note}
> +
>  \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / Device Operation / Packet Transmission}
>  
>  Transmitting a single packet is simple, but varies depending on
> @@ -3936,6 +3956,14 @@ MUST format the fields in struct virtio_blk_req
>  according to the native endian of the guest rather than
>  (necessarily when not using the legacy interface) little-endian.
>  
> +When using the legacy interface, transitional drivers
> +SHOULD ignore the \field{len} value in used ring entries.
> +\begin{note}
> +Historically, some devices put the total descriptor length,
> +or the total length of device-writable buffers there,
> +even when only the status byte was actually written.
> +\end{note}
> +
>  The \field{reserved} field was previously called \field{ioprio}.  \field{ioprio}
>  is a hint about the relative priorities of requests to the device:
>  higher numbers indicate more important requests.
> @@ -4259,6 +4287,14 @@ MUST format the fields in struct virtio_console_control
>  according to the native endian of the guest rather than
>  (necessarily when not using the legacy interface) little-endian.
>  
> +When using the legacy interface, the driver SHOULD ignore the
> +\field{len} value in used ring entries for the transmit queues
> +and the control transmitq.
> +\begin{note}
> +Historically, some devices put the total descriptor length there,
> +even though no data was actually written.
> +\end{note}
> +
>  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  Types / Console Device / Legacy Interface: Framing Requirements}
>  
> @@ -4301,7 +4337,7 @@ by random data by the device.
>  
>  The driver MUST NOT place driver-readable buffers into the queue.
>  
> -The driver MUST examine the length written by the driver to determine
> +The driver MUST examine the length written by the device to determine
>  how many random bytes were received.
>  
>  \devicenormative{\subsubsection}{Device Operation}{Device Types / Entropy Device / Device Operation}
> @@ -4432,6 +4468,15 @@ before using the pages.
>  The driver MUST update \field{actual} after changing the number
>  of pages in the balloon.
>  
> +\paragraph{Legacy Interface: Device Operation}\label{sec:Device
> +Types / Memory Balloon Device / Device Operation / Legacy
> +Interface: Device Operation}
> +When using the legacy interface, the driver SHOULD ignore the \field{len} value in used ring entries.
> +\begin{note}
> +Historically, some devices put the total descriptor length there,
> +even though no data was actually written.
> +\end{note}
> +
>  \subsubsection{Memory Statistics}\label{sec:Device Types / Memory Balloon Device / Device Operation / Memory Statistics}
>  
>  The stats virtqueue is atypical because communication is driven
> @@ -4649,6 +4694,16 @@ RESET.}.
>  Device operation consists of operating request queues, the control
>  queue and the event queue.
>  
> +\paragraph{Legacy Interface: Device Operation}\label{sec:Device
> +Types / SCSI Host Device / Device Operation / Legacy
> +Interface: Device Operation}
> +When using the legacy interface, the driver SHOULD ignore the \field{len} value in used ring entries.
> +\begin{note}
> +Historically, devices put the total descriptor length,
> +or the total length of device-writable buffers there,
> +even when only part of the buffers were actually written.
> +\end{note}
> +
>  \subsubsection{Device Operation: Request Queues}\label{sec:Device Types / SCSI Host Device / Device Operation / Device Operation: Request Queues}
>  
>  The driver queues requests to an arbitrary request queue, and
>