Re: [Qemu-devel] [PATCH for-1.7] pci: unregister vmstate_pcibus on unplug

[Michael S. Tsirkin]

On Tue, Nov 19, 2013 at 01:51:58PM +0100, Andreas Färber wrote:
> Am 06.11.2013 23:52, schrieb Bandan Das:
> > 
> > PCIBus registers a vmstate during init. Unregister it upon
> > removal/unplug.
> > 
> > Signed-off-by: Bandan Das <address@hidden>
> 
> Michael, this patch looks good for 1.7 to me, are you planning to still
> pick it up? Only one small comment below.
> 
> Cc: address@hidden
> > ---
> > Note that I didn't add a instance_init to register vmstate (yet) 
> > due to concerns expressed by Andreas that we shouldn't be registering 
> > global state there.
> 
> 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.
> 
> >  hw/pci/pci.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > 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, DeviceState 
> > *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);
> 
> 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,
> 
> Reviewed-by: Andreas Färber <address@hidden>
> 
> Thanks,
> Andreas
> 


Thanks for the review, I'll review and hopefully merge
later today.

> >  
> >  static Property pci_props[] = {
> >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > @@ -73,6 +74,7 @@ static const TypeInfo pci_bus_info = {
> >      .name = TYPE_PCI_BUS,
> >      .parent = TYPE_BUS,
> >      .instance_size = sizeof(PCIBus),
> > +    .instance_finalize = pci_bus_finalize,
> >      .class_init = pci_bus_class_init,
> >  };
> >  
> > @@ -401,6 +403,12 @@ int pci_bus_num(PCIBus *s)
> >      return s->parent_dev->config[PCI_SECONDARY_BUS];
> >  }
> >  
> > +static void pci_bus_finalize(Object *obj)
> > +{
> > +    PCIBus *bus = PCI_BUS(obj);
> > +    vmstate_unregister(NULL, &vmstate_pcibus, bus);
> > +}
> > +
> >  static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
> >  {
> >      PCIDevice *s = container_of(pv, PCIDevice, config);
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg