Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: virtio 1 support

[Michael S. Tsirkin]

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" <address@hidden> 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 <address@hidden>
> > ---
> >  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