Re: [Qemu-devel] [virtio-dev][RFC v3] virtio-sdm: new device specification

[Edgar E. Iglesias]

On Wed, Sep 07, 2016 at 09:24:39AM +0200, Christian Pinto wrote:
> Hello Edgar,
> 
> thanks for your comments.
>

Thanks for the clarification, I have a few follow-up questions/comments.
 
> 
> On 06/09/2016 23:43, Edgar E. Iglesias wrote:
> >Hi,
> >
> >Sorry for the delay. I have a few questions.
> >
> >I don't fully understand the purpose of this. Could you elaborate a little 
> >on that?
> >You need a real IPI signal to drive this I guess, so it's a little bit of a
> >chicken and egg problem.
> 
> Usually in AMP-like systems CPUs signal each other with IPIs by exploiting
> a dedicated hardware peripheral, like a hardware mailbox. So I see the
> concept of the SDM (master, slave and communication channels) as the
> hardware device to actually implement and deliver the IPIs.
> In this case the SDM is designed in a way to be able to deliver multiple
> signals (e.g., BOOT, RESET) and not just a single interrupt.
> 
> >A useful feature I can see is multiplexing a single underlying IPI (driving
> >an sdm channel) into multiple "virtual" IPI signals. Is that the main
> >use-case?
> 
> In the SDM multiple slaves can connect to the same SDM channel (e.g.,
> socket)
> and the master uses the channel to deliver the signal (e.g., IRQ) to the
> destination slave device. The other way around is also possible, slaves
> signaling
> the master.
> I guess this is the multiplexing features you were talking about.

Right, but for this to be more useful, I think you should pass on some
payload with the IRQ signal. That will allow this channel to be used
to enable notification mechanisms for many other channels over a single
HW irq (e.g the payload would be like the irq vector or irq line nr).

Another question:
For the reverese path, I don't see how it will be reasonably implemented
in the remote cpus various sw stacks sharing a single virtio queue.
It would seem like you would need a reverese channel per remoteproc
to avoid locking (very messy in AMP systems) to safely update
the reverese gh_vq queue?

Or how do you see multiple AMP systems safely updating a single virtio
queue in parallel?

Best regards,
Edgar


> 
> However it is worth to notice that one SDM channel is not dedicated to one
> specific signal, but rather it is used to deliver all the signals
> implemented
> by the device.
> 
> >
> >Regarding the boot and reset. Typically there's a reset signal you release 
> >and
> >the remote CPU starts running. You can ofcourse reset the CPU and restart
> >at anytime. Not sure if you need an additional BOOT virtual signal.
> >
> >There may also be additional mechanisms to control the startup address of 
> >the remote CPU.
> >Any thoughts on that?
> 
> The purpose of the BOOT signal is also to pass the remote CPU its startup
> address. So the idea is to use the payload of the signal to send the slave
> processor
> its startup address over the SDM channel and SDM slave device. Our
> implementation
> for QEMU uses the BOOT signal for this purpose.
> 
> This part of the semantic of the BOOT signal is not described in the
> documentation
> since I believe it is a choice of the actual implementation of the SDM
> whether to
> pass the startup address or not (i.e. there might be a platform where the
> boot
> address of slave processors is somehow hard-coded).
> I can add the explanation to the device specification to make it more clear.
> 
> 
> Thanks,
> 
> Christian
> 
> >
> >Best regards,
> >Edgar
> >
> >
> >On Tue, Aug 30, 2016 at 01:22:26PM +0200, Christian Pinto wrote:
> >>Hello,
> >>
> >>are there any comments?
> >>
> >>
> >>Christian
> >>
> >>
> >>On 09/08/2016 09:37, 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 <address@hidden>
> >>>Signed-off-by: Baptiste Reynal <address@hidden>
> >>>
> >>>---
> >>>v2 -> v3:
> >>>- Removed master field from configuration and replaced with device_id
> >>>- Added new RESET signal
> >>>- Added signals definition into device specs
> >>>- Added feature bits associated to signals
> >>>
> >>>v1 -> v2:
> >>>- Fixed some typos
> >>>- Removed dependencies from QEMU
> >>>- Added explanation on how SDM can be used in AMP systems
> >>>- Explained semantics of payload field in SDMSignalData struct
> >>>---
> >>>  content.tex    |   2 +
> >>>  virtio-sdm.tex | 166 
> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 168 insertions(+)
> >>>  create mode 100644 virtio-sdm.tex
> >>>
> >>>diff --git a/content.tex b/content.tex
> >>>index 3317916..7fcf779 100644
> >>>--- a/content.tex
> >>>+++ b/content.tex
> >>>@@ -5643,6 +5643,8 @@ descriptor for the \field{sense_len}, 
> >>>\field{residual},
> >>>  \field{status_qualifier}, \field{status}, \field{response} and
> >>>  \field{sense} fields.
> >>>+\input{virtio-sdm.tex}
> >>>+
> >>>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>>  Currently there are three device-independent feature bits defined:
> >>>diff --git a/virtio-sdm.tex b/virtio-sdm.tex
> >>>new file mode 100644
> >>>index 0000000..ee43e23
> >>>--- /dev/null
> >>>+++ b/virtio-sdm.tex
> >>>@@ -0,0 +1,166 @@
> >>>+\section{Signal Distribution Module}\label{sec:Device Types / SDM Device}
> >>>+
> >>>+The virtio Signal Distribution Module is meant to enable Inter Processor 
> >>>signal
> >>>+exchange.
> >>>+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.
> >>>+In AMP systems signals are used for various purposes, an example are 
> >>>remoteproc
> >>>+or RPMSG. In the former signals are used by a master processor to trigger
> >>>+the boot of a slave processor. While the latter, RPMSG, uses virtio 
> >>>queues as a
> >>>+message exchange medium between processors. In this case the SDM can be 
> >>>used to
> >>>+generate the interrupt associated to the kick of a virtio queue.
> >>>+
> >>>+There are three signal types supported by the device, namely the
> >>>+\textit{IRQ} signal, \textit{BOOT} signal and \textit{RESET} signal. Such 
> >>>set of
> >>>+signals covers most of the needs of an AMP system, where a master 
> >>>processor can
> >>>+trigger the boot or reset of slave processors, and processors send IRQs 
> >>>between
> >>>+each other.
> >>>+
> >>>+\subsection{Device ID}\label{sec:Device Types / SDM Device / Device ID}
> >>>+
> >>>+21
> >>>+
> >>>+\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 device-to-driver communication (i.e., notification of 
> >>>a
> >>>+signal), while queue 1 for driver-to-device communication.
> >>>+
> >>>+\subsection{Feature bits}\label{sec:Device Types / SDM Device / Feature 
> >>>bits}
> >>>+
> >>>+\begin{description}
> >>>+\item[VIRTIO_SDM_F_IRQ_SIG (0)] Device supports the IRQ signal.
> >>>+
> >>>+\item[VIRTIO_SDM_F_BOOT_SIG (1)] Device supports the BOOT signal.
> >>>+
> >>>+\item[VIRTIO_SDM_F_RESET_SIG (2)] Device supports the RESET signal.
> >>>+\end{description}
> >>>+
> >>>+This set of bits is used by each virtio SDM device to declare which of the
> >>>+signals it supports. By specification each device can support all or a 
> >>>subset of
> >>>+the defined signals.
> >>>+
> >>>+\subsection{Device configuration layout}\label{sec:Device Types / SDM 
> >>>Device /
> >>>+Device configuration layout}
> >>>+
> >>>+The device configuration is composed by three fields:
> >>>+\texttt{max_slaves}, \texttt{current_slaves} and the \texttt{device_id}.
> >>>+
> >>>+\begin{lstlisting}
> >>>+struct virtio_sdm_config {
> >>>+  u16   max_slaves;
> >>>+  u16   current_slaves;
> >>>+  u32   device_id;
> >>>+};
> >>>+\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
> >>>+driver to figure out whether a specific instance is a master or a slave.
> >>>+The \texttt{max_slaves} field contains the maximum number of slaves 
> >>>supported by
> >>>+the SDM device. A configuration change notification is sent to the driver 
> >>>each
> >>>+time the value of \texttt{max_slaves} is changed from the device side.
> >>>+Finally, the \texttt{current_slaves} field contains the actual number of 
> >>>slaves
> >>>+registered to the master SDM. This field is a read only field. Finally the
> >>>+\texttt{device_id} field is used by each driver to know the ID of the 
> >>>device it
> >>>+is attached to, the field contains 0 in case of a master instance. A 
> >>>driver
> >>>+figures out whether it is attached to a master or a slave instance thanks 
> >>>to this
> >>>+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 driver 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. It has to be noted that the behavior of the device is
> >>>+independent from the communication channel used, that is a detail of each
> >>>+specific implementation of the SDM device.
> >>>+
> >>>+The last phase of the initialization is the negotiation of the feature 
> >>>bits.
> >>>+Each device implementation declares the signals supported by offering all 
> >>>or a
> >>>+subset of the three feature bits (VIRTIO_SDM_F_IRQ_SIG, 
> >>>VIRTIO_SDM_F_BOOT_SIG,
> >>>+VIRTIO_SDM_F_RESET_SIG). The SDM driver will be aware of the set of 
> >>>signals to
> >>>+handle thanks to this negotiation phase.
> >>>+
> >>>+\subsection{Device Operation}\label{sec:Device Types / SDM Device / Device
> >>>+peration}
> >>>+
> >>>+The SDM device handles signals coming from the two following sources:
> >>>+
> >>>+\begin{enumerate}
> >>>+\item The local processor to which the device is attached to.
> >>>+\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 a second SDM device instance.
> >>>+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 themselves 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,
> >>>+    SDM_RESET,
> >>>+};
> >>>+
> >>>+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 three 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{SDM_RESET} is also meant to be used in AMP like scenarios, to
> >>>+trigger the reset of the target slave processor. As an assumption a driver
> >>>+cannot trigger the reset of the device it is attached to, so each driver
> >>>+implementation should ignore reset signals where the source slave 
> >>>corresponds to
> >>>+the device ID the driver is attached to.
> >>>+This is done by comparing, when a message is recevied, the value of
> >>>+the \texttt{slave} field of the \texttt{SDMSignalData} data structure 
> >>>with the
> >>>+\texttt{device_id} field of the configuration space.
> >>>+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. When instead a message is recevied, this field contains the 
> >>>ID of
> >>>+the slave source of the signal.
> >>>+The \texttt{payload} is used to pass extra accompanying information with 
> >>>the
> >>>+signal.
> >>>+The semantics of the payload field depends on the signal itself. In case 
> >>>of
> >>>+\texttt{SDM_IRQ} signal, the payload field is ignored since interrupts do 
> >>>not
> >>>+need any extra information to be handled. In the case of \texttt{SDM_BOOT}
> >>>+signal the payload contains the boot address of the slave processor, to 
> >>>be used
> >>>+at the destination to initialize the program counter register before the 
> >>>actual
> >>>+boot process is started. Finally the payload field is ignored also in 
> >>>case of
> >>>+\texttt{SDM_RESET} signal, since no extra information is needed to 
> >>>trigger the
> >>>+reset of the target processor.
> >>>+
> >>>+
> >>>+The \texttt{SDMSignalData} structure is first filled by the source SDM 
> >>>kernel
> >>>+virtio driver and sent over the gh_vq.
> >>
>