Re: [Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler

[Igor Mammedov <imammedo@redhat.com>]

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

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > it will allow to drop BusState.allow_hotplug field
> > and still keep ICC bus hotluggable for CPU/APIC devices.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/cpu/icc_bus.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> > index 7f44c59..cb5ed5c 100644
> > --- a/hw/cpu/icc_bus.c
> > +++ b/hw/cpu/icc_bus.c
> > @@ -24,18 +24,10 @@
> >  
> >  /* icc-bridge implementation */
> >  
> > -static void icc_bus_init(Object *obj)
> > -{
> > -    BusState *b = BUS(obj);
> > -
> > -    b->allow_hotplug = true;
> > -}
> > -
> >  static const TypeInfo icc_bus_info = {
> >      .name = TYPE_ICC_BUS,
> >      .parent = TYPE_BUS,
> >      .instance_size = sizeof(ICCBus),
> > -    .instance_init = icc_bus_init,
> >  };
> >  
> >  
> > @@ -100,11 +92,19 @@ static void icc_bridge_init(Object *obj)
> >      s->icc_bus.apic_address_space = &s->apic_container;
> >  }
> >  
> > +static void icc_bridge_realize(DeviceState *dev, Error **errp)
> > +{
> > +    ICCBridgeState *s = ICC_BRIGDE(dev);
> > +
> > +    qbus_set_hotplug_handler(BUS(&s->icc_bus), dev, errp);
> > +}
> > +
> >  static void icc_bridge_class_init(ObjectClass *oc, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(oc);
> >  
> >      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> > +    dc->realize = icc_bridge_realize;
> >  }
> >  
> >  static const TypeInfo icc_bridge_info = {
> > @@ -113,6 +113,10 @@ static const TypeInfo icc_bridge_info = {
> >      .instance_init  = icc_bridge_init,
> >      .instance_size  = sizeof(ICCBridgeState),
> >      .class_init = icc_bridge_class_init,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_HOTPLUG_HANDLER },
> > +        { }
> > +    }
> >  };
> >  
> >  
> > 
> 
> If you do this, aren't you enabling CPU hot-unplug?  Should
> hotplug_handler_plug and/or hotplug_handler_unplug return an error if
> there is no callback?

hotplug/unplug of x86 CPU is not usable with device_add/del yet, so
it's not used for CPUs yet.
x86 CPU nor APIC don't have DeviceClass.unplug callback so for old
behavior any attempt to call device_del would cause abort.
With this path attempt would become NOP.

I've thought it would be better to add hotplug_handler_plug and/or hotplug_handler_unplug
error handling to amended CPU unplug patches that were on list.
Leaving this series purpose clean (drop legacy unplug and provide
infrastructure for async unplug that will be used for CPU and pc-dimm devices)

> 
> Paolo