[RFC,04/11] virtio_ring: implement endian reversal based on VERSION_1 feature.

Commit Message

Cornelia Huck Oct. 7, 2014, 2:39 p.m. UTC

From: Rusty Russell <[e-mail]>

[Cornelia Huck: we don't need the vq->vring.num -> vq->ring_mask change]
Signed-off-by: Rusty Russell <[e-mail]>
Signed-off-by: Cornelia Huck <[e-mail]>
---
 drivers/virtio/virtio_ring.c |  195 ++++++++++++++++++++++++++++++------------
 1 file changed, 138 insertions(+), 57 deletions(-)

Comments

Michael S. Tsirkin Oct. 22, 2014, 2:02 p.m. UTC | #1

On Tue, Oct 07, 2014 at 04:39:45PM +0200, Cornelia Huck wrote:
> From: Rusty Russell <[e-mail]>
> 
> [Cornelia Huck: we don't need the vq->vring.num -> vq->ring_mask change]
> Signed-off-by: Rusty Russell <[e-mail]>
> Signed-off-by: Cornelia Huck <[e-mail]>
> ---
>  drivers/virtio/virtio_ring.c |  195 ++++++++++++++++++++++++++++++------------
>  1 file changed, 138 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 1cfb5ba..350c39b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -145,42 +145,54 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq,
>  	i = 0;
>  	for (n = 0; n < out_sgs; n++) {
>  		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
> -			desc[i].flags = VRING_DESC_F_NEXT;
> -			desc[i].addr = sg_phys(sg);
> -			desc[i].len = sg->length;
> -			desc[i].next = i+1;
> +			desc[i].flags = cpu_to_virtio16(vq->vq.vdev,
> +							  VRING_DESC_F_NEXT);
> +			desc[i].addr = cpu_to_virtio64(vq->vq.vdev,
> +							 sg_phys(sg));
> +			desc[i].len = cpu_to_virtio32(vq->vq.vdev,
> +							sg->length);
> +			desc[i].next = cpu_to_virtio16(vq->vq.vdev,
> +							 i+1);
>  			i++;
>  		}
>  	}
>  	for (; n < (out_sgs + in_sgs); n++) {
>  		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
> -			desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
> -			desc[i].addr = sg_phys(sg);
> -			desc[i].len = sg->length;
> -			desc[i].next = i+1;
> +			desc[i].flags =	cpu_to_virtio16(vq->vq.vdev,
> +							  VRING_DESC_F_NEXT|
> +							  VRING_DESC_F_WRITE);
> +			desc[i].addr = cpu_to_virtio64(vq->vq.vdev,
> +							 sg_phys(sg));
> +			desc[i].len = cpu_to_virtio32(vq->vq.vdev,
> +							sg->length);
> +			desc[i].next = cpu_to_virtio16(vq->vq.vdev, i+1);
>  			i++;
>  		}
>  	}
> -	BUG_ON(i != total_sg);
>  
>  	/* Last one doesn't continue. */
> -	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
> +	desc[i-1].flags &= ~cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
>  	desc[i-1].next = 0;
>  
> -	/* We're about to use a buffer */
> -	vq->vq.num_free--;
> -
>  	/* Use a single buffer which doesn't continue */
>  	head = vq->free_head;
> -	vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
> -	vq->vring.desc[head].addr = virt_to_phys(desc);
> +	vq->vring.desc[head].flags =
> +		cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT);
> +	vq->vring.desc[head].addr =
> +		cpu_to_virtio64(vq->vq.vdev, virt_to_phys(desc));
>  	/* kmemleak gives a false positive, as it's hidden by virt_to_phys */
>  	kmemleak_ignore(desc);
> -	vq->vring.desc[head].len = i * sizeof(struct vring_desc);
> +	vq->vring.desc[head].len =
> +		cpu_to_virtio32(vq->vq.vdev, i * sizeof(struct vring_desc));
>  
> -	/* Update free pointer */
> +	BUG_ON(i != total_sg);
> +

Why move the BUG_ON here?
I think I'll move it back ...

> +	/* Update free pointer (we store this in native endian) */
>  	vq->free_head = vq->vring.desc[head].next;
>  
> +	/* We've just used a buffer */
> +	vq->vq.num_free--;
> +
>  	return head;
>  }
>  
> @@ -199,6 +211,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>  	struct scatterlist *sg;
>  	unsigned int i, n, avail, uninitialized_var(prev), total_sg;
>  	int head;
> +	u16 nexti;
>  
>  	START_USE(vq);
>  
> @@ -253,26 +266,46 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>  	vq->vq.num_free -= total_sg;
>  
>  	head = i = vq->free_head;
> +
>  	for (n = 0; n < out_sgs; n++) {
>  		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
> -			vq->vring.desc[i].flags = VRING_DESC_F_NEXT;
> -			vq->vring.desc[i].addr = sg_phys(sg);
> -			vq->vring.desc[i].len = sg->length;
> +			vq->vring.desc[i].flags =
> +				cpu_to_virtio16(vq->vq.vdev,
> +						  VRING_DESC_F_NEXT);
> +			vq->vring.desc[i].addr =
> +				cpu_to_virtio64(vq->vq.vdev, sg_phys(sg));
> +			vq->vring.desc[i].len =
> +				cpu_to_virtio32(vq->vq.vdev, sg->length);
> +
> +			/* We chained .next in native: fix endian. */
> +			nexti = vq->vring.desc[i].next;
> +			vq->vring.desc[i].next
> +				= virtio_to_cpu_u16(vq->vq.vdev, nexti);
>  			prev = i;
> -			i = vq->vring.desc[i].next;
> +			i = nexti;
>  		}
>  	}
>  	for (; n < (out_sgs + in_sgs); n++) {
>  		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
> -			vq->vring.desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
> -			vq->vring.desc[i].addr = sg_phys(sg);
> -			vq->vring.desc[i].len = sg->length;
> +			vq->vring.desc[i].flags =
> +				cpu_to_virtio16(vq->vq.vdev,
> +						  VRING_DESC_F_NEXT|
> +						  VRING_DESC_F_WRITE);
> +			vq->vring.desc[i].addr =
> +				cpu_to_virtio64(vq->vq.vdev, sg_phys(sg));
> +			vq->vring.desc[i].len =
> +				cpu_to_virtio32(vq->vq.vdev, sg->length);
> +			/* We chained .next in native: fix endian. */
> +			nexti = vq->vring.desc[i].next;
> +			vq->vring.desc[i].next =
> +				virtio_to_cpu_u16(vq->vq.vdev, nexti);
>  			prev = i;
> -			i = vq->vring.desc[i].next;
> +			i = nexti;
>  		}
>  	}
>  	/* Last one doesn't continue. */
> -	vq->vring.desc[prev].flags &= ~VRING_DESC_F_NEXT;
> +	vq->vring.desc[prev].flags &=
> +		~cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
>  
>  	/* Update free pointer */
>  	vq->free_head = i;
> @@ -283,15 +316,16 @@ add_head:
>  
>  	/* Put entry in available array (but don't update avail->idx until they
>  	 * do sync). */
> -	avail = (vq->vring.avail->idx & (vq->vring.num-1));
> -	vq->vring.avail->ring[avail] = head;
> +	avail = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.avail->idx);
> +	vq->vring.avail->ring[avail & (vq->vring.num - 1)] =
> +		cpu_to_virtio16(vq->vq.vdev, head);
>  
> -	/* Descriptors and available array need to be set before we expose the
> -	 * new available array entries. */
> +	/* Descriptors and available array need to be set
> +	 * before we expose the new available array entries. */
>  	virtio_wmb(vq->weak_barriers);
> -	vq->vring.avail->idx++;
> -	vq->num_added++;
> +	vq->vring.avail->idx = cpu_to_virtio16(vq->vq.vdev, avail + 1);
>  
> +	vq->num_added++;
>  	/* This is very unlikely, but theoretically possible.  Kick
>  	 * just in case. */
>  	if (unlikely(vq->num_added == (1 << 16) - 1))
> @@ -408,8 +442,9 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
>  	 * event. */
>  	virtio_mb(vq->weak_barriers);
>  
> -	old = vq->vring.avail->idx - vq->num_added;
> -	new = vq->vring.avail->idx;
> +	new = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.avail->idx);
> +
> +	old = new - vq->num_added;
>  	vq->num_added = 0;
>  
>  #ifdef DEBUG
> @@ -421,10 +456,17 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
>  #endif
>  
>  	if (vq->event) {
> -		needs_kick = vring_need_event(vring_avail_event(&vq->vring),
> -					      new, old);
> +		u16 avail;
> +
> +		avail =	virtio_to_cpu_u16(vq->vq.vdev,
> +					  vring_avail_event(&vq->vring));
> +
> +		needs_kick = vring_need_event(avail, new, old);
>  	} else {
> -		needs_kick = !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> +		u16 flags;
> +
> +		flags = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->flags);
> +		needs_kick = !(flags & VRING_USED_F_NO_NOTIFY);
>  	}
>  	END_USE(vq);
>  	return needs_kick;
> @@ -486,11 +528,20 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
>  	i = head;
>  
>  	/* Free the indirect table */
> -	if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)
> -		kfree(phys_to_virt(vq->vring.desc[i].addr));
> +	if (vq->vring.desc[i].flags &
> +	    cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)) {
> +		kfree(phys_to_virt(virtio_to_cpu_u64(vq->vq.vdev,
> +						     vq->vring.desc[i].addr)));
> +	}
> +
> +	while (vq->vring.desc[i].flags &
> +	       cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT)) {
> +		u16 next;
>  
> -	while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
> -		i = vq->vring.desc[i].next;
> +		/* Convert endian of next back to native. */
> +		next = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.desc[i].next);
> +		vq->vring.desc[i].next = next;
> +		i = next;
>  		vq->vq.num_free++;
>  	}
>  
> @@ -502,7 +553,8 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
>  
>  static inline bool more_used(const struct vring_virtqueue *vq)
>  {
> -	return vq->last_used_idx != vq->vring.used->idx;
> +	return vq->last_used_idx
> +		!= virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->idx);
>  }
>  
>  /**
> @@ -527,6 +579,8 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  	void *ret;
>  	unsigned int i;
>  	u16 last_used;
> +	const int no_intr =
> +		cpu_to_virtio16(vq->vq.vdev, VRING_AVAIL_F_NO_INTERRUPT);
>  
>  	START_USE(vq);
>  
> @@ -545,8 +599,9 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  	virtio_rmb(vq->weak_barriers);
>  
>  	last_used = (vq->last_used_idx & (vq->vring.num - 1));
> -	i = vq->vring.used->ring[last_used].id;
> -	*len = vq->vring.used->ring[last_used].len;
> +	i = virtio_to_cpu_u32(vq->vq.vdev, vq->vring.used->ring[last_used].id);
> +	*len = virtio_to_cpu_u32(vq->vq.vdev,
> +				 vq->vring.used->ring[last_used].len);
>  
>  	if (unlikely(i >= vq->vring.num)) {
>  		BAD_RING(vq, "id %u out of range\n", i);
> @@ -561,10 +616,11 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  	ret = vq->data[i];
>  	detach_buf(vq, i);
>  	vq->last_used_idx++;
> +
>  	/* If we expect an interrupt for the next entry, tell host
>  	 * by writing event index and flush out the write before
>  	 * the read in the next get_buf call. */
> -	if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
> +	if (!(vq->vring.avail->flags & no_intr)) {
>  		vring_used_event(&vq->vring) = vq->last_used_idx;
>  		virtio_mb(vq->weak_barriers);
>  	}
> @@ -591,7 +647,8 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  
> -	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> +	vq->vring.avail->flags |=
> +		cpu_to_virtio16(vq->vq.vdev, VRING_AVAIL_F_NO_INTERRUPT);
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
>  
> @@ -619,8 +676,12 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>  	/* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
>  	 * either clear the flags bit or point the event index at the next
>  	 * entry. Always do both to keep code simple. */
> -	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
> -	vring_used_event(&vq->vring) = last_used_idx = vq->last_used_idx;
> +	vq->vring.avail->flags &=
> +		cpu_to_virtio16(vq->vq.vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
> +	last_used_idx = vq->last_used_idx;
> +	vring_used_event(&vq->vring) = cpu_to_virtio16(vq->vq.vdev,
> +							 last_used_idx);
> +
>  	END_USE(vq);
>  	return last_used_idx;
>  }
> @@ -640,7 +701,9 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  
>  	virtio_mb(vq->weak_barriers);
> -	return (u16)last_used_idx != vq->vring.used->idx;
> +
> +	return (u16)last_used_idx !=
> +		virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->idx);
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_poll);
>  
> @@ -678,7 +741,7 @@ EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
>  bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> -	u16 bufs;
> +	u16 bufs, used_idx;
>  
>  	START_USE(vq);
>  
> @@ -687,12 +750,17 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>  	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
>  	 * either clear the flags bit or point the event index at the next
>  	 * entry. Always do both to keep code simple. */
> -	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
> +	vq->vring.avail->flags &=
> +		cpu_to_virtio16(vq->vq.vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
>  	/* TODO: tune this threshold */
> -	bufs = (u16)(vq->vring.avail->idx - vq->last_used_idx) * 3 / 4;
> -	vring_used_event(&vq->vring) = vq->last_used_idx + bufs;
> +	bufs = (u16)(virtio_to_cpu_u16(vq->vq.vdev, vq->vring.avail->idx)
> +		     - vq->last_used_idx) * 3 / 4;
> +	vring_used_event(&vq->vring) =
> +		cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx + bufs);
>  	virtio_mb(vq->weak_barriers);
> -	if (unlikely((u16)(vq->vring.used->idx - vq->last_used_idx) > bufs)) {
> +	used_idx = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->idx);
> +
> +	if (unlikely((u16)(used_idx - vq->last_used_idx) > bufs)) {
>  		END_USE(vq);
>  		return false;
>  	}
> @@ -719,12 +787,19 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
>  	START_USE(vq);
>  
>  	for (i = 0; i < vq->vring.num; i++) {
> +		u16 avail;
> +
>  		if (!vq->data[i])
>  			continue;
>  		/* detach_buf clears data, so grab it now. */
>  		buf = vq->data[i];
>  		detach_buf(vq, i);
> -		vq->vring.avail->idx--;
> +
> +		/* AKA "vq->vring.avail->idx++" */
> +		avail = virtio_to_cpu_u16(vq->vq.vdev,  vq->vring.avail->idx);
> +		vq->vring.avail->idx = cpu_to_virtio16(vq->vq.vdev,
> +							 avail - 1);
> +
>  		END_USE(vq);
>  		return buf;
>  	}
> @@ -800,12 +875,18 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>  
>  	/* No callback?  Tell other side not to bother us. */
> -	if (!callback)
> -		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> +	if (!callback) {
> +		u16 flag;
> +
> +		flag = cpu_to_virtio16(vq->vq.vdev,
> +					 VRING_AVAIL_F_NO_INTERRUPT);
> +		vq->vring.avail->flags |= flag;
> +	}
>  
>  	/* Put everything in free lists. */
>  	vq->free_head = 0;
>  	for (i = 0; i < num-1; i++) {
> +		/* This is for our use, so always our endian. */
>  		vq->vring.desc[i].next = i+1;
>  		vq->data[i] = NULL;
>  	}
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Virtualization mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cornelia Huck Oct. 22, 2014, 2:28 p.m. UTC | #2

On Wed, 22 Oct 2014 17:02:26 +0300
"Michael S. Tsirkin" <[e-mail]> wrote:

> On Tue, Oct 07, 2014 at 04:39:45PM +0200, Cornelia Huck wrote:
> > From: Rusty Russell <[e-mail]>
> > 
> > [Cornelia Huck: we don't need the vq->vring.num -> vq->ring_mask change]
> > Signed-off-by: Rusty Russell <[e-mail]>
> > Signed-off-by: Cornelia Huck <[e-mail]>
> > ---
> >  drivers/virtio/virtio_ring.c |  195 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 138 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 1cfb5ba..350c39b 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -145,42 +145,54 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq,
> >  	i = 0;
> >  	for (n = 0; n < out_sgs; n++) {
> >  		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
> > -			desc[i].flags = VRING_DESC_F_NEXT;
> > -			desc[i].addr = sg_phys(sg);
> > -			desc[i].len = sg->length;
> > -			desc[i].next = i+1;
> > +			desc[i].flags = cpu_to_virtio16(vq->vq.vdev,
> > +							  VRING_DESC_F_NEXT);
> > +			desc[i].addr = cpu_to_virtio64(vq->vq.vdev,
> > +							 sg_phys(sg));
> > +			desc[i].len = cpu_to_virtio32(vq->vq.vdev,
> > +							sg->length);
> > +			desc[i].next = cpu_to_virtio16(vq->vq.vdev,
> > +							 i+1);
> >  			i++;
> >  		}
> >  	}
> >  	for (; n < (out_sgs + in_sgs); n++) {
> >  		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
> > -			desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
> > -			desc[i].addr = sg_phys(sg);
> > -			desc[i].len = sg->length;
> > -			desc[i].next = i+1;
> > +			desc[i].flags =	cpu_to_virtio16(vq->vq.vdev,
> > +							  VRING_DESC_F_NEXT|
> > +							  VRING_DESC_F_WRITE);
> > +			desc[i].addr = cpu_to_virtio64(vq->vq.vdev,
> > +							 sg_phys(sg));
> > +			desc[i].len = cpu_to_virtio32(vq->vq.vdev,
> > +							sg->length);
> > +			desc[i].next = cpu_to_virtio16(vq->vq.vdev, i+1);
> >  			i++;
> >  		}
> >  	}
> > -	BUG_ON(i != total_sg);
> >  
> >  	/* Last one doesn't continue. */
> > -	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
> > +	desc[i-1].flags &= ~cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
> >  	desc[i-1].next = 0;
> >  
> > -	/* We're about to use a buffer */
> > -	vq->vq.num_free--;
> > -
> >  	/* Use a single buffer which doesn't continue */
> >  	head = vq->free_head;
> > -	vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
> > -	vq->vring.desc[head].addr = virt_to_phys(desc);
> > +	vq->vring.desc[head].flags =
> > +		cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT);
> > +	vq->vring.desc[head].addr =
> > +		cpu_to_virtio64(vq->vq.vdev, virt_to_phys(desc));
> >  	/* kmemleak gives a false positive, as it's hidden by virt_to_phys */
> >  	kmemleak_ignore(desc);
> > -	vq->vring.desc[head].len = i * sizeof(struct vring_desc);
> > +	vq->vring.desc[head].len =
> > +		cpu_to_virtio32(vq->vq.vdev, i * sizeof(struct vring_desc));
> >  
> > -	/* Update free pointer */
> > +	BUG_ON(i != total_sg);
> > +
> 
> Why move the BUG_ON here?
> I think I'll move it back ...

IIRC this was in the original patch I applied - but you're right, the
statement can stay in the old place.

> 
> > +	/* Update free pointer (we store this in native endian) */
> >  	vq->free_head = vq->vring.desc[head].next;
> >  
> > +	/* We've just used a buffer */
> > +	vq->vq.num_free--;
> > +
> >  	return head;
> >  }
> >  

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

Michael S. Tsirkin Oct. 22, 2014, 2:37 p.m. UTC | #3

On Wed, Oct 22, 2014 at 04:28:34PM +0200, Cornelia Huck wrote:
> On Wed, 22 Oct 2014 17:02:26 +0300
> "Michael S. Tsirkin" <[e-mail]> wrote:
> 
> > On Tue, Oct 07, 2014 at 04:39:45PM +0200, Cornelia Huck wrote:
> > > From: Rusty Russell <[e-mail]>
> > > 
> > > [Cornelia Huck: we don't need the vq->vring.num -> vq->ring_mask change]
> > > Signed-off-by: Rusty Russell <[e-mail]>
> > > Signed-off-by: Cornelia Huck <[e-mail]>
> > > ---
> > >  drivers/virtio/virtio_ring.c |  195 ++++++++++++++++++++++++++++++------------
> > >  1 file changed, 138 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 1cfb5ba..350c39b 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -145,42 +145,54 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq,
> > >  	i = 0;
> > >  	for (n = 0; n < out_sgs; n++) {
> > >  		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
> > > -			desc[i].flags = VRING_DESC_F_NEXT;
> > > -			desc[i].addr = sg_phys(sg);
> > > -			desc[i].len = sg->length;
> > > -			desc[i].next = i+1;
> > > +			desc[i].flags = cpu_to_virtio16(vq->vq.vdev,
> > > +							  VRING_DESC_F_NEXT);
> > > +			desc[i].addr = cpu_to_virtio64(vq->vq.vdev,
> > > +							 sg_phys(sg));
> > > +			desc[i].len = cpu_to_virtio32(vq->vq.vdev,
> > > +							sg->length);
> > > +			desc[i].next = cpu_to_virtio16(vq->vq.vdev,
> > > +							 i+1);
> > >  			i++;
> > >  		}
> > >  	}
> > >  	for (; n < (out_sgs + in_sgs); n++) {
> > >  		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
> > > -			desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
> > > -			desc[i].addr = sg_phys(sg);
> > > -			desc[i].len = sg->length;
> > > -			desc[i].next = i+1;
> > > +			desc[i].flags =	cpu_to_virtio16(vq->vq.vdev,
> > > +							  VRING_DESC_F_NEXT|
> > > +							  VRING_DESC_F_WRITE);
> > > +			desc[i].addr = cpu_to_virtio64(vq->vq.vdev,
> > > +							 sg_phys(sg));
> > > +			desc[i].len = cpu_to_virtio32(vq->vq.vdev,
> > > +							sg->length);
> > > +			desc[i].next = cpu_to_virtio16(vq->vq.vdev, i+1);
> > >  			i++;
> > >  		}
> > >  	}
> > > -	BUG_ON(i != total_sg);
> > >  
> > >  	/* Last one doesn't continue. */
> > > -	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
> > > +	desc[i-1].flags &= ~cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
> > >  	desc[i-1].next = 0;
> > >  
> > > -	/* We're about to use a buffer */
> > > -	vq->vq.num_free--;
> > > -
> > >  	/* Use a single buffer which doesn't continue */
> > >  	head = vq->free_head;
> > > -	vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
> > > -	vq->vring.desc[head].addr = virt_to_phys(desc);
> > > +	vq->vring.desc[head].flags =
> > > +		cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT);
> > > +	vq->vring.desc[head].addr =
> > > +		cpu_to_virtio64(vq->vq.vdev, virt_to_phys(desc));
> > >  	/* kmemleak gives a false positive, as it's hidden by virt_to_phys */
> > >  	kmemleak_ignore(desc);
> > > -	vq->vring.desc[head].len = i * sizeof(struct vring_desc);
> > > +	vq->vring.desc[head].len =
> > > +		cpu_to_virtio32(vq->vq.vdev, i * sizeof(struct vring_desc));
> > >  
> > > -	/* Update free pointer */
> > > +	BUG_ON(i != total_sg);
> > > +
> > 
> > Why move the BUG_ON here?
> > I think I'll move it back ...
> 
> IIRC this was in the original patch I applied - but you're right, the
> statement can stay in the old place.

I think I'll do it more gradually: mechanically add accessors
everywhere as a 1st step. Split long lines and other cleanup
as a second step.

> > 
> > > +	/* Update free pointer (we store this in native endian) */
> > >  	vq->free_head = vq->vring.desc[head].next;
> > >  
> > > +	/* We've just used a buffer */
> > > +	vq->vq.num_free--;
> > > +
> > >  	return head;
> > >  }
> > >  
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 1cfb5ba..350c39b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -145,42 +145,54 @@  static inline int vring_add_indirect(struct vring_virtqueue *vq,
 	i = 0;
 	for (n = 0; n < out_sgs; n++) {
 		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
-			desc[i].flags = VRING_DESC_F_NEXT;
-			desc[i].addr = sg_phys(sg);
-			desc[i].len = sg->length;
-			desc[i].next = i+1;
+			desc[i].flags = cpu_to_virtio_u16(vq->vq.vdev,
+							  VRING_DESC_F_NEXT);
+			desc[i].addr = cpu_to_virtio_u64(vq->vq.vdev,
+							 sg_phys(sg));
+			desc[i].len = cpu_to_virtio_u32(vq->vq.vdev,
+							sg->length);
+			desc[i].next = cpu_to_virtio_u16(vq->vq.vdev,
+							 i+1);
 			i++;
 		}
 	}
 	for (; n < (out_sgs + in_sgs); n++) {
 		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
-			desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
-			desc[i].addr = sg_phys(sg);
-			desc[i].len = sg->length;
-			desc[i].next = i+1;
+			desc[i].flags =	cpu_to_virtio_u16(vq->vq.vdev,
+							  VRING_DESC_F_NEXT|
+							  VRING_DESC_F_WRITE);
+			desc[i].addr = cpu_to_virtio_u64(vq->vq.vdev,
+							 sg_phys(sg));
+			desc[i].len = cpu_to_virtio_u32(vq->vq.vdev,
+							sg->length);
+			desc[i].next = cpu_to_virtio_u16(vq->vq.vdev, i+1);
 			i++;
 		}
 	}
-	BUG_ON(i != total_sg);
 
 	/* Last one doesn't continue. */
-	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
+	desc[i-1].flags &= ~cpu_to_virtio_u16(vq->vq.vdev, VRING_DESC_F_NEXT);
 	desc[i-1].next = 0;
 
-	/* We're about to use a buffer */
-	vq->vq.num_free--;
-
 	/* Use a single buffer which doesn't continue */
 	head = vq->free_head;
-	vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
-	vq->vring.desc[head].addr = virt_to_phys(desc);
+	vq->vring.desc[head].flags =
+		cpu_to_virtio_u16(vq->vq.vdev, VRING_DESC_F_INDIRECT);
+	vq->vring.desc[head].addr =
+		cpu_to_virtio_u64(vq->vq.vdev, virt_to_phys(desc));
 	/* kmemleak gives a false positive, as it's hidden by virt_to_phys */
 	kmemleak_ignore(desc);
-	vq->vring.desc[head].len = i * sizeof(struct vring_desc);
+	vq->vring.desc[head].len =
+		cpu_to_virtio_u32(vq->vq.vdev, i * sizeof(struct vring_desc));
 
-	/* Update free pointer */
+	BUG_ON(i != total_sg);
+
+	/* Update free pointer (we store this in native endian) */
 	vq->free_head = vq->vring.desc[head].next;
 
+	/* We've just used a buffer */
+	vq->vq.num_free--;
+
 	return head;
 }
 
@@ -199,6 +211,7 @@  static inline int virtqueue_add(struct virtqueue *_vq,
 	struct scatterlist *sg;
 	unsigned int i, n, avail, uninitialized_var(prev), total_sg;
 	int head;
+	u16 nexti;
 
 	START_USE(vq);
 
@@ -253,26 +266,46 @@  static inline int virtqueue_add(struct virtqueue *_vq,
 	vq->vq.num_free -= total_sg;
 
 	head = i = vq->free_head;
+
 	for (n = 0; n < out_sgs; n++) {
 		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
-			vq->vring.desc[i].flags = VRING_DESC_F_NEXT;
-			vq->vring.desc[i].addr = sg_phys(sg);
-			vq->vring.desc[i].len = sg->length;
+			vq->vring.desc[i].flags =
+				cpu_to_virtio_u16(vq->vq.vdev,
+						  VRING_DESC_F_NEXT);
+			vq->vring.desc[i].addr =
+				cpu_to_virtio_u64(vq->vq.vdev, sg_phys(sg));
+			vq->vring.desc[i].len =
+				cpu_to_virtio_u32(vq->vq.vdev, sg->length);
+
+			/* We chained .next in native: fix endian. */
+			nexti = vq->vring.desc[i].next;
+			vq->vring.desc[i].next
+				= virtio_to_cpu_u16(vq->vq.vdev, nexti);
 			prev = i;
-			i = vq->vring.desc[i].next;
+			i = nexti;
 		}
 	}
 	for (; n < (out_sgs + in_sgs); n++) {
 		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
-			vq->vring.desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
-			vq->vring.desc[i].addr = sg_phys(sg);
-			vq->vring.desc[i].len = sg->length;
+			vq->vring.desc[i].flags =
+				cpu_to_virtio_u16(vq->vq.vdev,
+						  VRING_DESC_F_NEXT|
+						  VRING_DESC_F_WRITE);
+			vq->vring.desc[i].addr =
+				cpu_to_virtio_u64(vq->vq.vdev, sg_phys(sg));
+			vq->vring.desc[i].len =
+				cpu_to_virtio_u32(vq->vq.vdev, sg->length);
+			/* We chained .next in native: fix endian. */
+			nexti = vq->vring.desc[i].next;
+			vq->vring.desc[i].next =
+				virtio_to_cpu_u16(vq->vq.vdev, nexti);
 			prev = i;
-			i = vq->vring.desc[i].next;
+			i = nexti;
 		}
 	}
 	/* Last one doesn't continue. */
-	vq->vring.desc[prev].flags &= ~VRING_DESC_F_NEXT;
+	vq->vring.desc[prev].flags &=
+		~cpu_to_virtio_u16(vq->vq.vdev, VRING_DESC_F_NEXT);
 
 	/* Update free pointer */
 	vq->free_head = i;
@@ -283,15 +316,16 @@  add_head:
 
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync). */
-	avail = (vq->vring.avail->idx & (vq->vring.num-1));
-	vq->vring.avail->ring[avail] = head;
+	avail = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.avail->idx);
+	vq->vring.avail->ring[avail & (vq->vring.num - 1)] =
+		cpu_to_virtio_u16(vq->vq.vdev, head);
 
-	/* Descriptors and available array need to be set before we expose the
-	 * new available array entries. */
+	/* Descriptors and available array need to be set
+	 * before we expose the new available array entries. */
 	virtio_wmb(vq->weak_barriers);
-	vq->vring.avail->idx++;
-	vq->num_added++;
+	vq->vring.avail->idx = cpu_to_virtio_u16(vq->vq.vdev, avail + 1);
 
+	vq->num_added++;
 	/* This is very unlikely, but theoretically possible.  Kick
 	 * just in case. */
 	if (unlikely(vq->num_added == (1 << 16) - 1))
@@ -408,8 +442,9 @@  bool virtqueue_kick_prepare(struct virtqueue *_vq)
 	 * event. */
 	virtio_mb(vq->weak_barriers);
 
-	old = vq->vring.avail->idx - vq->num_added;
-	new = vq->vring.avail->idx;
+	new = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.avail->idx);
+
+	old = new - vq->num_added;
 	vq->num_added = 0;
 
 #ifdef DEBUG
@@ -421,10 +456,17 @@  bool virtqueue_kick_prepare(struct virtqueue *_vq)
 #endif
 
 	if (vq->event) {
-		needs_kick = vring_need_event(vring_avail_event(&vq->vring),
-					      new, old);
+		u16 avail;
+
+		avail =	virtio_to_cpu_u16(vq->vq.vdev,
+					  vring_avail_event(&vq->vring));
+
+		needs_kick = vring_need_event(avail, new, old);
 	} else {
-		needs_kick = !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
+		u16 flags;
+
+		flags = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->flags);
+		needs_kick = !(flags & VRING_USED_F_NO_NOTIFY);
 	}
 	END_USE(vq);
 	return needs_kick;
@@ -486,11 +528,20 @@  static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
 	i = head;
 
 	/* Free the indirect table */
-	if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)
-		kfree(phys_to_virt(vq->vring.desc[i].addr));
+	if (vq->vring.desc[i].flags &
+	    cpu_to_virtio_u16(vq->vq.vdev, VRING_DESC_F_INDIRECT)) {
+		kfree(phys_to_virt(virtio_to_cpu_u64(vq->vq.vdev,
+						     vq->vring.desc[i].addr)));
+	}
+
+	while (vq->vring.desc[i].flags &
+	       cpu_to_virtio_u16(vq->vq.vdev, VRING_DESC_F_NEXT)) {
+		u16 next;
 
-	while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
-		i = vq->vring.desc[i].next;
+		/* Convert endian of next back to native. */
+		next = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.desc[i].next);
+		vq->vring.desc[i].next = next;
+		i = next;
 		vq->vq.num_free++;
 	}
 
@@ -502,7 +553,8 @@  static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
 
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
-	return vq->last_used_idx != vq->vring.used->idx;
+	return vq->last_used_idx
+		!= virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->idx);
 }
 
 /**
@@ -527,6 +579,8 @@  void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	void *ret;
 	unsigned int i;
 	u16 last_used;
+	const int no_intr =
+		cpu_to_virtio_u16(vq->vq.vdev, VRING_AVAIL_F_NO_INTERRUPT);
 
 	START_USE(vq);
 
@@ -545,8 +599,9 @@  void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	virtio_rmb(vq->weak_barriers);
 
 	last_used = (vq->last_used_idx & (vq->vring.num - 1));
-	i = vq->vring.used->ring[last_used].id;
-	*len = vq->vring.used->ring[last_used].len;
+	i = virtio_to_cpu_u32(vq->vq.vdev, vq->vring.used->ring[last_used].id);
+	*len = virtio_to_cpu_u32(vq->vq.vdev,
+				 vq->vring.used->ring[last_used].len);
 
 	if (unlikely(i >= vq->vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", i);
@@ -561,10 +616,11 @@  void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	ret = vq->data[i];
 	detach_buf(vq, i);
 	vq->last_used_idx++;
+
 	/* If we expect an interrupt for the next entry, tell host
 	 * by writing event index and flush out the write before
 	 * the read in the next get_buf call. */
-	if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
+	if (!(vq->vring.avail->flags & no_intr)) {
 		vring_used_event(&vq->vring) = vq->last_used_idx;
 		virtio_mb(vq->weak_barriers);
 	}
@@ -591,7 +647,8 @@  void virtqueue_disable_cb(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+	vq->vring.avail->flags |=
+		cpu_to_virtio_u16(vq->vq.vdev, VRING_AVAIL_F_NO_INTERRUPT);
 }
 EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
 
@@ -619,8 +676,12 @@  unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always do both to keep code simple. */
-	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
-	vring_used_event(&vq->vring) = last_used_idx = vq->last_used_idx;
+	vq->vring.avail->flags &=
+		cpu_to_virtio_u16(vq->vq.vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
+	last_used_idx = vq->last_used_idx;
+	vring_used_event(&vq->vring) = cpu_to_virtio_u16(vq->vq.vdev,
+							 last_used_idx);
+
 	END_USE(vq);
 	return last_used_idx;
 }
@@ -640,7 +701,9 @@  bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
 	virtio_mb(vq->weak_barriers);
-	return (u16)last_used_idx != vq->vring.used->idx;
+
+	return (u16)last_used_idx !=
+		virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->idx);
 }
 EXPORT_SYMBOL_GPL(virtqueue_poll);
 
@@ -678,7 +741,7 @@  EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
 bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
-	u16 bufs;
+	u16 bufs, used_idx;
 
 	START_USE(vq);
 
@@ -687,12 +750,17 @@  bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always do both to keep code simple. */
-	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	vq->vring.avail->flags &=
+		cpu_to_virtio_u16(vq->vq.vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
 	/* TODO: tune this threshold */
-	bufs = (u16)(vq->vring.avail->idx - vq->last_used_idx) * 3 / 4;
-	vring_used_event(&vq->vring) = vq->last_used_idx + bufs;
+	bufs = (u16)(virtio_to_cpu_u16(vq->vq.vdev, vq->vring.avail->idx)
+		     - vq->last_used_idx) * 3 / 4;
+	vring_used_event(&vq->vring) =
+		cpu_to_virtio_u16(vq->vq.vdev, vq->last_used_idx + bufs);
 	virtio_mb(vq->weak_barriers);
-	if (unlikely((u16)(vq->vring.used->idx - vq->last_used_idx) > bufs)) {
+	used_idx = virtio_to_cpu_u16(vq->vq.vdev, vq->vring.used->idx);
+
+	if (unlikely((u16)(used_idx - vq->last_used_idx) > bufs)) {
 		END_USE(vq);
 		return false;
 	}
@@ -719,12 +787,19 @@  void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
 	START_USE(vq);
 
 	for (i = 0; i < vq->vring.num; i++) {
+		u16 avail;
+
 		if (!vq->data[i])
 			continue;
 		/* detach_buf clears data, so grab it now. */
 		buf = vq->data[i];
 		detach_buf(vq, i);
-		vq->vring.avail->idx--;
+
+		/* AKA "vq->vring.avail->idx++" */
+		avail = virtio_to_cpu_u16(vq->vq.vdev,  vq->vring.avail->idx);
+		vq->vring.avail->idx = cpu_to_virtio_u16(vq->vq.vdev,
+							 avail - 1);
+
 		END_USE(vq);
 		return buf;
 	}
@@ -800,12 +875,18 @@  struct virtqueue *vring_new_virtqueue(unsigned int index,
 	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
 
 	/* No callback?  Tell other side not to bother us. */
-	if (!callback)
-		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+	if (!callback) {
+		u16 flag;
+
+		flag = cpu_to_virtio_u16(vq->vq.vdev,
+					 VRING_AVAIL_F_NO_INTERRUPT);
+		vq->vring.avail->flags |= flag;
+	}
 
 	/* Put everything in free lists. */
 	vq->free_head = 0;
 	for (i = 0; i < num-1; i++) {
+		/* This is for our use, so always our endian. */
 		vq->vring.desc[i].next = i+1;
 		vq->data[i] = NULL;
 	}