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

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

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