From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rusty Russell Subject: Re: [PATCH] virtio_pci: fix capability format, comments Date: Thu, 30 May 2013 11:03:38 +0930 Message-ID: <8738t5metp.fsf@rustcorp.com.au> References: <20130528080816.GA23399@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20130528080816.GA23399@redhat.com> 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: "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org, kvm@vger.kernel.org Cc: David Howells , Dave Jones , "Paul E. McKenney" , linux-kernel@vger.kernel.org List-Id: virtualization@lists.linuxfoundation.org Ik1pY2hhZWwgUy4gVHNpcmtpbiIgPG1zdEByZWRoYXQuY29tPiB3cml0ZXM6Cgo+IC0gcXVldWUg c2l6ZSBjYW4gYWN0dWFsbHkgYmUgMCB3aGljaCBpcyBub3QgYSBwb3dlciBvZiAyCgpBY3R1YWxs eSwgdGhhdCBwb2ludHMgdG8gYSBmbGF3IGluIHRoZSBjb2RlLiAgV2hlbiB3ZSBzaHV0IGRvd24g dGhlCnF1ZXVlLCB3ZSBzaG91bGQgaWRlYWxseSByZXNldCBpdCB0byB3aGF0IHRoZSBkZXZpY2Ug c3RhcnRlZCB3aXRoLApyYXRoZXIgdGhhbiAwLgoKU2VlIGJlbG93LgoKPiAtIGZpeCBjYXBhYmls aXR5IGZvcm1hdC4gUENJIHNwZWMgc2F5czoKPiAJVGhlIGxheW91dCBvZiB0aGUgaW5mb3JtYXRp b24gaXMgdmVuZG9yIHNwZWNpZmljLCBleGNlcHQgdGhhdCB0aGUgYnl0ZQo+IAlpbW1lZGlhdGVs eSBmb2xsb3dpbmcgdGhlIOKAnE5leHTigJ0gcG9pbnRlciBpbiB0aGUgY2FwYWJpbGl0eSBzdHJ1 Y3R1cmUgaXMKPiAJZGVmaW5lZCB0byBiZSBhIGxlbmd0aCBmaWVsZC4KPiAJVGhpcyBsZW5ndGgg ZmllbGQgcHJvdmlkZXMgdGhlIG51bWJlciBvZiBieXRlcyBpbiB0aGUgY2FwYWJpbGl0eQo+IAlz dHJ1Y3R1cmUgKGluY2x1ZGluZyB0aGUgSUQgYW5kIE5leHQgcG9pbnRlciBieXRlcykuCgpUaGF0 IHBhcnQncyBkZWZpbml0ZWx5IGNvcnJlY3Q6IGFwcGxpZWQuCgpUaGFua3MsClJ1c3R5LgoKU3Vi amV0OiB2aXJ0aW9fcGNpOiBzYXZlIHRoZSBkZXNpcmVkIHJpbmdzaXplLgoKTVNUIHBvaW50cyBv dXQgdGhhdCAwIGlzbid0IGEgcG93ZXIgb2YgMi4gIFRoaXMgbWVhbnMgd2UgY2FuJ3QgcmUtb3Bl bgphIHZpcnRpbyBkZXZpY2Ugb25jZSB3ZSB3cml0ZSAwIGludG8gdGhlIHF1ZXVlIGxlbmd0aC4K CldlIHNob3VsZCByZXN0b3JlIHRoZSBhbW91bnQgdGhlIGRldmljZSBvcmlnaW5hbGx5IGFza2Vk IGZvci4KClNpZ25lZC1vZmYtYnk6IFJ1c3R5IFJ1c3NlbGwgPHJ1c3R5QHJ1c3Rjb3JwLmNvbS5h dT4KCmRpZmYgLS1naXQgYS9kcml2ZXJzL3ZpcnRpby92aXJ0aW9fcGNpLWNvbW1vbi5oIGIvZHJp dmVycy92aXJ0aW8vdmlydGlvX3BjaS1jb21tb24uaAppbmRleCBiYTFiZjgxLi4wZTMxNDNiIDEw MDY0NAotLS0gYS9kcml2ZXJzL3ZpcnRpby92aXJ0aW9fcGNpLWNvbW1vbi5oCisrKyBiL2RyaXZl cnMvdmlydGlvL3ZpcnRpb19wY2ktY29tbW9uLmgKQEAgLTgyLDYgKzgyLDkgQEAgc3RydWN0IHZp cnRpb19wY2lfdnFfaW5mbyB7CiAKIAkvKiBNU0ktWCB2ZWN0b3IgKG9yIG5vbmUpICovCiAJdW5z aWduZWQgbXNpeF92ZWN0b3I7CisKKwkvKiBXaGF0IHNpemUgZGlkIHRoZSBkZXZpY2UgKndhbnQq IHRoaXMgdG8gYmU/ICovCisJdTE2IGRlc2lyZWRfbnVtOwogfTsKIAogLyogdGhlIG5vdGlmeSBm dW5jdGlvbiB1c2VkIHdoZW4gY3JlYXRpbmcgYSB2aXJ0IHF1ZXVlICovCmRpZmYgLS1naXQgYS9k cml2ZXJzL3ZpcnRpby92aXJ0aW9fcGNpLmMgYi9kcml2ZXJzL3ZpcnRpby92aXJ0aW9fcGNpLmMK aW5kZXggMGYwZTNhNi4uOGIzNWMyZSAxMDA2NDQKLS0tIGEvZHJpdmVycy92aXJ0aW8vdmlydGlv X3BjaS5jCisrKyBiL2RyaXZlcnMvdmlydGlvL3ZpcnRpb19wY2kuYwpAQCAtMjMwLDYgKzIzMCw3 IEBAIHN0YXRpYyBzdHJ1Y3QgdmlydHF1ZXVlICpzZXR1cF92cShzdHJ1Y3QgdmlydGlvX3BjaV9k ZXZpY2UgKnZwX2RldiwKIAkJcmV0dXJuIEVSUl9QVFIoLUVOT01FTSk7CiAKIAlpbmZvLT5tc2l4 X3ZlY3RvciA9IG1zaXhfdmVjOworCWluZm8tPmRlc2lyZWRfbnVtID0gbnVtOwogCiAJLyogZ2V0 IG9mZnNldCBvZiBub3RpZmljYXRpb24gd29yZCBmb3IgdGhpcyB2cSAoc2hvdWxkbid0IHdyYXAp ICovCiAJb2ZmID0gaW9yZWFkMTYoJnZwX2Rldi0+Y29tbW9uLT5xdWV1ZV9ub3RpZnlfb2ZmKTsK QEAgLTM1MCw3ICszNTEsNyBAQCBzdGF0aWMgdm9pZCBkZWxfdnEoc3RydWN0IHZpcnRxdWV1ZSAq dnEpCiAJdnJpbmdfZGVsX3ZpcnRxdWV1ZSh2cSk7CiAKIAkvKiBUaGlzIGlzIGZvciBvdXIgb3du IGJlbmVmaXQsIG5vdCB0aGUgZGV2aWNlJ3MhICovCi0JaW93cml0ZTE2KDAsICZ2cF9kZXYtPmNv bW1vbi0+cXVldWVfc2l6ZSk7CisJaW93cml0ZTE2KGluZm8tPmRlc2lyZWRfbnVtLCAmdnBfZGV2 LT5jb21tb24tPnF1ZXVlX3NpemUpOwogCWlvd3JpdGU2NF90d29wYXJ0KDAsICZ2cF9kZXYtPmNv bW1vbi0+cXVldWVfZGVzYyk7CiAJaW93cml0ZTY0X3R3b3BhcnQoMCwgJnZwX2Rldi0+Y29tbW9u LT5xdWV1ZV9hdmFpbCk7CiAJaW93cml0ZTY0X3R3b3BhcnQoMCwgJnZwX2Rldi0+Y29tbW9uLT5x dWV1ZV91c2VkKTsKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X18KVmlydHVhbGl6YXRpb24gbWFpbGluZyBsaXN0ClZpcnR1YWxpemF0aW9uQGxpc3RzLmxpbnV4 LWZvdW5kYXRpb24ub3JnCmh0dHBzOi8vbGlzdHMubGludXhmb3VuZGF0aW9uLm9yZy9tYWlsbWFu L2xpc3RpbmZvL3ZpcnR1YWxpemF0aW9u From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967521Ab3E3Fey (ORCPT ); Thu, 30 May 2013 01:34:54 -0400 Received: from ozlabs.org ([203.10.76.45]:47105 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967474Ab3E3Fed convert rfc822-to-8bit (ORCPT ); Thu, 30 May 2013 01:34:33 -0400 From: Rusty Russell To: "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org, kvm@vger.kernel.org Cc: "Paul E. McKenney" , David Howells , Dave Jones , linux-kernel@vger.kernel.org Subject: Re: [PATCH] virtio_pci: fix capability format, comments In-Reply-To: <20130528080816.GA23399@redhat.com> References: <20130528080816.GA23399@redhat.com> User-Agent: Notmuch/0.15.2+81~gd2c8818 (http://notmuchmail.org) Emacs/23.4.1 (i686-pc-linux-gnu) Date: Thu, 30 May 2013 11:03:38 +0930 Message-ID: <8738t5metp.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 "Michael S. Tsirkin" writes: > - queue size can actually be 0 which is not a power of 2 Actually, that points to a flaw in the code. When we shut down the queue, we should ideally reset it to what the device started with, rather than 0. See below. > - fix capability format. PCI spec says: > The layout of the information is vendor specific, except that the byte > immediately following the “Next” pointer in the capability structure is > defined to be a length field. > This length field provides the number of bytes in the capability > structure (including the ID and Next pointer bytes). That part's definitely correct: applied. Thanks, Rusty. Subjet: virtio_pci: save the desired ringsize. MST points out that 0 isn't a power of 2. This means we can't re-open a virtio device once we write 0 into the queue length. We should restore the amount the device originally asked for. Signed-off-by: Rusty Russell diff --git a/drivers/virtio/virtio_pci-common.h b/drivers/virtio/virtio_pci-common.h index ba1bf81..0e3143b 100644 --- a/drivers/virtio/virtio_pci-common.h +++ b/drivers/virtio/virtio_pci-common.h @@ -82,6 +82,9 @@ struct virtio_pci_vq_info { /* MSI-X vector (or none) */ unsigned msix_vector; + + /* What size did the device *want* this to be? */ + u16 desired_num; }; /* the notify function used when creating a virt queue */ diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index 0f0e3a6..8b35c2e 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -230,6 +230,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, return ERR_PTR(-ENOMEM); info->msix_vector = msix_vec; + info->desired_num = num; /* get offset of notification word for this vq (shouldn't wrap) */ off = ioread16(&vp_dev->common->queue_notify_off); @@ -350,7 +351,7 @@ static void del_vq(struct virtqueue *vq) vring_del_virtqueue(vq); /* This is for our own benefit, not the device's! */ - iowrite16(0, &vp_dev->common->queue_size); + iowrite16(info->desired_num, &vp_dev->common->queue_size); iowrite64_twopart(0, &vp_dev->common->queue_desc); iowrite64_twopart(0, &vp_dev->common->queue_avail); iowrite64_twopart(0, &vp_dev->common->queue_used);