[PATCH V5 0/2] virtio: Add specification for virtio-gpio

[PATCH V5 0/2] virtio: Add specification for virtio-gpio

[Viresh Kumar]

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.


Version history of the specification I sent:

V4 -> V5:
- Split into two patches, irq stuff in patch 2.
- Use separate virtqueue for sending data from device/driver.
- Allow parallel processing of requests for different GPIOs, only one request at
  a time for the same GPIO line.
- Same goes for interrupt side, only one interrupt request per GPIO line.
- Improved formatting in general.
- Add new sections explaining message flow sequence.

V3 -> V4:
- The GPIO line names must be unique within a device.
- The gpio_names[0] field is dropped and gpio_names_offset field is
  added in place of the 2 bytes of padding.
- New interrupts must not be initiated by the device, without a response
  for the previous one.

V2 -> V3:
- Unused char in name string should be marked 0 now.
- s/host/device/ and s/guest/driver/
- Added a new feature for IRQ mode, VIRTIO_GPIO_F_IRQ.
- A new feature should be added for Version information if required
  later on.

V1 -> V2:
- gpio_names_size is 32 bit.
- gpio field is 16 bit.
- padding added 16 bit.
- Added packed attribute to few structures
- Add the missing 'type' field to the request
- Dropped to _nodata request/responses to simplify a bit, updated
  related text.

--
Viresh

[1] https://lists.oasis-open.org/archives/virtio-dev/202106/msg00030.html

Viresh Kumar (2):
  virtio-gpio: Add the device specification
  virtio-gpio: Add support for interrupts

 conformance.tex |  26 ++-
 content.tex     |   3 +-
 virtio-gpio.tex | 464 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 488 insertions(+), 5 deletions(-)
 create mode 100644 virtio-gpio.tex

-- 
2.31.1.272.g89b43f80a514

[PATCH V5 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

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" <lkml@metux.net>.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 conformance.tex |  26 +++-
 content.tex     |   3 +-
 virtio-gpio.tex | 317 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 341 insertions(+), 5 deletions(-)
 create mode 100644 virtio-gpio.tex

diff --git a/conformance.tex b/conformance.tex
index 94d7a06db899..9cc2d229d801 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -30,8 +30,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Driver Conformance / IOMMU Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / Sound Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / Memory Driver Conformance},
-\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance} or
-\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance}.
+\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance},
+\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance} or
+\ref{sec:Conformance / Driver Conformance / General Purpose IO Driver Conformance}.
 
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
@@ -54,8 +55,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Device Conformance / IOMMU Device Conformance},
 \ref{sec:Conformance / Device Conformance / Sound Device Conformance},
 \ref{sec:Conformance / Device Conformance / Memory Device Conformance},
-\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance} or
-\ref{sec:Conformance / Device Conformance / SCMI Device Conformance}.
+\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance},
+\ref{sec:Conformance / Device Conformance / SCMI Device Conformance} or
+\ref{sec:Conformance / Device Conformance / General Purpose IO Device Conformance}.
 
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
@@ -301,6 +303,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Device Types / SCMI Device / Device Operation / Setting Up eventq Buffers}
 \end{itemize}
 
+\conformance{\subsection}{General Purpose IO Driver Conformance}\label{sec:Conformance / Driver Conformance / General Purpose IO Driver Conformance}
+
+An General Purpose IO driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / GPIO Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -550,6 +560,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / SCMI Device / Device Operation / Shared Memory Operation}
 \end{itemize}
 
+\conformance{\subsection}{General Purpose IO Device Conformance}\label{sec:Conformance / Device Conformance / General Purpose IO Device Conformance}
+
+An General Purpose IO device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / GPIO Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
 A conformant implementation MUST be either transitional or
 non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index 5c70a3c415a3..a8c9167b3e50 100644
--- a/content.tex
+++ b/content.tex
@@ -2876,7 +2876,7 @@ \chapter{Device Types}\label{sec:Device Types}
 \hline
 40         &   Bluetooth device \\
 \hline
-41         &   GPIO device \\
+41         &   General Purpose IO (GPIO) device \\
 \hline
 \end{tabular}
 
@@ -6583,6 +6583,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-mem.tex}
 \input{virtio-i2c.tex}
 \input{virtio-scmi.tex}
+\input{virtio-gpio.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/virtio-gpio.tex b/virtio-gpio.tex
new file mode 100644
index 000000000000..40020620d714
--- /dev/null
+++ b/virtio-gpio.tex
@@ -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:
+
+\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}
+\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.
+
+\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
+    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}
+
+
+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization}
+
+\begin{itemize}
+\item The driver MUST configure and initialize both the virtqueues.
+
+\item The driver MUST read device configuration.
+
+\item The driver MUST populate the \field{rxq} virtqueue with \field{struct
+      virtio_gpio_msg} buffer.
+\end{itemize}
+
+\subsection{Device Operation}\label{sec:Device Types / GPIO Device / Device Operation}
+
+The communication between device and driver takes place using a pair of request
+and response messages, both of which are represented by \field{struct
+virtio_gpio_msg}.
+
+\begin{lstlisting}
+struct virtio_gpio_msg {
+        le16 type;
+        le16 gpio;
+        le32 value;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{type}] is the GPIO message type.
+\item[\field{gpio}] is the GPIO line number.
+\item[\field{value}] is a message specific value.
+\end{description}
+
+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.
+
+For the current version of the specification, the sender is always the driver
+and receiver is always the device.
+
+\begin{lstlisting}
+/* GPIO message types: driver -> device */
+#define VIRTIO_GPIO_MSG_REQUEST                 0x0001
+#define VIRTIO_GPIO_MSG_FREE                    0x0002
+#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
+otherwise, i.e. 0 as success, and negative value as POSIX error code, positive
+values as invalid, unless stated otherwise.
+
+\subsubsection{Device Operation: Request}\label{sec:Device Types / GPIO Device / Device Operation / Request }
+
+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
+Message & \field{VIRTIO_GPIO_MSG_REQUEST} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_REQUEST} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: Free}\label{sec:Device Types / GPIO Device / Device Operation / Free }
+
+The driver initiates this message to notify the device that a previously
+requested line has been unassigned and can be deactivated.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_FREE} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_FREE} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: Get Direction}\label{sec:Device Types / GPIO Device / Device Operation / Get Direction }
+
+The driver initiates this message to request the device to return a line's
+configured direction.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_GET_DIRECTION} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = output, 1 = input, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: Direction In}\label{sec:Device Types / GPIO Device / Device Operation / Direction In }
+
+The driver initiates this message to request the device to configure a line for
+input.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_SET_DIRECTION_IN} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_SET_DIRECTION_IN} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: Direction Out}\label{sec:Device Types / GPIO Device / Device Operation / Direction Out }
+
+The driver initiates this message to request the device to configure a line for
+output, and set its initial output value.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+    Message & \field{VIRTIO_GPIO_MSG_SET_DIRECTION_OUT} & line number & 0 = low, 1 = high \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_SET_DIRECTION_OUT} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: Get Value}\label{sec:Device Types / GPIO Device / Device Operation / Get Value }
+
+The driver initiates this message to request the device to return current value
+sensed on a line configured for input.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_GET_VALUE} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_GET_VALUE} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = low, 1 = high, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: Set Value}\label{sec:Device Types / GPIO Device / Device Operation / Set Value }
+
+The driver initiates this message to request the device to set the value of a
+line configured for output.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_SET_VALUE} & line number & 0 = low, 1 = high \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_SET_VALUE} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
+
+\begin{itemize}
+\item The driver MUST only send messages on the \field{txq} virtqueue.
+
+\item The driver MUST set all the fields of the \field{struct virtio_gpio_msg}
+      before sending it to the device.
+
+\item The driver MUST NOT set value of a GPIO line configured for input.
+
+\item The driver MUST NOT get value of a GPIO line configured for output.
+
+\item The driver MUST NOT initiate another message for a GPIO line, before
+      response to the previously sent message is received for the same line.
+\end{itemize}
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
+
+\begin{itemize}
+\item The device MUST only send messages on the \field{rxq} virtqueue.
+
+\item The device MUST set all the fields of the \field{struct virtio_gpio_msg}
+      before sending it to the driver.
+
+\item The device MUST set all the fields of the \field{struct
+      virtio_gpio_config} on receiving a request from the driver.
+
+\item The device MUST set the \field{names_size} field as zero in the
+      \field{struct virtio_gpio_config}, if it doesn't implement the
+      \field{gpio_names} field.
+
+  \item The device MUST set the \field{names_size} field, in the \field{struct
+      virtio_gpio_config}, with the size of \field{gpio_names} memory block in
+      bytes, if it implements the \field{gpio_names} field.
+
+\item If the device implements the \field{gpio_names} field, then the strings in
+      that field MUST be zero-terminated and an unique and valid string MUST be
+      added for each supported GPIO line.
+\end{itemize}
+
+\subsection{Message flow}\label{sec:Device Types / GPIO Device / Message Flow}
+
+This section describe how the messages flow between device and driver.
+
+\subsubsection{Driver Requests}\label{sec:Device Types / GPIO Device / Message Flow / Requests}
+
+All the messages, except \field{VIRTIO_GPIO_MSG_IRQ_EVENT}, are initiated by the
+driver and sent to the device over the \field{txq} virtqueue.
+
+\begin{itemize}
+\item The driver prepares a buffer of type \field{struct virtio_gpio_msg} and
+      sets its \field{type} to one of the message types from
+      \field{VIRTIO_GPIO_MSG_*} (except \field{VIRTIO_GPIO_MSG_IRQ_EVENT}),
+      \field{gpio} field with a GPIO line number, and \field{value} to message
+      defined value.
+
+\item The driver sends the buffer to the device over the \field{txq} virtqueue.
+
+\item The driver MUST NOT initiate another request for the same GPIO line before
+      receiving a response from the device for the line.
+
+\item The device, after receiving the buffer from the driver, processes it and
+      prepares a buffer of type \field{struct virtio_gpio_msg} and sets its
+      \field{type} field to the \field{type} field of the received buffer OR'ed
+      with \field{VIRTIO_GPIO_MSG_RESPONSE}, \field{gpio} field with
+      \field{gpio} field of the received buffer, and \field{value} to a message
+      defined value.
+
+\item The device sends the buffer to the driver over the \field{rxq} virtqueue.
+
+\item The driver receives and processes the buffer.
+
+\item The driver can now initiate another request for the same GPIO line.
+
+\item The driver can initiate an request for a different GPIO line before
+      receiving response for a previous request for another line.
+\end{itemize}
-- 
2.31.1.272.g89b43f80a514

[virtio-dev] [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 virtio-gpio.tex | 153 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 150 insertions(+), 3 deletions(-)

diff --git a/virtio-gpio.tex b/virtio-gpio.tex
index 40020620d714..56e0bde4d986 100644
--- a/virtio-gpio.tex
+++ b/virtio-gpio.tex
@@ -20,7 +20,9 @@ \subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
 
 \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}
 
@@ -79,6 +81,12 @@ \subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device
 
 \item The driver MUST populate the \field{rxq} virtqueue with \field{struct
       virtio_gpio_msg} buffer.
+
+\item The driver MUST check the presence of \field{VIRTIO_GPIO_F_IRQ} feature
+      before initiating any IRQ messages.
+
+\item The device MUST mask interrupts on all the GPIO lines, if the
+      \field{VIRTIO_GPIO_F_IRQ} feature is supported.
 \end{itemize}
 
 \subsection{Device Operation}\label{sec:Device Types / GPIO Device / Device Operation}
@@ -111,8 +119,12 @@ \subsection{Device Operation}\label{sec:Device Types / GPIO Device / Device Oper
 with VIRTIO_GPIO_MSG_RESPONSE mask and updating the \field{value} field based on
 message type.
 
-For the current version of the specification, the sender is always the driver
-and receiver is always the device.
+The device is the sender only for the messages of type
+\field{VIRTIO_GPIO_MSG_IRQ_EVENT} and the device sends it over the \field{rxq}
+virtqueue.
+
+The driver is the sender for all other message types and send them over
+\field{txq} virtqueue.
 
 \begin{lstlisting}
 /* GPIO message types: driver -> device */
@@ -123,11 +135,27 @@ \subsection{Device Operation}\label{sec:Device Types / GPIO Device / Device Oper
 #define VIRTIO_GPIO_MSG_SET_DIRECTION_OUT       0x0005
 #define VIRTIO_GPIO_MSG_GET_VALUE               0x0006
 #define VIRTIO_GPIO_MSG_SET_VALUE               0x0007
+#define VIRTIO_GPIO_MSG_IRQ_TYPE                0x0008
+#define VIRTIO_GPIO_MSG_IRQ_MASK                0x0009
+#define VIRTIO_GPIO_MSG_IRQ_UNMASK              0x000a
+
+/* GPIO message type: from device -> driver */
+#define VIRTIO_GPIO_MSG_IRQ_EVENT               0x000b
 
 /* GPIO response mask, to be Or'ed with one of the above */
 #define VIRTIO_GPIO_MSG_RESPONSE                    0x8000
 \end{lstlisting}
 
+\begin{lstlisting}
+/* GPIO interrupt type */
+#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}
+
 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
 otherwise, i.e. 0 as success, and negative value as POSIX error code, positive
@@ -238,6 +266,81 @@ \subsubsection{Device Operation: Set Value}\label{sec:Device Types / GPIO Device
 \hline
 \end{tabular}
 
+\subsubsection{Device Operation: IRQ Type}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Type }
+
+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.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_IRQ_TYPE} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: IRQ Mask}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Mask }
+
+The driver initiates this message to request the device to mask IRQ on a GPIO
+line configured for input.
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
+supported by the device.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_IRQ_MASK} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_IRQ_MASK} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: IRQ Unmask}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Unmask }
+
+The driver initiates this message to request the device to unmask IRQ on a GPIO
+line configured for input.
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
+supported by the device.
+
+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
+\hline
+\end{tabular}
+
+\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }
+
+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} \\
+\hline
+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}
+
 \drivernormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
 
 \begin{itemize}
@@ -252,6 +355,11 @@ \subsubsection{Device Operation: Set Value}\label{sec:Device Types / GPIO Device
 
 \item The driver MUST NOT initiate another message for a GPIO line, before
       response to the previously sent message is received for the same line.
+
+\item The driver MUST NOT send IRQ messages for a GPIO line configured for output.
+
+\item The driver MUST NOT initiate IRQ messages if the \field{VIRTIO_GPIO_F_IRQ}
+      feature is not supported by the device.
 \end{itemize}
 
 \devicenormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
@@ -315,3 +423,42 @@ \subsubsection{Driver Requests}\label{sec:Device Types / GPIO Device / Message F
 \item The driver can initiate an request for a different GPIO line before
       receiving response for a previous request for another line.
 \end{itemize}
+
+\subsubsection{Device Interrupts}\label{sec:Device Types / GPIO Device / Message Flow / Interrupts}
+
+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.
+
+\item The driver at this point may initiate a new request (like masking of the
+      IRQ line) and send it over to the device over the \field{txq} virtqueue.
+      The device must respond to them, without waiting for a response to the
+      IRQ itself.
+
+\item The driver prepares a buffer of type \field{struct virtio_gpio_msg} and
+      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.
+
+\item The driver sends the buffer to the device over the \field{txq} virtqueue.
+
+\item The device receives and processes the buffer.
+
+\item The device can initiate another IRQ request for the same GPIO line now.
+
+\item The driver can initiate an IRQ request for a different GPIO line before
+      receiving response for a previous IRQ request for another line.
+\end{itemize}
-- 
2.31.1.272.g89b43f80a514


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Arnd Bergmann]

> 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" <lkml@metux.net>.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I'm not too familiar with either virtio specification or gpio, but
there are a few things
that stuck out to me anyway, so I'll comment here anyway on the things that
surprised me the most.

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

> +\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
> +
> +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}
> +\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.

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.

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

> +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?

> +#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
> +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.

> +\subsubsection{Device Operation: Request}\label{sec:Device Types / GPIO Device / Device Operation / Request }
> +
> +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
> +Message & \field{VIRTIO_GPIO_MSG_REQUEST} & line number & 0 \\
> +\hline
> +Response & \field{VIRTIO_GPIO_MSG_REQUEST} \newline | \field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = success, \newline or -errno = failure \\
> +\hline
> +\end{tabular}
> +
> +\subsubsection{Device Operation: Free}\label{sec:Device Types / GPIO Device / Device Operation / Free }
> +
> +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.

> +\subsection{Message flow}\label{sec:Device Types / GPIO Device / Message Flow}
> +
> +This section describe how the messages flow between device and driver.

                     describes

     Arnd

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Fri, Jul 16, 2021 at 9:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> +\subsubsection{Device Operation: IRQ Type}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Type }
> +
> +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?

> +\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }
> +
> +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} \\
> +\hline
> +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.

For the 'errno' value, see my comments on patch 1/2.

> +\subsubsection{Device Interrupts}\label{sec:Device Types / GPIO Device / Message Flow / Interrupts}
> +
> +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.
> +
> +\item The driver at this point may initiate a new request (like masking of the
> +      IRQ line) and send it over to the device over the \field{txq} virtqueue.
> +      The device must respond to them, without waiting for a response to the
> +      IRQ itself.
> +
> +\item The driver prepares a buffer of type \field{struct virtio_gpio_msg} and
> +      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?

       Arnd

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Geert Uytterhoeven]

Hi Viresh,

On Fri, Jul 16, 2021 at 9:40 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 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" <lkml@metux.net>.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks for your patch!

Disclaimer: IANAVP (not a virtio person here ;-)

> --- /dev/null
> +++ b/virtio-gpio.tex
> @@ -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:
> +
> +\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[];

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.

> +};
> +\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.
> +
> +\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
> +    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}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V5 0/2] virtio: Add specification for virtio-gpio

[Arnd Bergmann]

On Fri, Jul 16, 2021 at 9:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> 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.

Hi Viresh,

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.

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

> - Supports freeing of a GPIO line after use.

I agree it makes sense have both alloc and free as a pair if you have one
of them, but as I commented I'm not convinced we do need them at all,
so removing both may be better here.

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

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

These are good.

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.

       Arnd

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On Fri, 16 Jul 2021 at 14:33, Arnd Bergmann <arnd@kernel.org> wrote:
> On Fri, Jul 16, 2021 at 9:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +\subsubsection{Device Operation: IRQ Type}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Type }
> > +
> > +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.

> > +\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }
> > +
> > +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} \\
> > +\hline
> > +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.

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

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

> > +\item The driver prepares a buffer of type \field{struct virtio_gpio_msg} and
> > +      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.

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

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

Okay.

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

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

--
viresh

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Michael S. Tsirkin]

On Fri, Jul 16, 2021 at 01:09:18PM +0530, Viresh Kumar wrote:
> +\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}

...


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

-- 
MST

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On Fri, 16 Jul 2021 at 20:52, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jul 16, 2021 at 01:09:18PM +0530, Viresh Kumar wrote:
> > +\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}
> > +
> > +\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
> > +    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.

Hmm, interesting. This block of data, for gpio names, can be really
large as well,
lets say if we have 256 GPIOs with each requiring 10 bytes for name,
its over 2KB.

Yeah, that can't go with the config structure then.

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

Not really.

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

For example in Linux we need this information before the
GPIO device is registered with the framework and we can fetch data
over virtio there.

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

Right.

Thanks Michael for your feedback.

--
viresh

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On Fri, 16 Jul 2021 at 14:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

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

Looks like this field is going away and we need a separate request for this.

Thanks for taking time to look at this.

--
viresh

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Fri, Jul 16, 2021 at 5:17 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On Fri, 16 Jul 2021 at 14:33, Arnd Bergmann <arnd@kernel.org> wrote:
> > On Fri, Jul 16, 2021 at 9:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > +\subsubsection{Device Operation: IRQ Type}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Type }
> > > +
> > > +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.

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.

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

Let's see what the gpio maintainers think

> > > +\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }
> > > +
> > > +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} \\
> > > +\hline
> > > +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.
>
> > 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).
>
> > 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?

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.

> > > +\item The driver prepares a buffer of type \field{struct virtio_gpio_msg} and
> > > +      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.

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

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

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

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?

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

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

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.

       Arnd

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On Fri, 16 Jul 2021 at 13:54, Arnd Bergmann <arnd@kernel.org> wrote:
> > +\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".

Right.

> > +\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
> > +
> > +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}
> > +\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.
>
> 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.

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

Okay, I will add some details on it.

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

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

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

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

> > +#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
> > +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.

That's what I thought earlier and had my own error types, but then
Enrico suggested
using these and I thought, why not ?

Now that I looked at
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

POSIX only defines meaning of various error names and don't attach a numerical
value to them. So, we can't use them as is.

> > +\subsubsection{Device Operation: Free}\label{sec:Device Types / GPIO Device / Device Operation / Free }
> > +
> > +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.

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.

--
Viresh

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On Fri, 16 Jul 2021 at 21:49, Arnd Bergmann <arnd@kernel.org> wrote:
> On Fri, Jul 16, 2021 at 5:17 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On Fri, 16 Jul 2021 at 14:33, Arnd Bergmann <arnd@kernel.org> wrote:

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

And I understood it now only :(

I am not sure if this kind of per-line info would be really useful,
and I now doubt
if we should have a config structure field for the entire GPIO block
or not. i.e.
something that conveys the trigger types for entire GPIO device.

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

Thanks for explaining again.

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

I did add that kind of information in the tables for each of the message types,
maybe they weren't that readable in the text version.

Here is an example of it:

+\begin{tabular}{ |l||l|l|l| }
+\hline
+Fields & \field{type} & \field{gpio} & \field{value} \\
+\hline
+Message & \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
+\hline
+Response & \field{VIRTIO_GPIO_MSG_GET_DIRECTION} \newline |
\field{VIRTIO_GPIO_MSG_RESPONSE} & line number & 0 = output, 1 =
input, \newline or -errno = failure \\
+\hline
+\end{tabular}

The driver sends a message with "type" VIRTIO_GPIO_MSG_GET_DIRECTION and
the device responds with a buffer with "type" field set to
VIRTIO_GPIO_MSG_GET_DIRECTION | VIRTIO_GPIO_MSG_RESPONSE and
the direction set in the "value" field.

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

Will work on that.

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

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

--
Viresh

Re: [PATCH V5 0/2] virtio: Add specification for virtio-gpio

[Viresh Kumar]

On Fri, 16 Jul 2021 at 15:28, Arnd Bergmann <arnd@kernel.org> wrote:
> On Fri, Jul 16, 2021 at 9:39 AM Viresh Kumar <viresh.kumar@linaro.org> 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.

Thanks for that.

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

So the Linux gpiolib implementation does use this information, not entirely
sure how apart from displaying that in sysfs.

The gpio-names thing is going to be dropped anyway from the config structure
and will be shared over virtqueue if really required.

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

Sure.

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

Sure.

Thanks for taking time to review this Arnd.

--
Viresh

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Arnd Bergmann]

On Fri, Jul 16, 2021 at 6:26 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On Fri, 16 Jul 2021 at 13:54, Arnd Bergmann <arnd@kernel.org> wrote:
> > > +
> > > +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.
> >
> > 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.

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?

> > > +\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
> > > +    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.
>
> Okay, will do. Though this field is going away now (after Michael's
> feedback) and a request will take its place.

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.

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

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.

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

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

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

Yes, I think that is rather normal operation for a virtqueue.

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

ok, great.

> > > +\subsubsection{Device Operation: Free}\label{sec:Device Types / GPIO Device / Device Operation / Free }
> > > +
> > > +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.
>
> 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.

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.

       Arnd

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Fri, Jul 16, 2021 at 6:50 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On Fri, 16 Jul 2021 at 21:49, Arnd Bergmann <arnd@kernel.org> wrote:
> > > 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 ?

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.

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

Ok.

         Arnd

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On 16-07-21, 21:56, Viresh Kumar wrote:
> On Fri, 16 Jul 2021 at 13:54, Arnd Bergmann <arnd@kernel.org> wrote:

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

Okay, I tested this and it works. I was able to send multiple requests in
parallel and it worked out fine.

So I am going to switch back to what I had earlier, i.e. one virtqueue for all
requests originating from driver and its response from device.

And a different virtqueue for interrupts.

-- 
viresh

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On 16-07-21, 20:20, Arnd Bergmann wrote:
> 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.

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

Yeah, users should be using that.

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

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

Naah, you understood it correctly earlier.

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

And this is what I am going to do now :)

In the irq case, the driver must send a response message as well, to inform that
interrupt is processed and it is ready for the next one.

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

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

Activate functioning of a pin ?

I see many drivers in Linux who do some reg read/write in request, not exactly
sure what's going on but something on the lines of activating/deactivating a
line. Examples: gpio-cadence.c, gpio-din2.c, gpio-cs5535.c, gpio-mb86s7x.c, etc.

> Maybe LinusW can clarify whether he thinks it is needed.

+1

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

-- 
viresh

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 16-07-21, 13:09, Viresh Kumar wrote:
> 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 ?

-- 
viresh

[virtio-dev] Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Arnd Bergmann]

On Mon, Jul 19, 2021 at 11:29 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-07-21, 20:20, Arnd Bergmann wrote:
> > 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.

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

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

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.

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

Ok, good.

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

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.

     Arnd

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On 19-07-21, 12:40, Arnd Bergmann wrote:
> 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.

I don't have a really good argument for keeping it, dropping it for now unless
someone thinks it is important. :)

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

Yes, we normally end up defining them in DT via "gpio-line-names" binding, but I
think it would be better to get them over the protocol instead here as this
should be self sufficient. Something like DT will still be required to link
users to the GPIO device, and that's outside of the scope of the protocol, so DT
works well there.

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

There were more, I just didn't get to them :)

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

I think keeping APIs like ACTIVATE/DEACTIVATE makes it cleaner, than adding
another state of the PIN itself.

-- 
viresh

Re: [PATCH V5 1/2] virtio-gpio: Add the device specification

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Jul 19, 2021 at 12:40 PM Arnd Bergmann <arnd@kernel.org> wrote:
> On Mon, Jul 19, 2021 at 11:29 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 16-07-21, 20:20, Arnd Bergmann wrote:
> > > 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.

Exactly, using the GPIO aggregator you can aggregate all GPIOs you want
to export to a specific guest, followed by adjusting the permissions
on the newly created /dev/gpiochip*.

Note to $self: look into interrupt support for the GPIO Aggregator.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Mon, Jul 19, 2021 at 12:24 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-07-21, 13:09, Viresh Kumar wrote:
> > 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.

          Arnd

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Michael S. Tsirkin]

On Mon, Jul 19, 2021 at 03:54:11PM +0530, Viresh Kumar wrote:
> On 16-07-21, 13:09, Viresh Kumar wrote:
> > 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.

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.


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

> -- 
> viresh

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 19-07-21, 11:11, Michael S. Tsirkin wrote:
> On Mon, Jul 19, 2021 at 03:54:11PM +0530, Viresh Kumar wrote:
> > On 16-07-21, 13:09, Viresh Kumar wrote:
> > > 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.
> 
> 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.

So what you are suggesting is, re-queue of the buffer would mean an
acknowledgement for the interrupt from the driver.

Hmm, I was looking at parallelizing the interrupts for various GPIOs. Lets say
we have X number of GPIO lines, with what you are suggesting, we will only honor
1 interrupt at a time. What I was thinking of was providing multiple buffers at
the beginning itself, lets say 5-10, so we can process the interrupts in
parallel in separate threads (probably running on different virtual CPUs in the
guest).

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

-- 
viresh

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 16-07-21, 20:49, Arnd Bergmann wrote:
> 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.

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.

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

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.

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

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

Okay..

-- 
viresh

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 19-07-21, 14:00, Arnd Bergmann wrote:
> 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.

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

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.

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

I would like the interrupt handler at the guest to share same code
across irq types, so re-queuing a buffer only after handling the
interrupt work then. Moreover I am not sure currently when does
irq_bus_sync_lock/unlock() get called in this whole sequence, where we
actually mask/unmask the interrupts.

-- 
viresh

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Tue, Jul 20, 2021 at 7:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16-07-21, 20:49, Arnd Bergmann wrote:
> > 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.
>
> 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.

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?

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

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

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

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

       Arnd

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 20-07-21, 09:01, Arnd Bergmann wrote:
> 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

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

True.

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

Yeah, I understood it later.

-- 
viresh

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Tue, Jul 20, 2021 at 8:11 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-07-21, 14:00, Arnd Bergmann wrote:
> > 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.

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

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

But you said we can't enqueue the event from irq context in your
other reply, so I think it wouldn't work after all.

       Arnd

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Tue, Jul 20, 2021 at 9:11 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-07-21, 09:01, Arnd Bergmann wrote:
> > 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

I don't see it spell out whether this acts as a barrier or not. As far
as I can tell, the notification may be deferred, e.g. if the host is
multi-threaded and virtqueue_kick() needs to send an IPI to another
processor to look at the data.

       Arnd

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 20-07-21, 09:17, Arnd Bergmann wrote:
> 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.

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

> Ideally this could even replace the VIRTIO_GPIO_MSG_IRQ_TYPE,
> VIRTIO_GPIO_MSG_IRQ_MASK

Maybe yes.

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

> 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

-- 
viresh

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Tue, Jul 20, 2021 at 9:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-07-21, 09:17, Arnd Bergmann wrote:
> > 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.
>
> > 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.

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.

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

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.

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

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.

       Arnd

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 20-07-21, 10:10, Arnd Bergmann wrote:
> 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.

I will put more thought to the whole stuff and update the spec based
on what I get to finally.

But I really liked the idea of the token system, with a separate
message for each GPIO line. That will solve most of our problems.

-- 
viresh

Re: [PATCH V5 2/2] virtio-gpio: Add support for interrupts

[Michael S. Tsirkin]

On Tue, Jul 20, 2021 at 01:23:37PM +0530, Viresh Kumar wrote:
> On 20-07-21, 09:17, Arnd Bergmann wrote:
> > 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.
> 
> > 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.
> 
> > Ideally this could even replace the VIRTIO_GPIO_MSG_IRQ_TYPE,
> > VIRTIO_GPIO_MSG_IRQ_MASK
> 
> Maybe yes.
> 
> > 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'm not sure about the detail but buffers can be consumed
in any order. So sure you can also have an explicit unmask
or cancel or whatever message on the same vq.

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