Re: [Qemu-devel] [PATCH v2] virtio: make features 64bit wide

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, May 29, 2015 at 09:51:20AM +0200, Gerd Hoffmann wrote:
> Make features 64bit wide everywhere.  Exception: command line flags
> remain 32bit and are copyed into the lower 32 host_features at
> initialization time.
> 
> On migration a full 64bit guest_features field is sent if one of the
> high bits is set, additionally to the lower 32bit guest_features field
> which must stay for compatibility reasons.  That way we send the lower
> 32 feature bits twice, but that way the code is simpler because we don't
> have to split and compose the 64bit features into two 32bit fields.
> 
> This depends on "move host_features" patch by cornelia.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>


Thanks, this is very close to what I had in mind.
Question: why do we need the feature_flags field?
What's wrong with setting bits in host_features directly?


> ---
>  hw/9pfs/virtio-9p-device.c  |  2 +-
>  hw/block/virtio-blk.c       |  2 +-
>  hw/char/virtio-serial-bus.c |  2 +-
>  hw/net/virtio-net.c         | 18 ++++++++-------
>  hw/scsi/vhost-scsi.c        |  4 ++--
>  hw/scsi/virtio-scsi.c       |  4 ++--
>  hw/virtio/virtio-balloon.c  |  2 +-
>  hw/virtio/virtio-rng.c      |  2 +-
>  hw/virtio/virtio.c          | 54 +++++++++++++++++++++++++++++++++++++++------
>  include/hw/virtio/virtio.h  | 21 +++++++++---------
>  10 files changed, 77 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 30492ec..60f9ff9 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -21,7 +21,7 @@
>  #include "virtio-9p-coth.h"
>  #include "hw/virtio/virtio-access.h"
>  
> -static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
> +static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features)
>  {
>      virtio_add_feature(&features, VIRTIO_9P_MOUNT_TAG);
>      return features;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..cd539aa 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -718,7 +718,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
>      aio_context_release(blk_get_aio_context(s->blk));
>  }
>  
> -static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
> +static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
>  {
>      VirtIOBlock *s = VIRTIO_BLK(vdev);
>  
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 6e2ad82..95be9fc 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -498,7 +498,7 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>      }
>  }
>  
> -static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
> +static uint64_t get_features(VirtIODevice *vdev, uint64_t features)
>  {
>      VirtIOSerial *vser;
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3af6faf..b21ef6b 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -435,7 +435,7 @@ static void virtio_net_set_queues(VirtIONet *n)
>  
>  static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
>  
> -static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
> +static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
>      NetClientState *nc = qemu_get_queue(n->nic);
> @@ -468,9 +468,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
>      return vhost_net_get_features(get_vhost_net(nc->peer), features);
>  }
>  
> -static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
> +static uint64_t virtio_net_bad_features(VirtIODevice *vdev)
>  {
> -    uint32_t features = 0;
> +    uint64_t features = 0;
>  
>      /* Linux kernel 2.6.25.  It understood MAC (as everyone must),
>       * but also these: */
> @@ -1032,10 +1032,12 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
>              if (i == 0)
>                  return -1;
>              error_report("virtio-net unexpected empty queue: "
> -                    "i %zd mergeable %d offset %zd, size %zd, "
> -                    "guest hdr len %zd, host hdr len %zd guest features 0x%x",
> -                    i, n->mergeable_rx_bufs, offset, size,
> -                    n->guest_hdr_len, n->host_hdr_len, vdev->guest_features);
> +                         "i %zd mergeable %d offset %zd, size %zd, "
> +                         "guest hdr len %zd, host hdr len %zd "
> +                         "guest features 0x%" PRIx64,
> +                         i, n->mergeable_rx_bufs, offset, size,
> +                         n->guest_hdr_len, n->host_hdr_len,
> +                         vdev->guest_features);
>              exit(1);
>          }
>  
> @@ -1549,7 +1551,7 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>                               vdev, idx, mask);
>  }
>  
> -static void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> +static void virtio_net_set_config_size(VirtIONet *n, uint64_t host_features)
>  {
>      int i, config_size = 0;
>      virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 335f442..9c76486 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -151,8 +151,8 @@ static void vhost_scsi_stop(VHostSCSI *s)
>      vhost_dev_disable_notifiers(&s->dev, vdev);
>  }
>  
> -static uint32_t vhost_scsi_get_features(VirtIODevice *vdev,
> -                                        uint32_t features)
> +static uint64_t vhost_scsi_get_features(VirtIODevice *vdev,
> +                                        uint64_t features)
>  {
>      VHostSCSI *s = VHOST_SCSI(vdev);
>  
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index e242fef..36ae66e 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -628,8 +628,8 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
>      vs->cdb_size = virtio_ldl_p(vdev, &scsiconf->cdb_size);
>  }
>  
> -static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
> -                                         uint32_t requested_features)
> +static uint64_t virtio_scsi_get_features(VirtIODevice *vdev,
> +                                         uint64_t requested_features)
>  {
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 484c3c3..734f35b 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -310,7 +310,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>      trace_virtio_balloon_set_config(dev->actual, oldactual);
>  }
>  
> -static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
> +static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f)
>  {
>      f |= (1 << VIRTIO_BALLOON_F_STATS_VQ);
>      return f;
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 06e7178..420c39f 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -99,7 +99,7 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>      virtio_rng_process(vrng);
>  }
>  
> -static uint32_t get_features(VirtIODevice *vdev, uint32_t f)
> +static uint64_t get_features(VirtIODevice *vdev, uint64_t f)
>  {
>      return f;
>  }
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 96d9c6a..4c2911a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -893,6 +893,13 @@ static bool virtio_device_endian_needed(void *opaque)
>      return vdev->device_endian != virtio_default_endian();
>  }
>  
> +static bool virtio_64bit_features_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    return (vdev->host_features >> 32) != 0;
> +}
> +
>  static const VMStateDescription vmstate_virtio_device_endian = {
>      .name = "virtio/device_endian",
>      .version_id = 1,
> @@ -903,6 +910,16 @@ static const VMStateDescription vmstate_virtio_device_endian = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_virtio_64bit_features = {
> +    .name = "virtio/64bit_features",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(guest_features, VirtIODevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio = {
>      .name = "virtio",
>      .version_id = 1,
> @@ -916,6 +933,10 @@ static const VMStateDescription vmstate_virtio = {
>              .vmsd = &vmstate_virtio_device_endian,
>              .needed = &virtio_device_endian_needed
>          },
> +        {
> +            .vmsd = &vmstate_virtio_64bit_features,
> +            .needed = &virtio_64bit_features_needed
> +        },
>          { 0 }
>      }
>  };
> @@ -925,6 +946,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    uint32_t guest_features_lo = (vdev->guest_features & 0xffffffff);
>      int i;
>  
>      if (k->save_config) {
> @@ -934,7 +956,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      qemu_put_8s(f, &vdev->status);
>      qemu_put_8s(f, &vdev->isr);
>      qemu_put_be16s(f, &vdev->queue_sel);
> -    qemu_put_be32s(f, &vdev->guest_features);
> +    qemu_put_be32s(f, &guest_features_lo);
>      qemu_put_be32(f, vdev->config_len);
>      qemu_put_buffer(f, vdev->config, vdev->config_len);
>  
> @@ -1011,11 +1033,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>      }
>      qemu_get_be32s(f, &features);
>  
> -    if (virtio_set_features(vdev, features) < 0) {
> -        error_report("Features 0x%x unsupported. Allowed features: 0x%x",
> -                     features, vdev->host_features);
> -        return -1;
> -    }
>      config_len = qemu_get_be32(f);
>  
>      /*
> @@ -1081,6 +1098,28 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>          vdev->device_endian = virtio_default_endian();
>      }
>  
> +    if (virtio_64bit_features_needed(vdev)) {
> +        /*
> +         * Subsection load filled vdev->guest_features.  Run them
> +         * through virtio_set_features to sanity-check them against
> +         * host_features.
> +         */
> +        uint64_t features64 = vdev->guest_features;
> +        if (virtio_set_features(vdev, features64) < 0) {
> +            error_report("Features 0x%" PRIx64 " unsupported. "
> +                         "Allowed features: 0x%" PRIx64,
> +                         features64, vdev->host_features);
> +            return -1;
> +        }
> +    } else {
> +        if (virtio_set_features(vdev, features) < 0) {
> +            error_report("Features 0x%x unsupported. "
> +                         "Allowed features: 0x%" PRIx64,
> +                         features, vdev->host_features);
> +            return -1;
> +        }
> +    }
> +
>      for (i = 0; i < num; i++) {
>          if (vdev->vq[i].pa) {
>              uint16_t nheads;
> @@ -1176,6 +1215,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>      vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
>                                                       vdev);
>      vdev->device_endian = virtio_default_endian();
> +    vdev->host_features = vdev->feature_flags;
>  }
>  
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
> @@ -1347,7 +1387,7 @@ static void virtio_device_unrealize(DeviceState *dev, Error **errp)
>  }
>  
>  static Property virtio_properties[] = {
> -    DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
> +    DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, feature_flags),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index b620da5..261a208 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -73,8 +73,9 @@ struct VirtIODevice
>      uint8_t status;
>      uint8_t isr;
>      uint16_t queue_sel;
> -    uint32_t guest_features;
> -    uint32_t host_features;
> +    uint64_t guest_features;
> +    uint64_t host_features;
> +    uint32_t feature_flags;
>      size_t config_len;
>      void *config;
>      uint16_t config_vector;
> @@ -96,8 +97,8 @@ typedef struct VirtioDeviceClass {
>      /* This is what a VirtioDevice must implement */
>      DeviceRealize realize;
>      DeviceUnrealize unrealize;
> -    uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_features);
> -    uint32_t (*bad_features)(VirtIODevice *vdev);
> +    uint64_t (*get_features)(VirtIODevice *vdev, uint64_t requested_features);
> +    uint64_t (*bad_features)(VirtIODevice *vdev);
>      void (*set_features)(VirtIODevice *vdev, uint32_t val);
>      void (*get_config)(VirtIODevice *vdev, uint8_t *config);
>      void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
> @@ -223,21 +224,21 @@ void virtio_irq(VirtQueue *vq);
>  VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
>  VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
>  
> -static inline void virtio_add_feature(uint32_t *features, unsigned int fbit)
> +static inline void virtio_add_feature(uint64_t *features, unsigned int fbit)
>  {
> -    assert(fbit < 32);
> +    assert(fbit < 64);
>      *features |= (1 << fbit);
>  }
>  
> -static inline void virtio_clear_feature(uint32_t *features, unsigned int fbit)
> +static inline void virtio_clear_feature(uint64_t *features, unsigned int fbit)
>  {
> -    assert(fbit < 32);
> +    assert(fbit < 64);
>      *features &= ~(1 << fbit);
>  }
>  
> -static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit)
> +static inline bool __virtio_has_feature(uint64_t features, unsigned int fbit)
>  {
> -    assert(fbit < 32);
> +    assert(fbit < 64);
>      return !!(features & (1 << fbit));
>  }
>  
> -- 
> 1.8.3.1