[virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Enrico Weigelt, metux IT consult]

This patch adds specification for attaching general purpose IO (gpio) devices
via virtio. The protocol is specifically designed to be easily implemented in
software (e.g. hypervisors) as well as low end hardware (eg. silicon, FPGA,
tiny MCUs) and allows future extensions while retaining full backwards
compatibility.

Implementations for driver (Linux kernel) and device (Qemu) are publically
available and field tested since late 2020. Hardware implementations also
exist (but proprietary, cannot be published yet).

Device type ID 41 has been allocated by TC vote #3632.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>

---

changes v2: * fixed htlatex build error with underscores in labels
            * using code listings for structs and define's
            * fixed mixed-up device/driver wording
---
 conformance.tex |  28 +++++-
 content.tex     |   3 +-
 virtio-gpio.tex | 257 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 283 insertions(+), 5 deletions(-)
 create mode 100644 virtio-gpio.tex

diff --git a/conformance.tex b/conformance.tex
index a164cbb..e5956f4 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 / General Purpose IO 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 / General Purpose IO Device Conformance}.
 
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
@@ -288,6 +290,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}{General Purpose IO Driver Conformance}\label{sec:Conformance / Driver Conformance / General Purpose IO Driver Conformance}
+
+An General Purpose IO driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / General Purpose IO / Virtqueues}
+\item \ref{devicenormative:Device Types / General Purpose IO / Data flow}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -527,6 +538,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}{General Purpose IO Device Conformance}\label{sec:Conformance / Device Conformance / General Purpose IO Device Conformance}
+
+An General Purpose IO device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / General Purpose IO / Virtqueues}
+\item \ref{devicenormative:Device Types / General Purpose IO / Data flow}
+\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 5c70a3c..3e9d1a1 100644
--- a/content.tex
+++ b/content.tex
@@ -2876,7 +2876,7 @@ \chapter{Device Types}\label{sec:Device Types}
 \hline
 40         &   Bluetooth device \\
 \hline
-41         &   GPIO device \\
+41         &   General Purpose IO device \\
 \hline
 \end{tabular}
 
@@ -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 0000000..92dfa19
--- /dev/null
+++ b/virtio-gpio.tex
@@ -0,0 +1,257 @@
+\section{General Purpose IO Device}\label{sec:Device Types / General Purpose IO}
+
+The virtio gpio device is a general purpose IO device that supports a variable
+number of named IO lines that may be switched either as input or output and
+in logical level 0 or 1.
+
+\subsection{Device ID}\label{sec:Device Types / General Purpose IO / Device ID}
+  41
+
+\subsection{Version}\label{sec:Device Types / General Purpose IO / Version}
+  1
+
+\subsection{Device configuration layout}\label{sec:Device Types / General Purpose IO / Device configuration layout}
+
+General purpose IO configuration uses the following layout structure:
+
+\begin{lstlisting}
+struct virtio_gpio_config {
+    __u8    version;
+    __u8    reserved0;
+    __u16   num_gpios;
+    __u32   names_size;
+    __u8    reserved1[24];
+    __u8    name[32];
+    __u8    line_names[];
+};
+\end{lstlisting}
+
+\begin{itemize}
+    \item for \field{version} field currently only value 1 supported.
+    \item the \field{line names block} holds a stream of zero-terminated strings,
+        containing the individual line names in ASCII. line names must unique.
+    \item unspecified fields are reserved for future use and should be zero.
+    \item future versions may extend this configuration space by additional fields.
+\end{itemize}
+
+\subsection{Virtqueues}\label{sec:Device Types / General Purpose IO / Virtqueues}
+\begin{description}
+\item[0] rx (device to CPU)
+\item[1] tx (CPU to device)
+\end{description}
+
+The virtqueues transport messages of the type struct virtio_gpio_msg from device to CPU or CPU to device.
+
+\subsubsection{Virtqueues}\label{sec:Device Types / General Purpose IO / Virtqueues / Message format}
+
+The queues transport messages of the struct virtio_gpio_msg:
+
+\begin{lstlisting}
+struct virtio_gpio_msg {
+    __le16  type;
+    __le16  pin;
+    __le32  value;
+};
+\end{lstlisting}
+
+\subsubsection{Message types}\label{sec:Device Types / General Purpose IO / Virtqueues / Message types}
+
+\begin{lstlisting}
+/* messages types: driver -> device */
+#define VIRTIO_GPIO_MSG_CPU_REQUEST             0x0001
+#define VIRTIO_GPIO_MSG_CPU_DIRECTION_INPUT     0x0002
+#define VIRTIO_GPIO_MSG_CPU_DIRECTION_OUTPUT    0x0003
+#define VIRTIO_GPIO_MSG_CPU_GET_DIRECTION       0x0004
+#define VIRTIO_GPIO_MSG_CPU_GET_LEVEL           0x0005
+#define VIRTIO_GPIO_MSG_CPU_SET_LEVEL           0x0006
+
+/* message types: device -> driver */
+#define VIRTIO_GPIO_MSG_DEVICE_LEVEL            0x0011
+
+/* reply mask: device sets this bit on replies (along with request's message type)
+#define VIRTIO_GPIO_MSG_REPLY                   0x8000
+\end{lstlisting}
+
+\devicenormative{\subsubsection}{Virtqueues}{Device Types / General Purpose IO / Virtqueues}
+
+The device MUST read from the tx queue and write to rx queue.
+
+The device MUST NOT write to the tx queue.
+
+\drivernormative{\subsubsection}{Virtqueues}{Device Types / General Purpose IO / Virtqueues}
+
+The device MUST read from the rx queue and write to tx queue.
+
+The device MUST NOT write to the rx queue.
+
+\subsection{Data flow}\label{sec:Device Types / General Purpose IO / Data flow}
+
+\begin{itemize}
+    \item all operations, except \field{VIRTIO_GPIO_MSG_DEVICE_LEVEL}, are initiated by CPU (tx queue)
+    \item device replies with the orinal \field{type} value OR'ed with \field{VIRTIO_GPIO_MSG_REPLY} (rx queue)
+    \item requests are processed and replied in the they had been sent
+    \item async notifications by the device may be interleaved with request responses
+    \item VIRTIO_GPIO_MSG_DEVICE_LEVEL is only sent asynchronously from device to CPU
+    \item in replies, a negative \field{value} field denotes an Unix-style / POSIX errno code
+    \item the actual error values \textit{(despite being negative or not)} is only of informational
+          nature -- a device or vm host \textit{may} report more detailed error cause but is not required to.
+    \item valid direction values are: 0 = output, 1 = input
+    \item valid line level values are: 0 = inactive, 1 = active
+    \item CPU should not send messages with unspecified \field{type} value
+    \item CPU should ignore ignore messages with unspecified \field{type} value
+\end{itemize}
+
+
+\subsubsection{VIRTIO_GPIO_MSG_CPU_REQUEST}\label{sec:Device Types / General Purpose IO / Data flow / VIRTIO-GPIO-MSG-CPU-REQUEST}
+
+Notify the device that given line number is going to be used.
+
+\begin{tabular}{ll}
+    \hline
+    \textbf{request:} & \\
+    \hline
+    \field{line}  field: & logical line number \\
+    \field{value} field: & unused (should be zero) \\
+    \textbf{reply:} & \\
+    \hline
+    \field{value} field: & POSIX errno code (0 = success, non-zero = error) \\
+    \hline
+\end{tabular}
+
+\subsubsection{VIRTIO_GPIO_MSG_CPU_DIRECTION_INPUT}\label{sec:Device Types / General Purpose IO / Data flow / VIRTIO-GPIO-MSG-CPU-DIRECTION-INPUT}
+
+Set line line direction to input.
+
+\begin{tabular}{ll}
+    \hline
+    \textbf{request:} \\
+    \hline
+    \field{line}  field: & logical line number \\
+    \field{value} field: & unused (should be zero) \\
+    \hline
+    \textbf{reply:} & \\
+    \hline
+    \field{value} field: & POSIX errno code (0 = success, non-zero = error) \\
+    \hline
+\end{tabular}
+
+\subsubsection{VIRTIO_GPIO_MSG_CPU_DIRECTION_OUTPUT}\label{sec:Device Types / General Purpose IO / Data flow / VIRTIO-GPIO-MSG-CPU-DIRECTION-OUTPUT}
+
+Set line direction to output and given line level.
+
+\begin{tabular}{ll}
+    \hline
+    \textbf{request:} \\
+    \hline
+    \field{line}  field: & logical line number \\
+    \field{value} field: & output level (0=inactive, 1=active) \\
+    \hline
+    \textbf{reply:} & \\
+    \hline
+    \field{value} field: & POSIX errno code (0 = success, non-zero = error) \\
+    \hline
+\end{tabular}
+
+\subsubsection{VIRTIO_GPIO_MSG_CPU_GET_DIRECTION}\label{sec:Device Types / General Purpose IO / Data flow / VIRTIO-GPIO-MSG-CPU-GET-DIRECTION}
+
+Retrieve line direction.
+
+\begin{tabular}{ll}
+    \hline
+    \textbf{request:} & \\
+    \hline
+    \field{line}  field: & logical line number \\
+    \field{value} field: & unused (should be zero) \\
+    \hline
+    \textbf{reply:} & \\
+    \hline
+    \field{value} field: & direction (0=output, 1=input) or POSIX errno code \\
+    \hline
+\end{tabular}
+
+\subsubsection{VIRTIO_GPIO_MSG_CPU_GET_LEVEL}\label{sec:Device Types / General Purpose IO / Data flow / VIRTIO-GPIO-MSG-CPU-GET-LEVEL}
+
+Retrieve line level.
+
+\begin{tabular}{ll}
+    \hline
+    \textbf{request:} & \\
+    \hline
+    \field{line}  field: & logical line number \\
+    \field{value} field: & unused (should be zero) \\
+    \hline
+    \textbf{reply:} & \\
+    \hline
+    \field{value} field: & line level (0=inactive, 1=active) or errno code \\
+    \hline
+\end{tabular}
+
+\subsubsection{VIRTIO_GPIO_MSG_CPU_SET_LEVEL}\label{sec:Device Types / General Purpose IO / Data flow / VIRTIO-GPIO-MSG-CPU-SET-LEVEL}
+
+Set line level (output only)
+
+\begin{tabular}{ll}
+    \hline
+    \textbf{request:} & \\
+    \hline
+    \field{line}  field: & logical line number \\
+    \field{value} field: & line level (0=inactive, 1=active) \\
+    \hline
+    \textbf{reply:} & \\
+    \hline
+    \field{value} field: & new line level or (negative) POSIX errno code \\
+    \hline
+\end{tabular}
+
+\subsubsection{VIRTIO_GPIO_MSG_DEVICE_LEVEL}\label{sec:Device Types / General Purpose IO / Data flow / VIRTIO-GPIO-MSG-DEVICE-LEVEL}
+
+Async notification from device to CPU: line level changed
+
+\begin{tabular}{ll}
+    \hline
+    \textbf{request:} & \\
+    \hline
+    \field{line}  field: & logical line number \\
+    \field{value} field: & unused (should be zero) \\
+    \hline
+    \textbf{reply:} & \\
+    \field{value} field: & line level (0=inactive, 1=active) \\
+    \hline
+\end{tabular}
+
+\devicenormative{\subsubsection}{Data flow}{Device Types / General Purpose IO / Data flow}
+
+The device MUST reply to all driver requests in they had been sent.
+
+The device MUST copy the \field{line} field from the request to its reply.
+
+Except for async notification, the device MUST reply the orignal message type, but with the highest bit set
+(or'ed with VIRTIO_GPIO_MSG_REPLY)
+
+In case of error the device MUST fill an negative value into the \field{value} field, it SHOULD use
+an fitting POSIX / Unix errno value when applicable.
+
+On switching to output, the device SHOULD set internal output level before switching the line to output.
+
+The device SHOULD send VIRTIO_GPIO_MSG_DEVICE_LEVEL message for a particular line when it is in input
+direction and line level changes.
+
+\drivernormative{\subsubsection}{Data flow}{Device Types / General Purpose IO / Data flow}
+
+The driver MUST NOT send VIRTIO_GPIO_MSG_DEVICE_LEVEL and MUST NOT set the highest bit in the message type.
+
+The driver MUST NOT send messages with types not defined in this specification.
+
+\subsection{Future versions}\label{sec:Device Types / General Purpose IO / Future versions}
+
+\begin{itemize}
+    \item future versions must increment the ``version`` value
+    \item the basic data structures (config space, message format) should remain
+          backwards compatible, but may increased in size or use reserved fields
+    \item device needs to support commands in older versions
+    \item CPU should not send commands of newer versions that the device doesn't support
+\end{itemize}
+
+\subsection{Feature bits}\label{sec:Device Types / General Purpose IO / Feature bits}
+
+There are currently no feature bits defined for this device, but may be added in future versions.
-- 
2.20.1


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/

[virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Viresh Kumar]

+ All the people from previous version (Please cc them yourself while sending a
new version, these are the people interested in this stuff).

On 30-06-21, 13:58, Enrico Weigelt, metux IT consult wrote:
> This patch adds specification for attaching general purpose IO (gpio) devices
> via virtio. The protocol is specifically designed to be easily implemented in
> software (e.g. hypervisors) as well as low end hardware (eg. silicon, FPGA,
> tiny MCUs) and allows future extensions while retaining full backwards
> compatibility.
> 
> Implementations for driver (Linux kernel) and device (Qemu) are publically
> available and field tested since late 2020. Hardware implementations also
> exist (but proprietary, cannot be published yet).
> 
> Device type ID 41 has been allocated by TC vote #3632.
> 
> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> 
> ---
> 
> changes v2: * fixed htlatex build error with underscores in labels
>             * using code listings for structs and define's
>             * fixed mixed-up device/driver wording

What happened to all the detailed discussions that we went through ? Why did we
spend so much time discussing all that when you didn't had to take suggestions
from the reviews provided on the list?

This version may be good for you, but not for everyone else. We need a generic
enough solution which works seamlessly for everyone involved here. This isn't
some proprietary spec which we are working on here, which we can write based
only on our needs. The spec to be merged MUST be good enough for everyone.

I was expecting the discussions to go in better direction here to be honest and
not have another short lived struggle.

I don't see the improvements suggested for the config structure, nothing about
features, no interrupt support. You just reformatted the stuff and that's all.

-- 
viresh

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

Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Enrico Weigelt, metux IT consult]

On 30.06.21 17:22, Viresh Kumar wrote:
> + All the people from previous version (Please cc them yourself while sending a
> new version, these are the people interested in this stuff).

Phuh, it's hard to track them all. I'm assuming everybody interested in
this is already on the list or can easily subscribe :p

> What happened to all the detailed discussions that we went through ? Why did we
> spend so much time discussing all that when you didn't had to take suggestions
> from the reviews provided on the list?

Which ones are missing, besides those that we talked about doing as
optional feature (like irq controller, etc). Please let's do that in
separate rounds - the minimal base spec first, and then add optional
features.

> I don't see the improvements suggested for the config structure, nothing about
> features, no interrupt support. You just reformatted the stuff and that's all.

Don't worry, I haven't forgotten that. But that's something I'd *really*
like to have as optional features (so not all hardware is mandated to
implement that all) and i'd like to get the mandatory base finished,
before specifying the optional features like interrupt controller.

Let's discuss the optional features separately, feel free to share your
thoughts here.


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

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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Viresh Kumar]

On 30-06-21, 17:55, Enrico Weigelt, metux IT consult wrote:
> On 30.06.21 17:22, Viresh Kumar wrote:
> > + All the people from previous version (Please cc them yourself while sending a
> > new version, these are the people interested in this stuff).

Adding them again, not sure why they got dropped after your email.

> Phuh, it's hard to track them all. I'm assuming everybody interested in
> this is already on the list or can easily subscribe :p

Not necessarily.

I don't expect others, specially the GPIO maintainers or Arnd, to have
subscribed to these lists (and many times people try to subscribe to
lists with no-email option, so their reply can reach to everyone,
while don't want unnecessary emails).

If you are using git to send emails, then an easy way out is to create
an alias for GPIO and have all the interested folks in cc.

It is important to keep few of the people in cc list here.

> > What happened to all the detailed discussions that we went through ? Why did we
> > spend so much time discussing all that when you didn't had to take suggestions
> > from the reviews provided on the list?
> 
> Which ones are missing, besides those that we talked about doing as
> optional feature (like irq controller, etc). Please let's do that in
> separate rounds - the minimal base spec first, and then add optional
> features.

You must have closed the discussion first in the previous version
itself in that case.

> > I don't see the improvements suggested for the config structure, nothing about
> > features, no interrupt support. You just reformatted the stuff and that's all.
> 
> Don't worry, I haven't forgotten that. But that's something I'd *really*
> like to have as optional features (so not all hardware is mandated to
> implement that all) and i'd like to get the mandatory base finished,
> before specifying the optional features like interrupt controller.
> 
> Let's discuss the optional features separately, feel free to share your
> thoughts here.

What about the changes to the config structure to make it efficient,
easily extendable, etc Or the Free Msg, etc? These views are already
shared in details for the earlier version and I shouldn't be expected
to explain them again.

Over that, if you don't want to implement interrupts in your version
(I can surely send a patch for that), then you need to drop the
half-hearted interrupt support, i.e.  VIRTIO_GPIO_MSG_DEVICE_LEVEL, as
that is not the right way of implementing interrupts. This will make
the specification as well as Linux or backend code messy, as we would
be required to support interrupts in two ways in this case based on
irq-feature. If you want to support interrupts, then you need to do
them properly, else don't add them at all.

Please reply to the issues raised in the previous version itself now
and close them there. And please don't proceed with a new version
unless there is a clear consensus in favor or otherwise. It just ends
up wasting a lot of time for everyone.

I asked for a new version today as I expected you to have made all the
changes based on suggestions, where you haven't had a last say.

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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Arnd Bergmann]

On Wed, Jun 30, 2021 at 7:30 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 30-06-21, 17:55, Enrico Weigelt, metux IT consult wrote:
> > On 30.06.21 17:22, Viresh Kumar wrote:
> > > + All the people from previous version (Please cc them yourself while sending a
> > > new version, these are the people interested in this stuff).
>
> Adding them again, not sure why they got dropped after your email.

Thanks.

> > > I don't see the improvements suggested for the config structure, nothing about
> > > features, no interrupt support. You just reformatted the stuff and that's all.
> >
> > Don't worry, I haven't forgotten that. But that's something I'd *really*
> > like to have as optional features (so not all hardware is mandated to
> > implement that all) and i'd like to get the mandatory base finished,
> > before specifying the optional features like interrupt controller.
> >
> > Let's discuss the optional features separately, feel free to share your
> > thoughts here.
>
> What about the changes to the config structure to make it efficient,
> easily extendable, etc Or the Free Msg, etc? These views are already
> shared in details for the earlier version and I shouldn't be expected
> to explain them again.
>
> Over that, if you don't want to implement interrupts in your version
> (I can surely send a patch for that), then you need to drop the
> half-hearted interrupt support, i.e.  VIRTIO_GPIO_MSG_DEVICE_LEVEL, as
> that is not the right way of implementing interrupts. This will make
> the specification as well as Linux or backend code messy, as we would
> be required to support interrupts in two ways in this case based on
> irq-feature. If you want to support interrupts, then you need to do
> them properly, else don't add them at all.

Agreed, interrupt support is obviously something that can not be
retrofitted easily if you don't get it right from the start.

> Please reply to the issues raised in the previous version itself now
> and close them there. And please don't proceed with a new version
> unless there is a clear consensus in favor or otherwise. It just ends
> up wasting a lot of time for everyone.

+1

      Arnd

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/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Enrico Weigelt, metux IT consult]

On 30.06.21 19:30, Viresh Kumar wrote:

Hi,

> I don't expect others, specially the GPIO maintainers or Arnd, to have
> subscribed to these lists (and many times people try to subscribe to
> lists with no-email option, so their reply can reach to everyone,
> while don't want unnecessary emails).

okay, I'm hereby nominate you for the moderator of this virtual meetting
who takes care of those things :p

> What about the changes to the config structure to make it efficient,
> easily extendable, 

What exactly are you missing here, that cannot be easily added in future
versions ?

We currently have:

* plenty free bytes
* version field for clear distinction
* config space may be increased later

> etc Or the Free Msg, etc? 

You mean release a previously requested line ? Yes, that's indeed
slipped through. Sorry about that, and thanks for reminding me.

It really gotten too much and too heated mail traffic, so I lost track
a bit. I believe a good approach for solving those kind of problems is
just keeping a list of open issues and reposting the still remaining
points.

> Over that, if you don't want to implement interrupts in your version
> (I can surely send a patch for that), then you need to drop the
> half-hearted interrupt support, i.e.  VIRTIO_GPIO_MSG_DEVICE_LEVEL, as
> that is not the right way of implementing interrupts. 

This is message is an signal that tells the level has changed and what
the new level is. It is just an event. If you wanna use that event as
an interrupt source, it naturally fits an edge interrupt.

Note that we're still talking about an gpio controller, not an interrupt
controller. The gpio controller signals when something happened on some
input, and it even tells it along with that signal, no explicit readout
required (as on traditional controllers).

Masking out unintersting events is a more sophisticated functionality
(which needs lots of more gates in HW). Certainly good to have, but
that really shouldn't be mandatory for all. That's why I've explicitly
planned that as an *optional* feature (means: controller announces that
in a dedicated feature bit).

> This will make the specification as well as Linux or backend code messy, 
> as we would be required to support interrupts in two ways in this case 
> based on irq-feature.

Why so ? If HW has the IRQ controller feature bit set, we send the
mask / unmask messages, otherwise we don't. Will be a simple 'if'
statement in virtio_gpio_irq_mask() / virtio_gpio_irq_unmask(). Two
extra lines in each function.


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

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Enrico Weigelt, metux IT consult]

On 01.07.21 11:01, Arnd Bergmann wrote:

> Agreed, interrupt support is obviously something that can not be
> retrofitted easily if you don't get it right from the start.

It's basically present, the basic version (as now writting in this spec)
just can't do hardware masking - that part is planned as an optional
feature (which I haven't yet put into that spec version, since I wanted
to get the basic version finished up before doing the optional
features).

Actually, the optional feature will just add another message for
mask/unmask. I'll yet having conversations w/ HW engineers on certain
aspects, e.g. automatic masking (yes/no/configurable ?).

There're also other more sophisticated features like HW debounce,
pinmux, etc, that are to come later - as *optional* features that some
device may or may not support.

Note that this isn't just about moving drivers into some VMM, but also
about actual hardware.


--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-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Viresh Kumar]

On 01-07-21, 16:21, Enrico Weigelt, metux IT consult wrote:
> On 30.06.21 19:30, Viresh Kumar wrote:
> > I don't expect others, specially the GPIO maintainers or Arnd, to have
> > subscribed to these lists (and many times people try to subscribe to
> > lists with no-email option, so their reply can reach to everyone,
> > while don't want unnecessary emails).
> 
> okay, I'm hereby nominate you for the moderator of this virtual meetting
> who takes care of those things :p

What about start cc'ing LKML here then, so everyone gets every message
unconditionally ?

> > What about the changes to the config structure to make it efficient,
> > easily extendable,
> 
> What exactly are you missing here, that cannot be easily added in future
> versions ?
> 
> We currently have:
> 
> * plenty free bytes
> * version field for clear distinction
> * config space may be increased later

Please look at this here, I am not willing to explain them again.

https://lists.oasis-open.org/archives/virtio-comment/202106/msg00037.html

And reply right there on that thread. In short, version field needs to
go, we need to remove all extra padding and add gpio_names_offset.

If you don't agree, you can post your reasoning in reply to that, but
on the main thread please.

> > Over that, if you don't want to implement interrupts in your version
> > (I can surely send a patch for that), then you need to drop the
> > half-hearted interrupt support, i.e.  VIRTIO_GPIO_MSG_DEVICE_LEVEL, as
> > that is not the right way of implementing interrupts.
> 
> This is message is an signal that tells the level has changed and what
> the new level is.

You can call it whatever you want, but this really is an interrupt
from device to driver. And it shouldn't be implemented this way.

Moreover, an interrupt should never come in the first place until the
time the driver (guest) has requested the device (host) for enabling
it.

> It is just an event. If you wanna use that event as
> an interrupt source, it naturally fits an edge interrupt.
> 
> Note that we're still talking about an gpio controller, not an interrupt
> controller. The gpio controller signals when something happened on some
> input, and it even tells it along with that signal, no explicit readout
> required (as on traditional controllers).

No, you do polling if you want to read something. GPIO doesn't need an
interrupt if the users don't want to be notified about changes.

> Masking out unintersting events is a more sophisticated functionality
> (which needs lots of more gates in HW). Certainly good to have, but
> that really shouldn't be mandatory for all. That's why I've explicitly
> planned that as an *optional* feature (means: controller announces that
> in a dedicated feature bit).

Sure, but this kind of half-hearted interrupt implementation is not
going to fly. Sorry about that.

Either don't support interrupts, or if you want to do, then please do
it properly.

> > This will make the specification as well as Linux or backend code messy,
> > as we would be required to support interrupts in two ways in this case
> > based on irq-feature.
> 
> Why so ? If HW has the IRQ controller feature bit set, we send the
> mask / unmask messages, otherwise we don't. Will be a simple 'if'
> statement in virtio_gpio_irq_mask() / virtio_gpio_irq_unmask(). Two
> extra lines in each function.

We are writing a new specification here, and we aren't going to make
the specification or code confusing in this aspect.

From your Linux side driver, this is what you do on this event:

static void virtio_gpio_signal(struct virtio_gpio_priv *priv, int event,
                              int pin, int value)
{
       int mapped_irq = irq_find_mapping(priv->gc.irq.domain, pin);

       if ((pin < priv->num_gpios) && test_bit(pin, priv->irq_mask))
               generic_handle_irq(mapped_irq);
}

How can you even say this isn't interrupt ? You are trying the
interrupt in a hacky way and it isn't going to work. Sorry about that.

I am fine in implementing the interrupt support in driver and spec,
but you need to drop this event completely here for that.

-- 
viresh

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

Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Viresh Kumar]

On 01-07-21, 16:43, Enrico Weigelt, metux IT consult wrote:
> On 01.07.21 11:01, Arnd Bergmann wrote:
> 
> > Agreed, interrupt support is obviously something that can not be
> > retrofitted easily if you don't get it right from the start.
> 
> It's basically present, the basic version (as now writting in this spec)
> just can't do hardware masking - that part is planned as an optional
> feature (which I haven't yet put into that spec version, since I wanted
> to get the basic version finished up before doing the optional
> features).

Finally you called it interrupt :)

> Actually, the optional feature will just add another message for
> mask/unmask. I'll yet having conversations w/ HW engineers on certain
> aspects, e.g. automatic masking (yes/no/configurable ?).

An interrupt should never occur unless asked for. Here is an example
of how this is BROKEN right now.

Lets say the host passes 32 gpio lines to guest, guest moves them in
input mode (some user requested for them) and then frees the gpio
lines.

At this point there are no users of the GPIO block in the guest OS,
but the virtio traffic will continue for the interrupts, as the host
will keep sending the GPIO status changes to the guest.

This is simply BROKEN and NOT acceptable.

If you want to implement interrupts, then add them in a separate
feature or just don't. Partial implementations aren't going to work.

I don't really understand what's the big deal about it, though...

> There're also other more sophisticated features like HW debounce,
> pinmux, etc, that are to come later - as *optional* features that some
> device may or may not support.
> 
> Note that this isn't just about moving drivers into some VMM, but also
> about actual hardware.

... I do understand why you are so adamant on not changing this. And
lemme say this again for clarity.

We are not going to write a new protocol in a BUGGY way, because some
existing hardware implements it in a BROKEN way. You should have
upstreamed the specification before finalizing the hardware and make
sure such chicken and egg situations do not occur.

-- 
viresh

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

Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Viresh Kumar]

On 01-07-21, 22:21, Viresh Kumar wrote:
> On 01-07-21, 16:43, Enrico Weigelt, metux IT consult wrote:
> > On 01.07.21 11:01, Arnd Bergmann wrote:
> > 
> > > Agreed, interrupt support is obviously something that can not be
> > > retrofitted easily if you don't get it right from the start.
> > 
> > It's basically present, the basic version (as now writting in this spec)
> > just can't do hardware masking - that part is planned as an optional
> > feature (which I haven't yet put into that spec version, since I wanted
> > to get the basic version finished up before doing the optional
> > features).
> 
> Finally you called it interrupt :)
> 
> > Actually, the optional feature will just add another message for
> > mask/unmask. I'll yet having conversations w/ HW engineers on certain
> > aspects, e.g. automatic masking (yes/no/configurable ?).
> 
> An interrupt should never occur unless asked for. Here is an example
> of how this is BROKEN right now.
> 
> Lets say the host passes 32 gpio lines to guest, guest moves them in
> input mode (some user requested for them) and then frees the gpio
> lines.
> 
> At this point there are no users of the GPIO block in the guest OS,
> but the virtio traffic will continue for the interrupts, as the host
> will keep sending the GPIO status changes to the guest.
> 
> This is simply BROKEN and NOT acceptable.
> 
> If you want to implement interrupts, then add them in a separate
> feature or just don't. Partial implementations aren't going to work.

Will it be possible to move this stuff a bit faster? We are still where
we were in December last year. Not a lot has changed.

We have future work blocked because of this, Linux driver and generic Backend
implementation (and their users later on), etc.

We really need to move it forward.

Thanks.

-- 
viresh

Re: [virtio-dev] Re: [virtio-comment] [PATCH v2] virtio-gpio: add formal specification

[Michael S. Tsirkin]

On Wed, Jun 30, 2021 at 05:55:07PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 30.06.21 17:22, Viresh Kumar wrote:
> > + All the people from previous version (Please cc them yourself while sending a
> > new version, these are the people interested in this stuff).
> 
> Phuh, it's hard to track them all. I'm assuming everybody interested in
> this is already on the list or can easily subscribe :p

It's a matter of courtesy to help people who contributed by
spending time on review locate your new version.
Tracking what changed to address which comments and listing
that in the cover letter is also very much expected of contributors.
Please do this in the future.

-- 
MST


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