Sjur Brændeland <sjurbr@xxxxxxxxx> 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.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
