Re: [PATCH v3] drivers: gpio: add virtio-gpio guest driver

[Randy Dunlap]

On 12/7/20 12:48 PM, Enrico Weigelt, metux IT consult wrote:
> Introducing new GPIO driver for virtual GPIO devices via virtio.

  Introduce

> 
> The driver allows routing GPIO control into VM guests, eg. brigding
> virtual gpios to specific host gpios, or attaching simulators for
> automatic application testing.
> 

These ...

> Changes v3:
>     * spec: fixed type names
>     * spec: replace "host"/"guest" by "device"/"cpu"
>     * spec: change terminology from "events" to "messages"
>     * driver: fixed missing can_sleep flag
>     * driver: select VIRTIO instead of depends on
>     * driver: drop references to qemu in Kconfig
>     * driver: fixed incomplete error handling and possible deadlock
>               in case of sending buf failed
>     * driver: dropped unneeded WARN_ON
>     * driver: fixed retval of virtio_gpio_xmit()
>     * driver: dynamically allocate virtio buffers
>     * driver: added locking on gpio operations
>     * driver: added irq_chip functions
> 
> Changes v2:
>     * uapi: fixed header license
>     * driver: sorted include's
>     * driver: fixed formatting
>     * driver: fixed unneeded devm allocation - plain kzalloc/kfree is enough
>     * driver: fixed missing devm_kzalloc fail check
>     * driver: use devm_kcalloc() for array allocation
>     * spec: added virtio-gpio protocol specification

... Vx change descriptions go after the following "---" line.
I usually put them before the diffstat, with one blank line
separating them.

> 
> Signed-off-by: Enrico Weigelt, metux IT consult <info@xxxxxxxxx>
> ---
>  Documentation/gpio/virtio-gpio.rst | 176 +++++++++++++++++
>  MAINTAINERS                        |   6 +
>  drivers/gpio/Kconfig               |   7 +
>  drivers/gpio/Makefile              |   1 +
>  drivers/gpio/gpio-virtio.c         | 381 +++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/virtio_gpio.h   |  39 ++++
>  include/uapi/linux/virtio_ids.h    |   1 +
>  7 files changed, 611 insertions(+)
>  create mode 100644 Documentation/gpio/virtio-gpio.rst
>  create mode 100644 drivers/gpio/gpio-virtio.c
>  create mode 100644 include/uapi/linux/virtio_gpio.h
> 
> diff --git a/Documentation/gpio/virtio-gpio.rst b/Documentation/gpio/virtio-gpio.rst
> new file mode 100644
> index 000000000000..e7bf01ec1ce7
> --- /dev/null
> +++ b/Documentation/gpio/virtio-gpio.rst
> @@ -0,0 +1,176 @@
> +"""""""""""""""""
> +Virtio-GPIO protocol specification
> +"""""""""""""""""
> +...........
> +Specification for virtio-based virtiual GPIO devices

                                  virtual
although it seems redundant.

> +...........
> +
> ++------------
> ++Version_ 1.0
> ++------------
> +
> +===================
> +General
> +===================
> +
> +The virtio-gpio protocol provides access to general purpose IO devices
> +to virtual machine guests. These virtualized GPIOs could be either provided

   in  (?)

> +by some simulator (eg. virtual HIL), routed to some external device or

                     (e.g.

> +routed to real GPIOs on the host (eg. virtualized embedded applications).

                                    (e.g.

> +
> +Instead of simulating some existing real GPIO chip within an VMM, this

                                                      within a
and why not just "VM"?

> +protocol provides an hardware independent interface between host and guest

                     a hardware

> +that solely relies on an active virtio connection (no matter which transport
> +actually used), no other buses or additional platform driver logic required.
> +
> +===================
> +Protocol layout
> +===================
> +
> +----------------------
> +Configuration space
> +----------------------
> +
> ++--------+----------+-------------------------------+
> +| Offset | Type     | Description                   |
> ++========+==========+===============================+
> +| 0x00   | u8       | version                       |
> ++--------+----------+-------------------------------+
> +| 0x02   | u16      | number of GPIO lines          |
> ++--------+----------+-------------------------------+
> +| 0x04   | u32      | size of gpio name block       |
> ++--------+----------+-------------------------------+
> +| 0x20   | char[32] | device name (0-terminated)    |
> ++--------+----------+-------------------------------+
> +| 0x40   | char[]   | line names block              |
> ++--------+----------+-------------------------------+
> +
> +- for version field currently only value 1 supported.
> +- the line names block holds a stream of zero-terminated strings,
> +  holding the individual line names.
> +- unspecified fields are reserved for future use and should be zero.
> +
> +------------------------
> +Virtqueues and messages:
> +------------------------
> +
> +- Queue #0: transmission from device to cpu

                                           CPU

> +- Queue #1: transmission from cpu to device

                                 CPU

> +
> +The queues transport messages of the struct virtio_gpio_msg:
> +
> +Message format:
> +---------------
> +
> ++--------+----------+---------------+
> +| Offset | Type     | Description   |
> ++========+==========+===============+
> +| 0x00   | uint16   | event type    |
> ++--------+----------+---------------+
> +| 0x02   | uint16   | line id       |
> ++--------+----------+---------------+
> +| 0x04   | uint32   | value         |
> ++--------+----------+---------------+
> +
> +Message types:
> +--------------

Do the Message types go into the "event type" field above?
Please clarify.

> +
> ++-------+----------------------------------------+-----------------------------+
> +| Code  | Symbol                                |                             |
> ++=======+========================================+=============================+
> +| 0x01  | VIRTIO_GPIO_MSG_GUEST_REQUEST          | request gpio line           |
> ++-------+---------------------------------------+-----------------------------+
> +| 0x02  | VIRTIO_GPIO_MSG_GUEST_DIRECTION_INPUT  | set direction to input      |
> ++-------+---------------------------------------+-----------------------------+
> +| 0x03  | VIRTIO_GPIO_MSG_GUEST_DIRECTION_OUTPUT | set direction to output     |
> ++-------+---------------------------------------+-----------------------------+
> +| 0x04  | VIRTIO_GPIO_MSG_GUEST_GET_DIRECTION    | read current direction      |
> ++-------+---------------------------------------+-----------------------------+
> +| 0x05  | VIRTIO_GPIO_MSG_GUEST_GET_VALUE        | read current level          |
> ++-------+---------------------------------------+-----------------------------+
> +| 0x06  | VIRTIO_GPIO_MSG_GUEST_SET_VALUE        | set current (out) level     |
> ++-------+---------------------------------------+-----------------------------+
> +| 0x11  | VIRTIO_GPIO_MSG_HOST_LEVEL             | state changed (host->guest) |
> ++-------+----------------+-----------------------+-----------------------------+
> +
> +----------------------
> +Data flow:
> +----------------------
> +
> +- all operations, except ``VIRTIO_GPIO_MSG_HOST_LEVEL``, are guest-initiated
> +- host replies ``VIRTIO_GPIO_MSG_HOST_LEVEL`` OR'ed to the ``type`` field
> +- ``VIRTIO_GPIO_MSG_HOST_LEVEL`` is only sent asynchronically from host to guest

                                                 asynchronously

> +- in replies, a negative ``value`` field denotes an unix-style errno code

                                                    a Unix-style

> +- valid direction values are:> +  * 0 = output
> +  * 1 = input
> +- valid line state values are:
> +  * 0 = inactive
> +  * 1 = active
> +
> +VIRTIO_GPIO_MSG_GUEST_REQUEST
> +----------------------------
> +
> +- notify the host that given line# is going to be used
> +- request:
> +  * ``line`` field: line number
> +  * ``value`` field: unused
> +- reply:
> +  * ``value`` field: errno code (0 = success)
> +
> +VIRTIO_GPIO_MSG_GUEST_DIRECTION_INPUT
> +------------------------------------
> +
> +- set line line direction to input
> +- request:
> +  * ``line`` field: line number
> +  * ``value`` field: unused
> +- reply: value field holds errno
> +  * ``value`` field: errno code (0 = success)
> +
> +VIRTIO_GPIO_MSG_GUEST_DIRECTION_OUTPUT
> +-------------------------------------
> +
> +- set line direction to output and given line state
> +- request:
> +  * ``line`` field: line number
> +  * ``value`` field: output state (0=inactive, 1=active)
> +- reply:
> +  * ``value`` field: holds errno
> +
> +VIRTIO_GPIO_MSG_GUEST_GET_DIRECTION
> +----------------------------------
> +
> +- retrieve line direction
> +- request:
> +  * ``line`` field: line number
> +  * ``value`` field: unused
> +- reply:
> +  * ``value`` field: direction (0=output, 1=input) or errno code
> +
> +VIRTIO_GPIO_MSG_GUEST_GET_VALUE
> +------------------------------
> +
> +- retrieve line state value
> +- request:
> +  * ``line`` field: line number
> +  * ``value`` field: unused
> +- reply:
> +  * ``value`` field: line state (0=inactive, 1=active) or errno code
> +
> +VIRTIO_GPIO_MSG_GUEST_SET_VALUE
> +------------------------------
> +
> +- set line state value (output only)
> +- request:
> +  * ``line`` field: line number
> +  * ``value`` field: line state (0=inactive, 1=active)
> +- reply:
> +  * ``value`` field: new line state or errno code
> +
> +VIRTIO_GPIO_MSG_HOST_LEVEL
> +-------------------------
> +
> +- async notification from host to gues: line state changed

                                     guest:

> +- ``line`` field: line number
> +- ``value`` field: new line state (0=inactive, 1=active)


-- 
~Randy