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

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

+\begin{lstlisting}

virtio-gpio.tex new file +12 -0

+struct virtio_gpio_response {
+    u8 status;
+    u8 value[N];
+};
+
+/* 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.
+

I think the structure layout here is good, but I wonder if the description would be clearer
if you define two separate structures, with a fixed-length structure for normal requests, and the variable-length structure for VIRTIO_GPIO_MSG_GET_LINE_NAMES.

+\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}

virtio-gpio.tex new file +9 -0

+
+The driver sends this message to request the device to return a line's
+configured direction.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\

+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 = low, 1 = high & 1 \\
+\hline
+\end{tabularx}

overall this looks good, with the minor nits pointed out:

Reviewed-by Linus Walleij <[email protected]>

Quoted 17 lines

+\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}

virtio-gpio.tex new file +15 -0

+
+The driver sends this message to request the device to return a line's
+configured direction.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\

Quoted 4 lines

+\hline
+ & \field{VIRTIO_GPIO_STATUS_*} & \field{VIRTIO_GPIO_DIRECTION_*} & 1 \\
+\hline
+\end{tabularx}

Is it clearly defined which value of VIRTIO_GPIO_DIRECTION_[IN|OUT] mean "input" and which value means "output"? Else add some text clarifying this.

Quoted 2 lines · Linus Walleij

overall this looks good, with the minor nits pointed out:

Reviewed-by Linus Walleij <[email protected]>

Thanks.

Quoted 26 lines · Viresh Kumar

Quoted 11 lines

+\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}

virtio-gpio.tex new file +9 -0

+
+The driver sends this message to request the device to return a line's
+configured direction.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\

virtio-gpio.tex line 192

Quoted 10 lines

virtio-gpio.tex new file +10 -0

+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+    & \field{VIRTIO_GPIO_STATUS_*} & \field{VIRTIO_GPIO_DIRECTION_*} & 1 \\
+\hline
+\end{tabularx}

Is it clearly defined which value of VIRTIO_GPIO_DIRECTION_[IN|OUT] mean "input" and which value means "output"? Else add some text clarifying this.

virtio-gpio.tex new file +4 -0

+/* GPIO Direction types */
+#define VIRTIO_GPIO_DIRECTION_NONE              0x00
+#define VIRTIO_GPIO_DIRECTION_OUT               0x01
+#define VIRTIO_GPIO_DIRECTION_IN                0x02

Not sure if you missed this.

Were you looking for this or some text saying "VIRTIO_GPIO_DIRECTION_IN means direction-input", which looked self explanatory to me at least, though I can surely add it anyway.

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

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

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

On Wed, Jul 28, 2021 at 1:11 PM Viresh Kumar <[email protected]> wrote:

This also looks good to me overall, I have a few idea for clarifications below, but none of them are likely to change the actual behavior.

More importantly though, I would like to see a review by the linux irqchip maintainers (Marc Zyngier and/or Thomas Gleixner, both added to Cc here) before this gets merged into the virtio specification. I have a basic understanding
of the subsystem but probably got something wrong myself.

That review is probably easier once you post the driver code for it but if Thomas or Marc want to comment on the spec before seeing the driver, the patch is available at [1], along with the base virtio-gpio spec.

Quoted 11 lines

+\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type}

virtio-gpio.tex +10 -0

+
+The driver sends this message to request the device to set the IRQ trigger type,
+to one of the \field{VIRTIO_GPIO_IRQ_TYPE_*} values, for a line configured for
+input.
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
+enabled by the device.
+
+The device MUST mask the interrupt for a GPIO line, if the trigger type is set
+to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt

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

Quoted 2 lines

virtio-gpio.tex +2 -0

+status associated with the line. The device MUST unmask the interrupt for any
+other value of the trigger type.

I think the last sentence here is somewhat ambiguous, I'm not sure what it actually
means to unmask a level triggered line if the driver has not queued an event request
for that line.

For an edge triggered interrupt, I think it makes sense to unmask the interrupt at
the device level and latch any incoming events until a request gets queued on the
eventq.

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

Quoted 4 lines

+\item The driver MUST set the IRQ trigger type to

virtio-gpio.tex +2 -0

+    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
+    previously configured for a different trigger type.
 \end{itemize}

I'm not entirely sure why this "MUST" be done. Can't the device clean up the irq when the line is set back to direction=none or the queues are shut down?

Quoted 4 lines

+\item The driver queues a pair of buffers, interrupt-request and

virtio-gpio.tex +3 -0

+    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

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

Quoted 5 lines

virtio-gpio.tex +5 -0

+    \field{eventq} virtqueue.
+
+\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.

Ah, I guess that explains the flow. I was expecting the reverse order (first set-irq-type, then queue event request), but I suppose it works either way.

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

Quoted 6 lines

+\item The driver on receiving the notification from the device, processes the

virtio-gpio.tex +5 -0

+    interrupt. The driver may try to update the trigger-type of the interrupt
+    for the GPIO line over the \field{requestq} virtqueue.
+
+\item The driver may again queue, same or new, pair of buffers for that GPIO
+    line and notify the device.

I feel there needs to be another step between these two, as this is where some of the magic happens that guarantees we don't get too many or not enough interrupts. How about:

\item In a typical guest operating system kernel, the virtio-gpio
driver notifies another device driver that is associated with this gpio line to process the event.

\item In the case of a level triggered interrupt, the secondary device
driver must fully process and acknowledge the event at its source to return the line to its inactive state.

Quoted 8 lines · Arnd Bergmann

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

Quoted 4 lines

virtio-gpio.tex +4 -0

+The device MUST mask the interrupt for a GPIO line, if the trigger type is set
+to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
+status associated with the line. The device MUST unmask the interrupt for any
+other value of the trigger type.

I think the last sentence here is somewhat ambiguous, I'm not sure what it actually means to unmask a level triggered line if the driver has not queued an event request for that line.

(For everyone to follow, device == host, driver == guest)

There are two different situation where the interrupt can come and I believe the behavior should be different.

- The driver has requested for the trigger-type to be set and unmasking of the interrupt. The device MUST unmask the interrupt even if the buffer isn't queued over eventq, and so it must latch the interrupt value and report it as soon as the buffer is made available.

This behavior should be same for both edge or level interrupts.

- Once the interrupt is reported by the device, by returning the buffer over the eventq, the _level_ interrupt must be masked by the device until the time the buffer is re-queued by the driver, else it will end up reporting the same interrupt again as the line would have stayed at the same level until the interrupt handler had a chance to run and service the interrupt.

This is not the case for edge interrupt of course, where we need to latch value as that counts for a new interrupt.

Quoted 3 lines

For an edge triggered interrupt, I think it makes sense to unmask the interrupt at the device level and latch any incoming events until a request gets queued on the eventq.

I think the same holds true for level interrupts too.

Quoted 7 lines

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

Quoted 4 lines

+\item The driver MUST set the IRQ trigger type to

virtio-gpio.tex +2 -0

+    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
+    previously configured for a different trigger type.
 \end{itemize}

I'm not entirely sure why this "MUST" be done. Can't the device clean up the irq when the line is set back to direction=none or the queues are shut down?

Yes we can infer that from direction=none as well, but even then the device needs to return the unused buffers over the eventq. And I thought it maybe better to explicitly mark this to be done and not mix with other operations like direction=none.

For the queues being shut case, I see that as a crash or hard stop. The device may want to handle things in that case by its own way, but the spec must try to avoid that case entirely and not encourage people to do things that way :)

Quoted 4 lines

Quoted 4 lines

+\item The driver queues a pair of buffers, interrupt-request and

virtio-gpio.tex +3 -0

+    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

Quoted 8 lines

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

Quoted 5 lines

virtio-gpio.tex +5 -0

+    \field{eventq} virtqueue.
+
+\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.

Ah, I guess that explains the flow. I was expecting the reverse order (first set-irq-type, then queue event request), but I suppose it works either way.

Whatever the flow is here at the driver, the device may observe these differently as these are two different virtqueues here. The device should always provide the buffer before requesting the device to enable to interrupt IMO.

Wouldn't that cause the virtio-gpio device to send a second spurious interrupt reply for every hardware event?

During probe time, the level may be active if the device connected to the gpio has latched an event itself.

When that device gets initialized, its
driver would clear the status and bring the line to 'inactive' state before enabling interrupts. If the virtio-gpio device latches the state of the line having been active in the past, you get a first spurious reply.

The driver
will know how to deal with it, but it can be avoided.

For the later operation, the gpio client device signals an event by making the line active again, at this point the virtio-gpio device (in the host) gets signaled and queues the reply. If it then looks at the line, it is still active, so with your model it would have to latch another event.

The gpio slave driver in the guest then processes the event and deactivates the line in order to not get woken up again until something new happens.

If the virtio-gpio device has already latched another event, this logic fails and you get woken up again directly at the end-of-interrupt (i.e. requeueing the eventq message) even if the line is currently inactive.

Quoted 13 lines

Quoted 7 lines

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

Quoted 4 lines

+\item The driver MUST set the IRQ trigger type to

virtio-gpio.tex +2 -0

+    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
+    previously configured for a different trigger type.
 \end{itemize}

I'm not entirely sure why this "MUST" be done. Can't the device clean up the irq when the line is set back to direction=none or the queues are shut down?

Yes we can infer that from direction=none as well, but even then the device needs to return the unused buffers over the eventq. And I thought it maybe better to explicitly mark this to be done and not mix with other operations like direction=none.

Quoted 4 lines

For the queues being shut case, I see that as a crash or hard stop. The device may want to handle things in that case by its own way, but the spec must try to avoid that case entirely and not encourage people to do things that way :)

This still sounds like a "SHOULD" instead of "MUST" to me though, but I don't remember the exact definitions of these.

Fair enough, I don't care too much about this one, I was just thinking we'd expect it to do it the other way round since that is more consistent with the continued operation after the interrupts remain enabled and we go back and forth requeing new eventq requests without turning them off again.

Quoted 4 lines

Fair enough, I don't care too much about this one, I was just thinking we'd expect it to do it the other way round since that is more consistent with the continued operation after the interrupts remain enabled and we go back and forth requeing new eventq requests without turning them off again.

Coming back to the confusing part here. I think we should make the interrupt controller (device) dumb here and leave all intelligence at the users (driver).

What about defining the sequence of events like this:

For edge interrupt:

- driver sets trigger type to EDGE (falling/rising).
- device enables the interrupt line, latches a value only after this point.
- driver queues a buffer over eventq (here or before setting trigger type).
- Once buffer is available over eventq, device returns it back and latches the next interrupt as soon as it happens.
- driver on receiving the buffer, re-queues the buffer and handles the interrupt by calling client's handler. Though queuing the buffer it after handling the request won't make a huge difference (and may be better for simpler driver code) as the next interrupt is latched anyway by the device and can be processed only after this one is done at the driver (some locking will be there to make sure only one interrupt gets processed per line at a time).
- Note that in this sequence we don't mask/unmask the interrupt at all. That's what kernel will also do in handle_edge_irq() if I understood it correctly.

For level interrupt:

- driver sets trigger type to LEVEL (high/low).
- device enables the interrupt line, latches a value only after this point.
- driver queues a buffer over eventq (here or before setting trigger type).
- Once buffer is available over eventq, device returns it back and latches the next interrupt as soon as it happens. Its okay.
- The driver on receiving the buffer, passes control to irq-core which masks the interrupt by calling driver to set the trigger type to NONE.
- The device masks the interrupt and discards any latched value.
- The irq-core handles the interrupt by calling client's handler.
- The irq-core unmasks the interrupt by calling the driver to set the trigger type to LEVEL.
- The device unmasks the interrupt and latches any new value from here on.
- The driver queues the buffer again.

How does that sound ? This removes any logical dependency between availability of the buffer and the interrupt being masked or unmasked.

I think the last sentence here is somewhat ambiguous, I'm not sure what it actually
means to unmask a level triggered line if the driver has not queued an event request
for that line.

Quoted 3 lines · Arnd Bergmann

I think the last sentence here is somewhat ambiguous, I'm not sure what it actually means to unmask a level triggered line if the driver has not queued an event request for that line.

I think I now understand what you meant by this, I wasn't thinking of fasteoi earlier :)

What about following diff over this patch:

The device MUST mask the interrupt for a GPIO line, if the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt

+\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the

virtio-gpio.tex +3 -4

+    \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 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 interrupt is fully configured at this point.
 
 \item The device, on sensing an interrupt on a GPIO line, finds the matching

Quoted 11 lines · Viresh Kumar

Quoted 3 lines · Arnd Bergmann

I think the last sentence here is somewhat ambiguous, I'm not sure what it actually means to unmask a level triggered line if the driver has not queued an event request for that line.

I think I now understand what you meant by this, I wasn't thinking of fasteoi earlier :)

What about following diff over this patch:

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

Quoted 7 lines

The device MUST mask the interrupt for a GPIO line, if the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt

virtio-gpio.tex +3 -2

-status associated with the line. The device MUST unmask the interrupt for any
-other value of the trigger type.
+event associated with the line. The device MUST NOT notify the driver of an
+interrupt event if the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}
+for the GPIO line.

I actually thought that setting VIRTIO_GPIO_IRQ_TYPE_NONE would return the buffer to the driver, with status indicating that no event has happened.

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

Quoted 2 lines

virtio-gpio.tex +2 -0

+For level trigger type, the device only reports an interrupt if the
+corresponding interrupt level was sensed when the buffers are received.

I think you mean the right thing, but this part is very misleading: we clearly want
to be notified if the line was active when the buffer is received and also as soon as it becomes active afterwards, just not when it was active in the past.

The rest looks good to me now, but it's a little hard to read in diff form. Are you able to post the driver soon? I think our understanding of how this should work has converged enough that I'd review that next.

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

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

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

Version history of the specification I sent:

V6 -> V7:
- Drop Activate/Deactivate requests and merge them with set-direction one.
- Dropped separate calls to set direction to input or output (with a value), with a single call to set-direction (input, output, or none). Note that the driver needs to call set-value before calling set-direction-out now.
- Allow multiple messages for a GPIO line to be sent together, they must be processed in order though.
- The gpio-line names aren't required to be set for all the lines now, it is optional and can be present only for a subset of lines. Provided example as well.
- Merge irq-set/mask/unmask to a single set-irq-type message.
- We have only 6 message types now instead of 11 in v6.
- Mentioned about non-atomic nature of the these messages in commit log for patch 1/2.
- Improved spec content overall.