virtio DMA API core stuff

From: Andy Lutomirski <[email protected]>

Once virtio starts using the DMA API, we won't be able to safely DMA from the stack. virtio-net does a couple of config DMA requests from small stack buffers -- switch to using dynamically-allocated memory.

This should have no effect on any performance-critical code paths.

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/net/virtio_net.c | 53 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

Quoted 7 lines · Andy Lutomirski

From: Andy Lutomirski <[email protected]>

Once virtio starts using the DMA API, we won't be able to safely DMA from the stack. virtio-net does a couple of config DMA requests from small stack buffers -- switch to using dynamically-allocated memory.

Quoted 6 lines

This should have no effect on any performance-critical code paths.

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/net/virtio_net.c | 53 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

Reviewed-by Joerg Roedel <[email protected]>

From: Andy Lutomirski <[email protected]>

virtio_ring currently sends the device (usually a hypervisor) physical addresses of its I/O buffers. This is okay when DMA addresses and physical addresses are the same thing, but this isn't always the case. For example, this never works on Xen guests, and it is likely to fail if a physical "virtio" device ever ends up behind an IOMMU or swiotlb.

Hi Andy,

Quoted 8 lines · Andy Lutomirski

From: Andy Lutomirski <[email protected]>

virtio_ring currently sends the device (usually a hypervisor) physical addresses of its I/O buffers. This is okay when DMA addresses and physical addresses are the same thing, but this isn't always the case. For example, this never works on Xen guests, and it is likely to fail if a physical "virtio" device ever ends up behind an IOMMU or swiotlb.

The overall code looks good, but I havn't seen and dma_sync* calls. When swiotlb=force is in use this would break.

Quoted 4 lines

drivers/virtio/virtio_ring.c +4 -0

+             vq->vring.desc[head].addr = cpu_to_virtio64(_vq->vdev, vring_map_single(
+                     vq,
+                     desc, total_sg * sizeof(struct vring_desc),
+                     DMA_TO_DEVICE));

Nit: I think readability could be improved here by using a temporary variable for the return value of vring_map_single().

Quoted 12 lines · Joerg Roedel

Hi Andy,

Quoted 8 lines · Andy Lutomirski

From: Andy Lutomirski <[email protected]>

virtio_ring currently sends the device (usually a hypervisor) physical addresses of its I/O buffers. This is okay when DMA addresses and physical addresses are the same thing, but this isn't always the case. For example, this never works on Xen guests, and it is likely to fail if a physical "virtio" device ever ends up behind an IOMMU or swiotlb.

Quoted 8 lines

The overall code looks good, but I havn't seen and dma_sync* calls. When swiotlb=force is in use this would break.

Quoted 4 lines

drivers/virtio/virtio_ring.c +4 -0

+             vq->vring.desc[head].addr = cpu_to_virtio64(_vq->vdev, vring_map_single(
+                     vq,
+                     desc, total_sg * sizeof(struct vring_desc),
+                     DMA_TO_DEVICE));

Are you talking about a dma_sync call on the descriptor ring itself? Isn't dma_alloc_coherent supposed to make that unnecessary? I should move the allocation into the virtqueue code.

The docs suggest that I might need to "flush the processor's write buffers before telling devices to read that memory". I'm not sure how to do that.

Quoted 26 lines · Andy Lutomirski

Quoted 12 lines · Joerg Roedel

Hi Andy,

Quoted 8 lines · Andy Lutomirski

From: Andy Lutomirski <[email protected]>

virtio_ring currently sends the device (usually a hypervisor) physical addresses of its I/O buffers. This is okay when DMA addresses and physical addresses are the same thing, but this isn't always the case. For example, this never works on Xen guests, and it is likely to fail if a physical "virtio" device ever ends up behind an IOMMU or swiotlb.

Quoted 8 lines

The overall code looks good, but I havn't seen and dma_sync* calls. When swiotlb=force is in use this would break.

Quoted 4 lines

drivers/virtio/virtio_ring.c +4 -0

+             vq->vring.desc[head].addr = cpu_to_virtio64(_vq->vdev, vring_map_single(
+                     vq,
+                     desc, total_sg * sizeof(struct vring_desc),
+                     DMA_TO_DEVICE));

Are you talking about a dma_sync call on the descriptor ring itself? Isn't dma_alloc_coherent supposed to make that unnecessary? I should move the allocation into the virtqueue code.

Quoted 3 lines

The docs suggest that I might need to "flush the processor's write buffers before telling devices to read that memory". I'm not sure how to do that.

The write buffers should be flushed by the dma-api functions if necessary. For dma_alloc_coherent allocations you don't need to call dma_sync*, but for the map_single/map_page/map_sg ones, as these might be bounce-buffered.

static void detach_buf(struct vring_virtqueue *vq, unsigned int head) {

drivers/virtio/virtio_ring.c +14 -8

-	unsigned int i;
+	unsigned int i, j;
+	u16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
 
 	/* Clear data ptr. */
-	vq->data[head] = NULL;
+	vq->desc_state[head].data = NULL;
 
-	/* Put back on free list: find end */
+	/* Put back on free list: unmap first-level descriptors and find end */
 	i = head;
 
-	/* Free the indirect table */
-	if (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))
-		kfree(phys_to_virt(virtio64_to_cpu(vq->vq.vdev, vq->vring.desc[i].addr)));
-
-	while (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT)) {
+	while (vq->vring.desc[i].flags & nextflag) {
+		vring_unmap_one(vq, &vq->vring.desc[i]);
 		i = virtio16_to_cpu(vq->vq.vdev, vq->vring.desc[i].next);
 		vq->vq.num_free++;
 	}
 
+	vring_unmap_one(vq, &vq->vring.desc[i]);
 	vq->vring.desc[i].next = cpu_to_virtio16(vq->vq.vdev, vq->free_head);
 	vq->free_head = head;
+
 	/* Plus final descriptor */
 	vq->vq.num_free++;
+
+	/* Free the indirect table, if any, now that it's unmapped. */
+	if (vq->desc_state[head].indir_desc) {
+		struct vring_desc *indir_desc = vq->desc_state[head].indir_desc;
+		u32 len = vq->vring.desc[head].len;
+

Am 28.10.2015 um 10:17 schrieb Andy Lutomirski:

(quoted diff) +0 -0

@@ -423,27 +522,42 @@ EXPORT_SYMBOL_GPL(virtqueue_kick);

Quoted 42 lines

static void detach_buf(struct vring_virtqueue *vq, unsigned int head) {

drivers/virtio/virtio_ring.c +18 -8

-     unsigned int i;
+     unsigned int i, j;
+     u16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
 
      /* Clear data ptr. */
-     vq->data[head] = NULL;
+     vq->desc_state[head].data = NULL;
 
-     /* Put back on free list: find end */
+     /* Put back on free list: unmap first-level descriptors and find end */
      i = head;
 
-     /* Free the indirect table */
-     if (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))
-             kfree(phys_to_virt(virtio64_to_cpu(vq->vq.vdev, vq->vring.desc[i].addr)));
-
-     while (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT)) {
+     while (vq->vring.desc[i].flags & nextflag) {
+             vring_unmap_one(vq, &vq->vring.desc[i]);
              i = virtio16_to_cpu(vq->vq.vdev, vq->vring.desc[i].next);
              vq->vq.num_free++;
      }
 
+     vring_unmap_one(vq, &vq->vring.desc[i]);
      vq->vring.desc[i].next = cpu_to_virtio16(vq->vq.vdev, vq->free_head);
      vq->free_head = head;
+
      /* Plus final descriptor */
      vq->vq.num_free++;
+
+     /* Free the indirect table, if any, now that it's unmapped. */
+     if (vq->desc_state[head].indir_desc) {
+             struct vring_desc *indir_desc = vq->desc_state[head].indir_desc;
+             u32 len = vq->vring.desc[head].len;
+
+             BUG_ON(!(vq->vring.desc[head].flags & VRING_DESC_F_INDIRECT));
+             BUG_ON(len == 0 || len % sizeof(struct vring_desc));
+
+             for (j = 0; j < len / sizeof(struct vring_desc); j++)

drivers/virtio/virtio_ring.c EXPORT_SYMBOL_GPL(virtqueue_kick); · line 561

Quoted 6 lines

drivers/virtio/virtio_ring.c +5 -0

+                     vring_unmap_one(vq, &indir_desc[j]);
+
+             kfree(vq->desc_state[head].indir_desc);
+             vq->desc_state[head].indir_desc = NULL;
+     }
 }

something seems to be broken with indirect descriptors with that change

From: Andy Lutomirski <[email protected]>

This fixes virtio-pci on platforms and busses that have IOMMUs. This will break the experimental QEMU Q35 IOMMU support until QEMU is fixed. In exchange, it fixes physical virtio hardware as well as virtio-pci running under Xen.

We should clean up the virtqueue API to do its own allocation and teach virtqueue_get_avail and virtqueue_get_used to return DMA addresses directly.

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/virtio/virtio_pci_common.h |  3 ++-
 drivers/virtio/virtio_pci_legacy.c | 19 +++++++++++++++----
 drivers/virtio/virtio_pci_modern.c | 34 ++++++++++++++++++++++++----------
 3 files changed, 41 insertions(+), 15 deletions(-)

Quoted 12 lines · Andy Lutomirski

From: Andy Lutomirski <[email protected]>

This fixes virtio-pci on platforms and busses that have IOMMUs.
This
will break the experimental QEMU Q35 IOMMU support until QEMU is fixed. In exchange, it fixes physical virtio hardware as well as virtio-pci running under Xen.

We should clean up the virtqueue API to do its own allocation and teach virtqueue_get_avail and virtqueue_get_used to return DMA addresses directly.

Quoted 7 lines

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/virtio/virtio_pci_common.h |  3 ++-
 drivers/virtio/virtio_pci_legacy.c | 19 +++++++++++++++----
 drivers/virtio/virtio_pci_modern.c | 34 ++++++++++++++++++++++++--
--------
 3 files changed, 41 insertions(+), 15 deletions(-)

Same here, you need to call the dma_sync* functions when passing data from/to the virtio-device.

I think a good test for that is to boot a virtio kvm-guest with swiotlb=force and see if it still works.

Quoted 2 lines · Joerg Roedel

I think a good test for that is to boot a virtio kvm-guest with swiotlb=force and see if it still works.

That's useful but doesn't cover the cases where dma_wmb() is needed, right?

We should make sure we're handling descriptors properly as we would for real hardware, and ensuring that addresses etc. are written to the descriptor (and a write barrier occurs) *before* the bit is set which tells the 'hardware' that it owns that descriptor.

Same here, you need to call the dma_sync* functions when passing data from/to the virtio-device.

I think a good test for that is to boot a virtio kvm-guest with swiotlb=force and see if it still works.