From patchwork Tue Dec 2 15:41:36 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cornelia Huck X-Patchwork-Id: 416897 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3CF721402B6 for ; Wed, 3 Dec 2014 02:42:23 +1100 (AEDT) Received: from localhost ([::1]:37443 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xvpan-0001ld-P1 for incoming@patchwork.ozlabs.org; Tue, 02 Dec 2014 10:42:21 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50376) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XvpaO-0001Uz-GR for qemu-devel@nongnu.org; Tue, 02 Dec 2014 10:42:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XvpaD-0007z6-Af for qemu-devel@nongnu.org; Tue, 02 Dec 2014 10:41:56 -0500 Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:43726) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XvpaC-0007yi-UH for qemu-devel@nongnu.org; Tue, 02 Dec 2014 10:41:45 -0500 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 2 Dec 2014 15:41:43 -0000 Received: from d06dlp01.portsmouth.uk.ibm.com (9.149.20.13) by e06smtp17.uk.ibm.com (192.168.101.147) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 2 Dec 2014 15:41:42 -0000 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 921E517D805A for ; Tue, 2 Dec 2014 15:42:00 +0000 (GMT) Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sB2FfgD551183634 for ; Tue, 2 Dec 2014 15:41:42 GMT Received: from d06av08.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sB2FffQu012987 for ; Tue, 2 Dec 2014 08:41:42 -0700 Received: from gondolin (dyn-9-152-224-210.boeblingen.de.ibm.com [9.152.224.210]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id sB2FffFE012952; Tue, 2 Dec 2014 08:41:41 -0700 Date: Tue, 2 Dec 2014 16:41:36 +0100 From: Cornelia Huck To: "Michael S. Tsirkin" Message-ID: <20141202164136.1a57f358.cornelia.huck@de.ibm.com> In-Reply-To: <20141202155444.0731c48f.cornelia.huck@de.ibm.com> References: <1417525227-14051-1-git-send-email-cornelia.huck@de.ibm.com> <1417525227-14051-8-git-send-email-cornelia.huck@de.ibm.com> <20141202144628.GA2974@redhat.com> <20141202155444.0731c48f.cornelia.huck@de.ibm.com> Organization: IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz =?UTF-8?B?R2VzY2jDpGZ0c2bDvGhydW5nOg==?= Dirk Wittkopp Sitz der Gesellschaft: =?UTF-8?B?QsO2Ymxpbmdlbg==?= Registergericht: Amtsgericht Stuttgart, HRB 243294 X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.10; i686-pc-linux-gnu) Mime-Version: 1.0 X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14120215-0029-0000-0000-000001FB0F52 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 195.75.94.113 Cc: thuth@linux.vnet.ibm.com, rusty@rustcorp.com.au, qemu-devel@nongnu.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [Qemu-devel] [PATCH RFC v5 07/19] virtio: allow virtio-1 queue layout X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Tue, 2 Dec 2014 15:54:44 +0100 Cornelia Huck wrote: > On Tue, 2 Dec 2014 16:46:28 +0200 > "Michael S. Tsirkin" wrote: > > pa == -1ULL tricks look quite ugly. > > Can't we set desc/avail/used unconditionally, and drop > > the pa value? > > And have virtio_queue_get_addr() return desc? Let me see if I can come > up with a patch. I came up with the following (untested) patch, which should hopefully suit mmio as well. I haven't cared about migration yet. diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 8f69ffa..ac3c615 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -69,7 +69,6 @@ typedef struct VRing struct VirtQueue { VRing vring; - hwaddr pa; uint16_t last_avail_idx; /* Last used index value we have signalled on */ uint16_t signalled_used; @@ -92,12 +91,13 @@ struct VirtQueue }; /* virt queue functions */ -static void virtqueue_init(VirtQueue *vq) +static void virtqueue_update_rings(VirtQueue *vq) { - hwaddr pa = vq->pa; - - vq->vring.desc = pa; - vq->vring.avail = pa + vq->vring.num * sizeof(VRingDesc); + if (!vq->vring.desc) { + /* not yet setup -> nothing to do */ + return; + } + vq->vring.avail = vq->vring.desc + vq->vring.num * sizeof(VRingDesc); vq->vring.used = vring_align(vq->vring.avail + offsetof(VRingAvail, ring[vq->vring.num]), vq->vring.align); @@ -605,7 +605,6 @@ void virtio_reset(void *opaque) vdev->vq[i].vring.avail = 0; vdev->vq[i].vring.used = 0; vdev->vq[i].last_avail_idx = 0; - vdev->vq[i].pa = 0; vdev->vq[i].vector = VIRTIO_NO_VECTOR; vdev->vq[i].signalled_used = 0; vdev->vq[i].signalled_used_valid = false; @@ -708,17 +707,34 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data) void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) { - vdev->vq[n].pa = addr; - virtqueue_init(&vdev->vq[n]); + vdev->vq[n].vring.desc = addr; + virtqueue_update_rings(&vdev->vq[n]); } hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n) { - return vdev->vq[n].pa; + return vdev->vq[n].vring.desc; +} + +void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, + hwaddr avail, hwaddr used) +{ + vdev->vq[n].vring.desc = desc; + vdev->vq[n].vring.avail = avail; + vdev->vq[n].vring.used = used; } void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) { + /* + * For virtio-1 devices, the number of buffers may only be + * updated if the ring addresses have not yet been set up. + */ + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1) && + vdev->vq[n].vring.desc) { + error_report("tried to modify buffer num for virtio-1 device"); + return; + } /* Don't allow guest to flip queue between existent and * nonexistent states, or to set it to an invalid size. */ @@ -728,7 +744,7 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num) return; } vdev->vq[n].vring.num = num; - virtqueue_init(&vdev->vq[n]); + virtqueue_update_rings(&vdev->vq[n]); } int virtio_queue_get_num(VirtIODevice *vdev, int n) @@ -748,6 +764,11 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align) BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); + /* virtio-1 compliant devices cannot change the aligment */ + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { + error_report("tried to modify queue alignment for virtio-1 device"); + return; + } /* Check that the transport told us it was going to do this * (so a buggy transport will immediately assert rather than * silently failing to migrate this state) @@ -755,7 +776,7 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align) assert(k->has_variable_vring_alignment); vdev->vq[n].vring.align = align; - virtqueue_init(&vdev->vq[n]); + virtqueue_update_rings(&vdev->vq[n]); } void virtio_queue_notify_vq(VirtQueue *vq) @@ -949,7 +970,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) if (k->has_variable_vring_alignment) { qemu_put_be32(f, vdev->vq[i].vring.align); } - qemu_put_be64(f, vdev->vq[i].pa); + /* XXX virtio-1 devices */ + qemu_put_be64(f, vdev->vq[i].vring.desc); qemu_put_be16s(f, &vdev->vq[i].last_avail_idx); if (k->save_queue) { k->save_queue(qbus->parent, i, f); @@ -1044,13 +1066,14 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) if (k->has_variable_vring_alignment) { vdev->vq[i].vring.align = qemu_get_be32(f); } - vdev->vq[i].pa = qemu_get_be64(f); + vdev->vq[i].vring.desc = qemu_get_be64(f); qemu_get_be16s(f, &vdev->vq[i].last_avail_idx); vdev->vq[i].signalled_used_valid = false; vdev->vq[i].notification = true; - if (vdev->vq[i].pa) { - virtqueue_init(&vdev->vq[i]); + if (vdev->vq[i].vring.desc) { + /* XXX virtio-1 devices */ + virtqueue_update_rings(&vdev->vq[i]); } else if (vdev->vq[i].last_avail_idx) { error_report("VQ %d address 0x0 " "inconsistent with Host index 0x%x", @@ -1084,7 +1107,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) } for (i = 0; i < num; i++) { - if (vdev->vq[i].pa) { + if (vdev->vq[i].vring.desc) { uint16_t nheads; nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; /* Check it isn't doing strange things with descriptor numbers. */ diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 68c40db..80ee313 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -224,6 +224,8 @@ void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr); hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n); void virtio_queue_set_num(VirtIODevice *vdev, int n, int num); int virtio_queue_get_num(VirtIODevice *vdev, int n); +void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, + hwaddr avail, hwaddr used); void virtio_queue_set_align(VirtIODevice *vdev, int n, int align); void virtio_queue_notify(VirtIODevice *vdev, int n); uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);