[PATCH 1/2] vhost: init used ring after backend was set

[PATCH 1/2] vhost: init used ring after backend was set

[Jason Wang]

This patch move the used ring initialization after backend was set. This make us
possible to disable the backend and tweak the used ring then restart. And it's
also useful for log setting as used ring have been checked then.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c   |    4 ++++
 drivers/vhost/vhost.c |   11 +++--------
 drivers/vhost/vhost.h |    1 +
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index e224a92..957421b 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -637,6 +637,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 		vhost_net_enable_vq(n, vq);
 	}
 
+	r = init_used(vq);
+	if (r)
+		goto err_vq;
+
 	mutex_unlock(&vq->mutex);
 
 	if (oldsock) {
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2a10786..43a3fc6 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -578,15 +578,14 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
 	return 0;
 }
 
-static int init_used(struct vhost_virtqueue *vq,
-		     struct vring_used __user *used)
+int init_used(struct vhost_virtqueue *vq)
 {
-	int r = put_user(vq->used_flags, &used->flags);
+	int r = put_user(vq->used_flags, &vq->used->flags);
 
 	if (r)
 		return r;
 	vq->signalled_used_valid = false;
-	return get_user(vq->last_used_idx, &used->idx);
+	return get_user(vq->last_used_idx, &vq->used->idx);
 }
 
 static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
@@ -701,10 +700,6 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 			}
 		}
 
-		r = init_used(vq, (struct vring_used __user *)(unsigned long)
-			      a.used_user_addr);
-		if (r)
-			break;
 		vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
 		vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
 		vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 64889d2..bd9f196 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -147,6 +147,7 @@ int vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
 		      struct vhost_log *log, unsigned int *log_num);
 void vhost_discard_vq_desc(struct vhost_virtqueue *, int n);
 
+int init_used(struct vhost_virtqueue *);
 int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
 int vhost_add_used_n(struct vhost_virtqueue *, struct vring_used_elem *heads,
 		     unsigned count);

[PATCH 2/2] vhost: set log when updating used flags or avail event

[Jason Wang]

We need set log when updating used flags and avail event. Otherwise guest may
see stale values after migration and then do not exit or exit unexpectedly.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/vhost.c |   61 +++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 43a3fc6..c344d4f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -578,16 +578,6 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
 	return 0;
 }
 
-int init_used(struct vhost_virtqueue *vq)
-{
-	int r = put_user(vq->used_flags, &vq->used->flags);
-
-	if (r)
-		return r;
-	vq->signalled_used_valid = false;
-	return get_user(vq->last_used_idx, &vq->used->idx);
-}
-
 static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 {
 	struct file *eventfp, *filep = NULL,
@@ -954,6 +944,51 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
 	return 0;
 }
 
+static int vhost_update_used_flags(struct vhost_virtqueue *vq)
+{
+	if (put_user(vq->used_flags, &vq->used->flags) < 0)
+		return -EFAULT;
+	if (unlikely(vq->log_used)) {
+		/* Make sure the flag is seen before log. */
+		smp_wmb();
+		/* Log used flag write. */
+		log_write(vq->log_base,
+			  vq->log_addr + offsetof(struct vring_used, flags),
+			  sizeof vq->used->flags);
+		if (vq->log_ctx)
+			eventfd_signal(vq->log_ctx, 1);
+	}
+	return 0;
+}
+
+static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
+{
+	if (put_user(vq->avail_idx, vhost_avail_event(vq)))
+		return -EFAULT;
+	if (unlikely(vq->log_used)) {
+		/* Make sure the event is seen before log. */
+		smp_wmb();
+		/* Log avail event write */
+		log_write(vq->log_base,
+			  vq->log_addr + offsetof(struct vring_used,
+						  ring[vq->num]),
+			  sizeof avail_event);
+		if (vq->log_ctx)
+			eventfd_signal(vq->log_ctx, 1);
+	}
+	return 0;
+}
+
+int init_used(struct vhost_virtqueue *vq)
+{
+	int r = vhost_update_used_flags(vq);
+
+	if (r)
+		return r;
+	vq->signalled_used_valid = false;
+	return get_user(vq->last_used_idx, &vq->used->idx);
+}
+
 static int translate_desc(struct vhost_dev *dev, u64 addr, u32 len,
 			  struct iovec iov[], int iov_size)
 {
@@ -1425,14 +1460,14 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 		return false;
 	vq->used_flags &= ~VRING_USED_F_NO_NOTIFY;
 	if (!vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
-		r = put_user(vq->used_flags, &vq->used->flags);
+		r = vhost_update_used_flags(vq);
 		if (r) {
 			vq_err(vq, "Failed to enable notification at %p: %d\n",
 			       &vq->used->flags, r);
 			return false;
 		}
 	} else {
-		r = put_user(vq->avail_idx, vhost_avail_event(vq));
+		r = vhost_update_avail_event(vq, vq->avail_idx);
 		if (r) {
 			vq_err(vq, "Failed to update avail event index at %p: %d\n",
 			       vhost_avail_event(vq), r);
@@ -1475,7 +1510,7 @@ void vhost_disable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 		return;
 	vq->used_flags |= VRING_USED_F_NO_NOTIFY;
 	if (!vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX)) {
-		r = put_user(vq->used_flags, &vq->used->flags);
+		r = vhost_update_used_flags(vq);
 		if (r)
 			vq_err(vq, "Failed to enable notification at %p: %d\n",
 			       &vq->used->flags, r);

Re: [PATCH 2/2] vhost: set log when updating used flags or avail event

[Michael S. Tsirkin]

Subject: vhost: used ring logging cleanup

remove extra log bit setting for used ring updates: it's no longer
necessary.  Also, use vhost_avail_event instead of duplicating offset
math.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

> We need set log when updating used flags and avail event. Otherwise guest may
> see stale values after migration and then do not exit or exit unexpectedly.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

OK but this means we set the log twice now.
Also, hardcording offset is not as nice as using
vhost_avail_event. So I think the below is needed
on top. Comments?

 drivers/vhost/vhost.c |   29 +++++++++--------------------
 1 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 540591b..c5f96ba 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -946,14 +946,16 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
 
 static int vhost_update_used_flags(struct vhost_virtqueue *vq)
 {
+	void __user *used;
 	if (put_user(vq->used_flags, &vq->used->flags) < 0)
 		return -EFAULT;
 	if (unlikely(vq->log_used)) {
 		/* Make sure the flag is seen before log. */
 		smp_wmb();
 		/* Log used flag write. */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used, flags),
+		used = &vq->used->flags;
+		log_write(vq->log_base, vq->log_addr +
+			  (used - (void __user *)vq->used),
 			  sizeof vq->used->flags);
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
@@ -966,13 +968,14 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
 	if (put_user(vq->avail_idx, vhost_avail_event(vq)))
 		return -EFAULT;
 	if (unlikely(vq->log_used)) {
+		void __user *used;
 		/* Make sure the event is seen before log. */
 		smp_wmb();
 		/* Log avail event write */
-		log_write(vq->log_base,
-			  vq->log_addr + offsetof(struct vring_used,
-						  ring[vq->num]),
-			  sizeof avail_event);
+		used = vhost_avail_event(vq);
+		log_write(vq->log_base, vq->log_addr +
+			  (used - (void __user *)vq->used),
+			  sizeof *vhost_avail_event(vq));
 		if (vq->log_ctx)
 			eventfd_signal(vq->log_ctx, 1);
 	}
@@ -1474,20 +1477,6 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 			return false;
 		}
 	}
-	if (unlikely(vq->log_used)) {
-		void __user *used;
-		/* Make sure data is seen before log. */
-		smp_wmb();
-		used = vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX) ?
-			&vq->used->flags : vhost_avail_event(vq);
-		/* Log used flags or event index entry write. Both are 16 bit
-		 * fields. */
-		log_write(vq->log_base, vq->log_addr +
-			   (used - (void __user *)vq->used),
-			  sizeof(u16));
-		if (vq->log_ctx)
-			eventfd_signal(vq->log_ctx, 1);
-	}
 	/* They could have slipped one in as we were doing that: make
 	 * sure it's written, then check again. */
 	smp_mb();
-- 
1.7.5.53.gc233e

Re: [PATCH 2/2] vhost: set log when updating used flags or avail event

[Jason Wang]

On 07/07/2011 08:57 PM, Michael S. Tsirkin wrote:
> Subject: vhost: used ring logging cleanup
>
> remove extra log bit setting for used ring updates: it's no longer
> necessary.  Also, use vhost_avail_event instead of duplicating offset
> math.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>
>> We need set log when updating used flags and avail event. Otherwise guest may
>> see stale values after migration and then do not exit or exit unexpectedly.
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> OK but this means we set the log twice now.
> Also, hardcording offset is not as nice as using
> vhost_avail_event. So I think the below is needed
> on top. Comments?
>

Right, thanks for the cleanup.

>   drivers/vhost/vhost.c |   29 +++++++++--------------------
>   1 files changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 540591b..c5f96ba 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -946,14 +946,16 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
>
>   static int vhost_update_used_flags(struct vhost_virtqueue *vq)
>   {
> +	void __user *used;
>   	if (put_user(vq->used_flags,&vq->used->flags)<  0)
>   		return -EFAULT;
>   	if (unlikely(vq->log_used)) {
>   		/* Make sure the flag is seen before log. */
>   		smp_wmb();
>   		/* Log used flag write. */
> -		log_write(vq->log_base,
> -			  vq->log_addr + offsetof(struct vring_used, flags),
> +		used =&vq->used->flags;
> +		log_write(vq->log_base, vq->log_addr +
> +			  (used - (void __user *)vq->used),
>   			  sizeof vq->used->flags);
>   		if (vq->log_ctx)
>   			eventfd_signal(vq->log_ctx, 1);
> @@ -966,13 +968,14 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
>   	if (put_user(vq->avail_idx, vhost_avail_event(vq)))
>   		return -EFAULT;
>   	if (unlikely(vq->log_used)) {
> +		void __user *used;
>   		/* Make sure the event is seen before log. */
>   		smp_wmb();
>   		/* Log avail event write */
> -		log_write(vq->log_base,
> -			  vq->log_addr + offsetof(struct vring_used,
> -						  ring[vq->num]),
> -			  sizeof avail_event);
> +		used = vhost_avail_event(vq);
> +		log_write(vq->log_base, vq->log_addr +
> +			  (used - (void __user *)vq->used),
> +			  sizeof *vhost_avail_event(vq));
>   		if (vq->log_ctx)
>   			eventfd_signal(vq->log_ctx, 1);
>   	}
> @@ -1474,20 +1477,6 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>   			return false;
>   		}
>   	}
> -	if (unlikely(vq->log_used)) {
> -		void __user *used;
> -		/* Make sure data is seen before log. */
> -		smp_wmb();
> -		used = vhost_has_feature(dev, VIRTIO_RING_F_EVENT_IDX) ?
> -			&vq->used->flags : vhost_avail_event(vq);
> -		/* Log used flags or event index entry write. Both are 16 bit
> -		 * fields. */
> -		log_write(vq->log_base, vq->log_addr +
> -			   (used - (void __user *)vq->used),
> -			  sizeof(u16));
> -		if (vq->log_ctx)
> -			eventfd_signal(vq->log_ctx, 1);
> -	}
>   	/* They could have slipped one in as we were doing that: make
>   	 * sure it's written, then check again. */
>   	smp_mb();