virtio implementation (draft VI)

This adds the logic to convert the virtio ids into module aliases, and includes a modalias entry in sysfs.

Unfortunately this does not seem sufficient to have the module autoprobed at startup on my Ubuntu system. Greg? Am I missing some udev magic?

Quoted 3 lines · Rusty Russell

This adds the logic to convert the virtio ids into module aliases, and includes a modalias entry in sysfs.

Quoted 3 lines

Unfortunately this does not seem sufficient to have the module autoprobed at startup on my Ubuntu system. Greg? Am I missing some udev magic?

You also need to pass the MODINFO environment variable to the hotplug call so that udev can pick it up.

hope this helps,

Quoted 11 lines · Greg KH

Quoted 6 lines · Rusty Russell

This adds the logic to convert the virtio ids into module aliases, and includes a modalias entry in sysfs.

Unfortunately this does not seem sufficient to have the module autoprobed at startup on my Ubuntu system. Greg? Am I missing some udev magic?

You also need to pass the MODINFO environment variable to the hotplug call so that udev can pick it up.

Quoted 1 line

hope this helps,

Well, I finally decoded this as follows:

You need to implement the bus_type.uevent hook, and use add_uevent_var() to add a "MODALIAS=virtio:..." var. See drivers/pci/hotplug.c.

Not sure why I need the modalias here when it's in sysfs. But, it works.

Quoted 2 lines · Rusty Russell

Not sure why I need the modalias here when it's in sysfs. But, it works.

udev does not read from sysfs anymore, it just uses the environment variables. It's faster and race-free that way.

Quoted 6 lines · Greg KH

Quoted 2 lines · Rusty Russell

Not sure why I need the modalias here when it's in sysfs. But, it works.

udev does not read from sysfs anymore, it just uses the environment variables. It's faster and race-free that way.

Should I leave the modalias entry in sysfs for back compat, or not? This will be for >= 2.6.24 only...

Quoted 6 lines · Greg KH

Quoted 2 lines · Rusty Russell

Not sure why I need the modalias here when it's in sysfs. But, it works.

udev does not read from sysfs anymore, it just uses the environment variables. It's faster and race-free that way.

Should I leave the modalias entry in sysfs for back compat, or not? This will be for >= 2.6.24 only...

Quoted 2 lines · Rusty Russell

Not sure why I need the modalias here when it's in sysfs. But, it works.

udev does not read from sysfs anymore, it just uses the environment variables. It's faster and race-free that way.

Quoted 11 lines · Greg KH

Quoted 6 lines · Rusty Russell

This adds the logic to convert the virtio ids into module aliases, and includes a modalias entry in sysfs.

Unfortunately this does not seem sufficient to have the module autoprobed at startup on my Ubuntu system. Greg? Am I missing some udev magic?

You also need to pass the MODINFO environment variable to the hotplug call so that udev can pick it up.

Quoted 1 line

hope this helps,

Well, I finally decoded this as follows:

You need to implement the bus_type.uevent hook, and use add_uevent_var() to add a "MODALIAS=virtio:..." var. See drivers/pci/hotplug.c.

Not sure why I need the modalias here when it's in sysfs. But, it works.

Quoted 3 lines · Rusty Russell

This adds the logic to convert the virtio ids into module aliases, and includes a modalias entry in sysfs.

Quoted 3 lines

Unfortunately this does not seem sufficient to have the module autoprobed at startup on my Ubuntu system. Greg? Am I missing some udev magic?

You also need to pass the MODINFO environment variable to the hotplug call so that udev can pick it up.

hope this helps,

This adds the logic to convert the virtio ids into module aliases, and includes a modalias entry in sysfs.

Unfortunately this does not seem sufficient to have the module autoprobed at startup on my Ubuntu system. Greg? Am I missing some udev magic?

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

1) The rings are variable sized (2^n-1 elements).
2) They have an unfortunate limit of 65535 bytes per sg element.
3) The page numbers are always 64 bit (PAE anyone?)
4) They no longer place used[] on a separate page, just a separate cacheline.
5) We do a modulo on a variable. We could be tricky if we cared.
6) Interrupts and notifies are suppressed using flags within the rings.

Users need only implement the new_vq and free_vq hooks (KVM wants the guest to allocate the rings, lguest does it sanely).

Signed-off-by Rusty Russell <rusty-8n+1lVoiYb80n/[email protected]>

drivers/virtio/virtio_ring.c new file +16 -0

+irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+       struct vring_virtqueue *vq = to_vvq(_vq);
+
+       pr_debug("virtqueue interrupt for %p\n", vq);
+
+       if (unlikely(vq->broken))
+               return IRQ_HANDLED;
+
+       if (more_used(vq)) {
+               pr_debug("virtqueue callback for %p (%p)\n",
+                        vq, vq->vq.callback);
+               if (!vq->vq.callback)
+                       return IRQ_NONE;
+               if (!vq->vq.callback(&vq->vq))
+                       vq->vring.avail->flags |= 

VRING_AVAIL_F_NO_INTERRUPT;

Seems like there is a problem with shared irq line, the interrupt handler always returns IRQ_HANDLED (except for the trivial case were the callback is null).

It can be solved by having a host irq counter (in the shared ring) and a guest irq counter and return
mb(); return (host_counter!=guest_counter)? IRQ_HANDLED:IRQ_NONE;

Quoted 19 lines · Rusty Russell

drivers/virtio/virtio_ring.c new file +16 -0

+irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+       struct vring_virtqueue *vq = to_vvq(_vq);
+
+       pr_debug("virtqueue interrupt for %p\n", vq);
+
+       if (unlikely(vq->broken))
+               return IRQ_HANDLED;
+
+       if (more_used(vq)) {
+               pr_debug("virtqueue callback for %p (%p)\n",
+                        vq, vq->vq.callback);
+               if (!vq->vq.callback)
+                       return IRQ_NONE;
+               if (!vq->vq.callback(&vq->vq))
+                       vq->vring.avail->flags |=

VRING_AVAIL_F_NO_INTERRUPT;

drivers/virtio/virtio_ring.c new file +2 -0

+       } else
+               pr_debug("virtqueue %p no more used\n", vq);

drivers/virtio/virtio_ring.c line 265

Quoted 5 lines

drivers/virtio/virtio_ring.c new file +4 -0

+
+       return IRQ_HANDLED;
+}
+

Seems like there is a problem with shared irq line, the interrupt handler always returns IRQ_HANDLED (except for the trivial case were the callback is null).

Quoted 20 lines · Dor Laor

Quoted 19 lines · Rusty Russell

drivers/virtio/virtio_ring.c new file +16 -0

+irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+       struct vring_virtqueue *vq = to_vvq(_vq);
+
+       pr_debug("virtqueue interrupt for %p\n", vq);
+
+       if (unlikely(vq->broken))
+               return IRQ_HANDLED;
+
+       if (more_used(vq)) {
+               pr_debug("virtqueue callback for %p (%p)\n",
+                        vq, vq->vq.callback);
+               if (!vq->vq.callback)
+                       return IRQ_NONE;
+               if (!vq->vq.callback(&vq->vq))
+                       vq->vring.avail->flags |=

VRING_AVAIL_F_NO_INTERRUPT;

drivers/virtio/virtio_ring.c new file +2 -0

+       } else
+               pr_debug("virtqueue %p no more used\n", vq);

Quoted 8 lines

drivers/virtio/virtio_ring.c line 265

Quoted 5 lines

drivers/virtio/virtio_ring.c new file +4 -0

+
+       return IRQ_HANDLED;
+}
+

Seems like there is a problem with shared irq line, the interrupt handler always returns IRQ_HANDLED (except for the trivial case were the callback is null).

To reply for a second time, this time with thought. Sorry.

Yes, this code is wrong. It should be:

irqreturn_t vring_interrupt(int irq, void *_vq)

{
        struct vring_virtqueue *vq = to_vvq(_vq);

        if (!more_used(vq)) {
                pr_debug("virtqueue interrupt with no work for %p\n", vq);
                return IRQ_NONE;
        }

        if (unlikely(vq->broken))
                return IRQ_HANDLED;

        pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback);
        if (vq->vq.callback && !vq->vq.callback(&vq->vq))
                vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

        return IRQ_HANDLED;
}

Cheers,
Rusty.

At the moment it's not good enough, there is a potential race were the guest optimistically turn off
the VRING_AVAIL_F_NO_INTERRUPT in the vring_restart and afterwards finds there are more_used so
it consume them in the poll function.
If in the middle, packets arrive the host will see the flag is off and send irq.
In that case the above irq handler will report IRQ_NONE.

It's also not trivial to cancel the optimistic approach in the restart function since then there might be another race
that will result in missing irq reaching the guest.

I'll try to think of something better than a hypercall for switching irq on/off.

Quoted 9 lines · Dor Laor

At the moment it's not good enough, there is a potential race were the guest optimistically turn off
the VRING_AVAIL_F_NO_INTERRUPT in the vring_restart and afterwards finds there are more_used so
it consume them in the poll function.
If in the middle, packets arrive the host will see the flag is off and send irq.
In that case the above irq handler will report IRQ_NONE.

Good point. On the one hand, reporting IRQ_NONE occasionally is not fatal. On the other, it'd be nice to get this right.

Quoted 4 lines

It's also not trivial to cancel the optimistic approach in the restart function since then there might be another race
that will result in missing irq reaching the guest.

I did it optimistically because otherwise we need two barriers (and still have a window).

Quoted 3 lines

I'll try to think of something better than a hypercall for switching irq on/off.

Erk. That would be v. bad...

Quoted 8 lines · Dor Laor

At the moment it's not good enough, there is a potential race were the
guest optimistically turn off
the VRING_AVAIL_F_NO_INTERRUPT in the vring_restart and afterwards
finds there are more_used so
it consume them in the poll function. If in the middle, packets arrive the host will see the flag is off and send irq.
In that case the above irq handler will report IRQ_NONE.

Good point. On the one hand, reporting IRQ_NONE occasionally is not fatal. On the other, it'd be nice to get this right.

Quoted 4 lines

It's also not trivial to cancel the optimistic approach in the restart function since then there might be another race
that will result in missing irq reaching the guest.

I did it optimistically because otherwise we need two barriers (and still have a window).

Quoted 3 lines

I'll try to think of something better than a hypercall for switching irq on/off.

Erk. That would be v. bad...

At the moment it's not good enough, there is a potential race were the guest optimistically turn off
the VRING_AVAIL_F_NO_INTERRUPT in the vring_restart and afterwards finds there are more_used so
it consume them in the poll function.
If in the middle, packets arrive the host will see the flag is off and send irq.
In that case the above irq handler will report IRQ_NONE.

It's also not trivial to cancel the optimistic approach in the restart function since then there might be another race
that will result in missing irq reaching the guest.

I'll try to think of something better than a hypercall for switching irq on/off.

Quoted 20 lines · Dor Laor

Quoted 19 lines · Rusty Russell

drivers/virtio/virtio_ring.c new file +16 -0

+irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+       struct vring_virtqueue *vq = to_vvq(_vq);
+
+       pr_debug("virtqueue interrupt for %p\n", vq);
+
+       if (unlikely(vq->broken))
+               return IRQ_HANDLED;
+
+       if (more_used(vq)) {
+               pr_debug("virtqueue callback for %p (%p)\n",
+                        vq, vq->vq.callback);
+               if (!vq->vq.callback)
+                       return IRQ_NONE;
+               if (!vq->vq.callback(&vq->vq))
+                       vq->vring.avail->flags |=

VRING_AVAIL_F_NO_INTERRUPT;

drivers/virtio/virtio_ring.c new file +2 -0

+       } else
+               pr_debug("virtqueue %p no more used\n", vq);

Quoted 8 lines

drivers/virtio/virtio_ring.c line 265

Quoted 5 lines

drivers/virtio/virtio_ring.c new file +4 -0

+
+       return IRQ_HANDLED;
+}
+

Seems like there is a problem with shared irq line, the interrupt handler always returns IRQ_HANDLED (except for the trivial case were the callback is null).

To reply for a second time, this time with thought. Sorry.

Yes, this code is wrong. It should be:

irqreturn_t vring_interrupt(int irq, void *_vq)

{
        struct vring_virtqueue *vq = to_vvq(_vq);

        if (!more_used(vq)) {
                pr_debug("virtqueue interrupt with no work for %p\n", vq);
                return IRQ_NONE;
        }

        if (unlikely(vq->broken))
                return IRQ_HANDLED;

        pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback);
        if (vq->vq.callback && !vq->vq.callback(&vq->vq))
                vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

        return IRQ_HANDLED;
}

Cheers,
Rusty.

Seems like there is a problem with shared irq line, the interrupt handler always returns IRQ_HANDLED (except for the trivial case were the callback is null).

It can be solved by having a host irq counter (in the shared ring) and a guest irq counter and return
mb(); return (host_counter!=guest_counter)? IRQ_HANDLED:IRQ_NONE;

Quoted 19 lines · Rusty Russell

drivers/virtio/virtio_ring.c new file +16 -0

+irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+       struct vring_virtqueue *vq = to_vvq(_vq);
+
+       pr_debug("virtqueue interrupt for %p\n", vq);
+
+       if (unlikely(vq->broken))
+               return IRQ_HANDLED;
+
+       if (more_used(vq)) {
+               pr_debug("virtqueue callback for %p (%p)\n",
+                        vq, vq->vq.callback);
+               if (!vq->vq.callback)
+                       return IRQ_NONE;
+               if (!vq->vq.callback(&vq->vq))
+                       vq->vring.avail->flags |=

VRING_AVAIL_F_NO_INTERRUPT;

drivers/virtio/virtio_ring.c new file +2 -0

+       } else
+               pr_debug("virtqueue %p no more used\n", vq);

drivers/virtio/virtio_ring.c line 265

Quoted 5 lines

drivers/virtio/virtio_ring.c new file +4 -0

+
+       return IRQ_HANDLED;
+}
+

Seems like there is a problem with shared irq line, the interrupt handler always returns IRQ_HANDLED (except for the trivial case were the callback is null).

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

1) The rings are variable sized (2^n-1 elements).
2) They have an unfortunate limit of 65535 bytes per sg element.
3) The page numbers are always 64 bit (PAE anyone?)
4) They no longer place used[] on a separate page, just a separate cacheline.
5) We do a modulo on a variable. We could be tricky if we cared.
6) Interrupts and notifies are suppressed using flags within the rings.

Users need only implement the new_vq and free_vq hooks (KVM wants the guest to allocate the rings, lguest does it sanely).

Signed-off-by Rusty Russell <[email protected]>

drivers/virtio/virtio_ring.c new file +16 -0

+irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+       struct vring_virtqueue *vq = to_vvq(_vq);
+
+       pr_debug("virtqueue interrupt for %p\n", vq);
+
+       if (unlikely(vq->broken))
+               return IRQ_HANDLED;
+
+       if (more_used(vq)) {
+               pr_debug("virtqueue callback for %p (%p)\n",
+                        vq, vq->vq.callback);
+               if (!vq->vq.callback)
+                       return IRQ_NONE;
+               if (!vq->vq.callback(&vq->vq))
+                       vq->vring.avail->flags |= 

VRING_AVAIL_F_NO_INTERRUPT;

This attempts to implement a "virtual I/O" layer which should allow common drivers to be efficiently used across most virtual I/O mechanisms. It will no-doubt need further enhancement.

drivers/virtio/virtio.c new file +8 -0

+
+struct virtio_bus {
+	struct bus_type bus;
+	struct device dev;
+};
+
+static struct virtio_bus virtio_bus = {
+	.bus = {

include/linux/virtio_config.h new file +61 -0

+#ifndef _LINUX_VIRTIO_CONFIG_H
+#define _LINUX_VIRTIO_CONFIG_H
+/* Virtio devices use a standardized configuration space to define their
+ * features and pass configuration information, but each implementation can
+ * store and access that space differently. */
+#include <linux/types.h>
+
+/* Status byte for guest to report progress, and synchronize config. */
+/* We have seen device and processed generic fields (VIRTIO_CONFIG_F_VIRTIO) */
+#define VIRTIO_CONFIG_S_ACKNOWLEDGE	1
+/* We have found a driver for the device. */
+#define VIRTIO_CONFIG_S_DRIVER		2
+/* Driver has used its parts of the config, and is happy */
+#define VIRTIO_CONFIG_S_DRIVER_OK	4
+/* We've given up on this device. */
+#define VIRTIO_CONFIG_S_FAILED		0x80
+
+/* Feature byte (actually 7 bits availabe): */
+/* Requirements/features of the virtio implementation. */
+#define VIRTIO_CONFIG_F_VIRTIO 1
+/* Requirements/features of the virtqueue (may have more than one). */
+#define VIRTIO_CONFIG_F_VIRTQUEUE 2
+
+#ifdef __KERNEL__
+struct virtio_device;
+
+/**
+ * virtio_config_ops - operations for configuring a virtio device
+ * @find: search for the next configuration field of the given type.
+ *	vdev: the virtio_device
+ *	type: the feature type
+ *	len: the (returned) length of the field if found.
+ *	Returns a token if found, or NULL.  Never returnes the same field twice
+ *	(ie. it's used up).
+ * @get: read the value of a configuration field after find().
+ *	vdev: the virtio_device
+ *	token: the token returned from find().
+ *	buf: the buffer to write the field value into.
+ *	len: the length of the buffer (given by find()).
+ *	Note that contents are conventionally little-endian.
+ * @set: write the value of a configuration field after find().
+ *	vdev: the virtio_device
+ *	token: the token returned from find().
+ *	buf: the buffer to read the field value from.
+ *	len: the length of the buffer (given by find()).
+ *	Note that contents are conventionally little-endian.
+ * @get_status: read the status byte
+ *	vdev: the virtio_device
+ *	Returns the status byte
+ * @set_status: write the status byte
+ *	vdev: the virtio_device
+ *	status: the new status byte
+ * @find_vq: find the first VIRTIO_CONFIG_F_VIRTQUEUE and create a virtqueue.
+ *	vdev: the virtio_device
+ *	callback: the virqtueue callback
+ *	Returns the new virtqueue or ERR_PTR().
+ * @del_vq: free a virtqueue found by find_vq().
+ */
+struct virtio_config_ops
+{
+	void *(*find)(struct virtio_device *vdev, u8 type, unsigned *len);

include/linux/virtio_config.h new file +12 -0

+/**
+ * virtio_config_val - get a single virtio config and mark it used.
+ * @config: the virtio config space
+ * @type: the type to search for.
+ * @val: a pointer to the value to fill in.
+ *
+ * Once used, the config type is marked with VIRTIO_CONFIG_F_USED so it can't
+ * be found again.  This version does endian conversion. */
+#define virtio_config_val(vdev, type, v) ({				\
+	int _err = __virtio_config_val((vdev),(type),(v),sizeof(*(v))); \
+									\
+	BUILD_BUG_ON(sizeof(*(v)) != 1 && sizeof(*(v)) != 2		\

Quoted 3 lines · Rusty Russell

This attempts to implement a "virtual I/O" layer which should allow common drivers to be efficiently used across most virtual I/O mechanisms. It will no-doubt need further enhancement.

Hi Rusty,

Thanks for your update, as you can imagine, I'm much happier with this version ;-)

Quoted 8 lines

drivers/virtio/virtio.c new file +8 -0

+
+struct virtio_bus {
+	struct bus_type bus;
+	struct device dev;
+};
+
+static struct virtio_bus virtio_bus = {
+	.bus = {

drivers/virtio/virtio.c line 70

Quoted 10 lines

drivers/virtio/virtio.c new file +10 -0

+		.name  = "virtio",
+		.match = virtio_dev_match,
+		.dev_attrs = virtio_dev_attrs,
+	},
+	.dev = {
+		/* Can override this if you have a real bus behind it. */
+		.parent = NULL,
+		.bus_id = "virtio",
+	}
+};

This is a pattern I've seen a few times before, but could never understand what it's good for. What is your reason for defining a new data structure that is used only once, instead of just

static struct bus_type virtio_bus_type;
static struct device virtio_root_dev;

Also, I would not mix the two in a single source file. Instead, I think every driver that can provide virtio devices (pci, lguest, ...) should be responsible for setting the parent appropriately.

Quoted 10 lines

include/linux/virtio_config.h new file +10 -0

+struct virtio_config_ops
+{
+	void *(*find)(struct virtio_device *vdev, u8 type, unsigned *len);
+	void (*get)(struct virtio_device *vdev, void *token,
+		    void *buf, unsigned len);
+	void (*set)(struct virtio_device *vdev, void *token,
+		    const void *buf, unsigned len);
+	u8 (*get_status)(struct virtio_device *vdev);
+	void (*set_status)(struct virtio_device *vdev, u8 status);
+	struct virtqueue *(*find_vq)(struct virtio_device *vdev,

include/linux/virtio_config.h line 71

Quoted 3 lines

include/linux/virtio_config.h new file +3 -0

+				     bool (*callback)(struct virtqueue *));
+	void (*del_vq)(struct virtqueue *vq);
+};

The configuration logic looks more complicated than it should be. Maybe more of this can be done using data structure definitions instead of dynamic callback. E.g. the virtqueues of a given device can be listed as members of the struct virtio_device.

Quoted 16 lines

include/linux/virtio_config.h new file +16 -0

+/**
+ * virtio_config_val - get a single virtio config and mark it used.
+ * @config: the virtio config space
+ * @type: the type to search for.
+ * @val: a pointer to the value to fill in.
+ *
+ * Once used, the config type is marked with VIRTIO_CONFIG_F_USED so it can't
+ * be found again.  This version does endian conversion. */
+#define virtio_config_val(vdev, type, v) ({				\
+	int _err = __virtio_config_val((vdev),(type),(v),sizeof(*(v))); \
+									\
+	BUILD_BUG_ON(sizeof(*(v)) != 1 && sizeof(*(v)) != 2		\
+		     && sizeof(*(v)) != 4 && sizeof(*(v)) != 8);	\
+	if (!_err) {							\
+		switch (sizeof(*(v))) {					\
+		case 2: le16_to_cpus(v); break;				\

include/linux/virtio_config.h line 94

Quoted 6 lines

include/linux/virtio_config.h new file +6 -0

+		case 4: le32_to_cpus(v); break;				\
+		case 8: le64_to_cpus(v); break;				\
+		}							\
+	}								\
+	_err;								\
+})

Especially the macro looks like something better avoided...

Quoted 6 lines · Arnd Bergmann

This is a pattern I've seen a few times before, but could never understand what it's good for. What is your reason for defining a new data structure that is used only once, instead of just

static struct bus_type virtio_bus_type;
static struct device virtio_root_dev;

It's copied from the lguest bus which was copied from somewhere else. Creating a struct like this is a quiet complaint about the requirements to do so: it's not clear to me why I need to create a fake device, rather than making the bus the parent of the device if it needs one.

Quoted 3 lines

Also, I would not mix the two in a single source file. Instead, I think every driver that can provide virtio devices (pci, lguest, ...) should be responsible for setting the parent appropriately.

I don't mind: we could expose it.

Quoted 1 line · Rusty Russell

I don't mind: we could expose it.

What I mean with setting the parent appropriately is not to have a global device that is used by the hv-specific probing code, but to make sure that each of them provides their own one. The bus_type should either be global or provide the wrappers for device_register and driver_register that you have, but the host bridge device belongs with the code that probes it. E.g. when all virtio devices are behind PCI bridges, there does not need to be an empty /virtio or /lguest device node at all.

Quoted 1 line · Rusty Russell

I don't mind: we could expose it.

What I mean with setting the parent appropriately is not to have a global device that is used by the hv-specific probing code, but to make sure that each of them provides their own one. The bus_type should either be global or provide the wrappers for device_register and driver_register that you have, but the host bridge device belongs with the code that probes it. E.g. when all virtio devices are behind PCI bridges, there does not need to be an empty /virtio or /lguest device node at all.

Quoted 24 lines · Arnd Bergmann

Quoted 8 lines · Rusty Russell

drivers/virtio/virtio.c new file +8 -0

+
+struct virtio_bus {
+	struct bus_type bus;
+	struct device dev;
+};
+
+static struct virtio_bus virtio_bus = {
+	.bus = {

drivers/virtio/virtio.c line 70

Quoted 10 lines

drivers/virtio/virtio.c new file +10 -0

+		.name  = "virtio",
+		.match = virtio_dev_match,
+		.dev_attrs = virtio_dev_attrs,
+	},
+	.dev = {
+		/* Can override this if you have a real bus behind it. */
+		.parent = NULL,
+		.bus_id = "virtio",
+	}
+};

This is a pattern I've seen a few times before, but could never understand what it's good for. What is your reason for defining a new data structure that is used only once, instead of just

Quoted 2 lines

static struct bus_type virtio_bus_type;
static struct device virtio_root_dev;

It's copied from the lguest bus which was copied from somewhere else. Creating a struct like this is a quiet complaint about the requirements to do so: it's not clear to me why I need to create a fake device, rather than making the bus the parent of the device if it needs one.

Quoted 6 lines · Arnd Bergmann

This is a pattern I've seen a few times before, but could never understand what it's good for. What is your reason for defining a new data structure that is used only once, instead of just

static struct bus_type virtio_bus_type;
static struct device virtio_root_dev;

It's copied from the lguest bus which was copied from somewhere else. Creating a struct like this is a quiet complaint about the requirements to do so: it's not clear to me why I need to create a fake device, rather than making the bus the parent of the device if it needs one.

Quoted 3 lines

Also, I would not mix the two in a single source file. Instead, I think every driver that can provide virtio devices (pci, lguest, ...) should be responsible for setting the parent appropriately.

I don't mind: we could expose it.

Quoted 24 lines · Arnd Bergmann

Quoted 8 lines · Rusty Russell

drivers/virtio/virtio.c new file +8 -0

+
+struct virtio_bus {
+	struct bus_type bus;
+	struct device dev;
+};
+
+static struct virtio_bus virtio_bus = {
+	.bus = {

drivers/virtio/virtio.c line 70

Quoted 10 lines

drivers/virtio/virtio.c new file +10 -0

+		.name  = "virtio",
+		.match = virtio_dev_match,
+		.dev_attrs = virtio_dev_attrs,
+	},
+	.dev = {
+		/* Can override this if you have a real bus behind it. */
+		.parent = NULL,
+		.bus_id = "virtio",
+	}
+};

This is a pattern I've seen a few times before, but could never understand what it's good for. What is your reason for defining a new data structure that is used only once, instead of just

Quoted 2 lines

static struct bus_type virtio_bus_type;
static struct device virtio_root_dev;

It's copied from the lguest bus which was copied from somewhere else. Creating a struct like this is a quiet complaint about the requirements to do so: it's not clear to me why I need to create a fake device, rather than making the bus the parent of the device if it needs one.

Quoted 3 lines · Rusty Russell

This attempts to implement a "virtual I/O" layer which should allow common drivers to be efficiently used across most virtual I/O mechanisms. It will no-doubt need further enhancement.

Hi Rusty,

Thanks for your update, as you can imagine, I'm much happier with this version ;-)

Quoted 8 lines

drivers/virtio/virtio.c new file +8 -0

+
+struct virtio_bus {
+	struct bus_type bus;
+	struct device dev;
+};
+
+static struct virtio_bus virtio_bus = {
+	.bus = {

drivers/virtio/virtio.c line 70

Quoted 10 lines

drivers/virtio/virtio.c new file +10 -0

+		.name  = "virtio",
+		.match = virtio_dev_match,
+		.dev_attrs = virtio_dev_attrs,
+	},
+	.dev = {
+		/* Can override this if you have a real bus behind it. */
+		.parent = NULL,
+		.bus_id = "virtio",
+	}
+};

This is a pattern I've seen a few times before, but could never understand what it's good for. What is your reason for defining a new data structure that is used only once, instead of just

static struct bus_type virtio_bus_type;
static struct device virtio_root_dev;

Also, I would not mix the two in a single source file. Instead, I think every driver that can provide virtio devices (pci, lguest, ...) should be responsible for setting the parent appropriately.

Quoted 10 lines

include/linux/virtio_config.h new file +10 -0

+struct virtio_config_ops
+{
+	void *(*find)(struct virtio_device *vdev, u8 type, unsigned *len);
+	void (*get)(struct virtio_device *vdev, void *token,
+		    void *buf, unsigned len);
+	void (*set)(struct virtio_device *vdev, void *token,
+		    const void *buf, unsigned len);
+	u8 (*get_status)(struct virtio_device *vdev);
+	void (*set_status)(struct virtio_device *vdev, u8 status);
+	struct virtqueue *(*find_vq)(struct virtio_device *vdev,

include/linux/virtio_config.h line 71

Quoted 3 lines

include/linux/virtio_config.h new file +3 -0

+				     bool (*callback)(struct virtqueue *));
+	void (*del_vq)(struct virtqueue *vq);
+};

The configuration logic looks more complicated than it should be. Maybe more of this can be done using data structure definitions instead of dynamic callback. E.g. the virtqueues of a given device can be listed as members of the struct virtio_device.

Quoted 16 lines

include/linux/virtio_config.h new file +16 -0

+/**
+ * virtio_config_val - get a single virtio config and mark it used.
+ * @config: the virtio config space
+ * @type: the type to search for.
+ * @val: a pointer to the value to fill in.
+ *
+ * Once used, the config type is marked with VIRTIO_CONFIG_F_USED so it can't
+ * be found again.  This version does endian conversion. */
+#define virtio_config_val(vdev, type, v) ({				\
+	int _err = __virtio_config_val((vdev),(type),(v),sizeof(*(v))); \
+									\
+	BUILD_BUG_ON(sizeof(*(v)) != 1 && sizeof(*(v)) != 2		\
+		     && sizeof(*(v)) != 4 && sizeof(*(v)) != 8);	\
+	if (!_err) {							\
+		switch (sizeof(*(v))) {					\
+		case 2: le16_to_cpus(v); break;				\

include/linux/virtio_config.h line 94

Quoted 6 lines

include/linux/virtio_config.h new file +6 -0

+		case 4: le32_to_cpus(v); break;				\
+		case 8: le64_to_cpus(v); break;				\
+		}							\
+	}								\
+	_err;								\
+})

Especially the macro looks like something better avoided...

This attempts to implement a "virtual I/O" layer which should allow common drivers to be efficiently used across most virtual I/O mechanisms. It will no-doubt need further enhancement.

drivers/virtio/virtio.c new file +8 -0

+
+struct virtio_bus {
+	struct bus_type bus;
+	struct device dev;
+};
+
+static struct virtio_bus virtio_bus = {
+	.bus = {

include/linux/virtio_config.h new file +61 -0

+#ifndef _LINUX_VIRTIO_CONFIG_H
+#define _LINUX_VIRTIO_CONFIG_H
+/* Virtio devices use a standardized configuration space to define their
+ * features and pass configuration information, but each implementation can
+ * store and access that space differently. */
+#include <linux/types.h>
+
+/* Status byte for guest to report progress, and synchronize config. */
+/* We have seen device and processed generic fields (VIRTIO_CONFIG_F_VIRTIO) */
+#define VIRTIO_CONFIG_S_ACKNOWLEDGE	1
+/* We have found a driver for the device. */
+#define VIRTIO_CONFIG_S_DRIVER		2
+/* Driver has used its parts of the config, and is happy */
+#define VIRTIO_CONFIG_S_DRIVER_OK	4
+/* We've given up on this device. */
+#define VIRTIO_CONFIG_S_FAILED		0x80
+
+/* Feature byte (actually 7 bits availabe): */
+/* Requirements/features of the virtio implementation. */
+#define VIRTIO_CONFIG_F_VIRTIO 1
+/* Requirements/features of the virtqueue (may have more than one). */
+#define VIRTIO_CONFIG_F_VIRTQUEUE 2
+
+#ifdef __KERNEL__
+struct virtio_device;
+
+/**
+ * virtio_config_ops - operations for configuring a virtio device
+ * @find: search for the next configuration field of the given type.
+ *	vdev: the virtio_device
+ *	type: the feature type
+ *	len: the (returned) length of the field if found.
+ *	Returns a token if found, or NULL.  Never returnes the same field twice
+ *	(ie. it's used up).
+ * @get: read the value of a configuration field after find().
+ *	vdev: the virtio_device
+ *	token: the token returned from find().
+ *	buf: the buffer to write the field value into.
+ *	len: the length of the buffer (given by find()).
+ *	Note that contents are conventionally little-endian.
+ * @set: write the value of a configuration field after find().
+ *	vdev: the virtio_device
+ *	token: the token returned from find().
+ *	buf: the buffer to read the field value from.
+ *	len: the length of the buffer (given by find()).
+ *	Note that contents are conventionally little-endian.
+ * @get_status: read the status byte
+ *	vdev: the virtio_device
+ *	Returns the status byte
+ * @set_status: write the status byte
+ *	vdev: the virtio_device
+ *	status: the new status byte
+ * @find_vq: find the first VIRTIO_CONFIG_F_VIRTQUEUE and create a virtqueue.
+ *	vdev: the virtio_device
+ *	callback: the virqtueue callback
+ *	Returns the new virtqueue or ERR_PTR().
+ * @del_vq: free a virtqueue found by find_vq().
+ */
+struct virtio_config_ops
+{
+	void *(*find)(struct virtio_device *vdev, u8 type, unsigned *len);

include/linux/virtio_config.h new file +12 -0

+/**
+ * virtio_config_val - get a single virtio config and mark it used.
+ * @config: the virtio config space
+ * @type: the type to search for.
+ * @val: a pointer to the value to fill in.
+ *
+ * Once used, the config type is marked with VIRTIO_CONFIG_F_USED so it can't
+ * be found again.  This version does endian conversion. */
+#define virtio_config_val(vdev, type, v) ({				\
+	int _err = __virtio_config_val((vdev),(type),(v),sizeof(*(v))); \
+									\
+	BUILD_BUG_ON(sizeof(*(v)) != 1 && sizeof(*(v)) != 2		\