Re: [Qemu-stable] [PATCH] virtio-bus: Plug devices after features are negotiated

[Maxime Coquelin]

On 09/12/2016 10:51 AM, Cornelia Huck wrote:
> On Sat, 10 Sep 2016 10:23:37 +0200
> Maxime Coquelin <address@hidden> 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 <address@hidden>
> > Cc: Marcel Apfelbaum <address@hidden>
> > Cc: address@hidden
> > Acked-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Maxime Coquelin <address@hidden>
> > ---
> > 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