Re: [virtio-dev][RFC 2/2] virtio-sdm: new device specification

[Stefan Hajnoczi <stefanha@redhat.com>]

On Fri, Jun 17, 2016 at 06:03:14PM +0200, Christian Pinto wrote:
> This patch adds the specification of the Signal Dristribution Module virtio
> device to the current virtio specification document.
> 
> Signed-off-by: Christian Pinto <c.pinto@virtualopensystems.com>
> ---
>  virtio-sdm.tex | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 virtio-sdm.tex
> 
> diff --git a/virtio-sdm.tex b/virtio-sdm.tex
> new file mode 100644
> index 0000000..abbdb80
> --- /dev/null
> +++ b/virtio-sdm.tex
> @@ -0,0 +1,126 @@
> +\section{Signal Distribution Module}\label{sec:Device Types / SDM Device}
> +
> +The virtio Signal Distribution Module is meant to enable Inter Processor
> +interrupts in QEMU/KVM environment. The SDM enables different processors in the
> +same or different QEMU instances to send mutual signals. An example are Inter
> +Processor Interrupts used in AMP systems, for the processors involved to notify
> +the presence of new data in the communication queues.

Please rephrase without mentioning QEMU/KVM.  Focus on the device
functionality and purpose.  VIRTIO's scope is more general than
QEMU/KVM.

I'm not familiar with AMP systems.  Can you explain how SDM relates to
remoteproc and rpmsg?  There seems to be overlap because remoteproc can
also boot a remote processor.

> +
> +\subsection{Device ID}\label{sec:Device Types / SDM Device / Device ID}
> +
> +19
> +
> +\subsection{Virtqueues}\label{sec:Device Types / SDM Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] hg_vq
> +\item[1] gh_vq
> +\end{description}
> +
> +Queue 0 is used for host-guest communication (i.e., notification of a signal),
> +while queue 1 for guest-host communication.

I think the VIRTIO terminology is "device" (host) and "driver" (guest).
So it would be:

Queue 0 is used for device-to-driver communication (i.e. notification of
a signal), while queue 1 is for driver-to-device communication.

> +
> +\subsection{Feature bits}\label{sec:Device Types / SDM Device / Feature bits}
> +
> +None.
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / SDM Device / 
> +Device configuration layout}
> +
> +The device configuration is composed by three fields: \texttt{master},
> +\texttt{max_slaves} and \texttt{current_slaves}.
> +
> +\begin{lstlisting}
> +struct virtio_sdm_config {
> +	u8    master;
> +	u16   max_slaves;
> +	u16   current_slaves;
> +};
> +\end{lstlisting}
> +
> +The SDM device can be instantiated either as a master or as a slave. The master
> +slave behavior is meant on purpose to reflect the AMP like type of communication
> +where a master processor controls one or more slave processors.
> +A master SDM instance can send a signal to any of the slaves instances,
> +while slaves can only signal the master. 
> +
> +The \texttt{master} field of the configuration space is meant to be read by the
> +Linux driver to figure out whether a specific instance is a master or a slave.

The scope of VIRTIO is wider than Linux, just saying "the driver" is
preferred.

> +The \texttt{max_slaves} field contains the maximum number of slaves supported by
> +the SDM device. 
> +The value of \texttt{max_slaves} is initially set from the command line of QEMU,

The QEMU command-line is out of scope for the VIRTIO specification.
Please drop this part of the sentence.  I suggest saying that a
configuration change notification is sent when the value of max_slaves
changes.

> +but can be changed during operations through the configuration space.
> +Finally the \texttt{current_slaves} field contains the actual number of slaves
> +registered to the master SDM. This field is a read only field. 
> +
> +\subsection{Device Initialization}\label{sec:Device Types / SDM Device /
> +evice Initialization}
> +
> +During initialization the \texttt{hg_vq} and \texttt{gh_vq} are identified and
> +the device is immediately operational. A master instance can access the number
> +of slaves registered at any time by reading the configuration space of the
> +device.
> +
> +During the initialization phase the device connects also to the communication
> +channel that is specificied as a parameter when configuring the device. At the
> +current state there are two types of communication channels: local and
> +socket(TCP or UNIX).
> +The local channel is meant for SDM devices attached to the same QEMU instance,
> +while the network channel makes it possible to exchange signals between
> +SDMs in different instances of QEMU. 
> +The type of communication channel is chosen at QEMU boot time and cannot be
> +changed during operations. It has to be noted that the behavior of the device is
> +independent from the communication channel used.

This paragraph is specific to QEMU and not directly relevant to the
device specification.  It can be dropped (feel free to extract any
pieces of information in there that you consider relevant to the device
spec).

> +
> +\subsection{Device Operation}\label{sec:Device Types / SDM Device / Device
> +peration}
> +
> +The SDM device  handles signal coming from the two following sources:

s/  / /
s/signal/signals/

> +
> +\begin{enumerate}
> +\item The local processor to which the devics is attached to.

s/devics is attached to/device is attached/

> +\item The communication channel connecting to other slaves/master. 
> +\end{enumerate}
> +
> +The first case is verified when the processor attached to the SDM device wants
> +to send a signal to SDM device instance (being it the same QEMU instance or a
> +separate one).
> +The second case is instead when an SDM device instance receives a signal from
> +another SDM device, to be forwarded to the local processor.
> +It is important to note that due to the master/slave behavior, slaves cannot
> +signal among themsleves but only with the master SDM instance.
> +
> +In both cases, before sending over the communication channel, the signal is
> +packed in the \texttt{SDMSignalData} data structure.
> +
> +\begin{lstlisting}
> +enum sdm_signal_type {
> +    SDM_IRQ,
> +    SDM_BOOT,
> +};
> +
> +struct SDMSignalData {
> +    uint32_t type;
> +    uint32_t slave;
> +    uint32_t payload[2];
> +};
> +\end{lstlisting}
> +
> +The \texttt{type} field indicates the type of signal to be sent to the
> +destination SDM. The current implementation supports two signal types.
> +The \texttt{SDM_IRQ} signal is used to send an inter processor interrupt, while
> +the \texttt{SDM_BOOT} signal is sent to trigger the boot of the destination
> +processor. The boot signal is meant to be used in an AMP like scenario where a
> +master processor boots one or more slave processors (e.g., via remoteproc).
> +The \texttt{slave} field contains the id of the SDM instance destination of the
> +signal. Id 0 is reserved for the master, from 1 onwards for the slaves.
> +This means that the \texttt{slave} field will always contain 0 when the source
> +of the signal is a slave SDM instance, while the actual id of the slave in case
> +of a master.
> +The \texttt{payload} is used to pass extra accompanying information with the
> +signal. The use case for the payload field is a hardware mailbox, where data is
> +pushed into the mailbox and an interrupt is triggered towards all the devices
> +registered with the mailbox.

This spec cannot be implemented because it does not explain the
semantics of the payload.  Please provide those details.  Even if it
doesn't make it into the final spec it will help reviewers understand
how SDM works.

Attachment: signature.asc
Description: PGP signature