[2/2] vhost: set log when updating used flags or avail event
diff mbox series

Message ID 20110621100438.6777.20695.stgit@dhcp-91-7.nay.redhat.com.englab.nay.redhat.com
State New, archived
Headers show
Series
  • [1/2] vhost: init used ring after backend was set
Related show

Commit Message

Jason Wang June 21, 2011, 10:04 a.m. UTC
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(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Michael S. Tsirkin July 7, 2011, 12:57 p.m. UTC | #1
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();
Jason Wang July 8, 2011, 10:38 a.m. UTC | #2
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();

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

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