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

[Edgar E. Iglesias]

On Wed, Sep 07, 2016 at 05:39:08PM +0200, Christian Pinto wrote:
> 
> On 07/09/2016 09:51, Edgar E. Iglesias wrote:
> >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
> 
> All signals carry a 64 bits payload that for the time being is ignored
> for the IRQ signal. I now see your point, thanks for the clarification.
> The payload could definitely be used as an IRQ vector, so that SW
> can register to a specific IRQ line of the SDM HW interrupt.

Thanks Christian,

Perhaps we could update the spec and elaborate a little on this and
change the payload from ignored to provide and notifier/irq index?

> 
> >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?
> 
> The AMP systems are not all interacting with the same virtio queues.
> As described in the specification the device can be a master or a slave.
> The master processor will interact with a master SDM instance,
> while remote processors with slave SDM instances.
> Master/slave instances should be seen as multiple ports of the same
> SDM.
> Master and slave SDM instances communicate using the SDM Channel.
> So, no need for locking or to handle concurrent updates from
> multiple processors to the same virtio queue.



These are the parts that I find a little confusing:

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

...

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{SDMSignalData} structure is first filled by the source SDM kernel
virtio driver and sent over the gh_vq.


I interpret that as there being a single gh_vq queue and multiple slaves 
sending events
on that queue. Do you mean that there are multiple gh_vq's?
Am I missing something?

Best regards,
Edgar


> 
> Christian
> 
> >
> >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>
> >>>>><mailto:address@hidden>
> >>>>>Signed-off-by: Baptiste Reynal<address@hidden>
> >>>>><mailto: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.