From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44958) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YhcVk-0000Te-AV for qemu-devel@nongnu.org; Mon, 13 Apr 2015 07:26:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YhcVh-00088F-52 for qemu-devel@nongnu.org; Mon, 13 Apr 2015 07:26:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40754) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YhcVg-000889-UJ for qemu-devel@nongnu.org; Mon, 13 Apr 2015 07:26:37 -0400 Date: Mon, 13 Apr 2015 13:26:31 +0200 From: "Michael S. Tsirkin" Message-ID: <20150413132110-mutt-send-email-mst@redhat.com> References: <1428849904-28953-1-git-send-email-mst@redhat.com> <1428849904-28953-2-git-send-email-mst@redhat.com> <20150413100258.1f2c636f.cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150413100258.1f2c636f.cornelia.huck@de.ibm.com> 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: Cornelia Huck Cc: Rusty Russell , qemu-devel@nongnu.org, Anthony Liguori , virtualization@lists.linux-foundation.org On Mon, Apr 13, 2015 at 10:02:58AM +0200, Cornelia Huck wrote: > 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. The point is to avoid setting bad precedent for virtio 1 devices. It does have a bit of cost for transitional devices. > Without that, you wouldn't need the above change at all, > would you? I think so, yes. BTW I suspect the stats code is broken for cross-endian platforms: it should do LE unconditinally, should it not? > Also, doesn't get_features need to be modified as well so that > VERSION_1 is advertised? virtio_pci_device_plugged seems to set it ATM. I'll re-test to confirm. -- MST