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