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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
