Re: [RFCv2 04/12] virtio-ring: Refactor out the functions accessing user memory

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Dec 05, 2012 at 03:37:02PM +0100, Sjur Brændeland wrote:
> Isolate the access to user-memory in separate inline
> functions. This open up for reuse from host-side virtioqueue
> implementation accessing virtio-ring in kernel space.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/virtio/virtio_ring_host.c |   81 ++++++++++++++++++++++++++-----------
>  1 files changed, 57 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring_host.c b/drivers/virtio/virtio_ring_host.c
> index 192b838..0750099 100644
> --- a/drivers/virtio/virtio_ring_host.c
> +++ b/drivers/virtio/virtio_ring_host.c
> @@ -18,44 +18,45 @@
>  #include <linux/virtio_ring.h>
>  #include <linux/module.h>
>  #include <linux/uaccess.h>
> +#include <linux/kconfig.h>
>  
>  MODULE_LICENSE("GPL");
>  
> -struct vring_used_elem *vring_add_used_user(struct vring_host *vh,
> -				     unsigned int head, int len)
> +
> +static inline struct vring_used_elem *_vring_add_used(struct vring_host *vh,
> +						      u32 head, u32 len,
> +						      bool (*cpy)(void *dst,
> +								  void *src,
> +								  size_t s),
> +						      void (*wbarrier)(void))
>  {
>  	struct vring_used_elem  *used;
> +	u16 last_used;
>  
>  	/* The virtqueue contains a ring of used buffers.  Get a pointer to the
>  	 * next entry in that used ring. */
> -	used = &vh->vr.used->ring[vh->last_used_idx % vh->vr.num];
> -	if (__put_user(head, &used->id)) {
> -		pr_debug("Failed to write used id");
> +	used = &vh->vr.used->ring[vh->last_used_idx & (vh->vr.num - 1)];
> +	if (!cpy(&used->id, &head, sizeof(used->id)) ||
> +	    !cpy(&used->len, &len, sizeof(used->len)))
>  		return NULL;
> -	}
> -	if (__put_user(len, &used->len)) {
> -		pr_debug("Failed to write used len");
> +	wbarrier();
> +	last_used = vh->last_used_idx + 1;
> +	if (!cpy(&vh->vr.used->idx, &last_used, sizeof(vh->vr.used->idx)))
>  		return NULL;

I think this is broken: we need a 16 bit access, this is
doing a memcpy which is byte by byte.

> -	}
> -	/* Make sure buffer is written before we update index. */
> -	smp_wmb();
> -	if (__put_user(vh->last_used_idx + 1, &vh->vr.used->idx)) {
> -		pr_debug("Failed to increment used idx");
> -		return NULL;
> -	}
> -	vh->last_used_idx++;
> +	vh->last_used_idx = last_used;
>  	return used;
>  }
> -EXPORT_SYMBOL(vring_add_used_user);
>  
> -int vring_avail_desc_user(struct vring_host *vh)
> +static inline int _vring_avail_desc(struct vring_host *vh,
> +				    bool (*get)(u16 *dst, u16 *src),
> +				    void (*read_barrier)(void))
>  {
> -	int head;
> +	u16 head;
>  	u16 last_avail_idx;
>  
>  	/* Check it isn't doing very strange things with descriptor numbers. */
>  	last_avail_idx = vh->last_avail_idx;
> -	if (unlikely(__get_user(vh->avail_idx, &vh->vr.avail->idx))) {
> +	if (unlikely(!get(&vh->avail_idx, &vh->vr.avail->idx))) {
>  		pr_debug("Failed to access avail idx at %p\n",
>  		       &vh->vr.avail->idx);
>  		return -EFAULT;
> @@ -69,13 +70,12 @@ int vring_avail_desc_user(struct vring_host *vh)
>  		return vh->vr.num;
>  
>  	/* Only get avail ring entries after they have been exposed by guest. */
> -	smp_rmb();
> +	read_barrier();
>  
>  	/* Grab the next descriptor number they're advertising, and increment
>  	 * the index we've seen. */
> -	if (unlikely(__get_user(head,
> -				&vh->vr.avail->ring[last_avail_idx %
> -							vh->vr.num]))) {
> +	if (unlikely(!get(&head, &vh->vr.avail->ring[last_avail_idx &
> +						     (vh->vr.num - 1)]))) {
>  		pr_debug("Failed to read head: idx %d address %p\n",
>  			 last_avail_idx,
>  		       &vh->vr.avail->ring[last_avail_idx %
> @@ -92,6 +92,39 @@ int vring_avail_desc_user(struct vring_host *vh)
>  
>  	return head;
>  }
> +
> +static inline void smp_write_barrier(void)
> +{
> +	smp_wmb();
> +}
> +
> +static inline void smp_read_barrier(void)
> +{
> +	smp_rmb();
> +}
> +
> +static inline bool userspace_cpy_to(void  *dst, void *src, size_t s)
> +{
> +	return __copy_to_user(dst, src, s) == 0;
> +}
> +
> +static inline bool userspace_get(u16 *dst, u16 *src)
> +{
> +	return __get_user(*dst, src);
> +}
> +
> +struct vring_used_elem *vring_add_used_user(struct vring_host *vh,
> +					    unsigned int head, int len)
> +{
> +	return _vring_add_used(vh, head, len, userspace_cpy_to,
> +			       smp_write_barrier);
> +}
> +EXPORT_SYMBOL(vring_add_used_user);
> +
> +int vring_avail_desc_user(struct vring_host *vh)
> +{
> +	return _vring_avail_desc(vh, userspace_get, smp_read_barrier);
> +}
>  EXPORT_SYMBOL(vring_avail_desc_user);
>  
>  /* Each buffer in the virtqueues is actually a chain of descriptors.  This
> -- 
> 1.7.5.4