Re: [PULL 04/49] hw: Add QOM parentship relation with CPUs

[Igor Mammedov]

On Sun, 12 Jan 2025 23:16:40 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> QDev objects created with object_new() need to manually add
> their parent relationship with object_property_add_child().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Message-Id: <20240216110313.17039-22-philmd@linaro.org>
> ---
>  hw/i386/x86-common.c                     | 1 +
>  hw/microblaze/petalogix_ml605_mmu.c      | 1 +
>  hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 +
>  hw/mips/cps.c                            | 1 +
>  hw/ppc/e500.c                            | 1 +
>  hw/ppc/spapr.c                           | 1 +
>  6 files changed, 6 insertions(+)
> 
> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> index 97b4f7d4a0d..9c9ffb3484a 100644
> --- a/hw/i386/x86-common.c
> +++ b/hw/i386/x86-common.c
> @@ -60,6 +60,7 @@ static void x86_cpu_new(X86MachineState *x86ms, int64_t 
> apic_id, Error **errp)
>      if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
>          goto out;
>      }
> +    object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu));

I might  be missing something but why it needs to be done manually?

device_set_realized() will place any parent-less device under (1) 
/machine/unattached
while devices created with device_add() are be placed under 
/machine/peripheral[-anon]

The commit message unfortunately doesn't explain why [1] shall be replaced
by direct cpu[*] array property directly under machine.
 
Granted, those paths aren't any kind of ABI and wrt x86 cpus
nothing should break (or I'd say it shouldn't break our promises) 
But I'd rather not do this without a good reason/explanation.

>      qdev_realize(DEVICE(cpu), NULL, errp);
>  
>  out:
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
> b/hw/microblaze/petalogix_ml605_mmu.c
> index 8b44be75a22..b6be40915ac 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -83,6 +83,7 @@ petalogix_ml605_init(MachineState *machine)
>  
>      /* init CPUs */
>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
> +    object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu));
>      object_property_set_str(OBJECT(cpu), "version", "8.10.a", &error_abort);
>      /* Use FPU but don't use floating point conversion and square
>       * root instructions
> diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c 
> b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> index 2c0d8c34cd2..29629310ba2 100644
> --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
> +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> @@ -73,6 +73,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>      MemoryRegion *sysmem = get_system_memory();
>  
>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
> +    object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu));
>      object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort);
>      object_property_set_bool(OBJECT(cpu), "little-endian",
>                               !TARGET_BIG_ENDIAN, &error_abort);
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 0d8cbdc8924..293b405b965 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -87,6 +87,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>          /* All cores use the same clock tree */
>          qdev_connect_clock_in(DEVICE(cpu), "clk-in", s->clock);
>  
> +        object_property_add_child(OBJECT(dev), "cpu[*]", OBJECT(cpu));
>          if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) {
>              return;
>          }
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 4551157c011..17d63ced907 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -955,6 +955,7 @@ void ppce500_init(MachineState *machine)
>           */
>          object_property_set_bool(OBJECT(cs), "start-powered-off", i != 0,
>                                   &error_abort);
> +        object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cpu));
>          qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>  
>          if (!firstenv) {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 623842f8064..125be6d29fd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2705,6 +2705,7 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
>                                      &error_fatal);
>              object_property_set_int(core, CPU_CORE_PROP_CORE_ID, core_id,
>                                      &error_fatal);
> +            object_property_add_child(OBJECT(spapr), "cpu[*]", OBJECT(core));
>              qdev_realize(DEVICE(core), NULL, &error_fatal);
>  
>              object_unref(core);