From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rusty Russell Subject: Re: [RFC virtio-next 0/4] Introduce CAIF Virtio and reversed Vrings Date: Tue, 06 Nov 2012 12:39:14 +1030 Message-ID: <871ug75vp1.fsf@rustcorp.com.au> References: <1351723614-4145-1-git-send-email-sjur@brendeland.net> <87wqy54vo0.fsf@rustcorp.com.au> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Sjur =?utf-8?Q?Br=C3=A6ndeland?= Cc: "Michael S. Tsirkin" , netdev@vger.kernel.org, Linus Walleij , dmitry.tarnyagin@stericsson.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org List-Id: virtualization@lists.linuxfoundation.org U2p1ciBCcsOmbmRlbGFuZCA8c2p1cmJyQGdtYWlsLmNvbT4gd3JpdGVzOgo+IEhpIFJ1c3R5LAo+ Cj4+IFNvLCB0aGlzIGFkZHMgYW5vdGhlciBob3N0LXNpZGUgdmlydHF1ZXVlIGltcGxlbWVudGF0 aW9uLgo+Pgo+PiBDYW4gd2UgY29tYmluZSB0aGVtIHRvZ2V0aGVyIGNvbnZlbmllbnRseT8gIFlv dSBwdWxsZWQgb3V0IG1vcmUgc3R1ZmYKPj4gaW50byB2cmluZy5oIHdoaWNoIGlzIGEgc3RhcnQs IGJ1dCBpdCdzIGEgYml0IG92ZXJsb2FkZWQuCj4+IFBlcmhhcHMgd2Ugc2hvdWxkIHNlcGFyYXRl IHRoZSBjb21tb24gZmllbGRzIGludG8gc3RydWN0IHZyaW5nLCBhbmQgdXNlCj4+IGl0IHRvIGJ1 aWxkOgo+Pgo+PiAgICAgICAgIHN0cnVjdCB2cmluZ19ndWVzdCB7Cj4+ICAgICAgICAgICAgICAg ICBzdHJ1Y3QgdnJpbmcgdnI7Cj4+ICAgICAgICAgICAgICAgICB1MTYgbGFzdF91c2VkX2lkeDsK Pj4gICAgICAgICB9Owo+Pgo+PiAgICAgICAgIHN0cnVjdCB2cmluZ19ob3N0IHsKPj4gICAgICAg ICAgICAgICAgIHN0cnVjdCB2cmluZyB2cjsKPj4gICAgICAgICAgICAgICAgIHUxNiBsYXN0X2F2 YWlsX2lkeDsKPj4gICAgICAgICB9Owo+PiBJIGhhdmVuJ3QgbG9va2VkIGNsb3NlbHkgYXQgdmhv c3QgdG8gc2VlIHdoYXQgaXQgd2FudHMsIGJ1dCBJIHdvdWxkCj4+IHRoaW5rIHdlIGNvdWxkIHNo YXJlIG1vcmUgY29kZS4KPgo+IEkgaGF2ZSBwbGF5ZWQgYXJvdW5kIHdpdGggdGhlIGNvZGUgaW4g dmhvc3QuYyB0byBleHBsb3JlIHlvdXIgaWRlYS4KPiBUaGUgbWFpbiBpc3N1ZSBJIHJ1biBpbnRv IGlzIHRoYXQgdmhvc3QuYyBpcyBhY2Nlc3NpbmcgdXNlciBkYXRhIHdoaWxlIG15IG5ldwo+IGNv ZGUgZG9lcyBub3QuIFNvIEkgZW5kIHVwIHdpdGggc29tZSBxdWlya3kgY29kZSB0ZXN0aW5nIGlm IHRoZSByaW5nIGxpdmVzIGluCj4gdXNlciBtZW1vcnkgb3Igbm90LiAgQW5vdGhlciBpc3N1ZSBp cyBzcGFyc2Ugd2FybmluZ3Mgd2hlbgo+IGFjY2Vzc2luZyB1c2VyIG1lbW9yeS4KClNwYXJzZSBp cyBhIHNlcnZhbnQsIG5vdCBhIG1hc3Rlci4gIElmIHRoYXQncyB0aGUgb25seSB0aGluZyBzdG9w cGluZwp1cywgd2UgY2FuIGlnbm9yZSBpdCAob3IgaGFjayBhcm91bmQgaXQpLgoKPiBXaXRoIHlv dXIgc3VnZ2VzdGVkIGNoYW5nZXMgSSBlbmQgdXAgc2hhcmluZyBhYm91dCAxMDAgbGluZXMgb2Yg Y29kZS4KPiBTbyBpbiBzdW0sIEkgZmVlbCB0aGlzIGFkZCBtb3JlIGNvbXBsZXhpdHkgdGhhbiB3 aGF0IHdlIGdhaW4gYnkgc2hhcmluZy4KPgo+IEJlbG93IGlzIGFuIGluaXRpYWwgZHJhZnQgb2Yg dGhlIHJlLXVzYWJsZSBjb2RlLiBJIGFkZGVkICJpc191YWNjZXNzIiB0byBzdHJ1Y3QKPiB2aXJ0 aW9fcmluZyBpbiBvcmRlciB0byBrbm93IGlmIHRoZSByaW5nIGxpdmVzIGluIHVzZXIgbWVtb3J5 Lgo+Cj4gTGV0IG1lIGtub3cgd2hhdCB5b3UgdGhpbmsuCgpBZ3JlZWQsIHRoYXQncyBob3JyaWJs ZS4uLgoKRm9ydHVuYXRlbHksIHJlY2VudCBHQ0NzIHdpbGwgaW5saW5lIGZ1bmN0aW9uIHBvaW50 ZXJzLCBzbyBpbmxpbmluZyB0aGlzCmFuZCBoYW5kaW5nIGFuIGFjY2Vzc29yIGZ1bmN0aW9uIGdl dHMgb3B0aW1pemVkIGF3YXkuCgpJIHdvdWxkIHJlYWxseSBsaWtlIHRoaXMsIGJlY2F1c2UgSSdk IGxvdmUgdG8gaGF2ZSBhIGNvbmZpZyBvcHRpb24gdG8gZG8Kc3RyaWN0IGNoZWNraW5nIG9uIHRo ZSBmb3JtYXQgb2YgdGhlc2UgdGhpbmdzIChzaW1pbGFyIHRvIG15IHJlY2VudGx5CnBvc3RlZCBD T05GSUdfVklSVElPX0RFVklDRV9UT1JUVVJFIHBhdGNoKS4KClNlZSBiZWxvdy4KCj4gaW50IHZp cnRxdWV1ZV9hZGRfdXNlZChzdHJ1Y3QgdnJpbmdfaG9zdCAqdnIsIHVuc2lnbmVkIGludCBoZWFk LCBpbnQgbGVuLAo+IAkJICAgIHN0cnVjdCB2cmluZ191c2VkX2VsZW0gICoqdXNlZCkKPiB7Cj4g CS8qIFRoZSB2aXJ0cXVldWUgY29udGFpbnMgYSByaW5nIG9mIHVzZWQgYnVmZmVycy4gIEdldCBh IHBvaW50ZXIgdG8gdGhlCj4gCSAqIG5leHQgZW50cnkgaW4gdGhhdCB1c2VkIHJpbmcuICovCj4g CSp1c2VkID0gJnZyLT52cmluZy51c2VkLT5yaW5nW3ZyLT5sYXN0X3VzZWRfaWR4ICUgdnItPnZy aW5nLm51bV07Cj4gCWlmICh2ci0+aXNfdWFjY2Vzcykgewo+IAkJaWYodW5saWtlbHkoX19wdXRf dXNlcihoZWFkLCAmKCp1c2VkKS0+aWQpKSkgewo+IAkJCXByX2RlYnVnKCJGYWlsZWQgdG8gd3Jp dGUgdXNlZCBpZCIpOwo+IAkJCXJldHVybiAtRUZBVUxUOwo+IAkJfQo+IAkJaWYgKHVubGlrZWx5 KF9fcHV0X3VzZXIobGVuLCAmKCp1c2VkKS0+bGVuKSkpIHsKPiAJCQlwcl9kZWJ1ZygiRmFpbGVk IHRvIHdyaXRlIHVzZWQgbGVuIik7Cj4gCQkJcmV0dXJuIC1FRkFVTFQ7Cj4gCQl9Cj4gCQlzbXBf d21iKCk7Cj4gCQlpZiAoX19wdXRfdXNlcih2ci0+bGFzdF91c2VkX2lkeCArIDEsCj4gCQkJICAg ICAgICZ2ci0+dnJpbmcudXNlZC0+aWR4KSkgewo+IAkJCXByX2RlYnVnKCJGYWlsZWQgdG8gaW5j cmVtZW50IHVzZWQgaWR4Iik7Cj4gCQkJcmV0dXJuIC1FRkFVTFQ7Cj4gCQl9Cj4gCX0gZWxzZSB7 Cj4gCQkoKnVzZWQpLT5pZCA9IGhlYWQ7Cj4gCQkoKnVzZWQpLT5sZW4gPSBsZW47Cj4gCQlzbXBf d21iKCk7Cj4gCQl2ci0+dnJpbmcudXNlZC0+aWR4ID0gdnItPmxhc3RfdXNlZF9pZHggKyAxOwo+ IAl9Cj4gCXZyLT5sYXN0X3VzZWRfaWR4Kys7Cj4gCXJldHVybiAwOwo+IH0KCi8qIFVudGVzdGVk ISAqLwpzdGF0aWMgaW5saW5lIGJvb2wgaW5fa2VybmVsX3B1dCh1MzIgKmRzdCwgdTMyIHYpCnsK ICAgICAgICAqZHN0ID0gdjsKICAgICAgICByZXR1cm4gdHJ1ZTsKfQoKc3RhdGljIGlubGluZSBi b29sIHVzZXJzcGFjZV9wdXQodTMyICpkc3QsIHUzMiB2KQp7CglyZXR1cm4gX19wdXRfdXNlcihk c3QsIHYpID09IDA7Cn0KCnN0YXRpYyBpbmxpbmUgc3RydWN0IHZyaW5nX3VzZWRfZWxlbSAqdnJo X2FkZF91c2VkKHN0cnVjdCB2cmluZ19ob3N0ICp2ciwKICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgdW5zaWduZWQgaW50IGhlYWQsIHUzMiBsZW4sCiAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGJvb2wgKCpw dXQpKHUzMiAqZHN0LCB1MzIgdikpCnsKICAgICAgICBzdHJ1Y3QgdnJpbmdfdXNlZF9lbGVtICp1 c2VkOwoKCS8qIFRoZSB2aXJ0cXVldWUgY29udGFpbnMgYSByaW5nIG9mIHVzZWQgYnVmZmVycy4g IEdldCBhIHBvaW50ZXIgdG8gdGhlCgkgKiBuZXh0IGVudHJ5IGluIHRoYXQgdXNlZCByaW5nLiAq LwoJdXNlZCA9ICZ2ci0+dnJpbmcudXNlZC0+cmluZ1t2ci0+bGFzdF91c2VkX2lkeCAlIHZyLT52 cmluZy5udW1dOwogICAgICAgIAoJaWYgKCFwdXQoJnVzZWQtPmlkLCBoZWFkKSB8fCAhcHV0KCZ1 c2VkLT5sZW4gPSBsZW4pKQogICAgICAgICAgICAgICAgcmV0dXJuIE5VTEw7CglzbXBfd21iKCk7 CiAgICAgICAgaWYgKCFwdXQoJnZyLT52cmluZy51c2VkLT5pZHgsIHZyLT5sYXN0X3VzZWRfaWR4 ICsgMSkpCiAgICAgICAgICAgICAgICByZXR1cm4gTlVMTDsKCXZyLT5sYXN0X3VzZWRfaWR4Kys7 CglyZXR1cm4gdXNlZDsKfQoKQ2hlZXJzLApSdXN0eS4KX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX18KVmlydHVhbGl6YXRpb24gbWFpbGluZyBsaXN0ClZpcnR1 YWxpemF0aW9uQGxpc3RzLmxpbnV4LWZvdW5kYXRpb24ub3JnCmh0dHBzOi8vbGlzdHMubGludXhm b3VuZGF0aW9uLm9yZy9tYWlsbWFuL2xpc3RpbmZvL3ZpcnR1YWxpemF0aW9u From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933688Ab2KFCLh (ORCPT ); Mon, 5 Nov 2012 21:11:37 -0500 Received: from ozlabs.org ([203.10.76.45]:42108 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933421Ab2KFCKk convert rfc822-to-8bit (ORCPT ); Mon, 5 Nov 2012 21:10:40 -0500 From: Rusty Russell To: Sjur =?utf-8?Q?Br=C3=A6ndeland?= Cc: "Michael S. Tsirkin" , Linus Walleij , Ohad Ben-Cohen , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, dmitry.tarnyagin@stericsson.com Subject: Re: [RFC virtio-next 0/4] Introduce CAIF Virtio and reversed Vrings In-Reply-To: References: <1351723614-4145-1-git-send-email-sjur@brendeland.net> <87wqy54vo0.fsf@rustcorp.com.au> User-Agent: Notmuch/0.14 (http://notmuchmail.org) Emacs/23.4.1 (i686-pc-linux-gnu) Date: Tue, 06 Nov 2012 12:39:14 +1030 Message-ID: <871ug75vp1.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Sjur Brændeland 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.