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

[Paolo Bonzini <pbonzini@redhat.com>]

On 16/04/2015 14:57, 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>
> ---
> 
> Based on patch by Rusty Russell, added documentation
> for errors for virtio blk.
> 
> Paolo - could you please confirm that virtio scsi
> does not have a similar issue?

virtio-scsi has the same bug.

I would just treat len as rubbish for legacy implementations.

Paolo

>  conformance.tex |  3 +++
>  content.tex     | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/conformance.tex b/conformance.tex
> index 0f601d7..0945723 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -39,6 +39,7 @@ A driver MUST conform to the following normative statements:
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>  \item \ref{drivernormative:General Initialization And Device Operation / Device Initialization}
>  \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Updating idx}
>  \item \ref{drivernormative:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Notifying The Device}
> @@ -144,6 +145,7 @@ A device MUST conform to the following normative statements:
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Interrupt Suppression}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
>  \item \ref{devicenormative:Reserved Feature Bits}
>  \end{itemize}
> @@ -267,6 +269,7 @@ Feature Bits / Legacy Interface: A Note on Feature Bits}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
> +\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
>  \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
>  \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
>  \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
> diff --git a/content.tex b/content.tex
> index 344fd5f..054d332 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -636,6 +636,19 @@ that uninitialized memory has been overwritten when it has not.
>  The driver MUST NOT make assumptions about data in device-writable buffers
>  beyond the first \field{len} bytes, and SHOULD ignore this data.
>  
> +\drivernormative{\subsubsection}{Legacy Interface: The len Field}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring / Legacy Interface: The len Field}
> +
> +When using the legacy interface, a driver SHOULD treat a \field{len}
> +which exceeds the total length of device-writable buffers as if it
> +were equal to that length.
> +
> +\begin{note}
> +Some legacy device implementations incorrectly set \field{len} to the
> +total descriptor length, not the written length; in particular on
> +network device transmit packets (QEMU prior to version 1.3) and block
> +device writes (QEMU including version 2.2).
> +\end{note}
> +
>  \subsection{Virtqueue Notification Suppression}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Notification Suppression}
>  
>  The device can suppress notifications in a manner analogous to the way
> @@ -3936,6 +3949,12 @@ 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 \field{status} value differs from VIRTIO_BLK_S_OK,
> +the driver SHOULD treat a \field{len} which exceeds 1
> +as if it were 1. Historically, devices put the total descriptor length,
> +or the total length of device-writable buffers there,
> +even when only the status byte was actually written.
> +
>  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.
>