| Message ID | 1473430499-11581-1-git-send-email-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <cornelia.huck@de.ibm.com> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> 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, > ®ion->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
On 09/09/2016 05:39 PM, Michael S. Tsirkin wrote: > 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 <cornelia.huck@de.ibm.com> >> > Cc: Marcel Apfelbaum <marcel@redhat.com> >> > Cc: Michael S. Tsirkin <mst@redhat.com> >> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > Looks good - can you pls test and post a non-RFC? > Do you mean having this also in stable, or keeping it based on top of former pci-only patch? I already tested it for PCI, but cannot test it for CCW. Thanks, Maxime
On Fri, Sep 09, 2016 at 06:16:27PM +0200, Maxime Coquelin wrote: > > > On 09/09/2016 05:39 PM, Michael S. Tsirkin wrote: > > 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 <cornelia.huck@de.ibm.com> > > > > Cc: Marcel Apfelbaum <marcel@redhat.com> > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > Looks good - can you pls test and post a non-RFC? > > > > Do you mean having this also in stable, or keeping > it based on top of former pci-only patch? > > I already tested it for PCI, but cannot test it for CCW. > > Thanks, > Maxime Once you repost, ask for help testing.
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, ®ion->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. */
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 <cornelia.huck@de.ibm.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- 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(-)