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

[Bandan Das]

Andreas Färber <address@hidden> writes:

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

Ok, thanks! Based on your explanation, I think it should be safe to move
vmstate_register to instance_init as Paolo had suggested. If Michael 
and rest of the folks agree, I am inclined to send in a new version 
(which also fixes the issue you noted below).

Bandan


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