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.

Cc "Michael S. Tsirkin" <[email protected]>

Cc [email protected]

Reviewed-by Joerg Roedel <[email protected]>

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

---

Hi Michael and DaveM-

This is a prerequisite for the virtio DMA fixing project. It works as a standalone patch, though. Would it make sense to apply it to an appropriate networking tree now?

(This is unchanged from v2.)

drivers/net/virtio_net.c | 53 ++++++++++++++++++++++++++++++++----------------

 1 file changed, 36 insertions(+), 17 deletions(-)

drivers/net/virtio_net.c +36 -17

index d8838dedb7a4..4f10f8a58811 100644
@@ -976,31 +976,43 @@ 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;
+
+	struct {
+		struct virtio_net_ctrl_hdr ctrl;
+		virtio_net_ctrl_ack status;
+	} *buf;
+
 	unsigned out_num = 0, tmp;
+	bool ret;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-	ctrl.class = class;
-	ctrl.cmd = cmd;
+	buf = kmalloc(sizeof(*buf), GFP_ATOMIC);
+	if (!buf)
+		return false;
+	buf->status = ~0;
+
+	buf->ctrl.class = class;
+	buf->ctrl.cmd = cmd;
 	/* Add header */
-	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
+	sg_init_one(&hdr, &buf->ctrl, sizeof(buf->ctrl));
 	sgs[out_num++] = &hdr;
 
 	if (out)
 		sgs[out_num++] = out;
 
 	/* Add return status. */
-	sg_init_one(&stat, &status, sizeof(status));
+	sg_init_one(&stat, &buf->status, sizeof(buf->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;
+	if (unlikely(!virtqueue_kick(vi->cvq))) {
+		ret = (buf->status == VIRTIO_NET_OK);
+		goto out;
+	}
 
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.

@@ -1009,7 +1021,11 @@ 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;
+	ret = (buf->status == VIRTIO_NET_OK);
+
+out:
+	kfree(buf);
+	return ret;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)

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

@@ -1163,22 +1179,25 @@ 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);
+	cmdbyte = kmalloc(sizeof(*cmdbyte), GFP_ATOMIC);
+	if (!cmdbyte)
+		return;
 
-	sg_init_one(sg, &promisc, sizeof(promisc));
+	sg_init_one(sg, cmdbyte, sizeof(*cmdbyte));
 
+	*cmdbyte = ((dev->flags & IFF_PROMISC) != 0);
 	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");
-
-	sg_init_one(sg, &allmulti, sizeof(allmulti));
+			 *cmdbyte ? "en" : "dis");
 
+	*cmdbyte = ((dev->flags & IFF_ALLMULTI) != 0);
 	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");
+			 *cmdbyte ? "en" : "dis");
+
+	kfree(cmdbyte);
 
 	uc_count = netdev_uc_count(dev);
 	mc_count = netdev_mc_count(dev);

Signature · 1 line

2.4.3

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.

Cc "Michael S. Tsirkin" <[email protected]>

Cc [email protected]

Reviewed-by Joerg Roedel <[email protected]>

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

---

Hi Michael and DaveM-

This is a prerequisite for the virtio DMA fixing project. It works as a standalone patch, though. Would it make sense to apply it to an appropriate networking tree now?

(This is unchanged from v2.)

drivers/net/virtio_net.c | 53 ++++++++++++++++++++++++++++++++----------------

 1 file changed, 36 insertions(+), 17 deletions(-)

drivers/net/virtio_net.c +36 -17

index d8838dedb7a4..4f10f8a58811 100644
@@ -976,31 +976,43 @@ 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;
+
+	struct {
+		struct virtio_net_ctrl_hdr ctrl;
+		virtio_net_ctrl_ack status;
+	} *buf;
+
 	unsigned out_num = 0, tmp;
+	bool ret;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-	ctrl.class = class;
-	ctrl.cmd = cmd;
+	buf = kmalloc(sizeof(*buf), GFP_ATOMIC);
+	if (!buf)
+		return false;
+	buf->status = ~0;
+
+	buf->ctrl.class = class;
+	buf->ctrl.cmd = cmd;
 	/* Add header */
-	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
+	sg_init_one(&hdr, &buf->ctrl, sizeof(buf->ctrl));
 	sgs[out_num++] = &hdr;
 
 	if (out)
 		sgs[out_num++] = out;
 
 	/* Add return status. */
-	sg_init_one(&stat, &status, sizeof(status));
+	sg_init_one(&stat, &buf->status, sizeof(buf->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;
+	if (unlikely(!virtqueue_kick(vi->cvq))) {
+		ret = (buf->status == VIRTIO_NET_OK);
+		goto out;
+	}
 
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.

@@ -1009,7 +1021,11 @@ 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;
+	ret = (buf->status == VIRTIO_NET_OK);
+
+out:
+	kfree(buf);
+	return ret;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)

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

@@ -1163,22 +1179,25 @@ 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);
+	cmdbyte = kmalloc(sizeof(*cmdbyte), GFP_ATOMIC);
+	if (!cmdbyte)
+		return;
 
-	sg_init_one(sg, &promisc, sizeof(promisc));
+	sg_init_one(sg, cmdbyte, sizeof(*cmdbyte));
 
+	*cmdbyte = ((dev->flags & IFF_PROMISC) != 0);
 	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");
-
-	sg_init_one(sg, &allmulti, sizeof(allmulti));
+			 *cmdbyte ? "en" : "dis");
 
+	*cmdbyte = ((dev->flags & IFF_ALLMULTI) != 0);
 	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");
+			 *cmdbyte ? "en" : "dis");
+
+	kfree(cmdbyte);
 
 	uc_count = netdev_uc_count(dev);
 	mc_count = netdev_mc_count(dev);

Signature · 1 line

2.4.3

Changes from v2:
- Fix really embarrassing bug. This version actually works.

Quoted 3 lines · Andy Lutomirski

Changes from v2:
- Fix really embarrassing bug. This version actually works.

So embarrassing you didn't want to tell us what it was? ...

drivers/virtio/virtio_ring.c +1 -1

@@ -292,7 +292,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
                        vq, desc, total_sg * sizeof(struct vring_desc),
                        DMA_TO_DEVICE);
 
-               if (vring_mapping_error(vq, vq->vring.desc[head].addr))
+               if (vring_mapping_error(vq, addr))
                        goto unmap_release;
 
                vq->vring.desc[head].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_INDIRECT);

That wasn't going to be the reason for Christian's failure, was it?

Quoted 3 lines · Andy Lutomirski

Changes from v2:
- Fix really embarrassing bug. This version actually works.

So embarrassing you didn't want to tell us what it was? ...

drivers/virtio/virtio_ring.c +1 -1

@@ -292,7 +292,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
                        vq, desc, total_sg * sizeof(struct vring_desc),
                        DMA_TO_DEVICE);
 
-               if (vring_mapping_error(vq, vq->vring.desc[head].addr))
+               if (vring_mapping_error(vq, addr))
                        goto unmap_release;
 
                vq->vring.desc[head].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_INDIRECT);

That wasn't going to be the reason for Christian's failure, was it?

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.

Changes from v2:
- Fix really embarrassing bug. This version actually works.

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 (3):
virtio_net: Stop doing DMA from the stack
virtio_ring: Support DMA APIs
virtio_pci: Use the DMA API

 drivers/net/virtio_net.c           |  53 +++++++----
 drivers/virtio/Kconfig             |   2 +-
 drivers/virtio/virtio_pci_common.h |   3 +-
 drivers/virtio/virtio_pci_legacy.c |  19 +++-
 drivers/virtio/virtio_pci_modern.c |  34 +++++--
 drivers/virtio/virtio_ring.c       | 187 ++++++++++++++++++++++++++++++-------
 tools/virtio/linux/dma-mapping.h   |  17 ++++
 7 files changed, 246 insertions(+), 69 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

Signature · 1 line

2.4.3

Quoted 4 lines

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.

I'm very glad to see work on this making progress.

I suspect we'll have to find a way to make this optional though, and keep doing the non-DMA API thing with old devices. And I've been debating with myself whether a pci specific thing or a feature bit is preferable.

Thoughts?

Quoted 25 lines

Changes from v2:
- Fix really embarrassing bug. This version actually works.

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 (3):
virtio_net: Stop doing DMA from the stack
virtio_ring: Support DMA APIs
virtio_pci: Use the DMA API

 drivers/net/virtio_net.c           |  53 +++++++----
 drivers/virtio/Kconfig             |   2 +-
 drivers/virtio/virtio_pci_common.h |   3 +-
 drivers/virtio/virtio_pci_legacy.c |  19 +++-
 drivers/virtio/virtio_pci_modern.c |  34 +++++--
 drivers/virtio/virtio_ring.c       | 187 ++++++++++++++++++++++++++++++-------
 tools/virtio/linux/dma-mapping.h   |  17 ++++
 7 files changed, 246 insertions(+), 69 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

Signature · 1 line

2.4.3

Am 28.10.2015 um 16:17 schrieb Michael S. Tsirkin:

Quoted 6 lines · Andy Lutomirski

Quoted 4 lines

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.

Quoted 7 lines

I'm very glad to see work on this making progress.

I suspect we'll have to find a way to make this optional though, and keep doing the non-DMA API thing with old devices. And I've been debating with myself whether a pci specific thing or a feature bit is preferable.

We have discussed that at kernel summit. I will try to implement a dummy dma_ops for s390 that does 1:1 mapping and Ben will look into doing some quirk to handle "old" code in addition to also make it possible to mark devices as iommu bypass (IIRC, via device tree, Ben?)

Quoted 15 lines · Christian Borntraeger

Am 28.10.2015 um 16:17 schrieb Michael S. Tsirkin:

Quoted 6 lines · Andy Lutomirski

Quoted 4 lines

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.

Quoted 7 lines

I'm very glad to see work on this making progress.

I suspect we'll have to find a way to make this optional though, and keep doing the non-DMA API thing with old devices. And I've been debating with myself whether a pci specific thing or a feature bit is preferable.

Quoted 4 lines

We have discussed that at kernel summit. I will try to implement a dummy dma_ops for s390 that does 1:1 mapping and Ben will look into doing some quirk to handle "old" code in addition to also make it possible to mark devices as iommu bypass (IIRC, via device tree, Ben?)

Right. You never eschew the DMA API in the *driver* — you just expect the DMA API to do the right thing for devices which don't need translation (with platforms using per-device dma_ops and generally getting their act together).
We're pushing that on the platforms where it's currently an issue, including Power, SPARC and S390.

Signature · 1 line

dwmw2

E.g. on intel x86, there's an option iommu=pt which does the 1:1 thing for devices when used by kernel, but enables
the iommu if used by userspace/VMs.

Quoted 3 lines · Michael S. Tsirkin

E.g. on intel x86, there's an option iommu=pt which does the 1:1 thing for devices when used by kernel, but enables
the iommu if used by userspace/VMs.

That's none of your business.

You call the DMA API when you do DMA. That's all there is to it.

If the IOMMU happens to be in passthrough mode, or your device happens to not to be routed through an IOMMU today, then I/O virtual address you get back from the DMA API will look a *lot* like the physical address you asked the DMA to map. You might think there's no IOMMU. We couldn't possibly comment.

Use the DMA API. Always. Let the platform worry about whether it actually needs to *do* anything or not.

Signature · 1 line

dwmw2

Short answer - platforms need a way to discover, and express different security requirements of different devices.

If they continue to lack that, we'll need a custom API in virtio, and while this seems a bit less elegant, I would not see that as the end of the world at all, there are not that many virtio drivers.

Quoted 4 lines

Quoted 2 lines

OK so I guess that means we should prefer a transport-specific interface in virtio-pci then.

Why?

Because you said you are doing something device tree specific for ARM, aren't you?

Quoted 3 lines · Michael S. Tsirkin

Short answer - platforms need a way to discover, and express different security requirements of different devices.

Sure. PLATFORMS need that. Do not let it go anywhere near your device drivers. Including the virtio drivers.

Quoted 3 lines

If they continue to lack that, we'll need a custom API in virtio, and while this seems a bit less elegant, I would not see that as the end of the world at all, there are not that many virtio drivers.

No. If they continue to lack that, we fix them. This is a *platform* issue. The DMA API shall do the right thing. Do not second-guess it.

(From the other mail)

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

OK so I guess that means we should prefer a transport-specific interface in virtio-pci then.

Why?

Quoted 2 lines

Because you said you are doing something device tree specific for ARM, aren't you?

Nonono. The ARM platform code might do that, and the DMA API on ARM *might* give you I/O virtual addresses that look a lot like the physical addresses you asked it to map. That's none of your business. Drivers use DMA API. No more talky.

Signature · 1 line

dwmw2

Quoted 6 lines · David Woodhouse

Quoted 4 lines · Michael S. Tsirkin

Short answer - platforms need a way to discover, and express different
security requirements of different devices.

Quoted 3 lines

Sure. PLATFORMS need that. Do not let it go anywhere near your device
drivers. Including the virtio drivers.

But would there be any users of this outside the virtio subsystem? If no, maybe virtio core is a logical place to keep this.

I don't have a problem with extending DMA API to address more usecases.

Quoted 1 line

Drivers use DMA API. No more talky.

Well for virtio they don't ATM. And 1:1 mapping makes perfect sense for the wast majority of users, so I can't switch them over until the DMA API actually addresses all existing usecases.

Quoted 2 lines

I don't have a problem with extending DMA API to address more usecases.

No, this isn't an extension. This is fixing a bug, on certain platforms where the DMA API has currently done the wrong thing.

We have historically worked around that bug by introducing *another* bug, which is not to *use* the DMA API in the virtio driver.

Sure, we can co-ordinate those two bug-fixes. But let's not talk about them as anything other than bug-fixes.

Quoted 5 lines

Quoted 1 line

Drivers use DMA API. No more talky.

Well for virtio they don't ATM. And 1:1 mapping makes perfect sense for the wast majority of users, so I can't switch them over until the DMA API actually addresses all existing usecases.

That's still not your business; it's the platform's. And there are hardware implementations of the virtio protocols on real PCI cards. And we have the option of doing IOMMU translation for the virtio devices even in a virtual machine. Just don't get involved.

Signature · 1 line

dwmw2

Quoted 11 lines · David Woodhouse

Quoted 2 lines

I don't have a problem with extending DMA API to address more usecases.

No, this isn't an extension. This is fixing a bug, on certain platforms where the DMA API has currently done the wrong thing.

We have historically worked around that bug by introducing *another* bug, which is not to *use* the DMA API in the virtio driver.

Sure, we can co-ordinate those two bug-fixes. But let's not talk about them as anything other than bug-fixes.

It was pretty practical not to use it. All virtio devices at the time without exception bypassed the IOMMU, so it was a question of omitting a couple of function calls in virtio versus hacking on DMA implementation on multiple platforms. We have more policy options now, so I agree it's time to revisit this.

But for me, the most important thing is that we do coordinate.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Drivers use DMA API. No more talky.

Well for virtio they don't ATM. And 1:1 mapping makes perfect sense for the wast majority of users, so I can't switch them over until the DMA API actually addresses all existing usecases.

Quoted 9 lines

That's still not your business; it's the platform's. And there are hardware implementations of the virtio protocols on real PCI cards. And we have the option of doing IOMMU translation for the virtio devices even in a virtual machine. Just don't get involved.

--
dwmw2

I'm involved anyway, it's possible not to put all the code in the virtio subsystem in guest though. But I suspect we'll need to find a way for non-linux drivers within guest to work correctly too, and they might have trouble poking at things at the system level. So possibly virtio subsystem will have to tell platform "this device wants to bypass IOMMU" and then DMA API does the right thing.

Quoted 37 lines · Michael S. Tsirkin

Quoted 11 lines · David Woodhouse

Quoted 2 lines

I don't have a problem with extending DMA API to address more usecases.

No, this isn't an extension. This is fixing a bug, on certain platforms where the DMA API has currently done the wrong thing.

We have historically worked around that bug by introducing *another* bug, which is not to *use* the DMA API in the virtio driver.

Sure, we can co-ordinate those two bug-fixes. But let's not talk about them as anything other than bug-fixes.

It was pretty practical not to use it. All virtio devices at the time without exception bypassed the IOMMU, so it was a question of omitting a couple of function calls in virtio versus hacking on DMA implementation on multiple platforms. We have more policy options now, so I agree it's time to revisit this.

But for me, the most important thing is that we do coordinate.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Drivers use DMA API. No more talky.

Well for virtio they don't ATM. And 1:1 mapping makes perfect sense for the wast majority of users, so I can't switch them over until the DMA API actually addresses all existing usecases.

Quoted 9 lines

That's still not your business; it's the platform's. And there are hardware implementations of the virtio protocols on real PCI cards. And we have the option of doing IOMMU translation for the virtio devices even in a virtual machine. Just don't get involved.

--
dwmw2

Quoted 7 lines

I'm involved anyway, it's possible not to put all the code in the virtio subsystem in guest though. But I suspect we'll need to find a way for non-linux drivers within guest to work correctly too, and they might have trouble poking at things at the system level. So possibly virtio subsystem will have to tell platform "this device wants to bypass IOMMU" and then DMA API does the right thing.

After some discussion at KS, no one came up with an example where it's necessary, and the patches to convert virtqueue to use the DMA API are much nicer when they convert it unconditionally.

The two interesting cases we thought of were PPC and x86's emulated Q35 IOMMU. PPC will look in to architecting a devicetree-based way to indicate passthrough status and will add quirks for the existing virtio devices.

Everyone seems to agree that x86's emulated Q35 thing
is just buggy right now and should be taught to use the existing ACPI mechanism for enumerating passthrough devices.

I'll send a new version of the series soon.

--Andy

Example: you have a mix of assigned devices and virtio devices. You don't trust your assigned device vendor not to corrupt your memory so you want to limit the damage your assigned device can do to your guest,
so you use an IOMMU for that. Thus existing iommu=pt within guest is out.

But you trust your hypervisor (you have no choice anyway), and you don't want the overhead of tweaking IOMMU on data path for virtio. Thus iommu=on is out too.

Quoted 8 lines · Michael S. Tsirkin

Example: you have a mix of assigned devices and virtio devices. You don't trust your assigned device vendor not to corrupt your memory so you want to limit the damage your assigned device can do to your guest,
so you use an IOMMU for that. Thus existing iommu=pt within guest is out.

Quoted 3 lines

But you trust your hypervisor (you have no choice anyway), and you don't want the overhead of tweaking IOMMU on data path for virtio. Thus iommu=on is out too.

That's not at all special for virtio or guest VMs. Even with real hardware, we might want performance from *some* devices, and security from others. See the DMA_ATTR_IOMMU_BYPASS which is currently being discussed.

But of course the easy answer in *your* case it just to ask the hypervisor not to put the virtio devices behind an IOMMU at all. Which we were planning to remain the default behaviour.

In all cases, the DMA API shall do the right thing.

Signature · 1 line

dwmw2

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Quoted 4 lines · Michael S. Tsirkin

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Well, the only incompatibility comes from an experimental qemu feature, more explicitly from a bug in that features implementation. So why should we work around that in the kernel? I think it is not too hard to fix qemu to generate a correct DMAR table which excludes the virtio devices from iommu translation.

Joerg

Quoted 6 lines · Joerg Roedel

Quoted 4 lines · Michael S. Tsirkin

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Quoted 8 lines

Well, the only incompatibility comes from an experimental qemu feature, more explicitly from a bug in that features implementation. So why should we work around that in the kernel? I think it is not too hard to fix qemu to generate a correct DMAR table which excludes the virtio devices from iommu translation.

Joerg

It's not that easy - you'd have to dedicate some buses for iommu bypass, and teach management tools to only put virtio there - but it's possible.

This will absolutely address guests that don't need to set up IOMMU for virtio devices, and virtio that bypasses the IOMMU.

But the problem is that we do want to *allow* guests to set up IOMMU for virtio devices.
In that case, these are two other usecases:

A- monolitic virtio within QEMU:
iommu only needed for VFIO ->
guest should always use iommu=pt iommu=on works but is just useless overhead.

B- modular out of process virtio outside QEMU:
iommu needed for VFIO or kernel driver -> guest should use iommu=pt or iommu=on depending on security/performance requirements

Note that there could easily be a mix of these in the same system.

So for these cases we do need QEMU to specify to guest that IOMMU covers the virtio devices. Also, once one does this, the default on linux is iommu=on and not pt, which works but ATM is very slow.

This poses three problems:

1. How do we address the different needs of A and B? One way would be for virtio to pass the information to guest using some virtio specific way, and have drivers specify what kind of DMA access they want.

2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests use the more sensible iommu=pt.

3. Once we do allow IOMMU, how can we keep existing guests work in this configuration? Creating different hypervisor configurations depending on guest is very nasty. Again, one way would be some virtio specific interface.

I'd rather we figured the answers to this before merging Andy's patches because I'm concerned that instead of 1 broken configuration (virtio always bypasses IOMMU) we'll get two bad configurations (in the second one, virtio uses the slow default with no gain in security).

Suggestions wellcome.

--
MST

Quoted 64 lines · Michael S. Tsirkin

Quoted 6 lines · Joerg Roedel

Quoted 4 lines · Michael S. Tsirkin

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Quoted 8 lines

Well, the only incompatibility comes from an experimental qemu feature, more explicitly from a bug in that features implementation. So why should we work around that in the kernel? I think it is not too hard to fix qemu to generate a correct DMAR table which excludes the virtio devices from iommu translation.

Joerg

It's not that easy - you'd have to dedicate some buses for iommu bypass, and teach management tools to only put virtio there - but it's possible.

This will absolutely address guests that don't need to set up IOMMU for virtio devices, and virtio that bypasses the IOMMU.

But the problem is that we do want to *allow* guests to set up IOMMU for virtio devices.
In that case, these are two other usecases:

A- monolitic virtio within QEMU:
iommu only needed for VFIO ->
guest should always use iommu=pt iommu=on works but is just useless overhead.

B- modular out of process virtio outside QEMU:
iommu needed for VFIO or kernel driver -> guest should use iommu=pt or iommu=on depending on security/performance requirements

Note that there could easily be a mix of these in the same system.

So for these cases we do need QEMU to specify to guest that IOMMU covers the virtio devices. Also, once one does this, the default on linux is iommu=on and not pt, which works but ATM is very slow.

This poses three problems:

1. How do we address the different needs of A and B? One way would be for virtio to pass the information to guest using some virtio specific way, and have drivers specify what kind of DMA access they want.

2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests use the more sensible iommu=pt.

3. Once we do allow IOMMU, how can we keep existing guests work in this configuration? Creating different hypervisor configurations depending on guest is very nasty. Again, one way would be some virtio specific interface.

I'd rather we figured the answers to this before merging Andy's patches because I'm concerned that instead of 1 broken configuration (virtio always bypasses IOMMU) we'll get two bad configurations (in the second one, virtio uses the slow default with no gain in security).

Quoted 1 line

Suggestions wellcome.

I think there's still no downside of using my patches, even on x86.

Old kernels on new QEMU work unless IOMMU is enabled on the host. I think that's the best we can possibly do.
New kernels work at full speed on old QEMU.

New kernels with new QEMU and iommu enabled work slower. Even newer kernels with default passthrough work at full speed, and there's no obvious downside to the existence of kernels with just my patches.

--Andy

Quoted 3 lines

--
MST

Quoted 65 lines · Andy Lutomirski

Quoted 64 lines · Michael S. Tsirkin

Quoted 6 lines · Joerg Roedel

Quoted 4 lines · Michael S. Tsirkin

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Quoted 8 lines

Well, the only incompatibility comes from an experimental qemu feature, more explicitly from a bug in that features implementation. So why should we work around that in the kernel? I think it is not too hard to fix qemu to generate a correct DMAR table which excludes the virtio devices from iommu translation.

Joerg

It's not that easy - you'd have to dedicate some buses for iommu bypass, and teach management tools to only put virtio there - but it's possible.

This will absolutely address guests that don't need to set up IOMMU for virtio devices, and virtio that bypasses the IOMMU.

But the problem is that we do want to *allow* guests to set up IOMMU for virtio devices.
In that case, these are two other usecases:

A- monolitic virtio within QEMU:
iommu only needed for VFIO ->
guest should always use iommu=pt iommu=on works but is just useless overhead.

B- modular out of process virtio outside QEMU:
iommu needed for VFIO or kernel driver -> guest should use iommu=pt or iommu=on depending on security/performance requirements

Note that there could easily be a mix of these in the same system.

So for these cases we do need QEMU to specify to guest that IOMMU covers the virtio devices. Also, once one does this, the default on linux is iommu=on and not pt, which works but ATM is very slow.

This poses three problems:

1. How do we address the different needs of A and B? One way would be for virtio to pass the information to guest using some virtio specific way, and have drivers specify what kind of DMA access they want.

2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests use the more sensible iommu=pt.

3. Once we do allow IOMMU, how can we keep existing guests work in this configuration? Creating different hypervisor configurations depending on guest is very nasty. Again, one way would be some virtio specific interface.

I'd rather we figured the answers to this before merging Andy's patches because I'm concerned that instead of 1 broken configuration (virtio always bypasses IOMMU) we'll get two bad configurations (in the second one, virtio uses the slow default with no gain in security).

Quoted 7 lines

Quoted 1 line

Suggestions wellcome.

I think there's still no downside of using my patches, even on x86.

Old kernels on new QEMU work unless IOMMU is enabled on the host. I think that's the best we can possibly do.
New kernels work at full speed on old QEMU.

Only if IOMMU is disabled, right?

Quoted 6 lines

New kernels with new QEMU and iommu enabled work slower. Even newer kernels with default passthrough work at full speed, and there's no obvious downside to the existence of kernels with just my patches.

--Andy

I tried to explain the possible downside. Let me try again. Imagine that guest kernel notifies hypervisor that it wants IOMMU to actually work. This will make old kernel on new QEMU work even with IOMMU enabled on host - better than "the best we can do" that you described above. Specifically, QEMU will assume that if it didn't get notification, it's an old kernel so it should ignore the IOMMU.

But if we apply your patches this trick won't work.

Without implementing it all, I think the easiest incremental step would be to teach linux to make passthrough the default when running as a guest on top of QEMU, put your patches on top. If someone specifies non passthrough on command line it'll still be broken,
but not too bad.

Can you flesh out this trick?

On x86 IIUC the IOMMU more-or-less defaults to passthrough. If the kernel wants, it can switch it to a non-passthrough mode. My patches cause the virtio driver to do exactly this, except that the host implementation doesn't actually exist yet, so the patches will instead have no particular effect.

On powerpc and sparc, we *already* screwed up. The host already tells the guest that there's an IOMMU and that it's *enabled* because those platforms don't have selective IOMMU coverage the way that x86 does. So we need to work around it.

I think that, if we want fancy virt-friendly IOMMU stuff like you're talking about, then the right thing to do is to create a virtio bus instead of pretending to be PCI. That bus could have a virtio IOMMU and its own cross-platform enumeration mechanism for devices on the bus, and everything would be peachy.

I still don't understand what trick. If we want virtio devices to be assignable, then they should be translated through the IOMMU, and the DMA API is the right interface for that.

Quoted 8 lines · Andy Lutomirski

Can you flesh out this trick?

On x86 IIUC the IOMMU more-or-less defaults to passthrough. If the kernel wants, it can switch it to a non-passthrough mode. My patches cause the virtio driver to do exactly this, except that the host implementation doesn't actually exist yet, so the patches will instead have no particular effect.

At some level, yes — we're compatible with a 1982 IBM PC and thus the IOMMU is entirely disabled at boot until the kernel turns it on — except in TXT mode where we abandon that compatibility.

But no, the virtio driver has *nothing* to do with switching the device out of passthrough mode. It is either in passthrough mode, or it isn't.

If the VMM *doesn't* expose an IOMMU to the guest, obviously the devices are in passthrough mode. If the guest kernel doesn't have IOMMU support enabled, then obviously the devices are in passthrough mode. And if the ACPI tables exposed to the guest kernel *tell* it that the virtio devices are not actually behind the IOMMU (which qemu gets wrong), then it'll be in passthrough mode.

If the IOMMU is exposed, and enabled, and telling the guest kernel that it *does* cover the virtio devices, then those virtio devices will *not* be in passthrough mode.

You choosing to use the DMA API in the virtio device drivers instead of being buggy, has nothing to do with whether it's actually in passthrough mode or not. Whether it's in passthrough mode or not, using the DMA API is technically the right thing to do — because it should either *do* the translation, or return a 1:1 mapped IOVA, as appropriate.

Quoted 4 lines

On powerpc and sparc, we *already* screwed up. The host already tells the guest that there's an IOMMU and that it's *enabled* because those platforms don't have selective IOMMU coverage the way that x86 does. So we need to work around it.

No, we need it on x86 too because once we fix the virtio device driver bug and make it start using the DMA API, then we start to trip up on the qemu bug where it lies about which devices are covered by the IOMMU.

Of course, we still have that same qemu bug w.r.t. assigned devices, which it *also* claims are behind its IOMMU when they're not...

Quoted 5 lines

I think that, if we want fancy virt-friendly IOMMU stuff like you're talking about, then the right thing to do is to create a virtio bus instead of pretending to be PCI. That bus could have a virtio IOMMU and its own cross-platform enumeration mechanism for devices on the bus, and everything would be peachy.

That doesn't really help very much for the x86 case where the problem is compatibility with *existing* (arguably broken) qemu implementations.

Having said that, if this were real hardware I'd just be blacklisting it and saying "Another BIOS with broken DMAR tables --> IOMMU completely disabled". So perhaps we should just do that.

Quoted 3 lines

I still don't understand what trick. If we want virtio devices to be assignable, then they should be translated through the IOMMU, and the DMA API is the right interface for that.

The DMA API is the right interface *regardless* of whether there's actual translation to be done. The device driver itself should not be involved in any way with that decision.

When you want to access MMIO, you use ioremap() and writel() instead of doing random crap for yourself. When you want DMA, you use the DMA API to get a bus address for your device *even* if you expect there to be no IOMMU and you expect it to precisely match the physical address. No excuses.

Signature · 1 line

dwmw2

Quoted 4 lines · David Woodhouse

If the IOMMU is exposed, and enabled, and telling the guest kernel that it *does* cover the virtio devices, then those virtio devices will *not* be in passthrough mode.

This we need to fix. Because in most configurations if you are using kernel drivers, then you don't want IOMMU with virtio, but if you are using VFIO then you do.

Quoted 6 lines

You choosing to use the DMA API in the virtio device drivers instead of being buggy, has nothing to do with whether it's actually in passthrough mode or not. Whether it's in passthrough mode or not, using the DMA API is technically the right thing to do — because it should either *do* the translation, or return a 1:1 mapped IOVA, as appropriate.

Right but first we need to actually make DMA API do the right thing at least on x86,ppc and arm.

I'm not worried about qemu bugs that much.  I am interested in being able to use both VFIO and kernel drivers with virtio devices with good performance and without tweaking kernel parameters.

Quoted 4 lines

Having said that, if this were real hardware I'd just be blacklisting it and saying "Another BIOS with broken DMAR tables --> IOMMU completely disabled". So perhaps we should just do that.

Yes, once there is new QEMU where virtio is covered by the IOMMU, that would be one way to address existing QEMU bugs.

With virt, each device can have different priveledges:
some are part of hypervisor so with a kernel driver trying to get protection from them using an IOMMU which is also part of hypervisor makes no sense
- but when using a
userspace driver then getting protection from the userspace driver does make sense. Others are real devices so getting protection from them makes some sense.

Which is which? It's easiest for the device driver itself to gain that knowledge. Please note this is *not* the same question as whether a specific device is covered by an IOMMU.

Linux doesn't seem to support that usecase at the moment, if this is a generic problem then we need to teach Linux to solve it, but if virtio is unique in this requirement, then we should just keep doing virtio specific things to solve it.

Can you flesh out this trick?

On x86 IIUC the IOMMU more-or-less defaults to passthrough. If the kernel wants, it can switch it to a non-passthrough mode. My patches cause the virtio driver to do exactly this, except that the host implementation doesn't actually exist yet, so the patches will instead have no particular effect.

On powerpc and sparc, we *already* screwed up. The host already tells the guest that there's an IOMMU and that it's *enabled* because those platforms don't have selective IOMMU coverage the way that x86 does. So we need to work around it.

I think that, if we want fancy virt-friendly IOMMU stuff like you're talking about, then the right thing to do is to create a virtio bus instead of pretending to be PCI. That bus could have a virtio IOMMU and its own cross-platform enumeration mechanism for devices on the bus, and everything would be peachy.

I still don't understand what trick. If we want virtio devices to be assignable, then they should be translated through the IOMMU, and the DMA API is the right interface for that.

Quoted 65 lines · Andy Lutomirski

Quoted 64 lines · Michael S. Tsirkin

Quoted 6 lines · Joerg Roedel

Quoted 4 lines · Michael S. Tsirkin

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Quoted 8 lines

Well, the only incompatibility comes from an experimental qemu feature, more explicitly from a bug in that features implementation. So why should we work around that in the kernel? I think it is not too hard to fix qemu to generate a correct DMAR table which excludes the virtio devices from iommu translation.

Joerg

It's not that easy - you'd have to dedicate some buses for iommu bypass, and teach management tools to only put virtio there - but it's possible.

This will absolutely address guests that don't need to set up IOMMU for virtio devices, and virtio that bypasses the IOMMU.

But the problem is that we do want to *allow* guests to set up IOMMU for virtio devices.
In that case, these are two other usecases:

A- monolitic virtio within QEMU:
iommu only needed for VFIO ->
guest should always use iommu=pt iommu=on works but is just useless overhead.

B- modular out of process virtio outside QEMU:
iommu needed for VFIO or kernel driver -> guest should use iommu=pt or iommu=on depending on security/performance requirements

Note that there could easily be a mix of these in the same system.

So for these cases we do need QEMU to specify to guest that IOMMU covers the virtio devices. Also, once one does this, the default on linux is iommu=on and not pt, which works but ATM is very slow.

This poses three problems:

1. How do we address the different needs of A and B? One way would be for virtio to pass the information to guest using some virtio specific way, and have drivers specify what kind of DMA access they want.

2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests use the more sensible iommu=pt.

3. Once we do allow IOMMU, how can we keep existing guests work in this configuration? Creating different hypervisor configurations depending on guest is very nasty. Again, one way would be some virtio specific interface.

I'd rather we figured the answers to this before merging Andy's patches because I'm concerned that instead of 1 broken configuration (virtio always bypasses IOMMU) we'll get two bad configurations (in the second one, virtio uses the slow default with no gain in security).

Quoted 7 lines

Quoted 1 line

Suggestions wellcome.

I think there's still no downside of using my patches, even on x86.

Old kernels on new QEMU work unless IOMMU is enabled on the host. I think that's the best we can possibly do.
New kernels work at full speed on old QEMU.

Only if IOMMU is disabled, right?

Quoted 6 lines

New kernels with new QEMU and iommu enabled work slower. Even newer kernels with default passthrough work at full speed, and there's no obvious downside to the existence of kernels with just my patches.

--Andy

I tried to explain the possible downside. Let me try again. Imagine that guest kernel notifies hypervisor that it wants IOMMU to actually work. This will make old kernel on new QEMU work even with IOMMU enabled on host - better than "the best we can do" that you described above. Specifically, QEMU will assume that if it didn't get notification, it's an old kernel so it should ignore the IOMMU.

But if we apply your patches this trick won't work.

Without implementing it all, I think the easiest incremental step would be to teach linux to make passthrough the default when running as a guest on top of QEMU, put your patches on top. If someone specifies non passthrough on command line it'll still be broken,
but not too bad.

Quoted 4 lines · Michael S. Tsirkin

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Well, the only incompatibility comes from an experimental qemu feature, more explicitly from a bug in that features implementation. So why should we work around that in the kernel? I think it is not too hard to fix qemu to generate a correct DMAR table which excludes the virtio devices from iommu translation.

Joerg

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

I tried to generate a DMAR table that excludes some devices from IOMMU translation, however it does not help.

The reason is, as far as I understand, that Linux kernel does not allow any device being outside an IOMMU scope if the iommu kernel option is activated.

Does anybody know if it is "by design" or is simply an uncommon configuration?
(some devices in an IOMMU scope, while others outside *any* IOMMU scope)

Quoted 5 lines · Marcel Apfelbaum

I tried to generate a DMAR table that excludes some devices from IOMMU translation, however it does not help.

Quoted 8 lines

The reason is, as far as I understand, that Linux kernel does not allow any device being outside an IOMMU scope if the iommu kernel option is activated.

Does anybody know if it is "by design" or is simply an uncommon configuration?
(some devices in an IOMMU scope, while others outside *any* IOMMU scope)

That's a kernel bug in the way it handles per-device DMA operations. Or more to the point, in the way it doesn't — the non-translated devices end up being pointed to the intel_dma_ops despite the fact they shouldn't be. I'm working on that...

Quoted 5 lines · Marcel Apfelbaum

I tried to generate a DMAR table that excludes some devices from IOMMU translation, however it does not help.

Quoted 8 lines

The reason is, as far as I understand, that Linux kernel does not allow any device being outside an IOMMU scope if the iommu kernel option is activated.

Does anybody know if it is "by design" or is simply an uncommon configuration?
(some devices in an IOMMU scope, while others outside *any* IOMMU scope)

That's a kernel bug in the way it handles per-device DMA operations. Or more to the point, in the way it doesn't — the non-translated devices end up being pointed to the intel_dma_ops despite the fact they shouldn't be. I'm working on that...

Quoted 5 lines · Marcel Apfelbaum

I tried to generate a DMAR table that excludes some devices from IOMMU translation, however it does not help.

Quoted 8 lines

The reason is, as far as I understand, that Linux kernel does not allow any device being outside an IOMMU scope if the iommu kernel option is activated.

Does anybody know if it is "by design" or is simply an uncommon configuration?
(some devices in an IOMMU scope, while others outside *any* IOMMU scope)

That's a kernel bug in the way it handles per-device DMA operations. Or more to the point, in the way it doesn't — the non-translated devices end up being pointed to the intel_dma_ops despite the fact they shouldn't be. I'm working on that...

Signature · 1 line

dwmw2

Quoted 6 lines · David Woodhouse

Quoted 4 lines · Michael S. Tsirkin

But you trust your hypervisor (you have no choice anyway), and you don't want the overhead of tweaking IOMMU on data path for virtio. Thus iommu=on is out too.

Quoted 4 lines

That's not at all special for virtio or guest VMs. Even with real hardware, we might want performance from *some* devices, and security from others. See the DMA_ATTR_IOMMU_BYPASS which is currently being discussed.

Right. So let's wait for that discussion to play out?

Quoted 3 lines

But of course the easy answer in *your* case it just to ask the hypervisor not to put the virtio devices behind an IOMMU at all. Which we were planning to remain the default behaviour.

One can't do this for x86 ATM, can one?

Quoted 1 line

In all cases, the DMA API shall do the right thing.

I have no problem with that. For example, can we teach the DMA API on intel x86 to use PT for virtio by default? That would allow merging Andy's patches with full compatibility with old guests and hosts.

Example: you have a mix of assigned devices and virtio devices. You don't trust your assigned device vendor not to corrupt your memory so you want to limit the damage your assigned device can do to your guest, so you use an IOMMU for that. Thus existing iommu=pt within guest is out.

But you trust your hypervisor (you have no choice anyway), and you don't want the overhead of tweaking IOMMU on data path for virtio. Thus iommu=on is out too.

I'm not sure what ACPI has to do with it. It's about a way for guest users to specify whether they want to bypass an IOMMU for a given device.

By the way, a bunch of code is missing on the QEMU side to make this useful:
1. virtio ignores the iommu
2. vhost user ignores the iommu
3. dataplane ignores the iommu
4. vhost-net ignores the iommu
5. VFIO ignores the iommu

Quoted 8 lines · Michael S. Tsirkin

Example: you have a mix of assigned devices and virtio devices. You don't trust your assigned device vendor not to corrupt your memory so you want to limit the damage your assigned device can do to your guest, so you use an IOMMU for that. Thus existing iommu=pt within guest is out.

But you trust your hypervisor (you have no choice anyway), and you don't want the overhead of tweaking IOMMU on data path for virtio. Thus iommu=on is out too.

IOMMUs on x86 usually come with an ACPI table that describes which IOMMUs are in the system and which devices they translate. So you can easily describe all devices there that are not behind an IOMMU.

The ACPI table is built by the BIOS, and the platform intialization code sets the device dma_ops accordingly. If the BIOS provides wrong information in the ACPI table this is a platform bug.

Quoted 3 lines

I'm not sure what ACPI has to do with it. It's about a way for guest users to specify whether they want to bypass an IOMMU for a given device.

We have no way yet to request passthrough-mode per-device from the IOMMU drivers, but that can easily be added. But as I see it:

Quoted 7 lines

By the way, a bunch of code is missing on the QEMU side to make this useful:
1. virtio ignores the iommu
2. vhost user ignores the iommu
3. dataplane ignores the iommu
4. vhost-net ignores the iommu
5. VFIO ignores the iommu

Qemu does not implement IOMMU translation for virtio devices anyway (which is fine), so it just should tell the guest so in the ACPI table built to describe the emulated IOMMU.

Joerg

Example: you have a mix of assigned devices and virtio devices. You don't trust your assigned device vendor not to corrupt your memory so you want to limit the damage your assigned device can do to your guest, so you use an IOMMU for that. Thus existing iommu=pt within guest is out.

But you trust your hypervisor (you have no choice anyway), and you don't want the overhead of tweaking IOMMU on data path for virtio. Thus iommu=on is out too.

I'm not sure what ACPI has to do with it. It's about a way for guest users to specify whether they want to bypass an IOMMU for a given device.

By the way, a bunch of code is missing on the QEMU side to make this useful:
1. virtio ignores the iommu
2. vhost user ignores the iommu
3. dataplane ignores the iommu
4. vhost-net ignores the iommu
5. VFIO ignores the iommu

Isn't this specified by the hypervisor? I don't think this is a good way to do this: guest security should be up to guest.

Quoted 3 lines

Everyone seems to agree that x86's emulated Q35 thing
is just buggy right now and should be taught to use the existing ACPI mechanism for enumerating passthrough devices.

I'm not sure what ACPI has to do with it. It's about a way for guest users to specify whether they want to bypass an IOMMU for a given device.

By the way, a bunch of code is missing on the QEMU side to make this useful:
1. virtio ignores the iommu
2. vhost user ignores the iommu
3. dataplane ignores the iommu
4. vhost-net ignores the iommu
5. VFIO ignores the iommu

Quoted 3 lines

Everyone seems to agree that x86's emulated Q35 thing
is just buggy right now and should be taught to use the existing ACPI mechanism for enumerating passthrough devices.

I'm not sure what ACPI has to do with it. It's about a way for guest users to specify whether they want to bypass an IOMMU for a given device.

1. virtio ignores the iommu
2. vhost user ignores the iommu
3. dataplane ignores the iommu
4. vhost-net ignores the iommu
5. VFIO ignores the iommu

I think so far I only saw patches for 1 above.

Quoted 3 lines

Everyone seems to agree that x86's emulated Q35 thing
is just buggy right now and should be taught to use the existing ACPI mechanism for enumerating passthrough devices.

I'm not sure what ACPI has to do with it. It's about a way for guest users to specify whether they want to bypass an IOMMU for a given device.

1. virtio ignores the iommu
2. vhost user ignores the iommu
3. dataplane ignores the iommu
4. vhost-net ignores the iommu
5. VFIO ignores the iommu

I think so far I only saw patches for 1 above.

Quoted 2 lines

I don't have a problem with extending DMA API to address more usecases.

No, this isn't an extension. This is fixing a bug, on certain platforms where the DMA API has currently done the wrong thing.

We have historically worked around that bug by introducing *another* bug, which is not to *use* the DMA API in the virtio driver.

Sure, we can co-ordinate those two bug-fixes. But let's not talk about them as anything other than bug-fixes.

Quoted 5 lines

Quoted 1 line

Drivers use DMA API. No more talky.

Well for virtio they don't ATM. And 1:1 mapping makes perfect sense for the wast majority of users, so I can't switch them over until the DMA API actually addresses all existing usecases.

That's still not your business; it's the platform's. And there are hardware implementations of the virtio protocols on real PCI cards. And we have the option of doing IOMMU translation for the virtio devices even in a virtual machine. Just don't get involved.

Signature · 1 line

dwmw2

We have discussed that at kernel summit. I will try to implement a dummy dma_ops for s390 that does 1:1 mapping and Ben will look into doing some quirk to handle "old" code in addition to also make it possible to mark devices as iommu bypass (IIRC, via device tree, Ben?)

Quoted 4 lines · Christian Borntraeger

We have discussed that at kernel summit. I will try to implement a dummy dma_ops for s390 that does 1:1 mapping and Ben will look into doing some quirk to handle "old" code in addition to also make it possible to mark devices as iommu bypass (IIRC, via device tree, Ben?)

Something like that yes. I'll look into it when I'm back home.

Cheers,
Ben.

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 7 lines · Christian Borntraeger

We have discussed that at kernel summit. I will try to implement a dummy dma_ops for
s390 that does 1:1 mapping and Ben will look into doing some quirk to handle "old"
code in addition to also make it possible to mark devices as iommu bypass (IIRC,
via device tree, Ben?)

Quoted 4 lines

Something like that yes. I'll look into it when I'm back home.

Cheers,
Ben.

OK so I guess that means we should prefer a transport-specific interface in virtio-pci then.

Quoted 13 lines · Michael S. Tsirkin

Quoted 11 lines · Benjamin Herrenschmidt

Quoted 7 lines · Christian Borntraeger

We have discussed that at kernel summit. I will try to implement a dummy dma_ops for
s390 that does 1:1 mapping and Ben will look into doing some quirk to handle "old"
code in addition to also make it possible to mark devices as iommu bypass (IIRC,
via device tree, Ben?)

Something like that yes. I'll look into it when I'm back home.

Quoted 5 lines

Quoted 2 lines

Cheers,
Ben.

OK so I guess that means we should prefer a transport-specific interface in virtio-pci then.

Why?

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.

Changes from v2:
- Fix really embarrassing bug. This version actually works.

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 (3):
virtio_net: Stop doing DMA from the stack
virtio_ring: Support DMA APIs
virtio_pci: Use the DMA API

 drivers/net/virtio_net.c           |  53 +++++++----
 drivers/virtio/Kconfig             |   2 +-
 drivers/virtio/virtio_pci_common.h |   3 +-
 drivers/virtio/virtio_pci_legacy.c |  19 +++-
 drivers/virtio/virtio_pci_modern.c |  34 +++++--
 drivers/virtio/virtio_ring.c       | 187 ++++++++++++++++++++++++++++++-------
 tools/virtio/linux/dma-mapping.h   |  17 ++++
 7 files changed, 246 insertions(+), 69 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

Signature · 1 line

2.4.3

Quoted 4 lines · Andy Lutomirski

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.

So thinking hard about it, I don't see any real drawbacks to making this conditional on a new feature bit, that Xen can then set..

Quoted 4 lines

So thinking hard about it, I don't see any real drawbacks to making this conditional on a new feature bit, that Xen can then set..

Can you elaborate?  If I run QEMU, hosting Xen, hosting Linux, and the virtio device is provided by QEMU, then how does Xen set the bit? Similarly, how would Xen set the bit for a real physical device?

I understand that POWER and other platforms don't currently have a clean way to indicate that certain device don't have translation. And I understand that we may end up with a *quirk* which ensures that the DMA API does the right thing (i.e. nothing) in certain cases.

But we should *NOT* be involving the virtio device drivers in that quirk, in any way. And putting a feature bit in the virtio device itself doesn't seem at all sane either.

Bear in mind that qemu-system-x86_64 currently has the *same* problem with assigned physical devices. It's claiming they're translated, and they're not.

Quoted 6 lines · David Woodhouse

I understand that POWER and other platforms don't currently have a clean way to indicate that certain device don't have translation. And I understand that we may end up with a *quirk* which ensures that the DMA API does the right thing (i.e. nothing) in certain cases.

Quoted 7 lines

But we should *NOT* be involving the virtio device drivers in that quirk, in any way. And putting a feature bit in the virtio device itself doesn't seem at all sane either.

Bear in mind that qemu-system-x86_64 currently has the *same* problem with assigned physical devices. It's claiming they're translated, and they're not.

It's not that clear but yeah ... as I mentioned, I can't find a way to do that quirk that won't break when we want to actually use the iommu...

Ben.

Quoted 6 lines · David Woodhouse

I understand that POWER and other platforms don't currently have a clean way to indicate that certain device don't have translation. And I understand that we may end up with a *quirk* which ensures that the DMA API does the right thing (i.e. nothing) in certain cases.

Quoted 7 lines

But we should *NOT* be involving the virtio device drivers in that quirk, in any way. And putting a feature bit in the virtio device itself doesn't seem at all sane either.

Bear in mind that qemu-system-x86_64 currently has the *same* problem with assigned physical devices. It's claiming they're translated, and they're not.

It's not that clear but yeah ... as I mentioned, I can't find a way to do that quirk that won't break when we want to actually use the iommu...

Ben.

Quoted 8 lines · Andy Lutomirski

Quoted 4 lines

So thinking hard about it, I don't see any real drawbacks to making this conditional on a new feature bit, that Xen can then set..

Can you elaborate?  If I run QEMU, hosting Xen, hosting Linux, and the virtio device is provided by QEMU, then how does Xen set the bit? Similarly, how would Xen set the bit for a real physical device?

Right. This is *not* a fundamental characteristic of the device. This is all about how your *particular* hypervisor (in the set of turtles- all-the-way-down) happened to expose the thing to you.

This is why it lives in the DMAR table, in the Intel world, which *tells* you which devices are behind which IOMMU (and which are not).

And why I keep repeating myself that it has nothing to do with the actual device or the virtio drivers.

I understand that POWER and other platforms don't currently have a clean way to indicate that certain device don't have translation. And I understand that we may end up with a *quirk* which ensures that the DMA API does the right thing (i.e. nothing) in certain cases.

But we should *NOT* be involving the virtio device drivers in that quirk, in any way. And putting a feature bit in the virtio device itself doesn't seem at all sane either.

Bear in mind that qemu-system-x86_64 currently has the *same* problem with assigned physical devices. It's claiming they're translated, and they're not.

Signature · 1 line

dwmw2

David, there are two things a hypervisor needs to tell the guest.
1. The actual device is behind an IOMMU. This is what you are suggesting we use DMAR for.
2. Using IOMMU from kernel (as opposed to from userspace with VFIO) actually adds security. For exising virtio devices on KVM, the answer is no. And DMAR has no way to reflect that.

Quoted 7 lines · Michael S. Tsirkin

David, there are two things a hypervisor needs to tell the guest.
1. The actual device is behind an IOMMU. This is what you are suggesting we use DMAR for.
2. Using IOMMU from kernel (as opposed to from userspace with VFIO) actually adds security. For exising virtio devices on KVM, the answer is no. And DMAR has no way to reflect that.

Using the IOMMU from the kernel *always* adds security. It protects against device driver (and device) bugs which can be made exploitable by allowing DMA to anywhere in the system.

Sure, there are classes of that which are far more interesting, for example where you give the whole device to a guest and let it load the firmware. But "we trust the hypervisor" and "we trust the hardware" are not *so* far apart conceptually.

Hell, with ATS you *still* have to trust the hardware to a large extent.

I really think that something like the proposed DMA_ATTR_IOMMU_BYPASS should suffice

for the "who cares about security; we want performance" case.

--
dwmw2

There's that, and there's an "I care about security, but do not want to burn up cycles on fake protections that do not work" case.

Quoted 5 lines

There's that, and there's an "I care about security, but do not want to burn up cycles on fake protections that do not work" case.

It would seem to make most sense for this use case simply *not* to expose virtio devices to guests as being behind an IOMMU at all. Sure, there are esoteric use cases where the guest actually nests and runs further guests inside itself and wants to pass through the virtio devices from the real hardware host. But presumably those configurations will have multiple virtio devices assigned by the host anyway, and further tweaking the configuration to put them behind an IOMMU shouldn't be hard.

Signature · 2 lines

-- 
dwmw2

There's that, and there's an "I care about security, but do not want to burn up cycles on fake protections that do not work" case.

Quoted 7 lines · Michael S. Tsirkin

David, there are two things a hypervisor needs to tell the guest.
1. The actual device is behind an IOMMU. This is what you are suggesting we use DMAR for.
2. Using IOMMU from kernel (as opposed to from userspace with VFIO) actually adds security. For exising virtio devices on KVM, the answer is no. And DMAR has no way to reflect that.

Using the IOMMU from the kernel *always* adds security. It protects against device driver (and device) bugs which can be made exploitable by allowing DMA to anywhere in the system.

Sure, there are classes of that which are far more interesting, for example where you give the whole device to a guest and let it load the firmware. But "we trust the hypervisor" and "we trust the hardware" are not *so* far apart conceptually.

Hell, with ATS you *still* have to trust the hardware to a large extent.

I really think that something like the proposed DMA_ATTR_IOMMU_BYPASS should suffice

for the "who cares about security; we want performance" case.

--
dwmw2

David, there are two things a hypervisor needs to tell the guest.
1. The actual device is behind an IOMMU. This is what you are suggesting we use DMAR for.
2. Using IOMMU from kernel (as opposed to from userspace with VFIO) actually adds security. For exising virtio devices on KVM, the answer is no. And DMAR has no way to reflect that.

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines · Andy Lutomirski

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.

Quoted 2 lines

So thinking hard about it, I don't see any real drawbacks to making this conditional on a new feature bit, that Xen can then set..

Can you elaborate? If I run QEMU, hosting Xen, hosting Linux, and the virtio device is provided by QEMU, then how does Xen set the bit?

Similarly, how would Xen set the bit for a real physical device?

--Andy

Quoted 8 lines · Andy Lutomirski

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines · Andy Lutomirski

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.

Quoted 5 lines

Quoted 2 lines

So thinking hard about it, I don't see any real drawbacks to making this conditional on a new feature bit, that Xen can then set..

Can you elaborate? If I run QEMU, hosting Xen, hosting Linux, and the virtio device is provided by QEMU, then how does Xen set the bit?

You would run QEMU with the appropriate flag. E.g. -global virtio-pci,use_platform_dma=on

Quoted 4 lines

Similarly, how would Xen set the bit for a real physical device?

--Andy

There's no need to set bits for physical devices I think: from security point of view, using them from a VM isn't very different from using them from host.

Signature · 1 line

MST

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.

Changes from v2:
- Fix really embarrassing bug. This version actually works.

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 (3):
virtio_net: Stop doing DMA from the stack
virtio_ring: Support DMA APIs
virtio_pci: Use the DMA API

 drivers/net/virtio_net.c           |  53 +++++++----
 drivers/virtio/Kconfig             |   2 +-
 drivers/virtio/virtio_pci_common.h |   3 +-
 drivers/virtio/virtio_pci_legacy.c |  19 +++-
 drivers/virtio/virtio_pci_modern.c |  34 +++++--
 drivers/virtio/virtio_ring.c       | 187 ++++++++++++++++++++++++++++++-------
 tools/virtio/linux/dma-mapping.h   |  17 ++++
 7 files changed, 246 insertions(+), 69 deletions(-)
 create mode 100644 tools/virtio/linux/dma-mapping.h

Signature · 1 line

2.4.3