Re: [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated

[Michael S. Tsirkin]

On Fri, Sep 09, 2016 at 04:14:59PM +0200, Maxime Coquelin wrote:
> Currently, devices are plugged before features are negotiated.
> If the backend doesn't support VIRTIO_F_VERSION_1, the transport
> need to rewind some settings.
> 
> This is the case for both PCI and CCW.
> For CCW, a post_plugged callback had been introduced, where
> max_rev field is just updated if VIRTIO_F_VERSION_1 is not
> supported by the backend.
> For PCI, implementing the post_plugged would be much more
> complicated, so the current fix consists in checking whether
> the backend supports VIRTIO_F_VERSION_1 in the backend.
> 
> This patch propose to replace existing solutions with a common
> approach by negociating features before ->device_plugged() is
> called. A pre_plugged callback is introduced so that the
> transports can set their supported features.
> 
> Cc: Cornelia Huck <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Maxime Coquelin <address@hidden>


Acked-by: Michael S. Tsirkin <address@hidden>

Looks good - can you pls test and post a non-RFC?

> ---
> Cornelia,
> 
> This is what I proposed earlier today.
> 
> I have tested it successfully with PCI.
> Does it sound good to you? Would you have a try with CCW?
> 
> Note that this is based on my earlier series which fixes PCI.
> 
> Thanks,
> Maxime
> ---
>  hw/s390x/virtio-ccw.c          | 30 +++++++++++++++---------------
>  hw/virtio/virtio-bus.c         |  9 +++++----
>  hw/virtio/virtio-pci.c         | 18 ++++++++++++++----
>  include/hw/virtio/virtio-bus.h | 10 +++++-----
>  4 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index a554a24..e10a88d 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1261,6 +1261,16 @@ static int virtio_ccw_load_config(DeviceState *d, 
> QEMUFile *f)
>      return 0;
>  }
>  
> +static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> +{
> +   VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> +   VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> +
> +    if (dev->max_rev >= 1) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> +    }
> +}
> +
>  /* This is called by virtio-bus just after the device is plugged. */
>  static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>  {
> @@ -1270,6 +1280,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, 
> Error **errp)
>      SubchDev *sch = ccw_dev->sch;
>      int n = virtio_get_num_queues(vdev);
>  
> +    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
> +        dev->max_rev = 0;
> +    }
> +
>      if (virtio_get_num_queues(vdev) > VIRTIO_CCW_QUEUE_MAX) {
>          error_setg(errp, "The number of virtqueues %d "
>                     "exceeds ccw limit %d", n,
> @@ -1283,25 +1297,11 @@ 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);
>  }
>  
> -static void virtio_ccw_post_plugged(DeviceState *d, Error **errp)
> -{
> -   VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> -   VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> -
> -   if (!virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> -        /* A backend didn't support modern virtio. */
> -       dev->max_rev = 0;
> -   }
> -}
> -
>  static void virtio_ccw_device_unplugged(DeviceState *d)
>  {
>      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> @@ -1593,8 +1593,8 @@ static void virtio_ccw_bus_class_init(ObjectClass 
> *klass, void *data)
>      k->load_queue = virtio_ccw_load_queue;
>      k->save_config = virtio_ccw_save_config;
>      k->load_config = virtio_ccw_load_config;
> +    k->pre_plugged = virtio_ccw_pre_plugged;
>      k->device_plugged = virtio_ccw_device_plugged;
> -    k->post_plugged = virtio_ccw_post_plugged;
>      k->device_unplugged = virtio_ccw_device_unplugged;
>      k->ioeventfd_started = virtio_ccw_ioeventfd_started;
>      k->ioeventfd_set_started = virtio_ccw_ioeventfd_set_started;
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index a85b7c8..9d2111f 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -49,16 +49,17 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error 
> **errp)
>  
>      DPRINTF("%s: plug device.\n", qbus->name);
>  
> -    if (klass->device_plugged != NULL) {
> -        klass->device_plugged(qbus->parent, errp);
> +    if (klass->pre_plugged != NULL) {
> +        klass->pre_plugged(qbus->parent, errp);
>      }
>  
>      /* Get the features of the plugged device. */
>      assert(vdc->get_features != NULL);
>      vdev->host_features = vdc->get_features(vdev, vdev->host_features,
>                                              errp);
> -    if (klass->post_plugged != NULL) {
> -        klass->post_plugged(qbus->parent, errp);
> +
> +    if (klass->device_plugged != NULL) {
> +        klass->device_plugged(qbus->parent, errp);
>      }
>  }
>  
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 9e88d7b..07e6c60 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1569,6 +1569,18 @@ static void 
> virtio_pci_modern_io_region_unmap(VirtIOPCIProxy *proxy,
>                                  &region->mr);
>  }
>  
> +static void virtio_pci_pre_plugged(DeviceState *d, Error **errp)
> +{
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +
> +    if (virtio_pci_modern(proxy)) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> +    }
> +
> +    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
> +}
> +
>  /* This is called by virtio-bus just after the device is plugged. */
>  static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  {
> @@ -1585,7 +1597,7 @@ static void virtio_pci_device_plugged(DeviceState *d, 
> Error **errp)
>       * Virtio capabilities present without
>       * VIRTIO_F_VERSION_1 confuses guests
>       */
> -    if (!virtio_test_backend_virtio_1(vdev, errp)) {
> +    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
>          virtio_pci_disable_modern(proxy);
>      }
>  
> @@ -1637,7 +1649,6 @@ static void virtio_pci_device_plugged(DeviceState *d, 
> Error **errp)
>  
>          struct virtio_pci_cfg_cap *cfg_mask;
>  
> -        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
>          virtio_pci_modern_regions_init(proxy);
>  
>          virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
> @@ -1702,8 +1713,6 @@ static void virtio_pci_device_plugged(DeviceState *d, 
> Error **errp)
>      if (!kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>      }
> -
> -    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
>  }
>  
>  static void virtio_pci_device_unplugged(DeviceState *d)
> @@ -2459,6 +2468,7 @@ static void virtio_pci_bus_class_init(ObjectClass 
> *klass, void *data)
>      k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
>      k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
>      k->vmstate_change = virtio_pci_vmstate_change;
> +    k->pre_plugged = virtio_pci_pre_plugged;
>      k->device_plugged = virtio_pci_device_plugged;
>      k->device_unplugged = virtio_pci_device_unplugged;
>      k->query_nvectors = virtio_pci_query_nvectors;
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index f3e5ef3..24caa0a 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -54,16 +54,16 @@ typedef struct VirtioBusClass {
>      int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
>      void (*vmstate_change)(DeviceState *d, bool running);
>      /*
> +     * Expose the features the transport layer supports before
> +     * the negotiation takes place.
> +     */
> +    void (*pre_plugged)(DeviceState *d, Error **errp);
> +    /*
>       * transport independent init function.
>       * This is called by virtio-bus just after the device is plugged.
>       */
>      void (*device_plugged)(DeviceState *d, Error **errp);
>      /*
> -     * Re-evaluate setup after feature bits have been validated
> -     * by the device backend.
> -     */
> -    void (*post_plugged)(DeviceState *d, Error **errp);
> -    /*
>       * transport independent exit function.
>       * This is called by virtio-bus just before the device is unplugged.
>       */
> -- 
> 2.7.4