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

[PATCH V6 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, a
different 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 everything apart from number of gpios
  and size of the gpio_names_field.

- Supports freeing of a GPIO line after use, we call them activate/deactivate
  now, which suits their purpose better.

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

- All non-interrupt traffic happens on a single virtqueue, requestq. Interrupt
  traffic goes over eventq.

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

V5 -> V6:
- All non-interrupt traffic happens on a single virtqueue, requestq. Interrupt
  traffic goes over eventq now.
- Many fields dropped from the config structure.
- Separate message type to get gpio-names, added more description about how the
  names should be.
- Much clearer message flows, both non-irq messages and irq-messages.
- Parallelism supported for all type of messages, for different GPIO pins.
- All references to POSIX errors dropped, just reply pass or fail.
- request/free renamed to activate/deactivate, as that's what we will end up
  doing there, activate or deactivate a GPIO line.
- General purpose IO as I/O or Input/Output instead.
- Hopefully I didn't miss any review comments.

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 |  30 ++-
 content.tex     |   1 +
 virtio-gpio.tex | 608 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 635 insertions(+), 4 deletions(-)
 create mode 100644 virtio-gpio.tex

-- 
2.31.1.272.g89b43f80a514

[PATCH V6 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 |  28 +++-
 content.tex     |   1 +
 virtio-gpio.tex | 369 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 394 insertions(+), 4 deletions(-)
 create mode 100644 virtio-gpio.tex

diff --git a/conformance.tex b/conformance.tex
index 94d7a06db899..c52f1a40be2d 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 / GPIO 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 / GPIO Device Conformance}.
 
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
@@ -301,6 +303,15 @@ \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}{GPIO Driver Conformance}\label{sec:Conformance / Driver Conformance / GPIO Driver Conformance}
+
+A General Purpose Input/Output (GPIO) driver MUST conform to the following
+normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / GPIO Device / requestq Operation}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -550,6 +561,15 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / SCMI Device / Device Operation / Shared Memory Operation}
 \end{itemize}
 
+\conformance{\subsection}{GPIO Device Conformance}\label{sec:Conformance / Device Conformance / GPIO Device Conformance}
+
+A General Purpose Input/Output (GPIO) device MUST conform to the following
+normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / GPIO Device / requestq 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..511f2071959d 100644
--- a/content.tex
+++ b/content.tex
@@ -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..fa48780a29db
--- /dev/null
+++ b/virtio-gpio.tex
@@ -0,0 +1,369 @@
+\section{GPIO Device}\label{sec:Device Types / GPIO Device}
+
+The Virtio GPIO device is a virtual General Purpose Input/Output device that
+supports a variable number of named I/O lines, which can be configured in input
+mode or in output mode with logical level low (0) or high (1).
+
+\subsection{Device ID}\label{sec:Device Types / GPIO Device / Device ID}
+41
+
+\subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
+
+None currently defined.
+
+\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
+
+GPIO device uses the following configuration structure layout:
+
+\begin{lstlisting}
+struct virtio_gpio_config {
+    le16 ngpio;
+    u8 padding[2];
+    le32 gpio_names_size;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{ngpio}] is the total number of GPIO lines supported by the device.
+
+\item[\field{padding}] has no meaning and is reserved for future use. This
+    MUST be set to zero by the device.
+
+\item[\field{gpio_names_size}] is the size of the gpio-names memory block in
+    bytes, which can be fetched by the driver using the
+    \field{VIRTIO_GPIO_MSG_GET_NAMES} message. The device MUST set this to 0 if
+    it doesn't support names for the GPIO lines.
+\end{description}
+
+
+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization}
+
+\begin{itemize}
+\item The driver MUST configure and initialize the \field{requestq} virtqueue.
+\end{itemize}
+
+\subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation}
+
+The driver uses the \field{requestq} virtqueue to send messages to the device.
+The driver sends a pair of buffers, request (filled by driver) and response (to
+be filled by device later), to the device. The device in turn fills the response
+buffer and sends it back to the driver.
+
+\begin{lstlisting}
+struct virtio_gpio_request {
+    le16 type;
+    le16 gpio;
+    le32 value;
+};
+\end{lstlisting}
+
+All the fields of this structure are set by the driver and read by the device.
+
+\begin{description}
+\item[\field{type}] is the GPIO message type, i.e. one of
+    \field{VIRTIO_GPIO_MSG_*} values.
+
+\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
+    \field{ngpio}.
+
+\item[\field{value}] is a message specific value.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_gpio_response {
+    u8 status;
+    u8 value[N];
+};
+
+/* Possible values of the status field */
+#define VIRTIO_GPIO_STATUS_OK                   0x0
+#define VIRTIO_GPIO_STATUS_ERR                  0x1
+\end{lstlisting}
+
+All the fields of this structure are set by the device and read by the driver.
+
+\begin{description}
+\item[\field{status}] of the GPIO message,
+    \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR}
+    on failure.
+
+\item[\field{value}] is a message specific defined value. The value of N (i.e.
+    size of the \field{value} buffer in bytes) is 1 for most of the messages,
+    except \field{VIRTIO_GPIO_MSG_GET_NAMES}, where it is set to the value of
+    \field{gpio_names_size} field.
+\end{description}
+
+Following is the list of messages supported by the virtio gpio specification.
+
+\begin{lstlisting}
+/* GPIO message types */
+#define VIRTIO_GPIO_MSG_GET_NAMES               0x0001
+#define VIRTIO_GPIO_MSG_ACTIVATE                0x0002
+#define VIRTIO_GPIO_MSG_DEACTIVATE              0x0003
+#define VIRTIO_GPIO_MSG_GET_DIRECTION           0x0004
+#define VIRTIO_GPIO_MSG_SET_DIRECTION_IN        0x0005
+#define VIRTIO_GPIO_MSG_SET_DIRECTION_OUT       0x0006
+#define VIRTIO_GPIO_MSG_GET_VALUE               0x0007
+#define VIRTIO_GPIO_MSG_SET_VALUE               0x0008
+\end{lstlisting}
+
+\subsubsection{requestq Operation: Get Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Names}
+
+The driver sends this message to receive a stream of zero-terminated strings,
+where each string represents the name of a GPIO line, present in increasing
+order of the GPIO line numbers. The GPIO line names must be unique within a GPIO
+Device and must not be an empty string.
+
+These names of the GPIO lines should be most meaningful producer names for the
+system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd"
+and "ethernet reset" are reasonable line names as they describe what the line is
+used for, while "GPIO0" is not a good name to give to a GPIO line.
+
+The device MUST set the \field{gpio_names_size} to a non-zero value if this
+message is supported by the device, else it must be set to zero.
+
+The driver must allocate the \field{value[N]} buffer in the \field{struct
+virtio_gpio_response} for N bytes, where N = \field{gpio_names_size}.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_NAMES} & 0 & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & gpio-names & \field{gpio_names_size} \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Activate}\label{sec:Device Types / GPIO Device / requestq Operation / Activate}
+
+The driver sends this message to notify the device that one of its lines has
+been assigned for use.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_ACTIVATE} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Deactivate}\label{sec:Device Types / GPIO Device / requestq Operation / Deactivate}
+
+The driver sends this message to notify the device that a previously requested
+line has been unassigned and can be deactivated.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_DEACTIVATE} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}
+
+The driver sends this message to request the device to return a line's
+configured direction.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 = output, 1 = input & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Set Direction In}\label{sec:Device Types / GPIO Device / requestq Operation / Set Direction In}
+
+The driver sends this message to request the device to configure a line for
+input.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_SET_DIRECTION_IN} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Set Direction Out}\label{sec:Device Types / GPIO Device / requestq Operation / Set Direction Out}
+
+The driver sends this message to request the device to configure a line for
+output, and set its initial output value.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_SET_DIRECTION_OUT} & line number & 0 = low, 1 = high \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Get Value}\label{sec:Device Types / GPIO Device / requestq Operation / Get Value}
+
+The driver sends this message to request the device to return current value
+sensed on a line configured for input.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_VALUE} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 = low, 1 = high & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Device / requestq Operation / Set Value}
+
+The driver sends this message to request the device to set the value of a line
+configured for output.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_SET_VALUE} & line number & 0 = low, 1 = high \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow}
+
+\begin{itemize}
+\item The driver queues \field{struct virtio_gpio_request} and
+    \field{virtio_gpio_response} buffers to the \field{requestq} virtqueue,
+    after filling all fields of the \field{struct virtio_gpio_request} buffer as
+    defined by the specific message type.
+
+\item The driver notifies the device of the presence of buffers on the
+    \field{requestq} virtqueue.
+
+\item The device, after receiving the message from the driver, processes it and
+    fills all the fields of the \field{struct virtio_gpio_response} buffer
+    (received from the driver). The \field{status} must be set to
+    \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR}
+    on failure.
+
+\item The device puts the buffers back on the \field{requestq} virtqueue and
+    notifies the driver of the same.
+
+\item The driver fetches the buffers and processes the response received in the
+    \field{virtio_gpio_response} buffer.
+
+\item The driver can now send another message for the same GPIO line.
+\end{itemize}
+
+\drivernormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation}
+
+\begin{itemize}
+\item The driver MUST send messages on the \field{requestq} virtqueue.
+
+\item The driver MUST queue both \field{struct virtio_gpio_request} and
+    \field{virtio_gpio_response} for every message sent to the device.
+
+\item The \field{struct virtio_gpio_request} buffer MUST be filled by the driver
+    and MUST be read-only for the device.
+
+\item The \field{struct virtio_gpio_response} buffer MUST be filled by the
+    device and MUST be writable by 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 MAY send messages for different GPIO lines in parallel.
+
+\item The driver MUST NOT send another message, of any type, for a GPIO line
+    before receiving a response from the device for the previously sent message
+    for the same GPIO line.
+\end{itemize}
+
+\devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation}
+
+\begin{itemize}
+\item The device MUST set all the fields of the \field{struct
+    virtio_gpio_response} before sending it back to the driver.
+
+\item The device MUST set all the fields of the \field{struct
+    virtio_gpio_config} on receiving a configuration request from the driver.
+
+\item The device MUST set the \field{gpio_names_size} field as zero in the
+    \field{struct virtio_gpio_config}, if it doesn't implement names for
+    individual GPIO lines.
+
+\item The device MUST set the \field{gpio_names_size} field, in the
+    \field{struct virtio_gpio_config}, with the size of gpio-names memory block
+    in bytes, if the device implements names for individual GPIO lines. The
+    strings MUST be zero-terminated and an unique and a valid string MUST be
+    added for each supported GPIO line.
+
+\item The device MAY process messages out of order and in parallel and MAY send
+    message's response to the driver out of order.
+\end{itemize}
-- 
2.31.1.272.g89b43f80a514

[virtio-dev] [PATCH V6 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>
---
 conformance.tex |   2 +
 virtio-gpio.tex | 241 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 242 insertions(+), 1 deletion(-)

diff --git a/conformance.tex b/conformance.tex
index c52f1a40be2d..64bcc12d1199 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -310,6 +310,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 
 \begin{itemize}
 \item \ref{drivernormative:Device Types / GPIO Device / requestq Operation}
+\item \ref{drivernormative:Device Types / GPIO Device / eventq Operation}
 \end{itemize}
 
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
@@ -568,6 +569,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 
 \begin{itemize}
 \item \ref{devicenormative:Device Types / GPIO Device / requestq Operation}
+\item \ref{devicenormative:Device Types / GPIO Device / eventq Operation}
 \end{itemize}
 
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
diff --git a/virtio-gpio.tex b/virtio-gpio.tex
index fa48780a29db..f432a3f3ad0d 100644
--- a/virtio-gpio.tex
+++ b/virtio-gpio.tex
@@ -11,11 +11,17 @@ \subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
 
 \begin{description}
 \item[0] requestq
+\item[1] eventq
 \end{description}
 
+The \field{eventq} virtqueue is available only if the \field{VIRTIO_GPIO_F_IRQ}
+feature is enabled by the device.
+
 \subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
 
-None currently defined.
+\begin{description}
+\item[VIRTIO_GPIO_F_IRQ (0)] The device supports interrupts on GPIO lines.
+\end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
 
@@ -46,6 +52,15 @@ \subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device
 
 \begin{itemize}
 \item The driver MUST configure and initialize the \field{requestq} virtqueue.
+
+\item The driver MUST check the presence of \field{VIRTIO_GPIO_F_IRQ} feature
+    before initiating any IRQ messages.
+
+\item The driver MUST configure and initialize the \field{eventq} virtqueue if
+    the \field{VIRTIO_GPIO_F_IRQ} feature is enabled by the device.
+
+\item The device MUST have interrupts on all the GPIO lines masked, if the
+    \field{VIRTIO_GPIO_F_IRQ} feature is supported.
 \end{itemize}
 
 \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation}
@@ -111,6 +126,19 @@ \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / r
 #define VIRTIO_GPIO_MSG_SET_DIRECTION_OUT       0x0006
 #define VIRTIO_GPIO_MSG_GET_VALUE               0x0007
 #define VIRTIO_GPIO_MSG_SET_VALUE               0x0008
+#define VIRTIO_GPIO_MSG_IRQ_TYPE                0x0009
+#define VIRTIO_GPIO_MSG_IRQ_MASK                0x000a
+#define VIRTIO_GPIO_MSG_IRQ_UNMASK              0x000b
+\end{lstlisting}
+
+\begin{lstlisting}
+/* GPIO interrupt types */
+#define VIRTIO_GPIO_IRQ_TYPE_NONE               0x00
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING        0x01
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING       0x02
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH          0x03
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH         0x04
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW          0x08
 \end{lstlisting}
 
 \subsubsection{requestq Operation: Get Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Names}
@@ -294,6 +322,79 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi
 \hline
 \end{tabularx}
 
+\subsubsection{requestq Operation: IRQ Type}\label{sec:Device Types / GPIO Device / requestq 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
+enabled by the device.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: IRQ Mask}\label{sec:Device Types / GPIO Device / requestq 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
+enabled by the device.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_IRQ_MASK} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: IRQ Unmask}\label{sec:Device Types / GPIO Device / requestq 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
+enabled by the device.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
 \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow}
 
 \begin{itemize}
@@ -343,6 +444,17 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
 \item The driver MUST NOT send another message, of any type, for a GPIO line
     before receiving a response from the device for the previously sent message
     for the same GPIO 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 enabled by the device.
+
+\item If the driver has sent the \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} message
+    earlier for a GPIO line, then it MUST send the
+    \field{VIRTIO_GPIO_MSG_IRQ_MASK} message over the \field{requestq}
+    virtqueue, if it no longer wants to receive interrupts from the device.
 \end{itemize}
 
 \devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation}
@@ -367,3 +479,130 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
 \item The device MAY process messages out of order and in parallel and MAY send
     message's response to the driver out of order.
 \end{itemize}
+
+\subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eventq Operation}
+
+The \field{eventq} virtqueue is used for sending interrupt events from the
+device to the driver. The driver queues a separate pair of buffers,
+interrupt-request (filled by driver) and interrupt-response (to be filled by
+device later), to the \field{eventq} virtqueue for each GPIO line. The device,
+on sensing an interrupt, returns the pair of buffers of the respective GPIO line
+for which the interrupt is sensed.
+
+\begin{lstlisting}
+struct virtio_gpio_irq_request {
+    le16 gpio;
+};
+\end{lstlisting}
+
+This structure is filled by the driver and read by the device.
+
+\begin{description}
+\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
+    \field{ngpio}.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_gpio_irq_response {
+    u8 status;
+};
+
+/* Possible values of the interrupt status field */
+#define VIRTIO_GPIO_IRQ_INVALID                 0x0
+#define VIRTIO_GPIO_IRQ_VALID                   0x1
+\end{lstlisting}
+
+This structure is filled by the device and read by the driver.
+
+\begin{description}
+\item[\field{status}] of the interrupt event, \field{VIRTIO_GPIO_IRQ_VALID} on
+    valid interrupt and \field{VIRTIO_GPIO_IRQ_INVALID} otherwise on returning
+    the buffer back to the driver without an interrupt.
+\end{description}
+
+\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}
+
+\begin{itemize}
+\item The driver is requested by a user to enable interrupt for a GPIO and
+    configure it to a particular type.
+
+\item The driver sends the \field{VIRTIO_GPIO_MSG_IRQ_TYPE} message over the
+    \field{requestq} virtqueue and the device configures the GPIO line for the
+    same and responds.
+
+\item The driver queues a pair of buffers, interrupt-request and
+    interrupt-response, to the \field{eventq} virtqueue for the GPIO line.
+
+\item The driver notifies the device of the presence of new buffers on the
+    \field{eventq} virtqueue.
+
+\item The driver sends the \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} message over the
+    \field{requestq} virtqueue and the device configures the GPIO line for the
+    same and responds.
+
+\item The interrupt is fully configured at this point.
+
+\item The device, on sensing an interrupt on a GPIO line, finds the matching
+    buffers (based on GPIO line number) from the \field{eventq} virtqueue and
+    fills its \field{struct virtio_gpio_irq_response} buffer's \field{status}
+    with \field{VIRTIO_GPIO_IRQ_VALID} and returns the pair of buffers to the
+    device.
+
+\item The device notifies the driver of the presence of new buffers on the
+    \field{eventq} virtqueue.
+
+\item If the GPIO line is configured for Level interrupts, the device MUST
+    ignore any further interrupts for the line, until the time the buffers are
+    made available again by the driver.
+
+\item If the GPIO line is configured for Edge interrupts, the device MUST latch
+    the latest interrupt received for the line, until the time the buffers are
+    made available again by the driver. At that point, the device can again
+    return the buffers for the line if an interrupt was received while the
+    device was waiting for the buffers to be made available by the driver.
+
+\item The driver on receiving the notification from the device, processes the
+    interrupt. The driver may try to mask/unmask the interrupts for the GPIO
+    line over the \field{requestq} virtqueue.
+
+\item The driver may again queue, same or new, pair of buffers for that GPIO
+    line and notify the device.
+
+\item The driver may send the \field{VIRTIO_GPIO_MSG_IRQ_MASK} message over the
+    \field{requestq} virtqueue, once it no longer wants to receive the
+    interrupts for a GPIO line.
+
+\item The device must return the unused pair of buffers for that GPIO line, over
+    the \field{eventq} virtqueue, by setting the \field{status} field with
+    \field{VIRTIO_GPIO_IRQ_INVALID}.
+
+\item The driver can then free the associated buffer and remove it from the
+    \field{eventq} virtqueue.
+\end{itemize}
+
+\drivernormative{\subsubsection}{eventq Operation}{Device Types / GPIO Device / eventq Operation}
+
+\begin{itemize}
+\item The driver MUST queue a separate pair of buffers, interrupt-request and
+    interrupt-response, to the \field{eventq} virtqueue for each GPIO line for
+    which it is expecting an interrupt from the device.
+
+\item The driver MUST not queue a pair of buffers for a GPIO line which it is
+    not going to use as interrupt source, otherwise the buffer may never be
+    freed by the device (as no unmask request will follow from the driver).
+
+\item The driver MUST NOT add multiple pairs of buffers for the same GPIO line
+    on the \field{eventq} virtqueue. And so there can only be one interrupt
+    event for each GPIO line at any point of time.
+
+\item The pair of buffers for any GPIO line can either be owned by the device or
+    the driver at any particular point of time, but not both.
+\end{itemize}
+
+\devicenormative{\subsubsection}{eventq Operation}{Device Types / GPIO Device / eventq Operation}
+
+\begin{itemize}
+\item The device can send an interrupt event for a GPIO line to the driver, only
+    if a buffer for that GPIO line is provided by the driver in the first place
+    on the \field{eventq} virtqueue.
+\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

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

[Arnd Bergmann]

On Tue, Jul 20, 2021 at 5:28 PM 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>
> ---
> +
> +\item[\field{gpio_names_size}] is the size of the gpio-names memory block in
> +    bytes, which can be fetched by the driver using the
> +    \field{VIRTIO_GPIO_MSG_GET_NAMES} message. The device MUST set this to 0 if
> +    it doesn't support names for the GPIO lines.
> +\end{description}

I would be more comfortable with leaving out this field, and making the
name lookup fixed-length and per line.

> +\subsubsection{requestq Operation: Get Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Names}
> +
> +The driver sends this message to receive a stream of zero-terminated strings,
> +where each string represents the name of a GPIO line, present in increasing
> +order of the GPIO line numbers. The GPIO line names must be unique within a GPIO
> +Device and must not be an empty string.
> +
> +These names of the GPIO lines should be most meaningful producer names for the
> +system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd"
> +and "ethernet reset" are reasonable line names as they describe what the line is
> +used for, while "GPIO0" is not a good name to give to a GPIO line.
> +
> +The device MUST set the \field{gpio_names_size} to a non-zero value if this
> +message is supported by the device, else it must be set to zero.
> +
> +The driver must allocate the \field{value[N]} buffer in the \field{struct
> +virtio_gpio_response} for N bytes, where N = \field{gpio_names_size}.

Making this a per-line request also has the advantage that the device
can have names associated with some of the lines, but leave
some other lines unnamed, which I think is something that happens
in practice.

Not sure what a good upper limit for the length of the name is, but
making this fixed-length would seem simpler to me.

> +\subsubsection{requestq Operation: Activate}\label{sec:Device Types / GPIO Device / requestq Operation / Activate}
> +
> +The driver sends this message to notify the device that one of its lines has
> +been assigned for use.

Maybe expand this description a little more according to our previous
discussion.
In particular, I would expect this to describe

- what the controller hardware is expected to do as a result
- which of the other calls are allowed on lines that have been requested
  or have not been requested. This information could be duplicated in the
  respective descriptions. In particular, it is not obvious whether you should
  set the direction before or after 'request', and I assume only one of the
  two possibilities is allowed.

> +
> +\item The driver MUST NOT send another message, of any type, for a GPIO line
> +    before receiving a response from the device for the previously sent message
> +    for the same GPIO line.

I have a question here, not sure if this needs to be answered in the spec: why
can't the driver send activate, set-direction and set-value all at
once for a line?
Clearly if one request fails, the later ones would fail as well in
this example, but
I don't see this as a problem for either side.

> +\item The device MAY process messages out of order and in parallel and MAY send
> +    message's response to the driver out of order.

Here I have the opposite question: what is the benefit of allowing the requests
to be processed out of order? Should these not all be instantaneous, and easier
to handle by making the request queue serialized?

        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 V6 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On 26-07-21, 11:18, Arnd Bergmann wrote:
> I would be more comfortable with leaving out this field, and making the
> name lookup fixed-length and per line.

I can leave it from configuration, sure.

Doing it per-line seems to be too much overhead, since we are going to
call this for every line anyway (at least in Linux implementation). I
haven't tried it, but I somehow feel that probe() will take a
significant time to finish with that, lets say for 256 GPIO lines.

> Making this a per-line request also has the advantage that the device
> can have names associated with some of the lines, but leave
> some other lines unnamed, which I think is something that happens
> in practice.
> 
> Not sure what a good upper limit for the length of the name is, but
> making this fixed-length would seem simpler to me.

Or allocate 20-25 bytes for each string and allocate a buffer of size

25 * ngpios

The device can write 0 (null) for some of the GPIOs, where it doesn't
want to provide a name, or set it to a null terminated string with
length <= 25.

> > +\subsubsection{requestq Operation: Activate}\label{sec:Device Types / GPIO Device / requestq Operation / Activate}
> > +
> > +The driver sends this message to notify the device that one of its lines has
> > +been assigned for use.
> 
> Maybe expand this description a little more according to our previous
> discussion.
> In particular, I would expect this to describe
> 
> - what the controller hardware is expected to do as a result
> - which of the other calls are allowed on lines that have been requested
>   or have not been requested. This information could be duplicated in the
>   respective descriptions. In particular, it is not obvious whether you should
>   set the direction before or after 'request', and I assume only one of the
>   two possibilities is allowed.

Right.

> > +\item The driver MUST NOT send another message, of any type, for a GPIO line
> > +    before receiving a response from the device for the previously sent message
> > +    for the same GPIO line.
> 
> I have a question here, not sure if this needs to be answered in the spec: why
> can't the driver send activate, set-direction and set-value all at
> once for a line?
> Clearly if one request fails, the later ones would fail as well in
> this example, but
> I don't see this as a problem for either side.

The driver can already do set-direction and value at once. Merging
activate along with that would mean, that the host needs to keep a
reference count at its end to activate the line on the first call to
set-value, but not on others.

We would still need to keep the deactivate request, as the host
wouldn't know when the guest is done using a line. And for that reason
it looked better to have an explicit call to activate here.

> > +\item The device MAY process messages out of order and in parallel and MAY send
> > +    message's response to the driver out of order.
> 
> Here I have the opposite question: what is the benefit of allowing the requests
> to be processed out of order? Should these not all be instantaneous, and easier
> to handle by making the request queue serialized?

Lets say the guest sends two requests, almost at the same time to
the host and they are queued on the vq in this order only:

- set direction-out-with-value-1 for line 5.
- set direction-in for line 11.

Now, if the host is able to process them in parallel, then the second
request for line 11 may finish before the requests for line 5 (as it
has lesser amount of work to do).

I don't see why the host should wait for the first request to finish
before queuing up the buffers for the second request, while it has
already finished.

To be most efficient, the queue should be thought of as a series of
buffers which are present in any order, based on what finishes first.
It doesn't increase the complexity on any side to be honest. The code
remains straight forward with it.

-- 
viresh

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

[Arnd Bergmann]

On Tue, Jul 20, 2021 at 5:28 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> This patch adds support for interrupts to the virtio-gpio specification.
> This uses the feature bit 0 for the same.

I think the flow of event messages already looks great with this version, and I
like the level of detail in the explanations.

The only part I don't like is that it seems overly redundant to have three
ways to enable/disable irq delivery. More on that below.

> +\item If the GPIO line is configured for Level interrupts, the device MUST
> +    ignore any further interrupts for the line, until the time the buffers are
> +    made available again by the driver.

This wording sounds odd, since there are not technically any interrupt events
on the level triggered line, but instead the level is active. I would
describe this
as "the device MUST mask the interrupt line, until the time ...", though that
conflicts with the terminology for the mask/unmask request.

I would still hope that we can combine the irq_type, mask and request
steps into either one or two steps. I think the minimalistic approach would
be to expand the message to include both the "mask" and "set-type"
operations, and this should work, as long as we define sensible semantics.

Another option would be to fold the "set irq type" operation into "set
direction",
so an input can be configured as any of the five irq types, with
"IRQ_TYPE_NONE" implying that interrupts are disabled for this line.
This would still provide an extra way of masking the interrupt, so we just
need "set-direction" and "request event". In this case, setting the interrupt
type to "none" should probably cancel any pending irq and force the
message to be returned.

You could actually take that further and fold the "set value" into "set
direction", which also simplifies the output direction and avoids the
case where can you set a gpio to output before setting the value first.

         Arnd

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

[Arnd Bergmann]

On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 26-07-21, 11:18, Arnd Bergmann wrote:
> > I would be more comfortable with leaving out this field, and making the
> > name lookup fixed-length and per line.
>
> I can leave it from configuration, sure.
>
> Doing it per-line seems to be too much overhead, since we are going to
> call this for every line anyway (at least in Linux implementation). I
> haven't tried it, but I somehow feel that probe() will take a
> significant time to finish with that, lets say for 256 GPIO lines.

My guess is that this would not cause any notable overhead, but we
will know for sure if you can measure it.

> > Making this a per-line request also has the advantage that the device
> > can have names associated with some of the lines, but leave
> > some other lines unnamed, which I think is something that happens
> > in practice.
> >
> > Not sure what a good upper limit for the length of the name is, but
> > making this fixed-length would seem simpler to me.
>
> Or allocate 20-25 bytes for each string and allocate a buffer of size
>
> 25 * ngpios
>
> The device can write 0 (null) for some of the GPIOs, where it doesn't
> want to provide a name, or set it to a null terminated string with
> length <= 25.

I was thinking of a name limit in the context of doing the operation per
line. If you keep the list of nul-terminated strings, then I think I would
also not limit the length per line.

> > > +\item The driver MUST NOT send another message, of any type, for a GPIO line
> > > +    before receiving a response from the device for the previously sent message
> > > +    for the same GPIO line.
> >
> > I have a question here, not sure if this needs to be answered in the spec: why
> > can't the driver send activate, set-direction and set-value all at
> > once for a line?
> > Clearly if one request fails, the later ones would fail as well in
> > this example, but
> > I don't see this as a problem for either side.
>
> The driver can already do set-direction and value at once.

Ah, I completely missed that.

> Merging activate along with that would mean, that the host needs to keep
> a reference count at its end to activate the line on the first call to
> set-value, but not on others.

My question here was about something else, to rephrase: Can't we allow
the driver to queue multiple requests for one line and then have the device
process them in order (regardless of whether it processes requests
for other lines in parallel). The sequence I had in mind here was

driver:
    1. queue enable
    2. queue set-direction
    3. queue set-value
    4. notify device
device:
    5. process enable
    6. queue enable-response
    7. process set-direction
    8. queue set-direction response
    9. process set-value
    10. queue set-value response
    11. notify driver
driver:
     12. mark all requests as done

> We would still need to keep the deactivate request, as the host
> wouldn't know when the guest is done using a line. And for that reason
> it looked better to have an explicit call to activate here.

Folding the activate/deactivate is also an interesting option, as discussed
before. I don't think we actually need to do reference counting, just making
the direction a tristate. I think this would work fine here:

- default direction is VIRTIO_GPIO_MSG_SET_DIRECTION_NONE"
- as long as the direction is not set, nothing else is allowed on the line
- changing the direction from "none" to "in" or "out" implies "activate"
- changing the direction from "in" or "out" back to "none" implies "deactivate"

This is similar to folding the irq type into the direction command, and I
would slightly prefer it that way, but I'd like to hear what others think
about it.

> > > +\item The device MAY process messages out of order and in parallel and MAY send
> > > +    message's response to the driver out of order.
> >
> > Here I have the opposite question: what is the benefit of allowing the requests
> > to be processed out of order? Should these not all be instantaneous, and easier
> > to handle by making the request queue serialized?
>
> Lets say the guest sends two requests, almost at the same time to
> the host and they are queued on the vq in this order only:
>
> - set direction-out-with-value-1 for line 5.
> - set direction-in for line 11.
>
> Now, if the host is able to process them in parallel, then the second
> request for line 11 may finish before the requests for line 5 (as it
> has lesser amount of work to do).
>
> I don't see why the host should wait for the first request to finish
> before queuing up the buffers for the second request, while it has
> already finished.
>
> To be most efficient, the queue should be thought of as a series of
> buffers which are present in any order, based on what finishes first.
> It doesn't increase the complexity on any side to be honest. The code
> remains straight forward with it.

I expected the host side to be atomic here. Under which circumstance
would the 'set direction out with value' call in your example block?

        Arnd

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

[Geert Uytterhoeven]

Hi Viresh,

On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-07-21, 11:18, Arnd Bergmann wrote:
> > Here I have the opposite question: what is the benefit of allowing the requests
> > to be processed out of order? Should these not all be instantaneous, and easier
> > to handle by making the request queue serialized?
>
> Lets say the guest sends two requests, almost at the same time to
> the host and they are queued on the vq in this order only:
>
> - set direction-out-with-value-1 for line 5.
> - set direction-in for line 11.
>
> Now, if the host is able to process them in parallel, then the second
> request for line 11 may finish before the requests for line 5 (as it
> has lesser amount of work to do).

Speaking about parallel processing, have you already envisioned
support for .[sg]et_multiple()?

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 V6 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 26-07-21, 13:10, Arnd Bergmann wrote:
> On Tue, Jul 20, 2021 at 5:28 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > This patch adds support for interrupts to the virtio-gpio specification.
> > This uses the feature bit 0 for the same.
> 
> I think the flow of event messages already looks great with this version, and I
> like the level of detail in the explanations.

Thanks.

> The only part I don't like is that it seems overly redundant to have three
> ways to enable/disable irq delivery. More on that below.
> 
> > +\item If the GPIO line is configured for Level interrupts, the device MUST
> > +    ignore any further interrupts for the line, until the time the buffers are
> > +    made available again by the driver.
> 
> This wording sounds odd, since there are not technically any interrupt events
> on the level triggered line, but instead the level is active. I would
> describe this
> as "the device MUST mask the interrupt line, until the time ...", though that
> conflicts with the terminology for the mask/unmask request.

Right.

> I would still hope that we can combine the irq_type, mask and request

request ? you wanted to say "unmask" ?

> steps into either one or two steps. I think the minimalistic approach would
> be to expand the message to include both the "mask" and "set-type"
> operations, and this should work, as long as we define sensible semantics.

I think we can easily merge unmask and set-type into a single message,
since that will fit within the request structure used for most of the
requests on the requestq. i.e. a single byte of data.

mask() doesn't require irq-type, so it can live separately.

> Another option would be to fold the "set irq type" operation into "set
> direction",

Yeah, I would like to keep them separate and not mix irq-type with
directions in general, as you can set direction to input with or
without setting its interrupt type.

Over that interrupt support is enabled with a feature, so better keep
it separate.

> so an input can be configured as any of the five irq types, with
> "IRQ_TYPE_NONE" implying that interrupts are disabled for this line.
> This would still provide an extra way of masking the interrupt, so we just
> need "set-direction" and "request event". In this case, setting the interrupt
> type to "none" should probably cancel any pending irq and force the
> message to be returned.
> 
> You could actually take that further and fold the "set value" into "set
> direction",

At least gpio-direction-out already takes a value with it.

> which also simplifies the output direction and avoids the
> case where can you set a gpio to output before setting the value first.

Yeah, that is already done.

-- 
viresh

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

[Viresh Kumar]

On 26-07-21, 13:30, Geert Uytterhoeven wrote:
> Hi Viresh,
> 
> On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 26-07-21, 11:18, Arnd Bergmann wrote:
> > > Here I have the opposite question: what is the benefit of allowing the requests
> > > to be processed out of order? Should these not all be instantaneous, and easier
> > > to handle by making the request queue serialized?
> >
> > Lets say the guest sends two requests, almost at the same time to
> > the host and they are queued on the vq in this order only:
> >
> > - set direction-out-with-value-1 for line 5.
> > - set direction-in for line 11.
> >
> > Now, if the host is able to process them in parallel, then the second
> > request for line 11 may finish before the requests for line 5 (as it
> > has lesser amount of work to do).
> 
> Speaking about parallel processing, have you already envisioned
> support for .[sg]et_multiple()?

Not yet, but we can surely do that once someone needs it. Same goes
for lots of other gpio pinwise operations.

-- 
viresh

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

[Arnd Bergmann]

On Mon, Jul 26, 2021 at 1:51 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-07-21, 13:10, Arnd Bergmann wrote:
>
> > I would still hope that we can combine the irq_type, mask and request
>
> request ? you wanted to say "unmask" ?

No, I meant VIRTIO_GPIO_MSG_IRQ_TYPE,
VIRTIO_GPIO_MSG_IRQ_{UN,}MASK and virtio_gpio_irq_request messages.

All three can change the state between 'can interrupt' and 'can not interrupt',
so you only get notified if all three are in the active state (irq type != none,
mask disabled, request queued).

> > steps into either one or two steps. I think the minimalistic approach would
> > be to expand the message to include both the "mask" and "set-type"
> > operations, and this should work, as long as we define sensible semantics.
>
> I think we can easily merge unmask and set-type into a single message,
> since that will fit within the request structure used for most of the
> requests on the requestq. i.e. a single byte of data.
>
> mask() doesn't require irq-type, so it can live separately.

The question is whether it adds any value though. As far as I can tell,
it is completely redundant, duplicating either what is implied by
irq type==none, or what is implied by the request not being queued.

> > so an input can be configured as any of the five irq types, with
> > "IRQ_TYPE_NONE" implying that interrupts are disabled for this line.
> > This would still provide an extra way of masking the interrupt, so we just
> > need "set-direction" and "request event". In this case, setting the interrupt
> > type to "none" should probably cancel any pending irq and force the
> > message to be returned.
> >
> > You could actually take that further and fold the "set value" into "set
> > direction",
>
> At least gpio-direction-out already takes a value with it.

Ok.

        Arnd

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

[Viresh Kumar]

On 26-07-21, 14:13, Arnd Bergmann wrote:
> On Mon, Jul 26, 2021 at 1:51 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 26-07-21, 13:10, Arnd Bergmann wrote:
> >
> > > I would still hope that we can combine the irq_type, mask and request
> >
> > request ? you wanted to say "unmask" ?
> 
> No, I meant VIRTIO_GPIO_MSG_IRQ_TYPE,

Ah, I was confused since this request type isn't there anymore.

> VIRTIO_GPIO_MSG_IRQ_{UN,}MASK and virtio_gpio_irq_request messages.
> 
> All three can change the state between 'can interrupt' and 'can not interrupt',
> so you only get notified if all three are in the active state (irq type != none,
> mask disabled, request queued).
> 
> > > steps into either one or two steps. I think the minimalistic approach would
> > > be to expand the message to include both the "mask" and "set-type"
> > > operations, and this should work, as long as we define sensible semantics.
> >
> > I think we can easily merge unmask and set-type into a single message,
> > since that will fit within the request structure used for most of the
> > requests on the requestq. i.e. a single byte of data.
> >
> > mask() doesn't require irq-type, so it can live separately.
> 
> The question is whether it adds any value though. As far as I can tell,
> it is completely redundant, duplicating either what is implied by
> irq type==none, or what is implied by the request not being queued.

Right, we shouldn't have redundant stuff in here.

Though, binding of all interrupt operations to the eventq can be trick
IMO.

- How will the driver mask an interrupt for which the buffer is
  already sent over the eventq? There is no API available to recall a
  buffer. So we would need an API over the requestq for that.

- Latching of edge interrupt between the buffer is returned by the
  device and requeued by the driver. How will the device know if the
  user has simply requeued the eventq buffer (where the latched value
  is meaningful) or it has done something like free_irq() followed by
  request_irq() in the kernel (where the latched value must be
  discarded).

-- 
viresh

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

[Viresh Kumar]

On 26-07-21, 13:28, Arnd Bergmann wrote:
> On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Doing it per-line seems to be too much overhead, since we are going to
> > call this for every line anyway (at least in Linux implementation). I
> > haven't tried it, but I somehow feel that probe() will take a
> > significant time to finish with that, lets say for 256 GPIO lines.
> 
> My guess is that this would not cause any notable overhead, but we
> will know for sure if you can measure it.

I will surely try it to get the number out, but sending a request 256
times instead of just once looks inefficient from the face of it :)

> > Or allocate 20-25 bytes for each string and allocate a buffer of size
> >
> > 25 * ngpios
> >
> > The device can write 0 (null) for some of the GPIOs, where it doesn't
> > want to provide a name, or set it to a null terminated string with
> > length <= 25.
> 
> I was thinking of a name limit in the context of doing the operation per
> line.

I really don't want to do it, since in kernel at least we would run
that function for all lines right at probe().

> If you keep the list of nul-terminated strings, then I think I would
> also not limit the length per line.

The problem at hand is knowing the (maximum) size of the buffer to
pre-allocate at the driver. And here are the options:

- Keep gpio_names_size in the config structure (like this commit,
  which is kind of fine since we won't need this multiple times).

- Add a new message type for that, i.e. getting size of the
  gpio_names_block, but I don't like it since this will be used only
  once.

- Fix size for each GPIO pin's name to X characters and the driver
  allocates a buffer of size X * ngpio. This provides good boundaries
  between line names, more straightforward, less confusing, but more
  data (unnecessarily) to be sent.

Which one do you like ?

> > > I have a question here, not sure if this needs to be answered in the spec: why
> > > can't the driver send activate, set-direction and set-value all at
> > > once for a line?
> > > Clearly if one request fails, the later ones would fail as well in
> > > this example, but
> > > I don't see this as a problem for either side.
> 
> > Merging activate along with that would mean, that the host needs to keep
> > a reference count at its end to activate the line on the first call to
> > set-value, but not on others.
> 
> My question here was about something else, to rephrase: Can't we allow
> the driver to queue multiple requests for one line and then have the device
> process them in order (regardless of whether it processes requests
> for other lines in parallel). The sequence I had in mind here was
> 
> driver:
>     1. queue enable
>     2. queue set-direction
>     3. queue set-value
>     4. notify device
> device:
>     5. process enable
>     6. queue enable-response
>     7. process set-direction
>     8. queue set-direction response
>     9. process set-value
>     10. queue set-value response
>     11. notify driver
> driver:
>      12. mark all requests as done

Hmm, we can do that in principle by saying all requests for a
particular line must be served in order. But right now I have rather
limited the number of requests per line to 1 for a reason, i.e. there
will always be a single user of a GPIO line at any point of time. And
so the operations will always be sequential.

If you look at users of GPIO, lets say in kernel for now, they all use
generic APIs like request, set-direction, set-value, etc. There is no
fixed order of how the different callbacks will be called by the user,
and so we will never get the requests in bulk or groups (like you
mentioned above). Each operation has its own significance, and it
won't be right to return from, lets say gpio_set_value(), without
getting the result of the operation.

So when can something like this, what you showed above, can be useful
?

> > We would still need to keep the deactivate request, as the host
> > wouldn't know when the guest is done using a line. And for that reason
> > it looked better to have an explicit call to activate here.
> 
> Folding the activate/deactivate is also an interesting option, as discussed
> before. I don't think we actually need to do reference counting, just making
> the direction a tristate. I think this would work fine here:
> 
> - default direction is VIRTIO_GPIO_MSG_SET_DIRECTION_NONE"
> - as long as the direction is not set, nothing else is allowed on the line
> - changing the direction from "none" to "in" or "out" implies "activate"
> - changing the direction from "in" or "out" back to "none" implies "deactivate"

I am not able to see anything which can break with this, so this looks
okay. We can get away with activate/deactivate and use direction-none
for that.

> This is similar to folding the irq type into the direction command, and I
> would slightly prefer it that way, but I'd like to hear what others think
> about it.

I would really like to keep the irq stuff separate from rest of the
basic APIs. This would also be less confusing because the irq stuff is
handled separately using a feature flag here.

Over that the users won't club these either, like in Linux the user
will do gpio-request, then direction-input, and then request irq.

So these are all required to be sent at different times.

> > > Here I have the opposite question: what is the benefit of allowing the requests
> > > to be processed out of order? Should these not all be instantaneous, and easier
> > > to handle by making the request queue serialized?
> >
> > Lets say the guest sends two requests, almost at the same time to
> > the host and they are queued on the vq in this order only:
> >
> > - set direction-out-with-value-1 for line 5.
> > - set direction-in for line 11.
> >
> > Now, if the host is able to process them in parallel, then the second
> > request for line 11 may finish before the requests for line 5 (as it
> > has lesser amount of work to do).
> >
> > I don't see why the host should wait for the first request to finish
> > before queuing up the buffers for the second request, while it has
> > already finished.
> >
> > To be most efficient, the queue should be thought of as a series of
> > buffers which are present in any order, based on what finishes first.
> > It doesn't increase the complexity on any side to be honest. The code
> > remains straight forward with it.
> 
> I expected the host side to be atomic here. Under which circumstance
> would the 'set direction out with value' call in your example block?

Yes it will block, but another thread can initiate another request for
a different GPIO line.

-- 
viresh

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

[Arnd Bergmann]

On Tue, Jul 27, 2021 at 8:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-07-21, 14:13, Arnd Bergmann wrote:
> > The question is whether it adds any value though. As far as I can tell,
> > it is completely redundant, duplicating either what is implied by
> > irq type==none, or what is implied by the request not being queued.
>
> Right, we shouldn't have redundant stuff in here.
>
> Though, binding of all interrupt operations to the eventq can be trick
> IMO.
>
> - How will the driver mask an interrupt for which the buffer is
>   already sent over the eventq? There is no API available to recall a
>   buffer. So we would need an API over the requestq for that.

Agreed, there needs to be some way to cancel the request. I don't
think this would have to be a fast or common operation, so setting
the line back to IRQ_TYPE_NONE would be one way.

Another option would be to make the mask software-only and
just ignore an incoming event for a line that is configured as
masked, and then not requeue the request.

> - Latching of edge interrupt between the buffer is returned by the
>   device and requeued by the driver. How will the device know if the
>   user has simply requeued the eventq buffer (where the latched value
>   is meaningful) or it has done something like free_irq() followed by
>   request_irq() in the kernel (where the latched value must be
>   discarded).

I was expecting a 'mask' to not discard the latched event but simply
not deliver it until it gets unmasked. Either way, we can define that
a new set-irq-type request discards all latched edge interrupts.

       Arnd

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

[Arnd Bergmann]

On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-07-21, 13:28, Arnd Bergmann wrote:
> > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > Doing it per-line seems to be too much overhead, since we are going to
> > > call this for every line anyway (at least in Linux implementation). I
> > > haven't tried it, but I somehow feel that probe() will take a
> > > significant time to finish with that, lets say for 256 GPIO lines.
> >
> > My guess is that this would not cause any notable overhead, but we
> > will know for sure if you can measure it.
>
> I will surely try it to get the number out, but sending a request 256
> times instead of just once looks inefficient from the face of it :)

It could still be only one notification though, just a lot of requests
with a pair of buffers each, that you send all at once.

> > If you keep the list of nul-terminated strings, then I think I would
> > also not limit the length per line.
>
> The problem at hand is knowing the (maximum) size of the buffer to
> pre-allocate at the driver. And here are the options:
>
> - Keep gpio_names_size in the config structure (like this commit,
>   which is kind of fine since we won't need this multiple times).
>
> - Add a new message type for that, i.e. getting size of the
>   gpio_names_block, but I don't like it since this will be used only
>   once.
>
> - Fix size for each GPIO pin's name to X characters and the driver
>   allocates a buffer of size X * ngpio. This provides good boundaries
>   between line names, more straightforward, less confusing, but more
>   data (unnecessarily) to be sent.
>
> Which one do you like ?

If we stay with a single message to get all the names, I'd probably
also stay with having the buffer length in config space as you do in
the current version.

> > > > I have a question here, not sure if this needs to be answered in the spec: why
> > > > can't the driver send activate, set-direction and set-value all at
> > > > once for a line?
> > > > Clearly if one request fails, the later ones would fail as well in
> > > > this example, but
> > > > I don't see this as a problem for either side.
> >
> > > Merging activate along with that would mean, that the host needs to keep
> > > a reference count at its end to activate the line on the first call to
> > > set-value, but not on others.
> >
> > My question here was about something else, to rephrase: Can't we allow
> > the driver to queue multiple requests for one line and then have the device
> > process them in order (regardless of whether it processes requests
> > for other lines in parallel). The sequence I had in mind here was
> >
> > driver:
> >     1. queue enable
> >     2. queue set-direction
> >     3. queue set-value
> >     4. notify device
> > device:
> >     5. process enable
> >     6. queue enable-response
> >     7. process set-direction
> >     8. queue set-direction response
> >     9. process set-value
> >     10. queue set-value response
> >     11. notify driver
> > driver:
> >      12. mark all requests as done
>
> Hmm, we can do that in principle by saying all requests for a
> particular line must be served in order. But right now I have rather
> limited the number of requests per line to 1 for a reason, i.e. there
> will always be a single user of a GPIO line at any point of time. And
> so the operations will always be sequential.
>
> If you look at users of GPIO, lets say in kernel for now, they all use
> generic APIs like request, set-direction, set-value, etc. There is no
> fixed order of how the different callbacks will be called by the user,
> and so we will never get the requests in bulk or groups (like you
> mentioned above). Each operation has its own significance, and it
> won't be right to return from, lets say gpio_set_value(), without
> getting the result of the operation.
>
> So when can something like this, what you showed above, can be useful
> ?

Maybe this is something we can get advice for from the virtio
maintainers. It was just a feeling on my side that slightly relaxing
the requirement makes this more like other virtio drivers.

Functionally, I think there is no difference between mandating that the
driver only queues one request per gpio line over requiring that the
device processes all requests on a gpio line in sequence, but the
latter can be slightly more efficient.

> > This is similar to folding the irq type into the direction command, and I
> > would slightly prefer it that way, but I'd like to hear what others think
> > about it.
>
> I would really like to keep the irq stuff separate from rest of the
> basic APIs. This would also be less confusing because the irq stuff is
> handled separately using a feature flag here.
>
> Over that the users won't club these either, like in Linux the user
> will do gpio-request, then direction-input, and then request irq.
>
> So these are all required to be sent at different times.

ok

> > > To be most efficient, the queue should be thought of as a series of
> > > buffers which are present in any order, based on what finishes first.
> > > It doesn't increase the complexity on any side to be honest. The code
> > > remains straight forward with it.
> >
> > I expected the host side to be atomic here. Under which circumstance
> > would the 'set direction out with value' call in your example block?
>
> Yes it will block, but another thread can initiate another request for
> a different GPIO line.

Ok, I was hoping that the gpiolib interfaces would all end up being
non-blocking, but I agree that if the guest has to wait for a notification
it can be useful to have more than one going on at once. Especially
if the host implementation is in a separate process there is not even
a theoretical upper bound on how long it takes to process a request.

       Arnd

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

[Viresh Kumar]

On 27-07-21, 10:16, Arnd Bergmann wrote:
> On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 26-07-21, 13:28, Arnd Bergmann wrote:
> > > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > Doing it per-line seems to be too much overhead, since we are going to
> > > > call this for every line anyway (at least in Linux implementation). I
> > > > haven't tried it, but I somehow feel that probe() will take a
> > > > significant time to finish with that, lets say for 256 GPIO lines.
> > >
> > > My guess is that this would not cause any notable overhead, but we
> > > will know for sure if you can measure it.
> >
> > I will surely try it to get the number out, but sending a request 256
> > times instead of just once looks inefficient from the face of it :)
 
I created a small setup with qemu-arm64 over x86:
- Without any memcpy() of data or buffers in the GPIO drivers on both
  ends.
- i.e. essentially taking into consideration only the time to send
  receive requests, the virtio core may be copying packets in between
  though, not sure.

> It could still be only one notification though, just a lot of requests
> with a pair of buffers each, that you send all at once.

If we send only one notification at the end, it takes ~ 1ms for 256
GPIO pins, pretty much like a single request only. The downside here
is that the device (host) also needs to prepare the queue for equal
number (to ngpio) of elements. This is something the spec hasn't
touched until now, i.e. how many buffers must be queued for the
virtqueues. The max allowed in qemu is 1024 right now.

Even if I send them sequentially, with separate notification for each
request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024
lines.

Though a single request still sounds tempting to me :)

> > > My question here was about something else, to rephrase: Can't we allow
> > > the driver to queue multiple requests for one line and then have the device
> > > process them in order (regardless of whether it processes requests
> > > for other lines in parallel). The sequence I had in mind here was
> > >
> > > driver:
> > >     1. queue enable
> > >     2. queue set-direction
> > >     3. queue set-value
> > >     4. notify device
> > > device:
> > >     5. process enable
> > >     6. queue enable-response
> > >     7. process set-direction
> > >     8. queue set-direction response
> > >     9. process set-value
> > >     10. queue set-value response
> > >     11. notify driver
> > > driver:
> > >      12. mark all requests as done
> >
> > Hmm, we can do that in principle by saying all requests for a
> > particular line must be served in order. But right now I have rather
> > limited the number of requests per line to 1 for a reason, i.e. there
> > will always be a single user of a GPIO line at any point of time. And
> > so the operations will always be sequential.
> >
> > If you look at users of GPIO, lets say in kernel for now, they all use
> > generic APIs like request, set-direction, set-value, etc. There is no
> > fixed order of how the different callbacks will be called by the user,
> > and so we will never get the requests in bulk or groups (like you
> > mentioned above). Each operation has its own significance, and it
> > won't be right to return from, lets say gpio_set_value(), without
> > getting the result of the operation.
> >
> > So when can something like this, what you showed above, can be useful
> > ?
> 
> Maybe this is something we can get advice for from the virtio
> maintainers. It was just a feeling on my side that slightly relaxing
> the requirement makes this more like other virtio drivers.
> 
> Functionally, I think there is no difference between mandating that the
> driver only queues one request per gpio line over requiring that the
> device processes all requests on a gpio line in sequence, but the
> latter can be slightly more efficient.

Yes, it can be efficient but I don't see when this can be used in
practice. Specially in kernel, the way GPIO users handle stuff.

But anyway, I don't have any issue specifically with allowing multiple
requests per GPIO line in the spec itself. Spec can be kept open for
such an option.

> > > > To be most efficient, the queue should be thought of as a series of
> > > > buffers which are present in any order, based on what finishes first.
> > > > It doesn't increase the complexity on any side to be honest. The code
> > > > remains straight forward with it.
> > >
> > > I expected the host side to be atomic here. Under which circumstance
> > > would the 'set direction out with value' call in your example block?
> >
> > Yes it will block, but another thread can initiate another request for
> > a different GPIO line.
> 
> Ok, I was hoping that the gpiolib interfaces would all end up being
> non-blocking,

I am not sure how we can do that :(

> but I agree that if the guest has to wait for a notification
> it can be useful to have more than one going on at once. Especially
> if the host implementation is in a separate process there is not even
> a theoretical upper bound on how long it takes to process a request.

Right.

-- 
viresh

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

[Cornelia Huck]

On Tue, Jul 27 2021, Arnd Bergmann <arnd@kernel.org> wrote:

> On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 26-07-21, 13:28, Arnd Bergmann wrote:
>> > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

[Disclaimer: I basically don't know anything about gpio]

>> > > > I have a question here, not sure if this needs to be answered in the spec: why
>> > > > can't the driver send activate, set-direction and set-value all at
>> > > > once for a line?
>> > > > Clearly if one request fails, the later ones would fail as well in
>> > > > this example, but
>> > > > I don't see this as a problem for either side.
>> >
>> > > Merging activate along with that would mean, that the host needs to keep
>> > > a reference count at its end to activate the line on the first call to
>> > > set-value, but not on others.
>> >
>> > My question here was about something else, to rephrase: Can't we allow
>> > the driver to queue multiple requests for one line and then have the device
>> > process them in order (regardless of whether it processes requests
>> > for other lines in parallel). The sequence I had in mind here was
>> >
>> > driver:
>> >     1. queue enable
>> >     2. queue set-direction
>> >     3. queue set-value
>> >     4. notify device
>> > device:
>> >     5. process enable
>> >     6. queue enable-response
>> >     7. process set-direction
>> >     8. queue set-direction response
>> >     9. process set-value
>> >     10. queue set-value response
>> >     11. notify driver
>> > driver:
>> >      12. mark all requests as done

What are the ordering/sequence requirements here? E.g., are
enable/set-direction or enable/set-value/set-direction valid sequences?

If we consider the sequence 1-4 above as a command chain, what about
adding a chaining field that can point to the next command in the chain?
The driver would build the chain in one go, the device would process the
chain one after another, and stop if it encounters an error. The driver
would be free to submit chains for other lines, but not another one for
the same line as long as the current chain is not yet finished.

[Yes, I realize that this looks a lot like channel I/O...]

>>
>> Hmm, we can do that in principle by saying all requests for a
>> particular line must be served in order. But right now I have rather
>> limited the number of requests per line to 1 for a reason, i.e. there
>> will always be a single user of a GPIO line at any point of time. And
>> so the operations will always be sequential.
>>
>> If you look at users of GPIO, lets say in kernel for now, they all use
>> generic APIs like request, set-direction, set-value, etc. There is no
>> fixed order of how the different callbacks will be called by the user,
>> and so we will never get the requests in bulk or groups (like you
>> mentioned above). Each operation has its own significance, and it
>> won't be right to return from, lets say gpio_set_value(), without
>> getting the result of the operation.

You could match this to individual chains of length one, where you
cannot submit another request for a line before the current one is
finished.

>>
>> So when can something like this, what you showed above, can be useful
>> ?
>
> Maybe this is something we can get advice for from the virtio
> maintainers. It was just a feeling on my side that slightly relaxing
> the requirement makes this more like other virtio drivers.
>
> Functionally, I think there is no difference between mandating that the
> driver only queues one request per gpio line over requiring that the
> device processes all requests on a gpio line in sequence, but the
> latter can be slightly more efficient.

It might be worth considering for future drivers, or non-Linux drivers.


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

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

[Viresh Kumar]

On 27-07-21, 10:03, Arnd Bergmann wrote:
> On Tue, Jul 27, 2021 at 8:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 26-07-21, 14:13, Arnd Bergmann wrote:
> > > The question is whether it adds any value though. As far as I can tell,
> > > it is completely redundant, duplicating either what is implied by
> > > irq type==none, or what is implied by the request not being queued.
> >
> > Right, we shouldn't have redundant stuff in here.
> >
> > Though, binding of all interrupt operations to the eventq can be trick
> > IMO.
> >
> > - How will the driver mask an interrupt for which the buffer is
> >   already sent over the eventq? There is no API available to recall a
> >   buffer. So we would need an API over the requestq for that.
> 
> Agreed, there needs to be some way to cancel the request. I don't
> think this would have to be a fast or common operation, so setting
> the line back to IRQ_TYPE_NONE would be one way.

Hmm, how would that work ? I thought you were talking about inferring
all irq operations from the presence of the buffer in the eventq
itself. So once the buffer is sent to the host, we can't send it again
to change the irq-type.

> Another option would be to make the mask software-only and
> just ignore an incoming event for a line that is configured as
> masked, and then not requeue the request.

Hmm, I wonder what will happen if the driver gets removed/probed again
after sending the buffer :)

> > - Latching of edge interrupt between the buffer is returned by the
> >   device and requeued by the driver. How will the device know if the
> >   user has simply requeued the eventq buffer (where the latched value
> >   is meaningful) or it has done something like free_irq() followed by
> >   request_irq() in the kernel (where the latched value must be
> >   discarded).
> 
> I was expecting a 'mask' to not discard the latched event but simply
> not deliver it until it gets unmasked.

Right, we are aligned on that. Masked == buffer is owned by driver
here.

> Either way, we can define that
> a new set-irq-type request discards all latched edge interrupts.

Hmm, so you proposing to keep a separate (single) request for irq
handling over the controlq ?

-- 
viresh

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

[Arnd Bergmann]

On Tue, Jul 27, 2021 at 1:01 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-07-21, 10:03, Arnd Bergmann wrote:
> > On Tue, Jul 27, 2021 at 8:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 26-07-21, 14:13, Arnd Bergmann wrote:
> > > > The question is whether it adds any value though. As far as I can tell,
> > > > it is completely redundant, duplicating either what is implied by
> > > > irq type==none, or what is implied by the request not being queued.
> > >
> > > Right, we shouldn't have redundant stuff in here.
> > >
> > > Though, binding of all interrupt operations to the eventq can be trick
> > > IMO.
> > >
> > > - How will the driver mask an interrupt for which the buffer is
> > >   already sent over the eventq? There is no API available to recall a
> > >   buffer. So we would need an API over the requestq for that.
> >
> > Agreed, there needs to be some way to cancel the request. I don't
> > think this would have to be a fast or common operation, so setting
> > the line back to IRQ_TYPE_NONE would be one way.
>
> Hmm, how would that work ? I thought you were talking about inferring
> all irq operations from the presence of the buffer in the eventq
> itself. So once the buffer is sent to the host, we can't send it again
> to change the irq-type.

You are right, I was getting ahead of myself here: we could do this
if the set-irq-type oepration gets folded into set-direction as I
hinted as one of many options, but it does not work if the
set-irq-type and ack operations are both folded into queuing a
request on the event queue.

> > Another option would be to make the mask software-only and
> > just ignore an incoming event for a line that is configured as
> > masked, and then not requeue the request.
>
> Hmm, I wonder what will happen if the driver gets removed/probed again
> after sending the buffer :)

Removing the driver would disable the line (or set it to direction_none
if we go there) and shut down the virtqueue, so both of these should
lead to no event getting latched any more.

> > > - Latching of edge interrupt between the buffer is returned by the
> > >   device and requeued by the driver. How will the device know if the
> > >   user has simply requeued the eventq buffer (where the latched value
> > >   is meaningful) or it has done something like free_irq() followed by
> > >   request_irq() in the kernel (where the latched value must be
> > >   discarded).
> >
> > I was expecting a 'mask' to not discard the latched event but simply
> > not deliver it until it gets unmasked.
>
> Right, we are aligned on that. Masked == buffer is owned by driver
> here.
>
> > Either way, we can define that
> > a new set-irq-type request discards all latched edge interrupts.
>
> Hmm, so you proposing to keep a separate (single) request for irq
> handling over the controlq ?

This was under the assumption that we decide to still keep some
controlq request that sets the irq type and just remove the 'mask'
command.

If we go all the way to having only one message for interrupts, I
suppose it does get a little uglier than I was hoping for, but it would still
be doable: in this case, we could allow a flow like this on the eventq:

- driver requests edge interrupts
- (no event happened, so request remains pending)
- driver queues a new request asking for IRQ_TYPE_NONE notification
  in order to mask this line
- device replies to both requests saying no interrupt happened

Between this control flow and the version where set-type is part of
set-direction, I would prefer the other option, but you already said
that you don't like that one.

       Arnd

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

[Viresh Kumar]

On 27-07-21, 12:57, Cornelia Huck wrote:
> On Tue, Jul 27 2021, Arnd Bergmann <arnd@kernel.org> wrote:
> >> > driver:
> >> >     1. queue enable
> >> >     2. queue set-direction
> >> >     3. queue set-value
> >> >     4. notify device
> >> > device:
> >> >     5. process enable
> >> >     6. queue enable-response
> >> >     7. process set-direction
> >> >     8. queue set-direction response
> >> >     9. process set-value
> >> >     10. queue set-value response
> >> >     11. notify driver
> >> > driver:
> >> >      12. mark all requests as done
> 
> What are the ordering/sequence requirements here? E.g., are
> enable/set-direction or enable/set-value/set-direction valid sequences?

Yes, these are valid sequences. But there is not real hardfixed way of
doing things here, the users can call them most likely in any order.

> If we consider the sequence 1-4 above as a command chain, what about
> adding a chaining field that can point to the next command in the chain?
> The driver would build the chain in one go, the device would process the
> chain one after another, and stop if it encounters an error. The driver
> would be free to submit chains for other lines, but not another one for
> the same line as long as the current chain is not yet finished.

These commands aren't related necessarily and so this kind of linking
between them is not right. Each operation is really independent from
each other, though the user may not want to initiate the second
operation if the first one fails. This isn't something like I2C
messages, where the first one gives the address and second one gives
something else (like size) and is incomplete without the first one.

Here all instructions is actually independent of each other.

I think Arnd was just thinking of this to make it possible to send
notification only once, if that can give some improvement. But I am
quite sure that Linux today will most likely not end up using it that
way.

> > Maybe this is something we can get advice for from the virtio
> > maintainers. It was just a feeling on my side that slightly relaxing
> > the requirement makes this more like other virtio drivers.
> >
> > Functionally, I think there is no difference between mandating that the
> > driver only queues one request per gpio line over requiring that the
> > device processes all requests on a gpio line in sequence, but the
> > latter can be slightly more efficient.
> 
> It might be worth considering for future drivers, or non-Linux drivers.

Right, as I said in the other email just now, I am fine with allowing
this in spec at least (someone may want to use it later), i.e. queuing
of multiple request for the same GPIO line and them being processed in
the same order.

-- 
viresh

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

[Viresh Kumar]

On 27-07-21, 13:16, Arnd Bergmann wrote:
> This was under the assumption that we decide to still keep some
> controlq request that sets the irq type and just remove the 'mask'
> command.

Right, so I think I can fold all three of irq messages, mask, unmask
and type, to a single message type. Where the value of type can play
the role of masking/unmasking. This should work fine, I will see if I
get any more doubts on this.

> If we go all the way to having only one message for interrupts, I
> suppose it does get a little uglier than I was hoping for, but it would still
> be doable: in this case, we could allow a flow like this on the eventq:
> 
> - driver requests edge interrupts

This over eventq and ..

> - (no event happened, so request remains pending)
> - driver queues a new request asking for IRQ_TYPE_NONE notification
>   in order to mask this line

This over controlq. Right ?

> - device replies to both requests saying no interrupt happened

Yes, that can work.

> Between this control flow and the version where set-type is part of
> set-direction, I would prefer the other option, but you already said
> that you don't like that one.

Yeah, I would like to keep this away from set-direction, which I am
already going to use for activate/deactivate :)

-- 
viresh

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

[Arnd Bergmann]

On Tue, Jul 27, 2021 at 12:56 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-07-21, 10:16, Arnd Bergmann wrote:
> > On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > My guess is that this would not cause any notable overhead, but we
> > > > will know for sure if you can measure it.
> > >
> > > I will surely try it to get the number out, but sending a request 256
> > > times instead of just once looks inefficient from the face of it :)
>
> I created a small setup with qemu-arm64 over x86:
> - Without any memcpy() of data or buffers in the GPIO drivers on both
>   ends.
> - i.e. essentially taking into consideration only the time to send
>   receive requests, the virtio core may be copying packets in between
>   though, not sure.
>
> > It could still be only one notification though, just a lot of requests
> > with a pair of buffers each, that you send all at once.
>
> If we send only one notification at the end, it takes ~ 1ms for 256
> GPIO pins, pretty much like a single request only. The downside here
> is that the device (host) also needs to prepare the queue for equal
> number (to ngpio) of elements. This is something the spec hasn't
> touched until now, i.e. how many buffers must be queued for the
> virtqueues. The max allowed in qemu is 1024 right now.

1ms is borderline I guess, it's close to the range of things that gets
annoying for boot-time optimization.

> Even if I send them sequentially, with separate notification for each
> request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024
> lines.
>
> Though a single request still sounds tempting to me :)

Right, 80ms is definitely noticeable, this can easily make the difference
between an instant guest boot and a perceived delay.

> > > > I expected the host side to be atomic here. Under which circumstance
> > > > would the 'set direction out with value' call in your example block?
> > >
> > > Yes it will block, but another thread can initiate another request for
> > > a different GPIO line.
> >
> > Ok, I was hoping that the gpiolib interfaces would all end up being
> > non-blocking,
>
> I am not sure how we can do that :(

I suppose not without a complete redesign from scratch.

        Arnd

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

[Arnd Bergmann]

On Tue, Jul 27, 2021 at 1:26 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-07-21, 13:16, Arnd Bergmann wrote:
> > This was under the assumption that we decide to still keep some
> > controlq request that sets the irq type and just remove the 'mask'
> > command.
>
> Right, so I think I can fold all three of irq messages, mask, unmask
> and type, to a single message type. Where the value of type can play
> the role of masking/unmasking. This should work fine, I will see if I
> get any more doubts on this.
>
> > If we go all the way to having only one message for interrupts, I
> > suppose it does get a little uglier than I was hoping for, but it would still
> > be doable: in this case, we could allow a flow like this on the eventq:
> >
> > - driver requests edge interrupts
>
> This over eventq and ..
>
> > - (no event happened, so request remains pending)
> > - driver queues a new request asking for IRQ_TYPE_NONE notification
> >   in order to mask this line
>
> This over controlq. Right ?

Sorry for not being too clear: this was meant as the example
where both messages are on the eventq. It would work just
as well if the irq type setting is a message on the controlq though,
so I suppose it doesn't matter what I was thinking as long as
we end up with a working design ;-)

> > - device replies to both requests saying no interrupt happened
>
> Yes, that can work.
>
> > Between this control flow and the version where set-type is part of
> > set-direction, I would prefer the other option, but you already said
> > that you don't like that one.
>
> Yeah, I would like to keep this away from set-direction, which I am
> already going to use for activate/deactivate :)

Ok, so the set of relevant messages here would be:

- controlq: set direction (includes enable, but not set-irq-type)
- controlq: set-irq-type (may cancel outstanding eventq request)
- eventq: request notification (implies unmask, reply implies mask)

This sounds like a reasonably consistent and efficient way to do it,
I'm happy enough with that design for the next version.

        Arnd

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

[Viresh Kumar]

On Tue, 27 Jul 2021 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote:
> On Tue, Jul 27, 2021 at 12:56 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > If we send only one notification at the end, it takes ~ 1ms for 256
> > GPIO pins, pretty much like a single request only. The downside here
> > is that the device (host) also needs to prepare the queue for equal
> > number (to ngpio) of elements. This is something the spec hasn't
> > touched until now, i.e. how many buffers must be queued for the
> > virtqueues. The max allowed in qemu is 1024 right now.
>
> 1ms is borderline I guess, it's close to the range of things that gets
> annoying for boot-time optimization.
>
> > Even if I send them sequentially, with separate notification for each
> > request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024
> > lines.
> >
> > Though a single request still sounds tempting to me :)
>
> Right, 80ms is definitely noticeable, this can easily make the difference
> between an instant guest boot and a perceived delay.

It is also important to mention here that in my current example, there
was no copy
of the gpio line name string made anywhere.

If we choose this message to be per-line, then there will be at least one memcpy
at the host of the string for each GPIO line, there may be another
copy somewhere
in between at guest or host, not sure. Though in case of a single
request, this will
be a single memcpy operation at the guest (and others in between as
mentioned earlier),
but not per line and maybe more efficient.

So even in the best possible scenario, this will be several ms, at
least for separate
message for each GPIO line.

Anyway, how do you suggest I define this call now. Per-line or entire
block (fixed size or
from config structure) ? I will do it the way you suggest to cut short
the discussion :)

> > I am not sure how we can do that :(
>
> I suppose not without a complete redesign from scratch.

Yeah, I am not doing it then :)

--
viresh

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

[Viresh Kumar]

On Tue, 27 Jul 2021 at 19:47, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> It is also important to mention here that in my current example, there
> was no copy
> of the gpio line name string made anywhere.
>
> If we choose this message to be per-line, then there will be at least one memcpy
> at the host of the string for each GPIO line, there may be another
> copy somewhere
> in between at guest or host, not sure. Though in case of a single
> request, this will
> be a single memcpy operation at the guest (and others in between as
> mentioned earlier),
> but not per line and maybe more efficient.
>
> So even in the best possible scenario, this will be several ms, at
> least for separate
> message for each GPIO line.
>
> Anyway, how do you suggest I define this call now. Per-line or entire
> block (fixed size or
> from config structure) ? I will do it the way you suggest to cut short
> the discussion :)

Also to mention that this request will always be called (at least for Linux)
only once for all the GPIO lines, at probe time only. And never after that.

So making it per-line doesn't give us any flexibility that we may be
looking for.

--
Viresh

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

[Arnd Bergmann]

On Tue, Jul 27, 2021 at 4:20 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> So making it per-line doesn't give us any flexibility that we may be
> looking for.

I was not really looking for extra flexibility here, only for a simpler and
more consistent interface definition. But if the simpler interface comes
at the cost of more complexity and time overhead in the driver, it's
probably not worth it.

      Arnd