Re: [Qemu-devel] [PATCH for-2.5] virtio: handle non-virtio-1-capable backend

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

On Wed, Dec 02, 2015 at 03:09:58PM +0100, Cornelia Huck wrote:
> If you run a qemu advertising VERSION_1 with an old kernel where
> vhost did not yet support VERSION_1, you'll end up with a device
> that is {modern pci|ccw revision 1} but does not advertise VERSION_1.
> This is not a sensible configuration and is rejected by the Linux
> guest drivers.
> 
> To fix this, add a ->post_plugged() callback invoked after features
> have been queried that can handle the VERSION_1 bit being withdrawn
> and change pci (only setup modern if VERSION_1 is still present) and
> ccw (fall back to revision 0 if VERSION_1 is gone).
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>


Unfortunately this is too late: we need to know
whether modern will be supported to know whether
to add the pci express capability :(

Maybe this should be moved into plugged
callback?

> ---
> Michael: I think this should go into 2.5, as otherwise ccw (which
> defaults to virtio-1 with 2.5) will be broken with old host kernels.
> I need someone to give virtio-pci some testing, as I do not have
> a proper setup for that. ccw is working fine.
> 
> Changes from initial writeup:
> - Move pci-modern init to post_plugged instead of undoing [Jason]
> - Add a proper patch description + signoff
> - Add some comments
> ---
>  hw/s390x/virtio-ccw.c          | 12 ++++++
>  hw/virtio/virtio-bus.c         |  3 ++
>  hw/virtio/virtio-pci.c         | 96 ++++++++++++++++++++++++++----------------
>  include/hw/virtio/virtio-bus.h |  5 +++
>  4 files changed, 80 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index fb103b7..63da303 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1555,6 +1555,17 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>                            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);
> @@ -1891,6 +1902,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
>      k->save_config = virtio_ccw_save_config;
>      k->load_config = virtio_ccw_load_config;
>      k->device_plugged = virtio_ccw_device_plugged;
> +    k->post_plugged = virtio_ccw_post_plugged;
>      k->device_unplugged = virtio_ccw_device_unplugged;
>  }
>  
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index febda76..81c7cdd 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -56,6 +56,9 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>      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);
> +    }
>  }
>  
>  /* Reset the virtio_bus */
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index dd48562..af59496 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1626,7 +1626,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>      VirtioBusState *bus = &proxy->bus;
>      bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
>      bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
> -    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
>      uint8_t *config;
>      uint32_t size;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> @@ -1653,6 +1652,65 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  
>  
>      if (modern) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> +        /* Let's see if this feature bit sticks before doing further init. */
> +    }
> +
> +    if (proxy->nvectors) {
> +        int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
> +                                          proxy->msix_bar);
> +        if (err) {
> +            /* Notice when a system that supports MSIx can't initialize it.  */
> +            if (err != -ENOTSUP) {
> +                error_report("unable to init msix vectors to %" PRIu32,
> +                             proxy->nvectors);
> +            }
> +            proxy->nvectors = 0;
> +        }
> +    }
> +
> +    proxy->pci_dev.config_write = virtio_write_config;
> +    proxy->pci_dev.config_read = virtio_read_config;
> +
> +    if (legacy) {
> +        size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> +            + virtio_bus_get_vdev_config_len(bus);
> +        size = pow2ceil(size);
> +
> +        memory_region_init_io(&proxy->bar, OBJECT(proxy),
> +                              &virtio_pci_config_ops,
> +                              proxy, "virtio-pci", size);
> +
> +        pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar,
> +                         PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar);
> +    }
> +
> +    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_post_plugged(DeviceState *d, Error **errp)
> +{
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> +    bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
> +    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
> +    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +
> +    if (modern && !virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        /* A backend didn't support modern virtio. */
> +        if (legacy) {
> +            proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN;
> +            modern = false;
> +        } else {
> +            error_setg(errp, "can't fall back to legacy virtio");
> +            return;
> +        }
> +    }
> +    if (modern) {
>          struct virtio_pci_cap cap = {
>              .cap_len = sizeof cap,
>          };
> @@ -1672,7 +1730,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);
> @@ -1705,40 +1762,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>          pci_set_long(cfg_mask->pci_cfg_data, ~0x0);
>      }
>  
> -    if (proxy->nvectors) {
> -        int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
> -                                          proxy->msix_bar);
> -        if (err) {
> -            /* Notice when a system that supports MSIx can't initialize it.  */
> -            if (err != -ENOTSUP) {
> -                error_report("unable to init msix vectors to %" PRIu32,
> -                             proxy->nvectors);
> -            }
> -            proxy->nvectors = 0;
> -        }
> -    }
> -
> -    proxy->pci_dev.config_write = virtio_write_config;
> -    proxy->pci_dev.config_read = virtio_read_config;
> -
> -    if (legacy) {
> -        size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> -            + virtio_bus_get_vdev_config_len(bus);
> -        size = pow2ceil(size);
> -
> -        memory_region_init_io(&proxy->bar, OBJECT(proxy),
> -                              &virtio_pci_config_ops,
> -                              proxy, "virtio-pci", size);
> -
> -        pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar,
> -                         PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar);
> -    }
> -
> -    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)
> @@ -2474,6 +2497,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
>      k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
>      k->vmstate_change = virtio_pci_vmstate_change;
>      k->device_plugged = virtio_pci_device_plugged;
> +    k->post_plugged = virtio_pci_post_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 6c3d4cb..3f2c136 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -60,6 +60,11 @@ typedef struct VirtioBusClass {
>       */
>      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.3.9