From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vilk5-0002C7-9K for qemu-devel@nongnu.org; Tue, 19 Nov 2013 08:53:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vilk0-0005L1-Kk for qemu-devel@nongnu.org; Tue, 19 Nov 2013 08:53:25 -0500 Date: Tue, 19 Nov 2013 15:56:31 +0200 From: "Michael S. Tsirkin" Message-ID: <20131119135631.GA13479@redhat.com> References: <528B5EEE.5000705@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <528B5EEE.5000705@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-1.7] pci: unregister vmstate_pcibus on unplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Paolo Bonzini , Bandan Das , qemu-devel@nongnu.org, Anthony Liguori , qemu-stable On Tue, Nov 19, 2013 at 01:51:58PM +0100, Andreas F=E4rber wrote: > Am 06.11.2013 23:52, schrieb Bandan Das: > >=20 > > PCIBus registers a vmstate during init. Unregister it upon > > removal/unplug. > >=20 > > Signed-off-by: Bandan Das >=20 > Michael, this patch looks good for 1.7 to me, are you planning to still > pick it up? Only one small comment below. >=20 > Cc: qemu-stable@nongnu.org > > --- > > Note that I didn't add a instance_init to register vmstate (yet)=20 > > due to concerns expressed by Andreas that we shouldn't be registering= =20 > > global state there. >=20 > What's happening here is the following: instance_init does in fact not > register anything, but vmstate_unregister() becomes a no-op loop if the > vmsd+opaque combo is not registered, so it is safe. The registration > happens in pci_bus_new() / pci_bus_new_inplace(), which I believe all > PCI buses to date inside QEMU use, i.e. after instance_init, so in > practice unregistering will not be no-op. >=20 > > hw/pci/pci.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > >=20 > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index a98c8a0..63ef7ce 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -47,6 +47,7 @@ static void pcibus_dev_print(Monitor *mon, DeviceSt= ate *dev, int indent); > > static char *pcibus_get_dev_path(DeviceState *dev); > > static char *pcibus_get_fw_dev_path(DeviceState *dev); > > static int pcibus_reset(BusState *qbus); > > +static void pci_bus_finalize(Object *obj); >=20 > It may be nicer to avoid the prototype by moving the new > pci_bus_finalize() above pci_bus_info. But since what counts is the fix > to avoid segfaults during migration on access to a dangling opaque > pointer after hot-unplug of a PCI-PCI bridge, >=20 > Reviewed-by: Andreas F=E4rber >=20 > Thanks, > Andreas >=20 Thanks for the review, I'll review and hopefully merge later today. > > =20 > > static Property pci_props[] =3D { > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > > @@ -73,6 +74,7 @@ static const TypeInfo pci_bus_info =3D { > > .name =3D TYPE_PCI_BUS, > > .parent =3D TYPE_BUS, > > .instance_size =3D sizeof(PCIBus), > > + .instance_finalize =3D pci_bus_finalize, > > .class_init =3D pci_bus_class_init, > > }; > > =20 > > @@ -401,6 +403,12 @@ int pci_bus_num(PCIBus *s) > > return s->parent_dev->config[PCI_SECONDARY_BUS]; > > } > > =20 > > +static void pci_bus_finalize(Object *obj) > > +{ > > + PCIBus *bus =3D PCI_BUS(obj); > > + vmstate_unregister(NULL, &vmstate_pcibus, bus); > > +} > > + > > static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) > > { > > PCIDevice *s =3D container_of(pv, PCIDevice, config); >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg