* [PATCH V6 0/2] virtio: Add specification for virtio-gpio @ 2021-07-20 15:28 Viresh Kumar 2021-07-20 15:28 ` [PATCH V6 1/2] virtio-gpio: Add the device specification Viresh Kumar 2021-07-20 15:28 ` [virtio-dev] [PATCH V6 2/2] virtio-gpio: Add support for interrupts Viresh Kumar 0 siblings, 2 replies; 26+ messages in thread From: Viresh Kumar @ 2021-07-20 15:28 UTC (permalink / raw) To: Jason Wang, Michael S. Tsirkin, Arnd Bergmann, Cornelia Huck, Linus Walleij, Bartosz Golaszewski Cc: Viresh Kumar, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven Hello, This patchset adds virtio specification for GPIO devices. It supports basic GPIO line operations, along with optional interrupts on them (in a separate patch). This is an *alternative implementation* of the virtio-gpio specification, a different version of it was earlier posted by Enrico [1]. I took back V4 of the specification I posted earlier (on June 17th), to allow Enrico to come up with something that is robust and works for everyone (as he started this thing last year), but it didn't go as expected. I couldn't see any of my review comments being incorporated (or any intentions of them getting in ever) in the two versions Enrico posted and a month passed since then. And so I am trying to present an alternative approach here to solve the problem, which I believe is more clear and robust. I am open to suggestions to improve it further. I will let the virtio/gpio maintainers decide on its fate. Key differences from Enrico's approach [1]: - config structure is rearranged to remove everything apart from number of gpios and size of the gpio_names_field. - Supports freeing of a GPIO line after use, we call them activate/deactivate now, which suits their purpose better. - Interrupt implementation handled with feature bit 0. Either the interrupts are fully supported or not at all. - All non-interrupt traffic happens on a single virtqueue, requestq. Interrupt traffic goes over eventq. - Doesn't add any ordering restrictions on the device, it can respond earlier to the second request, while still processing the first one. - Clearly state that two requests can't be initiated for the same line by device or driver. Version history of the specification I sent: V5 -> V6: - All non-interrupt traffic happens on a single virtqueue, requestq. Interrupt traffic goes over eventq now. - Many fields dropped from the config structure. - Separate message type to get gpio-names, added more description about how the names should be. - Much clearer message flows, both non-irq messages and irq-messages. - Parallelism supported for all type of messages, for different GPIO pins. - All references to POSIX errors dropped, just reply pass or fail. - request/free renamed to activate/deactivate, as that's what we will end up doing there, activate or deactivate a GPIO line. - General purpose IO as I/O or Input/Output instead. - Hopefully I didn't miss any review comments. V4 -> V5: - Split into two patches, irq stuff in patch 2. - Use separate virtqueue for sending data from device/driver. - Allow parallel processing of requests for different GPIOs, only one request at a time for the same GPIO line. - Same goes for interrupt side, only one interrupt request per GPIO line. - Improved formatting in general. - Add new sections explaining message flow sequence. V3 -> V4: - The GPIO line names must be unique within a device. - The gpio_names[0] field is dropped and gpio_names_offset field is added in place of the 2 bytes of padding. - New interrupts must not be initiated by the device, without a response for the previous one. V2 -> V3: - Unused char in name string should be marked 0 now. - s/host/device/ and s/guest/driver/ - Added a new feature for IRQ mode, VIRTIO_GPIO_F_IRQ. - A new feature should be added for Version information if required later on. V1 -> V2: - gpio_names_size is 32 bit. - gpio field is 16 bit. - padding added 16 bit. - Added packed attribute to few structures - Add the missing 'type' field to the request - Dropped to _nodata request/responses to simplify a bit, updated related text. -- Viresh [1] https://lists.oasis-open.org/archives/virtio-dev/202106/msg00030.html Viresh Kumar (2): virtio-gpio: Add the device specification virtio-gpio: Add support for interrupts conformance.tex | 30 ++- content.tex | 1 + virtio-gpio.tex | 608 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 635 insertions(+), 4 deletions(-) create mode 100644 virtio-gpio.tex -- 2.31.1.272.g89b43f80a514 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-20 15:28 [PATCH V6 0/2] virtio: Add specification for virtio-gpio Viresh Kumar @ 2021-07-20 15:28 ` Viresh Kumar 2021-07-26 9:18 ` [virtio-dev] " Arnd Bergmann 2021-07-20 15:28 ` [virtio-dev] [PATCH V6 2/2] virtio-gpio: Add support for interrupts Viresh Kumar 1 sibling, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-20 15:28 UTC (permalink / raw) To: Jason Wang, Michael S. Tsirkin, Arnd Bergmann, Cornelia Huck, Linus Walleij, Bartosz Golaszewski Cc: Viresh Kumar, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven virtio-gpio is a virtual GPIO controller. It provides a way to flexibly communicate with the host GPIO controllers from the guest. This patch adds the specification for it. Based on the initial work posted by: "Enrico Weigelt, metux IT consult" <lkml@metux.net>. Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110 Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- conformance.tex | 28 +++- content.tex | 1 + virtio-gpio.tex | 369 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 394 insertions(+), 4 deletions(-) create mode 100644 virtio-gpio.tex diff --git a/conformance.tex b/conformance.tex index 94d7a06db899..c52f1a40be2d 100644 --- a/conformance.tex +++ b/conformance.tex @@ -30,8 +30,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \ref{sec:Conformance / Driver Conformance / IOMMU Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Sound Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Memory Driver Conformance}, -\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance} or -\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance}. +\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}, +\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance} or +\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance}. \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}. \end{itemize} @@ -54,8 +55,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \ref{sec:Conformance / Device Conformance / IOMMU Device Conformance}, \ref{sec:Conformance / Device Conformance / Sound Device Conformance}, \ref{sec:Conformance / Device Conformance / Memory Device Conformance}, -\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance} or -\ref{sec:Conformance / Device Conformance / SCMI Device Conformance}. +\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance}, +\ref{sec:Conformance / Device Conformance / SCMI Device Conformance} or +\ref{sec:Conformance / Device Conformance / GPIO Device Conformance}. \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}. \end{itemize} @@ -301,6 +303,15 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \item \ref{drivernormative:Device Types / SCMI Device / Device Operation / Setting Up eventq Buffers} \end{itemize} +\conformance{\subsection}{GPIO Driver Conformance}\label{sec:Conformance / Driver Conformance / GPIO Driver Conformance} + +A General Purpose Input/Output (GPIO) driver MUST conform to the following +normative statements: + +\begin{itemize} +\item \ref{drivernormative:Device Types / GPIO Device / requestq Operation} +\end{itemize} + \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance} A device MUST conform to the following normative statements: @@ -550,6 +561,15 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \item \ref{devicenormative:Device Types / SCMI Device / Device Operation / Shared Memory Operation} \end{itemize} +\conformance{\subsection}{GPIO Device Conformance}\label{sec:Conformance / Device Conformance / GPIO Device Conformance} + +A General Purpose Input/Output (GPIO) device MUST conform to the following +normative statements: + +\begin{itemize} +\item \ref{devicenormative:Device Types / GPIO Device / requestq Operation} +\end{itemize} + \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance} A conformant implementation MUST be either transitional or non-transitional, see \ref{intro:Legacy diff --git a/content.tex b/content.tex index 5c70a3c415a3..511f2071959d 100644 --- a/content.tex +++ b/content.tex @@ -6583,6 +6583,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device \input{virtio-mem.tex} \input{virtio-i2c.tex} \input{virtio-scmi.tex} +\input{virtio-gpio.tex} \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} diff --git a/virtio-gpio.tex b/virtio-gpio.tex new file mode 100644 index 000000000000..fa48780a29db --- /dev/null +++ b/virtio-gpio.tex @@ -0,0 +1,369 @@ +\section{GPIO Device}\label{sec:Device Types / GPIO Device} + +The Virtio GPIO device is a virtual General Purpose Input/Output device that +supports a variable number of named I/O lines, which can be configured in input +mode or in output mode with logical level low (0) or high (1). + +\subsection{Device ID}\label{sec:Device Types / GPIO Device / Device ID} +41 + +\subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues} + +\begin{description} +\item[0] requestq +\end{description} + +\subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits} + +None currently defined. + +\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout} + +GPIO device uses the following configuration structure layout: + +\begin{lstlisting} +struct virtio_gpio_config { + le16 ngpio; + u8 padding[2]; + le32 gpio_names_size; +}; +\end{lstlisting} + +\begin{description} +\item[\field{ngpio}] is the total number of GPIO lines supported by the device. + +\item[\field{padding}] has no meaning and is reserved for future use. This + MUST be set to zero by the device. + +\item[\field{gpio_names_size}] is the size of the gpio-names memory block in + bytes, which can be fetched by the driver using the + \field{VIRTIO_GPIO_MSG_GET_NAMES} message. The device MUST set this to 0 if + it doesn't support names for the GPIO lines. +\end{description} + + +\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization} + +\begin{itemize} +\item The driver MUST configure and initialize the \field{requestq} virtqueue. +\end{itemize} + +\subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation} + +The driver uses the \field{requestq} virtqueue to send messages to the device. +The driver sends a pair of buffers, request (filled by driver) and response (to +be filled by device later), to the device. The device in turn fills the response +buffer and sends it back to the driver. + +\begin{lstlisting} +struct virtio_gpio_request { + le16 type; + le16 gpio; + le32 value; +}; +\end{lstlisting} + +All the fields of this structure are set by the driver and read by the device. + +\begin{description} +\item[\field{type}] is the GPIO message type, i.e. one of + \field{VIRTIO_GPIO_MSG_*} values. + +\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} < + \field{ngpio}. + +\item[\field{value}] is a message specific value. +\end{description} + +\begin{lstlisting} +struct virtio_gpio_response { + u8 status; + u8 value[N]; +}; + +/* Possible values of the status field */ +#define VIRTIO_GPIO_STATUS_OK 0x0 +#define VIRTIO_GPIO_STATUS_ERR 0x1 +\end{lstlisting} + +All the fields of this structure are set by the device and read by the driver. + +\begin{description} +\item[\field{status}] of the GPIO message, + \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR} + on failure. + +\item[\field{value}] is a message specific defined value. The value of N (i.e. + size of the \field{value} buffer in bytes) is 1 for most of the messages, + except \field{VIRTIO_GPIO_MSG_GET_NAMES}, where it is set to the value of + \field{gpio_names_size} field. +\end{description} + +Following is the list of messages supported by the virtio gpio specification. + +\begin{lstlisting} +/* GPIO message types */ +#define VIRTIO_GPIO_MSG_GET_NAMES 0x0001 +#define VIRTIO_GPIO_MSG_ACTIVATE 0x0002 +#define VIRTIO_GPIO_MSG_DEACTIVATE 0x0003 +#define VIRTIO_GPIO_MSG_GET_DIRECTION 0x0004 +#define VIRTIO_GPIO_MSG_SET_DIRECTION_IN 0x0005 +#define VIRTIO_GPIO_MSG_SET_DIRECTION_OUT 0x0006 +#define VIRTIO_GPIO_MSG_GET_VALUE 0x0007 +#define VIRTIO_GPIO_MSG_SET_VALUE 0x0008 +\end{lstlisting} + +\subsubsection{requestq Operation: Get Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Names} + +The driver sends this message to receive a stream of zero-terminated strings, +where each string represents the name of a GPIO line, present in increasing +order of the GPIO line numbers. The GPIO line names must be unique within a GPIO +Device and must not be an empty string. + +These names of the GPIO lines should be most meaningful producer names for the +system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd" +and "ethernet reset" are reasonable line names as they describe what the line is +used for, while "GPIO0" is not a good name to give to a GPIO line. + +The device MUST set the \field{gpio_names_size} to a non-zero value if this +message is supported by the device, else it must be set to zero. + +The driver must allocate the \field{value[N]} buffer in the \field{struct +virtio_gpio_response} for N bytes, where N = \field{gpio_names_size}. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_GET_NAMES} & 0 & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & gpio-names & \field{gpio_names_size} \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Activate}\label{sec:Device Types / GPIO Device / requestq Operation / Activate} + +The driver sends this message to notify the device that one of its lines has +been assigned for use. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_ACTIVATE} & line number & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Deactivate}\label{sec:Device Types / GPIO Device / requestq Operation / Deactivate} + +The driver sends this message to notify the device that a previously requested +line has been unassigned and can be deactivated. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_DEACTIVATE} & line number & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction} + +The driver sends this message to request the device to return a line's +configured direction. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 = output, 1 = input & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Set Direction In}\label{sec:Device Types / GPIO Device / requestq Operation / Set Direction In} + +The driver sends this message to request the device to configure a line for +input. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_SET_DIRECTION_IN} & line number & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Set Direction Out}\label{sec:Device Types / GPIO Device / requestq Operation / Set Direction Out} + +The driver sends this message to request the device to configure a line for +output, and set its initial output value. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_SET_DIRECTION_OUT} & line number & 0 = low, 1 = high \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Get Value}\label{sec:Device Types / GPIO Device / requestq Operation / Get Value} + +The driver sends this message to request the device to return current value +sensed on a line configured for input. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_GET_VALUE} & line number & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 = low, 1 = high & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Device / requestq Operation / Set Value} + +The driver sends this message to request the device to set the value of a line +configured for output. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_SET_VALUE} & line number & 0 = low, 1 = high \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow} + +\begin{itemize} +\item The driver queues \field{struct virtio_gpio_request} and + \field{virtio_gpio_response} buffers to the \field{requestq} virtqueue, + after filling all fields of the \field{struct virtio_gpio_request} buffer as + defined by the specific message type. + +\item The driver notifies the device of the presence of buffers on the + \field{requestq} virtqueue. + +\item The device, after receiving the message from the driver, processes it and + fills all the fields of the \field{struct virtio_gpio_response} buffer + (received from the driver). The \field{status} must be set to + \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR} + on failure. + +\item The device puts the buffers back on the \field{requestq} virtqueue and + notifies the driver of the same. + +\item The driver fetches the buffers and processes the response received in the + \field{virtio_gpio_response} buffer. + +\item The driver can now send another message for the same GPIO line. +\end{itemize} + +\drivernormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation} + +\begin{itemize} +\item The driver MUST send messages on the \field{requestq} virtqueue. + +\item The driver MUST queue both \field{struct virtio_gpio_request} and + \field{virtio_gpio_response} for every message sent to the device. + +\item The \field{struct virtio_gpio_request} buffer MUST be filled by the driver + and MUST be read-only for the device. + +\item The \field{struct virtio_gpio_response} buffer MUST be filled by the + device and MUST be writable by the device. + +\item The driver MUST NOT set value of a GPIO line configured for input. + +\item The driver MUST NOT get value of a GPIO line configured for output. + +\item The driver MAY send messages for different GPIO lines in parallel. + +\item The driver MUST NOT send another message, of any type, for a GPIO line + before receiving a response from the device for the previously sent message + for the same GPIO line. +\end{itemize} + +\devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation} + +\begin{itemize} +\item The device MUST set all the fields of the \field{struct + virtio_gpio_response} before sending it back to the driver. + +\item The device MUST set all the fields of the \field{struct + virtio_gpio_config} on receiving a configuration request from the driver. + +\item The device MUST set the \field{gpio_names_size} field as zero in the + \field{struct virtio_gpio_config}, if it doesn't implement names for + individual GPIO lines. + +\item The device MUST set the \field{gpio_names_size} field, in the + \field{struct virtio_gpio_config}, with the size of gpio-names memory block + in bytes, if the device implements names for individual GPIO lines. The + strings MUST be zero-terminated and an unique and a valid string MUST be + added for each supported GPIO line. + +\item The device MAY process messages out of order and in parallel and MAY send + message's response to the driver out of order. +\end{itemize} -- 2.31.1.272.g89b43f80a514 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [virtio-dev] Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-20 15:28 ` [PATCH V6 1/2] virtio-gpio: Add the device specification Viresh Kumar @ 2021-07-26 9:18 ` Arnd Bergmann 2021-07-26 10:16 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Arnd Bergmann @ 2021-07-26 9:18 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 20, 2021 at 5:28 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > virtio-gpio is a virtual GPIO controller. It provides a way to flexibly > communicate with the host GPIO controllers from the guest. > > This patch adds the specification for it. > > Based on the initial work posted by: > "Enrico Weigelt, metux IT consult" <lkml@metux.net>. > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110 > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > + > +\item[\field{gpio_names_size}] is the size of the gpio-names memory block in > + bytes, which can be fetched by the driver using the > + \field{VIRTIO_GPIO_MSG_GET_NAMES} message. The device MUST set this to 0 if > + it doesn't support names for the GPIO lines. > +\end{description} I would be more comfortable with leaving out this field, and making the name lookup fixed-length and per line. > +\subsubsection{requestq Operation: Get Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Names} > + > +The driver sends this message to receive a stream of zero-terminated strings, > +where each string represents the name of a GPIO line, present in increasing > +order of the GPIO line numbers. The GPIO line names must be unique within a GPIO > +Device and must not be an empty string. > + > +These names of the GPIO lines should be most meaningful producer names for the > +system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd" > +and "ethernet reset" are reasonable line names as they describe what the line is > +used for, while "GPIO0" is not a good name to give to a GPIO line. > + > +The device MUST set the \field{gpio_names_size} to a non-zero value if this > +message is supported by the device, else it must be set to zero. > + > +The driver must allocate the \field{value[N]} buffer in the \field{struct > +virtio_gpio_response} for N bytes, where N = \field{gpio_names_size}. Making this a per-line request also has the advantage that the device can have names associated with some of the lines, but leave some other lines unnamed, which I think is something that happens in practice. Not sure what a good upper limit for the length of the name is, but making this fixed-length would seem simpler to me. > +\subsubsection{requestq Operation: Activate}\label{sec:Device Types / GPIO Device / requestq Operation / Activate} > + > +The driver sends this message to notify the device that one of its lines has > +been assigned for use. Maybe expand this description a little more according to our previous discussion. In particular, I would expect this to describe - what the controller hardware is expected to do as a result - which of the other calls are allowed on lines that have been requested or have not been requested. This information could be duplicated in the respective descriptions. In particular, it is not obvious whether you should set the direction before or after 'request', and I assume only one of the two possibilities is allowed. > + > +\item The driver MUST NOT send another message, of any type, for a GPIO line > + before receiving a response from the device for the previously sent message > + for the same GPIO line. I have a question here, not sure if this needs to be answered in the spec: why can't the driver send activate, set-direction and set-value all at once for a line? Clearly if one request fails, the later ones would fail as well in this example, but I don't see this as a problem for either side. > +\item The device MAY process messages out of order and in parallel and MAY send > + message's response to the driver out of order. Here I have the opposite question: what is the benefit of allowing the requests to be processed out of order? Should these not all be instantaneous, and easier to handle by making the request queue serialized? Arnd --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-26 9:18 ` [virtio-dev] " Arnd Bergmann @ 2021-07-26 10:16 ` Viresh Kumar 2021-07-26 11:28 ` Arnd Bergmann 2021-07-26 11:30 ` Geert Uytterhoeven 0 siblings, 2 replies; 26+ messages in thread From: Viresh Kumar @ 2021-07-26 10:16 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 26-07-21, 11:18, Arnd Bergmann wrote: > I would be more comfortable with leaving out this field, and making the > name lookup fixed-length and per line. I can leave it from configuration, sure. Doing it per-line seems to be too much overhead, since we are going to call this for every line anyway (at least in Linux implementation). I haven't tried it, but I somehow feel that probe() will take a significant time to finish with that, lets say for 256 GPIO lines. > Making this a per-line request also has the advantage that the device > can have names associated with some of the lines, but leave > some other lines unnamed, which I think is something that happens > in practice. > > Not sure what a good upper limit for the length of the name is, but > making this fixed-length would seem simpler to me. Or allocate 20-25 bytes for each string and allocate a buffer of size 25 * ngpios The device can write 0 (null) for some of the GPIOs, where it doesn't want to provide a name, or set it to a null terminated string with length <= 25. > > +\subsubsection{requestq Operation: Activate}\label{sec:Device Types / GPIO Device / requestq Operation / Activate} > > + > > +The driver sends this message to notify the device that one of its lines has > > +been assigned for use. > > Maybe expand this description a little more according to our previous > discussion. > In particular, I would expect this to describe > > - what the controller hardware is expected to do as a result > - which of the other calls are allowed on lines that have been requested > or have not been requested. This information could be duplicated in the > respective descriptions. In particular, it is not obvious whether you should > set the direction before or after 'request', and I assume only one of the > two possibilities is allowed. Right. > > +\item The driver MUST NOT send another message, of any type, for a GPIO line > > + before receiving a response from the device for the previously sent message > > + for the same GPIO line. > > I have a question here, not sure if this needs to be answered in the spec: why > can't the driver send activate, set-direction and set-value all at > once for a line? > Clearly if one request fails, the later ones would fail as well in > this example, but > I don't see this as a problem for either side. The driver can already do set-direction and value at once. Merging activate along with that would mean, that the host needs to keep a reference count at its end to activate the line on the first call to set-value, but not on others. We would still need to keep the deactivate request, as the host wouldn't know when the guest is done using a line. And for that reason it looked better to have an explicit call to activate here. > > +\item The device MAY process messages out of order and in parallel and MAY send > > + message's response to the driver out of order. > > Here I have the opposite question: what is the benefit of allowing the requests > to be processed out of order? Should these not all be instantaneous, and easier > to handle by making the request queue serialized? Lets say the guest sends two requests, almost at the same time to the host and they are queued on the vq in this order only: - set direction-out-with-value-1 for line 5. - set direction-in for line 11. Now, if the host is able to process them in parallel, then the second request for line 11 may finish before the requests for line 5 (as it has lesser amount of work to do). I don't see why the host should wait for the first request to finish before queuing up the buffers for the second request, while it has already finished. To be most efficient, the queue should be thought of as a series of buffers which are present in any order, based on what finishes first. It doesn't increase the complexity on any side to be honest. The code remains straight forward with it. -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-26 10:16 ` Viresh Kumar @ 2021-07-26 11:28 ` Arnd Bergmann 2021-07-27 7:55 ` Viresh Kumar 2021-07-26 11:30 ` Geert Uytterhoeven 1 sibling, 1 reply; 26+ messages in thread From: Arnd Bergmann @ 2021-07-26 11:28 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-07-21, 11:18, Arnd Bergmann wrote: > > I would be more comfortable with leaving out this field, and making the > > name lookup fixed-length and per line. > > I can leave it from configuration, sure. > > Doing it per-line seems to be too much overhead, since we are going to > call this for every line anyway (at least in Linux implementation). I > haven't tried it, but I somehow feel that probe() will take a > significant time to finish with that, lets say for 256 GPIO lines. My guess is that this would not cause any notable overhead, but we will know for sure if you can measure it. > > Making this a per-line request also has the advantage that the device > > can have names associated with some of the lines, but leave > > some other lines unnamed, which I think is something that happens > > in practice. > > > > Not sure what a good upper limit for the length of the name is, but > > making this fixed-length would seem simpler to me. > > Or allocate 20-25 bytes for each string and allocate a buffer of size > > 25 * ngpios > > The device can write 0 (null) for some of the GPIOs, where it doesn't > want to provide a name, or set it to a null terminated string with > length <= 25. I was thinking of a name limit in the context of doing the operation per line. If you keep the list of nul-terminated strings, then I think I would also not limit the length per line. > > > +\item The driver MUST NOT send another message, of any type, for a GPIO line > > > + before receiving a response from the device for the previously sent message > > > + for the same GPIO line. > > > > I have a question here, not sure if this needs to be answered in the spec: why > > can't the driver send activate, set-direction and set-value all at > > once for a line? > > Clearly if one request fails, the later ones would fail as well in > > this example, but > > I don't see this as a problem for either side. > > The driver can already do set-direction and value at once. Ah, I completely missed that. > Merging activate along with that would mean, that the host needs to keep > a reference count at its end to activate the line on the first call to > set-value, but not on others. My question here was about something else, to rephrase: Can't we allow the driver to queue multiple requests for one line and then have the device process them in order (regardless of whether it processes requests for other lines in parallel). The sequence I had in mind here was driver: 1. queue enable 2. queue set-direction 3. queue set-value 4. notify device device: 5. process enable 6. queue enable-response 7. process set-direction 8. queue set-direction response 9. process set-value 10. queue set-value response 11. notify driver driver: 12. mark all requests as done > We would still need to keep the deactivate request, as the host > wouldn't know when the guest is done using a line. And for that reason > it looked better to have an explicit call to activate here. Folding the activate/deactivate is also an interesting option, as discussed before. I don't think we actually need to do reference counting, just making the direction a tristate. I think this would work fine here: - default direction is VIRTIO_GPIO_MSG_SET_DIRECTION_NONE" - as long as the direction is not set, nothing else is allowed on the line - changing the direction from "none" to "in" or "out" implies "activate" - changing the direction from "in" or "out" back to "none" implies "deactivate" This is similar to folding the irq type into the direction command, and I would slightly prefer it that way, but I'd like to hear what others think about it. > > > +\item The device MAY process messages out of order and in parallel and MAY send > > > + message's response to the driver out of order. > > > > Here I have the opposite question: what is the benefit of allowing the requests > > to be processed out of order? Should these not all be instantaneous, and easier > > to handle by making the request queue serialized? > > Lets say the guest sends two requests, almost at the same time to > the host and they are queued on the vq in this order only: > > - set direction-out-with-value-1 for line 5. > - set direction-in for line 11. > > Now, if the host is able to process them in parallel, then the second > request for line 11 may finish before the requests for line 5 (as it > has lesser amount of work to do). > > I don't see why the host should wait for the first request to finish > before queuing up the buffers for the second request, while it has > already finished. > > To be most efficient, the queue should be thought of as a series of > buffers which are present in any order, based on what finishes first. > It doesn't increase the complexity on any side to be honest. The code > remains straight forward with it. I expected the host side to be atomic here. Under which circumstance would the 'set direction out with value' call in your example block? Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-26 11:28 ` Arnd Bergmann @ 2021-07-27 7:55 ` Viresh Kumar 2021-07-27 8:16 ` Arnd Bergmann 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 7:55 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 26-07-21, 13:28, Arnd Bergmann wrote: > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Doing it per-line seems to be too much overhead, since we are going to > > call this for every line anyway (at least in Linux implementation). I > > haven't tried it, but I somehow feel that probe() will take a > > significant time to finish with that, lets say for 256 GPIO lines. > > My guess is that this would not cause any notable overhead, but we > will know for sure if you can measure it. I will surely try it to get the number out, but sending a request 256 times instead of just once looks inefficient from the face of it :) > > Or allocate 20-25 bytes for each string and allocate a buffer of size > > > > 25 * ngpios > > > > The device can write 0 (null) for some of the GPIOs, where it doesn't > > want to provide a name, or set it to a null terminated string with > > length <= 25. > > I was thinking of a name limit in the context of doing the operation per > line. I really don't want to do it, since in kernel at least we would run that function for all lines right at probe(). > If you keep the list of nul-terminated strings, then I think I would > also not limit the length per line. The problem at hand is knowing the (maximum) size of the buffer to pre-allocate at the driver. And here are the options: - Keep gpio_names_size in the config structure (like this commit, which is kind of fine since we won't need this multiple times). - Add a new message type for that, i.e. getting size of the gpio_names_block, but I don't like it since this will be used only once. - Fix size for each GPIO pin's name to X characters and the driver allocates a buffer of size X * ngpio. This provides good boundaries between line names, more straightforward, less confusing, but more data (unnecessarily) to be sent. Which one do you like ? > > > I have a question here, not sure if this needs to be answered in the spec: why > > > can't the driver send activate, set-direction and set-value all at > > > once for a line? > > > Clearly if one request fails, the later ones would fail as well in > > > this example, but > > > I don't see this as a problem for either side. > > > Merging activate along with that would mean, that the host needs to keep > > a reference count at its end to activate the line on the first call to > > set-value, but not on others. > > My question here was about something else, to rephrase: Can't we allow > the driver to queue multiple requests for one line and then have the device > process them in order (regardless of whether it processes requests > for other lines in parallel). The sequence I had in mind here was > > driver: > 1. queue enable > 2. queue set-direction > 3. queue set-value > 4. notify device > device: > 5. process enable > 6. queue enable-response > 7. process set-direction > 8. queue set-direction response > 9. process set-value > 10. queue set-value response > 11. notify driver > driver: > 12. mark all requests as done Hmm, we can do that in principle by saying all requests for a particular line must be served in order. But right now I have rather limited the number of requests per line to 1 for a reason, i.e. there will always be a single user of a GPIO line at any point of time. And so the operations will always be sequential. If you look at users of GPIO, lets say in kernel for now, they all use generic APIs like request, set-direction, set-value, etc. There is no fixed order of how the different callbacks will be called by the user, and so we will never get the requests in bulk or groups (like you mentioned above). Each operation has its own significance, and it won't be right to return from, lets say gpio_set_value(), without getting the result of the operation. So when can something like this, what you showed above, can be useful ? > > We would still need to keep the deactivate request, as the host > > wouldn't know when the guest is done using a line. And for that reason > > it looked better to have an explicit call to activate here. > > Folding the activate/deactivate is also an interesting option, as discussed > before. I don't think we actually need to do reference counting, just making > the direction a tristate. I think this would work fine here: > > - default direction is VIRTIO_GPIO_MSG_SET_DIRECTION_NONE" > - as long as the direction is not set, nothing else is allowed on the line > - changing the direction from "none" to "in" or "out" implies "activate" > - changing the direction from "in" or "out" back to "none" implies "deactivate" I am not able to see anything which can break with this, so this looks okay. We can get away with activate/deactivate and use direction-none for that. > This is similar to folding the irq type into the direction command, and I > would slightly prefer it that way, but I'd like to hear what others think > about it. I would really like to keep the irq stuff separate from rest of the basic APIs. This would also be less confusing because the irq stuff is handled separately using a feature flag here. Over that the users won't club these either, like in Linux the user will do gpio-request, then direction-input, and then request irq. So these are all required to be sent at different times. > > > Here I have the opposite question: what is the benefit of allowing the requests > > > to be processed out of order? Should these not all be instantaneous, and easier > > > to handle by making the request queue serialized? > > > > Lets say the guest sends two requests, almost at the same time to > > the host and they are queued on the vq in this order only: > > > > - set direction-out-with-value-1 for line 5. > > - set direction-in for line 11. > > > > Now, if the host is able to process them in parallel, then the second > > request for line 11 may finish before the requests for line 5 (as it > > has lesser amount of work to do). > > > > I don't see why the host should wait for the first request to finish > > before queuing up the buffers for the second request, while it has > > already finished. > > > > To be most efficient, the queue should be thought of as a series of > > buffers which are present in any order, based on what finishes first. > > It doesn't increase the complexity on any side to be honest. The code > > remains straight forward with it. > > I expected the host side to be atomic here. Under which circumstance > would the 'set direction out with value' call in your example block? Yes it will block, but another thread can initiate another request for a different GPIO line. -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 7:55 ` Viresh Kumar @ 2021-07-27 8:16 ` Arnd Bergmann 2021-07-27 10:56 ` Viresh Kumar 2021-07-27 10:57 ` [virtio-dev] " Cornelia Huck 0 siblings, 2 replies; 26+ messages in thread From: Arnd Bergmann @ 2021-07-27 8:16 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 26-07-21, 13:28, Arnd Bergmann wrote: > > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > Doing it per-line seems to be too much overhead, since we are going to > > > call this for every line anyway (at least in Linux implementation). I > > > haven't tried it, but I somehow feel that probe() will take a > > > significant time to finish with that, lets say for 256 GPIO lines. > > > > My guess is that this would not cause any notable overhead, but we > > will know for sure if you can measure it. > > I will surely try it to get the number out, but sending a request 256 > times instead of just once looks inefficient from the face of it :) It could still be only one notification though, just a lot of requests with a pair of buffers each, that you send all at once. > > If you keep the list of nul-terminated strings, then I think I would > > also not limit the length per line. > > The problem at hand is knowing the (maximum) size of the buffer to > pre-allocate at the driver. And here are the options: > > - Keep gpio_names_size in the config structure (like this commit, > which is kind of fine since we won't need this multiple times). > > - Add a new message type for that, i.e. getting size of the > gpio_names_block, but I don't like it since this will be used only > once. > > - Fix size for each GPIO pin's name to X characters and the driver > allocates a buffer of size X * ngpio. This provides good boundaries > between line names, more straightforward, less confusing, but more > data (unnecessarily) to be sent. > > Which one do you like ? If we stay with a single message to get all the names, I'd probably also stay with having the buffer length in config space as you do in the current version. > > > > I have a question here, not sure if this needs to be answered in the spec: why > > > > can't the driver send activate, set-direction and set-value all at > > > > once for a line? > > > > Clearly if one request fails, the later ones would fail as well in > > > > this example, but > > > > I don't see this as a problem for either side. > > > > > Merging activate along with that would mean, that the host needs to keep > > > a reference count at its end to activate the line on the first call to > > > set-value, but not on others. > > > > My question here was about something else, to rephrase: Can't we allow > > the driver to queue multiple requests for one line and then have the device > > process them in order (regardless of whether it processes requests > > for other lines in parallel). The sequence I had in mind here was > > > > driver: > > 1. queue enable > > 2. queue set-direction > > 3. queue set-value > > 4. notify device > > device: > > 5. process enable > > 6. queue enable-response > > 7. process set-direction > > 8. queue set-direction response > > 9. process set-value > > 10. queue set-value response > > 11. notify driver > > driver: > > 12. mark all requests as done > > Hmm, we can do that in principle by saying all requests for a > particular line must be served in order. But right now I have rather > limited the number of requests per line to 1 for a reason, i.e. there > will always be a single user of a GPIO line at any point of time. And > so the operations will always be sequential. > > If you look at users of GPIO, lets say in kernel for now, they all use > generic APIs like request, set-direction, set-value, etc. There is no > fixed order of how the different callbacks will be called by the user, > and so we will never get the requests in bulk or groups (like you > mentioned above). Each operation has its own significance, and it > won't be right to return from, lets say gpio_set_value(), without > getting the result of the operation. > > So when can something like this, what you showed above, can be useful > ? Maybe this is something we can get advice for from the virtio maintainers. It was just a feeling on my side that slightly relaxing the requirement makes this more like other virtio drivers. Functionally, I think there is no difference between mandating that the driver only queues one request per gpio line over requiring that the device processes all requests on a gpio line in sequence, but the latter can be slightly more efficient. > > This is similar to folding the irq type into the direction command, and I > > would slightly prefer it that way, but I'd like to hear what others think > > about it. > > I would really like to keep the irq stuff separate from rest of the > basic APIs. This would also be less confusing because the irq stuff is > handled separately using a feature flag here. > > Over that the users won't club these either, like in Linux the user > will do gpio-request, then direction-input, and then request irq. > > So these are all required to be sent at different times. ok > > > To be most efficient, the queue should be thought of as a series of > > > buffers which are present in any order, based on what finishes first. > > > It doesn't increase the complexity on any side to be honest. The code > > > remains straight forward with it. > > > > I expected the host side to be atomic here. Under which circumstance > > would the 'set direction out with value' call in your example block? > > Yes it will block, but another thread can initiate another request for > a different GPIO line. Ok, I was hoping that the gpiolib interfaces would all end up being non-blocking, but I agree that if the guest has to wait for a notification it can be useful to have more than one going on at once. Especially if the host implementation is in a separate process there is not even a theoretical upper bound on how long it takes to process a request. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 8:16 ` Arnd Bergmann @ 2021-07-27 10:56 ` Viresh Kumar 2021-07-27 12:06 ` Arnd Bergmann 2021-07-27 10:57 ` [virtio-dev] " Cornelia Huck 1 sibling, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 10:56 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 27-07-21, 10:16, Arnd Bergmann wrote: > On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-07-21, 13:28, Arnd Bergmann wrote: > > > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > Doing it per-line seems to be too much overhead, since we are going to > > > > call this for every line anyway (at least in Linux implementation). I > > > > haven't tried it, but I somehow feel that probe() will take a > > > > significant time to finish with that, lets say for 256 GPIO lines. > > > > > > My guess is that this would not cause any notable overhead, but we > > > will know for sure if you can measure it. > > > > I will surely try it to get the number out, but sending a request 256 > > times instead of just once looks inefficient from the face of it :) I created a small setup with qemu-arm64 over x86: - Without any memcpy() of data or buffers in the GPIO drivers on both ends. - i.e. essentially taking into consideration only the time to send receive requests, the virtio core may be copying packets in between though, not sure. > It could still be only one notification though, just a lot of requests > with a pair of buffers each, that you send all at once. If we send only one notification at the end, it takes ~ 1ms for 256 GPIO pins, pretty much like a single request only. The downside here is that the device (host) also needs to prepare the queue for equal number (to ngpio) of elements. This is something the spec hasn't touched until now, i.e. how many buffers must be queued for the virtqueues. The max allowed in qemu is 1024 right now. Even if I send them sequentially, with separate notification for each request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024 lines. Though a single request still sounds tempting to me :) > > > My question here was about something else, to rephrase: Can't we allow > > > the driver to queue multiple requests for one line and then have the device > > > process them in order (regardless of whether it processes requests > > > for other lines in parallel). The sequence I had in mind here was > > > > > > driver: > > > 1. queue enable > > > 2. queue set-direction > > > 3. queue set-value > > > 4. notify device > > > device: > > > 5. process enable > > > 6. queue enable-response > > > 7. process set-direction > > > 8. queue set-direction response > > > 9. process set-value > > > 10. queue set-value response > > > 11. notify driver > > > driver: > > > 12. mark all requests as done > > > > Hmm, we can do that in principle by saying all requests for a > > particular line must be served in order. But right now I have rather > > limited the number of requests per line to 1 for a reason, i.e. there > > will always be a single user of a GPIO line at any point of time. And > > so the operations will always be sequential. > > > > If you look at users of GPIO, lets say in kernel for now, they all use > > generic APIs like request, set-direction, set-value, etc. There is no > > fixed order of how the different callbacks will be called by the user, > > and so we will never get the requests in bulk or groups (like you > > mentioned above). Each operation has its own significance, and it > > won't be right to return from, lets say gpio_set_value(), without > > getting the result of the operation. > > > > So when can something like this, what you showed above, can be useful > > ? > > Maybe this is something we can get advice for from the virtio > maintainers. It was just a feeling on my side that slightly relaxing > the requirement makes this more like other virtio drivers. > > Functionally, I think there is no difference between mandating that the > driver only queues one request per gpio line over requiring that the > device processes all requests on a gpio line in sequence, but the > latter can be slightly more efficient. Yes, it can be efficient but I don't see when this can be used in practice. Specially in kernel, the way GPIO users handle stuff. But anyway, I don't have any issue specifically with allowing multiple requests per GPIO line in the spec itself. Spec can be kept open for such an option. > > > > To be most efficient, the queue should be thought of as a series of > > > > buffers which are present in any order, based on what finishes first. > > > > It doesn't increase the complexity on any side to be honest. The code > > > > remains straight forward with it. > > > > > > I expected the host side to be atomic here. Under which circumstance > > > would the 'set direction out with value' call in your example block? > > > > Yes it will block, but another thread can initiate another request for > > a different GPIO line. > > Ok, I was hoping that the gpiolib interfaces would all end up being > non-blocking, I am not sure how we can do that :( > but I agree that if the guest has to wait for a notification > it can be useful to have more than one going on at once. Especially > if the host implementation is in a separate process there is not even > a theoretical upper bound on how long it takes to process a request. Right. -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 10:56 ` Viresh Kumar @ 2021-07-27 12:06 ` Arnd Bergmann 2021-07-27 14:17 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Arnd Bergmann @ 2021-07-27 12:06 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 27, 2021 at 12:56 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 27-07-21, 10:16, Arnd Bergmann wrote: > > On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > My guess is that this would not cause any notable overhead, but we > > > > will know for sure if you can measure it. > > > > > > I will surely try it to get the number out, but sending a request 256 > > > times instead of just once looks inefficient from the face of it :) > > I created a small setup with qemu-arm64 over x86: > - Without any memcpy() of data or buffers in the GPIO drivers on both > ends. > - i.e. essentially taking into consideration only the time to send > receive requests, the virtio core may be copying packets in between > though, not sure. > > > It could still be only one notification though, just a lot of requests > > with a pair of buffers each, that you send all at once. > > If we send only one notification at the end, it takes ~ 1ms for 256 > GPIO pins, pretty much like a single request only. The downside here > is that the device (host) also needs to prepare the queue for equal > number (to ngpio) of elements. This is something the spec hasn't > touched until now, i.e. how many buffers must be queued for the > virtqueues. The max allowed in qemu is 1024 right now. 1ms is borderline I guess, it's close to the range of things that gets annoying for boot-time optimization. > Even if I send them sequentially, with separate notification for each > request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024 > lines. > > Though a single request still sounds tempting to me :) Right, 80ms is definitely noticeable, this can easily make the difference between an instant guest boot and a perceived delay. > > > > I expected the host side to be atomic here. Under which circumstance > > > > would the 'set direction out with value' call in your example block? > > > > > > Yes it will block, but another thread can initiate another request for > > > a different GPIO line. > > > > Ok, I was hoping that the gpiolib interfaces would all end up being > > non-blocking, > > I am not sure how we can do that :( I suppose not without a complete redesign from scratch. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 12:06 ` Arnd Bergmann @ 2021-07-27 14:17 ` Viresh Kumar 2021-07-27 14:20 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 14:17 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, 27 Jul 2021 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote: > On Tue, Jul 27, 2021 at 12:56 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > If we send only one notification at the end, it takes ~ 1ms for 256 > > GPIO pins, pretty much like a single request only. The downside here > > is that the device (host) also needs to prepare the queue for equal > > number (to ngpio) of elements. This is something the spec hasn't > > touched until now, i.e. how many buffers must be queued for the > > virtqueues. The max allowed in qemu is 1024 right now. > > 1ms is borderline I guess, it's close to the range of things that gets > annoying for boot-time optimization. > > > Even if I send them sequentially, with separate notification for each > > request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024 > > lines. > > > > Though a single request still sounds tempting to me :) > > Right, 80ms is definitely noticeable, this can easily make the difference > between an instant guest boot and a perceived delay. It is also important to mention here that in my current example, there was no copy of the gpio line name string made anywhere. If we choose this message to be per-line, then there will be at least one memcpy at the host of the string for each GPIO line, there may be another copy somewhere in between at guest or host, not sure. Though in case of a single request, this will be a single memcpy operation at the guest (and others in between as mentioned earlier), but not per line and maybe more efficient. So even in the best possible scenario, this will be several ms, at least for separate message for each GPIO line. Anyway, how do you suggest I define this call now. Per-line or entire block (fixed size or from config structure) ? I will do it the way you suggest to cut short the discussion :) > > I am not sure how we can do that :( > > I suppose not without a complete redesign from scratch. Yeah, I am not doing it then :) -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 14:17 ` Viresh Kumar @ 2021-07-27 14:20 ` Viresh Kumar 2021-07-27 14:38 ` Arnd Bergmann 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 14:20 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, 27 Jul 2021 at 19:47, Viresh Kumar <viresh.kumar@linaro.org> wrote: > It is also important to mention here that in my current example, there > was no copy > of the gpio line name string made anywhere. > > If we choose this message to be per-line, then there will be at least one memcpy > at the host of the string for each GPIO line, there may be another > copy somewhere > in between at guest or host, not sure. Though in case of a single > request, this will > be a single memcpy operation at the guest (and others in between as > mentioned earlier), > but not per line and maybe more efficient. > > So even in the best possible scenario, this will be several ms, at > least for separate > message for each GPIO line. > > Anyway, how do you suggest I define this call now. Per-line or entire > block (fixed size or > from config structure) ? I will do it the way you suggest to cut short > the discussion :) Also to mention that this request will always be called (at least for Linux) only once for all the GPIO lines, at probe time only. And never after that. So making it per-line doesn't give us any flexibility that we may be looking for. -- Viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 14:20 ` Viresh Kumar @ 2021-07-27 14:38 ` Arnd Bergmann 0 siblings, 0 replies; 26+ messages in thread From: Arnd Bergmann @ 2021-07-27 14:38 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 27, 2021 at 4:20 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > So making it per-line doesn't give us any flexibility that we may be > looking for. I was not really looking for extra flexibility here, only for a simpler and more consistent interface definition. But if the simpler interface comes at the cost of more complexity and time overhead in the driver, it's probably not worth it. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* [virtio-dev] Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 8:16 ` Arnd Bergmann 2021-07-27 10:56 ` Viresh Kumar @ 2021-07-27 10:57 ` Cornelia Huck 2021-07-27 11:21 ` Viresh Kumar 1 sibling, 1 reply; 26+ messages in thread From: Cornelia Huck @ 2021-07-27 10:57 UTC (permalink / raw) To: Arnd Bergmann, Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 27 2021, Arnd Bergmann <arnd@kernel.org> wrote: > On Tue, Jul 27, 2021 at 9:55 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >> On 26-07-21, 13:28, Arnd Bergmann wrote: >> > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > [Disclaimer: I basically don't know anything about gpio] >> > > > I have a question here, not sure if this needs to be answered in the spec: why >> > > > can't the driver send activate, set-direction and set-value all at >> > > > once for a line? >> > > > Clearly if one request fails, the later ones would fail as well in >> > > > this example, but >> > > > I don't see this as a problem for either side. >> > >> > > Merging activate along with that would mean, that the host needs to keep >> > > a reference count at its end to activate the line on the first call to >> > > set-value, but not on others. >> > >> > My question here was about something else, to rephrase: Can't we allow >> > the driver to queue multiple requests for one line and then have the device >> > process them in order (regardless of whether it processes requests >> > for other lines in parallel). The sequence I had in mind here was >> > >> > driver: >> > 1. queue enable >> > 2. queue set-direction >> > 3. queue set-value >> > 4. notify device >> > device: >> > 5. process enable >> > 6. queue enable-response >> > 7. process set-direction >> > 8. queue set-direction response >> > 9. process set-value >> > 10. queue set-value response >> > 11. notify driver >> > driver: >> > 12. mark all requests as done What are the ordering/sequence requirements here? E.g., are enable/set-direction or enable/set-value/set-direction valid sequences? If we consider the sequence 1-4 above as a command chain, what about adding a chaining field that can point to the next command in the chain? The driver would build the chain in one go, the device would process the chain one after another, and stop if it encounters an error. The driver would be free to submit chains for other lines, but not another one for the same line as long as the current chain is not yet finished. [Yes, I realize that this looks a lot like channel I/O...] >> >> Hmm, we can do that in principle by saying all requests for a >> particular line must be served in order. But right now I have rather >> limited the number of requests per line to 1 for a reason, i.e. there >> will always be a single user of a GPIO line at any point of time. And >> so the operations will always be sequential. >> >> If you look at users of GPIO, lets say in kernel for now, they all use >> generic APIs like request, set-direction, set-value, etc. There is no >> fixed order of how the different callbacks will be called by the user, >> and so we will never get the requests in bulk or groups (like you >> mentioned above). Each operation has its own significance, and it >> won't be right to return from, lets say gpio_set_value(), without >> getting the result of the operation. You could match this to individual chains of length one, where you cannot submit another request for a line before the current one is finished. >> >> So when can something like this, what you showed above, can be useful >> ? > > Maybe this is something we can get advice for from the virtio > maintainers. It was just a feeling on my side that slightly relaxing > the requirement makes this more like other virtio drivers. > > Functionally, I think there is no difference between mandating that the > driver only queues one request per gpio line over requiring that the > device processes all requests on a gpio line in sequence, but the > latter can be slightly more efficient. It might be worth considering for future drivers, or non-Linux drivers. --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [virtio-dev] Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-27 10:57 ` [virtio-dev] " Cornelia Huck @ 2021-07-27 11:21 ` Viresh Kumar 0 siblings, 0 replies; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 11:21 UTC (permalink / raw) To: Cornelia Huck Cc: Arnd Bergmann, Jason Wang, Michael S. Tsirkin, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 27-07-21, 12:57, Cornelia Huck wrote: > On Tue, Jul 27 2021, Arnd Bergmann <arnd@kernel.org> wrote: > >> > driver: > >> > 1. queue enable > >> > 2. queue set-direction > >> > 3. queue set-value > >> > 4. notify device > >> > device: > >> > 5. process enable > >> > 6. queue enable-response > >> > 7. process set-direction > >> > 8. queue set-direction response > >> > 9. process set-value > >> > 10. queue set-value response > >> > 11. notify driver > >> > driver: > >> > 12. mark all requests as done > > What are the ordering/sequence requirements here? E.g., are > enable/set-direction or enable/set-value/set-direction valid sequences? Yes, these are valid sequences. But there is not real hardfixed way of doing things here, the users can call them most likely in any order. > If we consider the sequence 1-4 above as a command chain, what about > adding a chaining field that can point to the next command in the chain? > The driver would build the chain in one go, the device would process the > chain one after another, and stop if it encounters an error. The driver > would be free to submit chains for other lines, but not another one for > the same line as long as the current chain is not yet finished. These commands aren't related necessarily and so this kind of linking between them is not right. Each operation is really independent from each other, though the user may not want to initiate the second operation if the first one fails. This isn't something like I2C messages, where the first one gives the address and second one gives something else (like size) and is incomplete without the first one. Here all instructions is actually independent of each other. I think Arnd was just thinking of this to make it possible to send notification only once, if that can give some improvement. But I am quite sure that Linux today will most likely not end up using it that way. > > Maybe this is something we can get advice for from the virtio > > maintainers. It was just a feeling on my side that slightly relaxing > > the requirement makes this more like other virtio drivers. > > > > Functionally, I think there is no difference between mandating that the > > driver only queues one request per gpio line over requiring that the > > device processes all requests on a gpio line in sequence, but the > > latter can be slightly more efficient. > > It might be worth considering for future drivers, or non-Linux drivers. Right, as I said in the other email just now, I am fine with allowing this in spec at least (someone may want to use it later), i.e. queuing of multiple request for the same GPIO line and them being processed in the same order. -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-26 10:16 ` Viresh Kumar 2021-07-26 11:28 ` Arnd Bergmann @ 2021-07-26 11:30 ` Geert Uytterhoeven 2021-07-26 11:55 ` Viresh Kumar 1 sibling, 1 reply; 26+ messages in thread From: Geert Uytterhoeven @ 2021-07-26 11:30 UTC (permalink / raw) To: Viresh Kumar Cc: Arnd Bergmann, Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev Hi Viresh, On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 26-07-21, 11:18, Arnd Bergmann wrote: > > Here I have the opposite question: what is the benefit of allowing the requests > > to be processed out of order? Should these not all be instantaneous, and easier > > to handle by making the request queue serialized? > > Lets say the guest sends two requests, almost at the same time to > the host and they are queued on the vq in this order only: > > - set direction-out-with-value-1 for line 5. > - set direction-in for line 11. > > Now, if the host is able to process them in parallel, then the second > request for line 11 may finish before the requests for line 5 (as it > has lesser amount of work to do). Speaking about parallel processing, have you already envisioned support for .[sg]et_multiple()? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 1/2] virtio-gpio: Add the device specification 2021-07-26 11:30 ` Geert Uytterhoeven @ 2021-07-26 11:55 ` Viresh Kumar 0 siblings, 0 replies; 26+ messages in thread From: Viresh Kumar @ 2021-07-26 11:55 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Arnd Bergmann, Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev On 26-07-21, 13:30, Geert Uytterhoeven wrote: > Hi Viresh, > > On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-07-21, 11:18, Arnd Bergmann wrote: > > > Here I have the opposite question: what is the benefit of allowing the requests > > > to be processed out of order? Should these not all be instantaneous, and easier > > > to handle by making the request queue serialized? > > > > Lets say the guest sends two requests, almost at the same time to > > the host and they are queued on the vq in this order only: > > > > - set direction-out-with-value-1 for line 5. > > - set direction-in for line 11. > > > > Now, if the host is able to process them in parallel, then the second > > request for line 11 may finish before the requests for line 5 (as it > > has lesser amount of work to do). > > Speaking about parallel processing, have you already envisioned > support for .[sg]et_multiple()? Not yet, but we can surely do that once someone needs it. Same goes for lots of other gpio pinwise operations. -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* [virtio-dev] [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-20 15:28 [PATCH V6 0/2] virtio: Add specification for virtio-gpio Viresh Kumar 2021-07-20 15:28 ` [PATCH V6 1/2] virtio-gpio: Add the device specification Viresh Kumar @ 2021-07-20 15:28 ` Viresh Kumar 2021-07-26 11:10 ` Arnd Bergmann 1 sibling, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-20 15:28 UTC (permalink / raw) To: Jason Wang, Michael S. Tsirkin, Arnd Bergmann, Cornelia Huck, Linus Walleij, Bartosz Golaszewski Cc: Viresh Kumar, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven This patch adds support for interrupts to the virtio-gpio specification. This uses the feature bit 0 for the same. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- conformance.tex | 2 + virtio-gpio.tex | 241 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 242 insertions(+), 1 deletion(-) diff --git a/conformance.tex b/conformance.tex index c52f1a40be2d..64bcc12d1199 100644 --- a/conformance.tex +++ b/conformance.tex @@ -310,6 +310,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \begin{itemize} \item \ref{drivernormative:Device Types / GPIO Device / requestq Operation} +\item \ref{drivernormative:Device Types / GPIO Device / eventq Operation} \end{itemize} \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance} @@ -568,6 +569,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \begin{itemize} \item \ref{devicenormative:Device Types / GPIO Device / requestq Operation} +\item \ref{devicenormative:Device Types / GPIO Device / eventq Operation} \end{itemize} \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance} diff --git a/virtio-gpio.tex b/virtio-gpio.tex index fa48780a29db..f432a3f3ad0d 100644 --- a/virtio-gpio.tex +++ b/virtio-gpio.tex @@ -11,11 +11,17 @@ \subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues} \begin{description} \item[0] requestq +\item[1] eventq \end{description} +The \field{eventq} virtqueue is available only if the \field{VIRTIO_GPIO_F_IRQ} +feature is enabled by the device. + \subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits} -None currently defined. +\begin{description} +\item[VIRTIO_GPIO_F_IRQ (0)] The device supports interrupts on GPIO lines. +\end{description} \subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout} @@ -46,6 +52,15 @@ \subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device \begin{itemize} \item The driver MUST configure and initialize the \field{requestq} virtqueue. + +\item The driver MUST check the presence of \field{VIRTIO_GPIO_F_IRQ} feature + before initiating any IRQ messages. + +\item The driver MUST configure and initialize the \field{eventq} virtqueue if + the \field{VIRTIO_GPIO_F_IRQ} feature is enabled by the device. + +\item The device MUST have interrupts on all the GPIO lines masked, if the + \field{VIRTIO_GPIO_F_IRQ} feature is supported. \end{itemize} \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation} @@ -111,6 +126,19 @@ \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / r #define VIRTIO_GPIO_MSG_SET_DIRECTION_OUT 0x0006 #define VIRTIO_GPIO_MSG_GET_VALUE 0x0007 #define VIRTIO_GPIO_MSG_SET_VALUE 0x0008 +#define VIRTIO_GPIO_MSG_IRQ_TYPE 0x0009 +#define VIRTIO_GPIO_MSG_IRQ_MASK 0x000a +#define VIRTIO_GPIO_MSG_IRQ_UNMASK 0x000b +\end{lstlisting} + +\begin{lstlisting} +/* GPIO interrupt types */ +#define VIRTIO_GPIO_IRQ_TYPE_NONE 0x00 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING 0x01 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING 0x02 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH 0x03 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH 0x04 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW 0x08 \end{lstlisting} \subsubsection{requestq Operation: Get Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Names} @@ -294,6 +322,79 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi \hline \end{tabularx} +\subsubsection{requestq Operation: IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / IRQ Type} + +The driver initiates this message to request the device to set the IRQ trigger +type, to one of the \field{VIRTIO_GPIO_IRQ_TYPE_*} values, for a line configured +for input. + +This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is +enabled by the device. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: IRQ Mask}\label{sec:Device Types / GPIO Device / requestq Operation / IRQ Mask} + +The driver initiates this message to request the device to mask IRQ on a GPIO +line configured for input. + +This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is +enabled by the device. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_IRQ_MASK} & line number & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + +\subsubsection{requestq Operation: IRQ Unmask}\label{sec:Device Types / GPIO Device / requestq Operation / IRQ Unmask} + +The driver initiates this message to request the device to unmask IRQ on a GPIO +line configured for input. + +This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is +enabled by the device. + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ +\hline +& \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} & line number & 0 \\ +\hline +\end{tabularx} + +\begin{tabularx}{\textwidth}{ |l||X|X|X| } +\hline +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\ +\hline +& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\ +\hline +\end{tabularx} + \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow} \begin{itemize} @@ -343,6 +444,17 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D \item The driver MUST NOT send another message, of any type, for a GPIO line before receiving a response from the device for the previously sent message for the same GPIO line. + +\item The driver MUST NOT send IRQ messages for a GPIO line configured for + output. + +\item The driver MUST NOT initiate IRQ messages if the \field{VIRTIO_GPIO_F_IRQ} + feature is not enabled by the device. + +\item If the driver has sent the \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} message + earlier for a GPIO line, then it MUST send the + \field{VIRTIO_GPIO_MSG_IRQ_MASK} message over the \field{requestq} + virtqueue, if it no longer wants to receive interrupts from the device. \end{itemize} \devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation} @@ -367,3 +479,130 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D \item The device MAY process messages out of order and in parallel and MAY send message's response to the driver out of order. \end{itemize} + +\subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eventq Operation} + +The \field{eventq} virtqueue is used for sending interrupt events from the +device to the driver. The driver queues a separate pair of buffers, +interrupt-request (filled by driver) and interrupt-response (to be filled by +device later), to the \field{eventq} virtqueue for each GPIO line. The device, +on sensing an interrupt, returns the pair of buffers of the respective GPIO line +for which the interrupt is sensed. + +\begin{lstlisting} +struct virtio_gpio_irq_request { + le16 gpio; +}; +\end{lstlisting} + +This structure is filled by the driver and read by the device. + +\begin{description} +\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} < + \field{ngpio}. +\end{description} + +\begin{lstlisting} +struct virtio_gpio_irq_response { + u8 status; +}; + +/* Possible values of the interrupt status field */ +#define VIRTIO_GPIO_IRQ_INVALID 0x0 +#define VIRTIO_GPIO_IRQ_VALID 0x1 +\end{lstlisting} + +This structure is filled by the device and read by the driver. + +\begin{description} +\item[\field{status}] of the interrupt event, \field{VIRTIO_GPIO_IRQ_VALID} on + valid interrupt and \field{VIRTIO_GPIO_IRQ_INVALID} otherwise on returning + the buffer back to the driver without an interrupt. +\end{description} + +\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow} + +\begin{itemize} +\item The driver is requested by a user to enable interrupt for a GPIO and + configure it to a particular type. + +\item The driver sends the \field{VIRTIO_GPIO_MSG_IRQ_TYPE} message over the + \field{requestq} virtqueue and the device configures the GPIO line for the + same and responds. + +\item The driver queues a pair of buffers, interrupt-request and + interrupt-response, to the \field{eventq} virtqueue for the GPIO line. + +\item The driver notifies the device of the presence of new buffers on the + \field{eventq} virtqueue. + +\item The driver sends the \field{VIRTIO_GPIO_MSG_IRQ_UNMASK} message over the + \field{requestq} virtqueue and the device configures the GPIO line for the + same and responds. + +\item The interrupt is fully configured at this point. + +\item The device, on sensing an interrupt on a GPIO line, finds the matching + buffers (based on GPIO line number) from the \field{eventq} virtqueue and + fills its \field{struct virtio_gpio_irq_response} buffer's \field{status} + with \field{VIRTIO_GPIO_IRQ_VALID} and returns the pair of buffers to the + device. + +\item The device notifies the driver of the presence of new buffers on the + \field{eventq} virtqueue. + +\item If the GPIO line is configured for Level interrupts, the device MUST + ignore any further interrupts for the line, until the time the buffers are + made available again by the driver. + +\item If the GPIO line is configured for Edge interrupts, the device MUST latch + the latest interrupt received for the line, until the time the buffers are + made available again by the driver. At that point, the device can again + return the buffers for the line if an interrupt was received while the + device was waiting for the buffers to be made available by the driver. + +\item The driver on receiving the notification from the device, processes the + interrupt. The driver may try to mask/unmask the interrupts for the GPIO + line over the \field{requestq} virtqueue. + +\item The driver may again queue, same or new, pair of buffers for that GPIO + line and notify the device. + +\item The driver may send the \field{VIRTIO_GPIO_MSG_IRQ_MASK} message over the + \field{requestq} virtqueue, once it no longer wants to receive the + interrupts for a GPIO line. + +\item The device must return the unused pair of buffers for that GPIO line, over + the \field{eventq} virtqueue, by setting the \field{status} field with + \field{VIRTIO_GPIO_IRQ_INVALID}. + +\item The driver can then free the associated buffer and remove it from the + \field{eventq} virtqueue. +\end{itemize} + +\drivernormative{\subsubsection}{eventq Operation}{Device Types / GPIO Device / eventq Operation} + +\begin{itemize} +\item The driver MUST queue a separate pair of buffers, interrupt-request and + interrupt-response, to the \field{eventq} virtqueue for each GPIO line for + which it is expecting an interrupt from the device. + +\item The driver MUST not queue a pair of buffers for a GPIO line which it is + not going to use as interrupt source, otherwise the buffer may never be + freed by the device (as no unmask request will follow from the driver). + +\item The driver MUST NOT add multiple pairs of buffers for the same GPIO line + on the \field{eventq} virtqueue. And so there can only be one interrupt + event for each GPIO line at any point of time. + +\item The pair of buffers for any GPIO line can either be owned by the device or + the driver at any particular point of time, but not both. +\end{itemize} + +\devicenormative{\subsubsection}{eventq Operation}{Device Types / GPIO Device / eventq Operation} + +\begin{itemize} +\item The device can send an interrupt event for a GPIO line to the driver, only + if a buffer for that GPIO line is provided by the driver in the first place + on the \field{eventq} virtqueue. +\end{itemize} -- 2.31.1.272.g89b43f80a514 --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-20 15:28 ` [virtio-dev] [PATCH V6 2/2] virtio-gpio: Add support for interrupts Viresh Kumar @ 2021-07-26 11:10 ` Arnd Bergmann 2021-07-26 11:51 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Arnd Bergmann @ 2021-07-26 11:10 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 20, 2021 at 5:28 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > This patch adds support for interrupts to the virtio-gpio specification. > This uses the feature bit 0 for the same. I think the flow of event messages already looks great with this version, and I like the level of detail in the explanations. The only part I don't like is that it seems overly redundant to have three ways to enable/disable irq delivery. More on that below. > +\item If the GPIO line is configured for Level interrupts, the device MUST > + ignore any further interrupts for the line, until the time the buffers are > + made available again by the driver. This wording sounds odd, since there are not technically any interrupt events on the level triggered line, but instead the level is active. I would describe this as "the device MUST mask the interrupt line, until the time ...", though that conflicts with the terminology for the mask/unmask request. I would still hope that we can combine the irq_type, mask and request steps into either one or two steps. I think the minimalistic approach would be to expand the message to include both the "mask" and "set-type" operations, and this should work, as long as we define sensible semantics. Another option would be to fold the "set irq type" operation into "set direction", so an input can be configured as any of the five irq types, with "IRQ_TYPE_NONE" implying that interrupts are disabled for this line. This would still provide an extra way of masking the interrupt, so we just need "set-direction" and "request event". In this case, setting the interrupt type to "none" should probably cancel any pending irq and force the message to be returned. You could actually take that further and fold the "set value" into "set direction", which also simplifies the output direction and avoids the case where can you set a gpio to output before setting the value first. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-26 11:10 ` Arnd Bergmann @ 2021-07-26 11:51 ` Viresh Kumar 2021-07-26 12:13 ` Arnd Bergmann 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-26 11:51 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 26-07-21, 13:10, Arnd Bergmann wrote: > On Tue, Jul 20, 2021 at 5:28 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > This patch adds support for interrupts to the virtio-gpio specification. > > This uses the feature bit 0 for the same. > > I think the flow of event messages already looks great with this version, and I > like the level of detail in the explanations. Thanks. > The only part I don't like is that it seems overly redundant to have three > ways to enable/disable irq delivery. More on that below. > > > +\item If the GPIO line is configured for Level interrupts, the device MUST > > + ignore any further interrupts for the line, until the time the buffers are > > + made available again by the driver. > > This wording sounds odd, since there are not technically any interrupt events > on the level triggered line, but instead the level is active. I would > describe this > as "the device MUST mask the interrupt line, until the time ...", though that > conflicts with the terminology for the mask/unmask request. Right. > I would still hope that we can combine the irq_type, mask and request request ? you wanted to say "unmask" ? > steps into either one or two steps. I think the minimalistic approach would > be to expand the message to include both the "mask" and "set-type" > operations, and this should work, as long as we define sensible semantics. I think we can easily merge unmask and set-type into a single message, since that will fit within the request structure used for most of the requests on the requestq. i.e. a single byte of data. mask() doesn't require irq-type, so it can live separately. > Another option would be to fold the "set irq type" operation into "set > direction", Yeah, I would like to keep them separate and not mix irq-type with directions in general, as you can set direction to input with or without setting its interrupt type. Over that interrupt support is enabled with a feature, so better keep it separate. > so an input can be configured as any of the five irq types, with > "IRQ_TYPE_NONE" implying that interrupts are disabled for this line. > This would still provide an extra way of masking the interrupt, so we just > need "set-direction" and "request event". In this case, setting the interrupt > type to "none" should probably cancel any pending irq and force the > message to be returned. > > You could actually take that further and fold the "set value" into "set > direction", At least gpio-direction-out already takes a value with it. > which also simplifies the output direction and avoids the > case where can you set a gpio to output before setting the value first. Yeah, that is already done. -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-26 11:51 ` Viresh Kumar @ 2021-07-26 12:13 ` Arnd Bergmann 2021-07-27 6:23 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Arnd Bergmann @ 2021-07-26 12:13 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Mon, Jul 26, 2021 at 1:51 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 26-07-21, 13:10, Arnd Bergmann wrote: > > > I would still hope that we can combine the irq_type, mask and request > > request ? you wanted to say "unmask" ? No, I meant VIRTIO_GPIO_MSG_IRQ_TYPE, VIRTIO_GPIO_MSG_IRQ_{UN,}MASK and virtio_gpio_irq_request messages. All three can change the state between 'can interrupt' and 'can not interrupt', so you only get notified if all three are in the active state (irq type != none, mask disabled, request queued). > > steps into either one or two steps. I think the minimalistic approach would > > be to expand the message to include both the "mask" and "set-type" > > operations, and this should work, as long as we define sensible semantics. > > I think we can easily merge unmask and set-type into a single message, > since that will fit within the request structure used for most of the > requests on the requestq. i.e. a single byte of data. > > mask() doesn't require irq-type, so it can live separately. The question is whether it adds any value though. As far as I can tell, it is completely redundant, duplicating either what is implied by irq type==none, or what is implied by the request not being queued. > > so an input can be configured as any of the five irq types, with > > "IRQ_TYPE_NONE" implying that interrupts are disabled for this line. > > This would still provide an extra way of masking the interrupt, so we just > > need "set-direction" and "request event". In this case, setting the interrupt > > type to "none" should probably cancel any pending irq and force the > > message to be returned. > > > > You could actually take that further and fold the "set value" into "set > > direction", > > At least gpio-direction-out already takes a value with it. Ok. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-26 12:13 ` Arnd Bergmann @ 2021-07-27 6:23 ` Viresh Kumar 2021-07-27 8:03 ` Arnd Bergmann 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 6:23 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 26-07-21, 14:13, Arnd Bergmann wrote: > On Mon, Jul 26, 2021 at 1:51 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-07-21, 13:10, Arnd Bergmann wrote: > > > > > I would still hope that we can combine the irq_type, mask and request > > > > request ? you wanted to say "unmask" ? > > No, I meant VIRTIO_GPIO_MSG_IRQ_TYPE, Ah, I was confused since this request type isn't there anymore. > VIRTIO_GPIO_MSG_IRQ_{UN,}MASK and virtio_gpio_irq_request messages. > > All three can change the state between 'can interrupt' and 'can not interrupt', > so you only get notified if all three are in the active state (irq type != none, > mask disabled, request queued). > > > > steps into either one or two steps. I think the minimalistic approach would > > > be to expand the message to include both the "mask" and "set-type" > > > operations, and this should work, as long as we define sensible semantics. > > > > I think we can easily merge unmask and set-type into a single message, > > since that will fit within the request structure used for most of the > > requests on the requestq. i.e. a single byte of data. > > > > mask() doesn't require irq-type, so it can live separately. > > The question is whether it adds any value though. As far as I can tell, > it is completely redundant, duplicating either what is implied by > irq type==none, or what is implied by the request not being queued. Right, we shouldn't have redundant stuff in here. Though, binding of all interrupt operations to the eventq can be trick IMO. - How will the driver mask an interrupt for which the buffer is already sent over the eventq? There is no API available to recall a buffer. So we would need an API over the requestq for that. - Latching of edge interrupt between the buffer is returned by the device and requeued by the driver. How will the device know if the user has simply requeued the eventq buffer (where the latched value is meaningful) or it has done something like free_irq() followed by request_irq() in the kernel (where the latched value must be discarded). -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-27 6:23 ` Viresh Kumar @ 2021-07-27 8:03 ` Arnd Bergmann 2021-07-27 11:01 ` [virtio-dev] " Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Arnd Bergmann @ 2021-07-27 8:03 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 27, 2021 at 8:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 26-07-21, 14:13, Arnd Bergmann wrote: > > The question is whether it adds any value though. As far as I can tell, > > it is completely redundant, duplicating either what is implied by > > irq type==none, or what is implied by the request not being queued. > > Right, we shouldn't have redundant stuff in here. > > Though, binding of all interrupt operations to the eventq can be trick > IMO. > > - How will the driver mask an interrupt for which the buffer is > already sent over the eventq? There is no API available to recall a > buffer. So we would need an API over the requestq for that. Agreed, there needs to be some way to cancel the request. I don't think this would have to be a fast or common operation, so setting the line back to IRQ_TYPE_NONE would be one way. Another option would be to make the mask software-only and just ignore an incoming event for a line that is configured as masked, and then not requeue the request. > - Latching of edge interrupt between the buffer is returned by the > device and requeued by the driver. How will the device know if the > user has simply requeued the eventq buffer (where the latched value > is meaningful) or it has done something like free_irq() followed by > request_irq() in the kernel (where the latched value must be > discarded). I was expecting a 'mask' to not discard the latched event but simply not deliver it until it gets unmasked. Either way, we can define that a new set-irq-type request discards all latched edge interrupts. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [virtio-dev] Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-27 8:03 ` Arnd Bergmann @ 2021-07-27 11:01 ` Viresh Kumar 2021-07-27 11:16 ` Arnd Bergmann 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 11:01 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 27-07-21, 10:03, Arnd Bergmann wrote: > On Tue, Jul 27, 2021 at 8:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 26-07-21, 14:13, Arnd Bergmann wrote: > > > The question is whether it adds any value though. As far as I can tell, > > > it is completely redundant, duplicating either what is implied by > > > irq type==none, or what is implied by the request not being queued. > > > > Right, we shouldn't have redundant stuff in here. > > > > Though, binding of all interrupt operations to the eventq can be trick > > IMO. > > > > - How will the driver mask an interrupt for which the buffer is > > already sent over the eventq? There is no API available to recall a > > buffer. So we would need an API over the requestq for that. > > Agreed, there needs to be some way to cancel the request. I don't > think this would have to be a fast or common operation, so setting > the line back to IRQ_TYPE_NONE would be one way. Hmm, how would that work ? I thought you were talking about inferring all irq operations from the presence of the buffer in the eventq itself. So once the buffer is sent to the host, we can't send it again to change the irq-type. > Another option would be to make the mask software-only and > just ignore an incoming event for a line that is configured as > masked, and then not requeue the request. Hmm, I wonder what will happen if the driver gets removed/probed again after sending the buffer :) > > - Latching of edge interrupt between the buffer is returned by the > > device and requeued by the driver. How will the device know if the > > user has simply requeued the eventq buffer (where the latched value > > is meaningful) or it has done something like free_irq() followed by > > request_irq() in the kernel (where the latched value must be > > discarded). > > I was expecting a 'mask' to not discard the latched event but simply > not deliver it until it gets unmasked. Right, we are aligned on that. Masked == buffer is owned by driver here. > Either way, we can define that > a new set-irq-type request discards all latched edge interrupts. Hmm, so you proposing to keep a separate (single) request for irq handling over the controlq ? -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [virtio-dev] Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-27 11:01 ` [virtio-dev] " Viresh Kumar @ 2021-07-27 11:16 ` Arnd Bergmann 2021-07-27 11:26 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Arnd Bergmann @ 2021-07-27 11:16 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 27, 2021 at 1:01 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 27-07-21, 10:03, Arnd Bergmann wrote: > > On Tue, Jul 27, 2021 at 8:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > On 26-07-21, 14:13, Arnd Bergmann wrote: > > > > The question is whether it adds any value though. As far as I can tell, > > > > it is completely redundant, duplicating either what is implied by > > > > irq type==none, or what is implied by the request not being queued. > > > > > > Right, we shouldn't have redundant stuff in here. > > > > > > Though, binding of all interrupt operations to the eventq can be trick > > > IMO. > > > > > > - How will the driver mask an interrupt for which the buffer is > > > already sent over the eventq? There is no API available to recall a > > > buffer. So we would need an API over the requestq for that. > > > > Agreed, there needs to be some way to cancel the request. I don't > > think this would have to be a fast or common operation, so setting > > the line back to IRQ_TYPE_NONE would be one way. > > Hmm, how would that work ? I thought you were talking about inferring > all irq operations from the presence of the buffer in the eventq > itself. So once the buffer is sent to the host, we can't send it again > to change the irq-type. You are right, I was getting ahead of myself here: we could do this if the set-irq-type oepration gets folded into set-direction as I hinted as one of many options, but it does not work if the set-irq-type and ack operations are both folded into queuing a request on the event queue. > > Another option would be to make the mask software-only and > > just ignore an incoming event for a line that is configured as > > masked, and then not requeue the request. > > Hmm, I wonder what will happen if the driver gets removed/probed again > after sending the buffer :) Removing the driver would disable the line (or set it to direction_none if we go there) and shut down the virtqueue, so both of these should lead to no event getting latched any more. > > > - Latching of edge interrupt between the buffer is returned by the > > > device and requeued by the driver. How will the device know if the > > > user has simply requeued the eventq buffer (where the latched value > > > is meaningful) or it has done something like free_irq() followed by > > > request_irq() in the kernel (where the latched value must be > > > discarded). > > > > I was expecting a 'mask' to not discard the latched event but simply > > not deliver it until it gets unmasked. > > Right, we are aligned on that. Masked == buffer is owned by driver > here. > > > Either way, we can define that > > a new set-irq-type request discards all latched edge interrupts. > > Hmm, so you proposing to keep a separate (single) request for irq > handling over the controlq ? This was under the assumption that we decide to still keep some controlq request that sets the irq type and just remove the 'mask' command. If we go all the way to having only one message for interrupts, I suppose it does get a little uglier than I was hoping for, but it would still be doable: in this case, we could allow a flow like this on the eventq: - driver requests edge interrupts - (no event happened, so request remains pending) - driver queues a new request asking for IRQ_TYPE_NONE notification in order to mask this line - device replies to both requests saying no interrupt happened Between this control flow and the version where set-type is part of set-direction, I would prefer the other option, but you already said that you don't like that one. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [virtio-dev] Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-27 11:16 ` Arnd Bergmann @ 2021-07-27 11:26 ` Viresh Kumar 2021-07-27 12:42 ` Arnd Bergmann 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2021-07-27 11:26 UTC (permalink / raw) To: Arnd Bergmann Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On 27-07-21, 13:16, Arnd Bergmann wrote: > This was under the assumption that we decide to still keep some > controlq request that sets the irq type and just remove the 'mask' > command. Right, so I think I can fold all three of irq messages, mask, unmask and type, to a single message type. Where the value of type can play the role of masking/unmasking. This should work fine, I will see if I get any more doubts on this. > If we go all the way to having only one message for interrupts, I > suppose it does get a little uglier than I was hoping for, but it would still > be doable: in this case, we could allow a flow like this on the eventq: > > - driver requests edge interrupts This over eventq and .. > - (no event happened, so request remains pending) > - driver queues a new request asking for IRQ_TYPE_NONE notification > in order to mask this line This over controlq. Right ? > - device replies to both requests saying no interrupt happened Yes, that can work. > Between this control flow and the version where set-type is part of > set-direction, I would prefer the other option, but you already said > that you don't like that one. Yeah, I would like to keep this away from set-direction, which I am already going to use for activate/deactivate :) -- viresh ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [virtio-dev] Re: [PATCH V6 2/2] virtio-gpio: Add support for interrupts 2021-07-27 11:26 ` Viresh Kumar @ 2021-07-27 12:42 ` Arnd Bergmann 0 siblings, 0 replies; 26+ messages in thread From: Arnd Bergmann @ 2021-07-27 12:42 UTC (permalink / raw) To: Viresh Kumar Cc: Jason Wang, Michael S. Tsirkin, Cornelia Huck, Linus Walleij, Bartosz Golaszewski, Vincent Guittot, Jean-Philippe Brucker, Bill Mills, Alex Bennée, Enrico Weigelt, metux IT consult, virtio-dev, Geert Uytterhoeven On Tue, Jul 27, 2021 at 1:26 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 27-07-21, 13:16, Arnd Bergmann wrote: > > This was under the assumption that we decide to still keep some > > controlq request that sets the irq type and just remove the 'mask' > > command. > > Right, so I think I can fold all three of irq messages, mask, unmask > and type, to a single message type. Where the value of type can play > the role of masking/unmasking. This should work fine, I will see if I > get any more doubts on this. > > > If we go all the way to having only one message for interrupts, I > > suppose it does get a little uglier than I was hoping for, but it would still > > be doable: in this case, we could allow a flow like this on the eventq: > > > > - driver requests edge interrupts > > This over eventq and .. > > > - (no event happened, so request remains pending) > > - driver queues a new request asking for IRQ_TYPE_NONE notification > > in order to mask this line > > This over controlq. Right ? Sorry for not being too clear: this was meant as the example where both messages are on the eventq. It would work just as well if the irq type setting is a message on the controlq though, so I suppose it doesn't matter what I was thinking as long as we end up with a working design ;-) > > - device replies to both requests saying no interrupt happened > > Yes, that can work. > > > Between this control flow and the version where set-type is part of > > set-direction, I would prefer the other option, but you already said > > that you don't like that one. > > Yeah, I would like to keep this away from set-direction, which I am > already going to use for activate/deactivate :) Ok, so the set of relevant messages here would be: - controlq: set direction (includes enable, but not set-irq-type) - controlq: set-irq-type (may cancel outstanding eventq request) - eventq: request notification (implies unmask, reply implies mask) This sounds like a reasonably consistent and efficient way to do it, I'm happy enough with that design for the next version. Arnd ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-07-27 14:38 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-20 15:28 [PATCH V6 0/2] virtio: Add specification for virtio-gpio Viresh Kumar 2021-07-20 15:28 ` [PATCH V6 1/2] virtio-gpio: Add the device specification Viresh Kumar 2021-07-26 9:18 ` [virtio-dev] " Arnd Bergmann 2021-07-26 10:16 ` Viresh Kumar 2021-07-26 11:28 ` Arnd Bergmann 2021-07-27 7:55 ` Viresh Kumar 2021-07-27 8:16 ` Arnd Bergmann 2021-07-27 10:56 ` Viresh Kumar 2021-07-27 12:06 ` Arnd Bergmann 2021-07-27 14:17 ` Viresh Kumar 2021-07-27 14:20 ` Viresh Kumar 2021-07-27 14:38 ` Arnd Bergmann 2021-07-27 10:57 ` [virtio-dev] " Cornelia Huck 2021-07-27 11:21 ` Viresh Kumar 2021-07-26 11:30 ` Geert Uytterhoeven 2021-07-26 11:55 ` Viresh Kumar 2021-07-20 15:28 ` [virtio-dev] [PATCH V6 2/2] virtio-gpio: Add support for interrupts Viresh Kumar 2021-07-26 11:10 ` Arnd Bergmann 2021-07-26 11:51 ` Viresh Kumar 2021-07-26 12:13 ` Arnd Bergmann 2021-07-27 6:23 ` Viresh Kumar 2021-07-27 8:03 ` Arnd Bergmann 2021-07-27 11:01 ` [virtio-dev] " Viresh Kumar 2021-07-27 11:16 ` Arnd Bergmann 2021-07-27 11:26 ` Viresh Kumar 2021-07-27 12:42 ` Arnd Bergmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.