[virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Viresh Kumar]

virtio-gpio is a virtual GPIO controller. It provides a way to flexibly
communicate with the host GPIO controllers from the guest.

This patch adds the specification for it.

Based on the initial work done by:
"Enrico Weigelt, metux IT consult" <lkml@metux.net>.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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.
---
 conformance.tex |  26 ++++-
 content.tex     |   1 +
 virtio-gpio.tex | 260 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 283 insertions(+), 4 deletions(-)
 create mode 100644 virtio-gpio.tex

diff --git a/conformance.tex b/conformance.tex
index a164cbb69093..5341abe096c2 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -29,8 +29,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}
@@ -52,8 +53,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}
@@ -288,6 +290,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Device Types / SCMI Device / Device Operation / Setting Up eventq Buffers}
 \end{itemize}
 
+\conformance{\subsection}{GPIO Driver Conformance}\label{sec:Conformance / Driver Conformance / GPIO Driver Conformance}
+
+An GPIO driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / GPIO Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -527,6 +537,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / SCMI Device / Device Operation / Shared Memory Operation}
 \end{itemize}
 
+\conformance{\subsection}{GPIO Device Conformance}\label{sec:Conformance / Device Conformance / GPIO Device Conformance}
+
+An GPIO device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / GPIO Device / Device Operation}
+\end{itemize}
+
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
 A conformant implementation MUST be either transitional or
 non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index d9913d056317..e572ac3bb6c0 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..998711864175
--- /dev/null
+++ b/virtio-gpio.tex
@@ -0,0 +1,260 @@
+\section{GPIO Device}\label{sec:Device Types / GPIO Device}
+
+virtio-gpio is a virtual general purpose IO controller device. It provides a way
+to access the host GPIO devices from the guest. This device provides a hardware
+independent interface between the host and the guests. It allows the host to
+club together GPIO lines from otherwise independent GPIO controllers and present
+them as a single GPIO controller at the guest.
+
+\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] command
+\item[1] interrupt
+\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}
+
+All fields of this configuration are always available and are read-only for the driver.
+
+\begin{lstlisting}
+struct virtio_gpio_config {
+    char name[32];
+    u16 ngpio;
+    u16 padding;
+    u32 gpio_names_size;
+    char gpio_names[0];
+} __attribute__((packed));
+\end{lstlisting}
+
+\begin{description}
+\item[\field{name}] is a null-terminated string that represents the name of the
+    GPIO controller.
+\item[\field{ngpio}] is the total number of GPIO lines provided by the
+    controller.
+\item[\field{gpio_names_size}] is the size of the \field{gpio_names} string.
+    This field must be set to 0, if the \field(gpio_names) is not used.
+\item[\field{gpio_names}] is a stream of \field{ngpio} null-terminated strings,
+    where each string corresponds to a GPIO line.
+\end{description}
+
+
+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization}
+
+\begin{enumerate}
+\item The virtqueue is initialized.
+\end{enumerate}
+
+\subsection{Device Operation}\label{sec:Device Types / GPIO Device / Device Operation}
+
+The operations of a virtio GPIO device are almost always driven by the guest.
+The guest initiates one of the requests from \field{VIRTIO_GPIO_REQ_*} on the
+\field{command} virtqueue and the host responds synchronously on the same
+virtqueue with a response message. The host initiates an operation
+(\field{VIRTIO_GPIO_IRQ_EVENT}) only for reporting the detection of an interrupt
+on a GPIO line and uses the \field{interrupt} virtqueue for the same.
+
+\begin{lstlisting}
+/* GPIO request types */
+#define VIRTIO_GPIO_REQ_ACTIVATE                0x01
+#define VIRTIO_GPIO_REQ_DEACTIVATE              0x02
+#define VIRTIO_GPIO_REQ_GET_DIRECTION           0x03
+#define VIRTIO_GPIO_REQ_DIRECTION_IN            0x04
+#define VIRTIO_GPIO_REQ_DIRECTION_OUT           0x05
+#define VIRTIO_GPIO_REQ_GET_VALUE               0x06
+#define VIRTIO_GPIO_REQ_SET_VALUE               0x07
+#define VIRTIO_GPIO_REQ_IRQ_TYPE                0x08
+#define VIRTIO_GPIO_REQ_IRQ_MASK                0x09
+#define VIRTIO_GPIO_REQ_IRQ_UNMASK              0x0a
+#define VIRTIO_GPIO_IRQ_EVENT                   0x0b
+\end{lstlisting}
+
+\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / GPIO Device / Device Operation: Request Queue}
+
+The communication between the host and the guest take place using a pair of
+request and response messages. The virtio GPIO specification defines two request
+and two response layouts. The particular request/response pair used for each
+GPIO request type is specific later in the request specific sections.
+
+Supported request and response types:
+
+\begin{lstlisting}
+/* Virtio GPIO Request */
+struct virtio_gpio_request {
+        u16 type;
+        u16 gpio;
+        u8 data;
+} __attribute__((packed));
+
+/* Virtio GPIO Response */
+struct virtio_gpio_response {
+        u8 status;
+        u8 data;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{type}] GPIO request type.
+\item[\field{gpio}] GPIO line number.
+\item[\field{data}] Request/Response specific data, not used by all
+	request/response messages.
+\item[\field{status}] Status of the request, success or failure.
+\end{description}
+
+Here is the list of different values these fields can contain based on the
+specific request type.
+
+\begin{lstlisting}
+/* GPIO line direction */
+#define VIRTIO_GPIO_DIRECTION_OUT               0x0
+#define VIRTIO_GPIO_DIRECTION_IN                0x1
+
+/* GPIO line interrupt type */
+#define VIRTIO_GPIO_IRQ_TYPE_NONE               0x00
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING        0x01
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING       0x02
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH          0x03
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH         0x04
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW          0x08
+
+/* Possible values of the status field */
+#define VIRTIO_GPIO_STATUS_OK                   0x0
+#define VIRTIO_GPIO_STATUS_ERR                  0x1
+\end{lstlisting}
+
+\subsubsection{Device Operation: Activate}\label{sec:Device Types / GPIO Device / Device Operation / Activate }
+
+The \field{VIRTIO_GPIO_REQ_ACTIVATE} request is initiated by the guest to
+request the host to activate one of the GPIO lines for use. The guest sends the
+\field{struct virtio_gpio_request} to the host, and the host responds with
+\field{struct virtio_gpio_response}. Neither the request, nor the response uses
+the \field{data} field.
+
+\subsubsection{Device Operation: Deactivate}\label{sec:Device Types / GPIO Device / Device Operation / Deactivate }
+
+The \field{VIRTIO_GPIO_REQ_DEACTIVATE} request is initiated by the guest to
+request the host to deactivate one of the GPIO lines guest was previously using.
+The guest sends the \field{struct virtio_gpio_request} to the host, and the host
+responds with \field{struct virtio_gpio_response}. Neither the request, nor the
+response uses the \field{data} field.
+
+\subsubsection{Device Operation: Get Direction}\label{sec:Device Types / GPIO Device / Device Operation / Get Direction }
+
+The \field{VIRTIO_GPIO_REQ_GET_DIRECTION} request is initiated by the guest to
+request the host to return a GPIO line's configured direction. The guest sends
+the \field{struct virtio_gpio_request} to the host, and the host responds with
+\field{struct virtio_gpio_response}. The host shall set the \field{data} field
+in the response to \field{VIRTIO_GPIO_DIRECTION_OUT} or
+\field{VIRTIO_GPIO_DIRECTION_IN}. The request doesn't use the \field{data}
+field.
+
+\subsubsection{Device Operation: Direction In}\label{sec:Device Types / GPIO Device / Device Operation / Direction In }
+
+The \field{VIRTIO_GPIO_REQ_DIRECTION_IN} request is initiated by the guest to
+request the host to configure a GPIO line for input direction. The guest sends the
+\field{struct virtio_gpio_request} to the host, and the host responds with
+\field{struct virtio_gpio_response}. Neither the request, nor the response uses
+the \field{data} field.
+
+\subsubsection{Device Operation: Direction Out}\label{sec:Device Types / GPIO Device / Device Operation / Direction Out }
+
+The \field{VIRTIO_GPIO_REQ_DIRECTION_OUT} request is initiated by the guest to
+request the host to configure a GPIO line for output direction with an initial
+output value (0 for low, 1 for high). The guest sends the \field{struct
+virtio_gpio_request} to the host with initial output value set in the
+\field{data} field, and the host responds with \field{struct
+virtio_gpio_response}. The responds doesn't use the \field{data} field.
+
+\subsubsection{Device Operation: Get Value}\label{sec:Device Types / GPIO Device / Device Operation / Get Value }
+
+The \field{VIRTIO_GPIO_REQ_GET_VALUE} request is initiated by the guest to
+request the host to return the current value sensed on a GPIO line (0 for low, 1
+for high) configured for input. The guest sends the \field{struct
+virtio_gpio_request} to the host, and the host responds responds with
+\field{struct virtio_gpio_response} with its \field{data} field set to GPIO's
+value. The request doesn't use the \field{data} field.
+
+\subsubsection{Device Operation: Set Value}\label{sec:Device Types / GPIO Device / Device Operation / Set Value }
+
+The \field{VIRTIO_GPIO_REQ_SET_VALUE} request is initiated by the guest to
+request the host to set the value (0 for low, 1 for high) for a GPIO line
+configured for output. The guest sends the \field{struct virtio_gpio_request}
+to the host, with the output value set in the \field{data} field, and the host
+responds with \field{struct virtio_gpio_response}. The responds doesn't use the
+\field{data} field.
+
+\subsubsection{Device Operation: IRQ Type}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Type }
+
+The \field{VIRTIO_GPIO_REQ_IRQ_TYPE} request is initiated by the guest to
+request the host to set the IRQ trigger type (one of
+\field{VIRTIO_GPIO_IRQ_TYPE_*}) for a GPIO line configured for input. The guest
+sends the \field{struct virtio_gpio_request} to the host, with the IRQ trigger
+type set in the \field{data} field, and the host responds with \field{struct
+virtio_gpio_response}. The responds doesn't use the \field{data} field.
+
+\subsubsection{Device Operation: IRQ Mask}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Mask }
+
+The \field{VIRTIO_GPIO_REQ_IRQ_MASK} request is initiated by the guest to
+request the host to mask the specified GPIO line for interrupts. The guest sends
+the \field{struct virtio_gpio_request} to the host, and the host responds with
+\field{struct virtio_gpio_response}. Neither the request, nor the response uses
+the \field{data} field.
+
+\subsubsection{Device Operation: IRQ Unmask}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Unmask }
+
+The \field{VIRTIO_GPIO_REQ_IRQ_UNMASK} request is initiated by the guest to
+request the host to unmask the specified GPIO line for interrupts. The guest
+sends the \field{struct virtio_gpio_request} to the host, and the host responds
+with \field{struct virtio_gpio_response}. Neither the request, nor the response
+uses the \field{data} field.
+
+\subsubsection{Device Operation: IRQ Event}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Event }
+
+The \field{VIRTIO_GPIO_IRQ_EVENT} request is the only request initiated by the
+host to inform the guest that an interrupt is detected on one of the GPIO lines
+configured for input. The host sends the \field{struct virtio_gpio_request} to
+the guest, and the guest responds with \field{struct virtio_gpio_response}. This
+is the only request that uses the \field{interrupt} virtqueue, while all other
+requests use the \field{command} virtqueue. Neither the request, nor the
+response uses the \field{data} field.
+
+
+\drivernormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
+
+A driver MUST set all the fields of the \field{struct virtio_gpio_request}
+before sending the request, except for the requests where the \field{data} field
+isn't used by the request type.
+
+A driver MUST NOT use the \field{data} field in the response message if the
+\field{status} returned from the device is \field{VIRTIO_GPIO_STATUS_ERR}.
+
+A driver MUST NOT try to set value of a GPIO line configured for input.
+
+A driver MUST NOT try to get value of a GPIO line configured for output.
+
+A driver MUST NOT send IRQ related requests for a GPIO line configured for
+output.
+
+A driver MUST queue only one request at a time and wait for its response before
+queuing the next request.
+
+\devicenormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
+
+A device MUST set the \field{status} field to \field{VIRTIO_GPIO_STATUS_OK} for
+all successful requests.
+
+A device MUST set all the fields of the \field{struct virtio_gpio_response}
+before sending the response, except for the response where the \field{data}
+field isn't used by the request type, unless an error has occurred and it has
+set the \field{status} field to \field{VIRTIO_GPIO_STATUS_ERR}.
+
+A device MUST add a valid string in the \field{gpio_names} field of the
+\field{struct virtio_gpio_config} for every supported GPIO line, if the the
+\field{gpio_names_size} is not set to 0.
-- 
2.31.1.272.g89b43f80a514


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Enrico Weigelt, metux IT consult]

On 04.06.21 10:09, Viresh Kumar wrote:

Hi,

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

thanks for your input :)

<snip>

> +\begin{lstlisting}
> +struct virtio_gpio_config {
> +    char name[32];
> +    u16 ngpio;
> +    u16 padding;
> +    u32 gpio_names_size;
> +    char gpio_names[0];
> +} __attribute__((packed));
> +\end{lstlisting}

oh no, please don't change the structs. the original one was:

struct virtio_gpio_config {
         __u8    version;
         __u8    reserved0;
         __u16   num_gpios;
         __u32   names_size;
         __u8    reserved1[24];
         __u8    name[32];
};

The version is vital - we need an upgrade path for future extensions.
(eg. bulk operations)

And the paddings / reserved space should remain for simplicity in
addressing. Think of pure hw implementations (eg. fpga based host)

> +\begin{description}
> +\item[\field{name}] is a null-terminated string that represents the name of the
> +    GPIO controller.

Please also add that unused chars shall be zero. And it's allowed to use
the full 32 bytes (w/o trailing zero). Host needs to do bound checks
anyways.

> +\subsection{Device Operation}\label{sec:Device Types / GPIO Device / Device Operation}
> +
> +The operations of a virtio GPIO device are almost always driven by the guest.
> +The guest initiates one of the requests from \field{VIRTIO_GPIO_REQ_*} on the
> +\field{command} virtqueue and the host responds synchronously on the same
> +virtqueue with a response message. The host initiates an operation
> +(\field{VIRTIO_GPIO_IRQ_EVENT}) only for reporting the detection of an interrupt
> +on a GPIO line and uses the \field{interrupt} virtqueue for the same.
> +
> +\begin{lstlisting}
> +/* GPIO request types */
> +#define VIRTIO_GPIO_REQ_ACTIVATE                0x01
> +#define VIRTIO_GPIO_REQ_DEACTIVATE              0x02
> +#define VIRTIO_GPIO_REQ_GET_DIRECTION           0x03
> +#define VIRTIO_GPIO_REQ_DIRECTION_IN            0x04
> +#define VIRTIO_GPIO_REQ_DIRECTION_OUT           0x05
> +#define VIRTIO_GPIO_REQ_GET_VALUE               0x06
> +#define VIRTIO_GPIO_REQ_SET_VALUE               0x07
> +#define VIRTIO_GPIO_REQ_IRQ_TYPE                0x08
> +#define VIRTIO_GPIO_REQ_IRQ_MASK                0x09
> +#define VIRTIO_GPIO_REQ_IRQ_UNMASK              0x0a
> +#define VIRTIO_GPIO_IRQ_EVENT                   0x0b
> +\end{lstlisting}

Did you miss VIRTIO_GPIO_MSG_REPLY mask ?

This is important to differenciate between request and response. In real
hardware it's a matter of just a few gates. We only have one queue for
request and response.

> +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / GPIO Device / Device Operation: Request Queue}
> +
> +The communication between the host and the guest take place using a pair of
> +request and response messages. The virtio GPIO specification defines two request
> +and two response layouts. The particular request/response pair used for each
> +GPIO request type is specific later in the request specific sections.
> +
> +Supported request and response types:
> +
> +\begin{lstlisting}
> +/* Virtio GPIO Request */
> +struct virtio_gpio_request {
> +        u16 type;
> +        u16 gpio;

why not calling it "pin" ?

> +        u8 data;
> +} __attribute__((packed));
> +
> +/* Virtio GPIO Response */
> +struct virtio_gpio_response {
> +        u8 status;
> +        u8 data;
> +};
> +\end{lstlisting}

Please don't change that. I've got hard reasons for having one frame
layout for both directions: simplicity, especially for pure hardware
implementation. Saves a lot of gates that way. HW only needs one
physical buffer (eg. register memory) for request/response (another
one for interrupt). It only changes some register bits when necessary
and then hands back to the bus IO unit. No additional, request type
specific copying and transformation necessary.

(when doing the same via an serial link, it wouldn't take much more
than some latches and shift registers)

> +/* Possible values of the status field */
> +#define VIRTIO_GPIO_STATUS_OK                   0x0
> +#define VIRTIO_GPIO_STATUS_ERR                  0x1
> +\end{lstlisting}

Do we really need an explicit error reporting ?
I just don't recall any real gpio hardware that does this.

> +\subsubsection{Device Operation: Get Direction}\label{sec:Device Types / GPIO Device / Device Operation / Get Direction }
> +
> +The \field{VIRTIO_GPIO_REQ_GET_DIRECTION} request is initiated by the guest to

Instead of "guest" we should call it "device", since virtio is also
meant for real hardware devices.

> +\subsubsection{Device Operation: IRQ Type}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Type }
> +
> +The \field{VIRTIO_GPIO_REQ_IRQ_TYPE} request is initiated by the guest to
> +request the host to set the IRQ trigger type (one of
> +\field{VIRTIO_GPIO_IRQ_TYPE_*}) for a GPIO line configured for input. The guest
> +sends the \field{struct virtio_gpio_request} to the host, with the IRQ trigger
> +type set in the \field{data} field, and the host responds with \field{struct
> +virtio_gpio_response}. The responds doesn't use the \field{data} field.
> +
> +\subsubsection{Device Operation: IRQ Mask}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Mask }
> +
> +The \field{VIRTIO_GPIO_REQ_IRQ_MASK} request is initiated by the guest to
> +request the host to mask the specified GPIO line for interrupts. The guest sends
> +the \field{struct virtio_gpio_request} to the host, and the host responds with
> +\field{struct virtio_gpio_response}. Neither the request, nor the response uses
> +the \field{data} field.

Note that not all hw does support IRQs. Some only per bank, others per
line. hmm, we should add some flags for that in the device config space.

> +\devicenormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
> +
> +A device MUST set the \field{status} field to \field{VIRTIO_GPIO_STATUS_OK} for
> +all successful requests.

Actually I wonder which HW does report errors and what errors that 
exactly could be.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Viresh Kumar]

On Fri, 4 Jun 2021 at 17:55, Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 04.06.21 10:09, Viresh Kumar wrote:
\\> > +\begin{lstlisting}
> > +struct virtio_gpio_config {
> > +    char name[32];
> > +    u16 ngpio;
> > +    u16 padding;
> > +    u32 gpio_names_size;
> > +    char gpio_names[0];
> > +} __attribute__((packed));
> > +\end{lstlisting}
>
> oh no, please don't change the structs. the original one was:
>
> struct virtio_gpio_config {
>          __u8    version;
>          __u8    reserved0;
>          __u16   num_gpios;
>          __u32   names_size;
>          __u8    reserved1[24];
>          __u8    name[32];
> };
>
> The version is vital - we need an upgrade path for future extensions.
> (eg. bulk operations)

I did wonder about how versioning works in general for virtio and couldn't find
much for any device specification there.

Does anyone know how we handle different versions of the spec? Or is it left
for the individual protocols, GPIO in this case ?

Sure I will add it back, just wanted to get this disucussion going.

> And the paddings / reserved space should remain for simplicity in
> addressing. Think of pure hw implementations (eg. fpga based host)

How does that help actually ?  Maybe I am missing the FPGA kind of
background to see what you are able to se here.

Keeping the config without any unwanted padding (where doesn't
really optimize access) is useful for efficient access. For example I
would like to
read/write the entire config in just one go, lets say in Linux. But
this extra padding of
24 bytes in your structure makes it inefficient. I don't see similar
stuff (or maybe I missed
it) for other protocols and so it may be helpful if you can share some
of the benefits.

> > +\begin{description}
> > +\item[\field{name}] is a null-terminated string that represents the name of the
> > +    GPIO controller.
>
> Please also add that unused chars shall be zero.

Right

> And it's allowed to use
> the full 32 bytes (w/o trailing zero). Host needs to do bound checks
> anyways.

Why is that important, I rather think it should be null-terminated. 31
characters are more
than sufficient for the name?

> > +\subsection{Device Operation}\label{sec:Device Types / GPIO Device / Device Operation}
> > +
> > +The operations of a virtio GPIO device are almost always driven by the guest.
> > +The guest initiates one of the requests from \field{VIRTIO_GPIO_REQ_*} on the
> > +\field{command} virtqueue and the host responds synchronously on the same
> > +virtqueue with a response message. The host initiates an operation
> > +(\field{VIRTIO_GPIO_IRQ_EVENT}) only for reporting the detection of an interrupt
> > +on a GPIO line and uses the \field{interrupt} virtqueue for the same.
> > +
> > +\begin{lstlisting}
> > +/* GPIO request types */
> > +#define VIRTIO_GPIO_REQ_ACTIVATE                0x01
> > +#define VIRTIO_GPIO_REQ_DEACTIVATE              0x02
> > +#define VIRTIO_GPIO_REQ_GET_DIRECTION           0x03
> > +#define VIRTIO_GPIO_REQ_DIRECTION_IN            0x04
> > +#define VIRTIO_GPIO_REQ_DIRECTION_OUT           0x05
> > +#define VIRTIO_GPIO_REQ_GET_VALUE               0x06
> > +#define VIRTIO_GPIO_REQ_SET_VALUE               0x07
> > +#define VIRTIO_GPIO_REQ_IRQ_TYPE                0x08
> > +#define VIRTIO_GPIO_REQ_IRQ_MASK                0x09
> > +#define VIRTIO_GPIO_REQ_IRQ_UNMASK              0x0a
> > +#define VIRTIO_GPIO_IRQ_EVENT                   0x0b
> > +\end{lstlisting}
>
> Did you miss VIRTIO_GPIO_MSG_REPLY mask ?

I felt it isn't required.

> This is important to differenciate between request and response. In real
> hardware it's a matter of just a few gates. We only have one queue for
> request and response.

This specification is a bit different from the one you sent earlier.
In this one, what
I propose is that we use only 1 vq for any synchronous transfers.

i.e. guest will send a request (like set direction), and host will reply on the
same vq with a response. This happens on the "command" vq. There is no
need of a REQ/RESPONSE type here then. The response is there for the last
sent request only.

> > +\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / GPIO Device / Device Operation: Request Queue}
> > +
> > +The communication between the host and the guest take place using a pair of
> > +request and response messages. The virtio GPIO specification defines two request
> > +and two response layouts. The particular request/response pair used for each
> > +GPIO request type is specific later in the request specific sections.
> > +
> > +Supported request and response types:
> > +
> > +\begin{lstlisting}
> > +/* Virtio GPIO Request */
> > +struct virtio_gpio_request {
> > +        u16 type;
> > +        u16 gpio;
>
> why not calling it "pin" ?

It is more like a nomenclature to call a gpio line or gpio pin as "gpio" and so
I used it.

Also I looked at another specification while doing so [1], which few kernel
developers including Greg KH, which was developed for Google's project ARA.

It is already part of Linux as well.

> > +        u8 data;
> > +} __attribute__((packed));
> > +
> > +/* Virtio GPIO Response */
> > +struct virtio_gpio_response {
> > +        u8 status;
> > +        u8 data;
> > +};
> > +\end{lstlisting}
>
> Please don't change that. I've got hard reasons for having one frame
> layout for both directions: simplicity, especially for pure hardware
> implementation. Saves a lot of gates that way. HW only needs one
> physical buffer (eg. register memory) for request/response (another
> one for interrupt). It only changes some register bits when necessary
> and then hands back to the bus IO unit. No additional, request type
> specific copying and transformation necessary.

Hmm, I understand what you are saying here, but I feel that the specification
should just do the right thing and do only what is necessary.

Adding two more fields for (size doesn't matter), like GPIO and Type in
response looks fishy to me, because the response doesn't need that.

I understand it will make it a bit inefficient in your case, but adding those
may make it inefficient for other use cases. The specificaiton should be
OS and hardware independent and should do just the right thing.

Maybe I am wrong here.

Does anyone from virtio side have a say in this kind of situation ?
Is this how we write virtio spec in general ?

> (when doing the same via an serial link, it wouldn't take much more
> than some latches and shift registers)
>
> > +/* Possible values of the status field */
> > +#define VIRTIO_GPIO_STATUS_OK                   0x0
> > +#define VIRTIO_GPIO_STATUS_ERR                  0x1
> > +\end{lstlisting}
>
> Do we really need an explicit error reporting ?
> I just don't recall any real gpio hardware that does this.

Yes. This isn't about hardware really, but software. This protocol is required
to establish connection between two operating systems, guests/hosts and they
need to know if something failed or not. Like in Linux you will be rqeuired to
propagate if changing the diretion passed or not.

> > +\subsubsection{Device Operation: Get Direction}\label{sec:Device Types / GPIO Device / Device Operation / Get Direction }
> > +
> > +The \field{VIRTIO_GPIO_REQ_GET_DIRECTION} request is initiated by the guest to
>
> Instead of "guest" we should call it "device", since virtio is also
> meant for real hardware devices.

Sure.

> > +\subsubsection{Device Operation: IRQ Type}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Type }
> > +
> > +The \field{VIRTIO_GPIO_REQ_IRQ_TYPE} request is initiated by the guest to
> > +request the host to set the IRQ trigger type (one of
> > +\field{VIRTIO_GPIO_IRQ_TYPE_*}) for a GPIO line configured for input. The guest
> > +sends the \field{struct virtio_gpio_request} to the host, with the IRQ trigger
> > +type set in the \field{data} field, and the host responds with \field{struct
> > +virtio_gpio_response}. The responds doesn't use the \field{data} field.
> > +
> > +\subsubsection{Device Operation: IRQ Mask}\label{sec:Device Types / GPIO Device / Device Operation / IRQ Mask }
> > +
> > +The \field{VIRTIO_GPIO_REQ_IRQ_MASK} request is initiated by the guest to
> > +request the host to mask the specified GPIO line for interrupts. The guest sends
> > +the \field{struct virtio_gpio_request} to the host, and the host responds with
> > +\field{struct virtio_gpio_response}. Neither the request, nor the response uses
> > +the \field{data} field.
>
> Note that not all hw does support IRQs. Some only per bank, others per
> line. hmm, we should add some flags for that in the device config space.

Yes, I will add something for that then.

> > +\devicenormative{\subsubsection}{Device Operation}{Device Types / GPIO Device / Device Operation}
> > +
> > +A device MUST set the \field{status} field to \field{VIRTIO_GPIO_STATUS_OK} for
> > +all successful requests.
>
> Actually I wonder which HW does report errors and what errors that
> exactly could be.

Hardware doesn't, but we need here for software.

Thanks for your feedback Enrico.

--
Viresh

[1]  https://github.com/projectara/greybus-spec/blob/master/source/bridged_phy.rst#gpio-protocol

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Jason Wang]

在 2021/6/4 下午8:58, Viresh Kumar 写道:
> On Fri, 4 Jun 2021 at 17:55, Enrico Weigelt, metux IT consult
> <lkml@metux.net>  wrote:
>> On 04.06.21 10:09, Viresh Kumar wrote:
> \\> > +\begin{lstlisting}
>>> +struct virtio_gpio_config {
>>> +    char name[32];
>>> +    u16 ngpio;
>>> +    u16 padding;
>>> +    u32 gpio_names_size;
>>> +    char gpio_names[0];
>>> +} __attribute__((packed));
>>> +\end{lstlisting}
>> oh no, please don't change the structs. the original one was:
>>
>> struct virtio_gpio_config {
>>           __u8    version;
>>           __u8    reserved0;
>>           __u16   num_gpios;
>>           __u32   names_size;
>>           __u8    reserved1[24];
>>           __u8    name[32];
>> };
>>
>> The version is vital - we need an upgrade path for future extensions.
>> (eg. bulk operations)
> I did wonder about how versioning works in general for virtio and couldn't find
> much for any device specification there.
>
> Does anyone know how we handle different versions of the spec? Or is it left
> for the individual protocols, GPIO in this case ?


The spec define some general features and it also allow device to 
implement its own features.

I guess in this case, it might be just a feature like VIRTIO_GPIO_F_BULK?

I'm not familiar with GPIO, but a question is, consider we have 
virtqueue, so actually bulking is already supported?

Thanks


>
> Sure I will add it back, just wanted to get this disucussion going.
>


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Viresh Kumar]

On 07-06-21, 13:25, Jason Wang wrote:
> The spec define some general features and it also allow device to implement
> its own features.
> 
> I guess in this case, it might be just a feature like VIRTIO_GPIO_F_BULK?

Oh yes, it makes sense. So we don't really need a _version_ here, but
can just keep on adding more and more features.

> I'm not familiar with GPIO, but a question is, consider we have virtqueue,
> so actually bulking is already supported?

Virtqueue is there yes, but we don't have operations here yet (in this
version of the spec) which support setting 10 GPIO pins in output mode
with a single request from driver to device.

I think Enrico was looking to have a way to add provision for such
operations later on if required.

And the features will make that work. Thanks Jason.

-- 
viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Viresh Kumar]

On 04-06-21, 14:25, Enrico Weigelt, metux IT consult wrote:
> On 04.06.21 10:09, Viresh Kumar wrote:
> > +The \field{VIRTIO_GPIO_REQ_GET_DIRECTION} request is initiated by the guest to
> 
> Instead of "guest" we should call it "device", since virtio is also
> meant for real hardware devices.

Did you mean to say "driver" instead of "device" here ? The
Virtio-device is the virtual hardware entity which is presented to the
guest OS by the host OS. And the software talking to this device at
the guest is called as the "driver".

I have replaced "host" with "device" and "guest" with "driver" at as
many places as possible. Thanks.

-- 
viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Enrico Weigelt, metux IT consult]

On 07.06.21 07:45, Viresh Kumar wrote:
> On 04-06-21, 14:25, Enrico Weigelt, metux IT consult wrote:
>> On 04.06.21 10:09, Viresh Kumar wrote:
>>> +The \field{VIRTIO_GPIO_REQ_GET_DIRECTION} request is initiated by the guest to
>>
>> Instead of "guest" we should call it "device", since virtio is also
>> meant for real hardware devices.
> 
> Did you mean to say "driver" instead of "device" here ? The
> Virtio-device is the virtual hardware entity which is presented to the
> guest OS by the host OS. And the software talking to this device at
> the guest is called as the "driver".

Sorry, now I've mixed it up, too :o

The "host" in VM-terms is called "device" here (while the VM host
emulates the device), since the idea of virtio is it allows devices
also being implemented as real hardware (*1). Note that virtio is
actually a stack of multiple layers, eg. virtio device protocols is
separate from the underlying transport (eg. pcie, mmapped, ... *2).

> I have replaced "host" with "device" and "guest" with "driver" at as
> many places as possible. Thanks.

Yes, that seems correct. If we'd be looking from a pure hardware / bus
perspective, we might even call it "host" instead of "driver", but that
probably would totally confuse people looking from the VM
perspective.

--mtx


*1) If it was meant exclusively for VMs only, there had been lots of
     other ways to do that, which might even be more efficient in certain
     use cases.
*2) I'm actually planning some Linux-internal socket- or device-node
     based transport in order to allow separate userland servers for
     virtio-based devices, eg. for getting virgl running with containers.

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Enrico Weigelt, metux IT consult]

On 07.06.21 07:42, Viresh Kumar wrote:

>> I guess in this case, it might be just a feature like VIRTIO_GPIO_F_BULK?
> 
> Oh yes, it makes sense. So we don't really need a _version_ here, but
> can just keep on adding more and more features.

If newly added features are orthogonal, that would do the job.

>> I'm not familiar with GPIO, but a question is, consider we have virtqueue,
>> so actually bulking is already supported?
> 
> Virtqueue is there yes, but we don't have operations here yet (in this
> version of the spec) which support setting 10 GPIO pins in output mode
> with a single request from driver to device.
> 
> I think Enrico was looking to have a way to add provision for such
> operations later on if required.

Yes. Newer versions of the protocol could add more operations like
setting a bunch of gpios at once (only some HW really supports that, so
we yet have to sort out whether that should be blindly emulation by host
or refused - depending on how well this fits in to timing requirements).
Other new things could be configuring pullup/down (which has some
overlap with pinmux - some hw does it in the gpio controller, some by
separate pinmux), debounce filters, level thresholds, etc, etc ...

For now we're doing just the very basics - switch on/off, perhaps get an
irq if input changes. The more complex, not so common stuff is left for
future revisions. And to do that, we need to tell the driver what is
supported by the device.

If there's a more granular way instead of just a version number, am
very fine with that. (A little example would be very appreciated).

OTOH, I don't think that an practically not used version field does
hurt so bad.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH V2] virtio-gpio: add the device specification

[Viresh Kumar]

On 08-06-21, 13:02, Enrico Weigelt, metux IT consult wrote:
> If newly added features are orthogonal, that would do the job.

Yes they are.

> Yes. Newer versions of the protocol could add more operations like
> setting a bunch of gpios at once (only some HW really supports that, so
> we yet have to sort out whether that should be blindly emulation by host
> or refused - depending on how well this fits in to timing requirements).
> Other new things could be configuring pullup/down (which has some
> overlap with pinmux - some hw does it in the gpio controller, some by
> separate pinmux), debounce filters, level thresholds, etc, etc ...
> 
> For now we're doing just the very basics - switch on/off, perhaps get an
> irq if input changes. The more complex, not so common stuff is left for
> future revisions. And to do that, we need to tell the driver what is
> supported by the device.
> 
> If there's a more granular way instead of just a version number, am
> very fine with that. (A little example would be very appreciated).

I have added a feature (for IRQ) in the new spec, will send it
shortly.

> OTOH, I don't think that an practically not used version field does
> hurt so bad.

-- 
viresh

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/