From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET" Date: Mon, 20 Apr 2015 19:36:40 +0200 Message-ID: <20150420175905-mutt-send-email-mst@redhat.com> References: <1429257573-7359-1-git-send-email-famz@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <1429257573-7359-1-git-send-email-famz@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: Fam Zheng Cc: qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org, "Aneesh Kumar K.V" , Stefan Hajnoczi , Amit Shah , Paolo Bonzini List-Id: virtualization@lists.linuxfoundation.org T24gRnJpLCBBcHIgMTcsIDIwMTUgYXQgMDM6NTk6MTVQTSArMDgwMCwgRmFtIFpoZW5nIHdyb3Rl Ogo+IEN1cnJlbnRseSwgdmlydGlvIGNvZGUgY2hvb3NlcyB0byBraWxsIFFFTVUgaWYgdGhlIGd1 ZXN0IHBhc3NlcyBhbnkgaW52YWxpZAo+IGRhdGEgd2l0aCB2cmluZy4KPiBUaGF0IGhhcyBkcmF3 YmFja3Mgc3VjaCBhcyBsb3NpbmcgdW5zYXZlZCBkYXRhIChlLmcuIHdoZW4KPiBndWVzdCB1c2Vy IGlzIHdyaXRpbmcgYSB2ZXJ5IGxvbmcgZW1haWwpLCBvciBwb3NzaWJsZSBkZW5pYWwgb2Ygc2Vy dmljZSBpbgo+IGEgbmVzdGVkIHZtIHVzZSBjYXNlIHdoZXJlIHZpcnRpbyBkZXZpY2UgaXMgcGFz c2VkIHRocm91Z2guCj4gCj4gdmlydGlvLTEgaGFzIGludHJvZHVjZWQgYSBuZXcgc3RhdHVzIGJp dCAiTkVFRFMgUkVTRVQiIHdoaWNoIGNvdWxkIGJlIHVzZWQgdG8KPiBpbXByb3ZlIHRoaXMgYnkg Y29tbXVuaWNhdGluZyB0aGUgZXJyb3Igc3RhdGUgYmV0d2VlbiB2aXJ0aW8gZGV2aWNlcyBhbmQK PiBkcml2ZXJzLiBUaGUgZGV2aWNlIG5vdGlmaWVzIGd1ZXN0IHVwb24gc2V0dGluZyB0aGUgYml0 LCB0aGVuIHRoZSBndWVzdCBkcml2ZXIKPiBzaG91bGQgZGV0ZWN0IHRoaXMgYml0IGFuZCByZXBv cnQgdG8gdXNlcnNwYWNlLCBvciByZWNvdmVyIHRoZSBkZXZpY2UgYnkKPiByZXNldHRpbmcgaXQu CgpVbmZvcnR1bmF0ZWx5LCB2aXJ0aW8gMSBzcGVjIGRvZXMgbm90IGhhdmUgYSBjb25mb3JtYW5j ZSBzdGF0ZW1lbnQKdGhhdCByZXF1aXJlcyBkcml2ZXIgdG8gcmVjb3Zlci4gV2UgbWVyZWx5IGhh dmUgYSBub24tbm9ybWF0aXZlIGxvb2tpbmcKdGV4dDoKCU5vdGU6IEZvciBleGFtcGxlLCB0aGUg ZHJpdmVyIGNhbuKAmXQgYXNzdW1lIHJlcXVlc3RzIGluIGZsaWdodAoJd2lsbCBiZSBjb21wbGV0 ZWQgaWYgREVWSUNFX05FRURTX1JFU0VUIGlzIHNldCwgbm9yIGNhbiBpdCBhc3N1bWUgdGhhdAoJ dGhleSBoYXZlIG5vdCBiZWVuIGNvbXBsZXRlZC4gQSBnb29kIGltcGxlbWVudGF0aW9uIHdpbGwg dHJ5IHRvIHJlY292ZXIKCWJ5IGlzc3VpbmcgYSByZXNldC4KCkltcGxlbWVudGluZyB0aGlzIHJl c2V0IGZvciBhbGwgZGV2aWNlcyBpbiBhIHJhY2UtZnJlZSBtYW5uZXIgbWlnaHQgYWxzbwpiZSBm YXIgZnJvbSB0cml2aWFsLiAgSSB0aGluayB3ZSdkIG5lZWQgYSBmZWF0dXJlIGJpdCBmb3IgdGhp cy4KT1RPSCBhcyBsb25nIGFzIHdlIG1ha2UgdGhpcyBhIG5ldyBmZWF0dXJlLCB3b3VsZCBhbiBh YmlsaXR5IHRvCnJlc2V0IGEgc2luZ2xlIFZRIGJlIGEgYmV0dGVyIG1hdGNoIGZvciB3aGF0IHlv dSBhcmUgdHJ5aW5nIHRvCmFjaGlldmU/Cgo+IFRoaXMgc2VyaWVzIG1ha2VzIG5lY2Vzc2FyeSBj aGFuZ2VzIGluIHZpcnRpbyBjb3JlIGNvZGUsIGJhc2VkIG9uIHdoaWNoCj4gdmlydGlvLWJsayBp cyBjb252ZXJ0ZWQuIE90aGVyIGRldmljZXMgbm93IGtlZXAgdGhlIGV4aXN0aW5nIGJlaGF2aW9y IGJ5Cj4gcGFzc2luZyBpbiAiZXJyb3JfYWJvcnQiLiBUaGV5IHdpbGwgYmUgY29udmVydGVkIGlu IGZvbGxvd2luZyBzZXJpZXMuIFRoZSBMaW51eAo+IGRyaXZlciBwYXJ0IHdpbGwgYWxzbyBiZSB3 b3JrZWQgb24uCj4gCj4gT25lIGNvbmNlcm4gd2l0aCB0aGlzIGJlaGF2aW9yIGNoYW5nZSBpcyB0 aGF0IGl0J3Mgbm93IGhhcmRlciB0byBub3RpY2UgdGhlCj4gYWN0dWFsIGRyaXZlciBidWcgdGhh dCBjYXVzZWQgdGhlIGVycm9yLCBhcyB0aGUgZ3Vlc3QgY29udGludWVzIHRvIHJ1bi4gIFRvCj4g YWRkcmVzcyB0aGF0LCB3ZSBjb3VsZCBwcm9iYWJseSBhZGQgYSBuZXcgZXJyb3IgYWN0aW9uIG9w dGlvbiB0byB2aXJ0aW8KPiBkZXZpY2VzLCAgc2ltaWxhciB0byB0aGUgInJlYWQvd3JpdGUgd2Vy cm9yIiBpbiBibG9jayBsYXllciwgc28gdGhlIHZtIGNvdWxkIGJlCj4gcGF1c2VkIGFuZCB0aGUg bWFuYWdlbWVudCB3aWxsIGdldCBhbiBldmVudCBpbiBRTVAgbGlrZSBwdnBhbmljLiAgVGhpcyB3 b3JrIGNhbgo+IGJlIGRvbmUgb24gdG9wLgoKQXQgdGhlIGFyY2hpdGVjdHVyYWwgbGV2ZWwsIHRo YXQncyBvbmx5IG9uZSBjb25jZXJuLiBPdGhlcnMgd291bGQgYmUKLSB3b3JrbG9hZHMgc3VjaCBh cyBvcGVuc3RhY2sgaGFuZGxlIGd1ZXN0IGNyYXNoIGJldHRlciB0aGFuCiAgYSBndWVzdCB0aGF0 J3MgZS5nLiBzbG93IGJlY2F1c2Ugb2YgYSBtZW1vcnkgbGVhawotIGl0J3MgZWFzaWVyIGZvciBn dWVzdHMgdG8gcHJvYmUgaG9zdCBmb3Igc2VjdXJpdHkgaXNzdWVzCiAgaWYgZ3Vlc3QgaXNuJ3Qg a2lsbGVkCi0gZ3Vlc3QgY2FuIGZsb29kIGhvc3QgbG9nIHdpdGggZ3Vlc3QtdHJpZ2dlcmVkIGVy cm9ycwoKCkF0IHRoZSBpbXBsZW1lbnRhdGlvbiBsZXZlbCwgdGhlcmUncyBvbmUgYmlnIGlzc3Vl IHlvdSBzZWVtIHRvIGhhdmUKbWlzc2VkOiBETUEgdG8gaW52YWxpZCBtZW1vcnkgYWRkcmVzc2Vz IGNhdXNlcyBhIGNyYXNoIGluIG1lbW9yeSBjb3JlLgpJJ20gbm90IHN1cmUgd2hldGhlciBpdCBt YWtlcyBzZW5zZSB0byByZWNvdmVyIGZyb20gdmlydGlvIGNvcmUgYnVncwp3aGVuIHdlIGNhbid0 IHJlY292ZXIgZnJvbSBkZXZpY2UgYnVncy4KCgo+IAo+IAo+IEZhbSBaaGVuZyAoMTgpOgo+ICAg dmlydGlvOiBSZXR1cm4gZXJyb3IgZnJvbSB2aXJ0cXVldWVfbWFwX3NnCj4gICB2aXJ0aW86IFJl dHVybiBlcnJvciBmcm9tIHZpcnRxdWV1ZV9udW1faGVhZHMKPiAgIHZpcnRpbzogUmV0dXJuIGVy cm9yIGZyb20gdmlydHF1ZXVlX2dldF9oZWFkCj4gICB2aXJ0aW86IFJldHVybiBlcnJvciBmcm9t IHZpcnRxdWV1ZV9uZXh0X2Rlc2MKPiAgIHZpcnRpbzogUmV0dXJuIGVycm9yIGZyb20gdmlydHF1 ZXVlX2dldF9hdmFpbF9ieXRlcwo+ICAgdmlydGlvOiBSZXR1cm4gZXJyb3IgZnJvbSB2aXJ0cXVl dWVfcG9wCj4gICB2aXJ0aW86IFJldHVybiBlcnJvciBmcm9tIHZpcnRxdWV1ZV9hdmFpbF9ieXRl cwo+ICAgdmlydGlvOiBSZXR1cm4gZXJyb3IgZnJvbSB2aXJ0aW9fYWRkX3F1ZXVlCj4gICB2aXJ0 aW86IFJldHVybiBlcnJvciBmcm9tIHZpcnRpb19kZWxfcXVldWUKPiAgIHZpcnRpbzogQWRkIG1h Y3JvIGZvciBWSVJUSU9fQ09ORklHX1NfTkVFRFNfUkVTRVQKPiAgIHZpcnRpbzogQWRkICJuZWVk c19yZXNldCIgZmxhZyB0byB2aXJ0aW8gZGV2aWNlCj4gICB2aXJ0aW86IFJldHVybiAtRUlOVkFM IGlmIHRoZSB2ZGV2IG5lZWRzIHJlc2V0IGluIHZpcnRxdWV1ZV9wb3AKPiAgIHZpcnRpby1ibGs6 IEdyYWNlZnVsIGVycm9yIGhhbmRsaW5nIG9mIHZpcnRxdWV1ZV9wb3AKPiAgIHF0ZXN0OiBBZGQg IlFURVNUX0ZJTFRFUiIgdG8gZmlsdGVyIHRlc3QgY2FzZXMKPiAgIHF0ZXN0OiB2aXJ0aW8tYmxr OiBFeHRyYWN0ICJzZXR1cCIgZm9yIGZ1dHVyZSByZXVzZQo+ICAgbGlicW9zOiBBZGQgcXZpcnRp b19uZWVkc19yZXNldAo+ICAgcXRlc3Q6IEFkZCB0ZXN0IGNhc2UgZm9yICJuZWVkcyByZXNldCIg b2YgdmlydGlvLWJsawo+ICAgcXRlc3Q6IHZpcnRpby1ibGs6IFN1cHByZXNzIHZpcnRpbyBlcnJv ciBtZXNzYWdlcyBpbiAibWFrZSBjaGVjayIKPiAKPiAgaHcvOXBmcy92aXJ0aW8tOXAtZGV2aWNl LmMgICAgICAgICAgICAgICAgICAgICB8ICAgMiArLQo+ICBody85cGZzL3ZpcnRpby05cC5jICAg ICAgICAgICAgICAgICAgICAgICAgICAgIHwgICAyICstCj4gIGh3L2Jsb2NrL2RhdGFwbGFuZS92 aXJ0aW8tYmxrLmMgICAgICAgICAgICAgICAgfCAgIDkgKy0KPiAgaHcvYmxvY2svdmlydGlvLWJs ay5jICAgICAgICAgICAgICAgICAgICAgICAgICB8ICA2MiArKysrKy0tCj4gIGh3L2NoYXIvdmly dGlvLXNlcmlhbC1idXMuYyAgICAgICAgICAgICAgICAgICAgfCAgMzAgKystLQo+ICBody9uZXQv dmlydGlvLW5ldC5jICAgICAgICAgICAgICAgICAgICAgICAgICAgIHwgIDM2ICsrKy0tCj4gIGh3 L3Njc2kvdmlydGlvLXNjc2kuYyAgICAgICAgICAgICAgICAgICAgICAgICAgfCAgIDggKy0KPiAg aHcvdmlydGlvL3ZpcnRpby1iYWxsb29uLmMgICAgICAgICAgICAgICAgICAgICB8ICAxMyArLQo+ ICBody92aXJ0aW8vdmlydGlvLXJuZy5jICAgICAgICAgICAgICAgICAgICAgICAgIHwgICA2ICst Cj4gIGh3L3ZpcnRpby92aXJ0aW8uYyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfCAyMTQg KysrKysrKysrKysrKysrKysrLS0tLS0tLQo+ICBpbmNsdWRlL2h3L3ZpcnRpby92aXJ0aW8tYmxr LmggICAgICAgICAgICAgICAgIHwgICAzICstCj4gIGluY2x1ZGUvaHcvdmlydGlvL3ZpcnRpby5o ICAgICAgICAgICAgICAgICAgICAgfCAgMTcgKy0KPiAgaW5jbHVkZS9zdGFuZGFyZC1oZWFkZXJz L2xpbnV4L3ZpcnRpb19jb25maWcuaCB8ICAgMiArCj4gIHRlc3RzL01ha2VmaWxlICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgfCAgIDYgKy0KPiAgdGVzdHMvbGlicW9zL3ZpcnRpby5j ICAgICAgICAgICAgICAgICAgICAgICAgICB8ICAgNSArCj4gIHRlc3RzL2xpYnFvcy92aXJ0aW8u aCAgICAgICAgICAgICAgICAgICAgICAgICAgfCAgIDIgKwo+ICB0ZXN0cy92aXJ0aW8tYmxrLXRl c3QuYyAgICAgICAgICAgICAgICAgICAgICAgIHwgMTk2ICsrKysrKysrKysrKysrKysrKysrLS0K PiAgMTcgZmlsZXMgY2hhbmdlZCwgNDgyIGluc2VydGlvbnMoKyksIDEzMSBkZWxldGlvbnMoLSkK PiAKPiAtLSAKPiAxLjkuMwo+IApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fXwpWaXJ0dWFsaXphdGlvbiBtYWlsaW5nIGxpc3QKVmlydHVhbGl6YXRpb25AbGlz dHMubGludXgtZm91bmRhdGlvbi5vcmcKaHR0cHM6Ly9saXN0cy5saW51eGZvdW5kYXRpb24ub3Jn L21haWxtYW4vbGlzdGluZm8vdmlydHVhbGl6YXRpb24= From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57960) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkFtm-0008L9-MI for qemu-devel@nongnu.org; Mon, 20 Apr 2015 13:54:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkFth-0005gp-1o for qemu-devel@nongnu.org; Mon, 20 Apr 2015 13:54:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38254) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkFtg-0005gS-Jx for qemu-devel@nongnu.org; Mon, 20 Apr 2015 13:54:16 -0400 Date: Mon, 20 Apr 2015 19:36:40 +0200 From: "Michael S. Tsirkin" Message-ID: <20150420175905-mutt-send-email-mst@redhat.com> References: <1429257573-7359-1-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1429257573-7359-1-git-send-email-famz@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , Rusty Russell , qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org, "Aneesh Kumar K.V" , Stefan Hajnoczi , Amit Shah , Paolo Bonzini On Fri, Apr 17, 2015 at 03:59:15PM +0800, Fam Zheng wrote: > Currently, virtio code chooses to kill QEMU if the guest passes any inv= alid > data with vring. > That has drawbacks such as losing unsaved data (e.g. when > guest user is writing a very long email), or possible denial of service= in > a nested vm use case where virtio device is passed through. >=20 > virtio-1 has introduced a new status bit "NEEDS RESET" which could be u= sed to > improve this by communicating the error state between virtio devices an= d > drivers. The device notifies guest upon setting the bit, then the guest= driver > should detect this bit and report to userspace, or recover the device b= y > resetting it. Unfortunately, virtio 1 spec does not have a conformance statement that requires driver to recover. We merely have a non-normative looking text: Note: For example, the driver can=E2=80=99t assume requests in flight will be completed if DEVICE_NEEDS_RESET is set, nor can it assume that they have not been completed. A good implementation will try to recover by issuing a reset. Implementing this reset for all devices in a race-free manner might also be far from trivial. I think we'd need a feature bit for this. OTOH as long as we make this a new feature, would an ability to reset a single VQ be a better match for what you are trying to achieve? > This series makes necessary changes in virtio core code, based on which > virtio-blk is converted. Other devices now keep the existing behavior b= y > passing in "error_abort". They will be converted in following series. T= he Linux > driver part will also be worked on. >=20 > One concern with this behavior change is that it's now harder to notice= the > actual driver bug that caused the error, as the guest continues to run.= To > address that, we could probably add a new error action option to virtio > devices, similar to the "read/write werror" in block layer, so the vm = could be > paused and the management will get an event in QMP like pvpanic. This = work can > be done on top. At the architectural level, that's only one concern. Others would be - workloads such as openstack handle guest crash better than a guest that's e.g. slow because of a memory leak - it's easier for guests to probe host for security issues if guest isn't killed - guest can flood host log with guest-triggered errors At the implementation level, there's one big issue you seem to have missed: DMA to invalid memory addresses causes a crash in memory core. I'm not sure whether it makes sense to recover from virtio core bugs when we can't recover from device bugs. >=20 >=20 > Fam Zheng (18): > virtio: Return error from virtqueue_map_sg > virtio: Return error from virtqueue_num_heads > virtio: Return error from virtqueue_get_head > virtio: Return error from virtqueue_next_desc > virtio: Return error from virtqueue_get_avail_bytes > virtio: Return error from virtqueue_pop > virtio: Return error from virtqueue_avail_bytes > virtio: Return error from virtio_add_queue > virtio: Return error from virtio_del_queue > virtio: Add macro for VIRTIO_CONFIG_S_NEEDS_RESET > virtio: Add "needs_reset" flag to virtio device > virtio: Return -EINVAL if the vdev needs reset in virtqueue_pop > virtio-blk: Graceful error handling of virtqueue_pop > qtest: Add "QTEST_FILTER" to filter test cases > qtest: virtio-blk: Extract "setup" for future reuse > libqos: Add qvirtio_needs_reset > qtest: Add test case for "needs reset" of virtio-blk > qtest: virtio-blk: Suppress virtio error messages in "make check" >=20 > hw/9pfs/virtio-9p-device.c | 2 +- > hw/9pfs/virtio-9p.c | 2 +- > hw/block/dataplane/virtio-blk.c | 9 +- > hw/block/virtio-blk.c | 62 +++++-- > hw/char/virtio-serial-bus.c | 30 ++-- > hw/net/virtio-net.c | 36 +++-- > hw/scsi/virtio-scsi.c | 8 +- > hw/virtio/virtio-balloon.c | 13 +- > hw/virtio/virtio-rng.c | 6 +- > hw/virtio/virtio.c | 214 +++++++++++++++++= +------- > include/hw/virtio/virtio-blk.h | 3 +- > include/hw/virtio/virtio.h | 17 +- > include/standard-headers/linux/virtio_config.h | 2 + > tests/Makefile | 6 +- > tests/libqos/virtio.c | 5 + > tests/libqos/virtio.h | 2 + > tests/virtio-blk-test.c | 196 +++++++++++++++++= +++-- > 17 files changed, 482 insertions(+), 131 deletions(-) >=20 > --=20 > 1.9.3 >=20