From patchwork Mon Mar 28 21:14:16 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 88686 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 07C69B6F8E for ; Tue, 29 Mar 2011 08:15:55 +1100 (EST) Received: from localhost ([127.0.0.1]:60656 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q4Jmx-0004w6-JZ for incoming@patchwork.ozlabs.org; Mon, 28 Mar 2011 17:15:51 -0400 Received: from [140.186.70.92] (port=50280 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q4Jlp-0004Ym-Th for qemu-devel@nongnu.org; Mon, 28 Mar 2011 17:14:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q4Jlo-0006ly-FX for qemu-devel@nongnu.org; Mon, 28 Mar 2011 17:14:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12608) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q4Jlo-0006lt-3R for qemu-devel@nongnu.org; Mon, 28 Mar 2011 17:14:40 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2SLEZm1015948 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 28 Mar 2011 17:14:35 -0400 Received: from redhat.com (vpn-200-95.tlv.redhat.com [10.35.200.95]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id p2SLEUX3026249; Mon, 28 Mar 2011 17:14:31 -0400 Date: Mon, 28 Mar 2011 23:14:16 +0200 From: "Michael S. Tsirkin" To: qemu-devel@nongnu.org, Anthony Liguori , gleb@redhat.com, Jason Wang , Alex Williamson , Jes Sorensen , Amit Shah , Christoph Hellwig , armbru@redhat.com, kwolf@redhat.com Message-ID: <4e66df1d082db84937a97ef18595fafcc3d68e55.1301346785.git.mst@redhat.com> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Mutt-Fcc: =sent User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Subject: [Qemu-devel] [PATCH 1/3] virtio: don't exit on guest errors X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org When guest does something illegal, such as programming invalid index values in the virtio device, qemu currently tends to crash. With virtio, a better idea is to log an error, and set status to FAIL which stops the device. Add an API to do this, and fix core, blk and serial to use it on error. Signed-off-by: Michael S. Tsirkin --- hw/virtio-blk.c | 12 +++++-- hw/virtio-serial-bus.c | 13 +++++-- hw/virtio.c | 79 +++++++++++++++++++++++++++++++----------------- hw/virtio.h | 7 +++- 4 files changed, 73 insertions(+), 38 deletions(-) diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index cff21a9..fdaaf89 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -502,10 +502,14 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) req->next = s->rq; s->rq = req; - virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr, - req->elem.in_num, 1); - virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr, - req->elem.out_num, 0); + if (virtqueue_map_sg(s->vq, req->elem.in_sg, req->elem.in_addr, + req->elem.in_num, 1)) { + return -EINVAL; + } + if (virtqueue_map_sg(s->vq, req->elem.out_sg, req->elem.out_addr, + req->elem.out_num, 0)) { + return -EINVAL; + } } return 0; diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 7ae2b0d..8807a2f 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -679,10 +679,15 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) qemu_get_buffer(f, (unsigned char *)&port->elem, sizeof(port->elem)); - virtqueue_map_sg(port->elem.in_sg, port->elem.in_addr, - port->elem.in_num, 1); - virtqueue_map_sg(port->elem.out_sg, port->elem.out_addr, - port->elem.out_num, 1); + if (virtqueue_map_sg(port->ivq, port->elem.in_sg, + port->elem.in_addr, + port->elem.in_num, 1)) { + return -EINVAL; + } + if (virtqueue_map_sg(port->ovq, port->elem.out_sg, port->elem.out_addr, + port->elem.out_num, 1)) { + return -EINVAL; + } /* * Port was throttled on source machine. Let's diff --git a/hw/virtio.c b/hw/virtio.c index d5013b6..d1c8769 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -16,6 +16,7 @@ #include "trace.h" #include "virtio.h" #include "sysemu.h" +#include "qemu-error.h" /* The alignment to use between consumer and producer parts of vring. * x86 pagesize again. */ @@ -253,15 +254,15 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx) /* Check it isn't doing very strange things with descriptor numbers. */ if (num_heads > vq->vring.num) { - fprintf(stderr, "Guest moved used index from %u to %u", - idx, vring_avail_idx(vq)); - exit(1); + virtio_error(vq->vdev, "Guest moved used index from %u to %u", + idx, vring_avail_idx(vq)); + return 0; } return num_heads; } -static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx) +static int virtqueue_get_head(VirtQueue *vq, unsigned int idx) { unsigned int head; @@ -271,14 +272,14 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx) /* If their number is silly, that's a fatal mistake. */ if (head >= vq->vring.num) { - fprintf(stderr, "Guest says index %u is available", head); - exit(1); + virtio_error(vq->vdev, "Guest says index %u is available", head); + return -1; } return head; } -static unsigned virtqueue_next_desc(target_phys_addr_t desc_pa, +static unsigned virtqueue_next_desc(VirtQueue *vq, target_phys_addr_t desc_pa, unsigned int i, unsigned int max) { unsigned int next; @@ -293,8 +294,8 @@ static unsigned virtqueue_next_desc(target_phys_addr_t desc_pa, wmb(); if (next >= max) { - fprintf(stderr, "Desc next is %u", next); - exit(1); + virtio_error(vq->vdev, "Desc next is %u", next); + return max; } return next; @@ -316,18 +317,21 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes) max = vq->vring.num; num_bufs = total_bufs; i = virtqueue_get_head(vq, idx++); + if (i < 0) { + return 0; + } desc_pa = vq->vring.desc; if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) { if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) { - fprintf(stderr, "Invalid size for indirect buffer table\n"); - exit(1); + virtio_error(vq->vdev, "Invalid size for indirect buffer table\n"); + return 0; } /* If we've got too many, that implies a descriptor loop. */ if (num_bufs >= max) { - fprintf(stderr, "Looped descriptor"); - exit(1); + virtio_error(vq->vdev, "Looped descriptor"); + return 0; } /* loop over the indirect descriptor table */ @@ -340,8 +344,8 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes) do { /* If we've got too many, that implies a descriptor loop. */ if (++num_bufs > max) { - fprintf(stderr, "Looped descriptor"); - exit(1); + virtio_error(vq->vdev, "Looped descriptor"); + return 0; } if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) { @@ -353,7 +357,7 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes) (out_total += vring_desc_len(desc_pa, i)) >= out_bytes) return 1; } - } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max); + } while ((i = virtqueue_next_desc(vq, desc_pa, i, max)) != max); if (!indirect) total_bufs = num_bufs; @@ -364,8 +368,8 @@ int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes) return 0; } -void virtqueue_map_sg(struct iovec *sg, target_phys_addr_t *addr, - size_t num_sg, int is_write) +int virtqueue_map_sg(VirtQueue *vq, struct iovec *sg, + target_phys_addr_t *addr, size_t num_sg, int is_write) { unsigned int i; target_phys_addr_t len; @@ -374,15 +378,16 @@ void virtqueue_map_sg(struct iovec *sg, target_phys_addr_t *addr, len = sg[i].iov_len; sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write); if (sg[i].iov_base == NULL || len != sg[i].iov_len) { - fprintf(stderr, "virtio: trying to map MMIO memory\n"); - exit(1); + return -1; } } + return 0; } int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem) { - unsigned int i, head, max; + int i, head; + unsigned max; target_phys_addr_t desc_pa = vq->vring.desc; if (!virtqueue_num_heads(vq, vq->last_avail_idx)) @@ -394,11 +399,14 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem) max = vq->vring.num; i = head = virtqueue_get_head(vq, vq->last_avail_idx++); + if (i < 0) { + return 0; + } if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) { if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) { - fprintf(stderr, "Invalid size for indirect buffer table\n"); - exit(1); + virtio_error(vq->vdev, "Invalid size for indirect buffer table\n"); + return 0; } /* loop over the indirect descriptor table */ @@ -423,14 +431,18 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem) /* If we've got too many, that implies a descriptor loop. */ if ((elem->in_num + elem->out_num) > max) { - fprintf(stderr, "Looped descriptor"); - exit(1); + virtio_error(vq->vdev, "Looped descriptor"); + return 0; } - } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max); + } while ((i = virtqueue_next_desc(vq, desc_pa, i, max)) != max); /* Now map what we have collected */ - virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1); - virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0); + if (virtqueue_map_sg(vq, elem->in_sg, elem->in_addr, elem->in_num, 1)) { + return 0; + } + if (virtqueue_map_sg(vq, elem->out_sg, elem->out_addr, elem->out_num, 0)) { + return 0; + } elem->index = head; @@ -863,3 +875,14 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq) { return &vq->host_notifier; } + +void virtio_error(VirtIODevice *vdev, const char *fmt, ...) +{ + va_list ap; + + virtio_set_status(vdev, VIRTIO_CONFIG_S_FAILED); + + va_start(ap, fmt); + error_vprintf(fmt, ap); + va_end(ap); +} diff --git a/hw/virtio.h b/hw/virtio.h index 6d3d960..9771a9c 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -134,6 +134,9 @@ static inline void virtio_set_status(VirtIODevice *vdev, uint8_t val) vdev->status = val; } +void virtio_error(VirtIODevice *vdev, const char *fmt, ...) + __attribute__ ((format(printf, 2, 3))); + VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, void (*handle_output)(VirtIODevice *, VirtQueue *)); @@ -144,8 +147,8 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count); void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len, unsigned int idx); -void virtqueue_map_sg(struct iovec *sg, target_phys_addr_t *addr, - size_t num_sg, int is_write); +int virtqueue_map_sg(VirtQueue *vq, struct iovec *sg, + target_phys_addr_t *addr, size_t num_sg, int is_write); int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem); int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes);