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.

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]>

+\section{General Purpose IO Device}\label{sec:Device Types / GPIO Device}

+\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}

virtio-gpio.tex new file +12 -0

+
+GPIO device uses the following structure layout for configuration:
+
+\begin{lstlisting}
+struct virtio_gpio_config {
+    u8 name[32];
+    le16 ngpio;
+    le16 names_offset;
+    le32 names_size;
+
+    /* at offset defined by names_offset field */
+    u8 gpio_names[];

virtio-gpio.tex new file +4 -0

+#define VIRTIO_GPIO_MSG_GET_DIRECTION           0x0003
+#define VIRTIO_GPIO_MSG_SET_DIRECTION_IN        0x0004
+#define VIRTIO_GPIO_MSG_SET_DIRECTION_OUT       0x0005
+#define VIRTIO_GPIO_MSG_GET_VALUE               0x0006

+\subsubsection{Device Operation: Request}\label{sec:Device Types / GPIO Device / Device Operation / Request }

virtio-gpio.tex new file +8 -0

+
+The driver initiates this message to notify the device that one of its lines has
+been assigned for use.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline

Quoted 1 line

+\section{General Purpose IO Device}\label{sec:Device Types / GPIO Device}

Nitpicking: If you expand GPIO, I would write it as "General Purpose Input/Output"
or maybe "General Purpose I/O", but not just "General Purpose IO".

Quoted 20 lines

+\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}

virtio-gpio.tex new file +19 -0

+
+GPIO device uses the following structure layout for configuration:
+
+\begin{lstlisting}
+struct virtio_gpio_config {
+    u8 name[32];
+    le16 ngpio;
+    le16 names_offset;
+    le32 names_size;
+
+    /* at offset defined by names_offset field */
+    u8 gpio_names[];
+};
+\end{lstlisting}
+
+All fields of this structure, except \field{gpio_names}, must always be set by
+the device and all fields are read-only for the driver.
+
+\begin{description}

virtio-gpio.tex line 47

Quoted 3 lines

+\item[\field{name}] is a zero-terminated string that represents the name of the

virtio-gpio.tex new file +2 -0

+    GPIO device. The unused bytes in the string must be initialized to zero by
+    the device.

I'm a bit confused about why you have both a combined "name" field and separate "gpio_names" fields. I not too familiar with the internals of the gpio subsystem, but I see that in the devicetree bindings, we have only optional per gpio line names, but no name for the device itself.

Even if we decide that this is needed after all, it would be helpful to explain some background here about what this name is used for, as others may have the same question.

virtio-gpio.tex line 68

Quoted 7 lines

+\item[\field{gpio_names}] field is optional for a device to implement. If this

virtio-gpio.tex new file +6 -0

+    field isn't implemented by the device, then the device must set the
+    \field{names_size} field to zero. If this field is implemented by the
+    device, then it must contain a stream of \field{ngpio} zero-terminated
+    strings, where each string represents the name of a GPIO line, present in
+    increasing order of the GPIO line numbers. The GPIO line names must
+    be unique within a GPIO Device and must not be empty string.

Also here, maybe add a sentence to explain what the gpio names are used for and what they are not used for.

virtio-gpio.tex line 112

Quoted 9 lines

virtio-gpio.tex new file +9 -0

+A request is initiated by the sender by adding a buffer of type \field{struct
+virtio_gpio_msg} to its respective virtqueue, after setting all fields of the
+message and \field{type} field with one of the message types from
+\field{VIRTIO_GPIO_MSG_*}.
+
+In response, the receiver adds a copy of the same buffer on its respective
+virtqueue, after performing a logical OR operation to the \field{type} field
+with VIRTIO_GPIO_MSG_RESPONSE mask and updating the \field{value} field based on
+message type.

So you have one virtqueue for a request and another virtqueue for the reply?

As they are always treated as a transaction, wouldn't it be easier to have a single virtqueue and make each transaction have both an input and an output buffer?

Quoted 12 lines

virtio-gpio.tex new file +12 -0

+#define VIRTIO_GPIO_MSG_GET_DIRECTION           0x0003
+#define VIRTIO_GPIO_MSG_SET_DIRECTION_IN        0x0004
+#define VIRTIO_GPIO_MSG_SET_DIRECTION_OUT       0x0005
+#define VIRTIO_GPIO_MSG_GET_VALUE               0x0006
+#define VIRTIO_GPIO_MSG_SET_VALUE               0x0007
+
+/* GPIO response mask, to be Or'ed with one of the above */
+#define VIRTIO_GPIO_MSG_RESPONSE                    0x8000
+\end{lstlisting}
+
+The response messages may contain error codes (in the \field{value} field) on
+failures, they must be read as negative POSIX errno codes, unless stated

virtio-gpio.tex line 134

Quoted 2 lines

virtio-gpio.tex new file +2 -0

+otherwise, i.e. 0 as success, and negative value as POSIX error code, positive
+values as invalid, unless stated otherwise.

I don't understand how POSIX errno codes are meant to be used here, as I'm not aware of any generic definition of binary values. If you mean the Linux errno values, those cannot be used here as they are certainly architecture specific. You have to enumerate the possible response codes that the device is allowed to return, and what the meaning behind those is.

virtio-gpio.tex line 154

Quoted 4 lines

+\subsubsection{Device Operation: Free}\label{sec:Device Types / GPIO Device / Device Operation / Free }

virtio-gpio.tex new file +3 -0

+
+The driver initiates this message to notify the device that a previously
+requested line has been unassigned and can be deactivated.

How is this intended to be used? Why is this not up to the guest kernel to keep track
of? I would expect that if a gpio line is listed in the information that the driver is free
to use it at any time without having to request it first.

While we do have the notion of gpiod_get/gpiod_put in the in-kernel interface, this is only used for arbitration between drivers (or driver vs user space) normally,
but not communicated with gpio controllers as far as I know.

Quoted 5 lines · Arnd Bergmann

Quoted 5 lines

virtio-gpio.tex new file +5 -0

+
+All fields of this structure, except \field{gpio_names}, must always be set by
+the device and all fields are read-only for the driver.
+
+\begin{description}

Quoted 8 lines

virtio-gpio.tex line 47

Quoted 3 lines

+\item[\field{name}] is a zero-terminated string that represents the name of the

virtio-gpio.tex new file +2 -0

+    GPIO device. The unused bytes in the string must be initialized to zero by
+    the device.

I'm a bit confused about why you have both a combined "name" field and separate "gpio_names" fields. I not too familiar with the internals of the gpio subsystem, but I see that in the devicetree bindings, we have only optional per gpio line names, but no name for the device itself.

The combined name is for distinguishing the GPIO controller at the guest, which is normally done in the kernel by assigning value from dev_name(dev).

With virtio-mmio nodes assigned randomly by hypervisors (like Qemu), those names may not be very useful. And so I thought that this name can be useful.

This eventually would get into gpio_chip->label field at kernel.

Quoted 8 lines

Quoted 7 lines

+\item[\field{gpio_names}] field is optional for a device to implement. If this

virtio-gpio.tex new file +6 -0

+    field isn't implemented by the device, then the device must set the
+    \field{names_size} field to zero. If this field is implemented by the
+    device, then it must contain a stream of \field{ngpio} zero-terminated
+    strings, where each string represents the name of a GPIO line, present in
+    increasing order of the GPIO line numbers. The GPIO line names must
+    be unique within a GPIO Device and must not be empty string.

Quoted 2 lines

Also here, maybe add a sentence to explain what the gpio names are used for and what they are not used for.

Okay, will do. Though this field is going away now (after Michael's feedback) and a request will take its place.

Quoted 11 lines

virtio-gpio.tex line 112

Quoted 9 lines

virtio-gpio.tex new file +9 -0

+A request is initiated by the sender by adding a buffer of type \field{struct
+virtio_gpio_msg} to its respective virtqueue, after setting all fields of the
+message and \field{type} field with one of the message types from
+\field{VIRTIO_GPIO_MSG_*}.
+
+In response, the receiver adds a copy of the same buffer on its respective
+virtqueue, after performing a logical OR operation to the \field{type} field
+with VIRTIO_GPIO_MSG_RESPONSE mask and updating the \field{value} field based on
+message type.

So you have one virtqueue for a request and another virtqueue for the reply?

Yes, or I will say one for device to send message/response and one for driver.

Quoted 3 lines

As they are always treated as a transaction, wouldn't it be easier to have a single virtqueue and make each transaction have both an input and an output buffer?

parallel

That's what I did initially (and that's we did in I2C as well), but then I realised
that it may not work well with transactions done in parallel.

For example, we want to allow setting direction to input for pins 5 and 8 in parallel. I at that time thought that with using a single virtqueue for both request/response messages may block the virtqueue for further usage. But now that I think about it a bit more, it may not.

We should still be able to identify different response messages based on the address of the buffer (for example) and so what you are suggesting may actually work and won't be a bottleneck as I thought of it earlier.

If this works fine, I will move back to one virtqueue for all non-irq stuff, like I had until V4 and one virtqueue only for interrupt processing.

Quoted 11 lines

virtio-gpio.tex line 154

Quoted 4 lines

+\subsubsection{Device Operation: Free}\label{sec:Device Types / GPIO Device / Device Operation / Free }

virtio-gpio.tex new file +3 -0

+
+The driver initiates this message to notify the device that a previously
+requested line has been unassigned and can be deactivated.

How is this intended to be used? Why is this not up to the guest kernel to keep track
of? I would expect that if a gpio line is listed in the information that the driver is free
to use it at any time without having to request it first.

Quoted 4 lines

While we do have the notion of gpiod_get/gpiod_put in the in-kernel interface, this is only used for arbitration between drivers (or driver vs user space) normally,
but not communicated with gpio controllers as far as I know.

I looked at kernel implementation of GPIO drivers and few of them have implemented request/free callbacks and they do read/write to hardware registers to do some controller specific stuff.

Moreover, the host may want to arbitrate GPIO pins between guests for example and would need something like this to make it work.

And so I thought the specification should always allow it.

I still don't quite get it. Why would the guest care about a stable name?

Shouldn't all users of the gpio node be handled through phandle references and similar?

Right. Any thoughts about just having the names be part of the metadata in the DT? I think that sounds easier than the request, though both methods are fairly straightforward really.

Ah, so maybe it's more like what I was looking for then after all, rather than what I understood you do.

To clarify: the flow as I now understand it is

- driver wants something (configuration, gpio read, gpio write, irq ack, ...):
driver sends a message with transmit and receive buffer on the first queue and gets a reply back immediately

- device wants something (only an irq):
driver sets up a message to receive a buffer for a line, device fills it when the event happens.

That should not be an issue. I think the reply would always be instantaneous here, and the requests can just be processed in order.

Do you have an example for what the hardware driver would do here?

Maybe LinusW can clarify whether he thinks it is needed.

Even if the hardware requires the request/free style calls in case of passing through a line to a guest, I would expect that can be handled in host user space, i.e. the virtio device code calls 'request' before it passes down a gpio line to a guest.

Quoted 1 line · Arnd Bergmann

I still don't quite get it. Why would the guest care about a stable name?

For Linux userspace, debugging mostly, at guest userspace. Like what you get out of /sys/class/gpio/gpiochipN/label or in debugfs.

i.e. to easily identify the chip there since device names aren't fixed otherwise.

Though this was initially added by Enrico and I never though of removing it. If you still think it is a waste, will get rid of it.

Quoted 3 lines

Right. Any thoughts about just having the names be part of the metadata in the DT? I think that sounds easier than the request, though both methods are fairly straightforward really.

I think we shouldn't divide stuff in between spec and DT as the spec can be used without DT as well. And so keeping them entirely in the spec may be better. The same holds true otherwise for number of gpio lines as well, or the entire config structure.

Quoted 2 lines

That should not be an issue. I think the reply would always be instantaneous here, and the requests can just be processed in order.

They aren't required to be in order after all. Lets say 3 different requests are added by 3 different threads in guest, they can all reach in random order at the host and the host can send response to them in any order. At least in Linux, you can attach a void *data with each transfer and we can easily use that to identify the transfer which completed.

Quoted 4 lines

Even if the hardware requires the request/free style calls in case of passing through a line to a guest, I would expect that can be handled in host user space, i.e. the virtio device code calls 'request' before it passes down a gpio line to a guest.

That may not always work and this count of refcounting at the backend daemon may not be correct.

So, the guest calls set-direction-out-with-val-1, the backend activates the GPIO pin, sets direction/value and then deactivates it ? That won't work, right ?

On those lines, I think these operations must be renamed to ACTIVATE/DEACTIVATE instead of REQUEST/FREE, that is what we want to do here.

I still don't see it as an important enough part of the interface to be encoded in the virtio spec. In the most likely use cases I can think of, you would never need more than one gpio controller in a virtio machine, since you can have an arbitrary number of lines connected to it already.

This also simplifies the host side, as it avoids requiring the host to configure a name that ends up being the same almost always.

If Linus or Bartosz think we should have the name, leave it in there, otherwise I'd remove it.

Encoding the number of gpio lines in the config registers makes sense to me, but I don't see how you can sensibly get away without some other structure to describe what the lines are. It doesn't have to be DT, but that's what we use in practice.

Quoted 8 lines

That may not always work and this count of refcounting at the backend daemon may not be correct.

So, the guest calls set-direction-out-with-val-1, the backend activates the GPIO pin, sets direction/value and then deactivates it ? That won't work, right ?

On those lines, I think these operations must be renamed to ACTIVATE/DEACTIVATE instead of REQUEST/FREE, that is what we want to do here.

From the drivers you pointed to,

it seems to be something like a 'disconnected'
state where the pin is neither input nor output, so this could perhaps get reflected
in the interface as well, by having each pin be one of IN/OUT/NONE. Not sure if that is any better.

Quoted 11 lines · Viresh Kumar

Quoted 1 line · Arnd Bergmann

I still don't quite get it. Why would the guest care about a stable name?

For Linux userspace, debugging mostly, at guest userspace. Like what you get out of /sys/class/gpio/gpiochipN/label or in debugfs.

i.e. to easily identify the chip there since device names aren't fixed otherwise.

Though this was initially added by Enrico and I never though of removing it. If you still think it is a waste, will get rid of it.

I still don't see it as an important enough part of the interface to be encoded in the virtio spec. In the most likely use cases I can think of, you would never need more than one gpio controller in a virtio machine, since you can have an arbitrary number of lines connected to it already.

This also simplifies the host side, as it avoids requiring the host to configure a name that ends up being the same almost always.

Quoted 3 lines · Arnd Bergmann

As they are always treated as a transaction, wouldn't it be easier to have a single virtqueue and make each transaction have both an input and an output buffer?

parallel

That's what I did initially (and that's we did in I2C as well), but then I realised
that it may not work well with transactions done in parallel.

For example, we want to allow setting direction to input for pins 5 and 8 in parallel. I at that time thought that with using a single virtqueue for both request/response messages may block the virtqueue for further usage. But now that I think about it a bit more, it may not.

We should still be able to identify different response messages based on the address of the buffer (for example) and so what you are suggesting may actually work and won't be a bottleneck as I thought of it earlier.

If this works fine, I will move back to one virtqueue for all non-irq stuff, like I
had until V4 and one virtqueue only for interrupt processing.

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

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]>

virtio-gpio.tex new file +27 -0

@@ -0,0 +1,317 @@
+\section{General Purpose IO Device}\label{sec:Device Types / GPIO Device}
+
+The Virtio GPIO device is a virtual general purpose IO device that supports a
+variable number of named IO 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] txq (driver to device)
+\item[1] rxq (device to driver)
+\end{description}
+
+The \field{txq} virtqueue is used by the driver to send messages to the device
+and the \field{rxq} virtqueue is used by the device to send messages to the
+driver.
+
+\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 structure layout for configuration:

virtio-gpio.tex new file +23 -0

+};
+\end{lstlisting}
+
+All fields of this structure, except \field{gpio_names}, must always be set by
+the device and all fields are read-only for the driver.
+
+\begin{description}
+\item[\field{name}] is a zero-terminated string that represents the name of the
+    GPIO device. The unused bytes in the string must be initialized to zero by
+    the device.
+
+\item[\field{ngpio}] is the total number of GPIO lines supported by the device.
+
+\item[\field{names_offset}] is the offset of the \field{gpio_names} field from
+    the base address of the \field{struct virtio_gpio_config}. The device must
+    set its value to 40 for current version of the specification. In future this
+    value may change if the \field {struct virtio_gpio_config} is expanded by
+    adding more fields to it.
+
+\item[\field{names_size}] is the size of the \field{gpio_names} memory block.
+    The device must set it to the size, in bytes, of the \field{gpio_names}
+    field. The device must set this to zero, if the \field{gpio_names} field
+    isn't implemented by the device.

Is this the standard (virtio) way to define fields like this? Personally, I would write "__gpio_names[]", to make it more obvious if some code tries to access the field directly.
Of course the real implementation should provide a macro/function to access the names in a safe way.

+\begin{lstlisting}

virtio-gpio.tex new file +10 -0

+struct virtio_gpio_config {
+    u8 name[32];
+    le16 ngpio;
+    le16 names_offset;
+    le32 names_size;
+
+    /* at offset defined by names_offset field */
+    u8 gpio_names[];
+};
+\end{lstlisting}

Quoted 17 lines · Viresh Kumar

+\begin{lstlisting}

virtio-gpio.tex new file +16 -0

+struct virtio_gpio_config {
+    u8 name[32];
+    le16 ngpio;
+    le16 names_offset;
+    le32 names_size;
+
+    /* at offset defined by names_offset field */
+    u8 gpio_names[];
+};
+\end{lstlisting}
+
+\item[\field{gpio_names}] field is optional for a device to implement. If this
+    field isn't implemented by the device, then the device must set the
+    \field{names_size} field to zero. If this field is implemented by the
+    device, then it must contain a stream of \field{ngpio} zero-terminated
+    strings, where each string represents the name of a GPIO line, present in

virtio-gpio.tex line 70

Quoted 4 lines

virtio-gpio.tex new file +4 -0

+    increasing order of the GPIO line numbers. The GPIO line names must
+    be unique within a GPIO Device and must not be empty string.
+
+\end{description}

There's a problem with this approach: config space size is actually pretty limited since it might be in the IO space when using PCI. That is turn is recommended not to exceed 256 bytes.

Is it important to have the names accessible before driver is fully initialized?

If not I would just create a VQ and use that to retrieve this data.

Note: the competing virtio-gpio proposal on list shares the issue.

+\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }

virtio-gpio.tex +8 -0

+
+The device initiates this message to notify the driver of an IRQ event on a line
+previously configured for interrupt.
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
+supported by the device.
+
+This is the only message which is initiated by the device and not the driver.

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

virtio-gpio.tex +17 -0

+
+The \field{VIRTIO_GPIO_MSG_IRQ_EVENT} message type can only be initiated by the
+device and sent to the driver over the \field{rxq} virtqueue.
+
+\begin{itemize}
+\item The device, on sensing an interrupt for a GPIO line, prepares a buffer of
+      type \field{struct virtio_gpio_msg} and sets its \field{type} to
+      \field{VIRTIO_GPIO_MSG_IRQ_EVENT}, \field{gpio} field with a GPIO line
+      number, and \field{value} field to 0.
+
+\item The device sends the buffer to the driver over the \field{rxq} virtqueue.
+
+\item The device MUST NOT initiate another interrupt request for the same GPIO
+      line before receiving a response from the driver.
+
+\item The driver, after receiving the buffer from the driver, handles the
+      interrupt.

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

Quoted 8 lines · Viresh Kumar

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

virtio-gpio.tex +7 -0

+
+The driver initiates 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
+supported by the device.

Is there a way for the driver to know which trigger types are supported on a given line? If not, would that be useful, or do we assume that this knowledge exists in the place that sets the trigger type based on e.g. device tree data?

Quoted 13 lines

+\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }

virtio-gpio.tex +12 -0

+
+The device initiates this message to notify the driver of an IRQ event on a line
+previously configured for interrupt.
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
+supported by the device.
+
+This is the only message which is initiated by the device and not the driver.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\

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

Quoted 6 lines

+\hline

virtio-gpio.tex +5 -0

+Message & \field{VIRTIO_GPIO_MSG_IRQ_EVENT} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_IRQ_EVENT} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}

Can you clarify what "initiated by the device" means here?

Do you mean
the driver has to pre-fill the rxq with message buffers and the device picks one of them?

In this case I would not call it "initiated by
the device", unless that terminology is what the virtio spec calls the same thing elsewhere.

virtio-gpio.tex \subsubsection{Driver Requests}\label{sec:Device Types / GPIO Device / Message F · line 454

Quoted 4 lines

+\item The driver prepares a buffer of type \field{struct virtio_gpio_msg} and

virtio-gpio.tex +3 -0

+      sets its \field{type} field to (\field{VIRTIO_GPIO_MSG_IRQ_EVENT} |
+      \field{VIRTIO_GPIO_MSG_RESPONSE}), \field{gpio} field with \field{gpio} of
+      the received buffer, and \field{value} to message defined value.

Ah, ok. So the driver does have to fill in a request for an irq event on this particular line first for the rxq, not just a generic "send me something" request.

I think that makes sense, but I find the way you describe it confusing, so this should be explained a little better in the "Device Operation: IRQ Event" subsubsection, even if you end up repeating yourself here.

I'm missing a bit here to explain how edge vs level interrupts are handled. In particular:

- for an edge interrupt, what happens when a new event comes between the VIRTIO_GPIO_MSG_IRQ_EVENT message and the new buffer getting queued? Will the device send another irq-event message, does it not send one, or is this implementation defined?

- for level triggered interrupts, how does the driver know that the event response has been received by the device? Does this not require a more complicated handshake?

Quoted 14 lines · Arnd Bergmann

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

Quoted 8 lines · Viresh Kumar

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

virtio-gpio.tex +7 -0

+
+The driver initiates 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
+supported by the device.

Is there a way for the driver to know which trigger types are supported on a given line? If not, would that be useful, or do we assume that this knowledge exists in the place that sets the trigger type based on e.g. device tree data?

I think it would be better to not send data partially via DT, as it may not be available for all the use cases.

So, if something is needed, then it should be made available over the protocol itself.

I assumed that we can take it for granted that all trigger types are supported, but maybe not.

FWIW, I also took reference from another GPIO protocol developed for greybus:

https://github.com/projectara/greybus-spec/blob/master/source/bridged_phy.rst#gpio-protocol

and it also assumed all interrupt types would be supported.

I am fine with adding a field for that in the configuration structure, if you think it would make sense.

Quoted 13 lines

Quoted 13 lines

+\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }

virtio-gpio.tex +12 -0

+
+The device initiates this message to notify the driver of an IRQ event on a line
+previously configured for interrupt.
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
+supported by the device.
+
+This is the only message which is initiated by the device and not the driver.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\

Quoted 8 lines

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

Quoted 6 lines

+\hline

virtio-gpio.tex +5 -0

+Message & \field{VIRTIO_GPIO_MSG_IRQ_EVENT} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_IRQ_EVENT} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}

Can you clarify what "initiated by the device" means here?

I meant the initial message is sent by the device (host) and the driver (guest) responds with success/failure.

Quoted 3 lines

Do you mean
the driver has to pre-fill the rxq with message buffers and the device picks one of them?

This is how it works technically I think. AFAIU, the driver needs to keep the buffers filled (even for the responses to the messages initiated at the driver).

Quoted 3 lines

In this case I would not call it "initiated by
the device", unless that terminology is what the virtio spec calls the same thing elsewhere.

I am not sure what that should be called as either, maybe virtio-spec maintainers can share the preferred way.

Cornelia, Michael?

Quoted 4 lines

virtio-gpio.tex \subsubsection{Driver Requests}\label{sec:Device Types / GPIO Device / Message F · line 454

Quoted 4 lines

+\item The driver prepares a buffer of type \field{struct virtio_gpio_msg} and

virtio-gpio.tex +3 -0

+      sets its \field{type} field to (\field{VIRTIO_GPIO_MSG_IRQ_EVENT} |
+      \field{VIRTIO_GPIO_MSG_RESPONSE}), \field{gpio} field with \field{gpio} of
+      the received buffer, and \field{value} to message defined value.

This is the response message that the driver sends on receiving an interrupt, this isn't pre-filled and is sent on txq instead of rxq.

Quoted 3 lines

Ah, ok. So the driver does have to fill in a request for an irq event on this particular line first for the rxq, not just a generic "send me something" request.

Yes, and the driver also needs to add buffers here for receiving response to the messages initiated by the driver.

Quoted 7 lines

I'm missing a bit here to explain how edge vs level interrupts are handled. In particular:

- for an edge interrupt, what happens when a new event comes between the VIRTIO_GPIO_MSG_IRQ_EVENT message and the new buffer getting queued? Will the device send another irq-event message, does it not send one, or is this implementation defined?

The device MUST not send another irq request for the same GPIO line, until the time it has received a response from the driver. Once the response has arrived at the device, it can raise the interrupt again if the level interrupt is still active.

We should see it like how interrupts are handled in kernel, once an interrupt comes we disable it until the time the handler has got a chance to run, and the handler here is at the driver.

Quoted 3 lines

- for level triggered interrupts, how does the driver know that the event response has been received by the device? Does this not require a more complicated handshake?

Okay, maybe there is something I need to improve here if this isn't clear..

Every virtio-gpio transfer has two parts, a message is sent (maybe I should call it Request instead as I did earlier) by the sender, and receiver sends a response after processing the message.

The device (host) is sender for the irq-message, and it will wait until a response
is received from the driver (guest) for the same. That can be seen as returning from an irq-handler in kernel for example, at which point interrupt gets enabled again.

Was I able to clarify it ?

Quoted 6 lines · Viresh Kumar

Quoted 4 lines · Arnd Bergmann

Is there a way for the driver to know which trigger types are supported on a given line? If not, would that be useful, or do we assume that this knowledge exists in the place that sets the trigger type based on e.g. device tree data?

What I meant is that the guest OS kernel would only ever set a trigger type that makes sense based on what the device on the other end of the GPIO line needs, and this would be encoded in the interrupt-cells.

Ok, let's see what they think. I would have found this less confusing if you just describe what message the driver sends and how the device responds to it.

Quoted 2 lines

Yes, and the driver also needs to add buffers here for receiving response to the messages initiated by the driver.

Ok, I think that needs to be made clearer here. I'm still not sure I have understood exactly what messages you

Quoted 4 lines

The device MUST not send another irq request for the same GPIO line, until the time it has received a response from the driver. Once the response has arrived at the device, it can raise the interrupt again if the level interrupt is still active.

The part about not sending another interrupt is the easy part. What I mean is what happens if another edge event comes into the gpio controller before the driver responds. Does the device immediately send another interrupt message or does it skip that?

I'm not sure it's helpful to describe the protocol as symmetric when neither the virtqueue below it nor the idea of a gpio controller is symmetric.

I think a better way to describe it would be request/response pairs where the driver asks for something to be done and it gets an immediate response, as opposed to long-running event notifications where the driver queues up a descriptor and it only completes when an interrupt happens.

Quoted 9 lines · Arnd Bergmann

Quoted 4 lines

The device MUST not send another irq request for the same GPIO line, until the time it has received a response from the driver. Once the response has arrived at the device, it can raise the interrupt again if the level interrupt is still active.

The part about not sending another interrupt is the easy part. What I mean is what happens if another edge event comes into the gpio controller before the driver responds. Does the device immediately send another interrupt message or does it skip that?

Hmm, what should it do in that case ? What do we do in kernel for this ? I think maybe we should accumulate the interrupts in that period of time and send only the last one ?

Also will it make sense to send the trigger-type information to the driver on an interrupt? For example a user may have programmed a line for both edge trigger interrupts. Should we send info about which edge it is, rising or falling ?

Quoted 7 lines

I'm not sure it's helpful to describe the protocol as symmetric when neither the virtqueue below it nor the idea of a gpio controller is symmetric.

I think a better way to describe it would be request/response pairs where the driver asks for something to be done and it gets an immediate response, as opposed to long-running event notifications where the driver queues up a descriptor and it only completes when an interrupt happens.

Yes, I think it would be better to bind the request/response messages together. I will see if that will work fine or not before confirming.

It's important to get the order correct here, and there are a lot of variations in hardware, so we need to pick the one that works best. Unfortunately I can never quite remember what they are either ;-)

Looking at the two ways of handling irqs that we care about

1. edge interrupts:

void handle_edge_irq(struct irq_desc *desc)
{
...

        desc->irq_data.chip->irq_ack(&desc->irq_data);
        do {
              ...
              handle_irq_event(desc);
        } while ((desc->istate & IRQS_PENDING) &&
                 !irqd_irq_disabled(&desc->irq_data));
       ...
}

Here the irq gets rearmed first and then we call into the driver that has requested it. That driver processes all information that is associated with the event, or possibly multiple events that have happened since the handler waslast called. If another irq arrives after the driver is done looking for them, we will receive another event from the virtio device and all is good.

Ideally the 'irq_ack' would simply involve queuing another request for an event on the second virtqueue. However I don't know if there is a way for the virtio-gpio driver to know whether this request has been observed by the virtio-gpio device.

If not, the irq_ack may arrive at the device only after the handle_irq_event() function is complete and we miss an interrupt.

2. level interrupts:

void handle_level_irq(struct irq_desc *desc)

{
        mask_ack_irq(desc);
        ...
        handle_irq_event(desc);
        ...
        cond_unmask_irq(desc);
}

Going through this the literal way would mean sending a mask,

ack, and unmask request through virtio and waiting for a reply each time, but that does seem excessive.

As long as the interrupt is masked initially (since there is no event request pending immediately after the irq event reply),

I would hope that we can get away with simply enqueuing the request in the 'cond_unmask_irq' step. In this case, we would call all handlers for this the level irq with the line pending and masked. After the handlers are complete, it should no longer be pending and we can unmask it. If another event comes in after the handler, it gets pending again, and we get sent a new irq event reply after the Ack.

Quoted 27 lines · Arnd Bergmann

It's important to get the order correct here, and there are a lot of variations in hardware, so we need to pick the one that works best. Unfortunately I can never quite remember what they are either ;-)

Looking at the two ways of handling irqs that we care about

1. edge interrupts:

void handle_edge_irq(struct irq_desc *desc)
{
...

        desc->irq_data.chip->irq_ack(&desc->irq_data);
        do {
              ...
              handle_irq_event(desc);
        } while ((desc->istate & IRQS_PENDING) &&
                 !irqd_irq_disabled(&desc->irq_data));
       ...
}

Here the irq gets rearmed first and then we call into the driver that has requested it. That driver processes all information that is associated with the event, or possibly multiple events that have happened since the handler waslast called. If another irq arrives after the driver is done looking for them, we will receive another event from the virtio device and all is good.

Quoted 4 lines

Ideally the 'irq_ack' would simply involve queuing another request for an event on the second virtqueue. However I don't know if there is a way for the virtio-gpio driver to know whether this request has been observed by the virtio-gpio device.

The driver will send an event (virtqueue_kick()) after the buffer is queued, so we can just assume that device has been notified and it has seen it.

Quoted 14 lines

If not, the irq_ack may arrive at the device only after the handle_irq_event() function is complete and we miss an interrupt.

2. level interrupts:

void handle_level_irq(struct irq_desc *desc)

{
        mask_ack_irq(desc);
        ...
        handle_irq_event(desc);
        ...
        cond_unmask_irq(desc);
}

Quoted 1 line

Going through this the literal way would mean sending a mask,

One important think worth mentioning here is that the mask/unmask work here will be done when irq_bus_sync_unlock() is called as this is a slow bus and we can't do virtio-messages on mask/unmask, irq-context I believe.

Quoted 5 lines

ack, and unmask request through virtio and waiting for a reply each time, but that does seem excessive.

As long as the interrupt is masked initially (since there is no event request pending immediately after the irq event reply),

What irq event reply ?

That's what I was hoping for, I'm just not sure if this is required by the virtio spec or just the usual implementation of the device side. Have you found a section in the spec that guarantees that the virtqueue_kick() serves as a sufficient barrier that the driver can rely on the information having arrived?

Ok, in that case I guess the requeue has to be deferred, which may rule out everything other than the fasteoi method.

I meant the notification from device to driver that an event has happened.

Quoted 5 lines · Arnd Bergmann

That's what I was hoping for, I'm just not sure if this is required by the virtio spec or just the usual implementation of the device side. Have you found a section in the spec that guarantees that the virtqueue_kick() serves as a sufficient barrier that the driver can rely on the information having arrived?

This is what the spec say about it:

https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-9000023

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

Quoted 2 lines · Viresh Kumar

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

Michael (and other virtio experts),

I have a doubt about interrupt messages initiated by the device. There is nothing meaningful I want to return from the driver to the device on an interrupt event, but I need to make sure the device doesn't send another interrupt for the same GPIO line, before the previous one is serviced. I was thinking about sending some sort of response message for it, but am not sure how to implement it.

Another way out to implement this, is by adding to the spec that on interrupt the device MUST mask the interrupt line on the GPIO before sending the event to the driver and this irq can be re-enabled only by the driver sending another request over the primary virtqueue (and not interrupt virtqueue).

What do you suggest ?

I would still hope that we can simplify this to the 'Ack' being implied by requeuing the message from the gpio driver.

In case of a level interrupt:

device driver
1. queue message
2. line activates
3. send reply
4. notify guest
5. call handler
6. line may activate
7. goto 1

For edge interrupts, I'm still not sure how it would work. The options that I see are:

a) fasteoi style controller: when the device sends an event, this becomes implicitly masked as there is no way to send another until the message is requeued, but the device latches any further events, so that queuing the next message after the guest handler returns immediately results in the event getting delivered. This would use the minimum number of requests and let the driver use the exact same code for edge and level mode, but it does mean the possibility of extra wakeups, and it may require more work in the host.

b) require the requeue to happen in the guest before calling the handler to prevent missed events. Not sure if this is possible without another message, as the guest must be sure that the host has observed the requeue, but it cannot have returned any data yet.

c) explicit ack at start of guest: driver starts by sending an ack to the first virtqueue and waiting for it to be complete, then calls the handler, and only then requeues the request. This would presumably add a lot of extra latency.

Quoted 2 lines · Arnd Bergmann

I would still hope that we can simplify this to the 'Ack' being implied by requeuing the message from the gpio driver.

Which would mean that we process one interrupt at a time, I was hoping to do some sort parallelization here by allowing interrupts for multiple GPIO lines to be processed together.

Another way of doing that would be sending a mask of all GPIO pins where interrupt is pending on this irq request. That would require a separate bit for each GPIO pin, i.e. 8 32-bit values for 256 GPIO pins. Which would also require to change the size of ngpio field in the config structure to u8 instead of u16. I am not sure why it should be u16 really (Enrico had it this way), it sounds really big. Will we ever need anything over 256? And why not add another device in that case.

Quoted 5 lines

b) require the requeue to happen in the guest before calling the handler to prevent missed events. Not sure if this is possible without another message, as the guest must be sure that the host has observed the requeue, but it cannot have returned any data yet.

The driver does call virtqueue_kick() there, so an event must go to the device. Maybe that can be seen as the device has observed the event.

I don't think you can have a message for more than one line here, that would mess up the synchronization as then you have to keep double accounting of which gpios are already pending on each side.

The most sensible way I see this can be done is to have a message per line, and have it as a token that is either on the device or the driver side at any point. When the driver has sent the token over to the device, the irq is armed, and when the device replies, it is implicitly masked.

Ideally this could even replace the VIRTIO_GPIO_MSG_IRQ_TYPE, VIRTIO_GPIO_MSG_IRQ_MASK

and VIRTIO_GPIO_MSG_IRQ_UNMASK

messages with a single message type on the interrupt virtqueue:

struct virtio_gpio_irq_event {
      __le16 line;
      __u8 type;
      __u8 status;
}

When the driver wants an event on a gpio line, it enqueues this message to the virtqueue, and then the device either adds the corresponding file descriptor to its poll table, or replies immediately with one of these status words:

- request not supported
- level interrupt is still active (line remains at requested level)
- edge interrupt has happened since last reply

Quoted 4 lines · Arnd Bergmann

I don't think you can have a message for more than one line here, that would mess up the synchronization as then you have to keep double accounting of which gpios are already pending on each side.

Yes.

Quoted 5 lines

The most sensible way I see this can be done is to have a message per line, and have it as a token that is either on the device or the driver side at any point. When the driver has sent the token over to the device, the irq is armed, and when the device replies, it is implicitly masked.

I like the idea in general but at this point I am not very sure how it will work. Looking at kernel (and qemu) side implementations, what really transfers between device and driver is a series of buffers (marked as in or out sgs). Now these are either device writable or driver. I am not sure if the device is required to write anything here to the buffer, it can simply return the buffer as is and so we can make it driver writable only, i.e. only 1 sg element required.

Quoted 1 line

and VIRTIO_GPIO_MSG_IRQ_UNMASK

I think we would still require some kind of unmasking here as the driver would have already sent the buffer to the device and wants the ownership back, otherwise it may hit an interrupt later on.

Although I would like to keep the three routines separate, just to be robust here. For example what will we do if the user does this (yes it won't do it, but I am trying to handle the corner cases well):

1. irq-type-edge-falling
2. irq-unmask
3. irq-type-edge-both

Since we would have already sent the buffer at (2), we can't do (3) without getting the buffer back.

Tying everything with availability of the buffer itself makes it kind of messy.

I think it can be done either way:

a) if you have two buffers, the driver asks for an irq event, and the driver replies with a filled-out event saying what happened, or that it could not do it.

b) with just one buffer, we know that something happened, but now the driver has to ensure that the event request is valid before queuing it. If the driver asks for an event on a gpio line that is not an irq source, or configured as output, or has a mode that the device cannot support, the event would either have to be returned immediately or never.

I think we can live with getting a spurious interrupt in case the driver reconfigures the line later on to no longer be an interrupt source.

Right, this case would be easier for the guest if we separate setting the irq polarity from the action of asking for an event, but I suspect it would make the device implementation trickier in the process (which you may not care about as much).

An easy way to deal with this scenario would be to mandate that any VIRTIO_GPIO_MSG_SET_* command forces the irq event message to trigger and have to be requeued.

Quoted 4 lines · Arnd Bergmann

I don't think you can have a message for more than one line here, that would mess up the synchronization as then you have to keep double accounting of which gpios are already pending on each side.

Yes.

Quoted 5 lines

The most sensible way I see this can be done is to have a message per line, and have it as a token that is either on the device or the driver side at any point. When the driver has sent the token over to the device, the irq is armed, and when the device replies, it is implicitly masked.

I like the idea in general but at this point I am not very sure how it will work. Looking at kernel (and qemu) side implementations, what really transfers between device and driver is a series of buffers (marked as in or out sgs). Now these are either device writable or driver. I am not sure if the device is required to write anything here to the buffer, it can simply return the buffer as is and so we can make it driver writable only, i.e. only 1 sg element required.

Quoted 2 lines

Ideally this could even replace the VIRTIO_GPIO_MSG_IRQ_TYPE, VIRTIO_GPIO_MSG_IRQ_MASK

Maybe yes.

Quoted 1 line

and VIRTIO_GPIO_MSG_IRQ_UNMASK

I think we would still require some kind of unmasking here as the driver would have already sent the buffer to the device and wants the ownership back, otherwise it may hit an interrupt later on.

Although I would like to keep the three routines separate, just to be robust here. For example what will we do if the user does this (yes it won't do it, but I am trying to handle the corner cases well):

1. irq-type-edge-falling
2. irq-unmask
3. irq-type-edge-both

Since we would have already sent the buffer at (2), we can't do (3) without getting the buffer back.

Tying everything with availability of the buffer itself makes it kind of messy.

Quoted 16 lines

messages with a single message type on the interrupt virtqueue:

struct virtio_gpio_irq_event {
      __le16 line;
      __u8 type;
      __u8 status;
}

When the driver wants an event on a gpio line, it enqueues this message to the virtqueue, and then the device either adds the corresponding file descriptor to its poll table, or replies immediately with one of these status words:

- request not supported
- level interrupt is still active (line remains at requested level)
- edge interrupt has happened since last reply

Signature · 1 line

viresh

Quoted 2 lines · Viresh Kumar

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

Michael (and other virtio experts),

I have a doubt about interrupt messages initiated by the device. There is nothing meaningful I want to return from the driver to the device on an interrupt event, but I need to make sure the device doesn't send another interrupt for the same GPIO line, before the previous one is serviced. I was thinking about sending some sort of response message for it, but am not sure how to implement it.

Another way out to implement this, is by adding to the spec that on interrupt the device MUST mask the interrupt line on the GPIO before sending the event to the driver and this irq can be re-enabled only by the driver sending another request over the primary virtqueue (and not interrupt virtqueue).

What do you suggest ?

Signature · 1 line

viresh

Quoted 4 lines · Viresh Kumar

Quoted 2 lines · Viresh Kumar

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

Quoted 8 lines

Michael (and other virtio experts),

I have a doubt about interrupt messages initiated by the device. There is nothing meaningful I want to return from the driver to the device on an interrupt event, but I need to make sure the device doesn't send another interrupt for the same GPIO line, before the previous one is serviced. I was thinking about sending some sort of response message for it, but am not sure how to implement it.

My suggestion is this: driver adds a buffer to tell device it wants an interrupt. device uses the buffer to notify driver of the interrupt event.

Quoted 6 lines

Another way out to implement this, is by adding to the spec that on interrupt the device MUST mask the interrupt line on the GPIO before sending the event to the driver and this irq can be re-enabled only by the driver sending another request over the primary virtqueue (and not interrupt virtqueue).

What do you suggest ?

Well vqs fundamentally can cause spurious interrupts. First suggestion is closer to how other devices do this.

Hello,

This patchset adds virtio specification for GPIO devices. It supports basic GPIO line operations, along with optional interrupts on them (in a separate patch).

This is an *alternative implementation* of the virtio-gpio specification, another version of it was earlier posted by Enrico [1].

I took back V4 of the specification I posted earlier (on June 17th), to allow Enrico to come up with something that is robust and works for everyone (as he started this thing last year), but it didn't go as expected. I couldn't see any of my review comments being incorporated (or any intentions of them getting in ever) in the two versions Enrico posted and a month passed since then.

And so I am trying to present an alternative approach here to solve the problem, which I believe is more clear and robust. I am open to suggestions to improve it further.

I will let the virtio/gpio maintainers decide on its fate.

Key differences from Enrico's approach [1]:

- config structure is rearranged to remove unnecessary fields/padding, and place gpio_names block in such a way that we can expand the structure easily going forward, if required.

- Supports freeing of a GPIO line after use.

- Interrupt implementation handled with feature bit 0. Either the interrupts are fully supported or not at all.

- Doesn't add any ordering restrictions on the device, it can respond earlier to the second request, while still processing the first one.

- Clearly state that two requests can't be initiated for the same line by device or driver.

On Fri, Jul 16, 2021 at 9:39 AM Viresh Kumar <[email protected]> wrote:
I've looked at the two proposals again now, and yours does look more mature, so I agree it makes sense to continue with your version.

Quoted 7 lines

I will let the virtio/gpio maintainers decide on its fate.

Key differences from Enrico's approach [1]:

- config structure is rearranged to remove unnecessary fields/padding, and place gpio_names block in such a way that we can expand the structure easily going forward, if required.

Simplifying is good, maybe we can cut it down even further. I now actually wonder
if we shouldn't remove the gpio names from the virtio-gpu spec entirely.

In practice I assume this driver only makes sense to be used in combination with firmware describing how it interacts with other drivers (gpio-key, gpio-led,
mmc, i2c, ...) that require access to gpio lines for something they do, and these already rely on DT information or some equivalent.

As soon as we have a DT node for the device, the names can just be passed according ot the generic DT binding for gpio controllers.

Quoted 2 lines

- Interrupt implementation handled with feature bit 0. Either the interrupts are fully supported or not at all.

The interrupt definition looks better than Enrico's version, but the serialization
still looks incomplete, especially since you added level triggered mode. I always
get confused by those as well, so we'll need to have this reviewed by an irqchip person as well, but let's start with my questions for now.

The other comments I had regarding the use of errno values, and the split between txq/rxq messages are things you inherited from Enrico's version, so those need to be resolved regardless.