virtio core DMA API conversion

From: "Michael S. Tsirkin" <[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.

[I wrote the subject and commit message. mst wrote the code. --luto]

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

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/virtio_net.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

drivers/net/virtio_net.c +19 -15

index d8838dedb7a4..f94ab786088f 100644
@@ -140,6 +140,12 @@ struct virtnet_info {
 
 	/* CPU hot plug notifier */
 	struct notifier_block nb;
+
+	/* Control VQ buffers: protected by the rtnl lock */
+	struct virtio_net_ctrl_hdr ctrl_hdr;
+	virtio_net_ctrl_ack ctrl_status;
+	u8 ctrl_promisc;
+	u8 ctrl_allmulti;
 };
 
 struct padded_vnet_hdr {

@@ -976,31 +982,30 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *out)
 {
 	struct scatterlist *sgs[4], hdr, stat;
-	struct virtio_net_ctrl_hdr ctrl;
-	virtio_net_ctrl_ack status = ~0;
 	unsigned out_num = 0, tmp;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-	ctrl.class = class;
-	ctrl.cmd = cmd;
+	vi->ctrl_status = ~0;
+	vi->ctrl_hdr.class = class;
+	vi->ctrl_hdr.cmd = cmd;
 	/* Add header */
-	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
+	sg_init_one(&hdr, &vi->ctrl_hdr, sizeof(vi->ctrl_hdr));
 	sgs[out_num++] = &hdr;
 
 	if (out)
 		sgs[out_num++] = out;
 
 	/* Add return status. */
-	sg_init_one(&stat, &status, sizeof(status));
+	sg_init_one(&stat, &vi->ctrl_status, sizeof(vi->ctrl_status));
 	sgs[out_num] = &stat;
 
 	BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
 	virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
 
 	if (unlikely(!virtqueue_kick(vi->cvq)))
-		return status == VIRTIO_NET_OK;
+		return vi->ctrl_status == VIRTIO_NET_OK;
 
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.

@@ -1009,7 +1014,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	       !virtqueue_is_broken(vi->cvq))
 		cpu_relax();
 
-	return status == VIRTIO_NET_OK;
+	return vi->ctrl_status == VIRTIO_NET_OK;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)

@@ -1151,7 +1156,6 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg[2];
-	u8 promisc, allmulti;
 	struct virtio_net_ctrl_mac *mac_data;
 	struct netdev_hw_addr *ha;
 	int uc_count;

@@ -1163,22 +1167,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
 		return;
 
-	promisc = ((dev->flags & IFF_PROMISC) != 0);
-	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+	vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
+	vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-	sg_init_one(sg, &promisc, sizeof(promisc));
+	sg_init_one(sg, &vi->ctrl_promisc, sizeof(vi->ctrl_promisc));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC, sg))
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
-			 promisc ? "en" : "dis");
+			 vi->ctrl_promisc ? "en" : "dis");
 
-	sg_init_one(sg, &allmulti, sizeof(allmulti));
+	sg_init_one(sg, &vi->ctrl_allmulti, sizeof(vi->ctrl_allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_ALLMULTI, sg))
 		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
-			 allmulti ? "en" : "dis");
+			 vi->ctrl_allmulti ? "en" : "dis");
 
 	uc_count = netdev_uc_count(dev);
 	mc_count = netdev_mc_count(dev);

Am 30.10.2015 um 02:09 schrieb Andy Lutomirski:

Quoted 11 lines

From: "Michael S. Tsirkin" <[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.

[I wrote the subject and commit message. mst wrote the code. --luto]

Quoted 2 lines

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

Signed-off-by Michael S. Tsirkin <[email protected]>

I still get an error when using multiqueue:

# ethtool -L eth0 combined 4

[   33.534686] virtio_ccw 0.0.000d: DMA-API: device driver maps memory from stack [addr=00000000629e7c06]

From: "Michael S. Tsirkin" <[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.

[I wrote the subject and commit message. mst wrote the code. --luto]

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

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/virtio_net.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

drivers/net/virtio_net.c +19 -15

index d8838dedb7a4..f94ab786088f 100644
@@ -140,6 +140,12 @@ struct virtnet_info {
 
 	/* CPU hot plug notifier */
 	struct notifier_block nb;
+
+	/* Control VQ buffers: protected by the rtnl lock */
+	struct virtio_net_ctrl_hdr ctrl_hdr;
+	virtio_net_ctrl_ack ctrl_status;
+	u8 ctrl_promisc;
+	u8 ctrl_allmulti;
 };
 
 struct padded_vnet_hdr {

@@ -976,31 +982,30 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *out)
 {
 	struct scatterlist *sgs[4], hdr, stat;
-	struct virtio_net_ctrl_hdr ctrl;
-	virtio_net_ctrl_ack status = ~0;
 	unsigned out_num = 0, tmp;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-	ctrl.class = class;
-	ctrl.cmd = cmd;
+	vi->ctrl_status = ~0;
+	vi->ctrl_hdr.class = class;
+	vi->ctrl_hdr.cmd = cmd;
 	/* Add header */
-	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
+	sg_init_one(&hdr, &vi->ctrl_hdr, sizeof(vi->ctrl_hdr));
 	sgs[out_num++] = &hdr;
 
 	if (out)
 		sgs[out_num++] = out;
 
 	/* Add return status. */
-	sg_init_one(&stat, &status, sizeof(status));
+	sg_init_one(&stat, &vi->ctrl_status, sizeof(vi->ctrl_status));
 	sgs[out_num] = &stat;
 
 	BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
 	virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
 
 	if (unlikely(!virtqueue_kick(vi->cvq)))
-		return status == VIRTIO_NET_OK;
+		return vi->ctrl_status == VIRTIO_NET_OK;
 
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.

@@ -1009,7 +1014,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	       !virtqueue_is_broken(vi->cvq))
 		cpu_relax();
 
-	return status == VIRTIO_NET_OK;
+	return vi->ctrl_status == VIRTIO_NET_OK;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)

@@ -1151,7 +1156,6 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg[2];
-	u8 promisc, allmulti;
 	struct virtio_net_ctrl_mac *mac_data;
 	struct netdev_hw_addr *ha;
 	int uc_count;

@@ -1163,22 +1167,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
 		return;
 
-	promisc = ((dev->flags & IFF_PROMISC) != 0);
-	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+	vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
+	vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-	sg_init_one(sg, &promisc, sizeof(promisc));
+	sg_init_one(sg, &vi->ctrl_promisc, sizeof(vi->ctrl_promisc));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC, sg))
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
-			 promisc ? "en" : "dis");
+			 vi->ctrl_promisc ? "en" : "dis");
 
-	sg_init_one(sg, &allmulti, sizeof(allmulti));
+	sg_init_one(sg, &vi->ctrl_allmulti, sizeof(vi->ctrl_allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_ALLMULTI, sg))
 		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
-			 allmulti ? "en" : "dis");
+			 vi->ctrl_allmulti ? "en" : "dis");
 
 	uc_count = netdev_uc_count(dev);
 	mc_count = netdev_mc_count(dev);

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 immediate use case for me is to enable virtio on Xen guests. For that to work, we need vring to support DMA address translation as well as a corresponding change to virtio_pci or to another driver.

With this patch, if enabled, virtfs survives kmemleak and CONFIG_DMA_API_DEBUG.

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

---
 drivers/virtio/Kconfig           |   2 +-
 drivers/virtio/virtio_ring.c     | 190 +++++++++++++++++++++++++++++++--------
 tools/virtio/linux/dma-mapping.h |  17 ++++
 3 files changed, 172 insertions(+), 37 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

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

drivers/virtio/virtio_ring.c +7 -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]);

Quoted 48 lines · On Thu, 29 Oct 2015 18:09:47 -0700 Andy Lutomirski <[email protected]> wrote:

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 immediate use case for me is to enable virtio on Xen guests. For that to work, we need vring to support DMA address translation as well as a corresponding change to virtio_pci or to another driver.

With this patch, if enabled, virtfs survives kmemleak and CONFIG_DMA_API_DEBUG.

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

---
 drivers/virtio/Kconfig           |   2 +-
 drivers/virtio/virtio_ring.c     | 190 +++++++++++++++++++++++++++++++--------
 tools/virtio/linux/dma-mapping.h |  17 ++++
 3 files changed, 172 insertions(+), 37 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

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

(quoted diff) +6 -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++;
 	}

Review

Quoted 11 lines

(quoted diff) +7 -0

+	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;

This one needs to be virtio32_to_cpu(...) as well.

Review

Quoted 13 lines

(quoted diff) +11 -0

+
+		BUG_ON(!(vq->vring.desc[head].flags &
+			 cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)));
+		BUG_ON(len == 0 || len % sizeof(struct vring_desc));
+
+		for (j = 0; j < len / sizeof(struct vring_desc); j++)
+			vring_unmap_one(vq, &indir_desc[j]);
+
+		kfree(vq->desc_state[head].indir_desc);
+		vq->desc_state[head].indir_desc = NULL;
+	}
 }

With that change on top of your current branch, I can boot (root on virtio-blk, either virtio-1 or legacy virtio) on current qemu master with kvm enabled on s390. Haven't tried anything further.

Am 30.10.2015 um 13:01 schrieb Cornelia Huck:

Quoted 51 lines

Quoted 48 lines · On Thu, 29 Oct 2015 18:09:47 -0700 Andy Lutomirski <[email protected]> wrote:

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 immediate use case for me is to enable virtio on Xen guests. For that to work, we need vring to support DMA address translation as well as a corresponding change to virtio_pci or to another driver.

With this patch, if enabled, virtfs survives kmemleak and CONFIG_DMA_API_DEBUG.

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

---
 drivers/virtio/Kconfig           |   2 +-
 drivers/virtio/virtio_ring.c     | 190 +++++++++++++++++++++++++++++++--------
 tools/virtio/linux/dma-mapping.h |  17 ++++
 3 files changed, 172 insertions(+), 37 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

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

(quoted diff) +6 -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++;
     }

Quoted 13 lines

Review

Quoted 11 lines

(quoted diff) +7 -0

+    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;

This one needs to be virtio32_to_cpu(...) as well.

Yes, just did the exact same change

drivers/virtio/virtio_ring.c +1 -1

index f269e1c..f2249df 100644
@@ -556,7 +556,7 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
        /* 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;
+               u32 len = virtio32_to_cpu(vq->vq.vdev, vq->vring.desc[head].len);
 
                BUG_ON(!(vq->vring.desc[head].flags &
                         cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)));

now it boots.

Am 30.10.2015 um 13:01 schrieb Cornelia Huck:

Quoted 51 lines

Quoted 48 lines · On Thu, 29 Oct 2015 18:09:47 -0700 Andy Lutomirski <[email protected]> wrote:

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 immediate use case for me is to enable virtio on Xen guests. For that to work, we need vring to support DMA address translation as well as a corresponding change to virtio_pci or to another driver.

With this patch, if enabled, virtfs survives kmemleak and CONFIG_DMA_API_DEBUG.

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

---
 drivers/virtio/Kconfig           |   2 +-
 drivers/virtio/virtio_ring.c     | 190 +++++++++++++++++++++++++++++++--------
 tools/virtio/linux/dma-mapping.h |  17 ++++
 3 files changed, 172 insertions(+), 37 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

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

(quoted diff) +6 -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++;
     }

Quoted 13 lines

Review

Quoted 11 lines

(quoted diff) +7 -0

+    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;

This one needs to be virtio32_to_cpu(...) as well.

Yes, just did the exact same change

drivers/virtio/virtio_ring.c +1 -1

index f269e1c..f2249df 100644
@@ -556,7 +556,7 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
        /* 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;
+               u32 len = virtio32_to_cpu(vq->vq.vdev, vq->vring.desc[head].len);
 
                BUG_ON(!(vq->vring.desc[head].flags &
                         cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)));

now it boots.

This switches virtio to use the DMA API unconditionally. I'm sure it breaks things, but it seems to work on x86 using virtio-pci, with and without Xen, and using both the modern 1.0 variant and the legacy variant.

Andy Lutomirski (5):
virtio_ring: Support DMA APIs
virtio_pci: Use the DMA API
virtio: Add improved queue allocation API
virtio_mmio: Use the DMA API
virtio_pci: Use the DMA API

This switches virtio to use the DMA API unconditionally. I'm sure it breaks things, but it seems to work on x86 using virtio-pci, with and without Xen, and using both the modern 1.0 variant and the legacy variant.

This appears to work on native and Xen x86_64 using both modern and legacy virtio-pci. It also appears to work on arm and arm64.

It definitely won't work as-is on s390x, and I haven't been able to test Christian's patches because I can't get virtio-ccw to work in QEMU at all. I don't know what I'm doing wrong.

virtio-net: Stop doing DMA from the stack

 drivers/net/virtio_net.c           |  34 ++--
 drivers/virtio/Kconfig             |   2 +-
 drivers/virtio/virtio_mmio.c       |  67 ++-----
 drivers/virtio/virtio_pci_common.h |   6 -
 drivers/virtio/virtio_pci_legacy.c |  42 ++---
 drivers/virtio/virtio_pci_modern.c |  61 ++-----
 drivers/virtio/virtio_ring.c       | 348 ++++++++++++++++++++++++++++++-------

This switches virtio to use the DMA API unconditionally. I'm sure it breaks things, but it seems to work on x86 using virtio-pci, with and without Xen, and using both the modern 1.0 variant and the legacy variant.

This appears to work on native and Xen x86_64 using both modern and legacy virtio-pci. It also appears to work on arm and arm64.

It definitely won't work as-is on s390x, and I haven't been able to test Christian's patches because I can't get virtio-ccw to work in QEMU at all. I don't know what I'm doing wrong.

virtio-net: Stop doing DMA from the stack

 drivers/net/virtio_net.c           |  34 ++--
 drivers/virtio/Kconfig             |   2 +-
 drivers/virtio/virtio_mmio.c       |  67 ++-----
 drivers/virtio/virtio_pci_common.h |   6 -
 drivers/virtio/virtio_pci_legacy.c |  42 ++---
 drivers/virtio/virtio_pci_modern.c |  61 ++-----
 drivers/virtio/virtio_ring.c       | 348 ++++++++++++++++++++++++++++++-------

This switches virtio to use the DMA API unconditionally. I'm sure it breaks things, but it seems to work on x86 using virtio-pci, with and without Xen, and using both the modern 1.0 variant and the legacy variant.

This appears to work on native and Xen x86_64 using both modern and legacy virtio-pci. It also appears to work on arm and arm64.

It definitely won't work as-is on s390x, and I haven't been able to test Christian's patches because I can't get virtio-ccw to work in QEMU at all. I don't know what I'm doing wrong.

It doesn't work on ppc64. Ben, consider yourself pinged to send me a patch :)

It doesn't work on sparc64. I didn't realize at Kernel Summit that sparc64 has the same problem as ppc64.

DaveM, for background, we're trying to fix virtio to use the DMA API. That will require that every platform that uses virtio supplies valid DMA operations on devices that use virtio_ring. Unfortunately, QEMU historically ignores the IOMMU on virtio devices.

On x86, this isn't really a problem. x86 has a nice way for the platform to describe which devices are behind an IOMMU, and QEMU will be adjusted accordingly. The only thing that will break is a recently-added experimental mode.

Ben's plan for powerpc is to add a quirk for existing virtio-pci devices and to eventually update the devicetree stuff to allow QEMU to tell the guest which devices use the IOMMU.

AFAICT sparc has a similar problem to powerpc. DaveM, can you come up with a straightforward way to get sparc's DMA API to work correctly for virtio-pci devices?

NB: Sadly, the platforms I've successfully tested on don't include any big-endian platforms, so there could still be lurking endian problems.

Changes from v3:
- More big-endian fixes.
- Added better virtio-ring APIs that handle allocation and use them in virtio-mmio and virtio-pci.
- Switch to Michael's virtio-net patch.

Changes from v2:
- Fix vring_mapping_error incorrect argument

Changes from v1:
- Fix an endian conversion error causing a BUG to hit.
- Fix a DMA ordering issue (swiotlb=force works now).
- Minor cleanups.

Andy Lutomirski (5):
virtio_ring: Support DMA APIs
virtio_pci: Use the DMA API
virtio: Add improved queue allocation API
virtio_mmio: Use the DMA API
virtio_pci: Use the DMA API

Michael S. Tsirkin (1):
virtio-net: Stop doing DMA from the stack

 drivers/net/virtio_net.c           |  34 ++--
 drivers/virtio/Kconfig             |   2 +-
 drivers/virtio/virtio_mmio.c       |  67 ++-----
 drivers/virtio/virtio_pci_common.h |   6 -
 drivers/virtio/virtio_pci_legacy.c |  42 ++---
 drivers/virtio/virtio_pci_modern.c |  61 ++-----
 drivers/virtio/virtio_ring.c       | 348 ++++++++++++++++++++++++++++++-------
 include/linux/virtio.h             |  23 ++-
 include/linux/virtio_ring.h        |  35 ++++
 tools/virtio/linux/dma-mapping.h   |  17 ++
 10 files changed, 426 insertions(+), 209 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

Signature · 1 line

2.4.3

Well that's not exactly true. I think we would like to make it possible to put virtio devices behind an IOMMU on x86, but if this means existing guests break, then many people won't be able to use this option: having to find out which kernel version your guest is running is a significant burden.

So on the host side, we need to detect guests that don't program the IOMMU and make QEMU ignore it. I think we need to figure out a way to do this before we commit to the guest change.

Additionally, IOMMU overhead is very high when running within the VM. So for uses such as VFIO, we'd like a way to make something like iommu-pt the default.

Well that's not exactly true. I think we would like to make it possible to put virtio devices behind an IOMMU on x86, but if this means existing guests break, then many people won't be able to use this option: having to find out which kernel version your guest is running is a significant burden.

So on the host side, we need to detect guests that don't program the IOMMU and make QEMU ignore it. I think we need to figure out a way to do this before we commit to the guest change.

Additionally, IOMMU overhead is very high when running within the VM. So for uses such as VFIO, we'd like a way to make something like iommu-pt the default.

So ...

I've finally tried to sort that out for powerpc and I can't find a way to make that work that isn't a complete pile of stinking shit.

I'm very tempted to go back to my original idea: virtio itself should indicate it's "bypassing ability" via the virtio config space or some other bit (like the ProgIf of the PCI header etc...).

I don't see how I can make it work otherwise.

The problem with the statement "it's a platform matter" is that:

- It's not entirely correct. It's actually a feature of a specific
virtio implementation (qemu's) that it bypasses all the platform IOMMU facilities.

- The platforms (on powerpc there's at least 3 or 4 that have qemu
emulation and support some form of PCI) don't have an existing way to convey the information that a device bypasses the IOMMU (if any).

- Even if we add such a mechanism (new property in the device-tree),
we end up with a big turd: Because we need to be compatible with older qemus, we essentially need a quirk that will make all virtio devices assume that property is present. That will of course break whenever we try to use another implementation of virtio on powerpc which doesn't bypass the iommu.

We don't have a way to differenciate a virtio device that does the bypass from one that doesn't and the backward compatibility requirement forces us to treat all existing virtio devices as doing bypass.

The only way out of this while keeping the "platform" stuff would be to also bump some kind of version in the virtio config (or PCI header). I have no other way to differenciate between "this is an old qemu that doesn't do the 'bypass property' yet" from "this is a virtio device that doesn't bypass".

Any better idea ?

The problem here is that in some of the problematic cases the virtio driver may not even be loaded.  If someone runs an L1 guest with an IOMMU-bypassing virtio device and assigns it to L2 using vfio, then *boom* L1 crashes.  (Same if, say, DPDK gets used, I think.)

Quoted 7 lines

The only way out of this while keeping the "platform" stuff would be to also bump some kind of version in the virtio config (or PCI header). I have no other way to differenciate between "this is an old qemu that doesn't do the 'bypass property' yet" from "this is a virtio device that doesn't bypass".

Quoted 1 line

Any better idea ?

I'd suggest that, in the absence of the new DT binding, we assume that any PCI device with the virtio vendor ID is passthrough on powerpc.  I can do this in the virtio driver, but if it's in the platform code then vfio gets it right too (i.e. fails to load).

Quoted 14 lines · Andy Lutomirski

The problem here is that in some of the problematic cases the virtio driver may not even be loaded. If someone runs an L1 guest with an IOMMU-bypassing virtio device and assigns it to L2 using vfio, then *boom* L1 crashes. (Same if, say, DPDK gets used, I think.)

Quoted 8 lines

The only way out of this while keeping the "platform" stuff would be to also bump some kind of version in the virtio config (or PCI header). I have no other way to differenciate between "this is an old qemu that doesn't do the 'bypass property' yet" from "this is a virtio device that doesn't bypass".

Any better idea ?

Quoted 4 lines

I'd suggest that, in the absence of the new DT binding, we assume that any PCI device with the virtio vendor ID is passthrough on powerpc. I can do this in the virtio driver, but if it's in the platform code then vfio gets it right too (i.e. fails to load).

The problem is there isn't *a* virtio vendor ID. It's the RedHat vendor ID which will be used by more than just virtio, so we need to specifically list the devices.

Additionally, that still means that once we have a virtio device that actually uses the iommu, powerpc will not work since the "workaround" above will kick in.

The "in absence of the new DT binding" doesn't make that much sense.

Those platforms use device-trees defined since the dawn of ages by actual open firmware implementations, they either have no iommu representation in there (Macs, the platform code hooks it all up) or have various properties related to the iommu but no concept of "bypass" in there.

We can *add* a new property under some circumstances that indicates a bypass on a per-device basis, however that doesn't completely solve it:

- As I said above, what does the absence of that property mean ? An
old qemu that does bypass on all virtio or a new qemu trying to tell you that the virtio device actually does use the iommu (or some other environment that isn't qemu) ?

- On things like macs, the device-tree is generated by openbios, it
would have to have some added logic to try to figure that out, which means it needs to know *via different means* that some or all virtio devices bypass the iommu.

I thus go back to my original statement, it's a LOT easier to handle if the device itself is self describing, indicating whether it is set to bypass a host iommu or not. For L1->L2, well, that wouldn't be the first time qemu/VFIO plays tricks with the passed through device configuration space...

Note that the above can be solved via some kind of compromise: The device self describes the ability to honor the iommu, along with the property (or ACPI table entry) that indicates whether or not it does.

IE. We could use the revision or ProgIf field of the config space for example. Or something in virtio config. If it's an "old" device, we know it always bypass. If it's a new device, we know it only bypasses if the corresponding property is in. I still would have to sort out the openbios case for mac among others but it's at least a workable direction.

BTW. Don't you have a similar problem on x86 that today qemu claims that everything honors the iommu in ACPI ?

Which leaves the special case of Xen, where even preexisting devices don't bypass the IOMMU.  Can we keep this specific to powerpc and sparc?  On x86, this problem is basically nonexistent, since the IOMMU is properly self-describing.

IOW, I think that on x86 we should assume that all virtio devices honor the IOMMU.

Quoted 8 lines · Andy Lutomirski

Which leaves the special case of Xen, where even preexisting devices don't bypass the IOMMU. Can we keep this specific to powerpc and sparc? On x86, this problem is basically nonexistent, since the IOMMU is properly self-describing.

IOW, I think that on x86 we should assume that all virtio devices honor the IOMMU.

You don't like performances ? :-)

Quoted 8 lines · Andy Lutomirski

Which leaves the special case of Xen, where even preexisting devices don't bypass the IOMMU. Can we keep this specific to powerpc and sparc? On x86, this problem is basically nonexistent, since the IOMMU is properly self-describing.

IOW, I think that on x86 we should assume that all virtio devices honor the IOMMU.

You don't like performances ? :-)

/* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
static const struct pci_device_id virtio_pci_id_table[] = { { PCI_DEVICE(0x1af4, PCI_ANY_ID) },
        { 0 }
};

Can we match on that range?

Quoted 8 lines · Andy Lutomirski

/* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */

static const struct pci_device_id virtio_pci_id_table[] = {
        { PCI_DEVICE(0x1af4, PCI_ANY_ID) },
        { 0 }
};

Quoted 1 line

Can we match on that range?

We can, but the problem remains, how do we differenciate an existing device that does bypass vs. a newer one that needs the IOMMU and thus doesn't have the new "bypass" property in the device-tree.

We could do it the other way around: on powerpc, if a PCI device is in that range and doesn't have the "bypass" property at all, then it's assumed to bypass the IOMMU.  This means that everything that currently works continues working.  If someone builds a physical virtio device or uses another system in PCIe target mode speaking virtio, then it won't work until they upgrade their firmware to set bypass=0.  Meanwhile everyone using hypothetical new QEMU also gets bypass=0 and no ambiguity.

vfio will presumably notice the bypass and correctly refuse to map any current virtio devices.

Would that work?

Quoted 13 lines · Andy Lutomirski

We could do it the other way around: on powerpc, if a PCI device is in that range and doesn't have the "bypass" property at all, then it's assumed to bypass the IOMMU.  This means that everything that currently works continues working.  If someone builds a physical virtio device or uses another system in PCIe target mode speaking virtio, then it won't work until they upgrade their firmware to set bypass=0.  Meanwhile everyone using hypothetical new QEMU also gets bypass=0 and no ambiguity.

vfio will presumably notice the bypass and correctly refuse to map any current virtio devices.

Quoted 1 line

Would that work?

That would be extremely strange from a platform perspective. Any device in that vendor/device range would bypass the iommu unless some new property "actually-works-like-a-real-pci-device" happens to exist in the device-tree, which we would then need to define somewhere and handle accross at least 3 different platforms who get their device-tree from widly different places.

Also if tomorrow I create a PCI device that implements virtio-net and put it in a machine running IBM proprietary firmware (or Apple's or Sun's), it won't have that property...

This is not hypothetical. People are using virtio to do point-to-point communication between machines via PCIe today.

Cheers,
Ben.

But not virtio-pci I think - that's broken for that usecase since we use weaker barriers than required for real IO, as these have measureable overhead.  We could have a feature "is a real PCI device", that's completely reasonable.

Quoted 13 lines · Andy Lutomirski

We could do it the other way around: on powerpc, if a PCI device is in that range and doesn't have the "bypass" property at all, then it's assumed to bypass the IOMMU.  This means that everything that currently works continues working.  If someone builds a physical virtio device or uses another system in PCIe target mode speaking virtio, then it won't work until they upgrade their firmware to set bypass=0.  Meanwhile everyone using hypothetical new QEMU also gets bypass=0 and no ambiguity.

vfio will presumably notice the bypass and correctly refuse to map any current virtio devices.

Quoted 1 line

Would that work?

That would be extremely strange from a platform perspective. Any device in that vendor/device range would bypass the iommu unless some new property "actually-works-like-a-real-pci-device" happens to exist in the device-tree, which we would then need to define somewhere and handle accross at least 3 different platforms who get their device-tree from widly different places.

Also if tomorrow I create a PCI device that implements virtio-net and put it in a machine running IBM proprietary firmware (or Apple's or Sun's), it won't have that property...

This is not hypothetical. People are using virtio to do point-to-point communication between machines via PCIe today.

Cheers,
Ben.

Quoted 10 lines · Andy Lutomirski

We could do it the other way around: on powerpc, if a PCI device is in that range and doesn't have the "bypass" property at all, then it's assumed to bypass the IOMMU. This means that everything that currently works continues working. If someone builds a physical virtio device or uses another system in PCIe target mode speaking virtio, then it won't work until they upgrade their firmware to set bypass=0. Meanwhile everyone using hypothetical new QEMU also gets bypass=0 and no ambiguity.

Quoted 4 lines

vfio will presumably notice the bypass and correctly refuse to map any current virtio devices.

Would that work?

That would be extremely strange from a platform perspective. Any device in that vendor/device range would bypass the iommu unless some new property "actually-works-like-a-real-pci-device" happens to exist in the device-tree, which we would then need to define somewhere and handle accross at least 3 different platforms who get their device-tree from widly different places.

Also if tomorrow I create a PCI device that implements virtio-net and put it in a machine running IBM proprietary firmware (or Apple's or Sun's), it won't have that property...

This is not hypothetical. People are using virtio to do point-to-point communication between machines via PCIe today.

Cheers,
Ben.

Does that work on powerpc on existing kernels?

Anyway, here's another crazy idea: make the quirk assume that the IOMMU is bypasses if and only if the weak barriers bit is set on systems that are missing the new DT binding.

Quoted 6 lines · Andy Lutomirski

Does that work on powerpc on existing kernels?

Anyway, here's another crazy idea: make the quirk assume that the IOMMU is bypasses if and only if the weak barriers bit is set on systems that are missing the new DT binding.

"New DT bindings" doesn't mean much ... how do we change DT bindings on existing machines with a FW in flash ?

What about partition <-> partition virtio such as what we could do on PAPR systems. That would have the weak barrier bit.

Quoted 4 lines

What about partition <-> partition virtio such as what we could do on PAPR systems. That would have the weak barrier bit.

Is it partition <-> partition, bypassing IOMMU?

I think I'd settle for just something that doesn't regress non-experimental setups that actually work today and that allow new setups (x86 with fixed QEMU and maybe something more complicated on powerpc and/or sparc) to work in all cases.

We could certainly just make powerpc and sparc continue bypassing the IOMMU until someone comes up with a way to fix it.  I'll send out some patches that do that, and maybe that'll help this make progress.

Quoted 6 lines · Andy Lutomirski

Quoted 4 lines

What about partition <-> partition virtio such as what we could do on PAPR systems. That would have the weak barrier bit.

Is it partition <-> partition, bypassing IOMMU?

No.

Quoted 8 lines

I think I'd settle for just something that doesn't regress non-experimental setups that actually work today and that allow new setups (x86 with fixed QEMU and maybe something more complicated on powerpc and/or sparc) to work in all cases.

We could certainly just make powerpc and sparc continue bypassing the IOMMU until someone comes up with a way to fix it. I'll send out some patches that do that, and maybe that'll help this make progress.

But we haven't found a solution that works. All we have come up with is a quirk that will force bypass on virtio always and will not allow us to operate non-bypassing devices on either of those architectures in the future.

I'm not too happy about this.

Me neither.  At least it wouldn't be a regression, but it's still crappy.

I think that arm is fine, at least.  I was unable to find an arm QEMU config that has any problems with my patches.

Quoted 6 lines · Andy Lutomirski

Does that work on powerpc on existing kernels?

Anyway, here's another crazy idea: make the quirk assume that the IOMMU is bypasses if and only if the weak barriers bit is set on systems that are missing the new DT binding.

"New DT bindings" doesn't mean much ... how do we change DT bindings on existing machines with a FW in flash ?

What about partition <-> partition virtio such as what we could do on PAPR systems. That would have the weak barrier bit.

Quoted 8 lines · Andy Lutomirski

/* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */

static const struct pci_device_id virtio_pci_id_table[] = {
        { PCI_DEVICE(0x1af4, PCI_ANY_ID) },
        { 0 }
};

Quoted 1 line

Can we match on that range?

We can, but the problem remains, how do we differenciate an existing device that does bypass vs. a newer one that needs the IOMMU and thus doesn't have the new "bypass" property in the device-tree.

On Mon, Nov 9, 2015 at 6:04 PM, Benjamin Herrenschmidt

Quoted 5 lines

I thus go back to my original statement, it's a LOT easier to handle if the device itself is self describing, indicating whether it is set to bypass a host iommu or not. For L1->L2, well, that wouldn't be the first time qemu/VFIO plays tricks with the passed through device configuration space...

Which leaves the special case of Xen, where even preexisting devices don't bypass the IOMMU. Can we keep this specific to powerpc and sparc? On x86, this problem is basically nonexistent, since the IOMMU is properly self-describing.

IOW, I think that on x86 we should assume that all virtio devices honor the IOMMU.

From the guest driver POV, that is OK because either there is no IOMMU

Quoted 13 lines

Note that the above can be solved via some kind of compromise: The device self describes the ability to honor the iommu, along with the property (or ACPI table entry) that indicates whether or not it does.

IE. We could use the revision or ProgIf field of the config space for example. Or something in virtio config. If it's an "old" device, we know it always bypass. If it's a new device, we know it only bypasses if the corresponding property is in. I still would have to sort out the openbios case for mac among others but it's at least a workable direction.

Quoted 2 lines

BTW. Don't you have a similar problem on x86 that today qemu claims that everything honors the iommu in ACPI ?

Only on a single experimental configuration, and that can apparently just be fixed going forward without any real problems being caused.

Quoted 14 lines · Andy Lutomirski

The problem here is that in some of the problematic cases the virtio driver may not even be loaded. If someone runs an L1 guest with an IOMMU-bypassing virtio device and assigns it to L2 using vfio, then *boom* L1 crashes. (Same if, say, DPDK gets used, I think.)

Quoted 8 lines

The only way out of this while keeping the "platform" stuff would be to also bump some kind of version in the virtio config (or PCI header). I have no other way to differenciate between "this is an old qemu that doesn't do the 'bypass property' yet" from "this is a virtio device that doesn't bypass".

Any better idea ?

Quoted 4 lines

I'd suggest that, in the absence of the new DT binding, we assume that any PCI device with the virtio vendor ID is passthrough on powerpc. I can do this in the virtio driver, but if it's in the platform code then vfio gets it right too (i.e. fails to load).

The problem is there isn't *a* virtio vendor ID. It's the RedHat vendor ID which will be used by more than just virtio, so we need to specifically list the devices.

Additionally, that still means that once we have a virtio device that actually uses the iommu, powerpc will not work since the "workaround" above will kick in.

The "in absence of the new DT binding" doesn't make that much sense.

Those platforms use device-trees defined since the dawn of ages by actual open firmware implementations, they either have no iommu representation in there (Macs, the platform code hooks it all up) or have various properties related to the iommu but no concept of "bypass" in there.

We can *add* a new property under some circumstances that indicates a bypass on a per-device basis, however that doesn't completely solve it:

- As I said above, what does the absence of that property mean ? An
old qemu that does bypass on all virtio or a new qemu trying to tell you that the virtio device actually does use the iommu (or some other environment that isn't qemu) ?

- On things like macs, the device-tree is generated by openbios, it
would have to have some added logic to try to figure that out, which means it needs to know *via different means* that some or all virtio devices bypass the iommu.

I thus go back to my original statement, it's a LOT easier to handle if the device itself is self describing, indicating whether it is set to bypass a host iommu or not. For L1->L2, well, that wouldn't be the first time qemu/VFIO plays tricks with the passed through device configuration space...

Note that the above can be solved via some kind of compromise: The device self describes the ability to honor the iommu, along with the property (or ACPI table entry) that indicates whether or not it does.

IE. We could use the revision or ProgIf field of the config space for example. Or something in virtio config. If it's an "old" device, we know it always bypass. If it's a new device, we know it only bypasses if the corresponding property is in. I still would have to sort out the openbios case for mac among others but it's at least a workable direction.

BTW. Don't you have a similar problem on x86 that today qemu claims that everything honors the iommu in ACPI ?

Quoted 17 lines · Andy Lutomirski

The problem here is that in some of the problematic cases the virtio
driver may not even be loaded. If someone runs an L1 guest with an IOMMU-bypassing virtio device and assigns it to L2 using vfio, then *boom* L1 crashes. (Same if, say, DPDK gets used, I think.)

Quoted 11 lines

The only way out of this while keeping the "platform" stuff would be to
also bump some kind of version in the virtio config (or PCI header). I
have no other way to differenciate between "this is an old qemu that
doesn't do the 'bypass property' yet" from "this is a virtio device
that doesn't bypass".

Quoted 8 lines

Quoted 1 line

Any better idea ?

I'd suggest that, in the absence of the new DT binding, we assume that
any PCI device with the virtio vendor ID is passthrough on powerpc.
I
can do this in the virtio driver, but if it's in the platform code then vfio gets it right too (i.e. fails to load).

The problem is there isn't *a* virtio vendor ID. It's the RedHat vendor
ID which will be used by more than just virtio, so we need to specifically list the devices.

Additionally, that still means that once we have a virtio device that actually uses the iommu, powerpc will not work since the "workaround" above will kick in.

The "in absence of the new DT binding" doesn't make that much sense.

Those platforms use device-trees defined since the dawn of ages by actual open firmware implementations, they either have no iommu representation in there (Macs, the platform code hooks it all up) or have various properties related to the iommu but no concept of "bypass"
in there.

We can *add* a new property under some circumstances that indicates a bypass on a per-device basis, however that doesn't completely solve it:

- As I said above, what does the absence of that property mean ? An
old qemu that does bypass on all virtio or a new qemu trying to tell you that the virtio device actually does use the iommu (or some other environment that isn't qemu) ?

- On things like macs, the device-tree is generated by openbios, it
would have to have some added logic to try to figure that out, which means it needs to know *via different means* that some or all virtio devices bypass the iommu.

I thus go back to my original statement, it's a LOT easier to handle if
the device itself is self describing, indicating whether it is set to bypass a host iommu or not. For L1->L2, well, that wouldn't be the first time qemu/VFIO plays tricks with the passed through device configuration space...

Note that the above can be solved via some kind of compromise: The device self describes the ability to honor the iommu, along with the property (or ACPI table entry) that indicates whether or not it does.

IE. We could use the revision or ProgIf field of the config space for example. Or something in virtio config. If it's an "old" device, we know it always bypass. If it's a new device, we know it only bypasses if the corresponding property is in. I still would have to sort out the
openbios case for mac among others but it's at least a workable direction.

BTW. Don't you have a similar problem on x86 that today qemu claims that everything honors the iommu in ACPI ?

Unless somebody can come up with a better idea...

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Can something be done by means of PCIe capabilities? ATS (Address Translation Support) seems like a natural choice?

Quoted 17 lines · Andy Lutomirski

The problem here is that in some of the problematic cases the virtio
driver may not even be loaded. If someone runs an L1 guest with an IOMMU-bypassing virtio device and assigns it to L2 using vfio, then *boom* L1 crashes. (Same if, say, DPDK gets used, I think.)

Quoted 11 lines

The only way out of this while keeping the "platform" stuff would be to
also bump some kind of version in the virtio config (or PCI header). I
have no other way to differenciate between "this is an old qemu that
doesn't do the 'bypass property' yet" from "this is a virtio device
that doesn't bypass".

Quoted 8 lines

Quoted 1 line

Any better idea ?

I'd suggest that, in the absence of the new DT binding, we assume that
any PCI device with the virtio vendor ID is passthrough on powerpc.
I
can do this in the virtio driver, but if it's in the platform code then vfio gets it right too (i.e. fails to load).

The problem is there isn't *a* virtio vendor ID. It's the RedHat vendor
ID which will be used by more than just virtio, so we need to specifically list the devices.

Additionally, that still means that once we have a virtio device that actually uses the iommu, powerpc will not work since the "workaround" above will kick in.

The "in absence of the new DT binding" doesn't make that much sense.

Those platforms use device-trees defined since the dawn of ages by actual open firmware implementations, they either have no iommu representation in there (Macs, the platform code hooks it all up) or have various properties related to the iommu but no concept of "bypass"
in there.

We can *add* a new property under some circumstances that indicates a bypass on a per-device basis, however that doesn't completely solve it:

- As I said above, what does the absence of that property mean ? An
old qemu that does bypass on all virtio or a new qemu trying to tell you that the virtio device actually does use the iommu (or some other environment that isn't qemu) ?

- On things like macs, the device-tree is generated by openbios, it
would have to have some added logic to try to figure that out, which means it needs to know *via different means* that some or all virtio devices bypass the iommu.

I thus go back to my original statement, it's a LOT easier to handle if
the device itself is self describing, indicating whether it is set to bypass a host iommu or not. For L1->L2, well, that wouldn't be the first time qemu/VFIO plays tricks with the passed through device configuration space...

Note that the above can be solved via some kind of compromise: The device self describes the ability to honor the iommu, along with the property (or ACPI table entry) that indicates whether or not it does.

IE. We could use the revision or ProgIf field of the config space for example. Or something in virtio config. If it's an "old" device, we know it always bypass. If it's a new device, we know it only bypasses if the corresponding property is in. I still would have to sort out the
openbios case for mac among others but it's at least a workable direction.

BTW. Don't you have a similar problem on x86 that today qemu claims that everything honors the iommu in ACPI ?

Unless somebody can come up with a better idea...

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The "in absence of the new DT binding" doesn't make that much sense.

Those platforms use device-trees defined since the dawn of ages by actual open firmware implementations, they either have no iommu representation in there (Macs, the platform code hooks it all up) or have various properties related to the iommu but no concept of "bypass" in there.

We can *add* a new property under some circumstances that indicates a bypass on a per-device basis, however that doesn't completely solve it:

  - As I said above, what does the absence of that property mean ? An old qemu that does bypass on all virtio or a new qemu trying to tell you that the virtio device actually does use the iommu (or some other environment that isn't qemu) ?

You have the same problem when real PCIe devices appear that speak virtio. I think the only real (still not very nice) solution is to add a quirk to powerpc platform code that sets noop dma-ops for the existing virtio vendor/device-ids and add a DT property to opt-out of that quirk.

New vendor/device-ids (as for real devices) would just not be covered by the quirk and existing emulated devices continue to work.

The absence of the property just means that the quirk is in place and the system assumes no translation for virtio devices.