On Tue, 2015-04-28 at 22:06 +0100, Christopher Covington wrote:
> Hi,
>
> On 01/20/2015 01:12 PM, Pawel Moll wrote:
>
> > @@ -356,13 +346,6 @@ static struct virtqueue *vm_setup_vq(struct
> > virtio_device *vdev, unsigned index,
> > info->num /= 2;
> > }
> >
> > - /* Activate the queue */
> > - writel(info->num, vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
> > - writel(VIRTIO_MMIO_VRING_ALIGN,
> > - vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
> > - writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
> > - vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> > -
> > /* Create the vring */
> > vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN,
> > vdev,
> > true, info->queue, vm_notify, callback, name);
> > @@ -371,6 +354,33 @@ static struct virtqueue *vm_setup_vq(struct
> > virtio_device *vdev, unsigned index,
> > goto error_new_virtqueue;
> > }
> >
> > + /* Activate the queue */
> > + writel(info->num, vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
> > + if (vm_dev->version == 1) {
> > + writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
> > + writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
> > + vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> > + } else {
> > + u64 addr;
> > +
> > + addr = virt_to_phys(info->queue);
> > + writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
> > + writel((u32)(addr >> 32),
> > + vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
> > +
> > + addr = virt_to_phys(virtqueue_get_avail(vq));
> > + writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
> > + writel((u32)(addr >> 32),
> > + vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
> > +
> > + addr = virt_to_phys(virtqueue_get_used(vq));
> > + writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
> > + writel((u32)(addr >> 32),
> > + vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
> > +
> > + writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> > + }
> > +
> > vq->priv = info;
> > info->vq = vq;
>
> This patch moved the call to vring_new_virtqueue() in the legacy code flow
> before the VIRTIO_MMIO_QUEUE_NUM, VIRTIO_MMIO_QUEUE_ALIGN, and
> VIRTIO_MMIO_QUEUE_PFN writes.
Just to make sure: we're talking the legacy case only here, correct?
> Was this intentional?
Yes, it simply made the code cleaner. I remember stopping for a moment
doing this change and thinking what bad can it make. Haven't figured out
anything, but it seems I was wrong ;-)
> Could the old behavior be reinstated?
I see no big problem with this, but only for the "if (vm_dev->version ==
1)" case.
> We have an implementation that relies on knowing ahead of time what address
> range will be used, and is blind to memory accesses that occur before
> VIRTIO_MMIO_QUEUE_PFN is written to (or VIRTIO_MMIO_QUEUE_READY when we
> upgrade). Is such an implementation supported by the specification? We can't
> find any explicit mention that the driver is forbidden from writing to the
> memory region before VIRTIO_MMIO_QUEUE_READY is set to 1 (or
> VIRTIO_MMIO_QUEUE_PFN is set for legacy devices).
Hm. At the first glance I wouldn't expect the spec to impose such ban.
After all the driver is responsible for providing the ring memory, spec
doesn't care (or does it?) how is it coming into existence - it's the
guest's memory after all. Am I missing something obvious?
Pawel
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
