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

["Enrico Weigelt, metux IT consult" <lkml@metux.net>]

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