| Message ID | 1473495817-29952-1-git-send-email-maxime.coquelin@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/09/2016 10:23, 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 CCW, for which 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 it needs to know whether the backend supports > VIRTIO_F_VERSION_1 at plug time. > > Currently, nothing is done for PCI. Modern capabilitities get > exposed to the guest even if VIRTIO_F_VERSION_1 is not supported > by the backend, which confuses the guest. > > This patch proposes to replace existing post_plugged solution > with an approach that fits with both transports. > Features negociation is performed before ->device_plugged() call. > A pre_plugged callback is introduced so that the transports can > set their supported features. Have you tested virtio-mmio too? Paolo
On 09/10/2016 11:49 AM, Paolo Bonzini wrote: > > > On 10/09/2016 10:23, 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 CCW, for which 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 it needs to know whether the backend supports >> VIRTIO_F_VERSION_1 at plug time. >> >> Currently, nothing is done for PCI. Modern capabilitities get >> exposed to the guest even if VIRTIO_F_VERSION_1 is not supported >> by the backend, which confuses the guest. >> >> This patch proposes to replace existing post_plugged solution >> with an approach that fits with both transports. >> Features negociation is performed before ->device_plugged() call. >> A pre_plugged callback is introduced so that the transports can >> set their supported features. > > Have you tested virtio-mmio too? No, not tested. But virtio-mmio doesn't have the device_plugged callback, so no impact here. Thanks, Maxime
On Sat, 10 Sep 2016 10:23:37 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> 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 CCW, for which 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 s/the// > complicated, so it needs to know whether the backend supports > VIRTIO_F_VERSION_1 at plug time. > > Currently, nothing is done for PCI. Modern capabilitities get > exposed to the guest even if VIRTIO_F_VERSION_1 is not supported > by the backend, which confuses the guest. > > This patch proposes to replace existing post_plugged solution Nit: The patch does not propose anything, it just does it :) > with an approach that fits with both transports. > Features negociation is performed before ->device_plugged() call. > A pre_plugged callback is introduced so that the transports can > set their supported features. With all those callbacks and so on, I think we should add some kind of diagram/description under doc/ that describes the order in which they are invoked and what elements of the virtio device the code can expect to be in a reasonable state already. Nothing that needs to go with this patch, but this is getting rather complex. > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: qemu-stable@nongnu.org > Acked-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > Hi, > > This patch replaces series "virtio-pci: Improve device plugging > whith legacy backends", as fixes the problem in a cleaner and > more generic way. Goal is to have it also in stable tree. I think this is fine for stable, with one comment below. > > Michael, I added your ack, as the changes compared to the RFC > are minor: > - Rebased on top of your pci branch > - Improve error hanling when modern no supported and legacy > disabled by the user > > I ran check-qtest, and tested PCI with vhost-user-bridge with > and without VERSION_1 enabled. > > What is missing is testing CCW, Cornelia, can you handle that? I can confirm that it continues to work with revision set to 1 or 0. I still need to test what happens with an old host kernel (anyone knows where vhost gained virtio-1 support? If not, I'll manage to find out.) > > Thanks, > Maxime > --- > hw/s390x/virtio-ccw.c | 30 +++++++++++++++--------------- > hw/virtio/virtio-bus.c | 9 +++++---- > hw/virtio/virtio-pci.c | 36 ++++++++++++++++++++++++++++++++---- > hw/virtio/virtio-pci.h | 5 +++++ > include/hw/virtio/virtio-bus.h | 10 +++++----- > 5 files changed, 62 insertions(+), 28 deletions(-) (...) > 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. > */ I'm not sure we want to rip out an interface in stable. I think the interface may have value in itself - but OTOH, its only user is now gone...
On 09/12/2016 10:51 AM, Cornelia Huck wrote: > On Sat, 10 Sep 2016 10:23:37 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> 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 CCW, for which 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 > > s/the// Right. >> complicated, so it needs to know whether the backend supports >> VIRTIO_F_VERSION_1 at plug time. >> >> Currently, nothing is done for PCI. Modern capabilitities get >> exposed to the guest even if VIRTIO_F_VERSION_1 is not supported >> by the backend, which confuses the guest. >> >> This patch proposes to replace existing post_plugged solution > > Nit: The patch does not propose anything, it just does it :) Of course! :) >> with an approach that fits with both transports. >> Features negociation is performed before ->device_plugged() call. >> A pre_plugged callback is introduced so that the transports can >> set their supported features. > > With all those callbacks and so on, I think we should add some kind of > diagram/description under doc/ that describes the order in which they > are invoked and what elements of the virtio device the code can expect > to be in a reasonable state already. Nothing that needs to go with this > patch, but this is getting rather complex. > >> >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Cc: qemu-stable@nongnu.org >> Acked-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> Hi, >> >> This patch replaces series "virtio-pci: Improve device plugging >> whith legacy backends", as fixes the problem in a cleaner and >> more generic way. Goal is to have it also in stable tree. > > I think this is fine for stable, with one comment below. > >> >> Michael, I added your ack, as the changes compared to the RFC >> are minor: >> - Rebased on top of your pci branch >> - Improve error hanling when modern no supported and legacy >> disabled by the user >> >> I ran check-qtest, and tested PCI with vhost-user-bridge with >> and without VERSION_1 enabled. >> >> What is missing is testing CCW, Cornelia, can you handle that? > > I can confirm that it continues to work with revision set to 1 or 0. I > still need to test what happens with an old host kernel (anyone knows > where vhost gained virtio-1 support? If not, I'll manage to find out.) Sorry, I don't know. But thanks for testing, I appreciate it. > >> >> Thanks, >> Maxime >> --- >> hw/s390x/virtio-ccw.c | 30 +++++++++++++++--------------- >> hw/virtio/virtio-bus.c | 9 +++++---- >> hw/virtio/virtio-pci.c | 36 ++++++++++++++++++++++++++++++++---- >> hw/virtio/virtio-pci.h | 5 +++++ >> include/hw/virtio/virtio-bus.h | 10 +++++----- >> 5 files changed, 62 insertions(+), 28 deletions(-) > > (...) > >> 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. >> */ > > I'm not sure we want to rip out an interface in stable. I think the > interface may have value in itself - but OTOH, its only user is now > gone... As it is now, with ->get_features() being called before ->device_plugged(), it has not much value because ->post_plugged() and ->device_plugged() are called in a row. But maybe calling ->get_features() a second time after ->device_plugged would make sense if for some reason a feature becomes (not) supported during ->device_plugged execution? Thanks, Maxime
On Mon, 12 Sep 2016 11:18:52 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > On 09/12/2016 10:51 AM, Cornelia Huck wrote: > > On Sat, 10 Sep 2016 10:23:37 +0200 > > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >> 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. > >> */ > > > > I'm not sure we want to rip out an interface in stable. I think the > > interface may have value in itself - but OTOH, its only user is now > > gone... > > As it is now, with ->get_features() being called before > ->device_plugged(), it has not much value because ->post_plugged() and > ->device_plugged() are called in a row. > > But maybe calling ->get_features() a second time after ->device_plugged > would make sense if for some reason a feature becomes (not) supported > during ->device_plugged execution? I was thinking more of changes that are not related to feature negotiation, but I'm not too attached to that callback. If noone objects against removing it in stable, let's just go with your patch.
On 09/12/2016 10:51 AM, Cornelia Huck wrote: > On Sat, 10 Sep 2016 10:23:37 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> 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 CCW, for which 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 > s/the// > >> > complicated, so it needs to know whether the backend supports >> > VIRTIO_F_VERSION_1 at plug time. >> > >> > Currently, nothing is done for PCI. Modern capabilitities get >> > exposed to the guest even if VIRTIO_F_VERSION_1 is not supported >> > by the backend, which confuses the guest. >> > >> > This patch proposes to replace existing post_plugged solution > Nit: The patch does not propose anything, it just does it :) > Michael, Should I send a v2 fixing the above comments, or you can handle them when applying the patch? Thanks, Maxime
On 09/10/2016 03:23 AM, 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 CCW, for which 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 it needs to know whether the backend supports > VIRTIO_F_VERSION_1 at plug time. > > Currently, nothing is done for PCI. Modern capabilitities get s/capabilitities/capabilities/ > exposed to the guest even if VIRTIO_F_VERSION_1 is not supported > by the backend, which confuses the guest. > > This patch proposes to replace existing post_plugged solution > with an approach that fits with both transports. > Features negociation is performed before ->device_plugged() call. s/negociation/negotiation/ > 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: qemu-stable@nongnu.org > Acked-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > ---
On Mon, Sep 12, 2016 at 08:22:50PM +0200, Maxime Coquelin wrote: > > > On 09/12/2016 10:51 AM, Cornelia Huck wrote: > > On Sat, 10 Sep 2016 10:23:37 +0200 > > Maxime Coquelin <maxime.coquelin@redhat.com> 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 CCW, for which 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 > > s/the// > > > > > > complicated, so it needs to know whether the backend supports > > > > VIRTIO_F_VERSION_1 at plug time. > > > > > > > > Currently, nothing is done for PCI. Modern capabilitities get > > > > exposed to the guest even if VIRTIO_F_VERSION_1 is not supported > > > > by the backend, which confuses the guest. > > > > > > > > This patch proposes to replace existing post_plugged solution > > Nit: The patch does not propose anything, it just does it :) > > > Michael, > > Should I send a v2 fixing the above comments, or you can handle them > when applying the patch? > > Thanks, > Maxime It's easier if you post v2 including all acks. Note - after --- that no code was changed.
On 09/12/2016 08:41 PM, Eric Blake wrote: > On 09/10/2016 03:23 AM, 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 CCW, for which 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 it needs to know whether the backend supports >> VIRTIO_F_VERSION_1 at plug time. >> >> Currently, nothing is done for PCI. Modern capabilitities get > > s/capabilitities/capabilities/ > >> exposed to the guest even if VIRTIO_F_VERSION_1 is not supported >> by the backend, which confuses the guest. >> >> This patch proposes to replace existing post_plugged solution >> with an approach that fits with both transports. >> Features negociation is performed before ->device_plugged() call. > > s/negociation/negotiation/ Thanks Eric, it will be fixed in next version. Maxime
On 09/12/2016 09:58 PM, Michael S. Tsirkin wrote: > On Mon, Sep 12, 2016 at 08:22:50PM +0200, Maxime Coquelin wrote: >> >> >> On 09/12/2016 10:51 AM, Cornelia Huck wrote: >>> On Sat, 10 Sep 2016 10:23:37 +0200 >>> Maxime Coquelin <maxime.coquelin@redhat.com> 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 CCW, for which 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 >>> s/the// >>> >>>>> complicated, so it needs to know whether the backend supports >>>>> VIRTIO_F_VERSION_1 at plug time. >>>>> >>>>> Currently, nothing is done for PCI. Modern capabilitities get >>>>> exposed to the guest even if VIRTIO_F_VERSION_1 is not supported >>>>> by the backend, which confuses the guest. >>>>> >>>>> This patch proposes to replace existing post_plugged solution >>> Nit: The patch does not propose anything, it just does it :) >>> >> Michael, >> >> Should I send a v2 fixing the above comments, or you can handle them >> when applying the patch? >> >> Thanks, >> Maxime > > > It's easier if you post v2 including all acks. Ok, v2 is ready, waiting to collect some acks. > Note - after --- that no code was changed. Sorry, I thought the change was minor enough (as I mentioned in commit comment). I won't add acks next times if code is changed. For the v2, should I keep your Ack or remove it? Thanks, Maxime
On 09/10/2016 11:23 AM, 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 CCW, for which 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 it needs to know whether the backend supports > VIRTIO_F_VERSION_1 at plug time. > > Currently, nothing is done for PCI. Modern capabilitities get > exposed to the guest even if VIRTIO_F_VERSION_1 is not supported > by the backend, which confuses the guest. > > This patch proposes to replace existing post_plugged solution > with an approach that fits with both transports. > Features negociation is performed before ->device_plugged() call. > 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: qemu-stable@nongnu.org > Acked-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > Hi, > > This patch replaces series "virtio-pci: Improve device plugging > whith legacy backends", as fixes the problem in a cleaner and > more generic way. Goal is to have it also in stable tree. > > Michael, I added your ack, as the changes compared to the RFC > are minor: > - Rebased on top of your pci branch > - Improve error hanling when modern no supported and legacy > disabled by the user > > I ran check-qtest, and tested PCI with vhost-user-bridge with > and without VERSION_1 enabled. > > What is missing is testing CCW, Cornelia, can you handle that? > > Thanks, > Maxime > --- > hw/s390x/virtio-ccw.c | 30 +++++++++++++++--------------- > hw/virtio/virtio-bus.c | 9 +++++---- > hw/virtio/virtio-pci.c | 36 ++++++++++++++++++++++++++++++++---- > hw/virtio/virtio-pci.h | 5 +++++ > include/hw/virtio/virtio-bus.h | 10 +++++----- > 5 files changed, 62 insertions(+), 28 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 9678956..9f3f386 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 1492793..11f65bd 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 dde71a5..2d60a00 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1576,18 +1576,48 @@ 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) > { > VirtIOPCIProxy *proxy = VIRTIO_PCI(d); > VirtioBusState *bus = &proxy->bus; > bool legacy = virtio_pci_legacy(proxy); > - bool modern = virtio_pci_modern(proxy); > + bool 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); > > + /* > + * Virtio capabilities present without > + * VIRTIO_F_VERSION_1 confuses guests > + */ > + if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { > + virtio_pci_disable_modern(proxy); > + > + if (!legacy) { > + error_setg(errp, "Device doesn't support modern mode, and legacy" > + " mode is disabled"); > + error_append_hint(errp, "Set disable-legacy to off\n"); > + > + return; > + } > + } > + > + modern = virtio_pci_modern(proxy); > + > config = proxy->pci_dev.config; > if (proxy->class_code) { > pci_config_set_class(config, proxy->class_code); > @@ -1629,7 +1659,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); > @@ -1694,8 +1723,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) > @@ -2508,6 +2535,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/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index 0698157..541cbdb 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -181,6 +181,11 @@ static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) > proxy->disable_legacy = ON_OFF_AUTO_ON; > } > > +static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy) > +{ > + proxy->disable_modern = true; > +} > + > /* > * virtio-scsi-pci: This extends VirtioPCIProxy. > */ > 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. > */ > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel
On Tue, 13 Sep 2016 09:08:04 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > On 09/12/2016 09:58 PM, Michael S. Tsirkin wrote: > > On Mon, Sep 12, 2016 at 08:22:50PM +0200, Maxime Coquelin wrote: > >> > >> > >> On 09/12/2016 10:51 AM, Cornelia Huck wrote: > >>> On Sat, 10 Sep 2016 10:23:37 +0200 > >>> Maxime Coquelin <maxime.coquelin@redhat.com> 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 CCW, for which 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 > >>> s/the// > >>> > >>>>> complicated, so it needs to know whether the backend supports > >>>>> VIRTIO_F_VERSION_1 at plug time. > >>>>> > >>>>> Currently, nothing is done for PCI. Modern capabilitities get > >>>>> exposed to the guest even if VIRTIO_F_VERSION_1 is not supported > >>>>> by the backend, which confuses the guest. > >>>>> > >>>>> This patch proposes to replace existing post_plugged solution > >>> Nit: The patch does not propose anything, it just does it :) > >>> > >> Michael, > >> > >> Should I send a v2 fixing the above comments, or you can handle them > >> when applying the patch? > >> > >> Thanks, > >> Maxime > > > > > > It's easier if you post v2 including all acks. > Ok, v2 is ready, waiting to collect some acks. In the meanwhile, I've verified that everything works as expected as well with an old host kernel (3.18) and your patch for ccw. Therefore, Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> [ccw] Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
On 09/13/2016 10:59 AM, Cornelia Huck wrote: > On Tue, 13 Sep 2016 09:08:04 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >> On 09/12/2016 09:58 PM, Michael S. Tsirkin wrote: >>> On Mon, Sep 12, 2016 at 08:22:50PM +0200, Maxime Coquelin wrote: >>>> >>>> >>>> On 09/12/2016 10:51 AM, Cornelia Huck wrote: >>>>> On Sat, 10 Sep 2016 10:23:37 +0200 >>>>> Maxime Coquelin <maxime.coquelin@redhat.com> 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 CCW, for which 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 >>>>> s/the// >>>>> >>>>>>> complicated, so it needs to know whether the backend supports >>>>>>> VIRTIO_F_VERSION_1 at plug time. >>>>>>> >>>>>>> Currently, nothing is done for PCI. Modern capabilitities get >>>>>>> exposed to the guest even if VIRTIO_F_VERSION_1 is not supported >>>>>>> by the backend, which confuses the guest. >>>>>>> >>>>>>> This patch proposes to replace existing post_plugged solution >>>>> Nit: The patch does not propose anything, it just does it :) >>>>> >>>> Michael, >>>> >>>> Should I send a v2 fixing the above comments, or you can handle them >>>> when applying the patch? >>>> >>>> Thanks, >>>> Maxime >>> >>> >>> It's easier if you post v2 including all acks. >> Ok, v2 is ready, waiting to collect some acks. > > In the meanwhile, I've verified that everything works as expected as > well with an old host kernel (3.18) and your patch for ccw. Therefore, > > Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> [ccw] > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> Thanks for the time spent for testing. Maxime
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 9678956..9f3f386 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 1492793..11f65bd 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 dde71a5..2d60a00 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1576,18 +1576,48 @@ 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) { VirtIOPCIProxy *proxy = VIRTIO_PCI(d); VirtioBusState *bus = &proxy->bus; bool legacy = virtio_pci_legacy(proxy); - bool modern = virtio_pci_modern(proxy); + bool 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); + /* + * Virtio capabilities present without + * VIRTIO_F_VERSION_1 confuses guests + */ + if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) { + virtio_pci_disable_modern(proxy); + + if (!legacy) { + error_setg(errp, "Device doesn't support modern mode, and legacy" + " mode is disabled"); + error_append_hint(errp, "Set disable-legacy to off\n"); + + return; + } + } + + modern = virtio_pci_modern(proxy); + config = proxy->pci_dev.config; if (proxy->class_code) { pci_config_set_class(config, proxy->class_code); @@ -1629,7 +1659,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); @@ -1694,8 +1723,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) @@ -2508,6 +2535,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/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 0698157..541cbdb 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -181,6 +181,11 @@ static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) proxy->disable_legacy = ON_OFF_AUTO_ON; } +static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy) +{ + proxy->disable_modern = true; +} + /* * virtio-scsi-pci: This extends VirtioPCIProxy. */ 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. */