Re: [PATCH v9 14/16] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

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

On Wed, Mar 07, 2018 at 03:49:41PM +0100, Cornelia Huck wrote:
> On Wed, 7 Mar 2018 16:09:33 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Mar 07, 2018 at 12:11:58PM +0100, Cornelia Huck wrote:
> > > On Thu, 1 Mar 2018 01:31:37 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > > +the driver notifies the device by writing the following
> > > > +32-bit value to the Queue Notify address:
> > > > +\begin{lstlisting}
> > > > +le32 vqn : 16,
> > > > +     next_off : 15,
> > > > +     next_wrap : 1;  
> > > 
> > > Don't we want to write this as
> > > 
> > > le32 vqn : 16;
> > > le32 next_off :15;
> > > le32 next_wrap : 1;
> > > 
> > > ?  
> > 
> > Same thing in C, but would be more confusing IMHO since it will be up to
> > the reader to figure out which fields comprise the 32 bit integer.
> 
> It looked weird to me. Other opinions?
> 
> > > > @@ -2340,12 +2455,22 @@ GPR  &   Input Value     & Output Value \\
> > > >  \hline
> > > >    2   &  Subchannel ID    & Host Cookie  \\
> > > >  \hline
> > > > -  3   & Virtqueue number  &              \\
> > > > +  3   & Notification data &              \\
> > > >  \hline
> > > >    4   &   Host Cookie     &              \\
> > > >  \hline
> > > >  \end{tabular}
> > > >  
> > > > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > > +the \field{Notification data} includes the Virtqueue number.
> > > > +
> > > > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > > +the value has the following format:
> > > > +\lstinputlisting{notifications.c}  
> > > 
> > > And we probably want to make this be explicitly.  
> > 
> > Are you sure?
> > I looked at s390 code and it just uses VQ index in native
> > endian-ness, so I kept this consistent.
> 
> Native endian is always be on s390. The registers for the hypercall are
> therefore always be.

Oh I thought there's ppc LE now.

> > > > diff --git a/notifications.c b/notifications.c
> > > > new file mode 100644
> > > > index 0000000..2ae96d4
> > > > --- /dev/null
> > > > +++ b/notifications.c
> > > > @@ -0,0 +1,3 @@
> > > > +u32 vqn : 16,
> > > > +    next_off : 15,
> > > > +    next_wrap : 1;  
> > > 
> > > I'm wondering how useful the u32 notation is here.  
> > 
> > It says vqn in low 16 bits of a 32 bit counter, so e.g.
> > on LE system byte 0 and on BE system byte 3.
> > Compare to 
> > 
> > be32 vqn : 16,
> >      next_off : 15,
> >      next_wrap : 1;
> > 
> > where we say it's in low 16 bits of a 32 bit BE integer,
> > so byte 3.
> 
> I was referring to the endianness: AFAICS, pci and mmio want le, ccw
> wants be. If we want to be explicit, notifications.c does not make much
> sense IMO.

Will split up.