| Message ID | 1441356869-57861-5-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <cornelia.huck@de.ibm.com> > --- > 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
On Thu, 10 Sep 2015 12:07:18 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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. Huh? The first hunk of this patch does this. > > > - 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. I don't like the pci concept: much too coarse-grained and not very future proof. > > Revision numbers generally are a ccw specific concept. I'm not sure it > is wise to expose it to users. What is wrong about exposing transport-specific concepts? > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > 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 >
On Thu, Sep 10, 2015 at 11:11:20AM +0200, Cornelia Huck wrote: > On Thu, 10 Sep 2015 12:07:18 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > 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. > > Huh? The first hunk of this patch does this. Sure. Donnu how I could miss it, sorry. > > > > > - 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. > > I don't like the pci concept: much too coarse-grained and not very > future proof. > > > > > Revision numbers generally are a ccw specific concept. I'm not sure it > > is wise to expose it to users. > > What is wrong about exposing transport-specific concepts? Nothing. Go ahead and expose as much of the low level as makes sense. But it would be nice if there was also a portable way for people that just want "virtio" and don't care about the low level details of which transport it is. OTOH a conservative estimate of the # of people that will want to play with this is pretty close to 0, so maybe it does not matter much. > > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > > --- > > > 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 > >
On Thu, 10 Sep 2015 12:22:42 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Sep 10, 2015 at 11:11:20AM +0200, Cornelia Huck wrote: > > On Thu, 10 Sep 2015 12:07:18 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > 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. > > > > Huh? The first hunk of this patch does this. > > Sure. Donnu how I could miss it, sorry. > > > > > > > > - 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. > > > > I don't like the pci concept: much too coarse-grained and not very > > future proof. > > > > > > > > Revision numbers generally are a ccw specific concept. I'm not sure it > > > is wise to expose it to users. > > > > What is wrong about exposing transport-specific concepts? > > Nothing. Go ahead and expose as much of the low level as > makes sense. > > But it would be nice if there was also a portable way for people > that just want "virtio" and don't care about the low level details > of which transport it is. Proxy devices already look different depending on what transport you use, so I don't think it really matters. (And for most users, I'd expect they let a management layer take care of it anyway.) > > OTOH a conservative estimate of the # of people that will want > to play with this is pretty close to 0, so maybe it does not matter > much. The scsi vs. virtio-1 on ccw handling? Not very many (if anyone), yes. > > > > > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > > > --- > > > > 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(-)
On Fri, 4 Sep 2015 10:54:29 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> 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) > - 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) > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > 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(-) Scratch this; stale patch with broken compat handling. I'll send a v2 of the whole series.
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 */
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) - 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) Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> --- 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(-)