Re: [RFC PATCH 3/4] virtio: introudce a mdev based transport

[Jason Wang]

On 2019/9/11 äå5:36, Michael S. Tsirkin wrote:
> On Wed, Sep 11, 2019 at 10:38:39AM +0800, Jason Wang wrote:
> > On 2019/9/10 äå9:52, Michael S. Tsirkin wrote:
> > > On Tue, Sep 10, 2019 at 09:13:02PM +0800, Jason Wang wrote:
> > > > On 2019/9/10 äå6:01, Michael S. Tsirkin wrote:
> > > > > > +#ifndef _LINUX_VIRTIO_MDEV_H
> > > > > > +#define _LINUX_VIRTIO_MDEV_H
> > > > > > +
> > > > > > +#include <linux/interrupt.h>
> > > > > > +#include <linux/vringh.h>
> > > > > > +#include <uapi/linux/virtio_net.h>
> > > > > > +
> > > > > > +/*
> > > > > > + * Ioctls
> > > > > > + */
> > > > > Pls add a bit more content here. It's redundant to state these
> > > > > are ioctls. Much better to document what does each one do.
> > > > Ok.
> > > >
> > > >
> > > > > > +
> > > > > > +struct virtio_mdev_callback {
> > > > > > +	irqreturn_t (*callback)(void *);
> > > > > > +	void *private;
> > > > > > +};
> > > > > > +
> > > > > > +#define VIRTIO_MDEV 0xAF
> > > > > > +#define VIRTIO_MDEV_SET_VQ_CALLBACK _IOW(VIRTIO_MDEV, 0x00, \
> > > > > > +					 struct virtio_mdev_callback)
> > > > > > +#define VIRTIO_MDEV_SET_CONFIG_CALLBACK _IOW(VIRTIO_MDEV, 0x01, \
> > > > > > +					struct virtio_mdev_callback)
> > > > > Function pointer in an ioctl parameter? How does this ever make sense?
> > > > I admit this is hacky (casting).
> > > >
> > > >
> > > > > And can't we use a couple of registers for this, and avoid ioctls?
> > > > Yes, how about something like interrupt numbers for each virtqueue and
> > > > config?
> > > Should we just reuse VIRTIO_PCI_COMMON_Q_XXX then?
> >
> > You mean something like VIRTIO_PCI_COMMON_Q_MSIX? Then it becomes a PCI
> > transport in fact. And using either MSIX or irq number is actually another
> > layer of indirection. So I think we can just write callback function and
> > parameter through registers.
> I just realized, all these registers are just encoded so you
> can pass stuff through read/write. But it can instead be
> just a normal C function call with no messy encoding.
> So why do we want to do this encoding?


Just because it was easier to start as a POC since mdev_parent_ops is 
the only way to communicate between mdev driver and mdev device right 
now. We can invent private ops besides mdev_parent_ops, e.g a private 
pointer in mdev_parent_ops. I can try this in next version.

Thanks