From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 30 Jul 2021 09:52:16 +0100 Message-ID: <87a6m42ngf.wl-maz@kernel.org> From: Marc Zyngier Subject: Re: [PATCH V8 2/2] virtio-gpio: Add support for interrupts In-Reply-To: <33671b4e543557740acf85bc30ed8ad1ba9ef8aa.1627627021.git.viresh.kumar@linaro.org> References: <33671b4e543557740acf85bc30ed8ad1ba9ef8aa.1627627021.git.viresh.kumar@linaro.org> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII To: Viresh Kumar Cc: Jason Wang , "Michael S. Tsirkin" , Arnd Bergmann , Cornelia Huck , Linus Walleij , Bartosz Golaszewski , Vincent Guittot , Jean-Philippe Brucker , Bill Mills , Alex =?UTF-8?B?QmVubsOpZQ==?= , "Enrico Weigelt, metux IT consult" , virtio-dev@lists.oasis-open.org, Geert Uytterhoeven , stratos-dev@op-lists.linaro.org, Thomas Gleixner List-ID: On Fri, 30 Jul 2021 07:45:03 +0100, Viresh Kumar wrote: > > This patch adds support for interrupts to the virtio-gpio specification. > This uses the feature bit 0 for the same. > > Reviewed-by: Linus Walleij > Signed-off-by: Viresh Kumar > --- > conformance.tex | 2 + > virtio-gpio.tex | 234 +++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 235 insertions(+), 1 deletion(-) > > diff --git a/conformance.tex b/conformance.tex > index c52f1a40be2d..64bcc12d1199 100644 > --- a/conformance.tex > +++ b/conformance.tex > @@ -310,6 +310,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} > > \begin{itemize} > \item \ref{drivernormative:Device Types / GPIO Device / requestq Operation} > +\item \ref{drivernormative:Device Types / GPIO Device / eventq Operation} > \end{itemize} > > \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance} > @@ -568,6 +569,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} > > \begin{itemize} > \item \ref{devicenormative:Device Types / GPIO Device / requestq Operation} > +\item \ref{devicenormative:Device Types / GPIO Device / eventq Operation} > \end{itemize} > > \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance} > diff --git a/virtio-gpio.tex b/virtio-gpio.tex > index 1d1ac672db37..d5746fa883c0 100644 > --- a/virtio-gpio.tex > +++ b/virtio-gpio.tex > @@ -11,11 +11,17 @@ \subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues} > > \begin{description} > \item[0] requestq > +\item[1] eventq > \end{description} > > +The \field{eventq} virtqueue is available only if the \field{VIRTIO_GPIO_F_IRQ} > +feature is enabled by the device. > + > \subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits} > > -None currently defined. > +\begin{description} > +\item[VIRTIO_GPIO_F_IRQ (0)] The device supports interrupts on GPIO lines. > +\end{description} > > \subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout} > > @@ -46,6 +52,15 @@ \subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device > > \begin{itemize} > \item The driver MUST configure and initialize the \field{requestq} virtqueue. > + > +\item The driver MUST check the presence of \field{VIRTIO_GPIO_F_IRQ} feature > + before initiating any IRQ messages. > + > +\item The driver MUST configure and initialize the \field{eventq} virtqueue if > + the \field{VIRTIO_GPIO_F_IRQ} feature is enabled by the device. > + > +\item If the \field{VIRTIO_GPIO_F_IRQ} feature is supported, then the interrupt > + for all GPIO lines must be in \field{VIRTIO_GPIO_IRQ_TYPE_NONE} state. > \end{itemize} > > \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation} > @@ -105,11 +120,20 @@ \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / r > #define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003 > #define VIRTIO_GPIO_MSG_GET_VALUE 0x0004 > #define VIRTIO_GPIO_MSG_SET_VALUE 0x0005 > +#define VIRTIO_GPIO_MSG_SET_IRQ_TYPE 0x0006 > > /* GPIO Direction types */ > #define VIRTIO_GPIO_DIRECTION_NONE 0x00 > #define VIRTIO_GPIO_DIRECTION_OUT 0x01 > #define VIRTIO_GPIO_DIRECTION_IN 0x02 > + > +/* GPIO interrupt types */ > +#define VIRTIO_GPIO_IRQ_TYPE_NONE 0x00 > +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING 0x01 > +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING 0x02 > +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH 0x03 > +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH 0x04 > +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW 0x08 > \end{lstlisting} > > \subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names} > @@ -269,6 +293,66 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi > \hline > \end{tabularx} > > +\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type} > + > +This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is enabled > +by the device. > + > +In order to configure and receive interrupts for a GPIO line, the driver needs > +to perform two operations. The driver first needs to send the > +\field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the \field{requestq} > +virtqueue, and then queue a pair of buffers, of type > +\field{virtio_gpio_irq_request} and \field{virtio_gpio_irq_response}, over the > +\field{eventq} virtqueue for the GPIO line. A separate pair of buffers must be > +queued for each GPIO line, the driver wants to configure for interrupts. > + > +The driver sets the trigger type to values other than > +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, to unmask the interrupt and get notified over > +the \field{eventq} virtqueue. The driver sets the trigger type to > +\field{VIRTIO_GPIO_IRQ_TYPE_NONE} to mask the interrupt, and get back the unused > +buffers over the \field{eventq} virtqueue. > + > +Once the buffers are made available by the driver over the \field{eventq} > +virtqueue, the device can notify the driver of an active interrupt signaled on > +the GPIO line by returning the buffers to the driver. > + > +If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST > +mask the interrupt for the GPIO line and discard any latched interrupt event > +associated with it. The device MUST also return any unused pair of buffers > +for the GPIO line, over the \field{eventq} virtqueue, after setting the > +\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. If the driver > +queues another pair of buffers, while the trigger type remains set as > +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of > +type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers. > + > +The device MUST unmask the interrupt for a GPIO line, if the trigger type > +received from driver is any valid value other than > +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}. For edge trigger type, the device MUST latch > +the interrupt event from this point onward and report it to the driver as soon > +as a buffers are available. For level trigger type, the device MUST ignore the > +active interrupts signaled on the line, until the time the buffers are made > +available by the driver. The device MUST keep on notifying the driver of the > +interrupts, as and when an interrupt becomes active and the buffers are > +available on the \field{eventq} virtqueue. The device MUST stop notifying the > +driver, once the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, and > +return any unused buffers. > + > +\begin{tabularx}{\textwidth}{ |l||X|X|X| } > +\hline > +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\ > +\hline > +& \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \\ > +\hline > +\end{tabularx} > + > +\begin{tabularx}{\textwidth}{ |l||X|X| } > +\hline > +\textbf{Response} & \field{status} & \field{value} \\ > +\hline > +& \field{VIRTIO_GPIO_STATUS_*} & 0 \\ > +\hline > +\end{tabularx} > + > \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow} > > \begin{itemize} > @@ -312,6 +396,16 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D > > \item The driver MAY send multiple messages for same or different GPIO lines in > parallel. > + > +\item The driver MUST NOT send IRQ messages for a GPIO line configured for > + output. > + > +\item The driver MUST NOT send IRQ messages if the \field{VIRTIO_GPIO_F_IRQ} > + feature is not enabled by the device. > + > +\item The driver SHOULD set the IRQ trigger type to > + \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line, > + previously configured for a different trigger type. > \end{itemize} > > \devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation} > @@ -344,3 +438,141 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D > line, once the driver has requested to set its direction to > \field{VIRTIO_GPIO_DIRECTION_NONE}. > \end{itemize} > + > +\subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eventq Operation} > + > +The \field{eventq} virtqueue is used for sending interrupt events from the > +device to the driver. The driver queues a separate pair of buffers, > +\field{struct virtio_gpio_irq_request} (filled by driver) and \field{struct > +virtio_gpio_irq_response} (to be filled by device later), to the \field{eventq} > +virtqueue for each GPIO line. The device, on sensing an active interrupt, > +returns the pair of buffers of the respective GPIO line for which the interrupt > +is active. > + > +\begin{lstlisting} > +struct virtio_gpio_irq_request { > + le16 gpio; > +}; > +\end{lstlisting} > + > +This structure is filled by the driver and read by the device. > + > +\begin{description} > +\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} < > + \field{ngpio}. > +\end{description} > + > +\begin{lstlisting} > +struct virtio_gpio_irq_response { > + u8 status; > +}; > + > +/* Possible values of the interrupt status field */ > +#define VIRTIO_GPIO_IRQ_STATUS_INVALID 0x0 > +#define VIRTIO_GPIO_IRQ_STATUS_VALID 0x1 > +\end{lstlisting} > + > +This structure is filled by the device and read by the driver. > + > +\begin{description} > +\item[\field{status}] of the interrupt event, > + \field{VIRTIO_GPIO_IRQ_STATUS_VALID} on valid interrupt and > + \field{VIRTIO_GPIO_IRQ_STATUS_INVALID} otherwise on returning the buffer > + back to the driver without an interrupt. > +\end{description} > + > +\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow} > + > +\begin{itemize} > +\item The driver is requested by a client driver to enable interrupt for a GPIO > + line and configure it to a particular trigger type. > + > +\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the > + \field{requestq} virtqueue and the device configures the GPIO line for the > + requested trigger type and unmasks the interrupt. > + > +\item The driver queues a pair of buffers, interrupt-request and > + interrupt-response, to the \field{eventq} virtqueue for the GPIO line. > + > +\item The driver notifies the device of the presence of new buffers on the > + \field{eventq} virtqueue. > + > +\item The interrupt is fully configured at this point. > + > +\item The device, on sensing an active interrupt on the GPIO line, finds the > + matching buffers (based on GPIO line number) from the \field{eventq} > + virtqueue and fills its \field{struct virtio_gpio_irq_response} buffer's > + \field{status} with \field{VIRTIO_GPIO_IRQ_STATUS_VALID} and returns the > + pair of buffers to the device. > + > +\item The device notifies the driver of the presence of returned buffers on the > + \field{eventq} virtqueue. > + > +\item If the GPIO line is configured for level interrupts, the device MUST > + ignore an active interrupt signaled on this GPIO line, until the time the > + buffers are made available again by the driver. Once the buffers are > + available again, and the interrupt on the line is still active, the device > + MUST notify the driver again of an interrupt event. It feels like there is a problem here. A virtio device signals interrupts by using edge triggered signalling. Nothing wrong with that. However, signalling a level over a an edge signalling is much more tricky. For example, let's assume that the irqchip driver handles a level interrupt: it gets the message from the device indicating that a GPIO level interrupt is pending. During the handling, the interrupt is made pending again, without having ever transitioned via an inactive state. If you don't have a mechanism to retrigger the level, you have lost an interrupt. I can't see anything in this document that hints at a way to resample/retrigger a level interrupt, which is what you would normally do on EOI for an interrupt controller that implements level-over-edge signalling. > + > +\item If the GPIO line is configured for edge interrupts, the device MUST latch > + the latest interrupt received for this GPIO line, until the time the buffers > + are made available again by the driver. At that point, the device MUST > + notify the driver if an interrupt was received while the device was waiting > + for the buffers to be made available by the driver. If the interrupt becomes > + active at a later point of time, then the device MUST notify the driver at > + that instance. > + > +\item The driver on receiving the notification from the device, processes the > + interrupt. The driver may try to update the trigger-type of the interrupt > + for the GPIO line over the \field{requestq} virtqueue at this point. > + > +\item In a typical guest operating system kernel, the virtio-gpio driver > + notifies the client driver, that is associated with this gpio line, to > + process the event. > + > +\item In the case of a level triggered interrupt, the client driver must fully > + process and acknowledge the event at its source to return the line to its > + inactive state. This is not enough. Take a timer, for example: the interrupt fires, and the handler directly reprograms it so that it fires again immediately. The line never went down, and yet the interrupt has been handled. This isn't an imaginary case. This happens *all the time*. Thanks, M. -- Without deviation from the norm, progress is not possible.