From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57124) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWnhs-0003UD-6k for qemu-devel@nongnu.org; Wed, 24 Sep 2014 10:38:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XWnhj-0001Ic-7R for qemu-devel@nongnu.org; Wed, 24 Sep 2014 10:38:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11958) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWnhj-0001Ht-0s for qemu-devel@nongnu.org; Wed, 24 Sep 2014 10:38:03 -0400 Date: Wed, 24 Sep 2014 16:37:48 +0200 From: Igor Mammedov Message-ID: <20140924163748.18dbc40c@nial.usersys.redhat.com> In-Reply-To: <5422B775.90205@redhat.com> References: <1411559299-19042-1-git-send-email-imammedo@redhat.com> <1411559299-19042-15-git-send-email-imammedo@redhat.com> <5422B775.90205@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: dmitry@daynix.com, mst@redhat.com, agraf@suse.de, qemu-devel@nongnu.org, cornelia.huck@de.ibm.com, kraxel@redhat.com, amit.shah@redhat.com, borntraeger@de.ibm.com, rth@twiddle.net On Wed, 24 Sep 2014 14:22:13 +0200 Paolo Bonzini 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 > > --- > > 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