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

[Andreas Färber]

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

>  
>  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