Linux Virtualization - Jan 2015 - [PATCH RFC v6 18/20] virtio: support revision-specific features

On Thu, Dec 11, 2014 at 02:25:20PM +0100, Cornelia Huck
wrote:
> Devices may support different sets of feature bits depending on which
> revision they're operating at. Let's give the transport a way to
> re-query the device about its features when the revision has been
> changed.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
So now we have both get_features and get_features_rev, and
it's never clear which revision does host_features refer to.
IMHO that's just too messy.
Let's add get_legacy_features and host_legacy_features instead?

> ---
>  hw/s390x/virtio-ccw.c          |   12 ++++++++++--
>  hw/virtio/virtio-bus.c         |   14 ++++++++++++--
>  include/hw/virtio/virtio-bus.h |    3 +++
>  include/hw/virtio/virtio.h     |    3 +++
>  4 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index a55e851..8b6b2ab 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -699,6 +699,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>          }
>          ret = 0;
>          dev->revision = revinfo.revision;
> +        /* Re-evaluate which features the device wants to offer. */
> +        dev->host_features > +           
virtio_bus_get_vdev_features_rev(&dev->bus, dev->host_features,
> +                                             dev->revision >= 1 ? 1
: 0);
>          break;
>      default:
>          ret = -ENOSYS;
> @@ -712,6 +716,9 @@ static void virtio_sch_disable_cb(SubchDev *sch)
>      VirtioCcwDevice *dev = sch->driver_data;
>  
>      dev->revision = -1;
> +    /* Reset the device's features to legacy. */
> +    dev->host_features > +       
virtio_bus_get_vdev_features_rev(&dev->bus, dev->host_features, 0);
>  }
>  
>  static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice
*vdev)
> @@ -853,8 +860,9 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev,
VirtIODevice *vdev)
>      virtio_add_feature(&dev->host_features,
VIRTIO_F_NOTIFY_ON_EMPTY);
>      virtio_add_feature(&dev->host_features, VIRTIO_F_BAD_FEATURE);
>  
> -    dev->host_features = virtio_bus_get_vdev_features(&dev->bus,
> -                                                     
dev->host_features);
> +    /* All devices start in legacy mode. */
> +    dev->host_features > +       
virtio_bus_get_vdev_features_rev(&dev->bus, dev->host_features, 0);
>  
>      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                            parent->hotplugged, 1);
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 32e3fab..a30826c 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -97,18 +97,28 @@ size_t virtio_bus_get_vdev_config_len(VirtioBusState
*bus)
>  }
>  
>  /* Get the features of the plugged device. */
> -uint64_t virtio_bus_get_vdev_features(VirtioBusState *bus,
> -                                      uint64_t requested_features)
> +uint64_t virtio_bus_get_vdev_features_rev(VirtioBusState *bus,
> +                                          uint64_t requested_features,
> +                                          unsigned int revision)
>  {
>      VirtIODevice *vdev = virtio_bus_get_device(bus);
>      VirtioDeviceClass *k;
>  
>      assert(vdev != NULL);
>      k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    if (revision > 0 && k->get_features_rev) {
> +        return k->get_features_rev(vdev, requested_features, revision);
> +    }
>      assert(k->get_features != NULL);
>      return k->get_features(vdev, requested_features);
>  }
>  
> +uint64_t virtio_bus_get_vdev_features(VirtioBusState *bus,
> +                                      uint64_t requested_features)
> +{
> +    return virtio_bus_get_vdev_features_rev(bus, requested_features, 0);
> +}
> +
>  /* Get bad features of the plugged device. */
>  uint64_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus)
>  {
> diff --git a/include/hw/virtio/virtio-bus.h
b/include/hw/virtio/virtio-bus.h
> index 0a4dde1..f0916ef 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -84,6 +84,9 @@ size_t virtio_bus_get_vdev_config_len(VirtioBusState
*bus);
>  /* Get the features of the plugged device. */
>  uint64_t virtio_bus_get_vdev_features(VirtioBusState *bus,
>                                        uint64_t requested_features);
> +uint64_t virtio_bus_get_vdev_features_rev(VirtioBusState *bus,
> +                                          uint64_t requested_features,
> +                                          unsigned int revision);
>  /* Get bad features of the plugged device. */
>  uint64_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus);
>  /* Get config of the plugged device. */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 068211e..1338540 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -147,6 +147,9 @@ typedef struct VirtioDeviceClass {
>      DeviceRealize realize;
>      DeviceUnrealize unrealize;
>      uint64_t (*get_features)(VirtIODevice *vdev, uint64_t
requested_features);
> +    uint64_t (*get_features_rev)(VirtIODevice *vdev,
> +                                 uint64_t requested_features,
> +                                 unsigned int revision);
>      uint64_t (*bad_features)(VirtIODevice *vdev);
>      void (*set_features)(VirtIODevice *vdev, uint64_t val);
>      int (*validate_features)(VirtIODevice *vdev);
> -- 
> 1.7.9.5

On Sun, 28 Dec 2014 10:32:06 +0200
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> On Thu, Dec 11, 2014 at 02:25:20PM +0100, Cornelia Huck wrote:
> > Devices may support different sets of feature bits depending on which
> > revision they're operating at. Let's give the transport a way
to
> > re-query the device about its features when the revision has been
> > changed.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> 
> So now we have both get_features and get_features_rev, and
> it's never clear which revision does host_features refer to.
> IMHO that's just too messy.
> Let's add get_legacy_features and host_legacy_features instead?
I wanted to avoid touching anything that does not support version 1.
And this interface might still work for later revisions, no?

On Wed, Jan 07, 2015 at 05:22:32PM +0100, Cornelia Huck
wrote:
> On Sun, 28 Dec 2014 10:32:06 +0200
> "Michael S. Tsirkin" <mst at redhat.com> wrote:
> 
> > On Thu, Dec 11, 2014 at 02:25:20PM +0100, Cornelia Huck wrote:
> > > Devices may support different sets of feature bits depending on
which
> > > revision they're operating at. Let's give the transport a
way to
> > > re-query the device about its features when the revision has been
> > > changed.
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> > 
> > So now we have both get_features and get_features_rev, and
> > it's never clear which revision does host_features refer to.
> > IMHO that's just too messy.
> > Let's add get_legacy_features and host_legacy_features instead?
> 
> I wanted to avoid touching anything that does not support version 1.
> And this interface might still work for later revisions, no?
We can add _modern_ then, or rename host_features to host_legacy_features
everywhere as preparation.

-- 
MST