From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50448) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhXdY-000448-Rd for qemu-devel@nongnu.org; Wed, 07 Sep 2016 03:51:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhXdS-00061o-Or for qemu-devel@nongnu.org; Wed, 07 Sep 2016 03:51:11 -0400 Received: from mail-lf0-x243.google.com ([2a00:1450:4010:c07::243]:36471) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhXdS-00061k-AT for qemu-devel@nongnu.org; Wed, 07 Sep 2016 03:51:06 -0400 Received: by mail-lf0-x243.google.com with SMTP id j41so370405lfi.3 for ; Wed, 07 Sep 2016 00:51:06 -0700 (PDT) Date: Wed, 7 Sep 2016 09:51:03 +0200 From: "Edgar E. Iglesias" Message-ID: <20160907075103.GA10667@toto> References: <1470728248-8424-1-git-send-email-c.pinto@virtualopensystems.com> <8b21e409-9d1d-036d-6350-8e8181379bd9@virtualopensystems.com> <20160906214311.GA4814@toto> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [virtio-dev][RFC v3] virtio-sdm: new device specification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Pinto Cc: virtio-dev@lists.oasis-open.org, Baptiste Reynal , Claudio.Fontana@huawei.com, qemu-devel@nongnu.org, stefanha@redhat.com, cornelia.huck@de.ibm.com, Jani.Kokkonen@huawei.com, tech@virtualopensystems.com 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 > >>>Signed-off-by: Baptiste Reynal > >>> > >>>--- > >>>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. > >> >