Re: [virtio-comment] [PATCH] Update VIRTIO_F_EVENT_IDX throughout

[Joel Nider <joel@nider.org>]

On 2021-04-30 4:27 a.m., Cornelia Huck wrote:
> On Thu, 29 Apr 2021 16:10:16 -0700
> Joel Nider <joel@nider.org> wrote:
>
> > This flag has been called by many names in the past
> > (VIRTIO_RING_F_EVENT_IDX, VIRTIO_F_RING_EVENT_IDX) and there are
> > just a few stragglers left to clean up.
> >
> > Signed-off-by: Joel Nider <joel@nider.org>
> > ---
> >   content.tex     | 2 +-
> >   packed-ring.tex | 4 ++--
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 9232d5c..5a803dc 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -6581,7 +6581,7 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >   Descriptors}~\nameref{sec:Basic Facilities of a Virtio Device /
> >   Virtqueues / The Virtqueue Descriptor Table / Indirect
> >   Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}~\nameref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Support}.
> > -  \item[VIRTIO_F_RING_EVENT_IDX(29)] This feature enables the \field{used_event}
> > +  \item[VIRTIO_F_EVENT_IDX(29)] This feature enables the \field{used_event}
> Hm. The VIRTIO_F_RING_ version actually matches well with
> VIRTIO_F_RING_INDIRECT_DESC and VIRTIO_F_RING_PACKED. While the latter
> is used consistently throughout the spec, we seem to use
> VIRTIO_F_INDIRECT_DESC in other places. For added variety, Linux and
> QEMU use VIRTIO_RING_F_INDIRECT_DESC and VIRTIO_RING_F_EVENT_IDX (but
> VIRTIO_F_RING_PACKED.)
Clearly all of these flags apply to the v-rings - I don't think 
including 'RING' to makes it any clearer. So in the interest of 
readability, I would opt for the shorter flag names (more likely to fit 
in a 80-char line). Linux (and the other OSes and hypervisors) will 
eventually fall in line, once the spec is consistent.
> We should be consistent within the spec, so I'd vote for using the
> VIRTIO_F_RING_ versions everywhere here. (The Linux/QEMU variants are
> still easily recognizable, so there's actually not much potential for
> confusion.) The only downside is that this would mean more changes.
I fully support being consistent everywhere in the spec - that is my 
primary concern. I noticed that over time, the flag name seems to be 
gravitating towards the VIRTIO_F_EVENT_IDX (all places in section 2 
already use this name), so that's why I just changed the last two places 
that are not consistent. Truly, I will be happy once they are all the 
same (one way or the other).
> Opinions?
>
> >     and the \field{avail_event} fields as described in
> >   \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}, \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring} and \ref{sec:Packed Virtqueues / Driver and Device Event Suppression}.
> >   
> > diff --git a/packed-ring.tex b/packed-ring.tex
> > index ea92543..a9e6c16 100644
> > --- a/packed-ring.tex
> > +++ b/packed-ring.tex
> > @@ -336,7 +336,7 @@ \subsection{Driver and Device Event Suppression}
> >   /*
> >    * Enable events for a specific descriptor
> >    * (as specified by Descriptor Ring Change Event Offset/Wrap Counter).
> > - * Only valid if VIRTIO_F_RING_EVENT_IDX has been negotiated.
> > + * Only valid if VIRTIO_F_EVENT_IDX has been negotiated.
> >    */
> >   #define RING_EVENT_FLAGS_DESC 0x2
> >   /* The value 0x3 is reserved */
> > @@ -597,7 +597,7 @@ \subsubsection{Implementation Example}\label{sec:Basic Facilities of a Virtio De
> >   
> >   Below is a driver code example. It does not attempt to reduce
> >   the number of available buffer notifications, neither does it support
> > -the VIRTIO_F_RING_EVENT_IDX feature.
> > +the VIRTIO_F_EVENT_IDX feature.
> >   
> >   \begin{lstlisting}
> >   /* Note: vq->avail_wrap_count is initialized to 1 */