From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44451) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YhZKu-0001zJ-Bm for qemu-devel@nongnu.org; Mon, 13 Apr 2015 04:03:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YhZKq-0007j0-68 for qemu-devel@nongnu.org; Mon, 13 Apr 2015 04:03:16 -0400 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:45684) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YhZKp-0007iY-Sz for qemu-devel@nongnu.org; Mon, 13 Apr 2015 04:03:12 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 13 Apr 2015 09:03:08 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id E4EAD17D8066 for ; Mon, 13 Apr 2015 09:03:40 +0100 (BST) Received: from d06av10.portsmouth.uk.ibm.com (d06av10.portsmouth.uk.ibm.com [9.149.37.251]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t3D835xm8585536 for ; Mon, 13 Apr 2015 08:03:05 GMT Received: from d06av10.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av10.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t3D8353d003315 for ; Mon, 13 Apr 2015 02:03:05 -0600 Date: Mon, 13 Apr 2015 10:02:58 +0200 From: Cornelia Huck Message-ID: <20150413100258.1f2c636f.cornelia.huck@de.ibm.com> In-Reply-To: <1428849904-28953-2-git-send-email-mst@redhat.com> References: <1428849904-28953-1-git-send-email-mst@redhat.com> <1428849904-28953-2-git-send-email-mst@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: virtio 1 support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Rusty Russell , qemu-devel@nongnu.org, Anthony Liguori , virtualization@lists.linux-foundation.org On Sun, 12 Apr 2015 17:00:48 +0200 "Michael S. Tsirkin" wrote: > Virtio 1.0 doesn't include a modern balloon device. At some point we'll > likely define an incompatible interface with a different ID and > different semantics. But for now, it's not a big effort to support a > transitional balloon device: this has the advantage of supporting > existing drivers, transparently, as well as transports that don't allow > mixing virtio 0 and virtio 1 devices. And balloon is an easy device to > test, so it's also useful for people to test virtio core handling of > transitional devices. > > The only interface issue is with the stats buffer, which has misaligned > fields. We could leave it as is, but this sets a bad precedent that > others might copy by mistake. > > Signed-off-by: Michael S. Tsirkin > --- > hw/virtio/virtio-balloon.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index d2d7c3e..568a008 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -239,7 +239,8 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) > { > VirtIOBalloon *s = VIRTIO_BALLOON(vdev); > VirtQueueElement *elem = &s->stats_vq_elem; > - VirtIOBalloonStat stat; > + VirtIOBalloonStat legacy_stat; > + VirtIOBalloonStatModern modern_stat; > size_t offset = 0; > qemu_timeval tv; > > @@ -253,14 +254,28 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) > */ > reset_stats(s); > > - while (iov_to_buf(elem->out_sg, elem->out_num, offset, &stat, sizeof(stat)) > - == sizeof(stat)) { > - uint16_t tag = virtio_tswap16(vdev, stat.tag); > - uint64_t val = virtio_tswap64(vdev, stat.val); > + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { > + while (iov_to_buf(elem->out_sg, elem->out_num, offset, > + &modern_stat, sizeof(modern_stat)) > + == sizeof(modern_stat)) { > + uint16_t tag = le16_to_cpu(modern_stat.tag); > + uint64_t val = le64_to_cpu(modern_stat.val); > > - offset += sizeof(stat); > - if (tag < VIRTIO_BALLOON_S_NR) > - s->stats[tag] = val; > + offset += sizeof(modern_stat); > + if (tag < VIRTIO_BALLOON_S_NR) > + s->stats[tag] = val; > + } > + } else { > + while (iov_to_buf(elem->out_sg, elem->out_num, offset, > + &legacy_stat, sizeof(legacy_stat)) > + == sizeof(legacy_stat)) { > + uint16_t tag = virtio_tswap16(vdev, legacy_stat.tag); > + uint64_t val = virtio_tswap64(vdev, legacy_stat.val); > + > + offset += sizeof(legacy_stat); > + if (tag < VIRTIO_BALLOON_S_NR) > + s->stats[tag] = val; > + } > } > s->stats_vq_offset = offset; > I'm still not really convinced that changing the stat structure is an improvement. Without that, you wouldn't need the above change at all, would you? Also, doesn't get_features need to be modified as well so that VERSION_1 is advertised?