On Thu, 22 Jan 2015 13:06:09 +1100
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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 <cornelia.huck@xxxxxxxxxx>
> > ---
> > 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?
>
> > + 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...
>
> > {
> > - hwaddr pa = vq->pa;
> > + VRing *vring = &vdev->vq[n].vring;
> >
> > - vq->vring.desc = pa;
> > - vq->vring.avail = pa + vq->vring.num * sizeof(VRingDesc);
> > - vq->vring.used = vring_align(vq->vring.avail +
> > - offsetof(VRingAvail, ring[vq->vring.num]),
> > - vq->vring.align);
> > + if (!vring->desc) {
> > + /* not yet setup -> nothing to do */
> > + return;
> > + }
> > + vring->avail = vring->desc + vring->num * sizeof(VRingDesc);
> > + vring->used = vring_align(vring->avail +
> > + offsetof(VRingAvail, ring[vring->num]),
> > + vring->align);
>
> Would it make sense to implement this in terms of
> virtio_queue_set_rings()?
Perhaps a bit confusing, since that would re-write desc.
>
> > }
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
