Hello,
This adds a virtio based GPIO driver based on the proposed specification [1].
The first two versions [2] were sent by Enrico earlier and here is a newer version. I have retained the authorship of the work done by Enrico (1st patch) to make sure we don't loose his credits.
I have tested all basic operations of the patchset (with Qemu guest) with the libgpiod utility. I have also tested the handling of interrupts on the guest side. All works as expected.
I will now be working towards a Rust based hypervisor agnostic backend to provide a generic solution for that.
This should be merged only after the specifications are merged, I will keep everyone posted for the same.
I am not adding a version history here as a lot of changes have been made, from protocol to code and this really needs a full review from scratch.
-- Viresh
[1] https://lists.oasis-open.org/archives/virtio-comment/202106/msg00022.html [2] https://lore.kernel.org/linux-gpio/20201203191135.21576-2-info@metux.net/
Enrico Weigelt, metux IT consult (1): gpio: Add virtio-gpio driver
Viresh Kumar (2): gpio: virtio: Add IRQ support MAINTAINERS: Add entry for Virtio-gpio
MAINTAINERS | 7 + drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-virtio.c | 566 +++++++++++++++++++++++++++++++ include/uapi/linux/virtio_gpio.h | 56 +++ include/uapi/linux/virtio_ids.h | 1 + 6 files changed, 640 insertions(+) create mode 100644 drivers/gpio/gpio-virtio.c create mode 100644 include/uapi/linux/virtio_gpio.h
From: "Enrico Weigelt, metux IT consult" info@metux.net
This patch adds a new driver for Virtio based GPIO devices.
This allows a guest VM running Linux to access GPIO device provided by the host. It supports all basic operations for now, except interrupts for the GPIO lines.
Signed-off-by: "Enrico Weigelt, metux IT consult" info@metux.net Co-developed-by: Viresh Kumar viresh.kumar@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-virtio.c | 326 +++++++++++++++++++++++++++++++ include/uapi/linux/virtio_gpio.h | 41 ++++ include/uapi/linux/virtio_ids.h | 1 + 5 files changed, 378 insertions(+) create mode 100644 drivers/gpio/gpio-virtio.c create mode 100644 include/uapi/linux/virtio_gpio.h
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index e3607ec4c2e8..7f12fb65d130 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1633,6 +1633,15 @@ config GPIO_MOCKUP tools/testing/selftests/gpio/gpio-mockup.sh. Reference the usage in it.
+config GPIO_VIRTIO + tristate "VirtIO GPIO support" + depends on VIRTIO + help + Say Y here to enable guest support for virtio-based GPIO controllers. + + These virtual GPIOs can be routed to real GPIOs or attached to + simulators on the host (like QEMU). + endmenu
endif diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index c58a90a3c3b1..ace004c80871 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -162,6 +162,7 @@ obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o +obj-$(CONFIG_GPIO_VIRTIO) += gpio-virtio.o obj-$(CONFIG_GPIO_VISCONTI) += gpio-visconti.o obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c new file mode 100644 index 000000000000..1ba8ddf4693a --- /dev/null +++ b/drivers/gpio/gpio-virtio.c @@ -0,0 +1,326 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * GPIO driver for virtio-based virtual GPIO controllers + * + * Copyright (C) 2021 metux IT consult + * Enrico Weigelt, metux IT consult info@metux.net + * + * Copyright (C) 2021 Linaro. + * Viresh Kumar viresh.kumar@linaro.org + */ + +#include <linux/completion.h> +#include <linux/err.h> +#include <linux/gpio/driver.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/virtio_config.h> +#include <uapi/linux/virtio_gpio.h> +#include <uapi/linux/virtio_ids.h> + +struct virtio_gpio { + struct virtio_device *vdev; + struct mutex lock; + struct gpio_chip gc; + + struct completion completion; + struct virtqueue *command_vq; + struct virtio_gpio_request creq; + struct virtio_gpio_response cres; + + /* This must be kept as the last entry here, hint: gpio_names[0] */ + struct virtio_gpio_config config; +}; + +#define gpio_chip_to_vgpio(chip) container_of(chip, struct virtio_gpio, gc) + +static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio, + u8 txdata, u8 *rxdata) +{ + struct virtio_gpio_response *res = &vgpio->cres; + struct virtio_gpio_request *req = &vgpio->creq; + struct scatterlist *sgs[2], req_sg, res_sg; + struct device *dev = &vgpio->vdev->dev; + unsigned long time_left; + unsigned int len; + int ret; + + req->type = cpu_to_le16(type); + req->gpio = cpu_to_le16(gpio); + req->data = txdata; + + sg_init_one(&req_sg, req, sizeof(*req)); + sg_init_one(&res_sg, res, sizeof(*res)); + sgs[0] = &req_sg; + sgs[1] = &res_sg; + + mutex_lock(&vgpio->lock); + ret = virtqueue_add_sgs(vgpio->command_vq, sgs, 1, 1, res, GFP_KERNEL); + if (ret) { + dev_err(dev, "failed to add request to vq\n"); + goto out; + } + + reinit_completion(&vgpio->completion); + virtqueue_kick(vgpio->command_vq); + + time_left = wait_for_completion_timeout(&vgpio->completion, HZ / 10); + if (!time_left) { + dev_err(dev, "virtio GPIO backend timeout\n"); + return -ETIMEDOUT; + } + + WARN_ON(res != virtqueue_get_buf(vgpio->command_vq, &len)); + if (unlikely(res->status != VIRTIO_GPIO_STATUS_OK)) { + dev_err(dev, "virtio GPIO request failed: %d\n", gpio); + return -EINVAL; + } + + if (rxdata) + *rxdata = res->data; + +out: + mutex_unlock(&vgpio->lock); + + return ret; +} + +static int virtio_gpio_request(struct gpio_chip *gc, unsigned int gpio) +{ + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + + return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_ACTIVATE, gpio, 0, NULL); +} + +static void virtio_gpio_free(struct gpio_chip *gc, unsigned int gpio) +{ + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + + virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DEACTIVATE, gpio, 0, NULL); +} + +static int virtio_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) +{ + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + u8 direction; + int ret; + + ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_DIRECTION, gpio, 0, + &direction); + if (ret) + return ret; + + return direction; +} + +static int virtio_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio) +{ + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + + return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_IN, gpio, 0, + NULL); +} + +static int virtio_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio, + int value) +{ + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + + return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_OUT, gpio, (u8) + value, NULL); +} + +static int virtio_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{ + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + u8 value; + int ret; + + ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_VALUE, gpio, 0, &value); + if (ret) + return ret; + + return value; +} + +static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value) +{ + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + + virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_SET_VALUE, gpio, value, NULL); +} + +static void virtio_gpio_command(struct virtqueue *vq) +{ + struct virtio_gpio *vgpio = vq->vdev->priv; + + complete(&vgpio->completion); +} + +static int virtio_gpio_alloc_vqs(struct virtio_device *vdev) +{ + struct virtio_gpio *vgpio = vdev->priv; + const char * const names[] = { "command" }; + vq_callback_t *cbs[] = { + virtio_gpio_command, + }; + struct virtqueue *vqs[1] = {NULL}; + int ret; + + ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL); + if (ret) { + dev_err(&vdev->dev, "failed to allocate vqs: %d\n", ret); + return ret; + } + + vgpio->command_vq = vqs[0]; + + /* Mark the device ready to perform operations from within probe() */ + virtio_device_ready(vgpio->vdev); + return 0; +} + +static void virtio_gpio_free_vqs(struct virtio_device *vdev) +{ + vdev->config->reset(vdev); + vdev->config->del_vqs(vdev); +} + +static const char **parse_gpio_names(struct virtio_device *vdev, + struct virtio_gpio_config *config) +{ + struct device *dev = &vdev->dev; + char *str = config->gpio_names; + const char **names; + int i, len; + + if (!config->gpio_names_size) + return NULL; + + names = devm_kcalloc(dev, config->ngpio, sizeof(names), GFP_KERNEL); + if (!names) + return ERR_PTR(-ENOMEM); + + /* NULL terminate the string instead of checking it */ + config->gpio_names[config->gpio_names_size - 1] = '\0'; + + for (i = 0; i < config->ngpio; i++) { + /* + * We have already marked the last byte with '\0' + * earlier, this shall be enough here. + */ + if (str == config->gpio_names + config->gpio_names_size) { + dev_err(dev, "Invalid gpio_names\n"); + return ERR_PTR(-EINVAL); + } + + len = strlen(str); + if (!len) { + dev_err(dev, "Missing GPIO name: %d\n", i); + return ERR_PTR(-EINVAL); + } + + names[i] = str; + str += len + 1; + } + + return names; +} + +static int virtio_gpio_probe(struct virtio_device *vdev) +{ + struct virtio_gpio_config *config; + struct device *dev = &vdev->dev; + struct virtio_gpio *vgpio; + u32 size; + int ret; + + virtio_cread(vdev, struct virtio_gpio_config, gpio_names_size, &size); + size = cpu_to_le32(size); + + vgpio = devm_kzalloc(dev, sizeof(*vgpio) + size, GFP_KERNEL); + if (!vgpio) + return -ENOMEM; + + config = &vgpio->config; + + /* Read configuration */ + virtio_cread_bytes(vdev, 0, config, sizeof(*config) + size); + + /* NULL terminate the string instead of checking it */ + config->name[sizeof(config->name) - 1] = '\0'; + config->ngpio = cpu_to_le16(config->ngpio); + config->gpio_names_size = cpu_to_le32(config->gpio_names_size); + + if (!config->ngpio) { + dev_err(dev, "Number of GPIOs can't be zero\n"); + return -EINVAL; + } + + vgpio->vdev = vdev; + vgpio->gc.request = virtio_gpio_request; + vgpio->gc.free = virtio_gpio_free; + vgpio->gc.get_direction = virtio_gpio_get_direction; + vgpio->gc.direction_input = virtio_gpio_direction_input; + vgpio->gc.direction_output = virtio_gpio_direction_output; + vgpio->gc.get = virtio_gpio_get; + vgpio->gc.set = virtio_gpio_set; + vgpio->gc.ngpio = config->ngpio; + vgpio->gc.base = -1; /* Allocate base dynamically */ + vgpio->gc.label = config->name[0] ? + config->name : dev_name(dev); + vgpio->gc.parent = dev; + vgpio->gc.owner = THIS_MODULE; + vgpio->gc.can_sleep = true; + vgpio->gc.names = parse_gpio_names(vdev, config); + if (IS_ERR(vgpio->gc.names)) + return PTR_ERR(vgpio->gc.names); + + vdev->priv = vgpio; + mutex_init(&vgpio->lock); + init_completion(&vgpio->completion); + + ret = virtio_gpio_alloc_vqs(vdev); + if (ret) + return ret; + + ret = gpiochip_add(&vgpio->gc); + if (ret) { + virtio_gpio_free_vqs(vdev); + dev_err(dev, "Failed to add virtio-gpio controller\n"); + } + + return ret; +} + +static void virtio_gpio_remove(struct virtio_device *vdev) +{ + struct virtio_gpio *vgpio = vdev->priv; + + gpiochip_remove(&vgpio->gc); + virtio_gpio_free_vqs(vdev); +} + +static const struct virtio_device_id id_table[] = { + { VIRTIO_ID_GPIO, VIRTIO_DEV_ANY_ID }, + {}, +}; +MODULE_DEVICE_TABLE(virtio, id_table); + +static struct virtio_driver virtio_gpio_driver = { + .id_table = id_table, + .probe = virtio_gpio_probe, + .remove = virtio_gpio_remove, + .driver = { + .name = KBUILD_MODNAME, + .owner = THIS_MODULE, + }, +}; +module_virtio_driver(virtio_gpio_driver); + +MODULE_AUTHOR("Enrico Weigelt, metux IT consult info@metux.net"); +MODULE_AUTHOR("Viresh Kumar viresh.kumar@linaro.org"); +MODULE_DESCRIPTION("VirtIO GPIO driver"); +MODULE_LICENSE("GPL"); diff --git a/include/uapi/linux/virtio_gpio.h b/include/uapi/linux/virtio_gpio.h new file mode 100644 index 000000000000..e805e33a79cb --- /dev/null +++ b/include/uapi/linux/virtio_gpio.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ + +#ifndef _LINUX_VIRTIO_GPIO_H +#define _LINUX_VIRTIO_GPIO_H + +#include <linux/types.h> + +/* Virtio 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 + +/* Possible values of the status field */ +#define VIRTIO_GPIO_STATUS_OK 0x0 +#define VIRTIO_GPIO_STATUS_ERR 0x1 + +struct virtio_gpio_config { + char name[32]; + __u16 ngpio; + __u16 padding; + __u32 gpio_names_size; + char gpio_names[0]; +} __packed; + +/* Virtio GPIO Request / Response */ +struct virtio_gpio_request { + __u16 type; + __u16 gpio; + __u8 data; +}; + +struct virtio_gpio_response { + __u8 status; + __u8 data; +}; + +#endif /* _LINUX_VIRTIO_GPIO_H */ diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h index b89391dff6c9..0c24e8ae2499 100644 --- a/include/uapi/linux/virtio_ids.h +++ b/include/uapi/linux/virtio_ids.h @@ -55,5 +55,6 @@ #define VIRTIO_ID_PMEM 27 /* virtio pmem */ #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */ #define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */ +#define VIRTIO_ID_GPIO 41 /* virtio GPIO */
#endif /* _LINUX_VIRTIO_IDS_H */
On Thu, Jun 10, 2021 at 2:18 PM Viresh Kumar viresh.kumar@linaro.org wrote:
From: "Enrico Weigelt, metux IT consult" info@metux.net
This patch adds a new driver for Virtio based GPIO devices.
This allows a guest VM running Linux to access GPIO device provided by the host. It supports all basic operations for now, except interrupts for the GPIO lines.
Signed-off-by: "Enrico Weigelt, metux IT consult" info@metux.net Co-developed-by: Viresh Kumar viresh.kumar@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Can you give an example of how this would be hooked up to other drivers using those gpios. Can you give an example of how using the "gpio-keys" or "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?
Would qemu simply add the required DT properties to the device node that corresponds to the virtio device in this case?
From what I can tell, both the mmio and pci variants of virtio can have their
dev->of_node populated, but I don't see the logic in register_virtio_device() that looks up the of_node of the virtio_device that the of_gpio code then tries to refer to.
Arnd
On 10.06.21 15:22, Arnd Bergmann wrote:
Can you give an example of how this would be hooked up to other drivers using those gpios. Can you give an example of how using the "gpio-keys" or "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?
Connecting between self-probing bus'es and DT is generally tricky. IMHO we don't have any generic mechanism for that.
I've made a few attempts, but nothing practically useful, which would be accepted by the corresponding maintainers, yet. We'd either need some very special logic in DT probing or pseudo-bus'es for the mapping. (DT wants to do those connections via phandle's, which in turn need the referenced nodes to be present in the DT).
From what I can tell, both the mmio and pci variants of virtio can have their dev->of_node populated, but I don't see the logic in register_virtio_device() that looks up the of_node of the virtio_device that the of_gpio code then tries to refer to.
Have you ever successfully bound a virtio device via DT ?
--mtx
On 10-06-21, 15:22, Arnd Bergmann wrote:
Can you give an example of how this would be hooked up to other drivers using those gpios. Can you give an example of how using the "gpio-keys" or "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?
Would qemu simply add the required DT properties to the device node that corresponds to the virtio device in this case?
From what I can tell, both the mmio and pci variants of virtio can have their dev->of_node populated, but I don't see the logic in register_virtio_device() that looks up the of_node of the virtio_device that the of_gpio code then tries to refer to.
To be honest, I haven't tried this yet and I was expecting it to be already taken care of. I was relying on the DTB automatically generated by Qemu to get the driver probed and didn't have a look at it as well.
I now understand that it won't be that straight forward. The same must be true for adding an i2c device to an i2c bus over virtio (The way I tested that earlier was by using the sysfs file to add a device to a bus).
This may be something lacking generally for virtio-pci thing, not sure though.
On Mon, Jun 14, 2021 at 12:23 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 10-06-21, 15:22, Arnd Bergmann wrote:
Can you give an example of how this would be hooked up to other drivers using those gpios. Can you give an example of how using the "gpio-keys" or "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?
Would qemu simply add the required DT properties to the device node that corresponds to the virtio device in this case?
From what I can tell, both the mmio and pci variants of virtio can have their dev->of_node populated, but I don't see the logic in register_virtio_device() that looks up the of_node of the virtio_device that the of_gpio code then tries to refer to.
To be honest, I haven't tried this yet and I was expecting it to be already taken care of. I was relying on the DTB automatically generated by Qemu to get the driver probed and didn't have a look at it as well.
I now understand that it won't be that straight forward. The same must be true for adding an i2c device to an i2c bus over virtio (The way I tested that earlier was by using the sysfs file to add a device to a bus).
Yes, correct, we had the same discussion about i2c. Again, this is relatively straightforward when the controller and the device attached to it (i2c controller/client or gpio controller/function) are both emulated by qemu, but a lot harder when the controller and device are implemented in different programs.
This may be something lacking generally for virtio-pci thing, not sure though.
I think most importantly we need a DT binding to describe what device nodes are supposed to look like underneath a virtio-mmio or virtio-pci device in order for a hypervisor to pass down the information to a guest OS in a generic way. We can probably borrow the USB naming, and replace compatible="usbVID,PID" with compatible="virtioDID", with the device ID in hexadecimal digits, such as "virtio22" for I2C (virtio device ID 34 == 0x22) if we decide to have a sub-node under the device, or we just point dev->of_node of the virtio device to the platform/pci device that is its parent in Linux.
Adding the Linux guest code to the virtio layer should be fairly straightforward, and I suppose it could be mostly copied from the corresponding code that added this for mmc in commit 25185f3f31c9 ("mmc: Add SDIO function devicetree subnode parsing") and for USB in commit 69bec7259853 ("USB: core: let USB device know device node") and 1a7e3948cb9f ("USB: add device-tree support for interfaces").
Arnd
On Mon, 14 Jun 2021 at 14:33, Arnd Bergmann arnd@kernel.org wrote:
On Mon, Jun 14, 2021 at 12:23 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 10-06-21, 15:22, Arnd Bergmann wrote:
Can you give an example of how this would be hooked up to other drivers using those gpios. Can you give an example of how using the "gpio-keys" or "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?
Would qemu simply add the required DT properties to the device node that corresponds to the virtio device in this case?
From what I can tell, both the mmio and pci variants of virtio can have their dev->of_node populated, but I don't see the logic in register_virtio_device() that looks up the of_node of the virtio_device that the of_gpio code then tries to refer to.
To be honest, I haven't tried this yet and I was expecting it to be already taken care of. I was relying on the DTB automatically generated by Qemu to get the driver probed and didn't have a look at it as well.
I now understand that it won't be that straight forward. The same must be true for adding an i2c device to an i2c bus over virtio (The way I tested that earlier was by using the sysfs file to add a device to a bus).
Yes, correct, we had the same discussion about i2c. Again, this is relatively straightforward when the controller and the device attached to it (i2c controller/client or gpio controller/function) are both emulated by qemu, but a lot harder when the controller and device are implemented in different programs.
This may be something lacking generally for virtio-pci thing, not sure though.
I think most importantly we need a DT binding to describe what device nodes are supposed to look like underneath a virtio-mmio or virtio-pci device in order for a hypervisor to pass down the information to a guest OS in a generic way. We can probably borrow the USB naming, and replace compatible="usbVID,PID" with compatible="virtioDID", with the device ID in hexadecimal digits, such as "virtio22" for I2C (virtio device ID 34 == 0x22) if we decide to have a sub-node under the device, or we just point dev->of_node of the virtio device to the platform/pci device that is its parent in Linux.
Adding the Linux guest code to the virtio layer should be fairly straightforward, and I suppose it could be mostly copied from the corresponding code that added this for mmc in commit 25185f3f31c9 ("mmc: Add SDIO function devicetree subnode parsing") and for USB in commit 69bec7259853 ("USB: core: let USB device know device node") and 1a7e3948cb9f ("USB: add device-tree support for interfaces").
And something similar is also done with SCMI protocols which are defined in a SCMI node. A typical example:
cpu@0 { ... clocks = <&scmi_dvfs 0>; ... };
deviceX: deviceX@YYYYYYY { ... clocks = <&scmi_clk 0>; ... };
scmi: scmi { compatible = "arm,scmi-virtio"; #address-cells = <1>; #size-cells = <0>;
scmi_devpd: protocol@11 { reg = <0x11>; #power-domain-cells = <1>; };
scmi_clk: protocol@14 { reg = <0x14>; #clock-cells = <1>; };
scmi_sensors: protocol@15 { reg = <0x15>; #thermal-sensor-cells = <1>; };
scmi_dvfs: protocol@13 { reg = <0x13>; #clock-cells = <1>; }; };
Arnd
On 10.06.21 14:16, Viresh Kumar wrote:
From: "Enrico Weigelt, metux IT consult" info@metux.net
This patch adds a new driver for Virtio based GPIO devices.
This allows a guest VM running Linux to access GPIO device provided by the host. It supports all basic operations for now, except interrupts for the GPIO lines.
What exactly did you change here from my original driver ?
Your already changed the spec havily (w/o consulting me first), so I guess this driver hasn't so much in common w/ my original design.
Note that I made my original design decisions for good reaons (see virtio-dev list). It already support async notifications (IOW: irqs), had an easy upgrade path for future additions, etc.
Note #2: it's already implemented and running in the field (different kernels, different hypervisors, ...) - it just lacked the going through virtio comitte's formal specification process, which blocked mainlining.
Is there anything fundamentally wrong w/ my original design, why you invented a completely new one ?
--mtx
Hi Enrico,
On Thu, 10 Jun 2021 at 21:24, Enrico Weigelt, metux IT consult lkml@metux.net wrote:
On 10.06.21 14:16, Viresh Kumar wrote:
From: "Enrico Weigelt, metux IT consult" info@metux.net
This patch adds a new driver for Virtio based GPIO devices.
This allows a guest VM running Linux to access GPIO device provided by the host. It supports all basic operations for now, except interrupts for the GPIO lines.
What exactly did you change here from my original driver ?
I didn't write it from scratch and used your patch only to start with, and so you are still the Author of this particular patch.
This just followed the specification updates and so changes only the stuff that was different from your original specs. Apart from that as you know, the irqs weren't working in your case as they needed to be implemented differently (patch 2 does that) here.
Your already changed the spec havily (w/o consulting me first),
Isn't that what we are doing on the list? I believe that's why the lists exist, so people don't need to discuss in person, offline. I am happy to make changes in spec if you want to suggest something and if something breaks it for you.
so I guess this driver hasn't so much in common w/ my original design.
It actually has as I said earlier, it is still based on your patch.
Note that I made my original design decisions for good reaons (see virtio-dev list).
I tried to follow your patches from December on the Linux kernel list and the ID allocation one on virtio-comments list. I wasn't able to search for any other attempt at sending the specification, so may have missed something.
I do understand that there were reasons why you (and me) chose a particular way of doing things and if there is a good reason of following that, then we can still modify the spec and fix things for everyone. We just need to discuss our reasoning on the list and get through with a specfication which works for everyone as this will become a standard interface for many in the future and it needs to be robust and efficient for everyone.
It already support async notifications (IOW: irqs), had an easy upgrade path for future additions, etc.
I haven't changed irqs path, we still have a separate virtqueue (named "interrupt" vq) which handles just the interrupts now. And so will be faster than what you initially suggested.
In your original design you also received the responses for the requests on this virtqueue, wihch I have changed to get the response synchronously on the "command" virtqueue only.
This is from one of your earlier replies:
" I've been under the impression that queues only work in only one direction. (at least that's what my web research was telling).
Could you please give an example how bi-directional transmission within the same queue could look like ? "
It is actually possible and the right thing to do here as we aren't starting multiple requests together. The operation needs to be synchronous anyway and both request/response on the same channel work better there. Also that makes the interrupts reach faster, without additional delay due to responses.
Note #2: it's already implemented and running in the field (different kernels, different hypervisors, ...) - it just lacked the going through virtio comitte's formal specification process, which blocked mainlining.
I understand the pain you would be reqiured to go through because of this, but this is how any open source community will work, like Linux.
There will be changes in specification and code once you post it and any solutions already working in the field won't really matter during the discussion. That is why it is always the right thing to _upstream first_, so one can avoid these problems later on. Though I understand that the real world needs to move faster than community. But no one can really help in that.
Is there anything fundamentally wrong w/ my original design, why you invented a completely new one ?
Not much, and I haven't changed a lot as well.
Lemme point out few things which have changed in specification since your earlier version (the code just followed the specification, that's it).
- config structure - version information is replaced with virtio-features. - Irq support is added as a feature, as you suggested. - extra padding space (24 bytes) removed. If you see this patch we can now read the entire config structure in a single go. Why should anyone be required to copy extra 24 bytes? If we need more fields later, we can always do that with help of another feature-flag. So this is still extendable.
- Virtqueues: we still have two virtqueues (command and interrupt), just responses are moved to the command virtqueue only, as that is more efficient.
- Request/response: Request layout is same, added a new layout for response as the same layout is inefficient.
- IRQ support: Initial specification had no interface for configuring irqs from the guest, I added that.
That's all I can gather right now.
I don't think that's a lot and it is mostly improvements only. But if there is a good reason on why we should do things differently, we can still discuss that. I am open to suggestions.
Thanks
-- Viresh
Hi Viresh!
thanks for working on this, it's a really interesting driver.
My first question is conceptual:
We previously have Geerts driver for virtualization: drivers/gpio/gpio-aggregator.c
The idea with the aggregator is that a host script sets up a unique gpiochip for the virtualized instance using some poking in sysfs and pass that to the virtual machine. So this is Linux acting as virtualization host by definition.
I think virtio is more abstract and intended for the usecase where the hypervisor is not Linux, so this should be mentioned in the commit, possibly also in Kconfig so users immediately know what usecases the two different drivers are for.
Possibly both could be used: aggregator to pick out the GPIOs you want into a synthetic GPIO chip, and the actual talk between the hypervisor/host and the guest using virtio, even with linux-on-linux.
Yet another usecase would be to jit this with remoteproc/rpmsg and let a specific signal processor or real-time executive on another CPU with a few GPIOs around present these to Linux using this mechanism. Well that would certainly interest Bjorn and other rpmsg stakeholders, so they should have a look so that this provides what they need they day they need it. (CCed Bjorn and also Google who may want this for their Android emulators.)
On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar viresh.kumar@linaro.org wrote:
+static const char **parse_gpio_names(struct virtio_device *vdev,
struct virtio_gpio_config *config)
I really like this end-to-end plug-and-play that even provides the names over virtio.
I think my patch to the gpiolib to make it mandatory for names to be unique per-chip made it in, but please make that part of the spec so that we don't get the problem with non-unique names here.
I suppose the spec can be augmented later to also accept config settings like open drain pull up/down etc but no need to specify more than the basic for now.
But to be able to add more in the future, the client needs some kind of query mechanism or version number so the driver can adapt and not announce something the underlying virtio device cannot do. Do we have this? A bitmask for features, a version number that increase monotonically for new features to be presented or similar?
Because otherwise we have to bump this: +#define VIRTIO_ID_GPIO 41 /* virtio GPIO */
every time we add something new (and we will).
Yours, Linus Walleij
Hi Linus,
On 10-06-21, 22:46, Linus Walleij wrote:
Hi Viresh!
thanks for working on this, it's a really interesting driver.
My first question is conceptual:
We previously have Geerts driver for virtualization: drivers/gpio/gpio-aggregator.c
The idea with the aggregator is that a host script sets up a unique gpiochip for the virtualized instance using some poking in sysfs and pass that to the virtual machine. So this is Linux acting as virtualization host by definition.
I think virtio is more abstract and intended for the usecase where the hypervisor is not Linux, so this should be mentioned in the commit, possibly also in Kconfig so users immediately know what usecases the two different drivers are for.
Well, not actually.
The host can actually be anything. It can be a Xen based dom0, which runs some proprietary firmware, or Qemu running over Linux.
It is left for the host to decide how it wants to club together the GPIO pins from host and access them, with Linux host userspace it would be playing with /dev/gpiochipN, while for a raw one it may be accessing registers directly.
And so the backend running at host, needs to pass the gpiochip configurations and only the host understand it.
The way I test it for now is by running this with Qemu over my x86 box, so my host side is indeed playing with sysfs Linux.
Possibly both could be used: aggregator to pick out the GPIOs you want into a synthetic GPIO chip, and the actual talk between the hypervisor/host and the guest using virtio, even with linux-on-linux.
Not sure if I understand the aggregator thing for now, but we see the backend running at host (which talks to this Linux driver at guest) as a userspace thing and not a kernel driver. Not sure if aggregator can be used like that, but anyway..
Yet another usecase would be to jit this with remoteproc/rpmsg and let a specific signal processor or real-time executive on another CPU with a few GPIOs around present these to Linux using this mechanism. Well that would certainly interest Bjorn and other rpmsg stakeholders, so they should have a look so that this provides what they need they day they need it. (CCed Bjorn and also Google who may want this for their Android emulators.)
I am not very clear on the rpmsg thing, I know couple of folks at project Stratos were talking about it :)
@Alex, want to chime in here for me ? :)
On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar viresh.kumar@linaro.org wrote:
+static const char **parse_gpio_names(struct virtio_device *vdev,
struct virtio_gpio_config *config)I really like this end-to-end plug-and-play that even provides the names over virtio.
The credit goes to Enrico for this :)
I think my patch to the gpiolib to make it mandatory for names to be unique per-chip made it in, but please make that part of the spec so that we don't get the problem with non-unique names here.
Oh, that's nice. I will surely do that.
I suppose the spec can be augmented later to also accept config settings like open drain pull up/down etc but no need to specify more than the basic for now.
That's the plan.
But to be able to add more in the future, the client needs some kind of query mechanism or version number so the driver can adapt and not announce something the underlying virtio device cannot do. Do we have this? A bitmask for features, a version number that increase monotonically for new features to be presented or similar?
Because otherwise we have to bump this: +#define VIRTIO_ID_GPIO 41 /* virtio GPIO */
every time we add something new (and we will).
Yes, Virtio presents features for this. The patch 2/3 already uses one for IRQs. We won't need to bump up the IDs :)
Hi Viresh, Linus,
On Fri, Jun 11, 2021 at 5:56 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 10-06-21, 22:46, Linus Walleij wrote:
thanks for working on this, it's a really interesting driver.
My first question is conceptual:
We previously have Geerts driver for virtualization: drivers/gpio/gpio-aggregator.c
The idea with the aggregator is that a host script sets up a unique gpiochip for the virtualized instance using some poking in sysfs and pass that to the virtual machine. So this is Linux acting as virtualization host by definition.
The gpio-aggregator is running on the host...
I think virtio is more abstract and intended for the usecase where the hypervisor is not Linux, so this should be mentioned in the commit, possibly also in Kconfig so users immediately know what usecases the two different drivers are for.
... while the virtio-gpio driver is meant for the guest kernel.
I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have a virtio transport, but just hooked into the PL061 GPIO emulation in QEMU. The PL061 QEMU driver talked to the GPIO backend, which talked to /dev/gpiochipN on the host.
Well, not actually.
The host can actually be anything. It can be a Xen based dom0, which runs some proprietary firmware, or Qemu running over Linux.
It is left for the host to decide how it wants to club together the GPIO pins from host and access them, with Linux host userspace it would be playing with /dev/gpiochipN, while for a raw one it may be accessing registers directly.
And so the backend running at host, needs to pass the gpiochip configurations and only the host understand it.
So QEMU has to translate the virtio-gpio communication to e.g. /dev/gpiochipN on the host (or a different backend on non-Linux or bare-metal HV).
The way I test it for now is by running this with Qemu over my x86 box, so my host side is indeed playing with sysfs Linux.
Can you please share a link to the QEMU patches?
Possibly both could be used: aggregator to pick out the GPIOs you want into a synthetic GPIO chip, and the actual talk between the hypervisor/host and the guest using virtio, even with linux-on-linux.
Not sure if I understand the aggregator thing for now, but we see the backend running at host (which talks to this Linux driver at guest) as a userspace thing and not a kernel driver. Not sure if aggregator can be used like that, but anyway..
The GPIO aggregator came into play after talking to Alexander Graf and Peter Maydell. To reduce the attack surface, they didn't want QEMU to be responsible for exporting to the guest a subset of all GPIOs of a gpiochip, only a full gpiochip. However, the full gpiochip may contain critical GPIOs you do not want the guest to tamper with. Hence the GPIO aggregator was born, to take care of aggregating all GPIOs you want to export to a guest into a new virtual gpiochip.
You can find more information about the GPIO Aggregator's use cases in "[PATCH v7 0/6] gpio: Add GPIO Aggregator"[2].
[1] https://lore.kernel.org/linux-gpio/20200423090118.11199-1-geert+renesas@glid... [2] https://lore.kernel.org/linux-doc/20200511145257.22970-1-geert+renesas@glide...
Gr{oetje,eeting}s,
Geert
On 11-06-21, 09:42, Geert Uytterhoeven wrote:
Hi Viresh, Linus,
On Fri, Jun 11, 2021 at 5:56 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 10-06-21, 22:46, Linus Walleij wrote:
thanks for working on this, it's a really interesting driver.
My first question is conceptual:
We previously have Geerts driver for virtualization: drivers/gpio/gpio-aggregator.c
The idea with the aggregator is that a host script sets up a unique gpiochip for the virtualized instance using some poking in sysfs and pass that to the virtual machine. So this is Linux acting as virtualization host by definition.
The gpio-aggregator is running on the host...
I think virtio is more abstract and intended for the usecase where the hypervisor is not Linux, so this should be mentioned in the commit, possibly also in Kconfig so users immediately know what usecases the two different drivers are for.
... while the virtio-gpio driver is meant for the guest kernel.
I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have a virtio transport, but just hooked into the PL061 GPIO emulation in QEMU. The PL061 QEMU driver talked to the GPIO backend, which talked to /dev/gpiochipN on the host.
Hmm, interesting.
Well, not actually.
The host can actually be anything. It can be a Xen based dom0, which runs some proprietary firmware, or Qemu running over Linux.
It is left for the host to decide how it wants to club together the GPIO pins from host and access them, with Linux host userspace it would be playing with /dev/gpiochipN, while for a raw one it may be accessing registers directly.
And so the backend running at host, needs to pass the gpiochip configurations and only the host understand it.
So QEMU has to translate the virtio-gpio communication to e.g. /dev/gpiochipN on the host (or a different backend on non-Linux or bare-metal HV).
No, QEMU passes the raw messages to the backend daemon running in host userspace (which shares a socket with qemu). The backend understands the virtio/vhost protocols and so won't be required to change at all if we move from Qemu to something else. And that's what we (Linaro) are looking to do here with Project Stratos.
Create virtio based hypervisor agnostic backends.
The way I test it for now is by running this with Qemu over my x86 box, so my host side is indeed playing with sysfs Linux.
Can you please share a link to the QEMU patches?
Unfortunately, they aren't in good shape right now and the backend is a bit hacky (Just checking the data paths, but not touching /dev/gpiochipN at all for now).
I didn't implement one as I am going to implement the backend in Rust and not Qemu. So it doesn't depend on Qemu at all.
To give you an idea of the whole thing, here is what we have done for I2c for example, GPIO one will look very similar.
The Qemu patches:
https://yhbt.net/lore/all/cover.1617278395.git.viresh.kumar@linaro.org/T/
The stuff from tools/vhost-user-i2c/ directory (or patch 4/6) isn't used anymore and the following Rust implementation replaces it:
https://github.com/vireshk/vhost-device/tree/master/src/i2c
I can share the GPIO code once I have the Rust implementation ready.
The GPIO aggregator came into play after talking to Alexander Graf and Peter Maydell. To reduce the attack surface, they didn't want QEMU to be responsible for exporting to the guest a subset of all GPIOs of a gpiochip, only a full gpiochip. However, the full gpiochip may contain critical GPIOs you do not want the guest to tamper with. Hence the GPIO aggregator was born, to take care of aggregating all GPIOs you want to export to a guest into a new virtual gpiochip.
You can find more information about the GPIO Aggregator's use cases in "[PATCH v7 0/6] gpio: Add GPIO Aggregator"[2].
So I was actually looking to do some kind of aggregation on the host side's backend daemon to share only a subset of GPIO pins, I will see if that is something I can reuse. Thanks for sharing details.
Hi Viresh,
On Fri, Jun 11, 2021 at 10:01 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 11-06-21, 09:42, Geert Uytterhoeven wrote:
On Fri, Jun 11, 2021 at 5:56 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 10-06-21, 22:46, Linus Walleij wrote:
thanks for working on this, it's a really interesting driver.
My first question is conceptual:
We previously have Geerts driver for virtualization: drivers/gpio/gpio-aggregator.c
The idea with the aggregator is that a host script sets up a unique gpiochip for the virtualized instance using some poking in sysfs and pass that to the virtual machine. So this is Linux acting as virtualization host by definition.
The gpio-aggregator is running on the host...
I think virtio is more abstract and intended for the usecase where the hypervisor is not Linux, so this should be mentioned in the commit, possibly also in Kconfig so users immediately know what usecases the two different drivers are for.
... while the virtio-gpio driver is meant for the guest kernel.
I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have a virtio transport, but just hooked into the PL061 GPIO emulation in QEMU. The PL061 QEMU driver talked to the GPIO backend, which talked to /dev/gpiochipN on the host.
Hmm, interesting.
Well, not actually.
The host can actually be anything. It can be a Xen based dom0, which runs some proprietary firmware, or Qemu running over Linux.
It is left for the host to decide how it wants to club together the GPIO pins from host and access them, with Linux host userspace it would be playing with /dev/gpiochipN, while for a raw one it may be accessing registers directly.
And so the backend running at host, needs to pass the gpiochip configurations and only the host understand it.
So QEMU has to translate the virtio-gpio communication to e.g. /dev/gpiochipN on the host (or a different backend on non-Linux or bare-metal HV).
No, QEMU passes the raw messages to the backend daemon running in host userspace (which shares a socket with qemu). The backend understands the virtio/vhost protocols and so won't be required to change at all if we move from Qemu to something else. And that's what we (Linaro) are looking to do here with Project Stratos.
Create virtio based hypervisor agnostic backends.
OK, IC.
The way I test it for now is by running this with Qemu over my x86 box, so my host side is indeed playing with sysfs Linux.
Can you please share a link to the QEMU patches?
Unfortunately, they aren't in good shape right now and the backend is a bit hacky (Just checking the data paths, but not touching /dev/gpiochipN at all for now).
I didn't implement one as I am going to implement the backend in Rust and not Qemu. So it doesn't depend on Qemu at all.
OK.
To give you an idea of the whole thing, here is what we have done for I2c for example, GPIO one will look very similar.
Oh, you also have i2c. Nice!
The Qemu patches:
https://yhbt.net/lore/all/cover.1617278395.git.viresh.kumar@linaro.org/T/
The stuff from tools/vhost-user-i2c/ directory (or patch 4/6) isn't used anymore and the following Rust implementation replaces it:
Thanks for the links!
The GPIO aggregator came into play after talking to Alexander Graf and Peter Maydell. To reduce the attack surface, they didn't want QEMU to be responsible for exporting to the guest a subset of all GPIOs of a gpiochip, only a full gpiochip. However, the full gpiochip may contain critical GPIOs you do not want the guest to tamper with. Hence the GPIO aggregator was born, to take care of aggregating all GPIOs you want to export to a guest into a new virtual gpiochip.
You can find more information about the GPIO Aggregator's use cases in "[PATCH v7 0/6] gpio: Add GPIO Aggregator"[2].
So I was actually looking to do some kind of aggregation on the host side's backend daemon to share only a subset of GPIO pins, I will see if that is something I can reuse. Thanks for sharing details.
The same reasoning can apply to your backend daemon, so when using the GPIO aggregator, you can just control a full gpiochip, without having to implement access control on individual GPIO lines.
Gr{oetje,eeting}s,
Geert
On 11-06-21, 10:22, Geert Uytterhoeven wrote:
The same reasoning can apply to your backend daemon, so when using the GPIO aggregator, you can just control a full gpiochip, without having to implement access control on individual GPIO lines.
I tried to look at it and it surely looks very temping and may fit well and reduce size of my backend :)
I am now wondering how interrupts can be made to work here. Do you have anything in mind for that ?
GPIO sysfs already supports interrupts, just that you need to register irq for the specific GPIO pins inside the aggregator ?
Hi Viresh,
On Tue, Jun 15, 2021 at 1:15 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 11-06-21, 10:22, Geert Uytterhoeven wrote:
The same reasoning can apply to your backend daemon, so when using the GPIO aggregator, you can just control a full gpiochip, without having to implement access control on individual GPIO lines.
I tried to look at it and it surely looks very temping and may fit well and reduce size of my backend :)
I am now wondering how interrupts can be made to work here. Do you have anything in mind for that ?
GPIO sysfs already supports interrupts, just that you need to register irq for the specific GPIO pins inside the aggregator ?
So far I hadn't considered interrupts. Will think about it...
Gr{oetje,eeting}s,
Geert
On Tue, Jun 15, 2021 at 1:15 PM Viresh Kumar viresh.kumar@linaro.org wrote:
I am now wondering how interrupts can be made to work here. Do you have anything in mind for that ?
The aggregator does not aggregate interrupts only lines for now.
GPIO sysfs already supports interrupts, (...)
I hope that doesn't make you tempted to use sysfs to get interrupts, those are awful, they use sysfs_notify_dirent() which means that if two IRQs happen in fast succession you will miss one of them and think it was only one.
The GPIO character device supports low latency events forwarding interrupts to userspace including QEMU & similar, timestamps the events as close in time as possible to when they actually happen (which is necessary for any kind of industrial control) and will never miss an event if the hardware can register it. See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tool...
Yours, Linus Walleij
On 15-06-21, 22:03, Linus Walleij wrote:
On Tue, Jun 15, 2021 at 1:15 PM Viresh Kumar viresh.kumar@linaro.org wrote:
I am now wondering how interrupts can be made to work here. Do you have anything in mind for that ?
The aggregator does not aggregate interrupts only lines for now.
GPIO sysfs already supports interrupts, (...)
I hope that doesn't make you tempted to use sysfs to get interrupts, those are awful, they use sysfs_notify_dirent() which means that if two IRQs happen in fast succession you will miss one of them and think it was only one.
The GPIO character device supports low latency events forwarding interrupts to userspace including QEMU & similar, timestamps the events as close in time as possible to when they actually happen (which is necessary for any kind of industrial control) and will never miss an event if the hardware can register it. See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tool...
The current version of backend (I am working on) will be Linux userspace based, so I would be required to get an interrupt there.
I was planning to use /dev/gpiochipN only for now and not a sysfs file, so yes it should be fine.
On 11.06.21 10:01, Viresh Kumar wrote:
No, QEMU passes the raw messages to the backend daemon running in host userspace (which shares a socket with qemu). The backend understands the virtio/vhost protocols and so won't be required to change at all if we move from Qemu to something else. And that's what we (Linaro) are looking to do here with Project Stratos.
Note that this is completely different from my approach that I've posted in autumn last year. Viresh's protocol hasn't much in common with mine.
--mtx
On Mon, Jun 14, 2021 at 11:08 AM Enrico Weigelt, metux IT consult lkml@metux.net wrote:
On 11.06.21 10:01, Viresh Kumar wrote:
No, QEMU passes the raw messages to the backend daemon running in host userspace (which shares a socket with qemu). The backend understands the virtio/vhost protocols and so won't be required to change at all if we move from Qemu to something else. And that's what we (Linaro) are looking to do here with Project Stratos.
Note that this is completely different from my approach that I've posted in autumn last year. Viresh's protocol hasn't much in common with mine.
That's why we have a thing called standard. And AFAIU virtio API/ABIs should be officially registered and standardized.
On 14-06-21, 11:12, Andy Shevchenko wrote:
On Mon, Jun 14, 2021 at 11:08 AM Enrico Weigelt, metux IT consult lkml@metux.net wrote:
On 11.06.21 10:01, Viresh Kumar wrote:
No, QEMU passes the raw messages to the backend daemon running in host userspace (which shares a socket with qemu). The backend understands the virtio/vhost protocols and so won't be required to change at all if we move from Qemu to something else. And that's what we (Linaro) are looking to do here with Project Stratos.
Note that this is completely different from my approach that I've posted in autumn last year. Viresh's protocol hasn't much in common with mine.
That's why we have a thing called standard. And AFAIU virtio API/ABIs should be officially registered and standardized.
Yes and here is the latest version (which is based on the work done by Enrico earlier). It isn't accepted yet and is under review.
https://lists.oasis-open.org/archives/virtio-comment/202106/msg00022.html
On 14.06.21 10:12, Andy Shevchenko wrote:
That's why we have a thing called standard. And AFAIU virtio API/ABIs should be officially registered and standardized.
Absolutely.
I've submitted my spec to virtio folks last nov, but this wasn't in form of patch against their tex-based documentation yet (just ascii text), thus laying around in the pipeline.
(meanwhile the actual implementation is running in the field for over 6 month now)
Viresh picked it up, but made something totally different out of it. I wonder why he didn't consult me first. All that needed to be done was translated the ascii documentation into tex and rephrase a bit in order to match the formal terminology and structure used in virtio specs.
This makes me very unhappy.
--mtx
On 14-06-21, 11:17, Enrico Weigelt, metux IT consult wrote:
On 14.06.21 10:12, Andy Shevchenko wrote:
That's why we have a thing called standard. And AFAIU virtio API/ABIs should be officially registered and standardized.
Absolutely.
I've submitted my spec to virtio folks last nov, but this wasn't in form of patch against their tex-based documentation yet (just ascii text), thus laying around in the pipeline.
(meanwhile the actual implementation is running in the field for over 6 month now)
Viresh picked it up, but made something totally different out of it. I wonder why he didn't consult me first.
Here I asked you on 26th of May, if you would be fine if I take it forward as you didn't do anything with it formally in the last 5-6 months (Yes I know you were busy with other stuff).
https://lore.kernel.org/linux-doc/20210526033206.5v362hdywb55msve@vireshk-i7...
You chose not to reply.
I did the same before picking up the kernel code. You chose not to reply.
I am not inclined to do offlist discussions as they aren't fruitful eventually, and it is better to do these discussions over open lists, so others can chime in as well.
All that needed to be done was translated the ascii documentation into tex and rephrase a bit in order to match the formal terminology and structure used in virtio specs.
Not really. There were things lagging, which are normally caught in reviews and fixed/updated. But that never happened in this case. You only sent the specs once to virtio list, that too as an attachment and it never made it to the virtio guys there (as the list doesn't take attachments).
https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg06946.html
This makes me very unhappy.
I know you have solutions running in products which depend on the layout of the config structure or request/responses, based on your original write-up, but unless something is merged in open source code or specification, it is going to get changed, normally because of reviews.
And you will be required to change things based on reviews, you just can't say that I have products running the code and so I won't change it. That is why people say, Upstream first. So you don't get into this mess. Yes, this is tough and that's the way it is.
You keep saying that I have changed the original specification too much, which is incorrect, I tried to point out earlier what all is changed and why.
https://lore.kernel.org/linux-gpio/CAKohpokxSoSVtAJkL-T_OOoS8dW-gYG1Gs5=_Dwe...
You chose not to reply to that.
Lemme say this again. This is a generic protocol we are trying to define here. It needs to take everyone's view into account and their use cases. We are required here to collaborate. A solution thought to be good enough for one person or use-case, isn't going to fly.
The changes I did to it are required to make it useful for the generic solution for a protocol.
I am sure there would be shortcomings, like the one identified by Jean-Philippe Brucker, where he asked to add offset of the gpios-name thing. He made a sensible suggestion, which I am required to incorporate. I just can't reply to him and say I won't change it because I have already designed my product based on this.
Lets try to improve the code and specification here and make it work for both of us by giving very specific feedback on wherever you think the protocol or code is not efficient or correct. Being unhappy isn't going make anything better.
On 14-06-21, 10:07, Enrico Weigelt, metux IT consult wrote:
On 11.06.21 10:01, Viresh Kumar wrote:
No, QEMU passes the raw messages to the backend daemon running in host userspace (which shares a socket with qemu). The backend understands the virtio/vhost protocols and so won't be required to change at all if we move from Qemu to something else. And that's what we (Linaro) are looking to do here with Project Stratos.
This is one of the ways of using it, via a backend running in host userspace, maybe there are other ways of making this work which I may not have explored, like handling this within qemu.
Note that this is completely different from my approach that I've posted in autumn last year. Viresh's protocol hasn't much in common with mine.
The protocol hasn't changed in a sense on how it can be used. Yes few things have moved around, new operations were introduced, but nothing that will make a change at this level. We are trying to use the protocol in one of the possible ways and yours can be different.
On 14.06.21 11:12, Viresh Kumar wrote:
Note that this is completely different from my approach that I've posted in autumn last year. Viresh's protocol hasn't much in common with mine.
The protocol hasn't changed in a sense on how it can be used.
Sorry, but that's not correct. It's an entirely different protocol, entirely different signal flow, different data structures, different queue layout, ... in no way compatible.
The only thing they've got in common is they're both do gpios via virtio.
--mtx
On 11.06.21 09:42, Geert Uytterhoeven wrote:
hi,
I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have a virtio transport, but just hooked into the PL061 GPIO emulation in QEMU. The PL061 QEMU driver talked to the GPIO backend, which talked to /dev/gpiochipN on the host.
for qemu side you might be interested in my patch queue from last year (including the virtio-gpio implementation) - it also introduces an gpio backend subsys that allows attaching simulation gpio's to various backends. so far just implemented a dummy backend (that can be manipulated by qemu console) and using it just in the virtio-gpio device emulation.
https://github.com/oss-qm/qemu/tree/wip/gpio-v2
So QEMU has to translate the virtio-gpio communication to e.g. /dev/gpiochipN on the host (or a different backend on non-Linux or bare-metal HV).
For qemu case, yes, depending on your actual configuration. You can attach the virtual device to any gpio backend you like (once it's actually implemented). Yet only implemented the dummy, which doesn't speak to a real hosts gpio, but can be used simulations like HIL.
--mtx
On 14-06-21, 10:03, Enrico Weigelt, metux IT consult wrote:
for qemu side you might be interested in my patch queue from last year (including the virtio-gpio implementation) - it also introduces an gpio backend subsys that allows attaching simulation gpio's to various backends. so far just implemented a dummy backend (that can be manipulated by qemu console) and using it just in the virtio-gpio device emulation.
Interesting, so this is a qemu-specific backend you have here.
The way we are looking to write the backend (in Project Stratos at Linaro) is to make it hypervisor agnostic. So any hypervisor that understands the vhost protocol can use our backend without changing a single line of code. The backend will not be part of any hypervisor's/VMM's source code. FWIW, this doesn't add anything special to the virtio protocol (like GPIO).
Here is a C based backend we have for I2C:
https://yhbt.net/lore/all/cover.1617278395.git.viresh.kumar@linaro.org/T/#m3...
I started with keeping code in QEMU itself but now replaced it with one in RUST.
https://github.com/vireshk/vhost-device/tree/master/src/i2c
On Thu 10 Jun 15:46 CDT 2021, Linus Walleij wrote: [..]
Yet another usecase would be to jit this with remoteproc/rpmsg and let a specific signal processor or real-time executive on another CPU with a few GPIOs around present these to Linux using this mechanism. Well that would certainly interest Bjorn and other rpmsg stakeholders, so they should have a look so that this provides what they need they day they need it. (CCed Bjorn and also Google who may want this for their Android emulators.)
Right, your typical Qualcomm platform has a dedicated sensor subsystem, with some CPU core with dedicated I2C controllers and GPIOs for processing sensor input while the rest of the SoC is in deep sleep.
Combined with the virtio-i2c effort this could provide an alternative by simply tunneling the busses and GPIOs into Linux and use standard iio drivers, for cases where this suits your product requirements better.
And I've seen similar interest from others in the community as well.
Regards, Bjorn
On 16.06.21 05:30, Bjorn Andersson wrote:
Combined with the virtio-i2c effort this could provide an alternative by simply tunneling the busses and GPIOs into Linux and use standard iio drivers, for cases where this suits your product requirements better.
So, you wanna use virtio as logical interface between the two CPUs ? Interesting idea. Usually folks use rpmsg for those things.
What is running on the secondary CPU ? Some OS like Linux or some bare metal stuff ? What kind of CPU is that anyways ?
--mtx
On Wed, Jun 16, 2021 at 5:52 PM Enrico Weigelt, metux IT consult lkml@metux.net wrote:
On 16.06.21 05:30, Bjorn Andersson wrote:
Combined with the virtio-i2c effort this could provide an alternative by simply tunneling the busses and GPIOs into Linux and use standard iio drivers, for cases where this suits your product requirements better.
So, you wanna use virtio as logical interface between the two CPUs ? Interesting idea. Usually folks use rpmsg for those things.
rpmsg is using shared memory as transport mechanism and virtio is providing this. rpmsg is conceptually a child of virtio: when the subsystem was proposed by TI Arnd noted that virtio has large similarities in shared memory transport and as the potential reuse for buffer handling etc was excellent virtio was used as a base for rpmsg/remoteproc work.
Yours, Linus Walleij
On Wed 16 Jun 10:52 CDT 2021, Enrico Weigelt, metux IT consult wrote:
On 16.06.21 05:30, Bjorn Andersson wrote:
Combined with the virtio-i2c effort this could provide an alternative by simply tunneling the busses and GPIOs into Linux and use standard iio drivers, for cases where this suits your product requirements better.
So, you wanna use virtio as logical interface between the two CPUs ? Interesting idea. Usually folks use rpmsg for those things.
rpmsg is a layer on top of virtio, so this would be an extension of the existing model.
There's been discussions (and I believe some implementations) related to bridging I2C requests over rpmsg, but I think it's preferable to standardize around the virtio based bearer directly.
What is running on the secondary CPU ? Some OS like Linux or some bare metal stuff ? What kind of CPU is that anyways ?
These ideas revolves around platforms that implements something like the "Android Sensor Hub", which provides some resource constraint co-processor that deals with sensor device interaction and processing of the data without waking up the power-hungry ARM cores.
Given the focus on power consumption I would guess that these are not going to run Linux. Core-wise I've seen this implemented using primarily ARM and Hexagon cores.
Regards, Bjorn
This patch adds IRQ support for the virtio GPIO driver. Note that this uses the irq_bus_lock/unlock() callbacks since the operations over virtio can sleep.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/gpio/gpio-virtio.c | 256 ++++++++++++++++++++++++++++++- include/uapi/linux/virtio_gpio.h | 15 ++ 2 files changed, 263 insertions(+), 8 deletions(-)
diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c index 1ba8ddf4693a..8bc4b1876961 100644 --- a/drivers/gpio/gpio-virtio.c +++ b/drivers/gpio/gpio-virtio.c @@ -20,6 +20,13 @@ #include <uapi/linux/virtio_gpio.h> #include <uapi/linux/virtio_ids.h>
+struct vgpio_line { + u8 irq_type; + bool irq_type_pending; + bool masked; + bool masked_pending; +}; + struct virtio_gpio { struct virtio_device *vdev; struct mutex lock; @@ -30,14 +37,20 @@ struct virtio_gpio { struct virtio_gpio_request creq; struct virtio_gpio_response cres;
+ struct irq_chip *ic; + struct vgpio_line *lines; + struct virtqueue *interrupt_vq; + struct virtio_gpio_request ireq; + /* This must be kept as the last entry here, hint: gpio_names[0] */ struct virtio_gpio_config config; };
#define gpio_chip_to_vgpio(chip) container_of(chip, struct virtio_gpio, gc) +#define irq_data_to_gpio_chip(d) (d->domain->host_data)
-static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio, - u8 txdata, u8 *rxdata) +static int virtio_gpio_req_unlocked(struct virtio_gpio *vgpio, u16 type, + u16 gpio, u8 txdata, u8 *rxdata) { struct virtio_gpio_response *res = &vgpio->cres; struct virtio_gpio_request *req = &vgpio->creq; @@ -56,11 +69,10 @@ static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio, sgs[0] = &req_sg; sgs[1] = &res_sg;
- mutex_lock(&vgpio->lock); ret = virtqueue_add_sgs(vgpio->command_vq, sgs, 1, 1, res, GFP_KERNEL); if (ret) { dev_err(dev, "failed to add request to vq\n"); - goto out; + return ret; }
reinit_completion(&vgpio->completion); @@ -81,7 +93,16 @@ static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio, if (rxdata) *rxdata = res->data;
-out: + return ret; +} + +static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio, + u8 txdata, u8 *rxdata) +{ + int ret; + + mutex_lock(&vgpio->lock); + ret = virtio_gpio_req_unlocked(vgpio, type, gpio, txdata, rxdata); mutex_unlock(&vgpio->lock);
return ret; @@ -152,6 +173,183 @@ static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value) virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_SET_VALUE, gpio, value, NULL); }
+/* Interrupt handling */ +static void vgpio_irq_mask(struct virtio_gpio *vgpio, u16 gpio) +{ + int ret; + + ret = virtio_gpio_req_unlocked(vgpio, VIRTIO_GPIO_REQ_IRQ_MASK, gpio, 0, + NULL); + if (ret) + dev_err(&vgpio->vdev->dev, "failed to mask irq: %d\n", ret); +} + +static void vgpio_irq_unmask(struct virtio_gpio *vgpio, u16 gpio) +{ + int ret; + + ret = virtio_gpio_req_unlocked(vgpio, VIRTIO_GPIO_REQ_IRQ_UNMASK, gpio, + 0, NULL); + if (ret) + dev_err(&vgpio->vdev->dev, "failed to unmask irq: %d\n", ret); +} + +static void vgpio_irq_set_type(struct virtio_gpio *vgpio, u16 gpio, u8 type) +{ + int ret; + + ret = virtio_gpio_req_unlocked(vgpio, VIRTIO_GPIO_REQ_IRQ_TYPE, gpio, + type, NULL); + if (ret) + dev_err(&vgpio->vdev->dev, "failed to set irq type: %d\n", ret); +} + +static void virtio_gpio_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_to_gpio_chip(d); + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + struct vgpio_line *line = &vgpio->lines[d->hwirq]; + + line->masked = true; + line->masked_pending = true; +} + +static void virtio_gpio_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_to_gpio_chip(d); + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + struct vgpio_line *line = &vgpio->lines[d->hwirq]; + + line->masked = false; + line->masked_pending = true; +} + +static int virtio_gpio_irq_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *gc = irq_data_to_gpio_chip(d); + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + struct vgpio_line *line = &vgpio->lines[d->hwirq]; + u8 irq_type; + + switch (type) { + case IRQ_TYPE_NONE: + irq_type = VIRTIO_GPIO_IRQ_TYPE_NONE; + break; + case IRQ_TYPE_EDGE_RISING: + irq_type = VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING; + break; + case IRQ_TYPE_EDGE_FALLING: + irq_type = VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING; + break; + case IRQ_TYPE_EDGE_BOTH: + irq_type = VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH; + break; + case IRQ_TYPE_LEVEL_LOW: + irq_type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW; + break; + case IRQ_TYPE_LEVEL_HIGH: + irq_type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH; + break; + default: + dev_err(&vgpio->vdev->dev, "unsupported irq type: %u\n", type); + return -EINVAL; + } + + line->irq_type = irq_type; + line->irq_type_pending = true; + + return 0; +} + +static void virtio_gpio_irq_bus_lock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_to_gpio_chip(d); + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + + mutex_lock(&vgpio->lock); +} + +static void virtio_gpio_irq_bus_sync_unlock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_to_gpio_chip(d); + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); + struct vgpio_line *line = &vgpio->lines[d->hwirq]; + + if (line->irq_type_pending) { + vgpio_irq_set_type(vgpio, d->hwirq, line->irq_type); + line->irq_type_pending = false; + } + + if (line->masked_pending) { + if (line->masked) + vgpio_irq_mask(vgpio, d->hwirq); + else + vgpio_irq_unmask(vgpio, d->hwirq); + line->masked_pending = false; + } + + mutex_unlock(&vgpio->lock); +} + +static void virtio_gpio_interrupt_prepare(struct virtio_gpio *vgpio) +{ + struct scatterlist sg[1]; + + /* Clear request buffer */ + vgpio->ireq.type = 0; + vgpio->ireq.gpio = vgpio->config.ngpio; + + sg_init_one(sg, &vgpio->ireq, sizeof(vgpio->ireq)); + virtqueue_add_inbuf(vgpio->interrupt_vq, sg, 1, vgpio, GFP_KERNEL); + virtqueue_kick(vgpio->interrupt_vq); +} + +static void virtio_gpio_interrupt(struct virtqueue *vq) +{ + struct virtio_gpio *vgpio = vq->vdev->priv; + struct device *dev = &vq->vdev->dev; + unsigned int len; + int irq, gpio, ret; + void *data; + + data = virtqueue_get_buf(vgpio->interrupt_vq, &len); + if (!data || !len) { + dev_warn(dev, "No data received on interrupt\n"); + return; + } + + if (WARN_ON(data != vgpio)) + return; + + if (le16_to_cpu(vgpio->ireq.type) != VIRTIO_GPIO_REQ_IRQ_EVENT) { + dev_warn(dev, "Invalid irq-type: %d\n", vgpio->ireq.type); + goto out; + } + + gpio = le16_to_cpu(vgpio->ireq.gpio); + + if (gpio >= vgpio->config.ngpio) { + dev_warn(dev, "Invalid GPIO number: %d\n", gpio); + goto out; + } + + irq = irq_find_mapping(vgpio->gc.irq.domain, gpio); + if (!irq) { + dev_err(dev, "Failed to find IRQ for gpio: %d\n", gpio); + goto out; + } + + local_irq_disable(); + ret = generic_handle_irq(irq); + local_irq_enable(); + + if (ret) + dev_err(dev, "failed to invoke irq handler\n"); + +out: + virtio_gpio_interrupt_prepare(vgpio); +} + static void virtio_gpio_command(struct virtqueue *vq) { struct virtio_gpio *vgpio = vq->vdev->priv; @@ -162,14 +360,15 @@ static void virtio_gpio_command(struct virtqueue *vq) static int virtio_gpio_alloc_vqs(struct virtio_device *vdev) { struct virtio_gpio *vgpio = vdev->priv; - const char * const names[] = { "command" }; + const char * const names[] = { "command", "interrupt" }; vq_callback_t *cbs[] = { virtio_gpio_command, + virtio_gpio_interrupt, }; - struct virtqueue *vqs[1] = {NULL}; + struct virtqueue *vqs[2] = {NULL}; int ret;
- ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL); + ret = virtio_find_vqs(vdev, vgpio->ic ? 2 : 1, vqs, cbs, names, NULL); if (ret) { dev_err(&vdev->dev, "failed to allocate vqs: %d\n", ret); return ret; @@ -177,6 +376,13 @@ static int virtio_gpio_alloc_vqs(struct virtio_device *vdev)
vgpio->command_vq = vqs[0];
+ if (vgpio->ic) { + vgpio->interrupt_vq = vqs[1]; + + virtio_gpio_interrupt_prepare(vgpio); + virtqueue_enable_cb(vgpio->interrupt_vq); + } + /* Mark the device ready to perform operations from within probe() */ virtio_device_ready(vgpio->vdev); return 0; @@ -278,6 +484,34 @@ static int virtio_gpio_probe(struct virtio_device *vdev) if (IS_ERR(vgpio->gc.names)) return PTR_ERR(vgpio->gc.names);
+ if (virtio_has_feature(vdev, VIRTIO_GPIO_F_IRQ)) { + vgpio->lines = devm_kcalloc(dev, config->ngpio, + sizeof(*vgpio->lines), GFP_KERNEL); + if (!vgpio->lines) + return -ENOMEM; + + vgpio->ic = devm_kzalloc(dev, sizeof(*vgpio->ic), GFP_KERNEL); + if (!vgpio->ic) + return -ENOMEM; + + vgpio->gc.irq.chip = vgpio->ic; + vgpio->ic->name = vgpio->gc.label; + vgpio->ic->irq_mask = virtio_gpio_irq_mask; + vgpio->ic->irq_unmask = virtio_gpio_irq_unmask; + vgpio->ic->irq_set_type = virtio_gpio_irq_set_type; + + /* These are required to implement irqchip for slow busses */ + vgpio->ic->irq_bus_lock = virtio_gpio_irq_bus_lock; + vgpio->ic->irq_bus_sync_unlock = virtio_gpio_irq_bus_sync_unlock; + + /* The event comes from the outside so no parent handler */ + vgpio->gc.irq.parent_handler = NULL; + vgpio->gc.irq.num_parents = 0; + vgpio->gc.irq.parents = NULL; + vgpio->gc.irq.default_type = IRQ_TYPE_NONE; + vgpio->gc.irq.handler = handle_level_irq; + } + vdev->priv = vgpio; mutex_init(&vgpio->lock); init_completion(&vgpio->completion); @@ -309,7 +543,13 @@ static const struct virtio_device_id id_table[] = { }; MODULE_DEVICE_TABLE(virtio, id_table);
+static const unsigned int features[] = { + VIRTIO_GPIO_F_IRQ, +}; + static struct virtio_driver virtio_gpio_driver = { + .feature_table = features, + .feature_table_size = ARRAY_SIZE(features), .id_table = id_table, .probe = virtio_gpio_probe, .remove = virtio_gpio_remove, diff --git a/include/uapi/linux/virtio_gpio.h b/include/uapi/linux/virtio_gpio.h index e805e33a79cb..533d70d77d2c 100644 --- a/include/uapi/linux/virtio_gpio.h +++ b/include/uapi/linux/virtio_gpio.h @@ -5,6 +5,9 @@
#include <linux/types.h>
+/* Virtio GPIO Feature bits */ +#define VIRTIO_GPIO_F_IRQ 0 + /* Virtio GPIO request types */ #define VIRTIO_GPIO_REQ_ACTIVATE 0x01 #define VIRTIO_GPIO_REQ_DEACTIVATE 0x02 @@ -13,6 +16,18 @@ #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_REQ_IRQ_EVENT 0x0b + +/* Virtio GPIO IRQ types */ +#define VIRTIO_GPIO_IRQ_TYPE_NONE 0x00 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING 0x01 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING 0x02 +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH 0x03 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH 0x04 +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW 0x08
/* Possible values of the status field */ #define VIRTIO_GPIO_STATUS_OK 0x0
Hi Viresh!
thanks for this interesting patch!
On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar viresh.kumar@linaro.org wrote:
This patch adds IRQ support for the virtio GPIO driver. Note that this uses the irq_bus_lock/unlock() callbacks since the operations over virtio can sleep.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/gpio/gpio-virtio.c | 256 ++++++++++++++++++++++++++++++- include/uapi/linux/virtio_gpio.h | 15 ++
You also need to select GPIOLIB_IRQCHIP in the Kconfig entry for the gpio-virtio driver, because you make use of it.
+static void virtio_gpio_irq_mask(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_to_gpio_chip(d);struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);struct vgpio_line *line = &vgpio->lines[d->hwirq];line->masked = true;line->masked_pending = true;+}
+static void virtio_gpio_irq_unmask(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_to_gpio_chip(d);struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);struct vgpio_line *line = &vgpio->lines[d->hwirq];line->masked = false;line->masked_pending = true;+}
This looks dangerous in combination with this:
+static void virtio_gpio_interrupt(struct virtqueue *vq) +{
(...)
local_irq_disable();ret = generic_handle_irq(irq);local_irq_enable();
Nominally slow IRQs like those being marshalled over virtio should be nested, handle_nested_irq(irq); but are they? Or are they just quite slow not super slow?
If a threaded IRQF_ONESHOT was requested the IRQ core will kick the thread and *MASK* this IRQ, which means it will call back to your .irq_mask() function and expect it to be masked from this point.
But the IRQ will not actually be masked until you issue your callbacks in the .irq_bus_sync_unlock() callback right?
So from this point until .irq_bus_sync_unlock() get called and actually mask the IRQ, it could be fired again? I suppose the IRQ handler is reentrant? This would violate the API.
I would say that from this point and until you sync you need a spinlock or other locking primitive to stop this IRQ from fireing again, and a spinlock will imply local_irq_disable() so this gets really complex.
I would say only using nesting IRQs or guarantee this some other way, one way would be to specify that whatever is at the other side of virtio cannot send another GPIO IRQ message before the last one is handled, so you would need to send a specific (new) VIRTIO_GPIO_REQ_IRQ_ACK after all other messages have been sent in .irq_bus_sync_unlock() so that the next GPIO IRQ can be dispatched after that.
(Is this how messaged signalled interrupts work? No idea. When in doubt ask the IRQ maintainers.)
Thanks, Linus Walleij
On 10-06-21, 23:30, Linus Walleij wrote:
On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar viresh.kumar@linaro.org wrote:
+static void virtio_gpio_irq_unmask(struct irq_data *d) +{
struct gpio_chip *gc = irq_data_to_gpio_chip(d);struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);struct vgpio_line *line = &vgpio->lines[d->hwirq];line->masked = false;line->masked_pending = true;+}
This looks dangerous in combination with this:
+static void virtio_gpio_interrupt(struct virtqueue *vq) +{
(...)
local_irq_disable();ret = generic_handle_irq(irq);local_irq_enable();Nominally slow IRQs like those being marshalled over virtio should be nested, handle_nested_irq(irq); but are they?
Hmm, this is the call trace:
Call trace: virtio_gpio_interrupt+0x34/0x168 vring_interrupt+0x64/0x98 vp_vring_interrupt+0x5c/0xa8 vp_interrupt+0x40/0x78 __handle_irq_event_percpu+0x5c/0x180 handle_irq_event_percpu+0x38/0x90 handle_irq_event+0x48/0xe0 handle_fasteoi_irq+0xb0/0x138 generic_handle_irq+0x30/0x48 __handle_domain_irq+0x60/0xb8 gic_handle_irq+0x58/0x128 el1_irq+0xb0/0x180 arch_cpu_idle+0x18/0x28 default_idle_call+0x24/0x5c do_idle+0x1ec/0x288 cpu_startup_entry+0x28/0x68 rest_init+0xd8/0xe8 arch_call_rest_init+0x10/0x1c start_kernel+0x508/0x540
I don't see a threaded interrupt in the path and vp_vring_interrupt() already takes spin_lock_irqsave().
This is what handle_nested_irq() says:
* Handle interrupts which are nested into a threaded interrupt * handler. The handler function is called inside the calling * threads context.
So AFAICT, handle_nested_irq() is relevant if the irq-chip's handler is called in threaded context instead of hard one. In this case it is called from hard-irq context and so calling generic_handle_irq() looks to be the right thing.
Right ?
Or are they just quite slow not super slow?
It doesn't use another slow bus like I2C, but this should be slow anyway.
If a threaded IRQF_ONESHOT was requested the IRQ core will kick the thread and *MASK* this IRQ, which means it will call back to your .irq_mask() function and expect it to be masked from this point.
But the IRQ will not actually be masked until you issue your callbacks in the .irq_bus_sync_unlock() callback right?
Yes.
So from this point until .irq_bus_sync_unlock() get called and actually mask the IRQ, it could be fired again?
Since we are defining the spec right now, this is up to us to decide how we want to do it.
I suppose the IRQ handler is reentrant?
It shouldn't happen because of the locking in place in the virtqueue core (vp_vring_interrupt()).
This would violate the API.
I would say that from this point and until you sync you need a spinlock or other locking primitive to stop this IRQ from fireing again, and a spinlock will imply local_irq_disable() so this gets really complex.
I would say only using nesting IRQs or guarantee this some other way, one way would be to specify that whatever is at the other side of virtio cannot send another GPIO IRQ message before the last one is handled, so you would need to send a specific (new) VIRTIO_GPIO_REQ_IRQ_ACK after all other messages have been sent in .irq_bus_sync_unlock() so that the next GPIO IRQ can be dispatched after that.
I was thinking of mentioning this clearly in the spec at first, but now after checking the sequence of things it looks like Linux will do it anyway. Though adding this clearly in the spec can be better. We should just send a response message here instead of another message type VIRTIO_GPIO_REQ_IRQ_ACK.
(Is this how messaged signalled interrupts work? No idea. When in doubt ask the IRQ maintainers.)
This patch adds entry for Virtio-gpio in the MAINTAINERS file.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS index 9450e052f1b1..c55f0f0dda10 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19059,6 +19059,13 @@ F: Documentation/filesystems/virtiofs.rst F: fs/fuse/virtio_fs.c F: include/uapi/linux/virtio_fs.h
+VIRTIO GPIO DRIVER +M: Enrico Weigelt, metux IT consult info@metux.net +M: Viresh Kumar vireshk@kernel.org +S: Maintained +F: drivers/gpio/gpio-virtio.c +F: include/uapi/linux/virtio_gpio.h + VIRTIO GPU DRIVER M: David Airlie airlied@linux.ie M: Gerd Hoffmann kraxel@redhat.com
stratos-dev@op-lists.linaro.org
-
Andy Shevchenko -
Arnd Bergmann -
Bjorn Andersson -
Enrico Weigelt, metux IT consult -
Geert Uytterhoeven -
Linus Walleij -
Vincent Guittot -
Viresh Kumar