Re: [kvm-devel] [PATCH 1/6] virtio interace

Rusty Russell
2007-09-21T11:37:28Z

On Thu, 2007-09-20 at 14:27 +0200, Avi Kivity wrote:
> Rusty Russell wrote:
> > +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.

Hi Avi,

Agreed, fixed. I actually don't like this interface at all, but it is
simple until we determine something better.

> > +/**
> > + * 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?

off is a cookie, len isn't. The driver does "get me the foo field" and
it returns off + len. If it wants to read or write the foo field it
needs that length.

In practice, drivers use the virtio_config_vq() convenience interface
which does "find field, hand to ops->new_vq".

> > + * callback: the driver callback when the queue is used.
> >
>
> Missing callback return value description.

Indeed, that's always made me uncomfortable. Fixed. Although it's
possible that an explicit disable hook is better...

> > + * @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'

Thanks, fixed.

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

I doubt it: it's not a problem for lguest. But I wonder if I've chosen
the right fields...

Rusty.