[Qemu-devel] [PATCH v2 0/4] qdev: remove all remaining cannot_destroy_with_object_finalize_yet

[Qemu-devel] [PATCH v2 0/4] qdev: remove all remaining cannot_destroy_with_object_finalize_yet

[Laurent Vivier]

This series removes all the remaining uses of
cannot_destroy_with_object_finalize_yet to finally remove
the flag itself.

The ARM patch has already been sent alone and reviewed by Markus.
I have tested the ppc one on ppc64 machine with KVM and using
QDM device-list-properties command.

For the versatile one, the flag allowed to workaround a problem
in the bus unparent function: the bus unparent is trying to
unparent all the children of the bus. To do that, it has a list
of the children of the bus, and calls object_unparent() for each
child, and object_unparent() calls object_property_del_child() if
obj->parent is not NULL.  As qdev_set_parent_bus() set only
parent_bus and the list of children, parent is NULL and the child
is never deleted.  We can avoid the problem by moving the
qdev_set_parent_bus() to the realize part.

I've tested all the changes with "make check" (including
device-introspect-test). I've booted a versatilepb machine
with a 3.16.0-4 debian installer kernel.

Laurent Vivier (4):
  arm: remove remaining cannot_destroy_with_object_finalize_yet
  ppc: remove cannot_destroy_with_object_finalize_yet
  versatile: remove cannot_destroy_with_object_finalize_yet
  qdev: remove cannot_destroy_with_object_finalize_yet

 hw/arm/allwinner-a10.c  |  6 ------
 hw/arm/bcm2836.c        |  6 ------
 hw/arm/digic.c          |  6 ------
 hw/arm/fsl-imx25.c      |  5 -----
 hw/arm/fsl-imx31.c      |  5 -----
 hw/arm/fsl-imx6.c       |  5 -----
 hw/arm/xlnx-zynqmp.c    |  6 ------
 hw/pci-host/versatile.c | 35 ++++++++++++-----------------------
 include/hw/qdev-core.h  | 13 -------------
 qmp.c                   |  5 -----
 target/ppc/kvm.c        | 10 ----------
 11 files changed, 12 insertions(+), 90 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/4] arm: remove remaining cannot_destroy_with_object_finalize_yet

[Laurent Vivier]

With commit ce5b1bbf624b ("exec: move cpu_exec_init() calls to
realize functions"), we can now remove all the
remaining cannot_destroy_with_object_finalize_yet as
unsafe references have been moved to cpu_exec_realizefn().
(tested with QOM command provided by commit 4c315c27).

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/allwinner-a10.c | 6 ------
 hw/arm/bcm2836.c       | 6 ------
 hw/arm/digic.c         | 6 ------
 hw/arm/fsl-imx25.c     | 5 -----
 hw/arm/fsl-imx31.c     | 5 -----
 hw/arm/fsl-imx6.c      | 5 -----
 hw/arm/xlnx-zynqmp.c   | 6 ------
 7 files changed, 39 deletions(-)

diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index ca15d1c..f62a9a3 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -118,12 +118,6 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = aw_a10_realize;
-
-    /*
-     * Reason: creates an ARM CPU, thus use after free(), see
-     * arm_cpu_class_init()
-     */
-    dc->cannot_destroy_with_object_finalize_yet = true;
 }
 
 static const TypeInfo aw_a10_type_info = {
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 8451190..8c43291 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -160,12 +160,6 @@ static void bcm2836_class_init(ObjectClass *oc, void *data)
 
     dc->props = bcm2836_props;
     dc->realize = bcm2836_realize;
-
-    /*
-     * Reason: creates an ARM CPU, thus use after free(), see
-     * arm_cpu_class_init()
-     */
-    dc->cannot_destroy_with_object_finalize_yet = true;
 }
 
 static const TypeInfo bcm2836_type_info = {
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index d60ea39..94f3263 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -101,12 +101,6 @@ static void digic_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = digic_realize;
-
-    /*
-     * Reason: creates an ARM CPU, thus use after free(), see
-     * arm_cpu_class_init()
-     */
-    dc->cannot_destroy_with_object_finalize_yet = true;
 }
 
 static const TypeInfo digic_type_info = {
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 2126f73..9056f27 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -290,11 +290,6 @@ static void fsl_imx25_class_init(ObjectClass *oc, void *data)
 
     dc->realize = fsl_imx25_realize;
 
-    /*
-     * Reason: creates an ARM CPU, thus use after free(), see
-     * arm_cpu_class_init()
-     */
-    dc->cannot_destroy_with_object_finalize_yet = true;
     dc->desc = "i.MX25 SOC";
 }
 
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index dd1c713..d7e2d83 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -262,11 +262,6 @@ static void fsl_imx31_class_init(ObjectClass *oc, void *data)
 
     dc->realize = fsl_imx31_realize;
 
-    /*
-     * Reason: creates an ARM CPU, thus use after free(), see
-     * arm_cpu_class_init()
-     */
-    dc->cannot_destroy_with_object_finalize_yet = true;
     dc->desc = "i.MX31 SOC";
 }
 
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 76dd8a4..6969e73 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -442,11 +442,6 @@ static void fsl_imx6_class_init(ObjectClass *oc, void *data)
 
     dc->realize = fsl_imx6_realize;
 
-    /*
-     * Reason: creates an ARM CPU, thus use after free(), see
-     * arm_cpu_class_init()
-     */
-    dc->cannot_destroy_with_object_finalize_yet = true;
     dc->desc = "i.MX6 SOC";
 }
 
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index bc4e66b..4f67158 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -439,12 +439,6 @@ static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
 
     dc->props = xlnx_zynqmp_props;
     dc->realize = xlnx_zynqmp_realize;
-
-    /*
-     * Reason: creates an ARM CPU, thus use after free(), see
-     * arm_cpu_class_init()
-     */
-    dc->cannot_destroy_with_object_finalize_yet = true;
 }
 
 static const TypeInfo xlnx_zynqmp_type_info = {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 1/4] arm: remove remaining cannot_destroy_with_object_finalize_yet

[Alistair Francis]

On Fri, Apr 14, 2017 at 1:37 AM, Laurent Vivier <lvivier@redhat.com> wrote:
> With commit ce5b1bbf624b ("exec: move cpu_exec_init() calls to
> realize functions"), we can now remove all the
> remaining cannot_destroy_with_object_finalize_yet as
> unsafe references have been moved to cpu_exec_realizefn().
> (tested with QOM command provided by commit 4c315c27).
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/arm/allwinner-a10.c | 6 ------
>  hw/arm/bcm2836.c       | 6 ------
>  hw/arm/digic.c         | 6 ------
>  hw/arm/fsl-imx25.c     | 5 -----
>  hw/arm/fsl-imx31.c     | 5 -----
>  hw/arm/fsl-imx6.c      | 5 -----
>  hw/arm/xlnx-zynqmp.c   | 6 ------
>  7 files changed, 39 deletions(-)
>
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index ca15d1c..f62a9a3 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -118,12 +118,6 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>
>      dc->realize = aw_a10_realize;
> -
> -    /*
> -     * Reason: creates an ARM CPU, thus use after free(), see
> -     * arm_cpu_class_init()
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>
>  static const TypeInfo aw_a10_type_info = {
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 8451190..8c43291 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -160,12 +160,6 @@ static void bcm2836_class_init(ObjectClass *oc, void *data)
>
>      dc->props = bcm2836_props;
>      dc->realize = bcm2836_realize;
> -
> -    /*
> -     * Reason: creates an ARM CPU, thus use after free(), see
> -     * arm_cpu_class_init()
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>
>  static const TypeInfo bcm2836_type_info = {
> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> index d60ea39..94f3263 100644
> --- a/hw/arm/digic.c
> +++ b/hw/arm/digic.c
> @@ -101,12 +101,6 @@ static void digic_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>
>      dc->realize = digic_realize;
> -
> -    /*
> -     * Reason: creates an ARM CPU, thus use after free(), see
> -     * arm_cpu_class_init()
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>
>  static const TypeInfo digic_type_info = {
> diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
> index 2126f73..9056f27 100644
> --- a/hw/arm/fsl-imx25.c
> +++ b/hw/arm/fsl-imx25.c
> @@ -290,11 +290,6 @@ static void fsl_imx25_class_init(ObjectClass *oc, void *data)
>
>      dc->realize = fsl_imx25_realize;
>
> -    /*
> -     * Reason: creates an ARM CPU, thus use after free(), see
> -     * arm_cpu_class_init()
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>      dc->desc = "i.MX25 SOC";
>  }
>
> diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
> index dd1c713..d7e2d83 100644
> --- a/hw/arm/fsl-imx31.c
> +++ b/hw/arm/fsl-imx31.c
> @@ -262,11 +262,6 @@ static void fsl_imx31_class_init(ObjectClass *oc, void *data)
>
>      dc->realize = fsl_imx31_realize;
>
> -    /*
> -     * Reason: creates an ARM CPU, thus use after free(), see
> -     * arm_cpu_class_init()
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>      dc->desc = "i.MX31 SOC";
>  }
>
> diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
> index 76dd8a4..6969e73 100644
> --- a/hw/arm/fsl-imx6.c
> +++ b/hw/arm/fsl-imx6.c
> @@ -442,11 +442,6 @@ static void fsl_imx6_class_init(ObjectClass *oc, void *data)
>
>      dc->realize = fsl_imx6_realize;
>
> -    /*
> -     * Reason: creates an ARM CPU, thus use after free(), see
> -     * arm_cpu_class_init()
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>      dc->desc = "i.MX6 SOC";
>  }
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index bc4e66b..4f67158 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -439,12 +439,6 @@ static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
>
>      dc->props = xlnx_zynqmp_props;
>      dc->realize = xlnx_zynqmp_realize;
> -
> -    /*
> -     * Reason: creates an ARM CPU, thus use after free(), see
> -     * arm_cpu_class_init()
> -     */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>
>  static const TypeInfo xlnx_zynqmp_type_info = {
> --
> 2.9.3
>
>

[Qemu-devel] [PATCH v2 2/4] ppc: remove cannot_destroy_with_object_finalize_yet

[Laurent Vivier]

This removes the assert(kvm_enabled()) from kvmppc_host_cpu_initfn()

This assert can never be triggered as the function is only registered
when KVM is available (see also 4c315c2
"qdev: Protect device-list-properties against broken devices").

So we can remove the cannot_destroy_with_object_finalize_yet from
kvmppc_host_cpu_class_init() without fear and beyond reproach.
(as it has already be done for i386 with 771a13e "i386: Unset
cannot_destroy_with_object_finalize_yet on "host" model" and
e435601 "target-i386: Remove assert(kvm_enabled()) from
host_x86_cpu_initfn()")

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 target/ppc/kvm.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 9f1f132..64017ac 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2245,14 +2245,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
     }
 }
 
-static void kvmppc_host_cpu_initfn(Object *obj)
-{
-    assert(kvm_enabled());
-}
-
 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(oc);
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
     uint32_t vmx = kvmppc_get_vmx();
     uint32_t dfp = kvmppc_get_dfp();
@@ -2279,9 +2273,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
     if (icache_size != -1) {
         pcc->l1_icache_size = icache_size;
     }
-
-    /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */
-    dc->cannot_destroy_with_object_finalize_yet = true;
 }
 
 bool kvmppc_has_cap_epr(void)
@@ -2333,7 +2324,6 @@ static int kvm_ppc_register_host_cpu_type(void)
 {
     TypeInfo type_info = {
         .name = TYPE_HOST_POWERPC_CPU,
-        .instance_init = kvmppc_host_cpu_initfn,
         .class_init = kvmppc_host_cpu_class_init,
     };
     PowerPCCPUClass *pvr_pcc;
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/4] ppc: remove cannot_destroy_with_object_finalize_yet

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

On Fri, Apr 14, 2017 at 10:37:15AM +0200, Laurent Vivier wrote:
> This removes the assert(kvm_enabled()) from kvmppc_host_cpu_initfn()
> 
> This assert can never be triggered as the function is only registered
> when KVM is available (see also 4c315c2
> "qdev: Protect device-list-properties against broken devices").
> 
> So we can remove the cannot_destroy_with_object_finalize_yet from
> kvmppc_host_cpu_class_init() without fear and beyond reproach.
> (as it has already be done for i386 with 771a13e "i386: Unset
> cannot_destroy_with_object_finalize_yet on "host" model" and
> e435601 "target-i386: Remove assert(kvm_enabled()) from
> host_x86_cpu_initfn()")
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Applied to ppc-for-2.10 (fixing a contextual conflict on the way).


> ---
>  target/ppc/kvm.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9f1f132..64017ac 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2245,14 +2245,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>      }
>  }
>  
> -static void kvmppc_host_cpu_initfn(Object *obj)
> -{
> -    assert(kvm_enabled());
> -}
> -
>  static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(oc);
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>      uint32_t vmx = kvmppc_get_vmx();
>      uint32_t dfp = kvmppc_get_dfp();
> @@ -2279,9 +2273,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>      if (icache_size != -1) {
>          pcc->l1_icache_size = icache_size;
>      }
> -
> -    /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */
> -    dc->cannot_destroy_with_object_finalize_yet = true;
>  }
>  
>  bool kvmppc_has_cap_epr(void)
> @@ -2333,7 +2324,6 @@ static int kvm_ppc_register_host_cpu_type(void)
>  {
>      TypeInfo type_info = {
>          .name = TYPE_HOST_POWERPC_CPU,
> -        .instance_init = kvmppc_host_cpu_initfn,
>          .class_init = kvmppc_host_cpu_class_init,
>      };
>      PowerPCCPUClass *pvr_pcc;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/4] ppc: remove cannot_destroy_with_object_finalize_yet

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

On Tue, Apr 18, 2017 at 01:09:50PM +1000, David Gibson wrote:
> On Fri, Apr 14, 2017 at 10:37:15AM +0200, Laurent Vivier wrote:
> > This removes the assert(kvm_enabled()) from kvmppc_host_cpu_initfn()
> > 
> > This assert can never be triggered as the function is only registered
> > when KVM is available (see also 4c315c2
> > "qdev: Protect device-list-properties against broken devices").
> > 
> > So we can remove the cannot_destroy_with_object_finalize_yet from
> > kvmppc_host_cpu_class_init() without fear and beyond reproach.
> > (as it has already be done for i386 with 771a13e "i386: Unset
> > cannot_destroy_with_object_finalize_yet on "host" model" and
> > e435601 "target-i386: Remove assert(kvm_enabled()) from
> > host_x86_cpu_initfn()")
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Applied to ppc-for-2.10 (fixing a contextual conflict on the way).

Looks like this will go through Markus' tree instead, so:

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> 
> 
> > ---
> >  target/ppc/kvm.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 9f1f132..64017ac 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2245,14 +2245,8 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
> >      }
> >  }
> >  
> > -static void kvmppc_host_cpu_initfn(Object *obj)
> > -{
> > -    assert(kvm_enabled());
> > -}
> > -
> >  static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> >  {
> > -    DeviceClass *dc = DEVICE_CLASS(oc);
> >      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> >      uint32_t vmx = kvmppc_get_vmx();
> >      uint32_t dfp = kvmppc_get_dfp();
> > @@ -2279,9 +2273,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> >      if (icache_size != -1) {
> >          pcc->l1_icache_size = icache_size;
> >      }
> > -
> > -    /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */
> > -    dc->cannot_destroy_with_object_finalize_yet = true;
> >  }
> >  
> >  bool kvmppc_has_cap_epr(void)
> > @@ -2333,7 +2324,6 @@ static int kvm_ppc_register_host_cpu_type(void)
> >  {
> >      TypeInfo type_info = {
> >          .name = TYPE_HOST_POWERPC_CPU,
> > -        .instance_init = kvmppc_host_cpu_initfn,
> >          .class_init = kvmppc_host_cpu_class_init,
> >      };
> >      PowerPCCPUClass *pvr_pcc;
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH v2 3/4] versatile: remove cannot_destroy_with_object_finalize_yet

[Laurent Vivier]

cannot_destroy_with_object_finalize_yet was added by 4c315c2
("qdev: Protect device-list-properties against broken devices")
because "realview_pci" and "versatile_pci" were hanging
during "device-list-properties" cleanup (an infinite loop in
bus_unparent()).

We have this problem because the child is not removed from
the list of the PCI bus child because it has no defined parent:
qdev_set_parent_bus() set the device parent_bus pointer to bus, and
adds the device in the bus children list, but doesn't update the
device parent pointer.

To fix the problem, move all the involved parts to the realize function.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/pci-host/versatile.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 467cbb9..27fde46 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -380,20 +380,8 @@ static void pci_vpb_reset(DeviceState *d)
 
 static void pci_vpb_init(Object *obj)
 {
-    PCIHostState *h = PCI_HOST_BRIDGE(obj);
     PCIVPBState *s = PCI_VPB(obj);
 
-    memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);
-    memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);
-
-    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci",
-                        &s->pci_mem_space, &s->pci_io_space,
-                        PCI_DEVFN(11, 0), TYPE_PCI_BUS);
-    h->bus = &s->pci_bus;
-
-    object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_VERSATILE_PCI_HOST);
-    qdev_set_parent_bus(DEVICE(&s->pci_dev), BUS(&s->pci_bus));
-
     /* Window sizes for VersatilePB; realview_pci's init will override */
     s->mem_win_size[0] = 0x0c000000;
     s->mem_win_size[1] = 0x10000000;
@@ -403,10 +391,22 @@ static void pci_vpb_init(Object *obj)
 static void pci_vpb_realize(DeviceState *dev, Error **errp)
 {
     PCIVPBState *s = PCI_VPB(dev);
+    PCIHostState *h = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     pci_map_irq_fn mapfn;
     int i;
 
+    memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);
+    memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);
+
+    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), dev, "pci",
+                        &s->pci_mem_space, &s->pci_io_space,
+                        PCI_DEVFN(11, 0), TYPE_PCI_BUS);
+    h->bus = &s->pci_bus;
+
+    object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_VERSATILE_PCI_HOST);
+    qdev_set_parent_bus(DEVICE(&s->pci_dev), BUS(&s->pci_bus));
+
     for (i = 0; i < 4; i++) {
         sysbus_init_irq(sbd, &s->irq[i]);
     }
@@ -503,8 +503,6 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
     dc->reset = pci_vpb_reset;
     dc->vmsd = &pci_vpb_vmstate;
     dc->props = pci_vpb_properties;
-    /* Reason: object_unref() hangs */
-    dc->cannot_destroy_with_object_finalize_yet = true;
 }
 
 static const TypeInfo pci_vpb_info = {
@@ -526,19 +524,10 @@ static void pci_realview_init(Object *obj)
     s->mem_win_size[2] = 0x08000000;
 }
 
-static void pci_realview_class_init(ObjectClass *class, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(class);
-
-    /* Reason: object_unref() hangs */
-    dc->cannot_destroy_with_object_finalize_yet = true;
-}
-
 static const TypeInfo pci_realview_info = {
     .name          = "realview_pci",
     .parent        = TYPE_VERSATILE_PCI,
     .instance_init = pci_realview_init,
-    .class_init    = pci_realview_class_init,
 };
 
 static void versatile_pci_register_types(void)
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 3/4] versatile: remove cannot_destroy_with_object_finalize_yet

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> cannot_destroy_with_object_finalize_yet was added by 4c315c2
> ("qdev: Protect device-list-properties against broken devices")
> because "realview_pci" and "versatile_pci" were hanging
> during "device-list-properties" cleanup (an infinite loop in
> bus_unparent()).
>
> We have this problem because the child is not removed from
> the list of the PCI bus child because it has no defined parent:

s/child/children/

> qdev_set_parent_bus() set the device parent_bus pointer to bus, and
> adds the device in the bus children list, but doesn't update the
> device parent pointer.
>
> To fix the problem, move all the involved parts to the realize function.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

The devices aren't unpluggable.  There is no unrealize().  How work
needs to be spread between init and realize isn't obvious to me in
general.  I can see this work done both in init and in realize.  I
figure the move is okay.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 4/4] qdev: remove cannot_destroy_with_object_finalize_yet

[Laurent Vivier]

As all users have been removed, we can remove
cannot_destroy_with_object_finalize_yet field
from the DeviceClass structure.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 include/hw/qdev-core.h | 13 -------------
 qmp.c                  |  5 -----
 2 files changed, 18 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index b44b476..ac682a6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -113,19 +113,6 @@ typedef struct DeviceClass {
      * TODO remove once we're there
      */
     bool cannot_instantiate_with_device_add_yet;
-    /*
-     * Does this device model survive object_unref(object_new(TNAME))?
-     * All device models should, and this flag shouldn't exist.  Some
-     * devices crash in object_new(), some crash or hang in
-     * object_unref().  Makes introspecting properties with
-     * qmp_device_list_properties() dangerous.  Bad, because it's used
-     * by -device FOO,help.  This flag serves to protect that code.
-     * It should never be set without a comment explaining why it is
-     * set.
-     * TODO remove once we're there
-     */
-    bool cannot_destroy_with_object_finalize_yet;
-
     bool hotpluggable;
 
     /* callbacks */
diff --git a/qmp.c b/qmp.c
index a744e44..ab74cd7 100644
--- a/qmp.c
+++ b/qmp.c
@@ -548,11 +548,6 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
         return NULL;
     }
 
-    if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
-        error_setg(errp, "Can't list properties of device '%s'", typename);
-        return NULL;
-    }
-
     obj = object_new(typename);
 
     object_property_iter_init(&iter, obj);
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 4/4] qdev: remove cannot_destroy_with_object_finalize_yet

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> As all users have been removed, we can remove
> cannot_destroy_with_object_finalize_yet field
> from the DeviceClass structure.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Huzzah!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/4] qdev: remove all remaining cannot_destroy_with_object_finalize_yet

[Peter Maydell]

On 14 April 2017 at 09:37, Laurent Vivier <lvivier@redhat.com> wrote:
> This series removes all the remaining uses of
> cannot_destroy_with_object_finalize_yet to finally remove
> the flag itself.
>
> The ARM patch has already been sent alone and reviewed by Markus.
> I have tested the ppc one on ppc64 machine with KVM and using
> QDM device-list-properties command.
>
> For the versatile one, the flag allowed to workaround a problem
> in the bus unparent function: the bus unparent is trying to
> unparent all the children of the bus. To do that, it has a list
> of the children of the bus, and calls object_unparent() for each
> child, and object_unparent() calls object_property_del_child() if
> obj->parent is not NULL.  As qdev_set_parent_bus() set only
> parent_bus and the list of children, parent is NULL and the child
> is never deleted.  We can avoid the problem by moving the
> qdev_set_parent_bus() to the realize part.
>
> I've tested all the changes with "make check" (including
> device-introspect-test). I've booted a versatilepb machine
> with a 3.16.0-4 debian installer kernel.
>
> Laurent Vivier (4):
>   arm: remove remaining cannot_destroy_with_object_finalize_yet
>   ppc: remove cannot_destroy_with_object_finalize_yet
>   versatile: remove cannot_destroy_with_object_finalize_yet
>   qdev: remove cannot_destroy_with_object_finalize_yet

Markus -- are you planning to take this whole series through
your tree? I'm happy with the ARM patches but I guess we
should keep the whole series together since patch 4 depends
on the other 3...

I guess that means
Acked-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/4] qdev: remove all remaining cannot_destroy_with_object_finalize_yet

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 14 April 2017 at 09:37, Laurent Vivier <lvivier@redhat.com> wrote:
>> This series removes all the remaining uses of
>> cannot_destroy_with_object_finalize_yet to finally remove
>> the flag itself.
>>
>> The ARM patch has already been sent alone and reviewed by Markus.
>> I have tested the ppc one on ppc64 machine with KVM and using
>> QDM device-list-properties command.
>>
>> For the versatile one, the flag allowed to workaround a problem
>> in the bus unparent function: the bus unparent is trying to
>> unparent all the children of the bus. To do that, it has a list
>> of the children of the bus, and calls object_unparent() for each
>> child, and object_unparent() calls object_property_del_child() if
>> obj->parent is not NULL.  As qdev_set_parent_bus() set only
>> parent_bus and the list of children, parent is NULL and the child
>> is never deleted.  We can avoid the problem by moving the
>> qdev_set_parent_bus() to the realize part.
>>
>> I've tested all the changes with "make check" (including
>> device-introspect-test). I've booted a versatilepb machine
>> with a 3.16.0-4 debian installer kernel.
>>
>> Laurent Vivier (4):
>>   arm: remove remaining cannot_destroy_with_object_finalize_yet
>>   ppc: remove cannot_destroy_with_object_finalize_yet
>>   versatile: remove cannot_destroy_with_object_finalize_yet
>>   qdev: remove cannot_destroy_with_object_finalize_yet
>
> Markus -- are you planning to take this whole series through
> your tree? I'm happy with the ARM patches but I guess we
> should keep the whole series together since patch 4 depends
> on the other 3...

We have no qdev maintainer.  David, you wrote you applied PATCH 2.  Are
you okay with me taking all four?  If yes, would you like me to add your
Acked-by or Reviewed-by to PATCH 2?

> I guess that means
> Acked-by: Peter Maydell <peter.maydell@linaro.org>

Noted.

Re: [Qemu-devel] [PATCH v2 0/4] qdev: remove all remaining cannot_destroy_with_object_finalize_yet

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

On Thu, Apr 20, 2017 at 05:59:27PM +0200, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 14 April 2017 at 09:37, Laurent Vivier <lvivier@redhat.com> wrote:
> >> This series removes all the remaining uses of
> >> cannot_destroy_with_object_finalize_yet to finally remove
> >> the flag itself.
> >>
> >> The ARM patch has already been sent alone and reviewed by Markus.
> >> I have tested the ppc one on ppc64 machine with KVM and using
> >> QDM device-list-properties command.
> >>
> >> For the versatile one, the flag allowed to workaround a problem
> >> in the bus unparent function: the bus unparent is trying to
> >> unparent all the children of the bus. To do that, it has a list
> >> of the children of the bus, and calls object_unparent() for each
> >> child, and object_unparent() calls object_property_del_child() if
> >> obj->parent is not NULL.  As qdev_set_parent_bus() set only
> >> parent_bus and the list of children, parent is NULL and the child
> >> is never deleted.  We can avoid the problem by moving the
> >> qdev_set_parent_bus() to the realize part.
> >>
> >> I've tested all the changes with "make check" (including
> >> device-introspect-test). I've booted a versatilepb machine
> >> with a 3.16.0-4 debian installer kernel.
> >>
> >> Laurent Vivier (4):
> >>   arm: remove remaining cannot_destroy_with_object_finalize_yet
> >>   ppc: remove cannot_destroy_with_object_finalize_yet
> >>   versatile: remove cannot_destroy_with_object_finalize_yet
> >>   qdev: remove cannot_destroy_with_object_finalize_yet
> >
> > Markus -- are you planning to take this whole series through
> > your tree? I'm happy with the ARM patches but I guess we
> > should keep the whole series together since patch 4 depends
> > on the other 3...
> 
> We have no qdev maintainer.  David, you wrote you applied PATCH 2.  Are
> you okay with me taking all four?  If yes, would you like me to add your
> Acked-by or Reviewed-by to PATCH 2?

That's fine by me.  I've sent an Acked-by for that patch.

> 
> > I guess that means
> > Acked-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Noted.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] qdev: remove all remaining cannot_destroy_with_object_finalize_yet

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> This series removes all the remaining uses of
> cannot_destroy_with_object_finalize_yet to finally remove
> the flag itself.
>
> The ARM patch has already been sent alone and reviewed by Markus.
> I have tested the ppc one on ppc64 machine with KVM and using
> QDM device-list-properties command.
>
> For the versatile one, the flag allowed to workaround a problem
> in the bus unparent function: the bus unparent is trying to
> unparent all the children of the bus. To do that, it has a list
> of the children of the bus, and calls object_unparent() for each
> child, and object_unparent() calls object_property_del_child() if
> obj->parent is not NULL.  As qdev_set_parent_bus() set only
> parent_bus and the list of children, parent is NULL and the child
> is never deleted.  We can avoid the problem by moving the
> qdev_set_parent_bus() to the realize part.
>
> I've tested all the changes with "make check" (including
> device-introspect-test). I've booted a versatilepb machine
> with a 3.16.0-4 debian installer kernel.

Applied to my tree, thanks!