Re: [RFC virtio-next 0/4] Introduce CAIF Virtio and reversed Vrings

[Rusty Russell <rusty@rustcorp.com.au>]

Sjur Brændeland <sjurbr@gmail.com> writes:
> Hi Rusty,
>
>> So, this adds another host-side virtqueue implementation.
>>
>> Can we combine them together conveniently?  You pulled out more stuff
>> into vring.h which is a start, but it's a bit overloaded.
>> Perhaps we should separate the common fields into struct vring, and use
>> it to build:
>>
>>         struct vring_guest {
>>                 struct vring vr;
>>                 u16 last_used_idx;
>>         };
>>
>>         struct vring_host {
>>                 struct vring vr;
>>                 u16 last_avail_idx;
>>         };
>> I haven't looked closely at vhost to see what it wants, but I would
>> think we could share more code.
>
> I have played around with the code in vhost.c to explore your idea.
> The main issue I run into is that vhost.c is accessing user data while my new
> code does not. So I end up with some quirky code testing if the ring lives in
> user memory or not.  Another issue is sparse warnings when
> accessing user memory.

Sparse is a servant, not a master.  If that's the only thing stopping
us, we can ignore it (or hack around it).

> With your suggested changes I end up sharing about 100 lines of code.
> So in sum, I feel this add more complexity than what we gain by sharing.
>
> Below is an initial draft of the re-usable code. I added "is_uaccess" to struct
> virtio_ring in order to know if the ring lives in user memory.
>
> Let me know what you think.

Agreed, that's horrible...

Fortunately, recent GCCs will inline function pointers, so inlining this
and handing an accessor function gets optimized away.

I would really like this, because I'd love to have a config option to do
strict checking on the format of these things (similar to my recently
posted CONFIG_VIRTIO_DEVICE_TORTURE patch).

See below.

> int virtqueue_add_used(struct vring_host *vr, unsigned int head, int len,
> 		    struct vring_used_elem  **used)
> {
> 	/* The virtqueue contains a ring of used buffers.  Get a pointer to the
> 	 * next entry in that used ring. */
> 	*used = &vr->vring.used->ring[vr->last_used_idx % vr->vring.num];
> 	if (vr->is_uaccess) {
> 		if(unlikely(__put_user(head, &(*used)->id))) {
> 			pr_debug("Failed to write used id");
> 			return -EFAULT;
> 		}
> 		if (unlikely(__put_user(len, &(*used)->len))) {
> 			pr_debug("Failed to write used len");
> 			return -EFAULT;
> 		}
> 		smp_wmb();
> 		if (__put_user(vr->last_used_idx + 1,
> 			       &vr->vring.used->idx)) {
> 			pr_debug("Failed to increment used idx");
> 			return -EFAULT;
> 		}
> 	} else {
> 		(*used)->id = head;
> 		(*used)->len = len;
> 		smp_wmb();
> 		vr->vring.used->idx = vr->last_used_idx + 1;
> 	}
> 	vr->last_used_idx++;
> 	return 0;
> }

/* Untested! */
static inline bool in_kernel_put(u32 *dst, u32 v)
{
        *dst = v;
        return true;
}

static inline bool userspace_put(u32 *dst, u32 v)
{
	return __put_user(dst, v) == 0;
}

static inline struct vring_used_elem *vrh_add_used(struct vring_host *vr,
                                                   unsigned int head, u32 len,
                                                   bool (*put)(u32 *dst, u32 v))
{
        struct vring_used_elem *used;

	/* The virtqueue contains a ring of used buffers.  Get a pointer to the
	 * next entry in that used ring. */
	used = &vr->vring.used->ring[vr->last_used_idx % vr->vring.num];
        
	if (!put(&used->id, head) || !put(&used->len = len))
                return NULL;
	smp_wmb();
        if (!put(&vr->vring.used->idx, vr->last_used_idx + 1))
                return NULL;
	vr->last_used_idx++;
	return used;
}

Cheers,
Rusty.