Re: [Qemu-devel] [PATCH 4/4] virtio-ccw: enable virtio-1

[Michael S. Tsirkin]

On Fri, Sep 04, 2015 at 10:54:29AM +0200, Cornelia Huck wrote:
> Let's enable revision 1 for virtio-ccw devices. We can always offer
> VERSION_1 as drivers in legacy mode won't be able to see it anyway.
> 
> We have to introduce a way to set a lower maximum revision for a device
> to accommodate the following cases:
> - compat machines (to enforce legacy only)

But you don't actually set this for any compat machines.
If you don't, this seems a bit pointless.

> - virtio-blk with scsi support (version 1 + scsi is fenced by common
>   code, with a user-configured max revision of 0 we can allow scsi
>   via not offering VERSION_1)

For this use, for pci users need to do disable_modern=true.
I find it unfortunate that for ccw one needs to do max_revision=0.

Revision numbers generally are a ccw specific concept. I'm not sure it
is wise to expose it to users.

> Signed-off-by: Cornelia Huck <address@hidden>
> ---
>  hw/s390x/s390-virtio-ccw.c |  4 ++++
>  hw/s390x/virtio-ccw.c      | 30 ++++++++++++++++++++++++++++--
>  hw/s390x/virtio-ccw.h      |  6 ++++--
>  3 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e2a26e9..fb6e45c 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -287,6 +287,10 @@ static const TypeInfo ccw_machine_info = {
>              .driver   = TYPE_S390_SKEYS,\
>              .property = "migration-enabled",\
>              .value    = "off",\
> +        },{\
> +            .driver = TYPE_VIRTIO_CCW_DEVICE,\
> +            .property = "max_revision",\
> +            .value = 0,\
>          },
>  
>  static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index eed7b3e..fb103b7 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -467,7 +467,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                                                  MEMTXATTRS_UNSPECIFIED,
>                                                  NULL);
>              if (features.index == 0) {
> -                features.features = (uint32_t)vdev->host_features;
> +                if (dev->revision >= 1) {
> +                    /* Don't offer legacy features for modern devices. */
> +                    features.features = (uint32_t)
> +                        (vdev->host_features & ~VIRTIO_LEGACY_FEATURES);
> +                } else {
> +                    features.features = (uint32_t)vdev->host_features;
> +                }
>              } else if ((features.index == 1) && (dev->revision >= 1)) {
>                  /*
>                   * Only offer feature bits beyond 31 if the guest has
> @@ -768,7 +774,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           * need to fetch it here. Nothing to do for now, though.
>           */
>          if (dev->revision >= 0 ||
> -            revinfo.revision > virtio_ccw_rev_max(vdev)) {
> +            revinfo.revision > virtio_ccw_rev_max(dev)) {
>              ret = -ENOSYS;
>              break;
>          }
> @@ -1541,6 +1547,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, 
> Error **errp)
>  
>      sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
>  
> +    if (dev->max_rev >= 1) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> +    }
> +
>      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                            d->hotplugged, 1);
>  }
> @@ -1557,6 +1567,8 @@ static Property virtio_ccw_net_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1584,6 +1596,8 @@ static Property virtio_ccw_blk_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1611,6 +1625,8 @@ static Property virtio_ccw_serial_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1638,6 +1654,8 @@ static Property virtio_ccw_balloon_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1665,6 +1683,8 @@ static Property virtio_ccw_scsi_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1691,6 +1711,8 @@ static const TypeInfo virtio_ccw_scsi = {
>  #ifdef CONFIG_VHOST_SCSI
>  static Property vhost_ccw_scsi_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1729,6 +1751,8 @@ static Property virtio_ccw_rng_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1882,6 +1906,8 @@ static Property virtio_ccw_9p_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>              VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> index 692ddd7..7ab8367 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -88,6 +88,7 @@ struct VirtioCcwDevice {
>      SubchDev *sch;
>      char *bus_id;
>      int revision;
> +    uint32_t max_rev;
>      VirtioBusState bus;
>      bool ioeventfd_started;
>      bool ioeventfd_disabled;
> @@ -102,9 +103,10 @@ struct VirtioCcwDevice {
>  };
>  
>  /* The maximum virtio revision we support. */
> -static inline int virtio_ccw_rev_max(VirtIODevice *vdev)
> +#define VIRTIO_CCW_MAX_REV 1
> +static inline int virtio_ccw_rev_max(VirtioCcwDevice *dev)
>  {
> -    return 0;
> +    return dev->max_rev;
>  }
>  
>  /* virtual css bus type */
> -- 
> 2.3.8