[PATCH] virtio-spec: 64 bit features, used/avail event

[PATCH] virtio-spec: 64 bit features, used/avail event

[Michael S. Tsirkin]

I'm working on a patchset that modified the notificatin
hand-off in virtio to be basically like Xen:
each side published an index, the other side only triggers
an event when it crosses that index value
(Xen event indexes start at 1, ours start at 0 for
backward-compatiblity, but that's minor).

Especially for testing, it is very convenient to have
separate feature bits for this change in used and available
ring; since we've run out of bits in the 32 bit field,
I added another 32 bit and bit 31 enables that.

I started with using both flags and indexes in parallel,
but switched to doing either-or: this means we do
not need to tweak memory access ordering as index access just
replaces flags access.

A note on naming: the index replacing avail->flags is named
used_event, the index replacing used->flags is named
avail_event to stress the fact that these actually
point into the other side of the ring:
event is triggered when avail->idx == used->avail_event + 1
and when used->idx == avail->used_event + 1, respectively.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 virtio-spec.lyx |  419 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 411 insertions(+), 8 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index f7c9c38..08275d0 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -1,4 +1,4 @@
-#LyX 1.6.7 created this file. For more info see http://www.lyx.org/
+#LyX 1.6.8 created this file. For more info see http://www.lyx.org/
 \lyxformat 345
 \begin_document
 \begin_header
@@ -36,7 +36,7 @@
 \paperpagestyle default
 \tracking_changes true
 \output_changes true
-\author "" 
+\author "Michael S. Tsirkin" 
 \author "" 
 \end_header
 
@@ -953,6 +953,10 @@ ISR
 
 \size footnotesize
 Features
+\change_inserted 0 1304329091
+ bits 0:31
+\change_unchanged
+
 \end_layout
 
 \end_inset
@@ -964,6 +968,10 @@ Features
 
 \size footnotesize
 Features
+\change_inserted 0 1304329086
+ bits 0:31
+\change_unchanged
+
 \end_layout
 
 \end_inset
@@ -1186,6 +1194,177 @@ Vector
 \end_layout
 
 \begin_layout Standard
+
+\change_inserted 0 1304328924
+Finally, if feature bits (VIRTIO_F_FEATURES_HI) this is immediately followed
+ by two additional fields:
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1304328925
+\begin_inset Tabular
+<lyxtabular version="3" rows="4" columns="3">
+<features>
+<column alignment="left" valignment="top" width="0">
+<column alignment="left" valignment="top" width="0">
+<column alignment="left" valignment="top" width="0">
+<row>
+<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+Bits
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+32
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+32
+\end_layout
+
+\end_inset
+</cell>
+</row>
+<row>
+<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+Read/Write
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+R
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+R+W
+\end_layout
+
+\end_inset
+</cell>
+</row>
+<row>
+<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+Purpose
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+
+\size footnotesize
+Device
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+
+\size footnotesize
+Guest
+\end_layout
+
+\end_inset
+</cell>
+</row>
+<row>
+<cell alignment="center" valignment="top" bottomline="true" leftline="true" rightline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304328925
+
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" bottomline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304329099
+
+\size footnotesize
+Features bits 32:63
+\end_layout
+
+\end_inset
+</cell>
+<cell alignment="center" valignment="top" bottomline="true" leftline="true" usebox="none">
+\begin_inset Text
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304329102
+
+\size footnotesize
+Features bits 32:63
+\end_layout
+
+\end_inset
+</cell>
+</row>
+</lyxtabular>
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
 Immediately following these general headers, there may be device-specific
  headers:
 \end_layout
@@ -1348,7 +1527,20 @@ Feature Bits
 The least significant 31 bits of the first configuration field indicates
  the features that the device supports (the high bit is reserved, and will
  be used to indicate the presence of future feature bits elsewhere).
- The bits are allocated as follows:
+ 
+\change_inserted 0 1304331636
+If more than 31 feature bits are supported, the device indicates so by setting
+ feature bit 31 (see 
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "cha:Reserved-Feature-Bits"
+
+\end_inset
+
+).
+ 
+\change_unchanged
+The bits are allocated as follows:
 \end_layout
 
 \begin_layout Description
@@ -1372,7 +1564,29 @@ to
 \begin_inset space ~
 \end_inset
 
-30 Feature bits reserved for extensions to the queue mechanism
+
+\change_inserted 0 1304329326
+4
+\change_deleted 0 1304329325
+3
+\change_unchanged
+0 Feature bits reserved for extensions to the queue mechanism
+\change_inserted 0 1304329351
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1304329398
+41
+\begin_inset space ~
+\end_inset
+
+to
+\begin_inset space ~
+\end_inset
+
+63 Feature bits reserved for future extensions
 \end_layout
 
 \begin_layout Standard
@@ -1407,6 +1621,19 @@ This allows for forwards and backwards compatibility: if the device is enhanced
  support, it will not see that feature bit in the Device Features field
  and can go into backwards compatibility mode (or, for poor implementations,
  set the FAILED Device Status bit).
+\change_inserted 0 1304329423
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1304331742
+Access to feature bits 32 to 63 is enabled by Guest by setting feature bit
+ 31.
+ If this bit is unset, Device must assume that all feature bits > 31 are
+ unset.
+\change_unchanged
+
 \end_layout
 
 \begin_layout Subsubsection
@@ -1891,7 +2118,38 @@ flags
 
  field is currently 0 or 1: 1 indicating that we do not need an interrupt
  when the device consumes a descriptor from the available ring.
- This interrupt suppression is merely an optimization; it may not suppress
+ 
+\change_inserted 0 1304331587
+Alternatively, we can ask the device to delay interrupts until an entry
+ with an index specified by the 
+\begin_inset Quotes eld
+\end_inset
+
+used_event
+\begin_inset Quotes erd
+\end_inset
+
+ field is written in the used ring (equivalently, until the 
+\emph on
+idx
+\emph default
+ field in the used ring will reach the value 
+\emph on
+used_event + 1
+\emph default
+).
+ The method employed by the device is controlled by the VIRTIO_RING_F_USED_EVENT
+_IDX feature bit (see 
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "cha:Reserved-Feature-Bits"
+
+\end_inset
+
+).
+ 
+\change_unchanged
+This interrupt suppression is merely an optimization; it may not suppress
  interrupts entirely.
 \end_layout
 
@@ -1940,6 +2198,17 @@ struct vring_avail {
 \begin_layout Plain Layout
 
    u16 ring[qsz]; /* qsz is the Queue Size field read from device */
+\change_inserted 0 1304329945
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304329957
+
+   u16 used_event;
+\change_unchanged
+
 \end_layout
 
 \begin_layout Plain Layout
@@ -1963,8 +2232,63 @@ The used ring is where the device returns buffers once it is done with them.
 \emph on
 available
 \emph default
- ring (the flag is kept here because this is the only part of the virtqueue
- written by the device).
+ ring
+\change_inserted 0 1304331253
+.
+ Alternatively, the device can hint that no notification is necessary until
+ an entry with an index specified by the 
+\begin_inset Quotes eld
+\end_inset
+
+avail_event
+\begin_inset Quotes erd
+\end_inset
+
+ is written in the available ring (equivalently, until the 
+\emph on
+idx
+\emph default
+ field in the available ring will reach the value 
+\emph on
+avail_event + 1
+\emph default
+).
+
+\change_unchanged
+ 
+\change_inserted 0 1304331599
+The method employed by the device is controlled by the VIRTIO_RING_F_AVAIL_EVENT
+_IDX feature bit (see 
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "cha:Reserved-Feature-Bits"
+
+\end_inset
+
+).
+ 
+\change_deleted 0 1304331235
+(the flag is kept here because this is the only part of the virtqueue written
+ by the device)
+\change_inserted 0 1304331598
+
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304331235
+These fields are kept here because this is the only part of the virtqueue
+ written by the device
+\change_unchanged
+
+\end_layout
+
+\end_inset
+
+
+\change_unchanged
+.
 \end_layout
 
 \begin_layout Standard
@@ -2046,6 +2370,17 @@ struct vring_used {
 \begin_layout Plain Layout
 
     struct vring_used_elem ring[qsz];
+\change_inserted 0 1304330369
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1304330380
+
+    u16 avail_event;
+\change_unchanged
+
 \end_layout
 
 \begin_layout Plain Layout
@@ -3355,7 +3690,13 @@ Appendix B: Reserved Feature Bits
 \end_layout
 
 \begin_layout Standard
-Currently there are three device-independent feature bits defined:
+Currently there are 
+\change_inserted 0 1304330655
+five
+\change_deleted 0 1304330657
+three
+\change_unchanged
+ device-independent feature bits defined:
 \end_layout
 
 \begin_layout Description
@@ -3390,6 +3731,31 @@ reference "sub:Indirect-Descriptors"
 \end_layout
 
 \begin_layout Description
+
+\change_inserted 0 1304331394
+VIRTIO_F_RING_USED_EVENT_IDX(29) This feature indicates that the device
+ should ignore the 
+\emph on
+flags
+\emph default
+ field in the available ring structure.
+ Instead, the
+\emph on
+ used_event
+\emph default
+ field in this structure is used by guest to suppress device interrupts.
+ If unset, the device should ignore the 
+\emph on
+used_event
+\emph default
+ field; the 
+\emph on
+flags
+\emph default
+ field is used
+\end_layout
+
+\begin_layout Description
 VIRTIO_F_BAD_FEATURE(30) This feature should never be negotiated by the
  guest; doing so is an indication that the guest is faulty
 \begin_inset Foot
@@ -3403,6 +3769,43 @@ An experimental virtio PCI driver contained in Linux version 2.6.25 had this
 \end_inset
 
 
+\change_inserted 0 1304330854
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1304330961
+VIRTIO_F_FEATURES_HIGH(31) This feature indicates that the device supports
+ feature bits 32:63.
+ If unset, feature bits 32:63 are unset.
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1304331390
+VIRTIO_F_RING_AVAIL_EVENT_IDX(32) This feature indicates that the driver
+ should ignore the 
+\emph on
+flags
+\emph default
+ field in the used ring structure.
+ Instead, the 
+\emph on
+avail_event
+\emph default
+ field in this structure is used by the device to suppress notifications.
+ If unset, the device should ignore the 
+\emph on
+avail_event
+\emph default
+ field; the 
+\emph on
+flags
+\emph default
+ field is used
+\change_unchanged
+
 \end_layout
 
 \begin_layout Chapter*
-- 
1.7.5.53.gc233e

Re: [PATCH] virtio-spec: 64 bit features, used/avail event

[Avi Kivity]

On 05/02/2011 04:17 PM, Michael S. Tsirkin wrote:
> I'm working on a patchset that modified the notificatin
> hand-off in virtio to be basically like Xen:
> each side published an index, the other side only triggers
> an event when it crosses that index value
> (Xen event indexes start at 1, ours start at 0 for
> backward-compatiblity, but that's minor).
>
> Especially for testing, it is very convenient to have
> separate feature bits for this change in used and available
> ring; since we've run out of bits in the 32 bit field,
> I added another 32 bit and bit 31 enables that.
>

> -30 Feature bits reserved for extensions to the queue mechanism
> +
> +\change_inserted 0 1304329326
> +4
> +\change_deleted 0 1304329325
> +3
> +\change_unchanged
> +0 Feature bits reserved for extensions to the queue mechanism

That would be 24 to 30, 32 to 40.

> +\change_inserted 0 1304329351
> +
> +\end_layout
> +
> +\begin_layout Description
> +
> +\change_inserted 0 1304329398
> +41
> +\begin_inset space ~
> +\end_inset
> +
> +to
> +\begin_inset space ~
> +\end_inset
> +
> +63 Feature bits reserved for future extensions
>   \end_layout
>
>
> @@ -1891,7 +2118,38 @@ flags
>
>    field is currently 0 or 1: 1 indicating that we do not need an interrupt
>    when the device consumes a descriptor from the available ring.
> - This interrupt suppression is merely an optimization; it may not suppress
> +
> +\change_inserted 0 1304331587
> +Alternatively, we can ask the device to delay interrupts until an entry

we can ask -> the guest may ask

> + with an index specified by the
> +\begin_inset Quotes eld
> +\end_inset
> +
> +used_event
> +\begin_inset Quotes erd
> +\end_inset
> +
> + field is written in the used ring (equivalently, until the
> +\emph on
> +idx
> +\emph default
> + field in the used ring will reach the value
> +\emph on
> +used_event + 1
> +\emph default
> +).
> + The method employed by the device is controlled by the VIRTIO_RING_F_USED_EVENT
> +_IDX feature bit (see
> +\begin_inset CommandInset ref
> +LatexCommand ref
> +reference "cha:Reserved-Feature-Bits"
> +
> +\end_inset
> +
> +).
> +
> +\change_unchanged
> +This interrupt suppression is merely an optimization; it may not suppress
>    interrupts entirely.
>   \end_layout

This section is strangely worded, from the guest point of view, which is 
strange for the device spec.  It's better to say things explicitly.  
However, this issue is not introduced by the patch.

>
> @@ -1940,6 +2198,17 @@ struct vring_avail {
>   \begin_layout Plain Layout
>
>      u16 ring[qsz]; /* qsz is the Queue Size field read from device */
> +\change_inserted 0 1304329945
> +
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 0 1304329957
> +
> +   u16 used_event;
> +\change_unchanged
> +
>   \end_layout
>
>   \begin_layout Plain Layout
> @@ -1963,8 +2232,63 @@ The used ring is where the device returns buffers once it is done with them.
>   \emph on
>   available
>   \emph default
> - ring (the flag is kept here because this is the only part of the virtqueue
> - written by the device).
> + ring
> +\change_inserted 0 1304331253
> +.
> + Alternatively, the device can hint that no notification is necessary until

the guest can request the device to hint


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] virtio-spec: 64 bit features, used/avail event

[Michael S. Tsirkin]

On Mon, May 02, 2011 at 05:44:36PM +0300, Avi Kivity wrote:
> On 05/02/2011 04:17 PM, Michael S. Tsirkin wrote:
> >I'm working on a patchset that modified the notificatin
> >hand-off in virtio to be basically like Xen:
> >each side published an index, the other side only triggers
> >an event when it crosses that index value
> >(Xen event indexes start at 1, ours start at 0 for
> >backward-compatiblity, but that's minor).
> >
> >Especially for testing, it is very convenient to have
> >separate feature bits for this change in used and available
> >ring; since we've run out of bits in the 32 bit field,
> >I added another 32 bit and bit 31 enables that.
> >
> 
> >-30 Feature bits reserved for extensions to the queue mechanism
> >+
> >+\change_inserted 0 1304329326
> >+4
> >+\change_deleted 0 1304329325
> >+3
> >+\change_unchanged
> >+0 Feature bits reserved for extensions to the queue mechanism
> 
> That would be 24 to 30, 32 to 40.

I guess my points 31 is for feature bit extension
so it's generic too. Will
'extensions to the queue and feature negotiation mechanism'
address this?

> >+\change_inserted 0 1304329351
> >+
> >+\end_layout
> >+
> >+\begin_layout Description
> >+
> >+\change_inserted 0 1304329398
> >+41
> >+\begin_inset space ~
> >+\end_inset
> >+
> >+to
> >+\begin_inset space ~
> >+\end_inset
> >+
> >+63 Feature bits reserved for future extensions
> >  \end_layout
> >
> >
> >@@ -1891,7 +2118,38 @@ flags
> >
> >   field is currently 0 or 1: 1 indicating that we do not need an interrupt
> >   when the device consumes a descriptor from the available ring.
> >- This interrupt suppression is merely an optimization; it may not suppress
> >+
> >+\change_inserted 0 1304331587
> >+Alternatively, we can ask the device to delay interrupts until an entry
> 
> we can ask -> the guest may ask

OK.

> >+ with an index specified by the
> >+\begin_inset Quotes eld
> >+\end_inset
> >+
> >+used_event
> >+\begin_inset Quotes erd
> >+\end_inset
> >+
> >+ field is written in the used ring (equivalently, until the
> >+\emph on
> >+idx
> >+\emph default
> >+ field in the used ring will reach the value
> >+\emph on
> >+used_event + 1
> >+\emph default
> >+).
> >+ The method employed by the device is controlled by the VIRTIO_RING_F_USED_EVENT
> >+_IDX feature bit (see
> >+\begin_inset CommandInset ref
> >+LatexCommand ref
> >+reference "cha:Reserved-Feature-Bits"
> >+
> >+\end_inset
> >+
> >+).
> >+
> >+\change_unchanged
> >+This interrupt suppression is merely an optimization; it may not suppress
> >   interrupts entirely.
> >  \end_layout
> 
> This section is strangely worded, from the guest point of view,
> which is strange for the device spec.  It's better to say things
> explicitly.  However, this issue is not introduced by the patch.
> >
> >@@ -1940,6 +2198,17 @@ struct vring_avail {
> >  \begin_layout Plain Layout
> >
> >     u16 ring[qsz]; /* qsz is the Queue Size field read from device */
> >+\change_inserted 0 1304329945
> >+
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 0 1304329957
> >+
> >+   u16 used_event;
> >+\change_unchanged
> >+
> >  \end_layout
> >
> >  \begin_layout Plain Layout
> >@@ -1963,8 +2232,63 @@ The used ring is where the device returns buffers once it is done with them.
> >  \emph on
> >  available
> >  \emph default
> >- ring (the flag is kept here because this is the only part of the virtqueue
> >- written by the device).
> >+ ring
> >+\change_inserted 0 1304331253
> >+.
> >+ Alternatively, the device can hint that no notification is necessary until
> 
> the guest can request the device to hint

You mean request by negotiating a feature? Since feature negotiation
has a section of its own, I think it would be confusing to
refer to it here.

> 
> -- 
> error compiling committee.c: too many arguments to function