[PATCH V3 0/3] gpio: Add virtio based driver

[PATCH V3 0/3] gpio: Add virtio based driver

[Viresh Kumar]

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

-- 
2.31.1.272.g89b43f80a514

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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 */
-- 
2.31.1.272.g89b43f80a514

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Linus Walleij]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Geert Uytterhoeven]

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@glider.be
[2] https://lore.kernel.org/linux-doc/20200511145257.22970-1-geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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.

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Geert Uytterhoeven]

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:
>
> https://github.com/vireshk/vhost-device/tree/master/src/i2c

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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 ?

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Linus Walleij]

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/tools/gpio/gpio-event-mon.c

Yours,
Linus Walleij
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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/tools/gpio/gpio-event-mon.c

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.

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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=_DwebvJETE48Xw@mail.gmail.com/

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.

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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.

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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.
> 
> https://github.com/oss-qm/qemu/tree/wip/gpio-v2

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

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

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Bjorn Andersson]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Linus Walleij]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Bjorn Andersson]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Stratos-dev] [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Jean-Philippe Brucker]

On Thu, Jun 10, 2021 at 04:00:39PM +0000, Enrico Weigelt, metux IT consult via Stratos-dev 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?
> 
> Connecting between self-probing bus'es and DT is generally tricky. IMHO
> we don't have any generic mechanism for that.

DT does have a generic description of PCI endpoints, which virtio-iommu
relies on to express the relation between IOMMU and endpoint nodes [1].
I think the problem here is similar: the client node needs a phandle to
the GPIO controller which may use virtio-pci transport?

Note that it mostly works if the device is on the root PCI bus. Behind a
bridge the OS may change the device's bus number as needed, so the BDF
reference in DT is only valid if the software providing the DT description
(VMM or firmware) initializes bus numbers accordingly (and I don't
remember if Linux supports this case well).

Thanks,
Jean

[1] Documentation/devicetree/bindings/virtio/iommu.txt

> 
> 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
> 
> -- 
> ---
> 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
> -- 
> Stratos-dev mailing list
> Stratos-dev@op-lists.linaro.org
> https://op-lists.linaro.org/mailman/listinfo/stratos-dev
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

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.

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH V3 2/3] gpio: virtio: Add IRQ support

[Viresh Kumar]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 2/3] gpio: virtio: Add IRQ support

[Linus Walleij]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 2/3] gpio: virtio: Add IRQ support

[Viresh Kumar]

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

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Jean-Philippe Brucker]

Hi,

Not being very familiar with GPIO, I just have a few general comments and
one on the config space layout

On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote:
> +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;

mutex is still held

> +	}
> +
> +	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;

and here

> +	}
> +
> +	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)

(that dangling cast looks a bit odd to me)

> +			       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);

May fit better in the parent function

> +	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);

le32_to_cpu()? 
> +
> +	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);

leXX_to_cpu()?

> +
> +	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];

A variable-size array here will make it very difficult to append new
fields to virtio_gpio_config, when adding new features to the device. (New
fields cannot be inserted in between, since older drivers will expect
existing fields at a specific offset.)

You could replace it with a reference to the string array, for example
"__u16 gpio_names_offset" declaring the offset between the beginning of
device-specific config and the string array. The 'name' field could also
be indirect to avoid setting a fixed 32-char size, but that's not as
important.

> +} __packed;

No need for __packed, because the fields are naturally aligned (as
required by the virtio spec)

Thanks,
Jean

> +
> +/* 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 */
> -- 
> 2.31.1.272.g89b43f80a514
> 
> -- 
> Stratos-dev mailing list
> Stratos-dev@op-lists.linaro.org
> https://op-lists.linaro.org/mailman/listinfo/stratos-dev
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

On 10-06-21, 19:40, Jean-Philippe Brucker wrote:
> On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote:

Fixed everything else you suggested.

> > +struct virtio_gpio_config {
> > +	char name[32];
> > +	__u16 ngpio;
> > +	__u16 padding;
> > +	__u32 gpio_names_size;
> > +	char gpio_names[0];
> 
> A variable-size array here will make it very difficult to append new
> fields to virtio_gpio_config, when adding new features to the device. (New
> fields cannot be inserted in between, since older drivers will expect
> existing fields at a specific offset.)

Yes, I thought about that earlier and though maybe we will be able to
play with that using the virtio-features, I mean a different layout of
config structure if we really need to add a field in config, based on
the feature flag.

> You could replace it with a reference to the string array, for example
> "__u16 gpio_names_offset" declaring the offset between the beginning of
> device-specific config and the string array.

But, I like this idea more and it does make it very flexible. Will
adapt to it.

> The 'name' field could also be indirect to avoid setting a fixed
> 32-char size, but that's not as important.

Yeah, 32 bytes is really enough. One won't be able to make any sense
out of a bigger name anyway :)

> > +} __packed;
> 
> No need for __packed, because the fields are naturally aligned (as
> required by the virtio spec)

Yeah, I know, but I tend to add that for structures which aren't very
simple (like the request/response ones), just to avoid human errors
and hours of debugging someone need to go through. __packed won't harm
at least :)

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Stratos-dev] [PATCH V3 1/3] gpio: Add virtio-gpio driver

[Viresh Kumar]

On 11-06-21, 10:34, Arnd Bergmann wrote:
> Extraneous __packed annotations do cause real problems:
> 
> - On architectures without hardware unaligned accesses, the compiler is
>   forced to emit byte load/store instructions, which is slower and breaks
>   atomic updates to shared variables
> 
> - If a function takes a pointer of a packed struct member, and passes that
>   pointer to a function that expects a regular aligned pointer, you
> get undefined
>   behavior. Newer compilers produce a warning if you do that (we currently
>   shut up that warning because there are many false positives in the kernel),
>   but you can also run into CPU exceptions or broken code even on CPUs
>   that do support unaligned accesses when the variable ends up being
>   actually unaligned (as you just told the compiler that it is allowed to do).

I understand that these problems will happen if the structure isn't
aligned, but in this case the structure is aligned properly, just that
we are explicitly telling the compiler to not add any padding (it
won't have added any in here). Is it still harmful ?

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization