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

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

[Bandan Das]

PCIBus registers a vmstate during init. Unregister it upon
removal/unplug.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
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.

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

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 <bsd@redhat.com>

Michael, this patch looks good for 1.7 to me, are you planning to still
pick it up? Only one small comment below.

Cc: qemu-stable@nongnu.org

> ---
> 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 <afaerber@suse.de>

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

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 <bsd@redhat.com>
> 
> Michael, this patch looks good for 1.7 to me, are you planning to still
> pick it up? Only one small comment below.
> 
> Cc: qemu-stable@nongnu.org
> > ---
> > 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 <afaerber@suse.de>
> 
> 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

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

[Bandan Das]

Andreas Färber <afaerber@suse.de> 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 <bsd@redhat.com>
>
> Michael, this patch looks good for 1.7 to me, are you planning to still
> pick it up? Only one small comment below.
>
> Cc: qemu-stable@nongnu.org
>
>> ---
>> 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 <afaerber@suse.de>
>
> 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);

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

[Andreas Färber]

Am 19.11.2013 18:03, schrieb Bandan Das:
> Andreas Färber <afaerber@suse.de> 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 <bsd@redhat.com>
>>
>> Michael, this patch looks good for 1.7 to me, are you planning to still
>> pick it up? Only one small comment below.
>>
>> Cc: qemu-stable@nongnu.org
>>
>>> ---
>>> 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
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).

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Paolo Bonzini]

Il 19/11/2013 18:33, Andreas Färber ha scritto:
> 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
> 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.

I agree.  However, this is also a bug fix, and it would be a bit ugly to
tie it to large infrastructure changes (same reason why I want to have
my virtio hotplug/unplug fixes go in _before_ the realize patches, for
example).

With that in mind, Bandan's patch could be good for 1.7.

Paolo

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

[Bandan Das]

Andreas Färber <afaerber@suse.de> writes:

> Am 19.11.2013 18:03, schrieb Bandan Das:
>> Andreas Färber <afaerber@suse.de> 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 <bsd@redhat.com>
>>>
>>> Michael, this patch looks good for 1.7 to me, are you planning to still
>>> pick it up? Only one small comment below.
>>>
>>> Cc: qemu-stable@nongnu.org
>>>
>>>> ---
>>>> 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).