[PATCH 1/1] Add virtio-input driver.

[PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

virtio-input is basically evdev-events-over-virtio, so this driver isn't
much more than reading configuration from config space and forwarding
incoming events to the linux input layer.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/virtio/Kconfig            |  10 ++
 drivers/virtio/Makefile           |   1 +
 drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_ids.h   |   1 +
 include/uapi/linux/virtio_input.h |  65 ++++++++
 5 files changed, 390 insertions(+)
 create mode 100644 drivers/virtio/virtio_input.c
 create mode 100644 include/uapi/linux/virtio_input.h

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index b546da5..cab9f3f 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -48,6 +48,16 @@ config VIRTIO_BALLOON
 
 	 If unsure, say M.
 
+config VIRTIO_INPUT
+	tristate "Virtio input driver"
+	depends on VIRTIO
+	depends on INPUT
+	---help---
+	 This driver supports virtio input devices such as
+	 keyboards, mice and tablets.
+
+	 If unsure, say M.
+
  config VIRTIO_MMIO
 	tristate "Platform bus driver for memory mapped virtio devices"
 	depends on HAS_IOMEM
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index d85565b..41e30e3 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
 virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
 virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
 obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
+obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
new file mode 100644
index 0000000..2d425cf
--- /dev/null
+++ b/drivers/virtio/virtio_input.c
@@ -0,0 +1,313 @@
+#include <linux/module.h>
+#include <linux/virtio.h>
+#include <linux/input.h>
+
+#include <uapi/linux/virtio_ids.h>
+#include <uapi/linux/virtio_input.h>
+
+struct virtio_input {
+	struct virtio_device       *vdev;
+	struct input_dev           *idev;
+	char                       name[64];
+	char                       serial[64];
+	char                       phys[64];
+	struct virtqueue           *evt, *sts;
+	struct virtio_input_event  evts[64];
+};
+
+static ssize_t serial_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct input_dev *idev = to_input_dev(dev);
+	struct virtio_input *vi = input_get_drvdata(idev);
+	return sprintf(buf, "%s\n", vi->serial);
+}
+static DEVICE_ATTR_RO(serial);
+
+static struct attribute *dev_attrs[] = {
+	&dev_attr_serial.attr,
+	NULL
+};
+
+static umode_t dev_attrs_are_visible(struct kobject *kobj,
+				     struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct input_dev *idev = to_input_dev(dev);
+	struct virtio_input *vi = input_get_drvdata(idev);
+
+	if (a == &dev_attr_serial.attr && !strlen(vi->serial))
+		return 0;
+
+	return a->mode;
+}
+
+static struct attribute_group dev_attr_grp = {
+	.attrs =	dev_attrs,
+	.is_visible =	dev_attrs_are_visible,
+};
+
+static const struct attribute_group *dev_attr_groups[] = {
+	&dev_attr_grp,
+	NULL
+};
+
+static void virtinput_queue_evtbuf(struct virtio_input *vi,
+				   struct virtio_input_event *evtbuf)
+{
+	struct scatterlist sg[1];
+
+	sg_init_one(sg, evtbuf, sizeof(*evtbuf));
+	virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
+}
+
+static void virtinput_recv_events(struct virtqueue *vq)
+{
+	struct virtio_input *vi = vq->vdev->priv;
+	struct virtio_input_event *event;
+	unsigned int len;
+
+	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
+		input_event(vi->idev,
+			    le16_to_cpu(event->type),
+			    le16_to_cpu(event->code),
+			    le32_to_cpu(event->value));
+		virtinput_queue_evtbuf(vi, event);
+	}
+	virtqueue_kick(vq);
+}
+
+static int virtinput_send_status(struct virtio_input *vi,
+				 u16 type, u16 code, s32 value)
+{
+	struct virtio_input_event *stsbuf;
+	struct scatterlist sg[1];
+
+	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
+	if (!stsbuf)
+		return -ENOMEM;
+
+	stsbuf->type  = cpu_to_le16(type);
+	stsbuf->code  = cpu_to_le16(code);
+	stsbuf->value = cpu_to_le32(value);
+	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
+	virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
+	virtqueue_kick(vi->sts);
+	return 0;
+}
+
+static void virtinput_recv_status(struct virtqueue *vq)
+{
+	struct virtio_input *vi = vq->vdev->priv;
+	struct virtio_input_event *stsbuf;
+	unsigned int len;
+
+	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
+		kfree(stsbuf);
+	virtqueue_kick(vq);
+}
+
+static int virtinput_status(struct input_dev *idev, unsigned int type,
+			    unsigned int code, int value)
+{
+	struct virtio_input *vi = input_get_drvdata(idev);
+
+	return virtinput_send_status(vi, type, code, value);
+}
+
+static size_t virtinput_cfg_select(struct virtio_input *vi,
+				   u8 select, u8 subsel)
+{
+	u8 size;
+
+	virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
+	virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
+	virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
+	return size;
+}
+
+static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
+			       unsigned long *bits, unsigned int bitcount)
+{
+	unsigned int bit;
+	size_t bytes;
+	u8 cfg = 0;
+
+	bytes = virtinput_cfg_select(vi, select, subsel);
+	if (!bytes)
+		return;
+	if (bitcount > bytes*8)
+		bitcount = bytes*8;
+	if (select == VIRTIO_INPUT_CFG_EV_BITS)
+		set_bit(subsel, vi->idev->evbit);
+	for (bit = 0; bit < bitcount; bit++) {
+		if ((bit % 8) == 0)
+			virtio_cread(vi->vdev, struct virtio_input_config,
+				     u.bitmap[bit/8], &cfg);
+		if (cfg & (1 << (bit % 8)))
+			set_bit(bit, bits);
+	}
+}
+
+static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
+{
+	u32 size, mi, ma, fu, fl;
+
+	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
+	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
+	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
+	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
+	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
+	input_set_abs_params(vi->idev, abs,
+			     le32_to_cpu(mi), le32_to_cpu(ma),
+			     le32_to_cpu(fu), le32_to_cpu(fl));
+}
+
+static int virtinput_init_vqs(struct virtio_input *vi)
+{
+	struct virtqueue *vqs[2];
+	vq_callback_t *cbs[] = { virtinput_recv_events,
+				 virtinput_recv_status };
+	const char *names[] = { "events", "status" };
+	int i, err, size;
+
+	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
+	if (err)
+		return err;
+	vi->evt = vqs[0];
+	vi->sts = vqs[1];
+
+	size = virtqueue_get_vring_size(vi->evt);
+	if (size > ARRAY_SIZE(vi->evts))
+		size = ARRAY_SIZE(vi->evts);
+	for (i = 0; i < size; i++)
+		virtinput_queue_evtbuf(vi, &vi->evts[i]);
+
+	return 0;
+}
+
+static int virtinput_probe(struct virtio_device *vdev)
+{
+	struct virtio_input *vi;
+	size_t size;
+	int abs, err;
+
+	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
+	if (!vi) {
+		err = -ENOMEM;
+		goto out1;
+	}
+	vdev->priv = vi;
+	vi->vdev = vdev;
+
+	err = virtinput_init_vqs(vi);
+	if (err)
+		goto out2;
+
+	vi->idev = input_allocate_device();
+	if (!vi->idev) {
+		err = -ENOMEM;
+		goto out3;
+	}
+	input_set_drvdata(vi->idev, vi);
+
+	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
+	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
+			   vi->name, min(size, sizeof(vi->name)));
+	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
+	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
+			   vi->serial, min(size, sizeof(vi->serial)));
+	snprintf(vi->phys, sizeof(vi->phys),
+		 "virtio%d/input0", vdev->index);
+
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
+			   vi->idev->propbit, INPUT_PROP_CNT);
+	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
+	if (size)
+		set_bit(EV_REP, vi->idev->evbit);
+
+	vi->idev->name = vi->name;
+	vi->idev->phys = vi->phys;
+	vi->idev->id.bustype = BUS_VIRTUAL;
+	vi->idev->id.vendor  = 0x0001;
+	vi->idev->id.product = 0x0001;
+	vi->idev->id.version = 0x0100;
+	vi->idev->dev.parent = &vdev->dev;
+	vi->idev->dev.groups = dev_attr_groups;
+	vi->idev->event = virtinput_status;
+
+	/* device -> kernel */
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_KEY,
+			   vi->idev->keybit, KEY_CNT);
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REL,
+			   vi->idev->relbit, REL_CNT);
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_ABS,
+			   vi->idev->absbit, ABS_CNT);
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_MSC,
+			   vi->idev->mscbit, MSC_CNT);
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SW,
+			   vi->idev->swbit,  SW_CNT);
+
+	/* kernel -> device */
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_LED,
+			   vi->idev->ledbit, LED_CNT);
+	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SND,
+			   vi->idev->sndbit, SND_CNT);
+
+	if (test_bit(EV_ABS, vi->idev->evbit)) {
+		for (abs = 0; abs < ABS_CNT; abs++) {
+			if (!test_bit(abs, vi->idev->absbit))
+				continue;
+			virtinput_cfg_abs(vi, abs);
+		}
+	}
+
+	err = input_register_device(vi->idev);
+	if (err)
+		goto out4;
+
+	return 0;
+
+out4:
+	input_free_device(vi->idev);
+out3:
+	vdev->config->del_vqs(vdev);
+out2:
+	kfree(vi);
+out1:
+	return err;
+}
+
+static void virtinput_remove(struct virtio_device *vdev)
+{
+	struct virtio_input *vi = vdev->priv;
+
+	input_unregister_device(vi->idev);
+	vdev->config->reset(vdev);
+	vdev->config->del_vqs(vdev);
+	kfree(vi);
+}
+
+static unsigned int features[] = {
+};
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_INPUT, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static struct virtio_driver virtio_input_driver = {
+	.driver.name         = KBUILD_MODNAME,
+	.driver.owner        = THIS_MODULE,
+	.feature_table       = features,
+	.feature_table_size  = ARRAY_SIZE(features),
+	.id_table            = id_table,
+	.probe               = virtinput_probe,
+	.remove              = virtinput_remove,
+};
+
+module_virtio_driver(virtio_input_driver);
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Virtio input device driver");
+MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 284fc3a..5f60aa4 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -39,5 +39,6 @@
 #define VIRTIO_ID_9P		9 /* 9p virtio console */
 #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
 #define VIRTIO_ID_CAIF	       12 /* Virtio caif */
+#define VIRTIO_ID_INPUT        18 /* virtio input */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/include/uapi/linux/virtio_input.h b/include/uapi/linux/virtio_input.h
new file mode 100644
index 0000000..190d04a
--- /dev/null
+++ b/include/uapi/linux/virtio_input.h
@@ -0,0 +1,65 @@
+#ifndef _LINUX_VIRTIO_INPUT_H
+#define _LINUX_VIRTIO_INPUT_H
+/* This header is BSD licensed so anyone can use the definitions to implement
+ * compatible drivers/servers.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of IBM nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE. */
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+
+enum virtio_input_config_select {
+	VIRTIO_INPUT_CFG_UNSET      = 0x00,
+	VIRTIO_INPUT_CFG_ID_NAME    = 0x01,
+	VIRTIO_INPUT_CFG_ID_SERIAL  = 0x02,
+	VIRTIO_INPUT_CFG_PROP_BITS  = 0x10,
+	VIRTIO_INPUT_CFG_EV_BITS    = 0x11,
+	VIRTIO_INPUT_CFG_ABS_INFO   = 0x12,
+};
+
+struct virtio_input_absinfo {
+	__le32  min;
+	__le32  max;
+	__le32  fuzz;
+	__le32  flat;
+};
+
+struct virtio_input_config {
+	__u8    select;
+	__u8    subsel;
+	__u8    size;
+	__u8    reserved;
+	union {
+		char string[128];
+		__u8 bitmap[128];
+		struct virtio_input_absinfo abs;
+	} u;
+};
+
+struct virtio_input_event {
+	__le16 type;
+	__le16 code;
+	__le32 value;
+};
+
+#endif /* _LINUX_VIRTIO_INPUT_H */
-- 
1.8.3.1

Re: [PATCH 1/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 10:13:11AM +0100, Gerd Hoffmann wrote:
> virtio-input is basically evdev-events-over-virtio, so this driver isn't
> much more than reading configuration from config space and forwarding
> incoming events to the linux input layer.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

What worries me is how well are these events specified.
Will we be able to write drivers for non-linux guests?
Not sure where to discuss this - this seems like an inappropriate
thread.

Comments on linux driver below.

> ---
>  drivers/virtio/Kconfig            |  10 ++
>  drivers/virtio/Makefile           |   1 +
>  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/virtio_ids.h   |   1 +
>  include/uapi/linux/virtio_input.h |  65 ++++++++
>  5 files changed, 390 insertions(+)
>  create mode 100644 drivers/virtio/virtio_input.c
>  create mode 100644 include/uapi/linux/virtio_input.h
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index b546da5..cab9f3f 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
>  
>  	 If unsure, say M.
>  
> +config VIRTIO_INPUT
> +	tristate "Virtio input driver"
> +	depends on VIRTIO
> +	depends on INPUT
> +	---help---
> +	 This driver supports virtio input devices such as
> +	 keyboards, mice and tablets.
> +
> +	 If unsure, say M.
> +
>   config VIRTIO_MMIO
>  	tristate "Platform bus driver for memory mapped virtio devices"
>  	depends on HAS_IOMEM
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index d85565b..41e30e3 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
>  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> new file mode 100644
> index 0000000..2d425cf
> --- /dev/null
> +++ b/drivers/virtio/virtio_input.c
> @@ -0,0 +1,313 @@
> +#include <linux/module.h>
> +#include <linux/virtio.h>
> +#include <linux/input.h>
> +
> +#include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_input.h>
> +
> +struct virtio_input {
> +	struct virtio_device       *vdev;
> +	struct input_dev           *idev;
> +	char                       name[64];
> +	char                       serial[64];
> +	char                       phys[64];
> +	struct virtqueue           *evt, *sts;
> +	struct virtio_input_event  evts[64];
> +};
> +
> +static ssize_t serial_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct input_dev *idev = to_input_dev(dev);
> +	struct virtio_input *vi = input_get_drvdata(idev);
> +	return sprintf(buf, "%s\n", vi->serial);
> +}
> +static DEVICE_ATTR_RO(serial);
> +
> +static struct attribute *dev_attrs[] = {
> +	&dev_attr_serial.attr,
> +	NULL
> +};
> +
> +static umode_t dev_attrs_are_visible(struct kobject *kobj,
> +				     struct attribute *a, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct input_dev *idev = to_input_dev(dev);
> +	struct virtio_input *vi = input_get_drvdata(idev);
> +
> +	if (a == &dev_attr_serial.attr && !strlen(vi->serial))
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +static struct attribute_group dev_attr_grp = {
> +	.attrs =	dev_attrs,
> +	.is_visible =	dev_attrs_are_visible,
> +};
> +
> +static const struct attribute_group *dev_attr_groups[] = {
> +	&dev_attr_grp,
> +	NULL
> +};
> +
> +static void virtinput_queue_evtbuf(struct virtio_input *vi,
> +				   struct virtio_input_event *evtbuf)
> +{
> +	struct scatterlist sg[1];
> +
> +	sg_init_one(sg, evtbuf, sizeof(*evtbuf));
> +	virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
> +}
> +
> +static void virtinput_recv_events(struct virtqueue *vq)
> +{
> +	struct virtio_input *vi = vq->vdev->priv;
> +	struct virtio_input_event *event;
> +	unsigned int len;
> +
> +	while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> +		input_event(vi->idev,
> +			    le16_to_cpu(event->type),
> +			    le16_to_cpu(event->code),
> +			    le32_to_cpu(event->value));
> +		virtinput_queue_evtbuf(vi, event);
> +	}
> +	virtqueue_kick(vq);
> +}
> +
> +static int virtinput_send_status(struct virtio_input *vi,
> +				 u16 type, u16 code, s32 value)
> +{
> +	struct virtio_input_event *stsbuf;
> +	struct scatterlist sg[1];
> +
> +	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> +	if (!stsbuf)
> +		return -ENOMEM;

Does this return an error to userspace?
If so it's not a good idea I think, GFP_ATOMIC failures are
transient conditions and should not be reported
to userspace.
Can use GFP_KERNEL here?

> +
> +	stsbuf->type  = cpu_to_le16(type);
> +	stsbuf->code  = cpu_to_le16(code);
> +	stsbuf->value = cpu_to_le32(value);
> +	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> +	virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);

This can fail if queue is full. What prevents this from happening?

> +	virtqueue_kick(vi->sts);

Also what prevents multiple virtinput_send_status calls
from racing with each other? Is there locking at a higher level?

> +	return 0;
> +}
> +
> +static void virtinput_recv_status(struct virtqueue *vq)
> +{
> +	struct virtio_input *vi = vq->vdev->priv;
> +	struct virtio_input_event *stsbuf;
> +	unsigned int len;
> +
> +	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)

looks like this get_buf can race with add above.
Need some locking.

Also pls avoid != NULL, removing it makes code less verbose.

> +		kfree(stsbuf);
> +	virtqueue_kick(vq);

Why are you kicking here?

> +}
> +
> +static int virtinput_status(struct input_dev *idev, unsigned int type,
> +			    unsigned int code, int value)
> +{
> +	struct virtio_input *vi = input_get_drvdata(idev);
> +
> +	return virtinput_send_status(vi, type, code, value);
> +}
> +
> +static size_t virtinput_cfg_select(struct virtio_input *vi,
> +				   u8 select, u8 subsel)
> +{
> +	u8 size;
> +
> +	virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
> +	virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
> +	virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
> +	return size;
> +}
> +
> +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> +			       unsigned long *bits, unsigned int bitcount)
> +{
> +	unsigned int bit;
> +	size_t bytes;
> +	u8 cfg = 0;
> +
> +	bytes = virtinput_cfg_select(vi, select, subsel);
> +	if (!bytes)
> +		return;
> +	if (bitcount > bytes*8)
> +		bitcount = bytes*8;
> +	if (select == VIRTIO_INPUT_CFG_EV_BITS)
> +		set_bit(subsel, vi->idev->evbit);
> +	for (bit = 0; bit < bitcount; bit++) {
> +		if ((bit % 8) == 0)
> +			virtio_cread(vi->vdev, struct virtio_input_config,
> +				     u.bitmap[bit/8], &cfg);

coding style violations above. you need spaces around ops like / and *.
Please run checkpatch.pl

> +		if (cfg & (1 << (bit % 8)))
> +			set_bit(bit, bits);

what if not set? does something clear the mask?


> +	}

doesn't above just implement bitmap_copy or bitmap_or?  Will it hurt to
just do virtio_cread_bytes into a temporary buffer and then invoke
bitmap ops? Looks like the buffer is at most 256 bytes:
too large to be on stack, but you can allocate it at probe time.


> +}
> +
> +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> +{
> +	u32 size, mi, ma, fu, fl;
> +
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);

you read le field into u32 value.
Please run sparse on this code. you will get a ton
of warnings. Same error appears elsewhere.

> +	input_set_abs_params(vi->idev, abs,
> +			     le32_to_cpu(mi), le32_to_cpu(ma),
> +			     le32_to_cpu(fu), le32_to_cpu(fl));
> +}
> +
> +static int virtinput_init_vqs(struct virtio_input *vi)
> +{
> +	struct virtqueue *vqs[2];
> +	vq_callback_t *cbs[] = { virtinput_recv_events,
> +				 virtinput_recv_status };
> +	const char *names[] = { "events", "status" };
> +	int i, err, size;
> +
> +	err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
> +	if (err)
> +		return err;
> +	vi->evt = vqs[0];
> +	vi->sts = vqs[1];
> +
> +	size = virtqueue_get_vring_size(vi->evt);
> +	if (size > ARRAY_SIZE(vi->evts))
> +		size = ARRAY_SIZE(vi->evts);
> +	for (i = 0; i < size; i++)
> +		virtinput_queue_evtbuf(vi, &vi->evts[i]);
> +

you add a bunch of bufs but never kick apparently.
you really must kick otherwise device might not
see the buffers.

> +	return 0;
> +}
> +
> +static int virtinput_probe(struct virtio_device *vdev)
> +{
> +	struct virtio_input *vi;
> +	size_t size;
> +	int abs, err;

How about checking VERSION_1 and bailing out of not there?

> +
> +	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> +	if (!vi) {
> +		err = -ENOMEM;
> +		goto out1;
> +	}
> +	vdev->priv = vi;
> +	vi->vdev = vdev;
> +
> +	err = virtinput_init_vqs(vi);
> +	if (err)
> +		goto out2;
> +
> +	vi->idev = input_allocate_device();
> +	if (!vi->idev) {
> +		err = -ENOMEM;
> +		goto out3;
> +	}
> +	input_set_drvdata(vi->idev, vi);
> +
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
> +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> +			   vi->name, min(size, sizeof(vi->name)));
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
> +	virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> +			   vi->serial, min(size, sizeof(vi->serial)));
> +	snprintf(vi->phys, sizeof(vi->phys),
> +		 "virtio%d/input0", vdev->index);
> +
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
> +			   vi->idev->propbit, INPUT_PROP_CNT);
> +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
> +	if (size)
> +		set_bit(EV_REP, vi->idev->evbit);
> +
> +	vi->idev->name = vi->name;
> +	vi->idev->phys = vi->phys;
> +	vi->idev->id.bustype = BUS_VIRTUAL;
> +	vi->idev->id.vendor  = 0x0001;
> +	vi->idev->id.product = 0x0001;
> +	vi->idev->id.version = 0x0100;

Add comments explaining why these #s make sense?


> +	vi->idev->dev.parent = &vdev->dev;
> +	vi->idev->dev.groups = dev_attr_groups;
> +	vi->idev->event = virtinput_status;
> +
> +	/* device -> kernel */
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_KEY,
> +			   vi->idev->keybit, KEY_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REL,
> +			   vi->idev->relbit, REL_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_ABS,
> +			   vi->idev->absbit, ABS_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_MSC,
> +			   vi->idev->mscbit, MSC_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SW,
> +			   vi->idev->swbit,  SW_CNT);
> +
> +	/* kernel -> device */
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_LED,
> +			   vi->idev->ledbit, LED_CNT);
> +	virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SND,
> +			   vi->idev->sndbit, SND_CNT);
> +
> +	if (test_bit(EV_ABS, vi->idev->evbit)) {
> +		for (abs = 0; abs < ABS_CNT; abs++) {
> +			if (!test_bit(abs, vi->idev->absbit))
> +				continue;
> +			virtinput_cfg_abs(vi, abs);
> +		}
> +	}
> +
> +	err = input_register_device(vi->idev);

Once you do this, virtinput_status can get called,
and that will kick, correct?
If so, you must call device_ready before this.

> +	if (err)
> +		goto out4;
> +
> +	return 0;
> +
> +out4:
> +	input_free_device(vi->idev);
> +out3:
> +	vdev->config->del_vqs(vdev);
> +out2:
> +	kfree(vi);
> +out1:
> +	return err;

free on error is out of order with initialization.
Might lead to leaks or other bugs.
Also - can you name labels something sensible pls?
out is usually for exiting on success too...
E.g. out4 -> err_register etc.


> +}
> +
> +static void virtinput_remove(struct virtio_device *vdev)
> +{
> +	struct virtio_input *vi = vdev->priv;
> +
> +	input_unregister_device(vi->idev);
> +	vdev->config->reset(vdev);

You don't really need a reset if you just to del_vqs.
People do this if they want to prevent interrupts
without deleting vqs.

> +	vdev->config->del_vqs(vdev);
> +	kfree(vi);

free_device seems to be missing?

> +}
> +
> +static unsigned int features[] = {
> +};
> +static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_INPUT, VIRTIO_DEV_ANY_ID },
> +	{ 0 },
> +};
> +
> +static struct virtio_driver virtio_input_driver = {
> +	.driver.name         = KBUILD_MODNAME,
> +	.driver.owner        = THIS_MODULE,
> +	.feature_table       = features,
> +	.feature_table_size  = ARRAY_SIZE(features),
> +	.id_table            = id_table,
> +	.probe               = virtinput_probe,
> +	.remove              = virtinput_remove,
> +};
> +
> +module_virtio_driver(virtio_input_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Virtio input device driver");
> +MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 284fc3a..5f60aa4 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -39,5 +39,6 @@
>  #define VIRTIO_ID_9P		9 /* 9p virtio console */
>  #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
>  #define VIRTIO_ID_CAIF	       12 /* Virtio caif */
> +#define VIRTIO_ID_INPUT        18 /* virtio input */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/uapi/linux/virtio_input.h b/include/uapi/linux/virtio_input.h
> new file mode 100644
> index 0000000..190d04a
> --- /dev/null
> +++ b/include/uapi/linux/virtio_input.h
> @@ -0,0 +1,65 @@
> +#ifndef _LINUX_VIRTIO_INPUT_H
> +#define _LINUX_VIRTIO_INPUT_H
> +/* This header is BSD licensed so anyone can use the definitions to implement
> + * compatible drivers/servers.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors
> + *    may be used to endorse or promote products derived from this software
> + *    without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED.  IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE. */
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +
> +enum virtio_input_config_select {
> +	VIRTIO_INPUT_CFG_UNSET      = 0x00,
> +	VIRTIO_INPUT_CFG_ID_NAME    = 0x01,
> +	VIRTIO_INPUT_CFG_ID_SERIAL  = 0x02,
> +	VIRTIO_INPUT_CFG_PROP_BITS  = 0x10,
> +	VIRTIO_INPUT_CFG_EV_BITS    = 0x11,
> +	VIRTIO_INPUT_CFG_ABS_INFO   = 0x12,
> +};
> +
> +struct virtio_input_absinfo {
> +	__le32  min;
> +	__le32  max;
> +	__le32  fuzz;
> +	__le32  flat;
> +};
> +
> +struct virtio_input_config {
> +	__u8    select;
> +	__u8    subsel;
> +	__u8    size;
> +	__u8    reserved;
> +	union {
> +		char string[128];
> +		__u8 bitmap[128];
> +		struct virtio_input_absinfo abs;
> +	} u;
> +};
> +
> +struct virtio_input_event {
> +	__le16 type;
> +	__le16 code;
> +	__le32 value;
> +};
> +
> +#endif /* _LINUX_VIRTIO_INPUT_H */
> -- 
> 1.8.3.1

Re: [PATCH 1/1] Add virtio-input driver.

[David Herrmann]

Hi

(CC: Dmitry)

On Thu, Mar 19, 2015 at 10:13 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> virtio-input is basically evdev-events-over-virtio, so this driver isn't
> much more than reading configuration from config space and forwarding
> incoming events to the linux input layer.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/virtio/Kconfig            |  10 ++
>  drivers/virtio/Makefile           |   1 +
>  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/virtio_ids.h   |   1 +
>  include/uapi/linux/virtio_input.h |  65 ++++++++
>  5 files changed, 390 insertions(+)
>  create mode 100644 drivers/virtio/virtio_input.c
>  create mode 100644 include/uapi/linux/virtio_input.h
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index b546da5..cab9f3f 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
>
>          If unsure, say M.
>
> +config VIRTIO_INPUT
> +       tristate "Virtio input driver"
> +       depends on VIRTIO
> +       depends on INPUT
> +       ---help---
> +        This driver supports virtio input devices such as
> +        keyboards, mice and tablets.
> +
> +        If unsure, say M.
> +
>   config VIRTIO_MMIO
>         tristate "Platform bus driver for memory mapped virtio devices"
>         depends on HAS_IOMEM
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index d85565b..41e30e3 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
>  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> new file mode 100644
> index 0000000..2d425cf
> --- /dev/null
> +++ b/drivers/virtio/virtio_input.c
> @@ -0,0 +1,313 @@
> +#include <linux/module.h>
> +#include <linux/virtio.h>
> +#include <linux/input.h>
> +
> +#include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_input.h>
> +
> +struct virtio_input {
> +       struct virtio_device       *vdev;
> +       struct input_dev           *idev;
> +       char                       name[64];
> +       char                       serial[64];
> +       char                       phys[64];
> +       struct virtqueue           *evt, *sts;
> +       struct virtio_input_event  evts[64];
> +};
> +
> +static ssize_t serial_show(struct device *dev,
> +                          struct device_attribute *attr, char *buf)
> +{
> +       struct input_dev *idev = to_input_dev(dev);
> +       struct virtio_input *vi = input_get_drvdata(idev);
> +       return sprintf(buf, "%s\n", vi->serial);
> +}
> +static DEVICE_ATTR_RO(serial);
> +
> +static struct attribute *dev_attrs[] = {
> +       &dev_attr_serial.attr,
> +       NULL
> +};
> +
> +static umode_t dev_attrs_are_visible(struct kobject *kobj,
> +                                    struct attribute *a, int n)
> +{
> +       struct device *dev = container_of(kobj, struct device, kobj);
> +       struct input_dev *idev = to_input_dev(dev);
> +       struct virtio_input *vi = input_get_drvdata(idev);
> +
> +       if (a == &dev_attr_serial.attr && !strlen(vi->serial))
> +               return 0;
> +
> +       return a->mode;
> +}
> +
> +static struct attribute_group dev_attr_grp = {
> +       .attrs =        dev_attrs,
> +       .is_visible =   dev_attrs_are_visible,
> +};
> +
> +static const struct attribute_group *dev_attr_groups[] = {
> +       &dev_attr_grp,
> +       NULL
> +};
> +
> +static void virtinput_queue_evtbuf(struct virtio_input *vi,
> +                                  struct virtio_input_event *evtbuf)
> +{
> +       struct scatterlist sg[1];
> +
> +       sg_init_one(sg, evtbuf, sizeof(*evtbuf));
> +       virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
> +}
> +
> +static void virtinput_recv_events(struct virtqueue *vq)
> +{
> +       struct virtio_input *vi = vq->vdev->priv;
> +       struct virtio_input_event *event;
> +       unsigned int len;
> +
> +       while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> +               input_event(vi->idev,
> +                           le16_to_cpu(event->type),
> +                           le16_to_cpu(event->code),
> +                           le32_to_cpu(event->value));
> +               virtinput_queue_evtbuf(vi, event);
> +       }
> +       virtqueue_kick(vq);
> +}
> +
> +static int virtinput_send_status(struct virtio_input *vi,
> +                                u16 type, u16 code, s32 value)
> +{
> +       struct virtio_input_event *stsbuf;
> +       struct scatterlist sg[1];
> +
> +       stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> +       if (!stsbuf)
> +               return -ENOMEM;
> +
> +       stsbuf->type  = cpu_to_le16(type);
> +       stsbuf->code  = cpu_to_le16(code);
> +       stsbuf->value = cpu_to_le32(value);
> +       sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> +       virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> +       virtqueue_kick(vi->sts);

GFP_ATOMIC, eww. But everyone does that for input_event() callbacks..
we should fix that for user-space input one day.

> +       return 0;
> +}
> +
> +static void virtinput_recv_status(struct virtqueue *vq)
> +{
> +       struct virtio_input *vi = vq->vdev->priv;
> +       struct virtio_input_event *stsbuf;
> +       unsigned int len;
> +
> +       while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> +               kfree(stsbuf);
> +       virtqueue_kick(vq);
> +}
> +
> +static int virtinput_status(struct input_dev *idev, unsigned int type,
> +                           unsigned int code, int value)
> +{
> +       struct virtio_input *vi = input_get_drvdata(idev);
> +
> +       return virtinput_send_status(vi, type, code, value);
> +}
> +
> +static size_t virtinput_cfg_select(struct virtio_input *vi,
> +                                  u8 select, u8 subsel)
> +{
> +       u8 size;
> +
> +       virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
> +       virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
> +       virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
> +       return size;
> +}
> +
> +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> +                              unsigned long *bits, unsigned int bitcount)
> +{
> +       unsigned int bit;
> +       size_t bytes;
> +       u8 cfg = 0;
> +
> +       bytes = virtinput_cfg_select(vi, select, subsel);
> +       if (!bytes)
> +               return;
> +       if (bitcount > bytes*8)
> +               bitcount = bytes*8;
> +       if (select == VIRTIO_INPUT_CFG_EV_BITS)
> +               set_bit(subsel, vi->idev->evbit);
> +       for (bit = 0; bit < bitcount; bit++) {
> +               if ((bit % 8) == 0)
> +                       virtio_cread(vi->vdev, struct virtio_input_config,
> +                                    u.bitmap[bit/8], &cfg);
> +               if (cfg & (1 << (bit % 8)))
> +                       set_bit(bit, bits);
> +       }
> +}
> +
> +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> +{
> +       u32 size, mi, ma, fu, fl;
> +
> +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);

abs.resolution is missing. Please add it, we really also need to add
it to uinput one day.

> +       input_set_abs_params(vi->idev, abs,
> +                            le32_to_cpu(mi), le32_to_cpu(ma),
> +                            le32_to_cpu(fu), le32_to_cpu(fl));
> +}
> +
> +static int virtinput_init_vqs(struct virtio_input *vi)
> +{
> +       struct virtqueue *vqs[2];
> +       vq_callback_t *cbs[] = { virtinput_recv_events,
> +                                virtinput_recv_status };
> +       const char *names[] = { "events", "status" };
> +       int i, err, size;
> +
> +       err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
> +       if (err)
> +               return err;
> +       vi->evt = vqs[0];
> +       vi->sts = vqs[1];
> +
> +       size = virtqueue_get_vring_size(vi->evt);
> +       if (size > ARRAY_SIZE(vi->evts))
> +               size = ARRAY_SIZE(vi->evts);
> +       for (i = 0; i < size; i++)
> +               virtinput_queue_evtbuf(vi, &vi->evts[i]);
> +
> +       return 0;
> +}
> +
> +static int virtinput_probe(struct virtio_device *vdev)
> +{
> +       struct virtio_input *vi;
> +       size_t size;
> +       int abs, err;
> +
> +       vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> +       if (!vi) {
> +               err = -ENOMEM;
> +               goto out1;
> +       }
> +       vdev->priv = vi;
> +       vi->vdev = vdev;
> +
> +       err = virtinput_init_vqs(vi);
> +       if (err)
> +               goto out2;
> +
> +       vi->idev = input_allocate_device();
> +       if (!vi->idev) {
> +               err = -ENOMEM;
> +               goto out3;
> +       }
> +       input_set_drvdata(vi->idev, vi);
> +
> +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
> +       virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> +                          vi->name, min(size, sizeof(vi->name)));
> +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
> +       virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> +                          vi->serial, min(size, sizeof(vi->serial)));
> +       snprintf(vi->phys, sizeof(vi->phys),
> +                "virtio%d/input0", vdev->index);
> +
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
> +                          vi->idev->propbit, INPUT_PROP_CNT);
> +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
> +       if (size)
> +               set_bit(EV_REP, vi->idev->evbit);
> +
> +       vi->idev->name = vi->name;
> +       vi->idev->phys = vi->phys;

Can you set vi->idev->uniq to the virtio-bus path?

> +       vi->idev->id.bustype = BUS_VIRTUAL;
> +       vi->idev->id.vendor  = 0x0001;
> +       vi->idev->id.product = 0x0001;
> +       vi->idev->id.version = 0x0100;

Please don't hardcode those. All user-space based interaction with
input-devices relies on those IDs. Can we retrieve it from the host
just like the name?

> +       vi->idev->dev.parent = &vdev->dev;
> +       vi->idev->dev.groups = dev_attr_groups;
> +       vi->idev->event = virtinput_status;
> +
> +       /* device -> kernel */
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_KEY,
> +                          vi->idev->keybit, KEY_CNT);
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REL,
> +                          vi->idev->relbit, REL_CNT);
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_ABS,
> +                          vi->idev->absbit, ABS_CNT);
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_MSC,
> +                          vi->idev->mscbit, MSC_CNT);
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SW,
> +                          vi->idev->swbit,  SW_CNT);
> +
> +       /* kernel -> device */
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_LED,
> +                          vi->idev->ledbit, LED_CNT);
> +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_SND,
> +                          vi->idev->sndbit, SND_CNT);
> +
> +       if (test_bit(EV_ABS, vi->idev->evbit)) {
> +               for (abs = 0; abs < ABS_CNT; abs++) {
> +                       if (!test_bit(abs, vi->idev->absbit))
> +                               continue;
> +                       virtinput_cfg_abs(vi, abs);
> +               }
> +       }
> +
> +       err = input_register_device(vi->idev);
> +       if (err)
> +               goto out4;
> +
> +       return 0;
> +
> +out4:
> +       input_free_device(vi->idev);
> +out3:
> +       vdev->config->del_vqs(vdev);
> +out2:
> +       kfree(vi);
> +out1:
> +       return err;
> +}
> +
> +static void virtinput_remove(struct virtio_device *vdev)
> +{
> +       struct virtio_input *vi = vdev->priv;
> +
> +       input_unregister_device(vi->idev);
> +       vdev->config->reset(vdev);
> +       vdev->config->del_vqs(vdev);
> +       kfree(vi);
> +}
> +
> +static unsigned int features[] = {
> +};
> +static struct virtio_device_id id_table[] = {
> +       { VIRTIO_ID_INPUT, VIRTIO_DEV_ANY_ID },
> +       { 0 },
> +};
> +
> +static struct virtio_driver virtio_input_driver = {
> +       .driver.name         = KBUILD_MODNAME,
> +       .driver.owner        = THIS_MODULE,
> +       .feature_table       = features,
> +       .feature_table_size  = ARRAY_SIZE(features),
> +       .id_table            = id_table,
> +       .probe               = virtinput_probe,
> +       .remove              = virtinput_remove,
> +};
> +
> +module_virtio_driver(virtio_input_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Virtio input device driver");
> +MODULE_AUTHOR("Gerd Hoffmann <kraxel@redhat.com>");
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 284fc3a..5f60aa4 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -39,5 +39,6 @@
>  #define VIRTIO_ID_9P           9 /* 9p virtio console */
>  #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
>  #define VIRTIO_ID_CAIF        12 /* Virtio caif */
> +#define VIRTIO_ID_INPUT        18 /* virtio input */
>
>  #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/uapi/linux/virtio_input.h b/include/uapi/linux/virtio_input.h
> new file mode 100644
> index 0000000..190d04a
> --- /dev/null
> +++ b/include/uapi/linux/virtio_input.h
> @@ -0,0 +1,65 @@
> +#ifndef _LINUX_VIRTIO_INPUT_H
> +#define _LINUX_VIRTIO_INPUT_H
> +/* This header is BSD licensed so anyone can use the definitions to implement
> + * compatible drivers/servers.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors
> + *    may be used to endorse or promote products derived from this software
> + *    without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED.  IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE. */
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +
> +enum virtio_input_config_select {
> +       VIRTIO_INPUT_CFG_UNSET      = 0x00,
> +       VIRTIO_INPUT_CFG_ID_NAME    = 0x01,
> +       VIRTIO_INPUT_CFG_ID_SERIAL  = 0x02,
> +       VIRTIO_INPUT_CFG_PROP_BITS  = 0x10,
> +       VIRTIO_INPUT_CFG_EV_BITS    = 0x11,
> +       VIRTIO_INPUT_CFG_ABS_INFO   = 0x12,
> +};
> +
> +struct virtio_input_absinfo {
> +       __le32  min;
> +       __le32  max;
> +       __le32  fuzz;
> +       __le32  flat;
> +};
> +
> +struct virtio_input_config {
> +       __u8    select;
> +       __u8    subsel;
> +       __u8    size;
> +       __u8    reserved;
> +       union {
> +               char string[128];
> +               __u8 bitmap[128];
> +               struct virtio_input_absinfo abs;
> +       } u;
> +};
> +
> +struct virtio_input_event {
> +       __le16 type;
> +       __le16 code;
> +       __le32 value;
> +};
> +
> +#endif /* _LINUX_VIRTIO_INPUT_H */
> --

The input-parts look good to me (apart from my comments). No idea how
virtio exactly works, so I'll leave that to Rusty.

I put Dmitry on CC, he might have some more valuable input on the input-parts.

Thanks
David

Re: [PATCH 1/1] Add virtio-input driver.

[Dmitry Torokhov]

On Thu, Mar 19, 2015 at 01:29:49PM +0100, David Herrmann wrote:
> Hi
> 
> (CC: Dmitry)
> 
> On Thu, Mar 19, 2015 at 10:13 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > virtio-input is basically evdev-events-over-virtio, so this driver isn't
> > much more than reading configuration from config space and forwarding
> > incoming events to the linux input layer.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  drivers/virtio/Kconfig            |  10 ++
> >  drivers/virtio/Makefile           |   1 +
> >  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/virtio_ids.h   |   1 +
> >  include/uapi/linux/virtio_input.h |  65 ++++++++
> >  5 files changed, 390 insertions(+)
> >  create mode 100644 drivers/virtio/virtio_input.c
> >  create mode 100644 include/uapi/linux/virtio_input.h
> >
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index b546da5..cab9f3f 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -48,6 +48,16 @@ config VIRTIO_BALLOON
> >
> >          If unsure, say M.
> >
> > +config VIRTIO_INPUT
> > +       tristate "Virtio input driver"
> > +       depends on VIRTIO
> > +       depends on INPUT
> > +       ---help---
> > +        This driver supports virtio input devices such as
> > +        keyboards, mice and tablets.
> > +
> > +        If unsure, say M.
> > +
> >   config VIRTIO_MMIO
> >         tristate "Platform bus driver for memory mapped virtio devices"
> >         depends on HAS_IOMEM
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index d85565b..41e30e3 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -4,3 +4,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> >  virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> >  virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> >  obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > +obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> > new file mode 100644
> > index 0000000..2d425cf
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_input.c
> > @@ -0,0 +1,313 @@
> > +#include <linux/module.h>
> > +#include <linux/virtio.h>
> > +#include <linux/input.h>
> > +
> > +#include <uapi/linux/virtio_ids.h>
> > +#include <uapi/linux/virtio_input.h>
> > +
> > +struct virtio_input {
> > +       struct virtio_device       *vdev;
> > +       struct input_dev           *idev;
> > +       char                       name[64];
> > +       char                       serial[64];
> > +       char                       phys[64];
> > +       struct virtqueue           *evt, *sts;
> > +       struct virtio_input_event  evts[64];
> > +};
> > +
> > +static ssize_t serial_show(struct device *dev,
> > +                          struct device_attribute *attr, char *buf)
> > +{
> > +       struct input_dev *idev = to_input_dev(dev);
> > +       struct virtio_input *vi = input_get_drvdata(idev);
> > +       return sprintf(buf, "%s\n", vi->serial);
> > +}
> > +static DEVICE_ATTR_RO(serial);

What is serial? Serial number?

> > +
> > +static struct attribute *dev_attrs[] = {
> > +       &dev_attr_serial.attr,
> > +       NULL
> > +};
> > +
> > +static umode_t dev_attrs_are_visible(struct kobject *kobj,
> > +                                    struct attribute *a, int n)
> > +{
> > +       struct device *dev = container_of(kobj, struct device, kobj);
> > +       struct input_dev *idev = to_input_dev(dev);
> > +       struct virtio_input *vi = input_get_drvdata(idev);
> > +
> > +       if (a == &dev_attr_serial.attr && !strlen(vi->serial))
> > +               return 0;
> > +
> > +       return a->mode;
> > +}
> > +
> > +static struct attribute_group dev_attr_grp = {
> > +       .attrs =        dev_attrs,
> > +       .is_visible =   dev_attrs_are_visible,
> > +};
> > +
> > +static const struct attribute_group *dev_attr_groups[] = {
> > +       &dev_attr_grp,
> > +       NULL
> > +};
> > +
> > +static void virtinput_queue_evtbuf(struct virtio_input *vi,
> > +                                  struct virtio_input_event *evtbuf)
> > +{
> > +       struct scatterlist sg[1];
> > +
> > +       sg_init_one(sg, evtbuf, sizeof(*evtbuf));
> > +       virtqueue_add_inbuf(vi->evt, sg, 1, evtbuf, GFP_ATOMIC);
> > +}
> > +
> > +static void virtinput_recv_events(struct virtqueue *vq)
> > +{
> > +       struct virtio_input *vi = vq->vdev->priv;
> > +       struct virtio_input_event *event;
> > +       unsigned int len;
> > +
> > +       while ((event = virtqueue_get_buf(vi->evt, &len)) != NULL) {
> > +               input_event(vi->idev,
> > +                           le16_to_cpu(event->type),
> > +                           le16_to_cpu(event->code),
> > +                           le32_to_cpu(event->value));
> > +               virtinput_queue_evtbuf(vi, event);
> > +       }
> > +       virtqueue_kick(vq);
> > +}
> > +
> > +static int virtinput_send_status(struct virtio_input *vi,
> > +                                u16 type, u16 code, s32 value)
> > +{
> > +       struct virtio_input_event *stsbuf;
> > +       struct scatterlist sg[1];
> > +
> > +       stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > +       if (!stsbuf)
> > +               return -ENOMEM;
> > +
> > +       stsbuf->type  = cpu_to_le16(type);
> > +       stsbuf->code  = cpu_to_le16(code);
> > +       stsbuf->value = cpu_to_le32(value);
> > +       sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > +       virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> > +       virtqueue_kick(vi->sts);
> 
> GFP_ATOMIC, eww. But everyone does that for input_event() callbacks..
> we should fix that for user-space input one day.
> 
> > +       return 0;
> > +}
> > +
> > +static void virtinput_recv_status(struct virtqueue *vq)
> > +{
> > +       struct virtio_input *vi = vq->vdev->priv;
> > +       struct virtio_input_event *stsbuf;
> > +       unsigned int len;
> > +
> > +       while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> > +               kfree(stsbuf);
> > +       virtqueue_kick(vq);
> > +}
> > +
> > +static int virtinput_status(struct input_dev *idev, unsigned int type,
> > +                           unsigned int code, int value)
> > +{
> > +       struct virtio_input *vi = input_get_drvdata(idev);
> > +
> > +       return virtinput_send_status(vi, type, code, value);
> > +}
> > +
> > +static size_t virtinput_cfg_select(struct virtio_input *vi,
> > +                                  u8 select, u8 subsel)
> > +{
> > +       u8 size;
> > +
> > +       virtio_cwrite(vi->vdev, struct virtio_input_config, select, &select);
> > +       virtio_cwrite(vi->vdev, struct virtio_input_config, subsel, &subsel);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, size, &size);
> > +       return size;
> > +}
> > +
> > +static void virtinput_cfg_bits(struct virtio_input *vi, int select, int subsel,
> > +                              unsigned long *bits, unsigned int bitcount)
> > +{
> > +       unsigned int bit;
> > +       size_t bytes;
> > +       u8 cfg = 0;
> > +
> > +       bytes = virtinput_cfg_select(vi, select, subsel);
> > +       if (!bytes)
> > +               return;
> > +       if (bitcount > bytes*8)
> > +               bitcount = bytes*8;
> > +       if (select == VIRTIO_INPUT_CFG_EV_BITS)
> > +               set_bit(subsel, vi->idev->evbit);
> > +       for (bit = 0; bit < bitcount; bit++) {
> > +               if ((bit % 8) == 0)
> > +                       virtio_cread(vi->vdev, struct virtio_input_config,
> > +                                    u.bitmap[bit/8], &cfg);
> > +               if (cfg & (1 << (bit % 8)))
> > +                       set_bit(bit, bits);
> > +       }
> > +}
> > +
> > +static void virtinput_cfg_abs(struct virtio_input *vi, int abs)
> > +{
> > +       u32 size, mi, ma, fu, fl;
> > +
> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> 
> abs.resolution is missing. Please add it, we really also need to add
> it to uinput one day.
> 
> > +       input_set_abs_params(vi->idev, abs,
> > +                            le32_to_cpu(mi), le32_to_cpu(ma),
> > +                            le32_to_cpu(fu), le32_to_cpu(fl));
> > +}
> > +
> > +static int virtinput_init_vqs(struct virtio_input *vi)
> > +{
> > +       struct virtqueue *vqs[2];
> > +       vq_callback_t *cbs[] = { virtinput_recv_events,
> > +                                virtinput_recv_status };
> > +       const char *names[] = { "events", "status" };
> > +       int i, err, size;
> > +
> > +       err = vi->vdev->config->find_vqs(vi->vdev, 2, vqs, cbs, names);
> > +       if (err)
> > +               return err;
> > +       vi->evt = vqs[0];
> > +       vi->sts = vqs[1];
> > +
> > +       size = virtqueue_get_vring_size(vi->evt);
> > +       if (size > ARRAY_SIZE(vi->evts))
> > +               size = ARRAY_SIZE(vi->evts);
> > +       for (i = 0; i < size; i++)
> > +               virtinput_queue_evtbuf(vi, &vi->evts[i]);
> > +
> > +       return 0;
> > +}
> > +
> > +static int virtinput_probe(struct virtio_device *vdev)
> > +{
> > +       struct virtio_input *vi;
> > +       size_t size;
> > +       int abs, err;
> > +
> > +       vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> > +       if (!vi) {
> > +               err = -ENOMEM;
> > +               goto out1;
> > +       }
> > +       vdev->priv = vi;
> > +       vi->vdev = vdev;
> > +
> > +       err = virtinput_init_vqs(vi);
> > +       if (err)
> > +               goto out2;
> > +
> > +       vi->idev = input_allocate_device();
> > +       if (!vi->idev) {
> > +               err = -ENOMEM;
> > +               goto out3;
> > +       }
> > +       input_set_drvdata(vi->idev, vi);
> > +
> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
> > +       virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > +                          vi->name, min(size, sizeof(vi->name)));
> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
> > +       virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
> > +                          vi->serial, min(size, sizeof(vi->serial)));
> > +       snprintf(vi->phys, sizeof(vi->phys),
> > +                "virtio%d/input0", vdev->index);
> > +
> > +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
> > +                          vi->idev->propbit, INPUT_PROP_CNT);
> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
> > +       if (size)
> > +               set_bit(EV_REP, vi->idev->evbit);
> > +
> > +       vi->idev->name = vi->name;
> > +       vi->idev->phys = vi->phys;
> 
> Can you set vi->idev->uniq to the virtio-bus path?

No, uniq can't be phys as phys is unique within the system while uniq is
like serial number or UUID and should never repeat.

Thanks.

-- 
Dmitry

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > +static int virtinput_send_status(struct virtio_input *vi,
> > +                                u16 type, u16 code, s32 value)
> > +{
> > +       struct virtio_input_event *stsbuf;
> > +       struct scatterlist sg[1];
> > +
> > +       stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > +       if (!stsbuf)
> > +               return -ENOMEM;
> > +
> > +       stsbuf->type  = cpu_to_le16(type);
> > +       stsbuf->code  = cpu_to_le16(code);
> > +       stsbuf->value = cpu_to_le32(value);
> > +       sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > +       virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> > +       virtqueue_kick(vi->sts);
> 
> GFP_ATOMIC, eww. But everyone does that for input_event() callbacks..

Yea, did it this way because I saw it elsewhere.

> we should fix that for user-space input one day.

Sounds like I have to use GFP_ATOMIC and can't switch to GFP_KERNEL,
correct?

> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> 
> abs.resolution is missing. Please add it, we really also need to add
> it to uinput one day.

Ok.  How should I handle cases where the resolution is either not known
or not fixed?  Just leave it zero?

> > +       vi->idev->name = vi->name;
> > +       vi->idev->phys = vi->phys;
> 
> Can you set vi->idev->uniq to the virtio-bus path?
> 
> > +       vi->idev->id.bustype = BUS_VIRTUAL;
> > +       vi->idev->id.vendor  = 0x0001;
> > +       vi->idev->id.product = 0x0001;
> > +       vi->idev->id.version = 0x0100;
> 
> Please don't hardcode those. All user-space based interaction with
> input-devices relies on those IDs. Can we retrieve it from the host
> just like the name?

Yes, we can.

There will be emulated devices, i.e. the input coming from
vnc/gtk/whatever will be sent to the virtio devices (instead of ps/2 or
usb).  For these we should probably have fixed IDs per device.  There
are keyboard/mouse/tablet at the moment.  Suggestions how to pick IDs?

There will also be pass-through support, i.e. qemu
opening /dev/input/event<nr> and forwarding everything to the guest.
How should that be handled best?  Copy all four from the host?  Even
though the bustype is BUS_USB?  Not sure this actually improves things
because the guest can match the device, or whenever this confuses apps
due to BUS_USB being applied to virtio devices ...

cheers,
  Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[David Herrmann]

Hey

On Thu, Mar 19, 2015 at 5:27 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Mar 19, 2015 at 01:29:49PM +0100, David Herrmann wrote:
[...]
>> > +static int virtinput_probe(struct virtio_device *vdev)
>> > +{
>> > +       struct virtio_input *vi;
>> > +       size_t size;
>> > +       int abs, err;
>> > +
>> > +       vi = kzalloc(sizeof(*vi), GFP_KERNEL);
>> > +       if (!vi) {
>> > +               err = -ENOMEM;
>> > +               goto out1;
>> > +       }
>> > +       vdev->priv = vi;
>> > +       vi->vdev = vdev;
>> > +
>> > +       err = virtinput_init_vqs(vi);
>> > +       if (err)
>> > +               goto out2;
>> > +
>> > +       vi->idev = input_allocate_device();
>> > +       if (!vi->idev) {
>> > +               err = -ENOMEM;
>> > +               goto out3;
>> > +       }
>> > +       input_set_drvdata(vi->idev, vi);
>> > +
>> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
>> > +       virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
>> > +                          vi->name, min(size, sizeof(vi->name)));
>> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_SERIAL, 0);
>> > +       virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config, u),
>> > +                          vi->serial, min(size, sizeof(vi->serial)));
>> > +       snprintf(vi->phys, sizeof(vi->phys),
>> > +                "virtio%d/input0", vdev->index);
>> > +
>> > +       virtinput_cfg_bits(vi, VIRTIO_INPUT_CFG_PROP_BITS, 0,
>> > +                          vi->idev->propbit, INPUT_PROP_CNT);
>> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_EV_BITS, EV_REP);
>> > +       if (size)
>> > +               set_bit(EV_REP, vi->idev->evbit);
>> > +
>> > +       vi->idev->name = vi->name;
>> > +       vi->idev->phys = vi->phys;
>>
>> Can you set vi->idev->uniq to the virtio-bus path?
>
> No, uniq can't be phys as phys is unique within the system while uniq is
> like serial number or UUID and should never repeat.

...sorry, my bad! We should still forward it from the host, imo. It's
really handy for applications to re-detect devices.

Thanks
David

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > > +static ssize_t serial_show(struct device *dev,
> > > +                          struct device_attribute *attr, char *buf)
> > > +{
> > > +       struct input_dev *idev = to_input_dev(dev);
> > > +       struct virtio_input *vi = input_get_drvdata(idev);
> > > +       return sprintf(buf, "%s\n", vi->serial);
> > > +}
> > > +static DEVICE_ATTR_RO(serial);
> 
> What is serial? Serial number?

Yes.  You can (optionally) configure a serial number on the host side,
and if that is the case it'll show up here.

> > Can you set vi->idev->uniq to the virtio-bus path?
> 
> No, uniq can't be phys as phys is unique within the system while uniq is
> like serial number or UUID and should never repeat.

Ok, so I guess I should just fill uniq with serial (if present).

cheers,
  Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[David Herrmann]

Hi

On Fri, Mar 20, 2015 at 10:48 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
>> > +static int virtinput_send_status(struct virtio_input *vi,
>> > +                                u16 type, u16 code, s32 value)
>> > +{
>> > +       struct virtio_input_event *stsbuf;
>> > +       struct scatterlist sg[1];
>> > +
>> > +       stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
>> > +       if (!stsbuf)
>> > +               return -ENOMEM;
>> > +
>> > +       stsbuf->type  = cpu_to_le16(type);
>> > +       stsbuf->code  = cpu_to_le16(code);
>> > +       stsbuf->value = cpu_to_le32(value);
>> > +       sg_init_one(sg, stsbuf, sizeof(*stsbuf));
>> > +       virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
>> > +       virtqueue_kick(vi->sts);
>>
>> GFP_ATOMIC, eww. But everyone does that for input_event() callbacks..
>
> Yea, did it this way because I saw it elsewhere.
>
>> we should fix that for user-space input one day.
>
> Sounds like I have to use GFP_ATOMIC and can't switch to GFP_KERNEL,
> correct?

You cannot use GFP_KERNEL in this context, correct. GFP_ATOMIC is also
what all HID backends do, so it's fine here.

>> > +       size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
>> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
>> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
>> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
>> > +       virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
>>
>> abs.resolution is missing. Please add it, we really also need to add
>> it to uinput one day.
>
> Ok.  How should I handle cases where the resolution is either not known
> or not fixed?  Just leave it zero?

Leave it 0, which is what you already do by not assigning it.

>> > +       vi->idev->name = vi->name;
>> > +       vi->idev->phys = vi->phys;
>>
>> Can you set vi->idev->uniq to the virtio-bus path?
>>
>> > +       vi->idev->id.bustype = BUS_VIRTUAL;
>> > +       vi->idev->id.vendor  = 0x0001;
>> > +       vi->idev->id.product = 0x0001;
>> > +       vi->idev->id.version = 0x0100;
>>
>> Please don't hardcode those. All user-space based interaction with
>> input-devices relies on those IDs. Can we retrieve it from the host
>> just like the name?
>
> Yes, we can.
>
> There will be emulated devices, i.e. the input coming from
> vnc/gtk/whatever will be sent to the virtio devices (instead of ps/2 or
> usb).  For these we should probably have fixed IDs per device.  There
> are keyboard/mouse/tablet at the moment.  Suggestions how to pick IDs?
>
> There will also be pass-through support, i.e. qemu
> opening /dev/input/event<nr> and forwarding everything to the guest.
> How should that be handled best?  Copy all four from the host?  Even
> though the bustype is BUS_USB?  Not sure this actually improves things
> because the guest can match the device, or whenever this confuses apps
> due to BUS_USB being applied to virtio devices ...

Lemme give an example: We have databases in user-space, that allow
applications to figure out the mouse DPI values of a device. Those
databases match on all four, bus+vid+pid+ver (sometimes even more,
like name and dmi). If one of those is not forwarded, it will not be
detected.

I'd like to see all four forwarded from the host. I'd be fine with
"bus" being set to VIRTUAL, but I'm not sure why that would be a good
thing to do?

Thanks
David

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > There will also be pass-through support, i.e. qemu
> > opening /dev/input/event<nr> and forwarding everything to the guest.
> > How should that be handled best?  Copy all four from the host?  Even
> > though the bustype is BUS_USB?  Not sure this actually improves things
> > because the guest can match the device, or whenever this confuses apps
> > due to BUS_USB being applied to virtio devices ...
> 
> Lemme give an example: We have databases in user-space, that allow
> applications to figure out the mouse DPI values of a device. Those
> databases match on all four, bus+vid+pid+ver (sometimes even more,
> like name and dmi). If one of those is not forwarded, it will not be
> detected.

Ok, so forward as much as possible.

> I'd like to see all four forwarded from the host. I'd be fine with
> "bus" being set to VIRTUAL, but I'm not sure why that would be a good
> thing to do?

I think for the emulated devices it's fine to use VIRTUAL.

For the passthrough case suspected we could confuse apps because ->phys
points to a virtio device whereas ->type says "I'm usb".

But at least the device database probably doesn't care much about the
physical path I guess, because the mouse is the same no matter which usb
port I plug it in, correct?

cheers,
  Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[David Herrmann]

Hi

On Fri, Mar 20, 2015 at 11:36 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> I'd like to see all four forwarded from the host. I'd be fine with
>> "bus" being set to VIRTUAL, but I'm not sure why that would be a good
>> thing to do?
>
> I think for the emulated devices it's fine to use VIRTUAL.

Yes, on the host side just use BUS_VIRTUAL if you don't have a real bus to set.

> For the passthrough case suspected we could confuse apps because ->phys
> points to a virtio device whereas ->type says "I'm usb".

That's not an issue. The "phys" field hasn't been standardized in any
way that I'm aware of (except on a per-driver basis, maybe).

> But at least the device database probably doesn't care much about the
> physical path I guess, because the mouse is the same no matter which usb
> port I plug it in, correct?

Correct!

Thanks
David

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > +static int virtinput_send_status(struct virtio_input *vi,
> > +				 u16 type, u16 code, s32 value)
> > +{
> > +	struct virtio_input_event *stsbuf;
> > +	struct scatterlist sg[1];
> > +
> > +	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > +	if (!stsbuf)
> > +		return -ENOMEM;
> 
> Does this return an error to userspace?
> If so it's not a good idea I think, GFP_ATOMIC failures are
> transient conditions and should not be reported
> to userspace.
> Can use GFP_KERNEL here?

Waiting for an answer from the ioput guys here.

> > +
> > +	stsbuf->type  = cpu_to_le16(type);
> > +	stsbuf->code  = cpu_to_le16(code);
> > +	stsbuf->value = cpu_to_le32(value);
> > +	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > +	virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> 
> This can fail if queue is full. What prevents this from happening?

Nothing.  It's highly unlikely though given the throughput we have for
input devices, not sure it is that useful to put too much effort into
this.  Except for freeing stsbuf in the error case.

> > +	virtqueue_kick(vi->sts);
> 
> Also what prevents multiple virtinput_send_status calls
> from racing with each other? Is there locking at a higher level?

I think input_dev->event_lock does this.

> > +static void virtinput_recv_status(struct virtqueue *vq)
> > +{
> > +	struct virtio_input *vi = vq->vdev->priv;
> > +	struct virtio_input_event *stsbuf;
> > +	unsigned int len;
> > +
> > +	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> 
> looks like this get_buf can race with add above.

Yes, it can.

> Need some locking.

I'll add it.

> Also pls avoid != NULL, removing it makes code less verbose.
> 
> > +		kfree(stsbuf);
> > +	virtqueue_kick(vq);
> 
> Why are you kicking here?

Hmm, it is pointless indeed.

> > +	if (select == VIRTIO_INPUT_CFG_EV_BITS)
> > +		set_bit(subsel, vi->idev->evbit);
> > +	for (bit = 0; bit < bitcount; bit++) {
> > +		if ((bit % 8) == 0)
> > +			virtio_cread(vi->vdev, struct virtio_input_config,
> > +				     u.bitmap[bit/8], &cfg);
> 
> coding style violations above. you need spaces around ops like / and *.
> Please run checkpatch.pl
> 
> > +		if (cfg & (1 << (bit % 8)))
> > +			set_bit(bit, bits);
> 
> what if not set? does something clear the mask?

kzalloc?

> > +	}
> 
> doesn't above just implement bitmap_copy or bitmap_or?

Not fully sure how bitmaps are defined.  virtio has a stream of bytes,
first byte carries bits 0-7, second 8-15 etc.  linux kernel bitmaps ops
are operating on longs, and native byteorder longs would be something
else ...

> > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> 
> you read le field into u32 value.
> Please run sparse on this code. you will get a ton
> of warnings. Same error appears elsewhere.

Indeed.  IIRC that wasn't the case a while back.  Guess those bitwise
annotations have been added with the virtio 1.0 patches?

In any case I'll fix it up.

> > +static int virtinput_probe(struct virtio_device *vdev)
> > +{
> > +	struct virtio_input *vi;
> > +	size_t size;
> > +	int abs, err;
> 
> How about checking VERSION_1 and bailing out of not there?

I don't think this is needed.  There isn't a hard dependency on virtio
1.0.  It's just that config space is relatively large and because of
that I want it be 1.0 on the host (qemu) side to not allocate large
portions of I/O address space for the legacy virtio pci bar.

> > +	vi->idev->name = vi->name;
> > +	vi->idev->phys = vi->phys;
> > +	vi->idev->id.bustype = BUS_VIRTUAL;
> > +	vi->idev->id.vendor  = 0x0001;
> > +	vi->idev->id.product = 0x0001;
> > +	vi->idev->id.version = 0x0100;
> 
> Add comments explaining why these #s make sense?

See other subthread, will be changed to be host-provided (like name).

> > +	err = input_register_device(vi->idev);
> 
> Once you do this, virtinput_status can get called,
> and that will kick, correct?

Correct.

> If so, you must call device_ready before this.

Ok.

> > +	if (err)
> > +		goto out4;
> > +
> > +	return 0;
> > +
> > +out4:
> > +	input_free_device(vi->idev);
> > +out3:
> > +	vdev->config->del_vqs(vdev);
> > +out2:
> > +	kfree(vi);
> > +out1:
> > +	return err;
> 
> free on error is out of order with initialization.
> Might lead to leaks or other bugs.
> Also - can you name labels something sensible pls?
> out is usually for exiting on success too...
> E.g. out4 -> err_register etc.

Will fix.

> > +static void virtinput_remove(struct virtio_device *vdev)
> > +{
> > +	struct virtio_input *vi = vdev->priv;
> > +
> > +	input_unregister_device(vi->idev);
> > +	vdev->config->reset(vdev);
> 
> You don't really need a reset if you just to del_vqs.
> People do this if they want to prevent interrupts
> without deleting vqs.

Ok.

> > +	vdev->config->del_vqs(vdev);
> > +	kfree(vi);
> 
> free_device seems to be missing?

Indeed, good catch.

thanks,
  Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Fri, Mar 20, 2015 at 11:28:47AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +static int virtinput_send_status(struct virtio_input *vi,
> > > +				 u16 type, u16 code, s32 value)
> > > +{
> > > +	struct virtio_input_event *stsbuf;
> > > +	struct scatterlist sg[1];
> > > +
> > > +	stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > > +	if (!stsbuf)
> > > +		return -ENOMEM;
> > 
> > Does this return an error to userspace?
> > If so it's not a good idea I think, GFP_ATOMIC failures are
> > transient conditions and should not be reported
> > to userspace.
> > Can use GFP_KERNEL here?
> 
> Waiting for an answer from the ioput guys here.
> 
> > > +
> > > +	stsbuf->type  = cpu_to_le16(type);
> > > +	stsbuf->code  = cpu_to_le16(code);
> > > +	stsbuf->value = cpu_to_le32(value);
> > > +	sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > > +	virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> > 
> > This can fail if queue is full. What prevents this from happening?
> 
> Nothing.  It's highly unlikely though given the throughput we have for
> input devices, not sure it is that useful to put too much effort into
> this.  Except for freeing stsbuf in the error case.
> 
> > > +	virtqueue_kick(vi->sts);
> > 
> > Also what prevents multiple virtinput_send_status calls
> > from racing with each other? Is there locking at a higher level?
> 
> I think input_dev->event_lock does this.
> 
> > > +static void virtinput_recv_status(struct virtqueue *vq)
> > > +{
> > > +	struct virtio_input *vi = vq->vdev->priv;
> > > +	struct virtio_input_event *stsbuf;
> > > +	unsigned int len;
> > > +
> > > +	while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> > 
> > looks like this get_buf can race with add above.
> 
> Yes, it can.
> 
> > Need some locking.
> 
> I'll add it.
> 
> > Also pls avoid != NULL, removing it makes code less verbose.
> > 
> > > +		kfree(stsbuf);
> > > +	virtqueue_kick(vq);
> > 
> > Why are you kicking here?
> 
> Hmm, it is pointless indeed.
> 
> > > +	if (select == VIRTIO_INPUT_CFG_EV_BITS)
> > > +		set_bit(subsel, vi->idev->evbit);
> > > +	for (bit = 0; bit < bitcount; bit++) {
> > > +		if ((bit % 8) == 0)
> > > +			virtio_cread(vi->vdev, struct virtio_input_config,
> > > +				     u.bitmap[bit/8], &cfg);
> > 
> > coding style violations above. you need spaces around ops like / and *.
> > Please run checkpatch.pl
> > 
> > > +		if (cfg & (1 << (bit % 8)))
> > > +			set_bit(bit, bits);
> > 
> > what if not set? does something clear the mask?
> 
> kzalloc?

So you are really just reading in array of bytes?
All this set bit trickery is just to convert things from LE?

> > > +	}
> > 
> > doesn't above just implement bitmap_copy or bitmap_or?
> 
> Not fully sure how bitmaps are defined.  virtio has a stream of bytes,
> first byte carries bits 0-7, second 8-15 etc.  linux kernel bitmaps ops
> are operating on longs, and native byteorder longs would be something
> else ...

This still looks too complex.
At least, this needs a comment explaining what the function does,
and maybe wrap it in a helper like virtio_input_bitmap_copy or
virtio_bitmap_or.


> > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > 
> > you read le field into u32 value.
> > Please run sparse on this code. you will get a ton
> > of warnings. Same error appears elsewhere.
> 
> Indeed.  IIRC that wasn't the case a while back.  Guess those bitwise
> annotations have been added with the virtio 1.0 patches?
> 
> In any case I'll fix it up.

I see you still didn't in v2?

> > > +static int virtinput_probe(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_input *vi;
> > > +	size_t size;
> > > +	int abs, err;
> > 
> > How about checking VERSION_1 and bailing out of not there?
> 
> I don't think this is needed.  There isn't a hard dependency on virtio
> 1.0.  It's just that config space is relatively large and because of
> that I want it be 1.0 on the host (qemu) side to not allocate large
> portions of I/O address space for the legacy virtio pci bar.

You are doing leXXX everywhere, that's VERSION_1 dependency.
virtio_cread will do byteswaps differently without VERSION_1.
Just don't go there.

> > > +	vi->idev->name = vi->name;
> > > +	vi->idev->phys = vi->phys;
> > > +	vi->idev->id.bustype = BUS_VIRTUAL;
> > > +	vi->idev->id.vendor  = 0x0001;
> > > +	vi->idev->id.product = 0x0001;
> > > +	vi->idev->id.version = 0x0100;
> > 
> > Add comments explaining why these #s make sense?
> 
> See other subthread, will be changed to be host-provided (like name).
> 
> > > +	err = input_register_device(vi->idev);
> > 
> > Once you do this, virtinput_status can get called,
> > and that will kick, correct?
> 
> Correct.
> 
> > If so, you must call device_ready before this.
> 
> Ok.
> 
> > > +	if (err)
> > > +		goto out4;
> > > +
> > > +	return 0;
> > > +
> > > +out4:
> > > +	input_free_device(vi->idev);
> > > +out3:
> > > +	vdev->config->del_vqs(vdev);
> > > +out2:
> > > +	kfree(vi);
> > > +out1:
> > > +	return err;
> > 
> > free on error is out of order with initialization.
> > Might lead to leaks or other bugs.
> > Also - can you name labels something sensible pls?
> > out is usually for exiting on success too...
> > E.g. out4 -> err_register etc.
> 
> Will fix.
> 
> > > +static void virtinput_remove(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_input *vi = vdev->priv;
> > > +
> > > +	input_unregister_device(vi->idev);
> > > +	vdev->config->reset(vdev);
> > 
> > You don't really need a reset if you just to del_vqs.
> > People do this if they want to prevent interrupts
> > without deleting vqs.
> 
> Ok.
> 
> > > +	vdev->config->del_vqs(vdev);
> > > +	kfree(vi);
> > 
> > free_device seems to be missing?
> 
> Indeed, good catch.
> 
> thanks,
>   Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > > > +		if (cfg & (1 << (bit % 8)))
> > > > +			set_bit(bit, bits);
> > > 
> > > what if not set? does something clear the mask?
> > 
> > kzalloc?
> 
> So you are really just reading in array of bytes?
> All this set bit trickery is just to convert things from LE?

Trickery?  Just checking each bit from virtio config space, then set it
in the input layer bitmap.  It's a simple stupid loop.

Surely not the most efficient way, but hey, it's not in the hot path and
I'm sure I'm setting the bits correctly because this uses the standard
linux kernel bitops.

> At least, this needs a comment explaining what the function does,
> and maybe wrap it in a helper like virtio_input_bitmap_copy or
> virtio_bitmap_or.

Can do that, sure.

> > > > +	size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > > > +	virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> > > 
> > > you read le field into u32 value.
> > > Please run sparse on this code. you will get a ton
> > > of warnings. Same error appears elsewhere.
> > 
> > Indeed.  IIRC that wasn't the case a while back.  Guess those bitwise
> > annotations have been added with the virtio 1.0 patches?
> > 
> > In any case I'll fix it up.
> 
> I see you still didn't in v2?

v2 builds fine without sparse warnings.  virtio_cread handles swapping
if needed and returns native endian, so I have to store this in normal
u32 variables and pass it on to the input layer as-is.

> You are doing leXXX everywhere, that's VERSION_1 dependency.
> virtio_cread will do byteswaps differently without VERSION_1.
> Just don't go there.

Changed that for v2, for the config space structs.  They have normal u32
in there now.  virtio_cread() wants it this way.

cheers,
  Gerd

Re: [virtio-dev] Re: [PATCH 1/1] Add virtio-input driver.

[Paolo Bonzini]

On 23/03/2015 08:53, Gerd Hoffmann wrote:
>>>>> > > > > +		if (cfg & (1 << (bit % 8)))
>>>>> > > > > +			set_bit(bit, bits);
>>>> > > > 
>>>> > > > what if not set? does something clear the mask?
>>> > > 
>>> > > kzalloc?
>> > 
>> > So you are really just reading in array of bytes?
>> > All this set bit trickery is just to convert things from LE?
> Trickery?  Just checking each bit from virtio config space, then set it
> in the input layer bitmap.  It's a simple stupid loop.
> 
> Surely not the most efficient way, but hey, it's not in the hot path and
> I'm sure I'm setting the bits correctly because this uses the standard
> linux kernel bitops.

Use __set_bit though, because set_bit is an atomic operation.

Paolo

>> > At least, this needs a comment explaining what the function does,
>> > and maybe wrap it in a helper like virtio_input_bitmap_copy or
>> > virtio_bitmap_or.
> Can do that, sure.

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > At least, this needs a comment explaining what the function does,
> > and maybe wrap it in a helper like virtio_input_bitmap_copy or
> > virtio_bitmap_or.
> 
> Can do that, sure.

Well, the function where this is in already cares about the bitmap copy
only.  Can add a comment though.

> > You are doing leXXX everywhere, that's VERSION_1 dependency.
> > virtio_cread will do byteswaps differently without VERSION_1.
> > Just don't go there.
> 
> Changed that for v2, for the config space structs.  They have normal u32
> in there now.  virtio_cread() wants it this way.

I liked the __le32 in the config space structs more though, so I've
waded through the virtio_config.h header file.

To me it looks like we need separate virtio_cread() versions for
non-transitional drivers, which do __le32 -> u32 translation instead of
__virtio32 -> u32 translation, so I can have __le32 types in the config
space structs.

Or I could use vdev->config->get() directly instead of virtio_cread, but
I'll loose sparse checking that way.

Hmm.  Recommendations?  Better ideas?

cheers,
  Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Mon, Mar 23, 2015 at 02:44:52PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > At least, this needs a comment explaining what the function does,
> > > and maybe wrap it in a helper like virtio_input_bitmap_copy or
> > > virtio_bitmap_or.
> > 
> > Can do that, sure.
> 
> Well, the function where this is in already cares about the bitmap copy
> only.  Can add a comment though.

OK, I think that will be enough for now.

> > > You are doing leXXX everywhere, that's VERSION_1 dependency.
> > > virtio_cread will do byteswaps differently without VERSION_1.
> > > Just don't go there.
> > 
> > Changed that for v2, for the config space structs.  They have normal u32
> > in there now.  virtio_cread() wants it this way.
> 
> I liked the __le32 in the config space structs more though, so I've
> waded through the virtio_config.h header file.
> 
> To me it looks like we need separate virtio_cread() versions for
> non-transitional drivers, which do __le32 -> u32 translation instead of
> __virtio32 -> u32 translation, so I can have __le32 types in the config
> space structs.
> 
> Or I could use vdev->config->get() directly instead of virtio_cread, but
> I'll loose sparse checking that way.
> 
> Hmm.  Recommendations?  Better ideas?
> 
> cheers,
>   Gerd

So to clarify, you dislike using __virtio32 in virtio input header?

-- 
MST

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

  Hi,

> > > > You are doing leXXX everywhere, that's VERSION_1 dependency.
> > > > virtio_cread will do byteswaps differently without VERSION_1.
> > > > Just don't go there.

> So to clarify, you dislike using __virtio32 in virtio input header?

Well, as I understand things __virtio32 implies byteorder depends on
whenever we are using VERSION_1 or not.  And non-transitional drivers
should not need it as everything is by definition little endian.

So, yes, your suggestion to just require VERSION_1 in the driver implies
in my eyes that there should be no reason to use __virtio32 instead of
__le32.

Or do I miss something here?

cheers,
  Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Mon, Mar 23, 2015 at 03:27:35PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > > > You are doing leXXX everywhere, that's VERSION_1 dependency.
> > > > > virtio_cread will do byteswaps differently without VERSION_1.
> > > > > Just don't go there.
> 
> > So to clarify, you dislike using __virtio32 in virtio input header?
> 
> Well, as I understand things __virtio32 implies byteorder depends on
> whenever we are using VERSION_1 or not.  And non-transitional drivers
> should not need it as everything is by definition little endian.
> 
> So, yes, your suggestion to just require VERSION_1 in the driver implies
> in my eyes that there should be no reason to use __virtio32 instead of
> __le32.
> 
> Or do I miss something here?
> 
> cheers,
>   Gerd
> 

You are right but then if you do require VERSION_1 then
__virtio32 becomes identical to __le32.
There's some runtime overhead as we check on each access,
but it shouldn't matter here, right?
I guess we could add virtio_cread_le - is this what
you'd like?


-- 
MST

Re: [PATCH 1/1] Add virtio-input driver.

[Gerd Hoffmann]

On Mo, 2015-03-23 at 15:54 +0100, Michael S. Tsirkin wrote:
> On Mon, Mar 23, 2015 at 03:27:35PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > > > You are doing leXXX everywhere, that's VERSION_1 dependency.
> > > > > > virtio_cread will do byteswaps differently without VERSION_1.
> > > > > > Just don't go there.
> > 
> > > So to clarify, you dislike using __virtio32 in virtio input header?
> > 
> > Well, as I understand things __virtio32 implies byteorder depends on
> > whenever we are using VERSION_1 or not.  And non-transitional drivers
> > should not need it as everything is by definition little endian.
> > 
> > So, yes, your suggestion to just require VERSION_1 in the driver implies
> > in my eyes that there should be no reason to use __virtio32 instead of
> > __le32.
> > 
> > Or do I miss something here?
> > 
> > cheers,
> >   Gerd
> > 
> 
> You are right but then if you do require VERSION_1 then
> __virtio32 becomes identical to __le32.

Except that sparse doesn't know that and throws errors when I mix the
two.

> There's some runtime overhead as we check on each access,
> but it shouldn't matter here, right?

Correct, config space is used at initialization time only.

> I guess we could add virtio_cread_le - is this what
> you'd like?

I just want something that makes both you and sparse happy.  I don't
care much whenever that is adding virtio_cread_le() or using __virtio32
even though it'll effectively is __le32 due to VERSION_1 being required.

cheers,
  Gerd

Re: [PATCH 1/1] Add virtio-input driver.

[Cornelia Huck]

On Mon, 23 Mar 2015 16:05:29 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mo, 2015-03-23 at 15:54 +0100, Michael S. Tsirkin wrote:
> > On Mon, Mar 23, 2015 at 03:27:35PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > > > You are doing leXXX everywhere, that's VERSION_1 dependency.
> > > > > > > virtio_cread will do byteswaps differently without VERSION_1.
> > > > > > > Just don't go there.
> > > 
> > > > So to clarify, you dislike using __virtio32 in virtio input header?
> > > 
> > > Well, as I understand things __virtio32 implies byteorder depends on
> > > whenever we are using VERSION_1 or not.  And non-transitional drivers
> > > should not need it as everything is by definition little endian.
> > > 
> > > So, yes, your suggestion to just require VERSION_1 in the driver implies
> > > in my eyes that there should be no reason to use __virtio32 instead of
> > > __le32.
> > > 
> > > Or do I miss something here?
> > > 
> > > cheers,
> > >   Gerd
> > > 
> > 
> > You are right but then if you do require VERSION_1 then
> > __virtio32 becomes identical to __le32.
> 
> Except that sparse doesn't know that and throws errors when I mix the
> two.
> 
> > There's some runtime overhead as we check on each access,
> > but it shouldn't matter here, right?
> 
> Correct, config space is used at initialization time only.
> 
> > I guess we could add virtio_cread_le - is this what
> > you'd like?
> 
> I just want something that makes both you and sparse happy.  I don't
> care much whenever that is adding virtio_cread_le() or using __virtio32
> even though it'll effectively is __le32 due to VERSION_1 being required.

I think it is clearer if you use __virtio32: The fact that virtio-input
is VERSION_1 only is not apparent from looking at the config space
definition, and explicitly forcing le accesses makes it look similar to
the oddballs like virtio-balloon. Just my two cents.

Re: [PATCH 1/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Mon, Mar 23, 2015 at 04:05:29PM +0100, Gerd Hoffmann wrote:
> On Mo, 2015-03-23 at 15:54 +0100, Michael S. Tsirkin wrote:
> > On Mon, Mar 23, 2015 at 03:27:35PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > > > You are doing leXXX everywhere, that's VERSION_1 dependency.
> > > > > > > virtio_cread will do byteswaps differently without VERSION_1.
> > > > > > > Just don't go there.
> > > 
> > > > So to clarify, you dislike using __virtio32 in virtio input header?
> > > 
> > > Well, as I understand things __virtio32 implies byteorder depends on
> > > whenever we are using VERSION_1 or not.  And non-transitional drivers
> > > should not need it as everything is by definition little endian.
> > > 
> > > So, yes, your suggestion to just require VERSION_1 in the driver implies
> > > in my eyes that there should be no reason to use __virtio32 instead of
> > > __le32.
> > > 
> > > Or do I miss something here?
> > > 
> > > cheers,
> > >   Gerd
> > > 
> > 
> > You are right but then if you do require VERSION_1 then
> > __virtio32 becomes identical to __le32.
> 
> Except that sparse doesn't know that and throws errors when I mix the
> two.
> 
> > There's some runtime overhead as we check on each access,
> > but it shouldn't matter here, right?
> 
> Correct, config space is used at initialization time only.
> 
> > I guess we could add virtio_cread_le - is this what
> > you'd like?
> 
> I just want something that makes both you and sparse happy.  I don't
> care much whenever that is adding virtio_cread_le() or using __virtio32
> even though it'll effectively is __le32 due to VERSION_1 being required.
> 
> cheers,
>   Gerd
> 

OK so how about we just use __virtio32 everywhere for now?

-- 
MST

Re: [PATCH 0/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 10:13:10AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> This patch adds a virtio driver for input devices.
> 
> Specification:
>   https://www.kraxel.org/cgit/virtio-spec/log/?h=virtio-input
>   https://www.kraxel.org/virtio/virtio-v1.0-csprd03-virtio-input.html#x1-2640007


OK, I don't know which thread should I use for spec discussions.
Referring to that:

	"See file:///usr/include/linux/input.h."

Is likely not present on many systems, or might not include
the info you refer to.

 "type, code and value are filled according to the linux input layer
 (evdev) interface"

Which version?  How will non-linux guests know what to implement?


> 
> Qemu patches;
>   https://lists.gnu.org/archive/html/qemu-devel/2015-03/threads.html#03973
> 
> Gerd Hoffmann (1):
>   Add virtio-input driver.
> 
>  drivers/virtio/Kconfig            |  10 ++
>  drivers/virtio/Makefile           |   1 +
>  drivers/virtio/virtio_input.c     | 313 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/virtio_ids.h   |   1 +
>  include/uapi/linux/virtio_input.h |  65 ++++++++
>  5 files changed, 390 insertions(+)
>  create mode 100644 drivers/virtio/virtio_input.c
>  create mode 100644 include/uapi/linux/virtio_input.h
> 
> -- 
> 1.8.3.1

Re: [PATCH 0/1] Add virtio-input driver.

[Gerd Hoffmann]

On Do, 2015-03-19 at 14:35 +0100, Michael S. Tsirkin wrote:
> On Thu, Mar 19, 2015 at 10:13:10AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > This patch adds a virtio driver for input devices.
> > 
> > Specification:
> >   https://www.kraxel.org/cgit/virtio-spec/log/?h=virtio-input
> >   https://www.kraxel.org/virtio/virtio-v1.0-csprd03-virtio-input.html#x1-2640007
> 
> 
> OK, I don't know which thread should I use for spec discussions.
> Referring to that:
> 
> 	"See file:///usr/include/linux/input.h."
> 
> Is likely not present on many systems, or might not include
> the info you refer to.

Dunno what the best way to deal with it is.  Link to the version online
@ kernel.org instead maybe?

>  "type, code and value are filled according to the linux input layer
>  (evdev) interface"
> 
> Which version?

Latest.  As far I know there never ever have been incompatible changes
to the interface, and given this is userspace/kernel abi I don't expect
that to happen in the future.

>   How will non-linux guests know what to implement?

There are some docs on the linux input layer and evdev events in
Documentation/input/ in the kernel tree.

cheers,
  Gerd

Re: [PATCH 0/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 03:46:44PM +0100, Gerd Hoffmann wrote:
> On Do, 2015-03-19 at 14:35 +0100, Michael S. Tsirkin wrote:
> > On Thu, Mar 19, 2015 at 10:13:10AM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > This patch adds a virtio driver for input devices.
> > > 
> > > Specification:
> > >   https://www.kraxel.org/cgit/virtio-spec/log/?h=virtio-input
> > >   https://www.kraxel.org/virtio/virtio-v1.0-csprd03-virtio-input.html#x1-2640007
> > 
> > 
> > OK, I don't know which thread should I use for spec discussions.
> > Referring to that:
> > 
> > 	"See file:///usr/include/linux/input.h."
> > 
> > Is likely not present on many systems, or might not include
> > the info you refer to.
> 
> Dunno what the best way to deal with it is.  Link to the version online
> @ kernel.org instead maybe?
> 
> >  "type, code and value are filled according to the linux input layer
> >  (evdev) interface"
> > 
> > Which version?
> 
> Latest.  As far I know there never ever have been incompatible changes
> to the interface, and given this is userspace/kernel abi I don't expect
> that to happen in the future.

More events are added though, are they not? And distros backport rundom
subsets.
So I worry: what happens e.g. if you migrate between hosts which expose
slightly different subsets of events?
Might e.g. a button get stuck because button-press event was
sent but button-release wasn't?


> >   How will non-linux guests know what to implement?
> 
> There are some docs on the linux input layer and evdev events in
> Documentation/input/ in the kernel tree.
> 
> cheers,
>   Gerd



-- 
MST

Re: [PATCH 0/1] Add virtio-input driver.

[Paolo Bonzini]

On 19/03/2015 17:33, Michael S. Tsirkin wrote:
> On Thu, Mar 19, 2015 at 03:46:44PM +0100, Gerd Hoffmann wrote:
>> Latest.  As far I know there never ever have been incompatible changes
>> to the interface, and given this is userspace/kernel abi I don't expect
>> that to happen in the future.
> 
> More events are added though, are they not? And distros backport rundom
> subsets.
> So I worry: what happens e.g. if you migrate between hosts which expose
> slightly different subsets of events?
> Might e.g. a button get stuck because button-press event was
> sent but button-release wasn't?

I think this is the same as SCSI.  You can migrate between hosts which
expose slightly different command sets.

Paolo

Re: [PATCH 0/1] Add virtio-input driver.

[Michael S. Tsirkin]

On Thu, Mar 19, 2015 at 03:46:44PM +0100, Gerd Hoffmann wrote:
> On Do, 2015-03-19 at 14:35 +0100, Michael S. Tsirkin wrote:
> > On Thu, Mar 19, 2015 at 10:13:10AM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > This patch adds a virtio driver for input devices.
> > > 
> > > Specification:
> > >   https://www.kraxel.org/cgit/virtio-spec/log/?h=virtio-input
> > >   https://www.kraxel.org/virtio/virtio-v1.0-csprd03-virtio-input.html#x1-2640007
> > 
> > 
> > OK, I don't know which thread should I use for spec discussions.
> > Referring to that:
> > 
> > 	"See file:///usr/include/linux/input.h."
> > 
> > Is likely not present on many systems, or might not include
> > the info you refer to.
> 
> Dunno what the best way to deal with it is.  Link to the version online
> @ kernel.org instead maybe?
> 
> >  "type, code and value are filled according to the linux input layer
> >  (evdev) interface"
> > 
> > Which version?
> 
> Latest.  As far I know there never ever have been incompatible changes
> to the interface, and given this is userspace/kernel abi I don't expect
> that to happen in the future.
> 
> >   How will non-linux guests know what to implement?
> 
> There are some docs on the linux input layer and evdev events in
> Documentation/input/ in the kernel tree.
> 
> cheers,
>   Gerd


Also, the spec needs to be rewritten a bit more formally,
with conformance clauses separated from freetext description,
and linked to from appropriate section.

" motion events are send from the device" send->sent

-- 
MST