Re: [virtio] [PATCH] used ring: define the meaning and requirements of the len field.

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 14, 2015 at 10:59:06AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Fri, Apr 10, 2015 at 12:49:56PM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > On Tue, Apr 07, 2015 at 10:54:49AM +0930, Rusty Russell wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >> > On Fri, Mar 20, 2015 at 11:48:55AM +1030, Rusty Russell wrote:
> >> >> >> +\devicenormative{\subsubsection}{Virtqueue Notification Suppression}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> >> >> >> +
> >> >> >> +The device MUST set \field{len} prior to updating the used \field{idx}.
> >> >> >> +
> >> >> >> +The device MUST write at least \field{len} bytes to descriptor,
> >> >> >> +beginning at the first device-writable buffer,
> >> >> >> +prior to updating the used \field{idx}.
> >> >> >> +
> >> >> >> +The device MAY write more than \field{len} bytes to descriptor.
> >> >> >> +
> >> >> >> +\begin{note}
> >> >> >> +There are potential error cases where a device might not know what
> >> >> >> +parts of the buffers have been written.  This is why \field{len} is
> >> >> >> +permitted to be an underestimate: that's preferable to the driver believing
> >> >> >> +that uninitialized memory has been overwritten when it has not.
> >> >> >> +\end{note}
> >> >> >> +
> >> >> >> +\drivernormative{\subsubsection}{Virtqueue Notification Suppression}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
> >> >> >> +
> >> >> >> +The driver MUST NOT make assumptions about data in device-writable buffers
> >> >> >> +beyond the first \field{len} bytes, and SHOULD ignore this data.
> >> >> >> +
> >> >> >>  \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
> >> >> >
> >> >> > We know legacy devices don't follow this, so we also need some text in
> >> >> > the legacy sections to document the differences.
> >> >> 
> >> >> Indeed, but it was specific to block devices.
> >> >> 
> >> >> It may be worth noting that older block devices would set erroneously
> >> >> set a length corresponding to the entire buffer length, and thus legacy
> >> >> drivers SHOULD rely on the status byte and ignore used.len?
> >> >> 
> >> >> Cheers,
> >> >> Rusty.
> >> >
> >> > QEMU before 1.3 also set used len in tx descriptor to packet length.
> >> 
> >> Yech.  How's this on top then?  (Untested, uprade seems to have broken
> >> tex here):
> >> 
> >> diff --git a/conformance.tex b/conformance.tex
> >> index 619859b..fae899a 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}
> >> @@ -143,6 +144,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}
> >> @@ -264,6 +266,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 8e8765d..a23148d 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -643,6 +643,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
> >
> > Didn't you want to mention read errors too?
> 
> Already mentioned in previous patch.

In the non-legacy case, yes.
But I mean mention that legacy block (and scsi? Paolo do you remember)
devices would overestimate len in case of errors.

-- 
MST