Skip to contentSkip to conversation

Fix various issue of vhost

PATCH Superseded Jason Wang 3 participants 42 messages Dec 12, 2018 – Dec 27, 2018

Subsystems (5)
Networking Network drivers KVM virtio core +1 more subsystems

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jason Wang

author
#0 lore raw

v2 cover Fix various issue of vhost

Hi:

This series tries to fix various issues of vhost:

- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
- Patch 4-7 fixes the diry page logging when device IOTLB is enabled. We should done through GPA instead of GIOVA, this was done through intorudce HVA->GPA reverse mapping and convert HVA to GPA during logging dirty pages.

Please consider them for -stable.

Thanks

Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Jason Wang (4):
vhost: make sure used idx is seen before log in vhost_add_used_n()
vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll() Revert "net: vhost: lock the vqs one by one"
vhost: log dirty page correctly

 drivers/vhost/net.c   |  11 ++++-
 drivers/vhost/vhost.c | 102 ++++++++++++++++++++++++++++++++++--------
 drivers/vhost/vhost.h |   3 +-
 3 files changed, 95 insertions(+), 21 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v2 1/4 vhost: make sure used idx is seen before log in vhost_add_used_n()

We miss a write barrier that guarantees used idx is updated and seen before log. This will let userspace sync and copy used ring before used idx is update. Fix this by adding a barrier before log_write().

Fixes 8dd014adfea6f ("vhost-net: mergeable buffers support")
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/vhost.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vhost/vhost.c +2 -0
index 6b98d8e3a5bf..5915f240275a 100644
@@ -2220,6 +2220,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		return -EFAULT;
 	}
 	if (unlikely(vq->log_used)) {
+		/* Make sure used idx is seen before log. */
+		smp_wmb();
 		/* Log used index update. */
 		log_write(vq->log_base,
 			  vq->log_addr + offsetof(struct vring_used, idx),
Signature · 1 line
2.17.1
maintainer Acked-by on v2 1/4
#2 lore raw

Re: [PATCH net V2 1/4] vhost: make sure used idx is seen before log in vhost_add_used_n()

Quoted 6 lines · Jason Wang

We miss a write barrier that guarantees used idx is updated and seen before log. This will let userspace sync and copy used ring before used idx is update. Fix this by adding a barrier before log_write().

Fixes 8dd014adfea6f ("vhost-net: mergeable buffers support")
Signed-off-by Jason Wang <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>

also for 4.20 and seems like a stable candidate.

Quoted 19 lines
---
 drivers/vhost/vhost.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vhost/vhost.c +2 -0
index 6b98d8e3a5bf..5915f240275a 100644
@@ -2220,6 +2220,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		return -EFAULT;
 	}
 	if (unlikely(vq->log_used)) {
+		/* Make sure used idx is seen before log. */
+		smp_wmb();
 		/* Log used index update. */
 		log_write(vq->log_base,
 			  vq->log_addr + offsetof(struct vring_used, idx),
Signature · 1 line
2.17.1
maintainer Acked-by on v2 1/4
#3 lore raw

Re: [PATCH net V2 1/4] vhost: make sure used idx is seen before log in vhost_add_used_n()

In reply to Jason Wang
Quoted 6 lines · Jason Wang

We miss a write barrier that guarantees used idx is updated and seen before log. This will let userspace sync and copy used ring before used idx is update. Fix this by adding a barrier before log_write().

Fixes 8dd014adfea6f ("vhost-net: mergeable buffers support")
Signed-off-by Jason Wang <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>

also for 4.20 and seems like a stable candidate.

Quoted 19 lines
---
 drivers/vhost/vhost.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vhost/vhost.c +2 -0
index 6b98d8e3a5bf..5915f240275a 100644
@@ -2220,6 +2220,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		return -EFAULT;
 	}
 	if (unlikely(vq->log_used)) {
+		/* Make sure used idx is seen before log. */
+		smp_wmb();
 		/* Log used index update. */
 		log_write(vq->log_base,
 			  vq->log_addr + offsetof(struct vring_used, idx),
Signature · 1 line
2.17.1
author
#4 lore raw

PATCH v2 1/4 vhost: make sure used idx is seen before log in vhost_add_used_n()

In reply to Jason Wang

We miss a write barrier that guarantees used idx is updated and seen before log. This will let userspace sync and copy used ring before used idx is update. Fix this by adding a barrier before log_write().

Fixes 8dd014adfea6f ("vhost-net: mergeable buffers support")
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/vhost.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/vhost/vhost.c +2 -0
index 6b98d8e3a5bf..5915f240275a 100644
@@ -2220,6 +2220,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		return -EFAULT;
 	}
 	if (unlikely(vq->log_used)) {
+		/* Make sure used idx is seen before log. */
+		smp_wmb();
 		/* Log used index update. */
 		log_write(vq->log_base,
 			  vq->log_addr + offsetof(struct vring_used, idx),
Signature · 1 line
2.17.1
author
#5 lore raw

PATCH v2 2/4 vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll()

In reply to Jason Wang

We used to hold the mutex of paired virtqueue in vhost_net_busy_poll(). But this will results an inconsistent lock order which may cause deadlock if we try to bring back the protection of device IOTLB with vq mutex that requires to hold mutex of all virtqueues at the same time.

Fix this simply by switching to use mutex_trylock(), when fail just skip the busy polling. This can happen when device IOTLB is under updating which should be rare.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/vhost/net.c +7 -1
index ab11b2bee273..ad7a6f475a44 100644
@@ -513,7 +513,13 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 	struct socket *sock;
 	struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
 
-	mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	/* Try to hold the vq mutex of the paired virtqueue. We can't
+	 * use mutex_lock() here since we could not guarantee a
+	 * consistenet lock ordering.
+	 */
+	if (!mutex_trylock(&vq->mutex))
+		return;
+
 	vhost_disable_notify(&net->dev, vq);
 	sock = rvq->private_data;
Signature · 1 line
2.17.1
maintainer Acked-by on v2 2/4
#6 lore raw

Re: [PATCH net V2 2/4] vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll()

Quoted 6 lines · Jason Wang

We used to hold the mutex of paired virtqueue in vhost_net_busy_poll(). But this will results an inconsistent lock order which may cause deadlock if we try to bring back the protection of device IOTLB with vq mutex that requires to hold mutex of all virtqueues at the same time.

Quoted 7 lines

Fix this simply by switching to use mutex_trylock(), when fail just skip the busy polling. This can happen when device IOTLB is under updating which should be rare.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>

and I think we should try to put this fix in 4.20 too.

Quoted 25 lines
---
 drivers/vhost/net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/vhost/net.c +7 -1
index ab11b2bee273..ad7a6f475a44 100644
@@ -513,7 +513,13 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 	struct socket *sock;
 	struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
 
-	mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	/* Try to hold the vq mutex of the paired virtqueue. We can't
+	 * use mutex_lock() here since we could not guarantee a
+	 * consistenet lock ordering.
+	 */
+	if (!mutex_trylock(&vq->mutex))
+		return;
+
 	vhost_disable_notify(&net->dev, vq);
 	sock = rvq->private_data;
Signature · 1 line
2.17.1
maintainer Acked-by on v2 2/4
#7 lore raw

Re: [PATCH net V2 2/4] vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll()

In reply to Jason Wang
Quoted 6 lines · Jason Wang

We used to hold the mutex of paired virtqueue in vhost_net_busy_poll(). But this will results an inconsistent lock order which may cause deadlock if we try to bring back the protection of device IOTLB with vq mutex that requires to hold mutex of all virtqueues at the same time.

Quoted 7 lines

Fix this simply by switching to use mutex_trylock(), when fail just skip the busy polling. This can happen when device IOTLB is under updating which should be rare.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>

and I think we should try to put this fix in 4.20 too.

Quoted 25 lines
---
 drivers/vhost/net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/vhost/net.c +7 -1
index ab11b2bee273..ad7a6f475a44 100644
@@ -513,7 +513,13 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 	struct socket *sock;
 	struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
 
-	mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	/* Try to hold the vq mutex of the paired virtqueue. We can't
+	 * use mutex_lock() here since we could not guarantee a
+	 * consistenet lock ordering.
+	 */
+	if (!mutex_trylock(&vq->mutex))
+		return;
+
 	vhost_disable_notify(&net->dev, vq);
 	sock = rvq->private_data;
Signature · 1 line
2.17.1
author
#8 lore raw

PATCH v2 2/4 vhost_net: switch to use mutex_trylock() in vhost_net_busy_poll()

In reply to Jason Wang

We used to hold the mutex of paired virtqueue in vhost_net_busy_poll(). But this will results an inconsistent lock order which may cause deadlock if we try to bring back the protection of device IOTLB with vq mutex that requires to hold mutex of all virtqueues at the same time.

Fix this simply by switching to use mutex_trylock(), when fail just skip the busy polling. This can happen when device IOTLB is under updating which should be rare.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/vhost/net.c +7 -1
index ab11b2bee273..ad7a6f475a44 100644
@@ -513,7 +513,13 @@ static void vhost_net_busy_poll(struct vhost_net *net,
 	struct socket *sock;
 	struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
 
-	mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+	/* Try to hold the vq mutex of the paired virtqueue. We can't
+	 * use mutex_lock() here since we could not guarantee a
+	 * consistenet lock ordering.
+	 */
+	if (!mutex_trylock(&vq->mutex))
+		return;
+
 	vhost_disable_notify(&net->dev, vq);
 	sock = rvq->private_data;
Signature · 1 line
2.17.1
author
#9 lore raw

PATCH v2 3/4 Revert "net: vhost: lock the vqs one by one"

In reply to Jason Wang

This reverts commit 78139c94dc8c96a478e67dab3bee84dc6eccb5fd. We don't protect device IOTLB with vq mutex, which will lead e.g use after free for device IOTLB entries. And since we've switched to use mutex_trylock() in previous patch, it's safe to revert it without having deadlock.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/vhost.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
drivers/vhost/vhost.c +17 -4
index 5915f240275a..55e5aa662ad5 100644
@@ -295,11 +295,8 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
 {
 	int i;
 
-	for (i = 0; i < d->nvqs; ++i) {
-		mutex_lock(&d->vqs[i]->mutex);
+	for (i = 0; i < d->nvqs; ++i)
 		__vhost_vq_meta_reset(d->vqs[i]);
-		mutex_unlock(&d->vqs[i]->mutex);
-	}
 }
 
 static void vhost_vq_reset(struct vhost_dev *dev,

@@ -895,6 +892,20 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
 #define vhost_get_used(vq, x, ptr) \
 	vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
 
+static void vhost_dev_lock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_lock_nested(&d->vqs[i]->mutex, i);
+}
+
+static void vhost_dev_unlock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_unlock(&d->vqs[i]->mutex);
+}
+
 static int vhost_new_umem_range(struct vhost_umem *umem,
 				u64 start, u64 size, u64 end,
 				u64 userspace_addr, int perm)

@@ -976,6 +987,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 	int ret = 0;
 
 	mutex_lock(&dev->mutex);
+	vhost_dev_lock_vqs(dev);
 	switch (msg->type) {
 	case VHOST_IOTLB_UPDATE:
 		if (!dev->iotlb) {

@@ -1009,6 +1021,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 		break;
 	}
 
+	vhost_dev_unlock_vqs(dev);
 	mutex_unlock(&dev->mutex);
 
 	return ret;
Signature · 1 line
2.17.1
maintainer Acked-by on v2 3/4
#10 lore raw

Re: [PATCH net V2 3/4] Revert "net: vhost: lock the vqs one by one"

Quoted 9 lines · Jason Wang

This reverts commit 78139c94dc8c96a478e67dab3bee84dc6eccb5fd. We don't protect device IOTLB with vq mutex, which will lead e.g use after free for device IOTLB entries. And since we've switched to use mutex_trylock() in previous patch, it's safe to revert it without having deadlock.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>

I'd try to put this in 4.20 if we can
and it's needed for -stable I think.

Also looks like we should allow iotlb entries per vq to improve locking. What do you think?

Quoted 60 lines
---
 drivers/vhost/vhost.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
drivers/vhost/vhost.c +17 -4
index 5915f240275a..55e5aa662ad5 100644
@@ -295,11 +295,8 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
 {
 	int i;
 
-	for (i = 0; i < d->nvqs; ++i) {
-		mutex_lock(&d->vqs[i]->mutex);
+	for (i = 0; i < d->nvqs; ++i)
 		__vhost_vq_meta_reset(d->vqs[i]);
-		mutex_unlock(&d->vqs[i]->mutex);
-	}
 }
 
 static void vhost_vq_reset(struct vhost_dev *dev,

@@ -895,6 +892,20 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
 #define vhost_get_used(vq, x, ptr) \
 	vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
 
+static void vhost_dev_lock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_lock_nested(&d->vqs[i]->mutex, i);
+}
+
+static void vhost_dev_unlock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_unlock(&d->vqs[i]->mutex);
+}
+
 static int vhost_new_umem_range(struct vhost_umem *umem,
 				u64 start, u64 size, u64 end,
 				u64 userspace_addr, int perm)

@@ -976,6 +987,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 	int ret = 0;
 
 	mutex_lock(&dev->mutex);
+	vhost_dev_lock_vqs(dev);
 	switch (msg->type) {
 	case VHOST_IOTLB_UPDATE:
 		if (!dev->iotlb) {

@@ -1009,6 +1021,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 		break;
 	}
 
+	vhost_dev_unlock_vqs(dev);
 	mutex_unlock(&dev->mutex);
 
 	return ret;
Signature · 1 line
2.17.1
author
#11 lore raw

Re: [PATCH net V2 3/4] Revert "net: vhost: lock the vqs one by one"

Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Jason Wang

This reverts commit 78139c94dc8c96a478e67dab3bee84dc6eccb5fd. We don't protect device IOTLB with vq mutex, which will lead e.g use after free for device IOTLB entries. And since we've switched to use mutex_trylock() in previous patch, it's safe to revert it without having deadlock.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang<[email protected]>
Signed-off-by Jason Wang<[email protected]>
Quoted 8 lines
Acked-by Michael S. Tsirkin<[email protected]>

I'd try to put this in 4.20 if we can
and it's needed for -stable I think.

Also looks like we should allow iotlb entries per vq to improve locking. What do you think?

Yes, we can do it for -next.

Thanks

author
#12 lore raw

Re: [PATCH net V2 3/4] Revert "net: vhost: lock the vqs one by one"

In reply to Michael S. Tsirkin
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Jason Wang

This reverts commit 78139c94dc8c96a478e67dab3bee84dc6eccb5fd. We don't protect device IOTLB with vq mutex, which will lead e.g use after free for device IOTLB entries. And since we've switched to use mutex_trylock() in previous patch, it's safe to revert it without having deadlock.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang<[email protected]>
Signed-off-by Jason Wang<[email protected]>
Quoted 8 lines
Acked-by Michael S. Tsirkin<[email protected]>

I'd try to put this in 4.20 if we can
and it's needed for -stable I think.

Also looks like we should allow iotlb entries per vq to improve locking. What do you think?

Yes, we can do it for -next.

Thanks

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

maintainer Acked-by on v2 3/4
#13 lore raw

Re: [PATCH net V2 3/4] Revert "net: vhost: lock the vqs one by one"

In reply to Jason Wang
Quoted 9 lines · Jason Wang

This reverts commit 78139c94dc8c96a478e67dab3bee84dc6eccb5fd. We don't protect device IOTLB with vq mutex, which will lead e.g use after free for device IOTLB entries. And since we've switched to use mutex_trylock() in previous patch, it's safe to revert it without having deadlock.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>

I'd try to put this in 4.20 if we can
and it's needed for -stable I think.

Also looks like we should allow iotlb entries per vq to improve locking. What do you think?

Quoted 60 lines
---
 drivers/vhost/vhost.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
drivers/vhost/vhost.c +17 -4
index 5915f240275a..55e5aa662ad5 100644
@@ -295,11 +295,8 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
 {
 	int i;
 
-	for (i = 0; i < d->nvqs; ++i) {
-		mutex_lock(&d->vqs[i]->mutex);
+	for (i = 0; i < d->nvqs; ++i)
 		__vhost_vq_meta_reset(d->vqs[i]);
-		mutex_unlock(&d->vqs[i]->mutex);
-	}
 }
 
 static void vhost_vq_reset(struct vhost_dev *dev,

@@ -895,6 +892,20 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
 #define vhost_get_used(vq, x, ptr) \
 	vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
 
+static void vhost_dev_lock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_lock_nested(&d->vqs[i]->mutex, i);
+}
+
+static void vhost_dev_unlock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_unlock(&d->vqs[i]->mutex);
+}
+
 static int vhost_new_umem_range(struct vhost_umem *umem,
 				u64 start, u64 size, u64 end,
 				u64 userspace_addr, int perm)

@@ -976,6 +987,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 	int ret = 0;
 
 	mutex_lock(&dev->mutex);
+	vhost_dev_lock_vqs(dev);
 	switch (msg->type) {
 	case VHOST_IOTLB_UPDATE:
 		if (!dev->iotlb) {

@@ -1009,6 +1021,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 		break;
 	}
 
+	vhost_dev_unlock_vqs(dev);
 	mutex_unlock(&dev->mutex);
 
 	return ret;
Signature · 1 line
2.17.1
author
#14 lore raw

PATCH v2 3/4 Revert "net: vhost: lock the vqs one by one"

In reply to Jason Wang

This reverts commit 78139c94dc8c96a478e67dab3bee84dc6eccb5fd. We don't protect device IOTLB with vq mutex, which will lead e.g use after free for device IOTLB entries. And since we've switched to use mutex_trylock() in previous patch, it's safe to revert it without having deadlock.

Fixes commit 78139c94dc8c ("net: vhost: lock the vqs one by one")
Cc Tonghao Zhang <[email protected]>
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/vhost.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
drivers/vhost/vhost.c +17 -4
index 5915f240275a..55e5aa662ad5 100644
@@ -295,11 +295,8 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
 {
 	int i;
 
-	for (i = 0; i < d->nvqs; ++i) {
-		mutex_lock(&d->vqs[i]->mutex);
+	for (i = 0; i < d->nvqs; ++i)
 		__vhost_vq_meta_reset(d->vqs[i]);
-		mutex_unlock(&d->vqs[i]->mutex);
-	}
 }
 
 static void vhost_vq_reset(struct vhost_dev *dev,

@@ -895,6 +892,20 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
 #define vhost_get_used(vq, x, ptr) \
 	vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
 
+static void vhost_dev_lock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_lock_nested(&d->vqs[i]->mutex, i);
+}
+
+static void vhost_dev_unlock_vqs(struct vhost_dev *d)
+{
+	int i = 0;
+	for (i = 0; i < d->nvqs; ++i)
+		mutex_unlock(&d->vqs[i]->mutex);
+}
+
 static int vhost_new_umem_range(struct vhost_umem *umem,
 				u64 start, u64 size, u64 end,
 				u64 userspace_addr, int perm)

@@ -976,6 +987,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 	int ret = 0;
 
 	mutex_lock(&dev->mutex);
+	vhost_dev_lock_vqs(dev);
 	switch (msg->type) {
 	case VHOST_IOTLB_UPDATE:
 		if (!dev->iotlb) {

@@ -1009,6 +1021,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
 		break;
 	}
 
+	vhost_dev_unlock_vqs(dev);
 	mutex_unlock(&dev->mutex);
 
 	return ret;
Signature · 1 line
2.17.1
author
#15 lore raw

PATCH v2 4/4 vhost: log dirty page correctly

In reply to Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/net.c   |  3 +-
 drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
 drivers/vhost/vhost.h |  3 +-
 3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
 		if (nvq->done_idx > VHOST_NET_BATCH)
 			vhost_net_signal_used(nvq);
 		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
 		total_len += vhost_len;
 		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
 			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +65 -14

113 lines not on this page · raw message

drivers/vhost/vhost.h +2 -1
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
 bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
 int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
 
 struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1
maintainer
#16 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 13 lines · Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

Quoted 7 lines

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>

It's a nasty bug for sure but it's been like this for a long time so I'm inclined to say let's put it in 4.21, and queue for stable.

So please split this out from this series.

Also, I'd like to see a feature bit that allows GPA in IOTLBs.

Quoted 40 lines
---
 drivers/vhost/net.c   |  3 +-
 drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
 drivers/vhost/vhost.h |  3 +-
 3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
 		if (nvq->done_idx > VHOST_NET_BATCH)
 			vhost_net_signal_used(nvq);
 		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
 		total_len += vhost_len;
 		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
 			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +12 -0
index 55e5aa662ad5..3660310604fd 100644
@@ -1733,11 +1733,67 @@ static int log_write(void __user *log_base,
 	return r;
 }
 
+static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
+{
+	struct vhost_umem *umem = vq->umem;
+	struct vhost_umem_node *u;
+	u64 gpa;
+	int r;
+	bool hit = false;
+
+	list_for_each_entry(u, &umem->umem_list, link) {
+		if (u->userspace_addr < hva &&
+		    u->userspace_addr + u->size >=
+		    hva + len) {

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1757

Quoted 10 lines
drivers/vhost/vhost.c +10 -0
+			gpa = u->start + hva - u->userspace_addr;
+			r = log_write(vq->log_base, gpa, len);
+			if (r < 0)
+				return r;
+			hit = true;
+		}
+	}
+
+	/* No reverse mapping, should be a bug */
+	WARN_ON(!hit);

Maybe it should but userspace can trigger this easily I think. We need to stop the device not warn in kernel log.

Also there's an error fd: VHOST_SET_VRING_ERR, need to wake it up.

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1769

Quoted 12 lines
drivers/vhost/vhost.c +12 -0
+	return 0;
+}
+
+static void log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
+{
+	struct iovec iov[64];
+	int i, ret;
+
+	if (!vq->iotlb) {
+		log_write(vq->log_base, vq->log_addr + used_offset, len);
+		return;
+	}

This change seems questionable. used ring writes use their own machinery it does not go through iotlb. Same should apply to log I think.

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1773

Quoted 4 lines
drivers/vhost/vhost.c +4 -0
+
+	ret = translate_desc(vq, (u64)(uintptr_t)vq->used + used_offset,
+			     len, iov, 64, VHOST_ACCESS_WO);
+	WARN_ON(ret < 0);

Same thing here. translation failures can be triggered from guest. warn on is not a good error handling strategy ...

Quoted 92 lines
drivers/vhost/vhost.c +27 -14
+
+	for (i = 0; i < ret; i++) {
+		ret = log_write_hva(vq,	(u64)(uintptr_t)iov[i].iov_base,
+				    iov[i].iov_len);
+		WARN_ON(ret);
+	}
+}
+
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len)
+		    unsigned int log_num, u64 len, struct iovec *iov, int count)
 {
 	int i, r;
 
+	if (vq->iotlb) {
+		for (i = 0; i < count; i++) {
+			r = log_write_hva(vq, (u64)(uintptr_t)iov[i].iov_base,
+					  iov[i].iov_len);
+			if (r < 0)
+				return r;
+		}
+		return 0;
+	}
+
 	/* Make sure data written is seen before log. */
 	smp_wmb();
 	for (i = 0; i < log_num; ++i) {

@@ -1769,9 +1825,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
 		smp_wmb();
 		/* Log used flag write. */
 		used = &vq->used->flags;
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof vq->used->flags);
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof vq->used->flags);
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}

@@ -1789,9 +1844,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
 		smp_wmb();
 		/* Log avail event write */
 		used = vhost_avail_event(vq);
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof *vhost_avail_event(vq));
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof *vhost_avail_event(vq));
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}

@@ -2191,10 +2245,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
 		/* Make sure data is seen before log. */
 		smp_wmb();
 		/* Log used ring entry write. */
-		log_write(vq->log_base,
-			  vq->log_addr +
-			   ((void __user *)used - (void __user *)vq->used),
-			  count * sizeof *used);
+		log_used(vq, ((void __user *)used - (void __user *)vq->used),
+			 count * sizeof *used);
 	}
 	old = vq->last_used_idx;
 	new = (vq->last_used_idx += count);

@@ -2236,9 +2288,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		/* Make sure used idx is seen before log. */
 		smp_wmb();
 		/* Log used index update. */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used, idx),
-			  sizeof vq->used->idx);
+		log_used(vq, offsetof(struct vring_used, idx),
+			 sizeof vq->used->idx);
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}
drivers/vhost/vhost.h +2 -1
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
 bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
 int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
 
 struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1
author
#17 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 21 lines · Michael S. Tsirkin
Quoted 13 lines · Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

Quoted 7 lines

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Quoted 5 lines

It's a nasty bug for sure but it's been like this for a long time so I'm inclined to say let's put it in 4.21, and queue for stable.

So please split this out from this series.

Ok.

Quoted 2 lines

Also, I'd like to see a feature bit that allows GPA in IOTLBs.

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Quoted 54 lines
Quoted 41 lines
---
  drivers/vhost/net.c   |  3 +-
  drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
  drivers/vhost/vhost.h |  3 +-
  3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
  		if (nvq->done_idx > VHOST_NET_BATCH)
  			vhost_net_signal_used(nvq);
  		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
  		total_len += vhost_len;
  		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
  			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +12 -0
index 55e5aa662ad5..3660310604fd 100644
@@ -1733,11 +1733,67 @@ static int log_write(void __user *log_base,
  	return r;
  }
 
+static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
+{
+	struct vhost_umem *umem = vq->umem;
+	struct vhost_umem_node *u;
+	u64 gpa;
+	int r;
+	bool hit = false;
+
+	list_for_each_entry(u, &umem->umem_list, link) {
+		if (u->userspace_addr < hva &&
+		    u->userspace_addr + u->size >=
+		    hva + len) {

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1757

Quoted 10 lines
drivers/vhost/vhost.c +10 -0
+			gpa = u->start + hva - u->userspace_addr;
+			r = log_write(vq->log_base, gpa, len);
+			if (r < 0)
+				return r;
+			hit = true;
+		}
+	}
+
+	/* No reverse mapping, should be a bug */
+	WARN_ON(!hit);

Maybe it should but userspace can trigger this easily I think. We need to stop the device not warn in kernel log.

Quoted 2 lines

Also there's an error fd: VHOST_SET_VRING_ERR, need to wake it up.

Ok.

Quoted 7 lines
Quoted 7 lines
drivers/vhost/vhost.c +7 -0
+	return 0;
+}
+
+static void log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
+{
+	struct iovec iov[64];
+	int i, ret;
Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1769

Quoted 5 lines
drivers/vhost/vhost.c +5 -0
+
+	if (!vq->iotlb) {
+		log_write(vq->log_base, vq->log_addr + used_offset, len);
+		return;
+	}

This change seems questionable. used ring writes use their own machinery it does not go through iotlb. Same should apply to log I think.

The problem is used ring may not be physically contiguous with Device IOTLB enabled. So it should go through it.

Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1773

Quoted 5 lines
drivers/vhost/vhost.c +4 -0
+
+	ret = translate_desc(vq, (u64)(uintptr_t)vq->used + used_offset,
+			     len, iov, 64, VHOST_ACCESS_WO);
+	WARN_ON(ret < 0);

Same thing here. translation failures can be triggered from guest. warn on is not a good error handling strategy ...

Ok. Let me fix it.

Thanks

Quoted 92 lines
Quoted 92 lines
drivers/vhost/vhost.c +27 -14
+
+	for (i = 0; i < ret; i++) {
+		ret = log_write_hva(vq,	(u64)(uintptr_t)iov[i].iov_base,
+				    iov[i].iov_len);
+		WARN_ON(ret);
+	}
+}
+
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len)
+		    unsigned int log_num, u64 len, struct iovec *iov, int count)
  {
  	int i, r;
 
+	if (vq->iotlb) {
+		for (i = 0; i < count; i++) {
+			r = log_write_hva(vq, (u64)(uintptr_t)iov[i].iov_base,
+					  iov[i].iov_len);
+			if (r < 0)
+				return r;
+		}
+		return 0;
+	}
+
  	/* Make sure data written is seen before log. */
  	smp_wmb();
  	for (i = 0; i < log_num; ++i) {

@@ -1769,9 +1825,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
  		smp_wmb();
  		/* Log used flag write. */
  		used = &vq->used->flags;
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof vq->used->flags);
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof vq->used->flags);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -1789,9 +1844,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
  		smp_wmb();
  		/* Log avail event write */
  		used = vhost_avail_event(vq);
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof *vhost_avail_event(vq));
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof *vhost_avail_event(vq));
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -2191,10 +2245,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
  		/* Make sure data is seen before log. */
  		smp_wmb();
  		/* Log used ring entry write. */
-		log_write(vq->log_base,
-			  vq->log_addr +
-			   ((void __user *)used - (void __user *)vq->used),
-			  count * sizeof *used);
+		log_used(vq, ((void __user *)used - (void __user *)vq->used),
+			 count * sizeof *used);
  	}
  	old = vq->last_used_idx;
  	new = (vq->last_used_idx += count);

@@ -2236,9 +2288,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
  		/* Make sure used idx is seen before log. */
  		smp_wmb();
  		/* Log used index update. */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used, idx),
-			  sizeof vq->used->idx);
+		log_used(vq, offsetof(struct vring_used, idx),
+			 sizeof vq->used->idx);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}
drivers/vhost/vhost.h +2 -1
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
  bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
  int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
 
  struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1
maintainer
#18 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 39 lines · Jason Wang
Quoted 21 lines · Michael S. Tsirkin
Quoted 13 lines · Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

Quoted 7 lines

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Quoted 5 lines

It's a nasty bug for sure but it's been like this for a long time so I'm inclined to say let's put it in 4.21, and queue for stable.

So please split this out from this series.

Ok.

Quoted 2 lines

Also, I'd like to see a feature bit that allows GPA in IOTLBs.

Just to make sure I understand this. It looks to me we should:

Quoted 8 lines

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 186 lines
Quoted 54 lines
Quoted 41 lines

---
  drivers/vhost/net.c   |  3 +-
  drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
  drivers/vhost/vhost.h |  3 +-
  3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
  		if (nvq->done_idx > VHOST_NET_BATCH)
  			vhost_net_signal_used(nvq);
  		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
  		total_len += vhost_len;
  		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
  			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +12 -0
index 55e5aa662ad5..3660310604fd 100644
@@ -1733,11 +1733,67 @@ static int log_write(void __user *log_base,
  	return r;
  }
+static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
+{
+	struct vhost_umem *umem = vq->umem;
+	struct vhost_umem_node *u;
+	u64 gpa;
+	int r;
+	bool hit = false;
+
+	list_for_each_entry(u, &umem->umem_list, link) {
+		if (u->userspace_addr < hva &&
+		    u->userspace_addr + u->size >=
+		    hva + len) {

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1756

Quoted 10 lines
drivers/vhost/vhost.c +10 -0
+			gpa = u->start + hva - u->userspace_addr;
+			r = log_write(vq->log_base, gpa, len);
+			if (r < 0)
+				return r;
+			hit = true;
+		}
+	}
+
+	/* No reverse mapping, should be a bug */
+	WARN_ON(!hit);

Maybe it should but userspace can trigger this easily I think. We need to stop the device not warn in kernel log.

Quoted 2 lines

Also there's an error fd: VHOST_SET_VRING_ERR, need to wake it up.

Ok.

Quoted 7 lines
Quoted 7 lines
drivers/vhost/vhost.c +7 -0
+	return 0;
+}
+
+static void log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
+{
+	struct iovec iov[64];
+	int i, ret;
Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1769

Quoted 5 lines
drivers/vhost/vhost.c +5 -0
+
+	if (!vq->iotlb) {
+		log_write(vq->log_base, vq->log_addr + used_offset, len);
+		return;
+	}

This change seems questionable. used ring writes use their own machinery it does not go through iotlb. Same should apply to log I think.

The problem is used ring may not be physically contiguous with Device IOTLB enabled. So it should go through it.

Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1773

Quoted 5 lines
drivers/vhost/vhost.c +4 -0
+
+	ret = translate_desc(vq, (u64)(uintptr_t)vq->used + used_offset,
+			     len, iov, 64, VHOST_ACCESS_WO);
+	WARN_ON(ret < 0);

Same thing here. translation failures can be triggered from guest. warn on is not a good error handling strategy ...

Ok. Let me fix it.

Thanks

Quoted 89 lines
Quoted 89 lines
drivers/vhost/vhost.c +27 -14
+
+	for (i = 0; i < ret; i++) {
+		ret = log_write_hva(vq,	(u64)(uintptr_t)iov[i].iov_base,
+				    iov[i].iov_len);
+		WARN_ON(ret);
+	}
+}
+
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len)
+		    unsigned int log_num, u64 len, struct iovec *iov, int count)
  {
  	int i, r;
+	if (vq->iotlb) {
+		for (i = 0; i < count; i++) {
+			r = log_write_hva(vq, (u64)(uintptr_t)iov[i].iov_base,
+					  iov[i].iov_len);
+			if (r < 0)
+				return r;
+		}
+		return 0;
+	}
+
  	/* Make sure data written is seen before log. */
  	smp_wmb();
  	for (i = 0; i < log_num; ++i) {

@@ -1769,9 +1825,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
  		smp_wmb();
  		/* Log used flag write. */
  		used = &vq->used->flags;
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof vq->used->flags);
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof vq->used->flags);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -1789,9 +1844,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
  		smp_wmb();
  		/* Log avail event write */
  		used = vhost_avail_event(vq);
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof *vhost_avail_event(vq));
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof *vhost_avail_event(vq));
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -2191,10 +2245,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
  		/* Make sure data is seen before log. */
  		smp_wmb();
  		/* Log used ring entry write. */
-		log_write(vq->log_base,
-			  vq->log_addr +
-			   ((void __user *)used - (void __user *)vq->used),
-			  count * sizeof *used);
+		log_used(vq, ((void __user *)used - (void __user *)vq->used),
+			 count * sizeof *used);
  	}
  	old = vq->last_used_idx;
  	new = (vq->last_used_idx += count);

@@ -2236,9 +2288,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
  		/* Make sure used idx is seen before log. */
  		smp_wmb();
  		/* Log used index update. */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used, idx),
-			  sizeof vq->used->idx);
+		log_used(vq, offsetof(struct vring_used, idx),
+			 sizeof vq->used->idx);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}
drivers/vhost/vhost.h +2 -1
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
  bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
  int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
  struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1
author
#19 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 9 lines · Michael S. Tsirkin
Quoted 9 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Quoted 8 lines
Quoted 1 line

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

maintainer
#20 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 20 lines · Jason Wang
Quoted 9 lines · Michael S. Tsirkin
Quoted 9 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Quoted 8 lines
Quoted 1 line

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky. If translations to HVA are done in a single place through a single table it's safer as there's a single risky place.

Signature · 1 line
MST
author
#21 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Thanks

Quoted 4 lines

If
translations to HVA are done in a single place through a single table it's safer as there's a single risky place.

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

maintainer
#22 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 30 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

Quoted 5 lines

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

Quoted 5 lines
Quoted 5 lines

If
translations to HVA are done in a single place through a single table it's safer as there's a single risky place.

author
#23 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 7 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Thanks

maintainer
#24 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 73 lines · Jason Wang
Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 7 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

Quoted 6 lines

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Thanks

So there are several points I would like to make

1. At the moment without an iommu it is possible to change GPA-HVA mappings and everything keeps working because a change in memory tables flushes the rings. However I don't see the iotlb cache being invalidated on that path - did I miss it? If it is not there it's a related minor bug.

2. qemu already has a GPA. Discarding it and re-calculating when logging is on just seems wrong.
However if you would like to *also* keep the HVA in the iotlb to avoid doing extra translations, that sounds like a reasonable optimization.

3. it also means that the hva->gpa translation only runs when logging is enabled. That is a rarely excercised path so any bugs there will not be caught.

So I really would like us long term to move away from hva->gpa translations, keep them for legacy userspace only but I don't really mind how we do it.

How about
- a new flag to pass an iotlb with *both* a gpa and hva
- for legacy userspace, calculate the gpa on iotlb update so the device then uses a shared code path

what do you think?

Signature · 5 lines
MST
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
author
#25 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 75 lines · Michael S. Tsirkin
Quoted 74 lines · Jason Wang
Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 6 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Quoted 6 lines
Quoted 1 line

Thanks

So there are several points I would like to make

1. At the moment without an iommu it is possible to change GPA-HVA mappings and everything keeps working because a change in memory tables flushes the rings.

Interesting, I don't know this before. But when can this happen?

Quoted 3 lines

However I don't see the iotlb cache being invalidated on that path - did I miss it? If it is not there it's a related minor bug.

It might have a bug. But a question is consider the case without IOMMU. We only update mem table (SET_MEM_TABLE), but not vring address. This looks like a bug as well?

Quoted 6 lines

2. qemu already has a GPA. Discarding it and re-calculating when logging is on just seems wrong.
However if you would like to *also* keep the HVA in the iotlb to avoid doing extra translations, that sounds like a reasonable optimization.

Yes, traverse GPA->HVA mapping seems unnecessary.

Quoted 4 lines

3. it also means that the hva->gpa translation only runs when logging is enabled. That is a rarely excercised path so any bugs there will not be caught.

I wonder maybe some kind of unit-test may help here.

Quoted 5 lines

So I really would like us long term to move away from hva->gpa translations, keep them for legacy userspace only but I don't really mind how we do it.

Quoted 8 lines

How about
- a new flag to pass an iotlb with *both* a gpa and hva
- for legacy userspace, calculate the gpa on iotlb update so the device then uses a shared code path

what do you think?

I don't object this idea so I can try, just want to figure out why it was a must.

Thanks

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

maintainer
#26 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 78 lines · Jason Wang
Quoted 76 lines · Michael S. Tsirkin
Quoted 75 lines · Jason Wang
Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 6 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Thanks

Quoted 8 lines
Quoted 5 lines

So there are several points I would like to make

1. At the moment without an iommu it is possible to change GPA-HVA mappings and everything keeps working because a change in memory tables flushes the rings.

Interesting, I don't know this before. But when can this happen?

It doesn't happen with existing qemu. But it seems like a valid thing to do to remap memory at a different address.

Quoted 9 lines
Quoted 4 lines

However I don't see the iotlb cache being invalidated on that path - did I miss it? If it is not there it's a related minor bug.

It might have a bug. But a question is consider the case without IOMMU. We only update mem table (SET_MEM_TABLE), but not vring address. This looks like a bug as well?

I think that without an iommu it can only work without races if backend is stopped or if the vring isn't in guest memory with ring aliasing).

Quoted 31 lines
Quoted 7 lines

2. qemu already has a GPA. Discarding it and re-calculating when logging is on just seems wrong.
However if you would like to *also* keep the HVA in the iotlb to avoid doing extra translations, that sounds like a reasonable optimization.

Yes, traverse GPA->HVA mapping seems unnecessary.

Quoted 4 lines

3. it also means that the hva->gpa translation only runs when logging is enabled. That is a rarely excercised path so any bugs there will not be caught.

I wonder maybe some kind of unit-test may help here.

Quoted 10 lines

So I really would like us long term to move away from hva->gpa translations, keep them for legacy userspace only but I don't really mind how we do it.

How about
- a new flag to pass an iotlb with *both* a gpa and hva
- for legacy userspace, calculate the gpa on iotlb update so the device then uses a shared code path

Quoted 8 lines
Quoted 3 lines

what do you think?

I don't object this idea so I can try, just want to figure out why it was a must.

Thanks

Not a must but I think it's a good interface extension.

Signature · 1 line
MST
author
#27 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

Quoted 79 lines · Michael S. Tsirkin
Quoted 78 lines · Jason Wang
Quoted 77 lines · Michael S. Tsirkin
Quoted 68 lines · Jason Wang
Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 6 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

Quoted 6 lines

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Thanks

So there are several points I would like to make

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

1. At the moment without an iommu it is possible to change GPA-HVA mappings and everything keeps working because a change in memory tables flushes the rings.

Interesting, I don't know this before. But when can this happen?

It doesn't happen with existing qemu. But it seems like a valid thing to do to remap memory at a different address.

Ok.

Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

However I don't see the iotlb cache being invalidated on that path - did I miss it? If it is not there it's a related minor bug.

It might have a bug. But a question is consider the case without IOMMU. We only update mem table (SET_MEM_TABLE), but not vring address. This looks like a bug as well?

I think that without an iommu it can only work without races if backend is stopped or if the vring isn't in guest memory with ring aliasing).

Right.

Quoted 26 lines
Quoted 26 lines
Quoted 6 lines

2. qemu already has a GPA. Discarding it and re-calculating when logging is on just seems wrong.
However if you would like to *also* keep the HVA in the iotlb to avoid doing extra translations, that sounds like a reasonable optimization.

Yes, traverse GPA->HVA mapping seems unnecessary.

Quoted 3 lines

3. it also means that the hva->gpa translation only runs when logging is enabled. That is a rarely excercised path so any bugs there will not be caught.

I wonder maybe some kind of unit-test may help here.

Quoted 9 lines

So I really would like us long term to move away from hva->gpa translations, keep them for legacy userspace only but I don't really mind how we do it.

How about
- a new flag to pass an iotlb with *both* a gpa and hva
- for legacy userspace, calculate the gpa on iotlb update so the device then uses a shared code path

Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

what do you think?

I don't object this idea so I can try, just want to figure out why it was a must.

Thanks

Not a must but I think it's a good interface extension.

Ok. let me try to do this.

Thanks

author
#28 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Michael S. Tsirkin
Quoted 79 lines · Michael S. Tsirkin
Quoted 78 lines · Jason Wang
Quoted 77 lines · Michael S. Tsirkin
Quoted 68 lines · Jason Wang
Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 6 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

Quoted 6 lines

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Thanks

So there are several points I would like to make

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

1. At the moment without an iommu it is possible to change GPA-HVA mappings and everything keeps working because a change in memory tables flushes the rings.

Interesting, I don't know this before. But when can this happen?

It doesn't happen with existing qemu. But it seems like a valid thing to do to remap memory at a different address.

Ok.

Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

However I don't see the iotlb cache being invalidated on that path - did I miss it? If it is not there it's a related minor bug.

It might have a bug. But a question is consider the case without IOMMU. We only update mem table (SET_MEM_TABLE), but not vring address. This looks like a bug as well?

I think that without an iommu it can only work without races if backend is stopped or if the vring isn't in guest memory with ring aliasing).

Right.

Quoted 26 lines
Quoted 26 lines
Quoted 6 lines

2. qemu already has a GPA. Discarding it and re-calculating when logging is on just seems wrong.
However if you would like to *also* keep the HVA in the iotlb to avoid doing extra translations, that sounds like a reasonable optimization.

Yes, traverse GPA->HVA mapping seems unnecessary.

Quoted 3 lines

3. it also means that the hva->gpa translation only runs when logging is enabled. That is a rarely excercised path so any bugs there will not be caught.

I wonder maybe some kind of unit-test may help here.

Quoted 9 lines

So I really would like us long term to move away from hva->gpa translations, keep them for legacy userspace only but I don't really mind how we do it.

How about
- a new flag to pass an iotlb with *both* a gpa and hva
- for legacy userspace, calculate the gpa on iotlb update so the device then uses a shared code path

Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

what do you think?

I don't object this idea so I can try, just want to figure out why it was a must.

Thanks

Not a must but I think it's a good interface extension.

Ok. let me try to do this.

Thanks

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

maintainer
#29 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Jason Wang
Quoted 78 lines · Jason Wang
Quoted 76 lines · Michael S. Tsirkin
Quoted 75 lines · Jason Wang
Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 6 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Thanks

Quoted 8 lines
Quoted 5 lines

So there are several points I would like to make

1. At the moment without an iommu it is possible to change GPA-HVA mappings and everything keeps working because a change in memory tables flushes the rings.

Interesting, I don't know this before. But when can this happen?

It doesn't happen with existing qemu. But it seems like a valid thing to do to remap memory at a different address.

Quoted 9 lines
Quoted 4 lines

However I don't see the iotlb cache being invalidated on that path - did I miss it? If it is not there it's a related minor bug.

It might have a bug. But a question is consider the case without IOMMU. We only update mem table (SET_MEM_TABLE), but not vring address. This looks like a bug as well?

I think that without an iommu it can only work without races if backend is stopped or if the vring isn't in guest memory with ring aliasing).

Quoted 31 lines
Quoted 7 lines

2. qemu already has a GPA. Discarding it and re-calculating when logging is on just seems wrong.
However if you would like to *also* keep the HVA in the iotlb to avoid doing extra translations, that sounds like a reasonable optimization.

Yes, traverse GPA->HVA mapping seems unnecessary.

Quoted 4 lines

3. it also means that the hva->gpa translation only runs when logging is enabled. That is a rarely excercised path so any bugs there will not be caught.

I wonder maybe some kind of unit-test may help here.

Quoted 10 lines

So I really would like us long term to move away from hva->gpa translations, keep them for legacy userspace only but I don't really mind how we do it.

How about
- a new flag to pass an iotlb with *both* a gpa and hva
- for legacy userspace, calculate the gpa on iotlb update so the device then uses a shared code path

Quoted 8 lines
Quoted 3 lines

what do you think?

I don't object this idea so I can try, just want to figure out why it was a must.

Thanks

Not a must but I think it's a good interface extension.

Signature · 5 lines
MST
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
author
#30 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Michael S. Tsirkin
Quoted 32 lines · Michael S. Tsirkin
Quoted 31 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Quoted 5 lines
Quoted 1 line

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

If qemu provides buggy GPA->HVA, we can't workaround this. And I don't get the point why we even want to try this. Buggy qemu code can crash itself in many ways.

Quoted 7 lines

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

So the difference whether or not use memory table information:

Current:

1) SET_MEM_TABLE: GPA->HVA

2) Qemu GIOVA->GPA

3) Qemu GPA->HVA

4) IOTLB_UPDATE: GIOVA->HVA

If I understand correctly you want to drop step 3 consider it might be buggy which is just 19 lines of code in qemu (vhost_memory_region_lookup()). This will ends up:

1) Do GPA->HVA translation in IOTLB_UPDATE path (I believe we won't want to do it during device IOTLB lookup).

2) Extra bits to enable this capability.

So this looks need more codes in kernel than what qemu did in userspace.  Is this really worthwhile?

Thanks

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

maintainer
#31 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Jason Wang
Quoted 30 lines · Jason Wang
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Jason Wang
Quoted 17 lines · Michael S. Tsirkin
Quoted 10 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 7 lines
Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky.

Quoted 5 lines

How can this be controlled by guest? HVA was generated from qemu ram blocks which is totally under the control of qemu memory core instead of guest.

Thanks

It is ultimately under guest influence as guest supplies IOVA->GPA translations. qemu translates GPA->HVA and gives the translated result to the kernel. If it's not buggy and kernel isn't buggy it's all fine.

But that's the approach that was proven not to work in the 20th century. In the 21st century we are trying defence in depth approach.

My point is that a single code path that is responsible for the HVA translations is better than two.

Quoted 5 lines
Quoted 5 lines

If
translations to HVA are done in a single place through a single table it's safer as there's a single risky place.

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

maintainer
#32 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Jason Wang
Quoted 20 lines · Jason Wang
Quoted 9 lines · Michael S. Tsirkin
Quoted 9 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Quoted 8 lines
Quoted 1 line

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 5 lines

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

The difference is in security not in performance. Getting a bad HVA corrupts QEMU memory and it might be guest controlled. Very risky. If translations to HVA are done in a single place through a single table it's safer as there's a single risky place.

Signature · 5 lines
MST
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
author
#33 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Michael S. Tsirkin
Quoted 9 lines · Michael S. Tsirkin
Quoted 9 lines

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Quoted 8 lines
Quoted 1 line

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

I wonder how much we can gain through this. Currently, qemu IOMMU gives GIOVA->GPA mapping, and qemu vhost code will translate GPA to HVA then pass GIOVA->HVA to vhost. It looks no difference to me.

Thanks

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

maintainer
#34 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Jason Wang
Quoted 39 lines · Jason Wang
Quoted 21 lines · Michael S. Tsirkin
Quoted 13 lines · Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

Quoted 7 lines

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Quoted 5 lines

It's a nasty bug for sure but it's been like this for a long time so I'm inclined to say let's put it in 4.21, and queue for stable.

So please split this out from this series.

Ok.

Quoted 2 lines

Also, I'd like to see a feature bit that allows GPA in IOTLBs.

Just to make sure I understand this. It looks to me we should:

Quoted 8 lines

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Not really. We already have GPA->HVA, so I suggested a flag to pass GIOVA->GPA in the IOTLB.

This has advantages for security since a single table needs then to be validated to ensure guest does not corrupt QEMU memory.

Quoted 178 lines
Quoted 54 lines
Quoted 41 lines

---
  drivers/vhost/net.c   |  3 +-
  drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
  drivers/vhost/vhost.h |  3 +-
  3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
  		if (nvq->done_idx > VHOST_NET_BATCH)
  			vhost_net_signal_used(nvq);
  		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
  		total_len += vhost_len;
  		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
  			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +12 -0
index 55e5aa662ad5..3660310604fd 100644
@@ -1733,11 +1733,67 @@ static int log_write(void __user *log_base,
  	return r;
  }
+static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
+{
+	struct vhost_umem *umem = vq->umem;
+	struct vhost_umem_node *u;
+	u64 gpa;
+	int r;
+	bool hit = false;
+
+	list_for_each_entry(u, &umem->umem_list, link) {
+		if (u->userspace_addr < hva &&
+		    u->userspace_addr + u->size >=
+		    hva + len) {

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1756

Quoted 10 lines
drivers/vhost/vhost.c +10 -0
+			gpa = u->start + hva - u->userspace_addr;
+			r = log_write(vq->log_base, gpa, len);
+			if (r < 0)
+				return r;
+			hit = true;
+		}
+	}
+
+	/* No reverse mapping, should be a bug */
+	WARN_ON(!hit);

Maybe it should but userspace can trigger this easily I think. We need to stop the device not warn in kernel log.

Quoted 2 lines

Also there's an error fd: VHOST_SET_VRING_ERR, need to wake it up.

Ok.

Quoted 7 lines
Quoted 7 lines
drivers/vhost/vhost.c +7 -0
+	return 0;
+}
+
+static void log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
+{
+	struct iovec iov[64];
+	int i, ret;
Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1769

Quoted 5 lines
drivers/vhost/vhost.c +5 -0
+
+	if (!vq->iotlb) {
+		log_write(vq->log_base, vq->log_addr + used_offset, len);
+		return;
+	}

This change seems questionable. used ring writes use their own machinery it does not go through iotlb. Same should apply to log I think.

The problem is used ring may not be physically contiguous with Device IOTLB enabled. So it should go through it.

Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1773

Quoted 5 lines
drivers/vhost/vhost.c +4 -0
+
+	ret = translate_desc(vq, (u64)(uintptr_t)vq->used + used_offset,
+			     len, iov, 64, VHOST_ACCESS_WO);
+	WARN_ON(ret < 0);

Same thing here. translation failures can be triggered from guest. warn on is not a good error handling strategy ...

Ok. Let me fix it.

Thanks

Quoted 81 lines
Quoted 81 lines
drivers/vhost/vhost.c +27 -14
+
+	for (i = 0; i < ret; i++) {
+		ret = log_write_hva(vq,	(u64)(uintptr_t)iov[i].iov_base,
+				    iov[i].iov_len);
+		WARN_ON(ret);
+	}
+}
+
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len)
+		    unsigned int log_num, u64 len, struct iovec *iov, int count)
  {
  	int i, r;
+	if (vq->iotlb) {
+		for (i = 0; i < count; i++) {
+			r = log_write_hva(vq, (u64)(uintptr_t)iov[i].iov_base,
+					  iov[i].iov_len);
+			if (r < 0)
+				return r;
+		}
+		return 0;
+	}
+
  	/* Make sure data written is seen before log. */
  	smp_wmb();
  	for (i = 0; i < log_num; ++i) {

@@ -1769,9 +1825,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
  		smp_wmb();
  		/* Log used flag write. */
  		used = &vq->used->flags;
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof vq->used->flags);
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof vq->used->flags);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -1789,9 +1844,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
  		smp_wmb();
  		/* Log avail event write */
  		used = vhost_avail_event(vq);
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof *vhost_avail_event(vq));
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof *vhost_avail_event(vq));
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -2191,10 +2245,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
  		/* Make sure data is seen before log. */
  		smp_wmb();
  		/* Log used ring entry write. */
-		log_write(vq->log_base,
-			  vq->log_addr +
-			   ((void __user *)used - (void __user *)vq->used),
-			  count * sizeof *used);
+		log_used(vq, ((void __user *)used - (void __user *)vq->used),
+			 count * sizeof *used);
  	}
  	old = vq->last_used_idx;
  	new = (vq->last_used_idx += count);

@@ -2236,9 +2288,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
  		/* Make sure used idx is seen before log. */
  		smp_wmb();
  		/* Log used index update. */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used, idx),
-			  sizeof vq->used->idx);
+		log_used(vq, offsetof(struct vring_used, idx),
+			 sizeof vq->used->idx);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}
drivers/vhost/vhost.h +0 -0
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
  bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/vhost/vhost.h +2 -1
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
  int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
  struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1

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

author
#35 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Michael S. Tsirkin
Quoted 21 lines · Michael S. Tsirkin
Quoted 13 lines · Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

Quoted 7 lines

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>
Quoted 5 lines

It's a nasty bug for sure but it's been like this for a long time so I'm inclined to say let's put it in 4.21, and queue for stable.

So please split this out from this series.

Ok.

Quoted 2 lines

Also, I'd like to see a feature bit that allows GPA in IOTLBs.

Just to make sure I understand this. It looks to me we should:

- allow passing GIOVA->GPA through UAPI

- cache GIOVA->GPA somewhere but still use GIOVA->HVA in device IOTLB for performance

Is this what you suggest?

Thanks

Quoted 54 lines
Quoted 41 lines
---
  drivers/vhost/net.c   |  3 +-
  drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
  drivers/vhost/vhost.h |  3 +-
  3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
  		if (nvq->done_idx > VHOST_NET_BATCH)
  			vhost_net_signal_used(nvq);
  		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
  		total_len += vhost_len;
  		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
  			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +12 -0
index 55e5aa662ad5..3660310604fd 100644
@@ -1733,11 +1733,67 @@ static int log_write(void __user *log_base,
  	return r;
  }
 
+static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
+{
+	struct vhost_umem *umem = vq->umem;
+	struct vhost_umem_node *u;
+	u64 gpa;
+	int r;
+	bool hit = false;
+
+	list_for_each_entry(u, &umem->umem_list, link) {
+		if (u->userspace_addr < hva &&
+		    u->userspace_addr + u->size >=
+		    hva + len) {

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1757

Quoted 10 lines
drivers/vhost/vhost.c +10 -0
+			gpa = u->start + hva - u->userspace_addr;
+			r = log_write(vq->log_base, gpa, len);
+			if (r < 0)
+				return r;
+			hit = true;
+		}
+	}
+
+	/* No reverse mapping, should be a bug */
+	WARN_ON(!hit);

Maybe it should but userspace can trigger this easily I think. We need to stop the device not warn in kernel log.

Quoted 2 lines

Also there's an error fd: VHOST_SET_VRING_ERR, need to wake it up.

Ok.

Quoted 7 lines
Quoted 7 lines
drivers/vhost/vhost.c +7 -0
+	return 0;
+}
+
+static void log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
+{
+	struct iovec iov[64];
+	int i, ret;
Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1769

Quoted 5 lines
drivers/vhost/vhost.c +5 -0
+
+	if (!vq->iotlb) {
+		log_write(vq->log_base, vq->log_addr + used_offset, len);
+		return;
+	}

This change seems questionable. used ring writes use their own machinery it does not go through iotlb. Same should apply to log I think.

The problem is used ring may not be physically contiguous with Device IOTLB enabled. So it should go through it.

Quoted 8 lines

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1773

Quoted 5 lines
drivers/vhost/vhost.c +4 -0
+
+	ret = translate_desc(vq, (u64)(uintptr_t)vq->used + used_offset,
+			     len, iov, 64, VHOST_ACCESS_WO);
+	WARN_ON(ret < 0);

Same thing here. translation failures can be triggered from guest. warn on is not a good error handling strategy ...

Ok. Let me fix it.

Thanks

Quoted 89 lines
Quoted 89 lines
drivers/vhost/vhost.c +27 -14
+
+	for (i = 0; i < ret; i++) {
+		ret = log_write_hva(vq,	(u64)(uintptr_t)iov[i].iov_base,
+				    iov[i].iov_len);
+		WARN_ON(ret);
+	}
+}
+
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len)
+		    unsigned int log_num, u64 len, struct iovec *iov, int count)
  {
  	int i, r;
 
+	if (vq->iotlb) {
+		for (i = 0; i < count; i++) {
+			r = log_write_hva(vq, (u64)(uintptr_t)iov[i].iov_base,
+					  iov[i].iov_len);
+			if (r < 0)
+				return r;
+		}
+		return 0;
+	}
+
  	/* Make sure data written is seen before log. */
  	smp_wmb();
  	for (i = 0; i < log_num; ++i) {

@@ -1769,9 +1825,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
  		smp_wmb();
  		/* Log used flag write. */
  		used = &vq->used->flags;
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof vq->used->flags);
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof vq->used->flags);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -1789,9 +1844,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
  		smp_wmb();
  		/* Log avail event write */
  		used = vhost_avail_event(vq);
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof *vhost_avail_event(vq));
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof *vhost_avail_event(vq));
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}

@@ -2191,10 +2245,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
  		/* Make sure data is seen before log. */
  		smp_wmb();
  		/* Log used ring entry write. */
-		log_write(vq->log_base,
-			  vq->log_addr +
-			   ((void __user *)used - (void __user *)vq->used),
-			  count * sizeof *used);
+		log_used(vq, ((void __user *)used - (void __user *)vq->used),
+			 count * sizeof *used);
  	}
  	old = vq->last_used_idx;
  	new = (vq->last_used_idx += count);

@@ -2236,9 +2288,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
  		/* Make sure used idx is seen before log. */
  		smp_wmb();
  		/* Log used index update. */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used, idx),
-			  sizeof vq->used->idx);
+		log_used(vq, offsetof(struct vring_used, idx),
+			 sizeof vq->used->idx);
  		if (vq->log_ctx)
  			eventfd_signal(vq->log_ctx, 1);
  	}
drivers/vhost/vhost.h +2 -1
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
  bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
  int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
Quoted 3 lines
Quoted 3 lines
drivers/vhost/vhost.h +0 -0
  struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1

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

maintainer
#36 lore raw

Re: [PATCH net V2 4/4] vhost: log dirty page correctly

In reply to Jason Wang
Quoted 13 lines · Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

Quoted 7 lines

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>

It's a nasty bug for sure but it's been like this for a long time so I'm inclined to say let's put it in 4.21, and queue for stable.

So please split this out from this series.

Also, I'd like to see a feature bit that allows GPA in IOTLBs.

Quoted 40 lines
---
 drivers/vhost/net.c   |  3 +-
 drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
 drivers/vhost/vhost.h |  3 +-
 3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
 		if (nvq->done_idx > VHOST_NET_BATCH)
 			vhost_net_signal_used(nvq);
 		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
 		total_len += vhost_len;
 		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
 			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +12 -0
index 55e5aa662ad5..3660310604fd 100644
@@ -1733,11 +1733,67 @@ static int log_write(void __user *log_base,
 	return r;
 }
 
+static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
+{
+	struct vhost_umem *umem = vq->umem;
+	struct vhost_umem_node *u;
+	u64 gpa;
+	int r;
+	bool hit = false;
+
+	list_for_each_entry(u, &umem->umem_list, link) {
+		if (u->userspace_addr < hva &&
+		    u->userspace_addr + u->size >=
+		    hva + len) {

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1757

Quoted 10 lines
drivers/vhost/vhost.c +10 -0
+			gpa = u->start + hva - u->userspace_addr;
+			r = log_write(vq->log_base, gpa, len);
+			if (r < 0)
+				return r;
+			hit = true;
+		}
+	}
+
+	/* No reverse mapping, should be a bug */
+	WARN_ON(!hit);

Maybe it should but userspace can trigger this easily I think. We need to stop the device not warn in kernel log.

Also there's an error fd: VHOST_SET_VRING_ERR, need to wake it up.

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1769

Quoted 12 lines
drivers/vhost/vhost.c +12 -0
+	return 0;
+}
+
+static void log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
+{
+	struct iovec iov[64];
+	int i, ret;
+
+	if (!vq->iotlb) {
+		log_write(vq->log_base, vq->log_addr + used_offset, len);
+		return;
+	}

This change seems questionable. used ring writes use their own machinery it does not go through iotlb. Same should apply to log I think.

drivers/vhost/vhost.c static int log_write(void __user *log_base, · line 1773

Quoted 4 lines
drivers/vhost/vhost.c +4 -0
+
+	ret = translate_desc(vq, (u64)(uintptr_t)vq->used + used_offset,
+			     len, iov, 64, VHOST_ACCESS_WO);
+	WARN_ON(ret < 0);

Same thing here. translation failures can be triggered from guest. warn on is not a good error handling strategy ...

Quoted 92 lines
drivers/vhost/vhost.c +27 -14
+
+	for (i = 0; i < ret; i++) {
+		ret = log_write_hva(vq,	(u64)(uintptr_t)iov[i].iov_base,
+				    iov[i].iov_len);
+		WARN_ON(ret);
+	}
+}
+
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len)
+		    unsigned int log_num, u64 len, struct iovec *iov, int count)
 {
 	int i, r;
 
+	if (vq->iotlb) {
+		for (i = 0; i < count; i++) {
+			r = log_write_hva(vq, (u64)(uintptr_t)iov[i].iov_base,
+					  iov[i].iov_len);
+			if (r < 0)
+				return r;
+		}
+		return 0;
+	}
+
 	/* Make sure data written is seen before log. */
 	smp_wmb();
 	for (i = 0; i < log_num; ++i) {

@@ -1769,9 +1825,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
 		smp_wmb();
 		/* Log used flag write. */
 		used = &vq->used->flags;
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof vq->used->flags);
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof vq->used->flags);
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}

@@ -1789,9 +1844,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
 		smp_wmb();
 		/* Log avail event write */
 		used = vhost_avail_event(vq);
-		log_write(vq->log_base, vq->log_addr +
-			  (used - (void __user *)vq->used),
-			  sizeof *vhost_avail_event(vq));
+		log_used(vq, (used - (void __user *)vq->used),
+			 sizeof *vhost_avail_event(vq));
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}

@@ -2191,10 +2245,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
 		/* Make sure data is seen before log. */
 		smp_wmb();
 		/* Log used ring entry write. */
-		log_write(vq->log_base,
-			  vq->log_addr +
-			   ((void __user *)used - (void __user *)vq->used),
-			  count * sizeof *used);
+		log_used(vq, ((void __user *)used - (void __user *)vq->used),
+			 count * sizeof *used);
 	}
 	old = vq->last_used_idx;
 	new = (vq->last_used_idx += count);

@@ -2236,9 +2288,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
 		/* Make sure used idx is seen before log. */
 		smp_wmb();
 		/* Log used index update. */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used, idx),
-			  sizeof vq->used->idx);
+		log_used(vq, offsetof(struct vring_used, idx),
+			 sizeof vq->used->idx);
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}
drivers/vhost/vhost.h +2 -1
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
 bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
 int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
 
 struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1
author
#37 lore raw

PATCH v2 4/4 vhost: log dirty page correctly

In reply to Jason Wang

Vhost dirty page logging API is designed to sync through GPA. But we try to log GIOVA when device IOTLB is enabled. This is wrong and may lead to missing data after migration.

To solve this issue, when logging with device IOTLB enabled, we will:

1) reuse the device IOTLB translation result of GIOVA->HVA mapping to get HVA, for writable descriptor, get HVA through iovec. For used ring update, translate its GIOVA to HVA
2) traverse the GPA->HVA mapping to get the possible GPA and log through GPA. Pay attention this reverse mapping is not guaranteed to be unique, so we should log each possible GPA in this case.

This fix the failure of scp to guest during migration. In -next, we will probably support passing GIOVA->GPA instead of GIOVA->HVA.

Fixes 6b1e6cc7855b ("vhost: new device IOTLB API")
Reported-by Jintack Lim <[email protected]>
Cc Jintack Lim <[email protected]>
Signed-off-by Jason Wang <[email protected]>
---
 drivers/vhost/net.c   |  3 +-
 drivers/vhost/vhost.c | 79 +++++++++++++++++++++++++++++++++++--------
 drivers/vhost/vhost.h |  3 +-
 3 files changed, 69 insertions(+), 16 deletions(-)
drivers/vhost/net.c +2 -1
index ad7a6f475a44..784df2b49628 100644
@@ -1192,7 +1192,8 @@ static void handle_rx(struct vhost_net *net)
 		if (nvq->done_idx > VHOST_NET_BATCH)
 			vhost_net_signal_used(nvq);
 		if (unlikely(vq_log))
-			vhost_log_write(vq, vq_log, log, vhost_len);
+			vhost_log_write(vq, vq_log, log, vhost_len,
+					vq->iov, in);
 		total_len += vhost_len;
 		if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
 			vhost_poll_queue(&vq->poll);
drivers/vhost/vhost.c +65 -14

113 lines not on this page · raw message

drivers/vhost/vhost.h +2 -1
index 466ef7542291..1b675dad5e05 100644
@@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
 bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-		    unsigned int log_num, u64 len);
+		    unsigned int log_num, u64 len,
+		    struct iovec *iov, int count);
 int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
 
 struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
Signature · 1 line
2.17.1
maintainer
#38 lore raw
In reply to Jason Wang

From: Jason Wang <[email protected]>
Date: Wed, 12 Dec 2018 18:08:15 +0800

Quoted 11 lines

This series tries to fix various issues of vhost:

- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
- Patch 4-7 fixes the diry page logging when device IOTLB is enabled. We should done through GPA instead of GIOVA, this was done through intorudce HVA->GPA reverse mapping and convert HVA to GPA during logging dirty pages.

Quoted 8 lines

Please consider them for -stable.

Thanks

Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Hello Jason.

Look like Michael wants you to split out patch #4 and target net-next with it.

So please do that and respin the first 3 patches here with Michael's ACKs.

Thanks.

author
#39 lore raw
Quoted 24 lines · David Miller

From: Jason Wang <[email protected]>
Date: Wed, 12 Dec 2018 18:08:15 +0800

Quoted 11 lines

This series tries to fix various issues of vhost:

- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
- Patch 4-7 fixes the diry page logging when device IOTLB is enabled. We should done through GPA instead of GIOVA, this was done through intorudce HVA->GPA reverse mapping and convert HVA to GPA during logging dirty pages.

Quoted 8 lines

Please consider them for -stable.

Thanks

Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Hello Jason.

Quoted 7 lines

Look like Michael wants you to split out patch #4 and target net-next with it.

So please do that and respin the first 3 patches here with Michael's ACKs.

Thanks.

Yes, will send V3.

Thanks

author
#40 lore raw
In reply to David Miller
Quoted 24 lines · David Miller

From: Jason Wang <[email protected]>
Date: Wed, 12 Dec 2018 18:08:15 +0800

Quoted 11 lines

This series tries to fix various issues of vhost:

- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
- Patch 4-7 fixes the diry page logging when device IOTLB is enabled. We should done through GPA instead of GIOVA, this was done through intorudce HVA->GPA reverse mapping and convert HVA to GPA during logging dirty pages.

Quoted 8 lines

Please consider them for -stable.

Thanks

Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Hello Jason.

Quoted 7 lines

Look like Michael wants you to split out patch #4 and target net-next with it.

So please do that and respin the first 3 patches here with Michael's ACKs.

Thanks.

Yes, will send V3.

Thanks

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

maintainer
#41 lore raw
In reply to Jason Wang

From: Jason Wang <[email protected]>
Date: Wed, 12 Dec 2018 18:08:15 +0800

Quoted 11 lines

This series tries to fix various issues of vhost:

- Patch 1 adds a missing write barrier between used idx updating and logging.
- Patch 2-3 brings back the protection of device IOTLB through vq mutex, this fixes possible use after free in device IOTLB entries.
- Patch 4-7 fixes the diry page logging when device IOTLB is enabled. We should done through GPA instead of GIOVA, this was done through intorudce HVA->GPA reverse mapping and convert HVA to GPA during logging dirty pages.

Quoted 8 lines

Please consider them for -stable.

Thanks

Changes from V1:
- silent compiler warning for 32bit.
- use mutex_trylock() on slowpath instead of mutex_lock() even on fast path.

Hello Jason.

Look like Michael wants you to split out patch #4 and target net-next with it.

So please do that and respin the first 3 patches here with Michael's ACKs.

Thanks.