virtio: Add specification for virtio-gpio

virtio-gpio is a virtual GPIO controller. It provides a way to flexibly communicate with the host GPIO controllers from the guest.

Note that the current implementation doesn't provide atomic APIs for GPIO configurations. i.e. the driver (guest) would need to implement sleep-able versions of the APIs as the guest will respond asynchronously over the virtqueue.

This patch adds the specification for it.

Based on the initial work posted by:
"Enrico Weigelt, metux IT consult" <[email protected]>.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/110

Reviewed-by Arnd Bergmann <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
 conformance.tex |  28 +++-
 content.tex     |   1 +
 virtio-gpio.tex | 346 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 371 insertions(+), 4 deletions(-)
 create mode 100644 virtio-gpio.tex

virtio-gpio.tex new file +26 -0

new file mode 100644
index 000000000000..1d1ac672db37
@@ -0,0 +1,346 @@
+\section{GPIO Device}\label{sec:Device Types / GPIO Device}
+
+The Virtio GPIO device is a virtual General Purpose Input/Output device that
+supports a variable number of named I/O lines, which can be configured in input
+mode or in output mode with logical level low (0) or high (1).
+
+\subsection{Device ID}\label{sec:Device Types / GPIO Device / Device ID}
+41
+
+\subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
+
+None currently defined.
+
+\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
+
+GPIO device uses the following configuration structure layout:
+
+\begin{lstlisting}
+struct virtio_gpio_config {
+    le16 ngpio;

virtio-gpio.tex new file +91 -0

+\end{itemize}
+
+\subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation}
+
+The driver uses the \field{requestq} virtqueue to send messages to the device.
+The driver sends a pair of buffers, request (filled by driver) and response (to
+be filled by device later), to the device. The device in turn fills the response
+buffer and sends it back to the driver.
+
+\begin{lstlisting}
+struct virtio_gpio_request {
+    le16 type;
+    le16 gpio;
+    le32 value;
+};
+\end{lstlisting}
+
+All the fields of this structure are set by the driver and read by the device.
+
+\begin{description}
+\item[\field{type}] is the GPIO message type, i.e. one of
+    \field{VIRTIO_GPIO_MSG_*} values.
+
+\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
+    \field{ngpio}.
+
+\item[\field{value}] is a message specific value.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_gpio_response {
+    u8 status;
+    u8 value;
+};
+
+/* Possible values of the status field */
+#define VIRTIO_GPIO_STATUS_OK                   0x0
+#define VIRTIO_GPIO_STATUS_ERR                  0x1
+\end{lstlisting}
+
+All the fields of this structure are set by the device and read by the driver.
+
+\begin{description}
+\item[\field{status}] of the GPIO message,
+    \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR}
+    on failure.
+
+\item[\field{value}] is a message specific value.
+\end{description}
+
+Following is the list of messages supported by the virtio gpio specification.
+
+\begin{lstlisting}
+/* GPIO message types */
+#define VIRTIO_GPIO_MSG_GET_LINE_NAMES          0x0001
+#define VIRTIO_GPIO_MSG_GET_DIRECTION           0x0002
+#define VIRTIO_GPIO_MSG_SET_DIRECTION           0x0003
+#define VIRTIO_GPIO_MSG_GET_VALUE               0x0004
+#define VIRTIO_GPIO_MSG_SET_VALUE               0x0005
+
+/* GPIO Direction types */
+#define VIRTIO_GPIO_DIRECTION_NONE              0x00
+#define VIRTIO_GPIO_DIRECTION_OUT               0x01
+#define VIRTIO_GPIO_DIRECTION_IN                0x02
+\end{lstlisting}
+
+\subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names}
+
+The driver sends this message to receive a stream of zero-terminated strings,
+where each string represents the name of a GPIO line, present in increasing
+order of the GPIO line numbers. The names of the GPIO lines are optional and may
+be present only for a subset of GPIO lines. If missing, then a zero-byte must be
+present for the GPIO line. If present, the name string must be zero-terminated
+and the name must be unique within a GPIO Device.
+
+These names of the GPIO lines should be most meaningful producer names for the
+system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd"
+and "ethernet reset" are reasonable line names as they describe what the line is
+used for, while "GPIO0" is not a good name to give to a GPIO line.
+
+Here is an example of how the gpio names memory block may look like for a GPIO
+device with 10 GPIO lines, where line names are provided only for lines 0
+("MMC-CD"), 5 ("Red LED Vdd") and 7 ("ethernet reset").
+
+\begin{lstlisting}
+u8 gpio_names[] = {
+    'M', 'M', 'C', '-', 'C', 'D', 0,
+    0,
+    0,
+    0,
+    0,

Quoted 4 lines · Viresh Kumar

+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization} +
+\begin{itemize}
+\item The driver MUST configure and initialize the \field{requestq} virtqueue.

Also outside of conformance sections. Maybe "The driver configures and initializes..." ? I don't think that one requires a normative statement.

Quoted 6 lines · Cornelia Huck

Quoted 4 lines · Viresh Kumar

+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization} +
+\begin{itemize}
+\item The driver MUST configure and initialize the \field{requestq} virtqueue.

Quoted 2 lines

Also outside of conformance sections. Maybe "The driver configures and initializes..." ? I don't think that one requires a normative statement.

Not sure I understood it well, but are you suggesting to drop this line altogether ? I wrote it as I saw similar lines/sections for other protocols, I can drop it though.

This patch adds support for interrupts to the virtio-gpio specification. This uses the feature bit 0 for the same.

Reviewed-by Linus Walleij <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

+\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}

virtio-gpio.tex +17 -0

+
+\begin{itemize}
+\item The driver is requested by a client driver to enable interrupt for a GPIO
+    line and configure it to a particular trigger type.
+
+\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the
+    \field{requestq} virtqueue and the device configures the GPIO line for the
+    requested trigger type and unmasks the interrupt.
+
+\item The driver queues a pair of buffers, interrupt-request and
+    interrupt-response, to the \field{eventq} virtqueue for the GPIO line.
+
+\item The driver notifies the device of the presence of new buffers on the
+    \field{eventq} virtqueue.
+
+\item The interrupt is fully configured at this point.
+

virtio-gpio.tex \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi · line 326

Quoted 8 lines · Viresh Kumar

virtio-gpio.tex +8 -0

+If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST
+mask the interrupt for the GPIO line and discard any latched interrupt event
+associated with it. The device MUST also return any unused pair of buffers
+for the GPIO line, over the \field{eventq} virtqueue, after setting the
+\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. If the driver
+queues another pair of buffers, while the trigger type remains set as
+\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of
+type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers.

We discussed this last part before, and you had already said you prefer this ordering,
but after you explain it here, the interaction of VIRTIO_GPIO_MSG_SET_IRQ_TYPE with queued buffers still seems inconsistent to me: If setting the

type to none cancels
any outstanding eventq request, why do newly queued eventq request get added to the queue? If we change the order to always require the type to be enabled before
queuing any events, this inconsistency would go away. With the behavior of the level
triggered interrupts changed to fasteoi, do you still see a need to keep this ordering?

Quoted 6 lines

+\item In a typical guest operating system kernel, the virtio-gpio driver

virtio-gpio.tex +5 -0

+    notifies the client driver, that is associated with this gpio line, to
+    process the event.
+
+\item In the case of a level triggered interrupt, the client driver must fully
+    process and acknowledge the event at its source to return the line to its

virtio-gpio.tex \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D · line 538

Quoted 4 lines

virtio-gpio.tex +4 -0

+    inactive state.
+
+\item The driver may again queue, same or new, pair of buffers for that GPIO
+    line and notify the device.

I suggested this bit, but on second thought it still doesn't quite capture the important bit that the 'level' line has to be set to low /before/ the new buffer gets queued (to avoid a spurious notification).

Otherwise there is no difference
between edge and level interrupts, obviously in both cases the interrupt has to be processed eventually.

This patch adds support for interrupts to the virtio-gpio specification. This uses the feature bit 0 for the same.

Reviewed-by Linus Walleij <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
 conformance.tex |   2 +
 virtio-gpio.tex | 234 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 235 insertions(+), 1 deletion(-)

conformance.tex +2 -0

index c52f1a40be2d..64bcc12d1199 100644
@@ -310,6 +310,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 
 \begin{itemize}
 \item \ref{drivernormative:Device Types / GPIO Device / requestq Operation}
+\item \ref{drivernormative:Device Types / GPIO Device / eventq Operation}
 \end{itemize}
 
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}

@@ -568,6 +569,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 
 \begin{itemize}
 \item \ref{devicenormative:Device Types / GPIO Device / requestq Operation}
+\item \ref{devicenormative:Device Types / GPIO Device / eventq Operation}
 \end{itemize}
 
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}

virtio-gpio.tex +160 -1

index 1d1ac672db37..d5746fa883c0 100644
@@ -11,11 +11,17 @@ \subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
 
 \begin{description}
 \item[0] requestq
+\item[1] eventq
 \end{description}
 
+The \field{eventq} virtqueue is available only if the \field{VIRTIO_GPIO_F_IRQ}
+feature is enabled by the device.
+
 \subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
 
-None currently defined.
+\begin{description}
+\item[VIRTIO_GPIO_F_IRQ (0)] The device supports interrupts on GPIO lines.
+\end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
 

@@ -46,6 +52,15 @@ \subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device
 
 \begin{itemize}
 \item The driver MUST configure and initialize the \field{requestq} virtqueue.
+
+\item The driver MUST check the presence of \field{VIRTIO_GPIO_F_IRQ} feature
+    before initiating any IRQ messages.
+
+\item The driver MUST configure and initialize the \field{eventq} virtqueue if
+    the \field{VIRTIO_GPIO_F_IRQ} feature is enabled by the device.
+
+\item If the \field{VIRTIO_GPIO_F_IRQ} feature is supported, then the interrupt
+    for all GPIO lines must be in \field{VIRTIO_GPIO_IRQ_TYPE_NONE} state.
 \end{itemize}
 
 \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation}

@@ -105,11 +120,20 @@ \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / r
 #define VIRTIO_GPIO_MSG_SET_DIRECTION           0x0003
 #define VIRTIO_GPIO_MSG_GET_VALUE               0x0004
 #define VIRTIO_GPIO_MSG_SET_VALUE               0x0005
+#define VIRTIO_GPIO_MSG_SET_IRQ_TYPE            0x0006
 
 /* GPIO Direction types */
 #define VIRTIO_GPIO_DIRECTION_NONE              0x00
 #define VIRTIO_GPIO_DIRECTION_OUT               0x01
 #define VIRTIO_GPIO_DIRECTION_IN                0x02
+
+/* GPIO interrupt types */
+#define VIRTIO_GPIO_IRQ_TYPE_NONE               0x00
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING        0x01
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING       0x02
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH          0x03
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH         0x04
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW          0x08
 \end{lstlisting}
 
 \subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names}

@@ -269,6 +293,66 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi
 \hline
 \end{tabularx}
 
+\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type}
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is enabled
+by the device.
+
+In order to configure and receive interrupts for a GPIO line, the driver needs
+to perform two operations. The driver first needs to send the
+\field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the \field{requestq}
+virtqueue, and then queue a pair of buffers, of type
+\field{virtio_gpio_irq_request} and \field{virtio_gpio_irq_response}, over the
+\field{eventq} virtqueue for the GPIO line. A separate pair of buffers must be
+queued for each GPIO line, the driver wants to configure for interrupts.
+
+The driver sets the trigger type to values other than
+\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, to unmask the interrupt and get notified over
+the \field{eventq} virtqueue. The driver sets the trigger type to
+\field{VIRTIO_GPIO_IRQ_TYPE_NONE} to mask the interrupt, and get back the unused
+buffers over the \field{eventq} virtqueue.
+
+Once the buffers are made available by the driver over the \field{eventq}
+virtqueue, the device can notify the driver of an active interrupt signaled on
+the GPIO line by returning the buffers to the driver.
+
+If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST
+mask the interrupt for the GPIO line and discard any latched interrupt event
+associated with it. The device MUST also return any unused pair of buffers
+for the GPIO line, over the \field{eventq} virtqueue, after setting the
+\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. If the driver
+queues another pair of buffers, while the trigger type remains set as
+\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of
+type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers.
+
+The device MUST unmask the interrupt for a GPIO line, if the trigger type
+received from driver is any valid value other than
+\field{VIRTIO_GPIO_IRQ_TYPE_NONE}. For edge trigger type, the device MUST latch
+the interrupt event from this point onward and report it to the driver as soon
+as a buffers are available. For level trigger type, the device MUST ignore the
+active interrupts signaled on the line, until the time the buffers are made
+available by the driver. The device MUST keep on notifying the driver of the
+interrupts, as and when an interrupt becomes active and the buffers are
+available on the \field{eventq} virtqueue. The device MUST stop notifying the
+driver, once the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, and
+return any unused buffers.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 \\
+\hline
+\end{tabularx}
+
 \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow}
 
 \begin{itemize}

@@ -312,6 +396,16 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
 
 \item The driver MAY send multiple messages for same or different GPIO lines in
     parallel.
+
+\item The driver MUST NOT send IRQ messages for a GPIO line configured for
+    output.
+
+\item The driver MUST NOT send IRQ messages if the \field{VIRTIO_GPIO_F_IRQ}
+    feature is not enabled by the device.
+
+\item The driver SHOULD set the IRQ trigger type to
+    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
+    previously configured for a different trigger type.
 \end{itemize}
 
 \devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation}

@@ -344,3 +438,141 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
     line, once the driver has requested to set its direction to
     \field{VIRTIO_GPIO_DIRECTION_NONE}.
 \end{itemize}
+
+\subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eventq Operation}
+
+The \field{eventq} virtqueue is used for sending interrupt events from the
+device to the driver. The driver queues a separate pair of buffers,
+\field{struct virtio_gpio_irq_request} (filled by driver) and \field{struct
+virtio_gpio_irq_response} (to be filled by device later), to the \field{eventq}
+virtqueue for each GPIO line. The device, on sensing an active interrupt,
+returns the pair of buffers of the respective GPIO line for which the interrupt
+is active.
+
+\begin{lstlisting}
+struct virtio_gpio_irq_request {
+    le16 gpio;
+};
+\end{lstlisting}
+
+This structure is filled by the driver and read by the device.
+
+\begin{description}
+\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
+    \field{ngpio}.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_gpio_irq_response {
+    u8 status;
+};
+
+/* Possible values of the interrupt status field */
+#define VIRTIO_GPIO_IRQ_STATUS_INVALID          0x0
+#define VIRTIO_GPIO_IRQ_STATUS_VALID            0x1
+\end{lstlisting}
+
+This structure is filled by the device and read by the driver.
+
+\begin{description}
+\item[\field{status}] of the interrupt event,
+    \field{VIRTIO_GPIO_IRQ_STATUS_VALID} on valid interrupt and
+    \field{VIRTIO_GPIO_IRQ_STATUS_INVALID} otherwise on returning the buffer
+    back to the driver without an interrupt.
+\end{description}
+
+\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}
+
+\begin{itemize}
+\item The driver is requested by a client driver to enable interrupt for a GPIO
+    line and configure it to a particular trigger type.
+
+\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the
+    \field{requestq} virtqueue and the device configures the GPIO line for the
+    requested trigger type and unmasks the interrupt.
+
+\item The driver queues a pair of buffers, interrupt-request and
+    interrupt-response, to the \field{eventq} virtqueue for the GPIO line.
+
+\item The driver notifies the device of the presence of new buffers on the
+    \field{eventq} virtqueue.
+
+\item The interrupt is fully configured at this point.
+
+\item The device, on sensing an active interrupt on the GPIO line, finds the
+    matching buffers (based on GPIO line number) from the \field{eventq}
+    virtqueue and fills its \field{struct virtio_gpio_irq_response} buffer's
+    \field{status} with \field{VIRTIO_GPIO_IRQ_STATUS_VALID} and returns the

virtio-gpio.tex +10 -0

+
+\item If the GPIO line is configured for edge interrupts, the device MUST latch
+    the latest interrupt received for this GPIO line, until the time the buffers
+    are made available again by the driver. At that point, the device MUST
+    notify the driver if an interrupt was received while the device was waiting
+    for the buffers to be made available by the driver. If the interrupt becomes
+    active at a later point of time, then the device MUST notify the driver at
+    that instance.
+
+\item The driver on receiving the notification from the device, processes the

virtio-gpio.tex \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D · line 515

Quoted 6 lines · Viresh Kumar

virtio-gpio.tex +6 -0

+
+\item If the GPIO line is configured for level interrupts, the device MUST
+    ignore an active interrupt signaled on this GPIO line, until the time the
+    buffers are made available again by the driver. Once the buffers are
+    available again, and the interrupt on the line is still active, the device
+    MUST notify the driver again of an interrupt event.

It feels like there is a problem here. A virtio device signals interrupts by using edge triggered signalling. Nothing wrong with that. However, signalling a level over a an edge signalling is much more tricky.

For example, let's assume that the irqchip driver handles a level
interrupt: it gets the message from the device indicating that a GPIO level interrupt is pending. During the handling, the interrupt is made pending again, without having ever transitioned via an inactive state. If you don't have a mechanism to retrigger the level, you have lost an interrupt.

I can't see anything in this document that hints at a way to resample/retrigger a level interrupt, which is what you would normally do on EOI for an interrupt controller that implements level-over-edge signalling.

Quoted 20 lines · Marc Zyngier

virtio-gpio.tex \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D · line 515

Quoted 6 lines · Viresh Kumar

virtio-gpio.tex +6 -0

+
+\item If the GPIO line is configured for level interrupts, the device MUST
+    ignore an active interrupt signaled on this GPIO line, until the time the
+    buffers are made available again by the driver. Once the buffers are
+    available again, and the interrupt on the line is still active, the device
+    MUST notify the driver again of an interrupt event.

It feels like there is a problem here. A virtio device signals interrupts by using edge triggered signalling. Nothing wrong with that. However, signalling a level over a an edge signalling is much more tricky.

For example, let's assume that the irqchip driver handles a level
interrupt: it gets the message from the device indicating that a GPIO level interrupt is pending. During the handling, the interrupt is made pending again, without having ever transitioned via an inactive state. If you don't have a mechanism to retrigger the level, you have lost an interrupt.

Quoted 4 lines

I can't see anything in this document that hints at a way to resample/retrigger a level interrupt, which is what you would normally do on EOI for an interrupt controller that implements level-over-edge signalling.

Thanks a lot for taking a closer look. I still hope this is just a problem that needs to be clarified in the description, not something wrong with the design, as I don't see the problem yet. As far as I can tell, this is different from an edge interrupt, since the eventq communication is really a two-way message.

The EOI for the level interrupt is the message being enqueued after the guest is done processing the interrupt. I can see four ways this could go:

1. If the interrupt has not been made pending again yet, nothing happens until it eventually becomes pending again (this is the obvious case)

2. If it is already pending, chip->irq_eoi() causes the new event descriptor to be queued on the eventq, and it signals the host about new buffers being available. The /host/ samples the level of the line, notices it is pending and puts the resulting buffer back on the 'used' queue even before returning from the guest 'notify' function.
The guest virtio core code keeps processing the 'used' buffers and gets back into the interrupt handler.

3. Same as 2., but the host handles the virtqueue ->notify asynchronously, and the guest has already checked the 'used' queue before the host adds back the buffer to tell it that the line is still active. Now the guest gets notified again after it returns from the virtqueue interrupt, in order to process the new 'used' buffer.

4. The gpio line actually goes into inactive state until after the new event is queued in chip->irq_eoi(), but becomes active immediately afterwards. Now the host gets interrupted and handles this by queuing the event reply but cannot interrupt the guest because it is still processing the original virtqueue event. However, since the event is queued, it will be processed the same way as 2. or 3. by the virtio core.

Do you see a problem with scenarios 2, 3 or 4, or with another case that I may have missed?

[This all assumes that the host is able to atomically enable interrupts and check the current level when processing the new buffer. If the host uses the Linux gpiolib ioctl interface, this means it has to register for an edge event on the line first, and then read the current value before adding the file descriptor to its poll table. I feel this is out of the scope of the virtio spec though.]

Quoted 30 lines · Arnd Bergmann

On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier <[email protected]> wrote:

The EOI for the level interrupt is the message being enqueued after the guest is done processing the interrupt. I can see four ways this could go:

1. If the interrupt has not been made pending again yet, nothing happens until it eventually becomes pending again (this is the obvious case)

2. If it is already pending, chip->irq_eoi() causes the new event descriptor to be queued on the eventq, and it signals the host about new buffers being available. The /host/ samples the level of the line, notices it is pending and puts the resulting buffer back on the 'used' queue even before returning from the guest 'notify' function.
The guest virtio core code keeps processing the 'used' buffers and gets back into the interrupt handler.

3. Same as 2., but the host handles the virtqueue ->notify asynchronously, and the guest has already checked the 'used' queue before the host adds back the buffer to tell it that the line is still active. Now the guest gets notified again after it returns from the virtqueue interrupt, in order to process the new 'used' buffer.

4. The gpio line actually goes into inactive state until after the new event is queued in chip->irq_eoi(), but becomes active immediately afterwards. Now the host gets interrupted and handles this by queuing the event reply but cannot interrupt the guest because it is still processing the original virtqueue event. However, since the event is queued, it will be processed the same way as 2. or 3. by the virtio core.

Quoted 2 lines

Do you see a problem with scenarios 2, 3 or 4, or with another case that I may have missed?

Thanks, that's really useful. I don't think you missed much. What the documentation is missing though is an actual interrupt controller specification. Although it describes the protocol in great length, at no point does it explain what the irq_response does in terms of interrupt life cycle (there is no interrupt life cycle at all). Same goes for the various states that an interrupt can get. As someone who spends way too much time reading interrupt controller specs, this is a first class defect.

Another point I have just realised is that this spec confuses interrupt mask with interrupt enable. It describes masking interrupts as an effect of setting the trigger type, but that's completely unusable. There is a strong expectation from SW that a masked interrupt doesn't loose signals. If the interrupt is masked by setting its trigger to NONE, then an edge interrupt coming at this point will be lost. No cookie for you.

I'm fine with this odd way of *enabling* the interrupt, but masking cannot lose any signal, ever.

Another unclear aspect is how you switch from one trigger type to another. Do you have to transition via NONE? I have the strong feeling that you should if you want to be robust against spurious signals.

Quoted 7 lines

[This all assumes that the host is able to atomically enable interrupts and check the current level when processing the new buffer. If the host uses the Linux gpiolib ioctl interface, this means it has to register for an edge event on the line first, and then read the current value before adding the file descriptor to its poll table. I feel this is out of the scope of the virtio spec though.]

There are certainly a number of implementation difficulties with this. At this stage, I'm more worried about the guest-visible aspects so far, but I guess that I should also look at the host side at some point.

Is there any sample code we could look at, both got guest and host?

Quoted 1 line · Marc Zyngier

Is there any sample code we could look at, both got guest and host?

Viresh said that he is close to sending it after updating the code to the latest spec.

Quoted 34 lines · Marc Zyngier

Quoted 30 lines · Arnd Bergmann

On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier <[email protected]> wrote:

The EOI for the level interrupt is the message being enqueued after the guest is done processing the interrupt. I can see four ways this could go:

1. If the interrupt has not been made pending again yet, nothing happens until it eventually becomes pending again (this is the obvious case)

2. If it is already pending, chip->irq_eoi() causes the new event descriptor to be queued on the eventq, and it signals the host about new buffers being available. The /host/ samples the level of the line, notices it is pending and puts the resulting buffer back on the 'used' queue even before returning from the guest 'notify' function.
The guest virtio core code keeps processing the 'used' buffers and gets back into the interrupt handler.

3. Same as 2., but the host handles the virtqueue ->notify asynchronously, and the guest has already checked the 'used' queue before the host adds back the buffer to tell it that the line is still active. Now the guest gets notified again after it returns from the virtqueue interrupt, in order to process the new 'used' buffer.

4. The gpio line actually goes into inactive state until after the new event is queued in chip->irq_eoi(), but becomes active immediately afterwards. Now the host gets interrupted and handles this by queuing the event reply but cannot interrupt the guest because it is still processing the original virtqueue event. However, since the event is queued, it will be processed the same way as 2. or 3. by the virtio core.

Quoted 2 lines

Do you see a problem with scenarios 2, 3 or 4, or with another case that I may have missed?

Quoted 8 lines

Thanks, that's really useful. I don't think you missed much. What the documentation is missing though is an actual interrupt controller specification. Although it describes the protocol in great length, at no point does it explain what the irq_response does in terms of interrupt life cycle (there is no interrupt life cycle at all). Same goes for the various states that an interrupt can get. As someone who spends way too much time reading interrupt controller specs, this is a first class defect.

Ok, makes sense.

Quoted 8 lines

Another point I have just realised is that this spec confuses interrupt mask with interrupt enable. It describes masking interrupts as an effect of setting the trigger type, but that's completely unusable. There is a strong expectation from SW that a masked interrupt doesn't loose signals. If the interrupt is masked by setting its trigger to NONE, then an edge interrupt coming at this point will be lost. No cookie for you.

Quoted 2 lines

I'm fine with this odd way of *enabling* the interrupt, but masking cannot lose any signal, ever.

Viresh's previous first version actually had this, I asked him to simplify it in order to reduce the number of possible states, as it seemed to me that it would be better not to have eight possible states when you have the various combinations of enabled/disabled, unmasked/masked and armed/not-armed.

The current version folds
unmasked/masked into armed/not-armed by masking the interrupt whenever the reply has been queued to the guest, until it gets queued again on the eventq.

This avoids the problem that using the controlq to mask explicitly mask the interrupt cannot be done from atomic context since it has to wait_for_completion() until the controlq message is returned. Queing up the eventq message to unmask the even can be done in atomic context, since this is a posted operation.

What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?

Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.

Quoted 3 lines · Arnd Bergmann

What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?

irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.

Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.

The later would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.

Quoted 4 lines

Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.

That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.

Hi Marc,

Quoted 10 lines · Marc Zyngier

Quoted 3 lines · Arnd Bergmann

What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?

irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.

Right, I already have that in place but I have a few doubts on that.

I can see irq_bus_lock/unlock() getting called around irq_mask/unmask/set_type() during request_irq(), and so I can issue all the virtio calls from irq_bus_unlock() there.

But I don't see the same sequence being followed by the irq core when an interrupt actually happens, i.e. when I call generic_handle_irq() from virtqueue callback for eventq vq. In this case, the irq core can directly call mask/unmask() around irq_eoi(), without calling the irq_bus_lock/unlock() guards.

How should the driver take care of these mask/unmask calls, or should it take care of it at all, as they should negate each other eventually ?

Quoted 5 lines

Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.

We discussed this sometime back over IRC, and Jean Philippe said this in context of virtio-iommu:

"From some profiling the overhead of context switching is much greater than that of adding buffers, so adding a new virtio interface to bypass the virtqueue might not show any improvement."

We were looking to explore the config space for normal GPIO operations as well then.

The host would be required to perform some action here on a config update and the guest would be waiting over an atomic operation for it to complete. So eventually it should also be slow.

Quoted 8 lines

The later would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.

Quoted 4 lines

Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.

Quoted 3 lines

That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.

Right, irq_bus_unlock() works fine for this though.

Quoted 9 lines · Arnd Bergmann

Quoted 2 lines · Marc Zyngier

Quoted 1 line · Arnd Bergmann

On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier <[email protected]> wrote:

Viresh's previous first version actually had this, I asked him to simplify it in order to reduce the number of possible states, as it seemed to me that it would be better not to have eight possible states when you have the various combinations of enabled/disabled, unmasked/masked and armed/not-armed.

What exactly is armed/not-armed? Is that equivalent to pending?

Quoted 4 lines

The current version folds
unmasked/masked into armed/not-armed by masking the interrupt whenever the reply has been queued to the guest, until it gets queued again on the eventq.

Huh. I understood that it was mask end enabled that were folded together. I'm lost.

Quoted 9 lines

This avoids the problem that using the controlq to mask explicitly mask the interrupt cannot be done from atomic context since it has to wait_for_completion() until the controlq message is returned. Queing up the eventq message to unmask the even can be done in atomic context, since this is a posted operation.

What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?

irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.

Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.

The latter would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.

Quoted 4 lines

Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.

That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.