On Sun, 12 Apr 2015 17:00:48 +0200 "Michael S. Tsirkin" <mst at redhat.com> 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 <mst at redhat.com> > --- > 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?
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" <mst at redhat.com> 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 <mst at redhat.com> > > --- > > 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
On Mon, 13 Apr 2015 13:26:31 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> On Mon, Apr 13, 2015 at 10:02:58AM +0200, Cornelia Huck wrote:> > 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.Indeed, pci sets it, but ccw does not. And virtio-net, for example, sets it explicitly as well. We need to unify this :)
On Mon, 13 Apr 2015 13:26:31 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> BTW I suspect the stats code is broken for > cross-endian platforms: it should do LE unconditinally, > should it not?Stats are guest-endian for legacy, so no. Only the config space is always LE.