Re: [PATCH 1/6] virtio interace

Avi Kivity
2007-09-20T12:27:54Z

Rusty Russell wrote:
> (Changes:
> - renamed sync to kick as Dor suggested
> - added new_vq and free_vq hooks to create virtqueues
> - define a simple virtio driver, which uses PCI ids
> - provide register/unregister_virtio_driver hooks)
>
> 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.
>
> The virtio drivers add and get I/O buffers; as the buffers are consumed
> the driver "interrupt" callbacks are invoked.
>
> It also provides driver register and unregister hooks, which are
> simply overridden at run time (eg. for a guest kernel which supports
> KVM paravirt and lguest).
>

> +++ b/drivers/virtio/virtio.c
> @@ -0,0 +1,20 @@
> +#include <linux/virtio.h>
> +
> +struct virtio_backend_ops virtio_backend_ops;
> +EXPORT_SYMBOL_GPL(virtio_backend_ops);
>

Suggest calling this virtio_transport_ops rather than the too-generic
virtio_backend_ops. Especially since Xen uses backend for something
completely different.

> +
> +/**
> + * virtqueue_ops - operations for virtqueue abstraction layer
> + * @new_vq: create a new virtqueue
> + * config: the virtio_config_space field describing the queue
> + * off: the offset in the config space of the queue configuration
> + * len: the length of the virtio_config_space field
>

'off, len' are really a magic cookie. Why does the interface care about
their meaning?

> + * callback: the driver callback when the queue is used.
>

Missing callback return value description.
> + * @kick: update after add_buf
> + * vq: the struct virtqueue
> + * After one or more add_buf calls, invoke this to kick the virtio layer.
>

'the other side'

I'm not thrilled about reusing pci ids. Maybe the s390 can say whether
this is a real issue.

--
error compiling committee.c: too many arguments to function