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

[Bandan Das]

Andreas Färber <address@hidden> writes:

> Am 19.11.2013 18:03, schrieb 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.
>
> Why? I still think that would be wrong. We had previously discussed with
> Paolo that VMState is global state, which according to Anthony should
> not be registered before realization. So far we have a mix of PCI host

Ugh. I again ignored this piece of information. And also got the other 
part wrong - pci_bus_new is called *after* instance_init, which 
means instance_init isn't the right place for a global state registration. 
Agreed, vmstate_register is at the right place currently.

> bridges instantiating PCIBus before or after realization depending on
> whether the bus name needs to depend on the device id or not (with trend
> towards instantiating the PCIBus during instance_init), at which point
> in time the state should not be registered yet. The sketched solution
> was to implement a "realized" property for BusState, so that we can
> decouple vmstate_register() from instantation time rather than moving it
> into instance_init.
>
> Andreas
>
>> 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).