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 three versions Enrico posted until now.
This patchset is already reviewed by Linus (GPIO Maintainer) and Arnd.
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.
- Request/free is merged with set-direction itself.
- 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 for different GPIO lines, it can respond earlier to the second request, while still processing the first one.
Version history of the my changes:
V7 -> V8: - Use fixed-length struct virtio_gpio_response for all requests and define a separate structure for get-names. - Explicitly write the expected values of direction in the get/set-direction tables. - Better/detailed interrupt handling information, based on fast-eoi mechanism now, where the driver notifies the device only once after the interrupt is handled. - Added reviewed-by tags collected so far.
V6 -> V7: - Drop Activate/Deactivate requests and merge them with set-direction one. - Dropped separate calls to set direction to input or output (with a value), with a single call to set-direction (input, output, or none). Note that the driver needs to call set-value before calling set-direction-out now. - Allow multiple messages for a GPIO line to be sent together, they must be processed in order though. - The gpio-line names aren't required to be set for all the lines now, it is optional and can be present only for a subset of lines. Provided example as well. - Merge irq-set/mask/unmask to a single set-irq-type message. - We have only 6 message types now instead of 11 in v6. - Mentioned about non-atomic nature of the these messages in commit log for patch 1/2. - Improved spec content overall.
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 | 578 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 605 insertions(+), 4 deletions(-) create mode 100644 virtio-gpio.tex
virtio-gpio is a virtual GPIO controller. It provides a way to flexibly communicate with the host GPIO controllers from the guest.
Note that the current implementation doesn't provide atomic APIs for GPIO configurations. i.e. the driver (guest) would need to implement sleep-able versions of the APIs as the guest will respond asynchronously over the virtqueue.
This patch adds the specification for it.
Based on the initial work posted by: "Enrico Weigelt, metux IT consult" lkml@metux.net.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110 Reviewed-by: Arnd Bergmann arnd@arndb.de Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- conformance.tex | 28 +++- content.tex | 1 + virtio-gpio.tex | 346 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 371 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 31b02e1dca0e..0008727a80df 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..1d1ac672db37 --- /dev/null +++ b/virtio-gpio.tex @@ -0,0 +1,346 @@ +\section{GPIO Device}\label{sec:Device Types / GPIO Device} + +The Virtio GPIO device is a virtual General Purpose Input/Output device that +supports a variable number of named I/O lines, which can be configured in input +mode or in output mode with logical level low (0) or high (1). + +\subsection{Device ID}\label{sec:Device Types / GPIO Device / Device ID} +41 + +\subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues} + +\begin{description} +\item[0] requestq +\end{description} + +\subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits} + +None currently defined. + +\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout} + +GPIO device uses the following configuration structure layout: + +\begin{lstlisting} +struct virtio_gpio_config { + le16 ngpio; + 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_LINE_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; +}; + +/* Possible values of the status field */ +#define VIRTIO_GPIO_STATUS_OK 0x0 +#define VIRTIO_GPIO_STATUS_ERR 0x1 +\end{lstlisting} + +All the fields of this structure are set by the device and read by the driver. + +\begin{description} +\item[\field{status}] of the GPIO message, + \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR} + on failure. + +\item[\field{value}] is a message specific value. +\end{description} + +Following is the list of messages supported by the virtio gpio specification. + +\begin{lstlisting} +/* GPIO message types */ +#define VIRTIO_GPIO_MSG_GET_LINE_NAMES 0x0001 +#define VIRTIO_GPIO_MSG_GET_DIRECTION 0x0002 +#define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003 +#define VIRTIO_GPIO_MSG_GET_VALUE 0x0004 +#define VIRTIO_GPIO_MSG_SET_VALUE 0x0005 + +/* GPIO Direction types */ +#define VIRTIO_GPIO_DIRECTION_NONE 0x00 +#define VIRTIO_GPIO_DIRECTION_OUT 0x01 +#define VIRTIO_GPIO_DIRECTION_IN 0x02 +\end{lstlisting} + +\subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names} + +The driver sends this message to receive a stream of zero-terminated strings, +where each string represents the name of a GPIO line, present in increasing +order of the GPIO line numbers. The names of the GPIO lines are optional and may +be present only for a subset of GPIO lines. If missing, then a zero-byte must be +present for the GPIO line. If present, the name string must be zero-terminated +and the name must be unique within a GPIO Device. + +These names of the GPIO lines should be most meaningful producer names for the +system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd" +and "ethernet reset" are reasonable line names as they describe what the line is +used for, while "GPIO0" is not a good name to give to a GPIO line. + +Here is an example of how the gpio names memory block may look like for a GPIO +device with 10 GPIO lines, where line names are provided only for lines 0 +("MMC-CD"), 5 ("Red LED Vdd") and 7 ("ethernet reset"). + +\begin{lstlisting} +u8 gpio_names[] = { + 'M', 'M', 'C', '-', 'C', 'D', 0, + 0, + 0, + 0, + 0, + 'R', 'e', 'd', ' ', 'L', 'E', 'D', ' ', 'V', 'd', 'd', 0, + 0, + 'E', 't', 'h', 'e', 'r', 'n', 'e', 't', ' ', 'r', 'e', 's', 'e', 't', 0, + 0, + 0 +}; +\end{lstlisting} + +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. + +This message type uses different layout for the response structure, as the +device needs to return the \field{gpio_names} array. + +\begin{lstlisting} +struct virtio_gpio_response_N { + u8 status; + u8 value[N]; +}; +\end{lstlisting} + +The driver must allocate the \field{value[N]} buffer in the \field{struct +virtio_gpio_response_N} 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_LINE_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: 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| } +\hline +\textbf{Response} & \field{status} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 = none, 1 = output, 2 = input \ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Set Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Set Direction} + +The driver sends this message to request the device to configure a line's +direction. The driver can either set the direction to +\field{VIRTIO_GPIO_DIRECTION_IN} or \field{VIRTIO_GPIO_DIRECTION_OUT}, which +also activates the line, or to \field{VIRTIO_GPIO_DIRECTION_NONE}, which +deactivates the line. + +The driver MUST set the value of the GPIO line, using the +\field{VIRTIO_GPIO_MSG_SET_VALUE} message, before setting the direction of the +line to output. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_MSG_SET_DIRECTION} & line number & 0 = none, 1 = output, 2 = input \ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X| } +\hline +\textbf{Response} & \field{status} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 \ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: 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. + +\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| } +\hline +\textbf{Response} & \field{status} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 = low, 1 = high \ +\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. +The line may already be configured for output or may get configured to output +later, at which point this output value must be used by the device for the line. + +\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| } +\hline +\textbf{Response} & \field{status} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 \ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow} + +\begin{itemize} +\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 send multiple messages in parallel for same or different + 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 MAY send multiple messages for same or different GPIO lines in + parallel. +\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 (if available) within the GPIO + device. + +\item The device MUST process multiple messages, for the same GPIO line, + sequentially and respond to them in the order they were received on the + virtqueue. + +\item The device MAY process messages, for different GPIO lines, out of order + and in parallel, and MAY send message's response to the driver out of order. + +\item The device MUST discard all state information corresponding to a GPIO + line, once the driver has requested to set its direction to + \field{VIRTIO_GPIO_DIRECTION_NONE}. +\end{itemize}
On Fri, Jul 30 2021, 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.
Note that the current implementation doesn't provide atomic APIs for GPIO configurations. i.e. the driver (guest) would need to implement sleep-able versions of the APIs as the guest will respond asynchronously over the virtqueue.
This patch adds the specification for it.
Based on the initial work posted by: "Enrico Weigelt, metux IT consult" lkml@metux.net.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110 Reviewed-by: Arnd Bergmann arnd@arndb.de Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
conformance.tex | 28 +++- content.tex | 1 + virtio-gpio.tex | 346 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 371 insertions(+), 4 deletions(-) create mode 100644 virtio-gpio.tex
Sorry for being late, but I do have a few minor nits from a spec standpoint.
diff --git a/virtio-gpio.tex b/virtio-gpio.tex new file mode 100644 index 000000000000..1d1ac672db37 --- /dev/null +++ b/virtio-gpio.tex @@ -0,0 +1,346 @@ +\section{GPIO Device}\label{sec:Device Types / GPIO Device}
+The Virtio GPIO device is a virtual General Purpose Input/Output device that +supports a variable number of named I/O lines, which can be configured in input +mode or in output mode with logical level low (0) or high (1).
+\subsection{Device ID}\label{sec:Device Types / GPIO Device / Device ID} +41
+\subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
+\begin{description} +\item[0] requestq +\end{description}
+\subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
+None currently defined.
+\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
+GPIO device uses the following configuration structure layout:
+\begin{lstlisting} +struct virtio_gpio_config {
- le16 ngpio;
- 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.
This is not a conformance section, so you can't use 'MUST'. Maybe "This is set to zero by the device." ? If it is a hard requirement, it might need a conformance entry.
+\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_LINE_NAMES} message. The device MUST set this to
- 0 if it doesn't support names for the GPIO lines.
Same here.
+\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.
Also outside of conformance sections. Maybe "The driver configures and initializes..." ? I don't think that one requires a normative statement.
+\end{itemize}
+\subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation}
+The driver uses the \field{requestq} virtqueue to send messages to the device. +The driver sends a pair of buffers, request (filled by driver) and response (to +be filled by device later), to the device. The device in turn fills the response +buffer and sends it back to the driver.
+\begin{lstlisting} +struct virtio_gpio_request {
- le16 type;
- le16 gpio;
- le32 value;
+}; +\end{lstlisting}
+All the fields of this structure are set by the driver and read by the device.
+\begin{description} +\item[\field{type}] is the GPIO message type, i.e. one of
- \field{VIRTIO_GPIO_MSG_*} values.
+\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
- \field{ngpio}.
+\item[\field{value}] is a message specific value. +\end{description}
+\begin{lstlisting} +struct virtio_gpio_response {
- u8 status;
- u8 value;
+};
+/* Possible values of the status field */ +#define VIRTIO_GPIO_STATUS_OK 0x0 +#define VIRTIO_GPIO_STATUS_ERR 0x1 +\end{lstlisting}
+All the fields of this structure are set by the device and read by the driver.
+\begin{description} +\item[\field{status}] of the GPIO message,
- \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR}
- on failure.
+\item[\field{value}] is a message specific value. +\end{description}
+Following is the list of messages supported by the virtio gpio specification.
+\begin{lstlisting} +/* GPIO message types */ +#define VIRTIO_GPIO_MSG_GET_LINE_NAMES 0x0001 +#define VIRTIO_GPIO_MSG_GET_DIRECTION 0x0002 +#define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003 +#define VIRTIO_GPIO_MSG_GET_VALUE 0x0004 +#define VIRTIO_GPIO_MSG_SET_VALUE 0x0005
+/* GPIO Direction types */ +#define VIRTIO_GPIO_DIRECTION_NONE 0x00 +#define VIRTIO_GPIO_DIRECTION_OUT 0x01 +#define VIRTIO_GPIO_DIRECTION_IN 0x02 +\end{lstlisting}
+\subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names}
+The driver sends this message to receive a stream of zero-terminated strings, +where each string represents the name of a GPIO line, present in increasing +order of the GPIO line numbers. The names of the GPIO lines are optional and may +be present only for a subset of GPIO lines. If missing, then a zero-byte must be +present for the GPIO line. If present, the name string must be zero-terminated +and the name must be unique within a GPIO Device.
+These names of the GPIO lines should be most meaningful producer names for the +system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd" +and "ethernet reset" are reasonable line names as they describe what the line is +used for, while "GPIO0" is not a good name to give to a GPIO line.
+Here is an example of how the gpio names memory block may look like for a GPIO +device with 10 GPIO lines, where line names are provided only for lines 0 +("MMC-CD"), 5 ("Red LED Vdd") and 7 ("ethernet reset").
+\begin{lstlisting} +u8 gpio_names[] = {
- 'M', 'M', 'C', '-', 'C', 'D', 0,
- 0,
- 0,
- 0,
- 0,
- 'R', 'e', 'd', ' ', 'L', 'E', 'D', ' ', 'V', 'd', 'd', 0,
- 0,
- 'E', 't', 'h', 'e', 'r', 'n', 'e', 't', ' ', 'r', 'e', 's', 'e', 't', 0,
- 0,
- 0
+}; +\end{lstlisting}
+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.
This should move to the conformance section.
On 06-08-21, 18:02, Cornelia Huck wrote:
On Fri, Jul 30 2021, Viresh Kumar viresh.kumar@linaro.org wrote:
+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization}
+\begin{itemize} +\item The driver MUST configure and initialize the \field{requestq} virtqueue.
Also outside of conformance sections. Maybe "The driver configures and initializes..." ? I don't think that one requires a normative statement.
Not sure I understood it well, but are you suggesting to drop this line altogether ? I wrote it as I saw similar lines/sections for other protocols, I can drop it though.
On Mon, Aug 09 2021, Viresh Kumar viresh.kumar@linaro.org wrote:
On 06-08-21, 18:02, Cornelia Huck wrote:
On Fri, Jul 30 2021, Viresh Kumar viresh.kumar@linaro.org wrote:
+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization}
+\begin{itemize} +\item The driver MUST configure and initialize the \field{requestq} virtqueue.
Also outside of conformance sections. Maybe "The driver configures and initializes..." ? I don't think that one requires a normative statement.
Not sure I understood it well, but are you suggesting to drop this line altogether ? I wrote it as I saw similar lines/sections for other protocols, I can drop it though.
No, just rewrite it as an informational text (i.e. get rid of the 'MUST', which only belongs into normative statements.)
This patch adds support for interrupts to the virtio-gpio specification. This uses the feature bit 0 for the same.
Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- conformance.tex | 2 + virtio-gpio.tex | 234 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 235 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 1d1ac672db37..d5746fa883c0 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 If the \field{VIRTIO_GPIO_F_IRQ} feature is supported, then the interrupt + for all GPIO lines must be in \field{VIRTIO_GPIO_IRQ_TYPE_NONE} state. \end{itemize}
\subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation} @@ -105,11 +120,20 @@ \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / r #define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003 #define VIRTIO_GPIO_MSG_GET_VALUE 0x0004 #define VIRTIO_GPIO_MSG_SET_VALUE 0x0005 +#define VIRTIO_GPIO_MSG_SET_IRQ_TYPE 0x0006
/* GPIO Direction types */ #define VIRTIO_GPIO_DIRECTION_NONE 0x00 #define VIRTIO_GPIO_DIRECTION_OUT 0x01 #define VIRTIO_GPIO_DIRECTION_IN 0x02 + +/* GPIO interrupt types */ +#define VIRTIO_GPIO_IRQ_TYPE_NONE 0x00 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING 0x01 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING 0x02 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH 0x03 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH 0x04 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW 0x08 \end{lstlisting}
\subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names} @@ -269,6 +293,66 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi \hline \end{tabularx}
+\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type} + +This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is enabled +by the device. + +In order to configure and receive interrupts for a GPIO line, the driver needs +to perform two operations. The driver first needs to send the +\field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the \field{requestq} +virtqueue, and then queue a pair of buffers, of type +\field{virtio_gpio_irq_request} and \field{virtio_gpio_irq_response}, over the +\field{eventq} virtqueue for the GPIO line. A separate pair of buffers must be +queued for each GPIO line, the driver wants to configure for interrupts. + +The driver sets the trigger type to values other than +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, to unmask the interrupt and get notified over +the \field{eventq} virtqueue. The driver sets the trigger type to +\field{VIRTIO_GPIO_IRQ_TYPE_NONE} to mask the interrupt, and get back the unused +buffers over the \field{eventq} virtqueue. + +Once the buffers are made available by the driver over the \field{eventq} +virtqueue, the device can notify the driver of an active interrupt signaled on +the GPIO line by returning the buffers to the driver. + +If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST +mask the interrupt for the GPIO line and discard any latched interrupt event +associated with it. The device MUST also return any unused pair of buffers +for the GPIO line, over the \field{eventq} virtqueue, after setting the +\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. If the driver +queues another pair of buffers, while the trigger type remains set as +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of +type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers. + +The device MUST unmask the interrupt for a GPIO line, if the trigger type +received from driver is any valid value other than +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}. For edge trigger type, the device MUST latch +the interrupt event from this point onward and report it to the driver as soon +as a buffers are available. For level trigger type, the device MUST ignore the +active interrupts signaled on the line, until the time the buffers are made +available by the driver. The device MUST keep on notifying the driver of the +interrupts, as and when an interrupt becomes active and the buffers are +available on the \field{eventq} virtqueue. The device MUST stop notifying the +driver, once the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, and +return any unused buffers. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X| } +\hline +\textbf{Response} & \field{status} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 \ +\hline +\end{tabularx} + \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow}
\begin{itemize} @@ -312,6 +396,16 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
\item The driver MAY send multiple messages for same or different GPIO lines in parallel. + +\item The driver MUST NOT send IRQ messages for a GPIO line configured for + output. + +\item The driver MUST NOT send IRQ messages if the \field{VIRTIO_GPIO_F_IRQ} + feature is not enabled by the device. + +\item The driver SHOULD set the IRQ trigger type to + \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line, + previously configured for a different trigger type. \end{itemize}
\devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation} @@ -344,3 +438,141 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D line, once the driver has requested to set its direction to \field{VIRTIO_GPIO_DIRECTION_NONE}. \end{itemize} + +\subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eventq Operation} + +The \field{eventq} virtqueue is used for sending interrupt events from the +device to the driver. The driver queues a separate pair of buffers, +\field{struct virtio_gpio_irq_request} (filled by driver) and \field{struct +virtio_gpio_irq_response} (to be filled by device later), to the \field{eventq} +virtqueue for each GPIO line. The device, on sensing an active interrupt, +returns the pair of buffers of the respective GPIO line for which the interrupt +is active. + +\begin{lstlisting} +struct virtio_gpio_irq_request { + le16 gpio; +}; +\end{lstlisting} + +This structure is filled by the driver and read by the device. + +\begin{description} +\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} < + \field{ngpio}. +\end{description} + +\begin{lstlisting} +struct virtio_gpio_irq_response { + u8 status; +}; + +/* Possible values of the interrupt status field */ +#define VIRTIO_GPIO_IRQ_STATUS_INVALID 0x0 +#define VIRTIO_GPIO_IRQ_STATUS_VALID 0x1 +\end{lstlisting} + +This structure is filled by the device and read by the driver. + +\begin{description} +\item[\field{status}] of the interrupt event, + \field{VIRTIO_GPIO_IRQ_STATUS_VALID} on valid interrupt and + \field{VIRTIO_GPIO_IRQ_STATUS_INVALID} otherwise on returning the buffer + back to the driver without an interrupt. +\end{description} + +\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow} + +\begin{itemize} +\item The driver is requested by a client driver to enable interrupt for a GPIO + line and configure it to a particular trigger type. + +\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the + \field{requestq} virtqueue and the device configures the GPIO line for the + requested trigger type and unmasks the interrupt. + +\item The driver queues a pair of buffers, interrupt-request and + interrupt-response, to the \field{eventq} virtqueue for the GPIO line. + +\item The driver notifies the device of the presence of new buffers on the + \field{eventq} virtqueue. + +\item The interrupt is fully configured at this point. + +\item The device, on sensing an active interrupt on the GPIO line, finds the + matching buffers (based on GPIO line number) from the \field{eventq} + virtqueue and fills its \field{struct virtio_gpio_irq_response} buffer's + \field{status} with \field{VIRTIO_GPIO_IRQ_STATUS_VALID} and returns the + pair of buffers to the device. + +\item The device notifies the driver of the presence of returned buffers on the + \field{eventq} virtqueue. + +\item If the GPIO line is configured for level interrupts, the device MUST + ignore an active interrupt signaled on this GPIO line, until the time the + buffers are made available again by the driver. Once the buffers are + available again, and the interrupt on the line is still active, the device + MUST notify the driver again of an interrupt event. + +\item If the GPIO line is configured for edge interrupts, the device MUST latch + the latest interrupt received for this GPIO line, until the time the buffers + are made available again by the driver. At that point, the device MUST + notify the driver if an interrupt was received while the device was waiting + for the buffers to be made available by the driver. If the interrupt becomes + active at a later point of time, then the device MUST notify the driver at + that instance. + +\item The driver on receiving the notification from the device, processes the + interrupt. The driver may try to update the trigger-type of the interrupt + for the GPIO line over the \field{requestq} virtqueue at this point. + +\item In a typical guest operating system kernel, the virtio-gpio driver + notifies the client driver, that is associated with this gpio line, to + process the event. + +\item In the case of a level triggered interrupt, the client driver must fully + process and acknowledge the event at its source to return the line to its + inactive state. + +\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_SET_IRQ_TYPE} message, with + \field{VIRTIO_GPIO_IRQ_TYPE_NONE} trigger type, 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_STATUS_INVALID}. + +\item The driver can then free the associated buffers. +\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 is not + configured for interrupt at the device, with a previous message of type + \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} with trigger type other than + \field{VIRTIO_GPIO_IRQ_TYPE_NONE}. + +\item The driver MUST NOT add multiple pairs of buffers for the same GPIO line + on the \field{eventq} virtqueue. 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 over the + \field{eventq} virtqueue and the interrupt for that line is unmasked. +\end{itemize}
On Fri, Jul 30, 2021 at 8:45 AM 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.
Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I think the flow you describe now addresses all the important concerns I had. I feel the text you use additional copy-editing, but I'm not a native English speaker myself and not qualified for that. Hope someone else can help out with that.
A few more comments on some details:
+If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST +mask the interrupt for the GPIO line and discard any latched interrupt event +associated with it. The device MUST also return any unused pair of buffers +for the GPIO line, over the \field{eventq} virtqueue, after setting the +\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. If the driver +queues another pair of buffers, while the trigger type remains set as +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of +type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers.
We discussed this last part before, and you had already said you prefer this ordering, but after you explain it here, the interaction of VIRTIO_GPIO_MSG_SET_IRQ_TYPE with queued buffers still seems inconsistent to me: If setting the type to none cancels any outstanding eventq request, why do newly queued eventq request get added to the queue? If we change the order to always require the type to be enabled before queuing any events, this inconsistency would go away. With the behavior of the level triggered interrupts changed to fasteoi, do you still see a need to keep this ordering?
+\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}
+\begin{itemize} +\item The driver is requested by a client driver to enable interrupt for a GPIO
- line and configure it to a particular trigger type.
+\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the
- \field{requestq} virtqueue and the device configures the GPIO line for the
- requested trigger type and unmasks the interrupt.
+\item The driver queues a pair of buffers, interrupt-request and
- interrupt-response, to the \field{eventq} virtqueue for the GPIO line.
+\item The driver notifies the device of the presence of new buffers on the
- \field{eventq} virtqueue.
+\item The interrupt is fully configured at this point.
+\item The device, on sensing an active interrupt on the GPIO line, finds the
- matching buffers (based on GPIO line number) from the \field{eventq}
- virtqueue and fills its \field{struct virtio_gpio_irq_response} buffer's
- \field{status} with \field{VIRTIO_GPIO_IRQ_STATUS_VALID} and returns the
- pair of buffers to the device.
+\item The device notifies the driver of the presence of returned buffers on the
- \field{eventq} virtqueue.
+\item If the GPIO line is configured for level interrupts, the device MUST
It would be nice to separate the normative text (using MUST etc) from the section describing the message flow.
+\item In a typical guest operating system kernel, the virtio-gpio driver
- notifies the client driver, that is associated with this gpio line, to
- process the event.
+\item In the case of a level triggered interrupt, the client driver must fully
- process and acknowledge the event at its source to return the line to its
- inactive state.
+\item The driver may again queue, same or new, pair of buffers for that GPIO
- line and notify the device.
I suggested this bit, but on second thought it still doesn't quite capture the important bit that the 'level' line has to be set to low /before/ the new buffer gets queued (to avoid a spurious notification). Otherwise there is no difference between edge and level interrupts, obviously in both cases the interrupt has to be processed eventually.
Arnd
On 30-07-21, 10:08, Arnd Bergmann wrote:
On Fri, Jul 30, 2021 at 8:45 AM Viresh Kumar viresh.kumar@linaro.org wrote:
+If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST +mask the interrupt for the GPIO line and discard any latched interrupt event +associated with it. The device MUST also return any unused pair of buffers +for the GPIO line, over the \field{eventq} virtqueue, after setting the +\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. If the driver +queues another pair of buffers, while the trigger type remains set as +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of +type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers.
We discussed this last part before, and you had already said you prefer this ordering, but after you explain it here, the interaction of VIRTIO_GPIO_MSG_SET_IRQ_TYPE with queued buffers still seems inconsistent to me: If setting the type to none cancels any outstanding eventq request, why do newly queued eventq request get added to the queue? If we change the order to always require the type to be enabled before queuing any events, this inconsistency would go away. With the behavior of the level triggered interrupts changed to fasteoi, do you still see a need to keep this ordering?
If you look at the message-flow sequence, I did change it to how you described above, i.e. the driver enables the interrupt first and then queue the message.
I wasn't sure if we wanted to force it entirely and so kept it like this. But I think it may be fine that way, so I will change this to mention that if buffers are received while the interrupt is disabled, then the device should just return them back unused. That will just make the expectation clear for both device and driver in this case.
+\item In a typical guest operating system kernel, the virtio-gpio driver
- notifies the client driver, that is associated with this gpio line, to
- process the event.
+\item In the case of a level triggered interrupt, the client driver must fully
- process and acknowledge the event at its source to return the line to its
- inactive state.
+\item The driver may again queue, same or new, pair of buffers for that GPIO
- line and notify the device.
I suggested this bit, but on second thought it still doesn't quite capture the important bit that the 'level' line has to be set to low /before/ the new buffer gets queued (to avoid a spurious notification).
I find the second item above say exactly the same thing. It says for level triggered interrupted, line must return to inactive state.
Otherwise there is no difference between edge and level interrupts, obviously in both cases the interrupt has to be processed eventually.
On Fri, 30 Jul 2021 07:45:03 +0100, 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.
Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
conformance.tex | 2 + virtio-gpio.tex | 234 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 235 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 1d1ac672db37..d5746fa883c0 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 If the \field{VIRTIO_GPIO_F_IRQ} feature is supported, then the interrupt
- for all GPIO lines must be in \field{VIRTIO_GPIO_IRQ_TYPE_NONE} state.
\end{itemize} \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation} @@ -105,11 +120,20 @@ \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / r #define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003 #define VIRTIO_GPIO_MSG_GET_VALUE 0x0004 #define VIRTIO_GPIO_MSG_SET_VALUE 0x0005 +#define VIRTIO_GPIO_MSG_SET_IRQ_TYPE 0x0006 /* GPIO Direction types */ #define VIRTIO_GPIO_DIRECTION_NONE 0x00 #define VIRTIO_GPIO_DIRECTION_OUT 0x01 #define VIRTIO_GPIO_DIRECTION_IN 0x02
+/* GPIO interrupt types */ +#define VIRTIO_GPIO_IRQ_TYPE_NONE 0x00 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING 0x01 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING 0x02 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH 0x03 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH 0x04 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW 0x08 \end{lstlisting} \subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names} @@ -269,6 +293,66 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi \hline \end{tabularx} +\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type}
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is enabled +by the device.
+In order to configure and receive interrupts for a GPIO line, the driver needs +to perform two operations. The driver first needs to send the +\field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the \field{requestq} +virtqueue, and then queue a pair of buffers, of type +\field{virtio_gpio_irq_request} and \field{virtio_gpio_irq_response}, over the +\field{eventq} virtqueue for the GPIO line. A separate pair of buffers must be +queued for each GPIO line, the driver wants to configure for interrupts.
+The driver sets the trigger type to values other than +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, to unmask the interrupt and get notified over +the \field{eventq} virtqueue. The driver sets the trigger type to +\field{VIRTIO_GPIO_IRQ_TYPE_NONE} to mask the interrupt, and get back the unused +buffers over the \field{eventq} virtqueue.
+Once the buffers are made available by the driver over the \field{eventq} +virtqueue, the device can notify the driver of an active interrupt signaled on +the GPIO line by returning the buffers to the driver.
+If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST +mask the interrupt for the GPIO line and discard any latched interrupt event +associated with it. The device MUST also return any unused pair of buffers +for the GPIO line, over the \field{eventq} virtqueue, after setting the +\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. If the driver +queues another pair of buffers, while the trigger type remains set as +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of +type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers.
+The device MUST unmask the interrupt for a GPIO line, if the trigger type +received from driver is any valid value other than +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}. For edge trigger type, the device MUST latch +the interrupt event from this point onward and report it to the driver as soon +as a buffers are available. For level trigger type, the device MUST ignore the +active interrupts signaled on the line, until the time the buffers are made +available by the driver. The device MUST keep on notifying the driver of the +interrupts, as and when an interrupt becomes active and the buffers are +available on the \field{eventq} virtqueue. The device MUST stop notifying the +driver, once the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, and +return any unused buffers.
+\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \ +\hline +\end{tabularx}
+\begin{tabularx}{\textwidth}{ |l||X|X| } +\hline +\textbf{Response} & \field{status} & \field{value} \ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 \ +\hline +\end{tabularx}
\subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow} \begin{itemize} @@ -312,6 +396,16 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D \item The driver MAY send multiple messages for same or different GPIO lines in parallel.
+\item The driver MUST NOT send IRQ messages for a GPIO line configured for
- output.
+\item The driver MUST NOT send IRQ messages if the \field{VIRTIO_GPIO_F_IRQ}
- feature is not enabled by the device.
+\item The driver SHOULD set the IRQ trigger type to
- \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
- previously configured for a different trigger type.
\end{itemize} \devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation} @@ -344,3 +438,141 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D line, once the driver has requested to set its direction to \field{VIRTIO_GPIO_DIRECTION_NONE}. \end{itemize}
+\subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eventq Operation}
+The \field{eventq} virtqueue is used for sending interrupt events from the +device to the driver. The driver queues a separate pair of buffers, +\field{struct virtio_gpio_irq_request} (filled by driver) and \field{struct +virtio_gpio_irq_response} (to be filled by device later), to the \field{eventq} +virtqueue for each GPIO line. The device, on sensing an active interrupt, +returns the pair of buffers of the respective GPIO line for which the interrupt +is active.
+\begin{lstlisting} +struct virtio_gpio_irq_request {
- le16 gpio;
+}; +\end{lstlisting}
+This structure is filled by the driver and read by the device.
+\begin{description} +\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
- \field{ngpio}.
+\end{description}
+\begin{lstlisting} +struct virtio_gpio_irq_response {
- u8 status;
+};
+/* Possible values of the interrupt status field */ +#define VIRTIO_GPIO_IRQ_STATUS_INVALID 0x0 +#define VIRTIO_GPIO_IRQ_STATUS_VALID 0x1 +\end{lstlisting}
+This structure is filled by the device and read by the driver.
+\begin{description} +\item[\field{status}] of the interrupt event,
- \field{VIRTIO_GPIO_IRQ_STATUS_VALID} on valid interrupt and
- \field{VIRTIO_GPIO_IRQ_STATUS_INVALID} otherwise on returning the buffer
- back to the driver without an interrupt.
+\end{description}
+\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}
+\begin{itemize} +\item The driver is requested by a client driver to enable interrupt for a GPIO
- line and configure it to a particular trigger type.
+\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the
- \field{requestq} virtqueue and the device configures the GPIO line for the
- requested trigger type and unmasks the interrupt.
+\item The driver queues a pair of buffers, interrupt-request and
- interrupt-response, to the \field{eventq} virtqueue for the GPIO line.
+\item The driver notifies the device of the presence of new buffers on the
- \field{eventq} virtqueue.
+\item The interrupt is fully configured at this point.
+\item The device, on sensing an active interrupt on the GPIO line, finds the
- matching buffers (based on GPIO line number) from the \field{eventq}
- virtqueue and fills its \field{struct virtio_gpio_irq_response} buffer's
- \field{status} with \field{VIRTIO_GPIO_IRQ_STATUS_VALID} and returns the
- pair of buffers to the device.
+\item The device notifies the driver of the presence of returned buffers on the
- \field{eventq} virtqueue.
+\item If the GPIO line is configured for level interrupts, the device MUST
- ignore an active interrupt signaled on this GPIO line, until the time the
- buffers are made available again by the driver. Once the buffers are
- available again, and the interrupt on the line is still active, the device
- MUST notify the driver again of an interrupt event.
It feels like there is a problem here. A virtio device signals interrupts by using edge triggered signalling. Nothing wrong with that. However, signalling a level over a an edge signalling is much more tricky.
For example, let's assume that the irqchip driver handles a level interrupt: it gets the message from the device indicating that a GPIO level interrupt is pending. During the handling, the interrupt is made pending again, without having ever transitioned via an inactive state. If you don't have a mechanism to retrigger the level, you have lost an interrupt.
I can't see anything in this document that hints at a way to resample/retrigger a level interrupt, which is what you would normally do on EOI for an interrupt controller that implements level-over-edge signalling.
+\item If the GPIO line is configured for edge interrupts, the device MUST latch
- the latest interrupt received for this GPIO line, until the time the buffers
- are made available again by the driver. At that point, the device MUST
- notify the driver if an interrupt was received while the device was waiting
- for the buffers to be made available by the driver. If the interrupt becomes
- active at a later point of time, then the device MUST notify the driver at
- that instance.
+\item The driver on receiving the notification from the device, processes the
- interrupt. The driver may try to update the trigger-type of the interrupt
- for the GPIO line over the \field{requestq} virtqueue at this point.
+\item In a typical guest operating system kernel, the virtio-gpio driver
- notifies the client driver, that is associated with this gpio line, to
- process the event.
+\item In the case of a level triggered interrupt, the client driver must fully
- process and acknowledge the event at its source to return the line to its
- inactive state.
This is not enough. Take a timer, for example: the interrupt fires, and the handler directly reprograms it so that it fires again immediately. The line never went down, and yet the interrupt has been handled. This isn't an imaginary case. This happens *all the time*.
Thanks,
M.
On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier maz@kernel.org wrote:
On Fri, 30 Jul 2021 07:45:03 +0100, Viresh Kumar viresh.kumar@linaro.org wrote:
+\item If the GPIO line is configured for level interrupts, the device MUST
- ignore an active interrupt signaled on this GPIO line, until the time the
- buffers are made available again by the driver. Once the buffers are
- available again, and the interrupt on the line is still active, the device
- MUST notify the driver again of an interrupt event.
It feels like there is a problem here. A virtio device signals interrupts by using edge triggered signalling. Nothing wrong with that. However, signalling a level over a an edge signalling is much more tricky.
For example, let's assume that the irqchip driver handles a level interrupt: it gets the message from the device indicating that a GPIO level interrupt is pending. During the handling, the interrupt is made pending again, without having ever transitioned via an inactive state. If you don't have a mechanism to retrigger the level, you have lost an interrupt.
I can't see anything in this document that hints at a way to resample/retrigger a level interrupt, which is what you would normally do on EOI for an interrupt controller that implements level-over-edge signalling.
Thanks a lot for taking a closer look. I still hope this is just a problem that needs to be clarified in the description, not something wrong with the design, as I don't see the problem yet. As far as I can tell, this is different from an edge interrupt, since the eventq communication is really a two-way message.
The EOI for the level interrupt is the message being enqueued after the guest is done processing the interrupt. I can see four ways this could go:
1. If the interrupt has not been made pending again yet, nothing happens until it eventually becomes pending again (this is the obvious case)
2. If it is already pending, chip->irq_eoi() causes the new event descriptor to be queued on the eventq, and it signals the host about new buffers being available. The /host/ samples the level of the line, notices it is pending and puts the resulting buffer back on the 'used' queue even before returning from the guest 'notify' function. The guest virtio core code keeps processing the 'used' buffers and gets back into the interrupt handler.
3. Same as 2., but the host handles the virtqueue ->notify asynchronously, and the guest has already checked the 'used' queue before the host adds back the buffer to tell it that the line is still active. Now the guest gets notified again after it returns from the virtqueue interrupt, in order to process the new 'used' buffer.
4. The gpio line actually goes into inactive state until after the new event is queued in chip->irq_eoi(), but becomes active immediately afterwards. Now the host gets interrupted and handles this by queuing the event reply but cannot interrupt the guest because it is still processing the original virtqueue event. However, since the event is queued, it will be processed the same way as 2. or 3. by the virtio core.
Do you see a problem with scenarios 2, 3 or 4, or with another case that I may have missed?
[This all assumes that the host is able to atomically enable interrupts and check the current level when processing the new buffer. If the host uses the Linux gpiolib ioctl interface, this means it has to register for an edge event on the line first, and then read the current value before adding the file descriptor to its poll table. I feel this is out of the scope of the virtio spec though.]
Arnd
On Fri, 30 Jul 2021 11:05:30 +0100, Arnd Bergmann arnd@kernel.org wrote:
On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier maz@kernel.org wrote:
On Fri, 30 Jul 2021 07:45:03 +0100, Viresh Kumar viresh.kumar@linaro.org wrote:
+\item If the GPIO line is configured for level interrupts, the device MUST
- ignore an active interrupt signaled on this GPIO line, until the time the
- buffers are made available again by the driver. Once the buffers are
- available again, and the interrupt on the line is still active, the device
- MUST notify the driver again of an interrupt event.
It feels like there is a problem here. A virtio device signals interrupts by using edge triggered signalling. Nothing wrong with that. However, signalling a level over a an edge signalling is much more tricky.
For example, let's assume that the irqchip driver handles a level interrupt: it gets the message from the device indicating that a GPIO level interrupt is pending. During the handling, the interrupt is made pending again, without having ever transitioned via an inactive state. If you don't have a mechanism to retrigger the level, you have lost an interrupt.
I can't see anything in this document that hints at a way to resample/retrigger a level interrupt, which is what you would normally do on EOI for an interrupt controller that implements level-over-edge signalling.
Thanks a lot for taking a closer look. I still hope this is just a problem that needs to be clarified in the description, not something wrong with the design, as I don't see the problem yet. As far as I can tell, this is different from an edge interrupt, since the eventq communication is really a two-way message.
I disagree with this description. The signalling is definitely edge (it is an event, not a change of state). It actually is a two-way edge signalling. Nothing wrong with that, but all the pitfalls of the edge signalling do apply.
The EOI for the level interrupt is the message being enqueued after the guest is done processing the interrupt. I can see four ways this could go:
- If the interrupt has not been made pending again yet, nothing happens
until it eventually becomes pending again (this is the obvious case)
- If it is already pending, chip->irq_eoi() causes the new event descriptor
to be queued on the eventq, and it signals the host about new buffers being available. The /host/ samples the level of the line, notices it is pending and puts the resulting buffer back on the 'used' queue even before returning from the guest 'notify' function. The guest virtio core code keeps processing the 'used' buffers and gets back into the interrupt handler.
- Same as 2., but the host handles the virtqueue ->notify asynchronously,
and the guest has already checked the 'used' queue before the host adds back the buffer to tell it that the line is still active. Now the guest gets notified again after it returns from the virtqueue interrupt, in order to process the new 'used' buffer.
- The gpio line actually goes into inactive state until after the new event is queued in chip->irq_eoi(), but becomes active immediately afterwards. Now the host gets interrupted and handles this by queuing the event reply but cannot interrupt the guest because it is still processing the original virtqueue event. However, since the event is queued, it will be processed the same way as 2. or 3. by the virtio core.
Do you see a problem with scenarios 2, 3 or 4, or with another case that I may have missed?
Thanks, that's really useful. I don't think you missed much. What the documentation is missing though is an actual interrupt controller specification. Although it describes the protocol in great length, at no point does it explain what the irq_response does in terms of interrupt life cycle (there is no interrupt life cycle at all). Same goes for the various states that an interrupt can get. As someone who spends way too much time reading interrupt controller specs, this is a first class defect.
Another point I have just realised is that this spec confuses interrupt mask with interrupt enable. It describes masking interrupts as an effect of setting the trigger type, but that's completely unusable. There is a strong expectation from SW that a masked interrupt doesn't loose signals. If the interrupt is masked by setting its trigger to NONE, then an edge interrupt coming at this point will be lost. No cookie for you.
I'm fine with this odd way of *enabling* the interrupt, but masking cannot lose any signal, ever.
Another unclear aspect is how you switch from one trigger type to another. Do you have to transition via NONE? I have the strong feeling that you should if you want to be robust against spurious signals.
[This all assumes that the host is able to atomically enable interrupts and check the current level when processing the new buffer. If the host uses the Linux gpiolib ioctl interface, this means it has to register for an edge event on the line first, and then read the current value before adding the file descriptor to its poll table. I feel this is out of the scope of the virtio spec though.]
There are certainly a number of implementation difficulties with this. At this stage, I'm more worried about the guest-visible aspects so far, but I guess that I should also look at the host side at some point.
Is there any sample code we could look at, both got guest and host?
Thanks,
M.
On Sat, Jul 31, 2021 at 11:31 AM Marc Zyngier maz@kernel.org wrote:
On Fri, 30 Jul 2021 11:05:30 +0100, Arnd Bergmann arnd@kernel.org wrote:
On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier maz@kernel.org wrote:
The EOI for the level interrupt is the message being enqueued after the guest is done processing the interrupt. I can see four ways this could go:
- If the interrupt has not been made pending again yet, nothing happens
until it eventually becomes pending again (this is the obvious case)
- If it is already pending, chip->irq_eoi() causes the new event descriptor
to be queued on the eventq, and it signals the host about new buffers being available. The /host/ samples the level of the line, notices it is pending and puts the resulting buffer back on the 'used' queue even before returning from the guest 'notify' function. The guest virtio core code keeps processing the 'used' buffers and gets back into the interrupt handler.
- Same as 2., but the host handles the virtqueue ->notify asynchronously,
and the guest has already checked the 'used' queue before the host adds back the buffer to tell it that the line is still active. Now the guest gets notified again after it returns from the virtqueue interrupt, in order to process the new 'used' buffer.
- The gpio line actually goes into inactive state until after the new event is queued in chip->irq_eoi(), but becomes active immediately afterwards. Now the host gets interrupted and handles this by queuing the event reply but cannot interrupt the guest because it is still processing the original virtqueue event. However, since the event is queued, it will be processed the same way as 2. or 3. by the virtio core.
Do you see a problem with scenarios 2, 3 or 4, or with another case that I may have missed?
Thanks, that's really useful. I don't think you missed much. What the documentation is missing though is an actual interrupt controller specification. Although it describes the protocol in great length, at no point does it explain what the irq_response does in terms of interrupt life cycle (there is no interrupt life cycle at all). Same goes for the various states that an interrupt can get. As someone who spends way too much time reading interrupt controller specs, this is a first class defect.
Ok, makes sense.
Another point I have just realised is that this spec confuses interrupt mask with interrupt enable. It describes masking interrupts as an effect of setting the trigger type, but that's completely unusable. There is a strong expectation from SW that a masked interrupt doesn't loose signals. If the interrupt is masked by setting its trigger to NONE, then an edge interrupt coming at this point will be lost. No cookie for you.
I'm fine with this odd way of *enabling* the interrupt, but masking cannot lose any signal, ever.
Viresh's previous first version actually had this, I asked him to simplify it in order to reduce the number of possible states, as it seemed to me that it would be better not to have eight possible states when you have the various combinations of enabled/disabled, unmasked/masked and armed/not-armed. The current version folds unmasked/masked into armed/not-armed by masking the interrupt whenever the reply has been queued to the guest, until it gets queued again on the eventq.
This avoids the problem that using the controlq to mask explicitly mask the interrupt cannot be done from atomic context since it has to wait_for_completion() until the controlq message is returned. Queing up the eventq message to unmask the even can be done in atomic context, since this is a posted operation.
What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?
Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.
Another unclear aspect is how you switch from one trigger type to another. Do you have to transition via NONE? I have the strong feeling that you should if you want to be robust against spurious signals.
Good idea.
[This all assumes that the host is able to atomically enable interrupts and check the current level when processing the new buffer. If the host uses the Linux gpiolib ioctl interface, this means it has to register for an edge event on the line first, and then read the current value before adding the file descriptor to its poll table. I feel this is out of the scope of the virtio spec though.]
There are certainly a number of implementation difficulties with this. At this stage, I'm more worried about the guest-visible aspects so far, but I guess that I should also look at the host side at some point.
Is there any sample code we could look at, both got guest and host?
Viresh said that he is close to sending it after updating the code to the latest spec.
Arnd
On 01-08-21, 15:43, Arnd Bergmann wrote:
On Sat, Jul 31, 2021 at 11:31 AM Marc Zyngier maz@kernel.org wrote:
Is there any sample code we could look at, both got guest and host?
Viresh said that he is close to sending it after updating the code to the latest spec.
I am close to sending code for the guest, but it will take time to get it done for the host (I am going to do a Rust implementation for the same).
On Sun, 01 Aug 2021 14:43:37 +0100, Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 31, 2021 at 11:31 AM Marc Zyngier maz@kernel.org wrote:
On Fri, 30 Jul 2021 11:05:30 +0100, Arnd Bergmann arnd@kernel.org wrote:
On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier maz@kernel.org wrote:
The EOI for the level interrupt is the message being enqueued after the guest is done processing the interrupt. I can see four ways this could go:
- If the interrupt has not been made pending again yet, nothing happens
until it eventually becomes pending again (this is the obvious case)
- If it is already pending, chip->irq_eoi() causes the new event descriptor
to be queued on the eventq, and it signals the host about new buffers being available. The /host/ samples the level of the line, notices it is pending and puts the resulting buffer back on the 'used' queue even before returning from the guest 'notify' function. The guest virtio core code keeps processing the 'used' buffers and gets back into the interrupt handler.
- Same as 2., but the host handles the virtqueue ->notify asynchronously,
and the guest has already checked the 'used' queue before the host adds back the buffer to tell it that the line is still active. Now the guest gets notified again after it returns from the virtqueue interrupt, in order to process the new 'used' buffer.
- The gpio line actually goes into inactive state until after the new event is queued in chip->irq_eoi(), but becomes active immediately afterwards. Now the host gets interrupted and handles this by queuing the event reply but cannot interrupt the guest because it is still processing the original virtqueue event. However, since the event is queued, it will be processed the same way as 2. or 3. by the virtio core.
Do you see a problem with scenarios 2, 3 or 4, or with another case that I may have missed?
Thanks, that's really useful. I don't think you missed much. What the documentation is missing though is an actual interrupt controller specification. Although it describes the protocol in great length, at no point does it explain what the irq_response does in terms of interrupt life cycle (there is no interrupt life cycle at all). Same goes for the various states that an interrupt can get. As someone who spends way too much time reading interrupt controller specs, this is a first class defect.
Ok, makes sense.
Another point I have just realised is that this spec confuses interrupt mask with interrupt enable. It describes masking interrupts as an effect of setting the trigger type, but that's completely unusable. There is a strong expectation from SW that a masked interrupt doesn't loose signals. If the interrupt is masked by setting its trigger to NONE, then an edge interrupt coming at this point will be lost. No cookie for you.
I'm fine with this odd way of *enabling* the interrupt, but masking cannot lose any signal, ever.
Viresh's previous first version actually had this, I asked him to simplify it in order to reduce the number of possible states, as it seemed to me that it would be better not to have eight possible states when you have the various combinations of enabled/disabled, unmasked/masked and armed/not-armed.
What exactly is armed/not-armed? Is that equivalent to pending?
The current version folds unmasked/masked into armed/not-armed by masking the interrupt whenever the reply has been queued to the guest, until it gets queued again on the eventq.
Huh. I understood that it was mask end enabled that were folded together. I'm lost.
This avoids the problem that using the controlq to mask explicitly mask the interrupt cannot be done from atomic context since it has to wait_for_completion() until the controlq message is returned. Queing up the eventq message to unmask the even can be done in atomic context, since this is a posted operation.
What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?
irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.
Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.
The later would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.
Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.
That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.
Thanks,
M.
Hi Marc,
On 02-08-21, 10:45, Marc Zyngier wrote:
On Sun, 01 Aug 2021 14:43:37 +0100, Arnd Bergmann arnd@kernel.org wrote:
What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?
irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.
Right, I already have that in place but I have a few doubts on that.
I can see irq_bus_lock/unlock() getting called around irq_mask/unmask/set_type() during request_irq(), and so I can issue all the virtio calls from irq_bus_unlock() there.
But I don't see the same sequence being followed by the irq core when an interrupt actually happens, i.e. when I call generic_handle_irq() from virtqueue callback for eventq vq. In this case, the irq core can directly call mask/unmask() around irq_eoi(), without calling the irq_bus_lock/unlock() guards.
How should the driver take care of these mask/unmask calls, or should it take care of it at all, as they should negate each other eventually ?
Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.
We discussed this sometime back over IRC, and Jean Philippe said this in context of virtio-iommu:
"From some profiling the overhead of context switching is much greater than that of adding buffers, so adding a new virtio interface to bypass the virtqueue might not show any improvement."
We were looking to explore the config space for normal GPIO operations as well then.
The host would be required to perform some action here on a config update and the guest would be waiting over an atomic operation for it to complete. So eventually it should also be slow.
The later would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.
Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.
That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.
Right, irq_bus_unlock() works fine for this though.
On Mon, 02 Aug 2021 11:49:42 +0100, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Marc,
On 02-08-21, 10:45, Marc Zyngier wrote:
On Sun, 01 Aug 2021 14:43:37 +0100, Arnd Bergmann arnd@kernel.org wrote:
What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?
irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.
Right, I already have that in place but I have a few doubts on that.
I can see irq_bus_lock/unlock() getting called around irq_mask/unmask/set_type() during request_irq(), and so I can issue all the virtio calls from irq_bus_unlock() there.
But I don't see the same sequence being followed by the irq core when an interrupt actually happens, i.e. when I call generic_handle_irq() from virtqueue callback for eventq vq. In this case, the irq core can directly call mask/unmask() around irq_eoi(), without calling the irq_bus_lock/unlock() guards.
You can't take that lock from the core irq code, for obvious reasons: you're in IRQ context, and you cannot sleep. You definitely need to use threaded interrupts for this.
How should the driver take care of these mask/unmask calls, or should it take care of it at all, as they should negate each other eventually ?
Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.
We discussed this sometime back over IRC, and Jean Philippe said this in context of virtio-iommu:
"From some profiling the overhead of context switching is much greater than that of adding buffers, so adding a new virtio interface to bypass the virtqueue might not show any improvement."
If you need something to be synchronous, there is not exactly a choice. Masking an interrupt is something that is much better as a synchronous interface, because it leads to tons of further simplifications. And in my book, simplicity is a much easier path to correctness.
We were looking to explore the config space for normal GPIO operations as well then.
The host would be required to perform some action here on a config update and the guest would be waiting over an atomic operation for it to complete. So eventually it should also be slow.
The later would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.
Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.
That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.
Right, irq_bus_unlock() works fine for this though.
To some extent (insert a comment about thrust and flying pigs here). It imposes a lot of restrictions, adds latency, and makes it harder to reason about when what happens when. It also means that you are probably designing for Linux instead of building something more generic.
Having contributed to a PV interrupt architecture in the recent past, I quickly realised that keeping things as simple as possible was a good way to avoid baked-in assumptions. Yes, a synchronous interface results in a few extra traps. But you can also now reason about it and *prove* things. YMMV.
Thanks,
M.
On Mon, Aug 2, 2021 at 11:45 AM Marc Zyngier maz@kernel.org wrote:
On Sun, 01 Aug 2021 14:43:37 +0100, Arnd Bergmann arnd@kernel.org wrote:
On Sat, Jul 31, 2021 at 11:31 AM Marc Zyngier maz@kernel.org wrote:
On Fri, 30 Jul 2021 11:05:30 +0100, Arnd Bergmann arnd@kernel.org wrote:
On Fri, Jul 30, 2021 at 10:52 AM Marc Zyngier maz@kernel.org wrote:
Viresh's previous first version actually had this, I asked him to simplify it in order to reduce the number of possible states, as it seemed to me that it would be better not to have eight possible states when you have the various combinations of enabled/disabled, unmasked/masked and armed/not-armed.
What exactly is armed/not-armed? Is that equivalent to pending?
The current version folds unmasked/masked into armed/not-armed by masking the interrupt whenever the reply has been queued to the guest, until it gets queued again on the eventq.
Huh. I understood that it was mask end enabled that were folded together. I'm lost.
Sorry, it's probably my fault for not knowing the correct terminology.
With 'armed' I mean that the virtio-gpio driver has sent a message to the eventq, which allows the device to reply that an event has happened. If it was already pending and enabled, the device can reply immediately, otherwise the notification will remain in 'armed' state until the host observes the interrupt, and then it sends it back.
After the event is returned to the guest, it is no longer armed, and the host can not send another event since it no longer owns a message buffer for the gpio line, until the guest re-arms it by providing a new message buffer.
This avoids the problem that using the controlq to mask explicitly mask the interrupt cannot be done from atomic context since it has to wait_for_completion() until the controlq message is returned. Queing up the eventq message to unmask the even can be done in atomic context, since this is a posted operation.
What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?
irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.
Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.
The latter would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.
I don't see the code in virtio-console you mention, but I don't think this would work here: the console driver just needs to tell the host that data is available but doesn't care if that makes it there, while the irq mask operation must be sure the host has actually prevented interrupts from getting delivered.
If we need to add an explicit 'mask' operation in the protocol, that would involve queueing a command on either the controlq or eventq, which leads to the already queued eventq buffer to get returned to the guest, but as you say this would add a lot of complexity.
Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.
That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.
What I meant is to only get one notification, and then not re-arm the interrupt by sending the new request.
If we send a separate 'mask' command, then the guest actually gets sent two buffers back: the completion of the 'mask' operation and the eventq buffer. Not sending the command means we get either no buffer back (if no interrupt happens while the line is masked) or one buffer.
Arnd
stratos-dev@op-lists.linaro.org
-
Arnd Bergmann -
Cornelia Huck -
Marc Zyngier -
Viresh Kumar