[PATCH 1/2] virtio_balloon: header update for virtio 1

[PATCH 1/2] virtio_balloon: header update for virtio 1

[Michael S. Tsirkin]

add modern header.
This patch is for virtio 1.0 branch, doesn't
apply to master.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio-balloon.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index f863bfe..79eca67 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -56,6 +56,12 @@ typedef struct VirtIOBalloonStat {
     uint64_t val;
 } QEMU_PACKED VirtIOBalloonStat;
 
+typedef struct virtio_balloon_stat_modern {
+       uint16_t tag;
+       uint8_t reserved[6];
+       uint64_t val;
+} VirtIOBalloonStatModern;
+
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
     VirtQueue *ivq, *dvq, *svq;
-- 
MST

[PATCH 2/2] virtio-balloon: virtio 1 support

[Michael S. Tsirkin]

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@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;
 
-- 
MST

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

[Cornelia Huck]

On Sun, 12 Apr 2015 17:00:48 +0200
"Michael S. Tsirkin" <mst@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@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?

Re: [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" <mst@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@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

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

[Cornelia Huck]

On Mon, 13 Apr 2015 13:26:31 +0200
"Michael S. Tsirkin" <mst@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 :)

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

[Michael S. Tsirkin]

On Mon, Apr 13, 2015 at 01:35:21PM +0200, Cornelia Huck wrote:
> On Mon, 13 Apr 2015 13:26:31 +0200
> "Michael S. Tsirkin" <mst@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 :)

I'm inclined to set it in transport and black-list in specific devices.

-- 
MST

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

[Cornelia Huck]

On Mon, 13 Apr 2015 13:49:42 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Apr 13, 2015 at 01:35:21PM +0200, Cornelia Huck wrote:
> > On Mon, 13 Apr 2015 13:26:31 +0200
> > "Michael S. Tsirkin" <mst@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 :)
> 
> I'm inclined to set it in transport and black-list in specific devices.
> 

Agreed, let's set it in the transports.

For ccw, I'd probably only want to offer it once the driver has
negotiated a revision >= 1, so something like the following (untested):

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 8b6b2ab..4d8cc24 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -703,6 +703,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
         dev->host_features =
             virtio_bus_get_vdev_features_rev(&dev->bus, dev->host_features,
                                              dev->revision >= 1 ? 1 : 0);
+        if (dev->revision >= 1) {
+            virtio_add_feature(&dev->host_features, VIRTIO_F_VERSION_1);
+        }
         break;
     default:
         ret = -ENOSYS;

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

[Cornelia Huck]

On Mon, 13 Apr 2015 13:26:31 +0200
"Michael S. Tsirkin" <mst@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.