[virtio-dev] Re: [PATCH v9 14/16] VIRTIO_F_NOTIFICATION_DATA: extra data to devices
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
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
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
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