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

[Cornelia Huck]

On Mon, 12 Sep 2016 11:18:52 +0200
Maxime Coquelin <address@hidden> wrote:

> On 09/12/2016 10:51 AM, Cornelia Huck wrote:
> > On Sat, 10 Sep 2016 10:23:37 +0200
> > Maxime Coquelin <address@hidden> 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.