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

2018-03-07 Thread Cornelia Huck
On Wed, 7 Mar 2018 17:10:21 +0200
"Michael S. Tsirkin"  wrote:

> 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"  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"  wrote:  

> > > > > @@ -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.

ppc is not s390 :)

I highly doubt that there will be non-be s390 ever (too much legacy
software).

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2018-03-07 Thread Michael S. Tsirkin
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"  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"  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 000..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.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2018-03-07 Thread Cornelia Huck
On Wed, 7 Mar 2018 16:09:33 +0200
"Michael S. Tsirkin"  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"  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.

> > > diff --git a/notifications.c b/notifications.c
> > > new file mode 100644
> > > index 000..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.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



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

2018-03-07 Thread Michael S. Tsirkin
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"  wrote:
> 
> > Motivation for the new feature is included in the text.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> >  content.tex  | 141 
> > ---
> >  introduction.tex |   4 +-
> >  notifications.c  |   3 ++
> >  3 files changed, 140 insertions(+), 8 deletions(-)
> >  create mode 100644 notifications.c
> > 
> > diff --git a/content.tex b/content.tex
> > index c57a918..4261913 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -283,9 +283,77 @@ Packed Virtqueues}).
> >  Every driver and device supports either the Packed or the Split
> >  Virtqueue format, or both.
> >  
> > +\subsection{Driver notifications} \label{sec:Virtqueues / Driver 
> > notifications}
> > +Driver is sometimes required to notify the device after
> > +making changes to the virtqueue.
> > +
> > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > +this notification involves sending the
> > +virtqueue number to the device (depending on the transport).
> > +
> > +However, some devices benefit from ability to find out the number of
> > +available descriptors in the ring, and whether to send
> > +interrupts to drivers without accessing virtqueue in memory:
> > +for efficiency or as a debugging aid.
> > +
> > +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
> > +has been negotiated, driver notifications to the device include
> > +the following information:
> > +
> > +\begin{description}
> > +\item [VQ number]
> > +\item [Offset]
> > +  Within the ring where the next available ring entry
> > +  will be written.
> > +  Without VIRTIO_F_RING_PACKED this refers to the
> > +  15 least significant bits of the available index.
> > +  With VIRTIO_F_RING_PACKED this refers to the offset
> > +  (in units of descritor entries)
> > +  within the descriptor ring where the next available
> > +  descriptor will be written.
> > +\item [Wrap Counter]
> > +  With VIRTIO_F_RING_PACKED this is the wrap counter
> > +  referring to the next available descriptor.
> > +  Without VIRTIO_F_RING_PACKED this is the most significant bit
> > +  of the available index.
> > +\end{description}
> > +
> > +Note that driver can trigger multiple notifications even without
> > +making any more changes to the ring. When VIRTIO_F_NOTIFICATION_DATA
> > +has been negotiated, these notifications would then have
> > +identical \field{Offset} and \field{Wrap Counter} values.
> > +
> >  \input{split-ring.tex}
> >  
> >  \input{packed-ring.tex}
> > +
> > +\subsubsection{Driver notifications}
> > +
> > +\label{sec:Packed Virtqueues / Driver notifications}
> > +Whenever not suppressed by Device Event Suppression,
> > +driver is required to notify the device after
> > +making changes to the virtqueue.
> > +
> > +Some devices benefit from ability to find out the number of
> > +available descriptors in the ring, and whether to send
> > +interrupts to drivers without accessing virtqueue in memory:
> > +for efficiency or as a debugging aid.
> > +
> > +To help with these optimizations, driver notifications
> > +to the device include the following information:
> > +
> > +\begin{itemize}
> > +\item VQ number
> > +\item Offset (in units of descriptor size) within the ring
> > +  where the next available descriptor will be written
> > +\item Wrap Counter referring to the next available
> > +  descriptor
> > +\end{itemize}
> > +
> > +Note that driver can trigger multiple notifications even without
> > +making any more changes to the ring. These would then have
> > +identical \field{Offset} and \field{Wrap Counter} values.
> 
> Isn't that duplicating the information for the generic case?
> 
> (And if you wanted to specify something specific for the packed case,
> shouldn't it go into packed-ring.tex?)

Right. Cut-n-pasted twice. Will drop.


> > +
> >  \chapter{General Initialization And Device Operation}\label{sec:General 
> > Initialization And Device Operation}
> >  
> >  We start with an overview of device initialization, then expand on the
> 
> (...)
> 
> > +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.


> > +\end{lstlisting}
> > +
> > +See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / 
> > Driver notifications}
> > +for the definition of the components.
> > +
> > +See \ref{sec:Virtio Transport Options / Virtio Over