[15/30] virtio-pci: replace BusState.allow_hotplug with hotplug_handler

Although virtio-pci-bus is internal object of composite
virtio-pci device and it doesn't participate in
-device/device_add hotplug flow, it's required by
bus_add_child() that bus must be hotpluggable to
be able to add child at runtime.
Set parent of virtio-pci-bus as NOP hotplug controller,
so that bus_add_child() would allow to add child
device during hotplug time when BusState.allow_hotplug
is dropped.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/virtio/virtio-pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Although virtio-pci-bus is internal object of composite
> virtio-pci device and it doesn't participate in
> -device/device_add hotplug flow, it's required by
> bus_add_child() that bus must be hotpluggable to
> be able to add child at runtime.
> Set parent of virtio-pci-bus as NOP hotplug controller,
> so that bus_add_child() would allow to add child
> device during hotplug time when BusState.allow_hotplug
> is dropped.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f560814..0486b25 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1070,6 +1070,10 @@ static const TypeInfo virtio_pci_info = {
>      .class_init    = virtio_pci_class_init,
>      .class_size    = sizeof(VirtioPCIClass),
>      .abstract      = true,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  /* virtio-blk-pci */
> @@ -1543,13 +1547,11 @@ static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
>                                 VirtIOPCIProxy *dev)
>  {
>      DeviceState *qdev = DEVICE(dev);
> -    BusState *qbus;
>      char virtio_bus_name[] = "virtio-bus";
>  
>      qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_PCI_BUS, qdev,
>                          virtio_bus_name);
> -    qbus = BUS(bus);
> -    qbus->allow_hotplug = 1;
> +    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
>  }
>  
>  static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> 

So this is the opposite of icc-bus; here you need to call the simple
hot-unplug callback in the hotplug handler, right?

Paolo

On Wed, 24 Sep 2014 14:23:25 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > Although virtio-pci-bus is internal object of composite
> > virtio-pci device and it doesn't participate in
> > -device/device_add hotplug flow, it's required by
> > bus_add_child() that bus must be hotpluggable to
> > be able to add child at runtime.
> > Set parent of virtio-pci-bus as NOP hotplug controller,
> > so that bus_add_child() would allow to add child
> > device during hotplug time when BusState.allow_hotplug
> > is dropped.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f560814..0486b25 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1070,6 +1070,10 @@ static const TypeInfo virtio_pci_info = {
> >      .class_init    = virtio_pci_class_init,
> >      .class_size    = sizeof(VirtioPCIClass),
> >      .abstract      = true,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_HOTPLUG_HANDLER },
> > +        { }
> > +    }
> >  };
> >  
> >  /* virtio-blk-pci */
> > @@ -1543,13 +1547,11 @@ static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> >                                 VirtIOPCIProxy *dev)
> >  {
> >      DeviceState *qdev = DEVICE(dev);
> > -    BusState *qbus;
> >      char virtio_bus_name[] = "virtio-bus";
> >  
> >      qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_PCI_BUS, qdev,
> >                          virtio_bus_name);
> > -    qbus = BUS(bus);
> > -    qbus->allow_hotplug = 1;
> > +    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
> >  }
> >  
> >  static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> > 
> 
> So this is the opposite of icc-bus; here you need to call the simple
> hot-unplug callback in the hotplug handler, right?
It's only to workaround bus_add_child() limitation, there is no need
to call unplug callback for child of this bus, since bus and child device
are manually managed by owner of bus without involving hotplug machinery
(via PCIDeviceClass.[init|exit] callbacks).

Perhaps instead we should just allow to add child to bus is bus_add_child()
regardless whether it's hotpluggable or not and drop this and other
similar patches.
> 
> Paolo

Il 24/09/2014 16:51, Igor Mammedov ha scritto:
>> > So this is the opposite of icc-bus; here you need to call the simple
>> > hot-unplug callback in the hotplug handler, right?
> It's only to workaround bus_add_child() limitation, there is no need
> to call unplug callback for child of this bus, since bus and child device
> are manually managed by owner of bus without involving hotplug machinery
> (via PCIDeviceClass.[init|exit] callbacks).
> 
> Perhaps instead we should just allow to add child to bus is bus_add_child()
> regardless whether it's hotpluggable or not and drop this and other
> similar patches.

Yeah, that would be great and would simply usb-storage as well.
Hotpluggability really refers to adding device after initial bring-up of
the bus.  If the controller is hotplugged together with something below,
I don't think that counts.

Paolo