[Qemu-devel] [PATCH v2 0/6] hw/arm: Use ARM_CPU_TYPE_NAME() and object_initialize_child()

[Qemu-devel] [PATCH v2 0/6] hw/arm: Use ARM_CPU_TYPE_NAME() and object_initialize_child()

[Philippe Mathieu-Daudé]

First we use ARM_CPU_TYPE_NAME() when we should.

Then is follow up of [1]:

  This series looks at Eduardo suggestions from [2]
  and Thomas commit aff39be0ed97 to replace various
  object_initialize + qdev_set_parent_bus calls by
  sysbus_init_child_obj().

Finally, some devices are declared orphean while they have a parent,
let them be together again.

Since v1 [3]:
- addressed Peter Maydell review comments

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01492.html
[2] https://patchwork.ozlabs.org/patch/943333/#1953608
[3] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00135.html

Philippe Mathieu-Daudé (6):
  hw/arm: Use ARM_CPU_TYPE_NAME() macro when appropriate
  hw/arm: Use object_initialize_child for correct reference counting
  hw/arm: Use sysbus_init_child_obj for correct reference counting
  hw/arm/fsl-imx: Add the cpu as child of the SoC object
  hw/dma/xilinx_axi: Use object_initialize_child for correct ref.
    counting
  hw/net/xilinx_axi: Use object_initialize_child for correct ref.
    counting

 hw/arm/allwinner-a10.c  |  3 ++-
 hw/arm/cubieboard.c     |  3 ++-
 hw/arm/digic.c          |  3 ++-
 hw/arm/exynos4_boards.c |  4 ++--
 hw/arm/fsl-imx25.c      |  4 +++-
 hw/arm/fsl-imx31.c      |  4 +++-
 hw/arm/fsl-imx6.c       |  3 ++-
 hw/arm/fsl-imx6ul.c     |  2 +-
 hw/arm/mcimx7d-sabre.c  |  9 ++++-----
 hw/arm/mps2-tz.c        | 15 +++++++--------
 hw/arm/musca.c          |  9 +++++----
 hw/arm/xlnx-zynqmp.c    |  8 ++++----
 hw/dma/xilinx_axidma.c  | 16 ++++++++--------
 hw/net/xilinx_axienet.c | 17 ++++++++---------
 14 files changed, 53 insertions(+), 47 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/6] hw/arm: Use ARM_CPU_TYPE_NAME() macro when appropriate

[Philippe Mathieu-Daudé]

Commit ba1ba5cca introduce the ARM_CPU_TYPE_NAME() macro.
Unify the code base by use it in all places.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: rebased, use in imx25/31 (pm215)
---
 hw/arm/allwinner-a10.c | 3 ++-
 hw/arm/cubieboard.c    | 3 ++-
 hw/arm/digic.c         | 3 ++-
 hw/arm/fsl-imx25.c     | 2 +-
 hw/arm/fsl-imx31.c     | 2 +-
 hw/arm/fsl-imx6.c      | 3 ++-
 hw/arm/fsl-imx6ul.c    | 2 +-
 hw/arm/xlnx-zynqmp.c   | 8 ++++----
 8 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index 73810a4440..118032c8c7 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -30,7 +30,8 @@ static void aw_a10_init(Object *obj)
     AwA10State *s = AW_A10(obj);
 
     object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
-                            "cortex-a8-" TYPE_ARM_CPU, &error_abort, NULL);
+                            ARM_CPU_TYPE_NAME("cortex-a8"),
+                            &error_abort, NULL);
 
     sysbus_init_child_obj(obj, "intc", &s->intc, sizeof(s->intc),
                           TYPE_AW_A10_PIC);
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index 38e0ca0f53..ed8d2333a0 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -81,7 +81,8 @@ static void cubieboard_init(MachineState *machine)
 
 static void cubieboard_machine_init(MachineClass *mc)
 {
-    mc->desc = "cubietech cubieboard";
+    mc->desc = "cubietech cubieboard (Cortex-A9)";
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
     mc->init = cubieboard_init;
     mc->block_default_type = IF_IDE;
     mc->units_per_default_bus = 1;
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index 4f52465875..22434a65a2 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -37,7 +37,8 @@ static void digic_init(Object *obj)
     int i;
 
     object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
-                            "arm946-" TYPE_ARM_CPU, &error_abort, NULL);
+                            ARM_CPU_TYPE_NAME("arm946"),
+                            &error_abort, NULL);
 
     for (i = 0; i < DIGIC4_NB_TIMERS; i++) {
 #define DIGIC_TIMER_NAME_MLEN    11
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 532d088298..2b2fdb203a 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -36,7 +36,7 @@ static void fsl_imx25_init(Object *obj)
     FslIMX25State *s = FSL_IMX25(obj);
     int i;
 
-    object_initialize(&s->cpu, sizeof(s->cpu), "arm926-" TYPE_ARM_CPU);
+    object_initialize(&s->cpu, sizeof(s->cpu), ARM_CPU_TYPE_NAME("arm926"));
 
     sysbus_init_child_obj(obj, "avic", &s->avic, sizeof(s->avic),
                           TYPE_IMX_AVIC);
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 1a37a7b997..6760de3c8c 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -33,7 +33,7 @@ static void fsl_imx31_init(Object *obj)
     FslIMX31State *s = FSL_IMX31(obj);
     int i;
 
-    object_initialize(&s->cpu, sizeof(s->cpu), "arm1136-" TYPE_ARM_CPU);
+    object_initialize(&s->cpu, sizeof(s->cpu), ARM_CPU_TYPE_NAME("arm1136"));
 
     sysbus_init_child_obj(obj, "avic", &s->avic, sizeof(s->avic),
                           TYPE_IMX_AVIC);
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 8c397ef04b..552145b24e 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -43,7 +43,8 @@ static void fsl_imx6_init(Object *obj)
     for (i = 0; i < MIN(ms->smp.cpus, FSL_IMX6_NUM_CPUS); i++) {
         snprintf(name, NAME_SIZE, "cpu%d", i);
         object_initialize_child(obj, name, &s->cpu[i], sizeof(s->cpu[i]),
-                                "cortex-a9-" TYPE_ARM_CPU, &error_abort, NULL);
+                                ARM_CPU_TYPE_NAME("cortex-a9"),
+                                &error_abort, NULL);
     }
 
     sysbus_init_child_obj(obj, "a9mpcore", &s->a9mpcore, sizeof(s->a9mpcore),
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index b074177a71..c405b68d1d 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -34,7 +34,7 @@ static void fsl_imx6ul_init(Object *obj)
     int i;
 
     object_initialize_child(obj, "cpu0", &s->cpu, sizeof(s->cpu),
-                            "cortex-a7-" TYPE_ARM_CPU, &error_abort, NULL);
+                            ARM_CPU_TYPE_NAME("cortex-a7"), &error_abort, NULL);
 
     /*
      * A7MPCORE
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 0f587e63d3..fb03c60ebb 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -196,8 +196,8 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
 
         object_initialize_child(OBJECT(&s->rpu_cluster), "rpu-cpu[*]",
                                 &s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
-                                "cortex-r5f-" TYPE_ARM_CPU, &error_abort,
-                                NULL);
+                                ARM_CPU_TYPE_NAME("cortex-r5f"),
+                                &error_abort, NULL);
 
         name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
         if (strcmp(name, boot_cpu)) {
@@ -237,8 +237,8 @@ static void xlnx_zynqmp_init(Object *obj)
     for (i = 0; i < num_apus; i++) {
         object_initialize_child(OBJECT(&s->apu_cluster), "apu-cpu[*]",
                                 &s->apu_cpu[i], sizeof(s->apu_cpu[i]),
-                                "cortex-a53-" TYPE_ARM_CPU, &error_abort,
-                                NULL);
+                                ARM_CPU_TYPE_NAME("cortex-a53"),
+                                &error_abort, NULL);
     }
 
     sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic),
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 1/6] hw/arm: Use ARM_CPU_TYPE_NAME() macro when appropriate

[Richard Henderson]

On 8/23/19 7:32 AM, Philippe Mathieu-Daudé wrote:
> Commit ba1ba5cca introduce the ARM_CPU_TYPE_NAME() macro.
> Unify the code base by use it in all places.
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: rebased, use in imx25/31 (pm215)
> ---
>  hw/arm/allwinner-a10.c | 3 ++-
>  hw/arm/cubieboard.c    | 3 ++-
>  hw/arm/digic.c         | 3 ++-
>  hw/arm/fsl-imx25.c     | 2 +-
>  hw/arm/fsl-imx31.c     | 2 +-
>  hw/arm/fsl-imx6.c      | 3 ++-
>  hw/arm/fsl-imx6ul.c    | 2 +-
>  hw/arm/xlnx-zynqmp.c   | 8 ++++----
>  8 files changed, 15 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v2 2/6] hw/arm: Use object_initialize_child for correct reference counting

[Philippe Mathieu-Daudé]

As explained in commit aff39be0ed97:

  Both functions, object_initialize() and object_property_add_child()
  increase the reference counter of the new object, so one of the
  references has to be dropped afterwards to get the reference
  counting right. Otherwise the child object will not be properly
  cleaned up when the parent gets destroyed.
  Thus let's use now object_initialize_child() instead to get the
  reference counting here right.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/mcimx7d-sabre.c |  9 ++++-----
 hw/arm/mps2-tz.c       | 15 +++++++--------
 hw/arm/musca.c         |  9 +++++----
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
index 97b8bb788a..78b87c502f 100644
--- a/hw/arm/mcimx7d-sabre.c
+++ b/hw/arm/mcimx7d-sabre.c
@@ -30,7 +30,6 @@ static void mcimx7d_sabre_init(MachineState *machine)
 {
     static struct arm_boot_info boot_info;
     MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
-    Object *soc;
     int i;
 
     if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
@@ -49,10 +48,10 @@ static void mcimx7d_sabre_init(MachineState *machine)
         .nb_cpus = machine->smp.cpus,
     };
 
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
-    soc = OBJECT(&s->soc);
-    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
-    object_property_set_bool(soc, true, "realized", &error_fatal);
+    object_initialize_child(OBJECT(machine), "soc",
+                            &s->soc, sizeof(s->soc),
+                            TYPE_FSL_IMX7, &error_fatal, NULL);
+    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_fatal);
 
     memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
                                          machine->ram_size);
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index d85dc2c4bd..6b24aaacde 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -427,10 +427,10 @@ static void mps2tz_common_init(MachineState *machine)
     /* The sec_resp_cfg output from the IoTKit must be split into multiple
      * lines, one for each of the PPCs we create here, plus one per MSC.
      */
-    object_initialize(&mms->sec_resp_splitter, sizeof(mms->sec_resp_splitter),
-                      TYPE_SPLIT_IRQ);
-    object_property_add_child(OBJECT(machine), "sec-resp-splitter",
-                              OBJECT(&mms->sec_resp_splitter), &error_abort);
+    object_initialize_child(OBJECT(machine), "sec-resp-splitter",
+                            &mms->sec_resp_splitter,
+                            sizeof(mms->sec_resp_splitter),
+                            TYPE_SPLIT_IRQ, &error_abort, NULL);
     object_property_set_int(OBJECT(&mms->sec_resp_splitter),
                             ARRAY_SIZE(mms->ppc) + ARRAY_SIZE(mms->msc),
                             "num-lines", &error_fatal);
@@ -465,10 +465,9 @@ static void mps2tz_common_init(MachineState *machine)
      * Tx, Rx and "combined" IRQs are sent to the NVIC separately.
      * Create the OR gate for this.
      */
-    object_initialize(&mms->uart_irq_orgate, sizeof(mms->uart_irq_orgate),
-                      TYPE_OR_IRQ);
-    object_property_add_child(OBJECT(mms), "uart-irq-orgate",
-                              OBJECT(&mms->uart_irq_orgate), &error_abort);
+    object_initialize_child(OBJECT(mms), "uart-irq-orgate",
+                            &mms->uart_irq_orgate, sizeof(mms->uart_irq_orgate),
+                            TYPE_OR_IRQ, &error_abort, NULL);
     object_property_set_int(OBJECT(&mms->uart_irq_orgate), 10, "num-lines",
                             &error_fatal);
     object_property_set_bool(OBJECT(&mms->uart_irq_orgate), true,
diff --git a/hw/arm/musca.c b/hw/arm/musca.c
index ddd8842732..68db4b5b38 100644
--- a/hw/arm/musca.c
+++ b/hw/arm/musca.c
@@ -424,10 +424,11 @@ static void musca_init(MachineState *machine)
      * The sec_resp_cfg output from the SSE-200 must be split into multiple
      * lines, one for each of the PPCs we create here.
      */
-    object_initialize(&mms->sec_resp_splitter, sizeof(mms->sec_resp_splitter),
-                      TYPE_SPLIT_IRQ);
-    object_property_add_child(OBJECT(machine), "sec-resp-splitter",
-                              OBJECT(&mms->sec_resp_splitter), &error_fatal);
+    object_initialize_child(OBJECT(machine), "sec-resp-splitter",
+                            &mms->sec_resp_splitter,
+                            sizeof(mms->sec_resp_splitter),
+                            TYPE_SPLIT_IRQ, &error_fatal, NULL);
+
     object_property_set_int(OBJECT(&mms->sec_resp_splitter),
                             ARRAY_SIZE(mms->ppc), "num-lines", &error_fatal);
     object_property_set_bool(OBJECT(&mms->sec_resp_splitter), true,
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 2/6] hw/arm: Use object_initialize_child for correct reference counting

[Thomas Huth]

On 8/23/19 4:32 PM, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/arm/mcimx7d-sabre.c |  9 ++++-----
>  hw/arm/mps2-tz.c       | 15 +++++++--------
>  hw/arm/musca.c         |  9 +++++----
>  3 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
> index 97b8bb788a..78b87c502f 100644
> --- a/hw/arm/mcimx7d-sabre.c
> +++ b/hw/arm/mcimx7d-sabre.c
> @@ -30,7 +30,6 @@ static void mcimx7d_sabre_init(MachineState *machine)
>  {
>      static struct arm_boot_info boot_info;
>      MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
> -    Object *soc;
>      int i;
>  
>      if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
> @@ -49,10 +48,10 @@ static void mcimx7d_sabre_init(MachineState *machine)
>          .nb_cpus = machine->smp.cpus,
>      };
>  
> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
> -    soc = OBJECT(&s->soc);
> -    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
> -    object_property_set_bool(soc, true, "realized", &error_fatal);
> +    object_initialize_child(OBJECT(machine), "soc",
> +                            &s->soc, sizeof(s->soc),
> +                            TYPE_FSL_IMX7, &error_fatal, NULL);

You could fit that into two instead of three lines.

But it's just a cosmetic nit, so:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/6] hw/arm: Use object_initialize_child for correct reference counting

[Richard Henderson]

On 8/23/19 7:32 AM, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/arm/mcimx7d-sabre.c |  9 ++++-----
>  hw/arm/mps2-tz.c       | 15 +++++++--------
>  hw/arm/musca.c         |  9 +++++----
>  3 files changed, 16 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v2 3/6] hw/arm: Use sysbus_init_child_obj for correct reference counting

[Philippe Mathieu-Daudé]

As explained in commit aff39be0ed97:

  Both functions, object_initialize() and qdev_set_parent_bus()
  increase the reference counter of the new object, so one of the
  references has to be dropped afterwards to get the reference
  counting right. Otherwise the child object will not be properly
  cleaned up when the parent gets destroyed.
  Thus let's use now sysbus_init_child_obj() instead to get the
  reference counting here right.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Corrected commit description (pm215)
---
 hw/arm/exynos4_boards.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index f69358a5ba..2781d8bd41 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -131,8 +131,8 @@ exynos4_boards_init_common(MachineState *machine,
     exynos4_boards_init_ram(s, get_system_memory(),
                             exynos4_board_ram_size[board_type]);
 
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_EXYNOS4210_SOC);
-    qdev_set_parent_bus(DEVICE(&s->soc), sysbus_get_default());
+    sysbus_init_child_obj(OBJECT(machine), "soc",
+                          &s->soc, sizeof(s->soc), TYPE_EXYNOS4210_SOC);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
                              &error_fatal);
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 3/6] hw/arm: Use sysbus_init_child_obj for correct reference counting

[Thomas Huth]

On 8/23/19 4:32 PM, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and qdev_set_parent_bus()

Commit aff39be0ed97 was not about qdev_set_parent_bus(), so the first
sentence sounds somewhat misleading here. Maybe rephrase the commit
message without that reference to aff39be0ed97 ?

>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.

Well, the parent here (the machine) currently never gets destroyed ...
so unless you've got a patch in your pipe to fix that, too, you should
maybe also rephrase this part of the commit message.

 Thomas

Re: [Qemu-devel] [PATCH v2 3/6] hw/arm: Use sysbus_init_child_obj for correct reference counting

[Peter Maydell]

On Fri, 23 Aug 2019 at 16:26, Thomas Huth <thuth@redhat.com> wrote:
>
> On 8/23/19 4:32 PM, Philippe Mathieu-Daudé wrote:
> > As explained in commit aff39be0ed97:
> >
> >   Both functions, object_initialize() and qdev_set_parent_bus()
>
> Commit aff39be0ed97 was not about qdev_set_parent_bus(), so the first
> sentence sounds somewhat misleading here. Maybe rephrase the commit
> message without that reference to aff39be0ed97 ?
>
> >   increase the reference counter of the new object, so one of the
> >   references has to be dropped afterwards to get the reference
> >   counting right. Otherwise the child object will not be properly
> >   cleaned up when the parent gets destroyed.
>
> Well, the parent here (the machine) currently never gets destroyed ...
> so unless you've got a patch in your pipe to fix that, too, you should
> maybe also rephrase this part of the commit message.

How about just making the commit message:

Both object_initialize() and qdev_set_parent_bus() increase the
reference counter of the new object, so one of the references has
to be dropped afterwards to get the reference counting right.
In machine model code this refcount leak is not particularly
problematic because (unlike devices) machines will never be
created on demand via QMP, and they are never destroyed.
But in any case let's use the new sysbus_init_child_obj() instead
to get the reference counting here right.

?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/6] hw/arm: Use sysbus_init_child_obj for correct reference counting

[Richard Henderson]

On 8/23/19 7:32 AM, Philippe Mathieu-Daudé wrote:
> diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
> index f69358a5ba..2781d8bd41 100644
> --- a/hw/arm/exynos4_boards.c
> +++ b/hw/arm/exynos4_boards.c
> @@ -131,8 +131,8 @@ exynos4_boards_init_common(MachineState *machine,
>      exynos4_boards_init_ram(s, get_system_memory(),
>                              exynos4_board_ram_size[board_type]);
>  
> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_EXYNOS4210_SOC);
> -    qdev_set_parent_bus(DEVICE(&s->soc), sysbus_get_default());
> +    sysbus_init_child_obj(OBJECT(machine), "soc",
> +                          &s->soc, sizeof(s->soc), TYPE_EXYNOS4210_SOC);
>      object_property_set_bool(OBJECT(&s->soc), true, "realized",
>                               &error_fatal);

Thomas' comments re the commit message notwithstanding,
the patch itself is correct.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v2 4/6] hw/arm/fsl-imx: Add the cpu as child of the SoC object

[Philippe Mathieu-Daudé]

Child properties form the composition tree. All objects need to be
a child of another object. Objects can only be a child of one object.

Respect this with the i.MX SoC, to get a cleaner composition tree.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: reworded commit description with Markus suggestions
---
 hw/arm/fsl-imx25.c | 4 +++-
 hw/arm/fsl-imx31.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 2b2fdb203a..3cb5a8fdfd 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -36,7 +36,9 @@ static void fsl_imx25_init(Object *obj)
     FslIMX25State *s = FSL_IMX25(obj);
     int i;
 
-    object_initialize(&s->cpu, sizeof(s->cpu), ARM_CPU_TYPE_NAME("arm926"));
+    object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
+                            ARM_CPU_TYPE_NAME("arm926"),
+                            &error_abort, NULL);
 
     sysbus_init_child_obj(obj, "avic", &s->avic, sizeof(s->avic),
                           TYPE_IMX_AVIC);
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 6760de3c8c..55e90d104b 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -33,7 +33,9 @@ static void fsl_imx31_init(Object *obj)
     FslIMX31State *s = FSL_IMX31(obj);
     int i;
 
-    object_initialize(&s->cpu, sizeof(s->cpu), ARM_CPU_TYPE_NAME("arm1136"));
+    object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
+                            ARM_CPU_TYPE_NAME("arm1136"),
+                            &error_abort, NULL);
 
     sysbus_init_child_obj(obj, "avic", &s->avic, sizeof(s->avic),
                           TYPE_IMX_AVIC);
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 4/6] hw/arm/fsl-imx: Add the cpu as child of the SoC object

[Richard Henderson]

On 8/23/19 7:32 AM, Philippe Mathieu-Daudé wrote:
> Child properties form the composition tree. All objects need to be
> a child of another object. Objects can only be a child of one object.
> 
> Respect this with the i.MX SoC, to get a cleaner composition tree.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: reworded commit description with Markus suggestions
> ---
>  hw/arm/fsl-imx25.c | 4 +++-
>  hw/arm/fsl-imx31.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v2 5/6] hw/dma/xilinx_axi: Use object_initialize_child for correct ref. counting

[Philippe Mathieu-Daudé]

As explained in commit aff39be0ed97:

  Both functions, object_initialize() and object_property_add_child()
  increase the reference counter of the new object, so one of the
  references has to be dropped afterwards to get the reference
  counting right. Otherwise the child object will not be properly
  cleaned up when the parent gets destroyed.
  Thus let's use now object_initialize_child() instead to get the
  reference counting here right.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: fix typo ENET -> DMA in TYPE macro name
---
 hw/dma/xilinx_axidma.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index d176df6d44..a254275b64 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -566,14 +566,14 @@ static void xilinx_axidma_init(Object *obj)
     XilinxAXIDMA *s = XILINX_AXI_DMA(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
-                      TYPE_XILINX_AXI_DMA_DATA_STREAM);
-    object_initialize(&s->rx_control_dev, sizeof(s->rx_control_dev),
-                      TYPE_XILINX_AXI_DMA_CONTROL_STREAM);
-    object_property_add_child(OBJECT(s), "axistream-connected-target",
-                              (Object *)&s->rx_data_dev, &error_abort);
-    object_property_add_child(OBJECT(s), "axistream-control-connected-target",
-                              (Object *)&s->rx_control_dev, &error_abort);
+    object_initialize_child(OBJECT(s), "axistream-connected-target",
+                            &s->rx_data_dev, sizeof(s->rx_data_dev),
+                            TYPE_XILINX_AXI_DMA_DATA_STREAM, &error_abort,
+                            NULL);
+    object_initialize_child(OBJECT(s), "axistream-control-connected-target",
+                            &s->rx_control_dev, sizeof(s->rx_control_dev),
+                            TYPE_XILINX_AXI_DMA_CONTROL_STREAM, &error_abort,
+                            NULL);
 
     sysbus_init_irq(sbd, &s->streams[0].irq);
     sysbus_init_irq(sbd, &s->streams[1].irq);
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 5/6] hw/dma/xilinx_axi: Use object_initialize_child for correct ref. counting

[Richard Henderson]

On 8/23/19 7:32 AM, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: fix typo ENET -> DMA in TYPE macro name
> ---
>  hw/dma/xilinx_axidma.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 5/6] hw/dma/xilinx_axi: Use object_initialize_child for correct ref. counting

[Thomas Huth]

On 8/23/19 4:32 PM, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: fix typo ENET -> DMA in TYPE macro name
> ---
>  hw/dma/xilinx_axidma.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index d176df6d44..a254275b64 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -566,14 +566,14 @@ static void xilinx_axidma_init(Object *obj)
>      XilinxAXIDMA *s = XILINX_AXI_DMA(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> -    object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
> -                      TYPE_XILINX_AXI_DMA_DATA_STREAM);
> -    object_initialize(&s->rx_control_dev, sizeof(s->rx_control_dev),
> -                      TYPE_XILINX_AXI_DMA_CONTROL_STREAM);
> -    object_property_add_child(OBJECT(s), "axistream-connected-target",
> -                              (Object *)&s->rx_data_dev, &error_abort);
> -    object_property_add_child(OBJECT(s), "axistream-control-connected-target",
> -                              (Object *)&s->rx_control_dev, &error_abort);
> +    object_initialize_child(OBJECT(s), "axistream-connected-target",
> +                            &s->rx_data_dev, sizeof(s->rx_data_dev),
> +                            TYPE_XILINX_AXI_DMA_DATA_STREAM, &error_abort,
> +                            NULL);
> +    object_initialize_child(OBJECT(s), "axistream-control-connected-target",
> +                            &s->rx_control_dev, sizeof(s->rx_control_dev),
> +                            TYPE_XILINX_AXI_DMA_CONTROL_STREAM, &error_abort,
> +                            NULL);
>  
>      sysbus_init_irq(sbd, &s->streams[0].irq);
>      sysbus_init_irq(sbd, &s->streams[1].irq);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v2 6/6] hw/net/xilinx_axi: Use object_initialize_child for correct ref. counting

[Philippe Mathieu-Daudé]

As explained in commit aff39be0ed97:

  Both functions, object_initialize() and object_property_add_child()
  increase the reference counter of the new object, so one of the
  references has to be dropped afterwards to get the reference
  counting right. Otherwise the child object will not be properly
  cleaned up when the parent gets destroyed.
  Thus let's use now object_initialize_child() instead to get the
  reference counting here right.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/xilinx_axienet.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index d8716a1f73..2c8c065401 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -994,15 +994,14 @@ static void xilinx_enet_init(Object *obj)
     XilinxAXIEnet *s = XILINX_AXI_ENET(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
-                      TYPE_XILINX_AXI_ENET_DATA_STREAM);
-    object_initialize(&s->rx_control_dev, sizeof(s->rx_control_dev),
-                      TYPE_XILINX_AXI_ENET_CONTROL_STREAM);
-    object_property_add_child(OBJECT(s), "axistream-connected-target",
-                              (Object *)&s->rx_data_dev, &error_abort);
-    object_property_add_child(OBJECT(s), "axistream-control-connected-target",
-                              (Object *)&s->rx_control_dev, &error_abort);
-
+    object_initialize_child(OBJECT(s), "axistream-connected-target",
+                            &s->rx_data_dev, sizeof(s->rx_data_dev),
+                            TYPE_XILINX_AXI_ENET_DATA_STREAM, &error_abort,
+                            NULL);
+    object_initialize_child(OBJECT(s), "axistream-control-connected-target",
+                            &s->rx_control_dev, sizeof(s->rx_control_dev),
+                            TYPE_XILINX_AXI_ENET_CONTROL_STREAM, &error_abort,
+                            NULL);
     sysbus_init_irq(sbd, &s->irq);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &enet_ops, s, "enet", 0x40000);
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 6/6] hw/net/xilinx_axi: Use object_initialize_child for correct ref. counting

[Richard Henderson]

On 8/23/19 7:32 AM, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/net/xilinx_axienet.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 6/6] hw/net/xilinx_axi: Use object_initialize_child for correct ref. counting

[Thomas Huth]

On 8/23/19 4:32 PM, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/net/xilinx_axienet.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index d8716a1f73..2c8c065401 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -994,15 +994,14 @@ static void xilinx_enet_init(Object *obj)
>      XilinxAXIEnet *s = XILINX_AXI_ENET(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> -    object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
> -                      TYPE_XILINX_AXI_ENET_DATA_STREAM);
> -    object_initialize(&s->rx_control_dev, sizeof(s->rx_control_dev),
> -                      TYPE_XILINX_AXI_ENET_CONTROL_STREAM);
> -    object_property_add_child(OBJECT(s), "axistream-connected-target",
> -                              (Object *)&s->rx_data_dev, &error_abort);
> -    object_property_add_child(OBJECT(s), "axistream-control-connected-target",
> -                              (Object *)&s->rx_control_dev, &error_abort);
> -
> +    object_initialize_child(OBJECT(s), "axistream-connected-target",
> +                            &s->rx_data_dev, sizeof(s->rx_data_dev),
> +                            TYPE_XILINX_AXI_ENET_DATA_STREAM, &error_abort,
> +                            NULL);
> +    object_initialize_child(OBJECT(s), "axistream-control-connected-target",
> +                            &s->rx_control_dev, sizeof(s->rx_control_dev),
> +                            TYPE_XILINX_AXI_ENET_CONTROL_STREAM, &error_abort,
> +                            NULL);
>      sysbus_init_irq(sbd, &s->irq);
>  
>      memory_region_init_io(&s->iomem, OBJECT(s), &enet_ops, s, "enet", 0x40000);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/6] hw/arm: Use ARM_CPU_TYPE_NAME() and object_initialize_child()

[Peter Maydell]

On Fri, 23 Aug 2019 at 15:33, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> First we use ARM_CPU_TYPE_NAME() when we should.
>
> Then is follow up of [1]:
>
>   This series looks at Eduardo suggestions from [2]
>   and Thomas commit aff39be0ed97 to replace various
>   object_initialize + qdev_set_parent_bus calls by
>   sysbus_init_child_obj().
>
> Finally, some devices are declared orphean while they have a parent,
> let them be together again.
>
> Since v1 [3]:
> - addressed Peter Maydell review comments
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01492.html
> [2] https://patchwork.ozlabs.org/patch/943333/#1953608
> [3] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00135.html
>
> Philippe Mathieu-Daudé (6):
>   hw/arm: Use ARM_CPU_TYPE_NAME() macro when appropriate
>   hw/arm: Use object_initialize_child for correct reference counting
>   hw/arm: Use sysbus_init_child_obj for correct reference counting
>   hw/arm/fsl-imx: Add the cpu as child of the SoC object
>   hw/dma/xilinx_axi: Use object_initialize_child for correct ref.
>     counting
>   hw/net/xilinx_axi: Use object_initialize_child for correct ref.
>     counting

This series is now in master (but I forgot to mention that I'd
applied it to target-arm.next when I did that).

thanks
-- PMM