Re: [Qemu-devel] [PATCH RFC v6 07/20] virtio: allow virtio-1 queue layout

[David Gibson]

On Wed, Jan 28, 2015 at 05:07:01PM +0100, Cornelia Huck wrote:
> On Thu, 22 Jan 2015 13:06:09 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Thu, Dec 11, 2014 at 02:25:09PM +0100, Cornelia Huck wrote:
> > > For virtio-1 devices, we allow a more complex queue layout that doesn't
> > > require descriptor table and rings on a physically-contigous memory area:
> > > add virtio_queue_set_rings() to allow transports to set this up.
> > > 
> > > Signed-off-by: Cornelia Huck <address@hidden>
> > > ---
> > >  hw/virtio/virtio-mmio.c    |    3 +++
> > >  hw/virtio/virtio.c         |   53 
> > > ++++++++++++++++++++++++++++----------------
> > >  include/hw/virtio/virtio.h |    3 +++
> > >  3 files changed, 40 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > > index 43b7e02..0c9b63b 100644
> > > --- a/hw/virtio/virtio-mmio.c
> > > +++ b/hw/virtio/virtio-mmio.c
> > > @@ -244,8 +244,11 @@ static void virtio_mmio_write(void *opaque, hwaddr 
> > > offset, uint64_t value,
> > >      case VIRTIO_MMIO_QUEUENUM:
> > >          DPRINTF("mmio_queue write %d max %d\n", (int)value, 
> > > VIRTQUEUE_MAX_SIZE);
> > >          virtio_queue_set_num(vdev, vdev->queue_sel, value);
> > > +        /* Note: only call this function for legacy devices */
> > 
> > It's not clear to me if this is an assertion that this *does* only
> > call the function for legacy devices or a fixme, that it *should* only
> > call the function for legacy devices.
> 
> It's more like a note to whoever takes the virtio-mmio legacy device
> code and writes a virtio-1 virtio-mmio device.
> 
> Does
> /* Note: this function must only be called for legacy devices */
> make that intention clearer?

Yes, I think that's better.

> 
> > 
> > > +        virtio_queue_update_rings(vdev, vdev->queue_sel);
> > >          break;
> > >      case VIRTIO_MMIO_QUEUEALIGN:
> > > +        /* Note: this is only valid for legacy devices */
> > >          virtio_queue_set_align(vdev, vdev->queue_sel, value);
> > >          break;
> > >      case VIRTIO_MMIO_QUEUEPFN:
> 
> (...)
> 
> > >  /* virt queue functions */
> > > -static void virtqueue_init(VirtQueue *vq)
> > > +void virtio_queue_update_rings(VirtIODevice *vdev, int n)
> > 
> > Perhaps something in the name to emphasise that this is only for <v1.0
> > devices?
> 
> virtio_queue_legacy_update_rings()? Maybe a bit long...

There aren't many callers, so I think long is ok in this case.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpi6Z_4EaOlw.pgp
Description: PGP signature