[RFC PATCH 0/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[RFC PATCH 0/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Philippe Mathieu-Daudé]

Following the discussion with Peter on my "cpus: Step toward
removing global 'first_cpu'" series [*], we now pass the array
of CPUs via property. Since we can not pass array of "link"
properties, we pass the QOM path of each CPU as a QList(String).

Tagged as RFC to discuss the idea of using qdev_prop_set_array
with qlist_append_str(object_get_canonical_path). Personally I
find it super simple.

Regards,

Phil.

[*] https://lore.kernel.org/qemu-devel/CAFEAcA9FT+QMyQSLCeLjd7tEfaoS9JazmkYWQE++s1AmF7Nfvw@mail.gmail.com/

Kevin Wolf (1):
  qdev: Add qdev_prop_set_array()

Philippe Mathieu-Daudé (4):
  hw/ppc/e500: Declare CPU QOM types using DEFINE_TYPES() macro
  hw/ppc/e500: QOM-attach CPUs to the machine container
  hw/ppc/e500: Inline sysbus_create_simple(E500_SPIN)
  hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

 include/hw/qdev-properties.h |  3 ++
 hw/core/qdev-properties.c    | 21 +++++++++++
 hw/ppc/e500.c                | 11 +++++-
 hw/ppc/ppce500_spin.c        | 69 ++++++++++++++++++++++++++----------
 4 files changed, 84 insertions(+), 20 deletions(-)

-- 
2.41.0

[PATCH 1/5] qdev: Add qdev_prop_set_array()

[Philippe Mathieu-Daudé]

From: Kevin Wolf <kwolf@redhat.com>

Instead of exposing the ugly hack of how we represent arrays in qdev (a
static "foo-len" property and after it is set, dynamically created
"foo[i]" properties) to boards, add an interface that allows setting the
whole array at once.

Once all internal users of devices with array properties have been
converted to use this function, we can change the implementation to move
away from this hack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20231030114802.3671871-4-peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/qdev-properties.h |  3 +++
 hw/core/qdev-properties.c    | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index e1df08876c..7fa2fdb7c9 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -206,6 +206,9 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
                            const uint8_t *value);
 void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
 
+/* Takes ownership of @values */
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values);
+
 void *object_field_prop_ptr(Object *obj, Property *prop);
 
 void qdev_prop_register_global(GlobalProperty *prop);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 357b8761b5..950ef48e01 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -3,12 +3,14 @@
 #include "qapi/error.h"
 #include "qapi/qapi-types-misc.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qlist.h"
 #include "qemu/ctype.h"
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
 #include "qemu/units.h"
 #include "qemu/cutils.h"
 #include "qdev-prop-internal.h"
+#include "qom/qom-qobject.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
@@ -739,6 +741,25 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
                             &error_abort);
 }
 
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
+{
+    const QListEntry *entry;
+    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
+    uint32_t i = 0;
+
+    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
+                            &error_abort);
+
+    QLIST_FOREACH_ENTRY(values, entry) {
+        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
+        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
+                                    &error_abort);
+        i++;
+    }
+
+    qobject_unref(values);
+}
+
 static GPtrArray *global_props(void)
 {
     static GPtrArray *gp;
-- 
2.41.0

[PATCH 2/5] hw/ppc/e500: Declare CPU QOM types using DEFINE_TYPES() macro

[Philippe Mathieu-Daudé]

When multiple QOM types are registered in the same file,
it is simpler to use the the DEFINE_TYPES() macro. In
particular because type array declared with such macro
are easier to review.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
I'm going to do that for each file I modify, so eventually
we'll get all converted.
---
 hw/ppc/ppce500_spin.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
index bbce63e8a4..e3608d8c16 100644
--- a/hw/ppc/ppce500_spin.c
+++ b/hw/ppc/ppce500_spin.c
@@ -195,17 +195,14 @@ static void ppce500_spin_class_init(ObjectClass *klass, void *data)
     dc->reset = spin_reset;
 }
 
-static const TypeInfo ppce500_spin_info = {
-    .name          = TYPE_E500_SPIN,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(SpinState),
-    .instance_init = ppce500_spin_initfn,
-    .class_init    = ppce500_spin_class_init,
+static const TypeInfo ppce500_spin_types[] = {
+    {
+        .name           = TYPE_E500_SPIN,
+        .parent         = TYPE_SYS_BUS_DEVICE,
+        .instance_size  = sizeof(SpinState),
+        .instance_init  = ppce500_spin_initfn,
+        .class_init     = ppce500_spin_class_init,
+    },
 };
 
-static void ppce500_spin_register_types(void)
-{
-    type_register_static(&ppce500_spin_info);
-}
-
-type_init(ppce500_spin_register_types)
+DEFINE_TYPES(ppce500_spin_types)
-- 
2.41.0

[PATCH 3/5] hw/ppc/e500: QOM-attach CPUs to the machine container

[Philippe Mathieu-Daudé]

Instead of having CPUs dangling in the /unattached/device
bucket, attach them to the machine container.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/e500.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index e04114fb3c..f8177c0280 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -946,6 +946,7 @@ void ppce500_init(MachineState *machine)
             exit(1);
         }
 
+        object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cs));
         /*
          * Secondary CPU starts in halted state for now. Needs to change
          * when implementing non-kernel boot.
-- 
2.41.0

[PATCH 4/5] hw/ppc/e500: Inline sysbus_create_simple(E500_SPIN)

[Philippe Mathieu-Daudé]

In the next commit we'll set properties to the TYPE_E500_SPIN
object. In order to ease next commit review, inline the
sysbus_create_simple() call first.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/e500.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index f8177c0280..e38f46df38 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1082,7 +1082,9 @@ void ppce500_init(MachineState *machine)
     }
 
     /* Register spinning region */
-    sysbus_create_simple("e500-spin", pmc->spin_base, NULL);
+    dev = qdev_new("e500-spin");
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, pmc->spin_base);
 
     if (pmc->has_mpc8xxx_gpio) {
         qemu_irq poweroff_irq;
-- 
2.41.0

[RFC PATCH 5/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Philippe Mathieu-Daudé]

Devices should avoid calling qemu_get_cpu() because this call
doesn't work as expected with heterogeneous machines. Such
devices often iterate over a cluster of CPUs, which the device's
parent has direct access (when creating the child device).

We can pass QOM as 'link' between objects, but we can't pass an
array of links. Here we exploits QAPI simplicity, by using
DEFINE_PROP_ARRAY and a list of strings, each string being the
CPU canonical path in QOM tree (which is constant and unique).
When the device realizes itself, the original CPU pointer is
recovered via a object_resolve_path() call.

Inspired-by: Peter Maydell <peter.maydell@linaro.org>
Inspired-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Tested with:
$ make check-qtest-ppc{,64}
$ make check-avocado AVOCADO_TAGS='machine:ppce500 machine:mpc8544ds'

RFC: See cover

FIXME: Should we free spin_cpu_list using g_autoptr(QList)?
---
 hw/ppc/e500.c         |  6 ++++++
 hw/ppc/ppce500_spin.c | 48 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index e38f46df38..8b31143dca 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -18,6 +18,7 @@
 #include "qemu/datadir.h"
 #include "qemu/units.h"
 #include "qemu/guest-random.h"
+#include "qapi/qmp/qlist.h"
 #include "qapi/error.h"
 #include "e500.h"
 #include "e500-ccsr.h"
@@ -930,11 +931,13 @@ void ppce500_init(MachineState *machine)
     SysBusDevice *s;
     PPCE500CCSRState *ccsr;
     I2CBus *i2c;
+    QList *spin_cpu_list = qlist_new();
 
     irqs = g_new0(IrqLines, smp_cpus);
     for (i = 0; i < smp_cpus; i++) {
         PowerPCCPU *cpu;
         CPUState *cs;
+        g_autofree char *cpu_qompath;
 
         cpu = POWERPC_CPU(object_new(machine->cpu_type));
         env = &cpu->env;
@@ -954,6 +957,8 @@ void ppce500_init(MachineState *machine)
         object_property_set_bool(OBJECT(cs), "start-powered-off", i != 0,
                                  &error_fatal);
         qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
+        cpu_qompath = object_get_canonical_path(OBJECT(cs));
+        qlist_append_str(spin_cpu_list, cpu_qompath);
 
         if (!firstenv) {
             firstenv = env;
@@ -1083,6 +1088,7 @@ void ppce500_init(MachineState *machine)
 
     /* Register spinning region */
     dev = qdev_new("e500-spin");
+    qdev_prop_set_array(dev, "cpus-qom-path", spin_cpu_list);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, pmc->spin_base);
 
diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
index e3608d8c16..a67046b2ea 100644
--- a/hw/ppc/ppce500_spin.c
+++ b/hw/ppc/ppce500_spin.c
@@ -30,11 +30,13 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/units.h"
+#include "qapi/error.h"
 #include "hw/hw.h"
 #include "hw/sysbus.h"
 #include "sysemu/hw_accel.h"
 #include "e500.h"
 #include "qom/object.h"
+#include "hw/qdev-properties.h"
 
 #define MAX_CPUS 32
 
@@ -46,6 +48,10 @@ typedef struct spin_info {
     uint64_t reserved;
 } QEMU_PACKED SpinInfo;
 
+/*
+ * QEMU interface:
+ *  + QOM array property "cpus-qom-path": QOM canonical path of each CPU.
+ */
 #define TYPE_E500_SPIN "e500-spin"
 OBJECT_DECLARE_SIMPLE_TYPE(SpinState, E500_SPIN)
 
@@ -54,6 +60,9 @@ struct SpinState {
 
     MemoryRegion iomem;
     SpinInfo spin[MAX_CPUS];
+    uint32_t cpu_count;
+    char **cpu_canonical_path;
+    CPUState **cpu;
 };
 
 static void spin_reset(DeviceState *dev)
@@ -121,16 +130,10 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
 {
     SpinState *s = opaque;
     int env_idx = addr / sizeof(SpinInfo);
-    CPUState *cpu;
+    CPUState *cpu = s->cpu[env_idx];
     SpinInfo *curspin = &s->spin[env_idx];
     uint8_t *curspin_p = (uint8_t*)curspin;
 
-    cpu = qemu_get_cpu(env_idx);
-    if (cpu == NULL) {
-        /* Unknown CPU */
-        return;
-    }
-
     if (cpu->cpu_index == 0) {
         /* primary CPU doesn't spin */
         return;
@@ -188,11 +191,42 @@ static void ppce500_spin_initfn(Object *obj)
     sysbus_init_mmio(dev, &s->iomem);
 }
 
+static void ppce500_spin_realize(DeviceState *dev, Error **errp)
+{
+    SpinState *s = E500_SPIN(dev);
+
+    if (s->cpu_count == 0) {
+        error_setg(errp, "'cpus-qom-path' property array must be set");
+        return;
+    } else if (s->cpu_count > MAX_CPUS) {
+        error_setg(errp, "at most %d CPUs are supported", MAX_CPUS);
+        return;
+    }
+
+    s->cpu = g_new(CPUState *, s->cpu_count);
+    for (unsigned i = 0; i < s->cpu_count; i++) {
+        bool ambiguous;
+        Object *obj;
+
+        obj = object_resolve_path(s->cpu_canonical_path[i], &ambiguous);
+        assert(!ambiguous);
+        s->cpu[i] = CPU(obj);
+    }
+}
+
+static Property ppce500_spin_properties[] = {
+    DEFINE_PROP_ARRAY("cpus-qom-path", SpinState, cpu_count,
+                      cpu_canonical_path, qdev_prop_string, char *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ppce500_spin_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->reset = spin_reset;
+    dc->realize = ppce500_spin_realize;
+    device_class_set_props(dc, ppce500_spin_properties);
 }
 
 static const TypeInfo ppce500_spin_types[] = {
-- 
2.41.0

Re: [PATCH 2/5] hw/ppc/e500: Declare CPU QOM types using DEFINE_TYPES() macro

[Daniel Henrique Barboza]

On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
> When multiple QOM types are registered in the same file,
> it is simpler to use the the DEFINE_TYPES() macro. In
> particular because type array declared with such macro
> are easier to review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

> I'm going to do that for each file I modify, so eventually
> we'll get all converted.
> ---
>   hw/ppc/ppce500_spin.c | 21 +++++++++------------
>   1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> index bbce63e8a4..e3608d8c16 100644
> --- a/hw/ppc/ppce500_spin.c
> +++ b/hw/ppc/ppce500_spin.c
> @@ -195,17 +195,14 @@ static void ppce500_spin_class_init(ObjectClass *klass, void *data)
>       dc->reset = spin_reset;
>   }
>   
> -static const TypeInfo ppce500_spin_info = {
> -    .name          = TYPE_E500_SPIN,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(SpinState),
> -    .instance_init = ppce500_spin_initfn,
> -    .class_init    = ppce500_spin_class_init,
> +static const TypeInfo ppce500_spin_types[] = {
> +    {
> +        .name           = TYPE_E500_SPIN,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(SpinState),
> +        .instance_init  = ppce500_spin_initfn,
> +        .class_init     = ppce500_spin_class_init,
> +    },
>   };
>   
> -static void ppce500_spin_register_types(void)
> -{
> -    type_register_static(&ppce500_spin_info);
> -}
> -
> -type_init(ppce500_spin_register_types)
> +DEFINE_TYPES(ppce500_spin_types)

Re: [PATCH 3/5] hw/ppc/e500: QOM-attach CPUs to the machine container

[Daniel Henrique Barboza]

On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
> Instead of having CPUs dangling in the /unattached/device
> bucket, attach them to the machine container.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   hw/ppc/e500.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e04114fb3c..f8177c0280 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -946,6 +946,7 @@ void ppce500_init(MachineState *machine)
>               exit(1);
>           }
>   
> +        object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cs));
>           /*
>            * Secondary CPU starts in halted state for now. Needs to change
>            * when implementing non-kernel boot.

Re: [PATCH 4/5] hw/ppc/e500: Inline sysbus_create_simple(E500_SPIN)

[Daniel Henrique Barboza]

On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
> In the next commit we'll set properties to the TYPE_E500_SPIN
> object. In order to ease next commit review, inline the
> sysbus_create_simple() call first.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   hw/ppc/e500.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index f8177c0280..e38f46df38 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -1082,7 +1082,9 @@ void ppce500_init(MachineState *machine)
>       }
>   
>       /* Register spinning region */
> -    sysbus_create_simple("e500-spin", pmc->spin_base, NULL);
> +    dev = qdev_new("e500-spin");
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, pmc->spin_base);
>   
>       if (pmc->has_mpc8xxx_gpio) {
>           qemu_irq poweroff_irq;

Re: [RFC PATCH 5/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Daniel Henrique Barboza]

On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
> Devices should avoid calling qemu_get_cpu() because this call
> doesn't work as expected with heterogeneous machines. Such
> devices often iterate over a cluster of CPUs, which the device's
> parent has direct access (when creating the child device).
> 
> We can pass QOM as 'link' between objects, but we can't pass an
> array of links. Here we exploits QAPI simplicity, by using
> DEFINE_PROP_ARRAY and a list of strings, each string being the
> CPU canonical path in QOM tree (which is constant and unique).
> When the device realizes itself, the original CPU pointer is
> recovered via a object_resolve_path() call.
> 
> Inspired-by: Peter Maydell <peter.maydell@linaro.org>
> Inspired-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Tested with:
> $ make check-qtest-ppc{,64}
> $ make check-avocado AVOCADO_TAGS='machine:ppce500 machine:mpc8544ds'
> 
> RFC: See cover
> 
> FIXME: Should we free spin_cpu_list using g_autoptr(QList)?

By looking at how object_property_set_qobject() works I *think* we can free it.
Perhaps try using g_autofree and see if something explodes, hehe

Thanks,

Daniel

> ---
>   hw/ppc/e500.c         |  6 ++++++
>   hw/ppc/ppce500_spin.c | 48 ++++++++++++++++++++++++++++++++++++-------
>   2 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e38f46df38..8b31143dca 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -18,6 +18,7 @@
>   #include "qemu/datadir.h"
>   #include "qemu/units.h"
>   #include "qemu/guest-random.h"
> +#include "qapi/qmp/qlist.h"
>   #include "qapi/error.h"
>   #include "e500.h"
>   #include "e500-ccsr.h"
> @@ -930,11 +931,13 @@ void ppce500_init(MachineState *machine)
>       SysBusDevice *s;
>       PPCE500CCSRState *ccsr;
>       I2CBus *i2c;
> +    QList *spin_cpu_list = qlist_new();
>   
>       irqs = g_new0(IrqLines, smp_cpus);
>       for (i = 0; i < smp_cpus; i++) {
>           PowerPCCPU *cpu;
>           CPUState *cs;
> +        g_autofree char *cpu_qompath;
>   
>           cpu = POWERPC_CPU(object_new(machine->cpu_type));
>           env = &cpu->env;
> @@ -954,6 +957,8 @@ void ppce500_init(MachineState *machine)
>           object_property_set_bool(OBJECT(cs), "start-powered-off", i != 0,
>                                    &error_fatal);
>           qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
> +        cpu_qompath = object_get_canonical_path(OBJECT(cs));
> +        qlist_append_str(spin_cpu_list, cpu_qompath);
>   
>           if (!firstenv) {
>               firstenv = env;
> @@ -1083,6 +1088,7 @@ void ppce500_init(MachineState *machine)
>   
>       /* Register spinning region */
>       dev = qdev_new("e500-spin");
> +    qdev_prop_set_array(dev, "cpus-qom-path", spin_cpu_list);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, pmc->spin_base);
>   
> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> index e3608d8c16..a67046b2ea 100644
> --- a/hw/ppc/ppce500_spin.c
> +++ b/hw/ppc/ppce500_spin.c
> @@ -30,11 +30,13 @@
>   #include "qemu/osdep.h"
>   #include "qemu/module.h"
>   #include "qemu/units.h"
> +#include "qapi/error.h"
>   #include "hw/hw.h"
>   #include "hw/sysbus.h"
>   #include "sysemu/hw_accel.h"
>   #include "e500.h"
>   #include "qom/object.h"
> +#include "hw/qdev-properties.h"
>   
>   #define MAX_CPUS 32
>   
> @@ -46,6 +48,10 @@ typedef struct spin_info {
>       uint64_t reserved;
>   } QEMU_PACKED SpinInfo;
>   
> +/*
> + * QEMU interface:
> + *  + QOM array property "cpus-qom-path": QOM canonical path of each CPU.
> + */
>   #define TYPE_E500_SPIN "e500-spin"
>   OBJECT_DECLARE_SIMPLE_TYPE(SpinState, E500_SPIN)
>   
> @@ -54,6 +60,9 @@ struct SpinState {
>   
>       MemoryRegion iomem;
>       SpinInfo spin[MAX_CPUS];
> +    uint32_t cpu_count;
> +    char **cpu_canonical_path;
> +    CPUState **cpu;
>   };
>   
>   static void spin_reset(DeviceState *dev)
> @@ -121,16 +130,10 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>   {
>       SpinState *s = opaque;
>       int env_idx = addr / sizeof(SpinInfo);
> -    CPUState *cpu;
> +    CPUState *cpu = s->cpu[env_idx];
>       SpinInfo *curspin = &s->spin[env_idx];
>       uint8_t *curspin_p = (uint8_t*)curspin;
>   
> -    cpu = qemu_get_cpu(env_idx);
> -    if (cpu == NULL) {
> -        /* Unknown CPU */
> -        return;
> -    }
> -
>       if (cpu->cpu_index == 0) {
>           /* primary CPU doesn't spin */
>           return;
> @@ -188,11 +191,42 @@ static void ppce500_spin_initfn(Object *obj)
>       sysbus_init_mmio(dev, &s->iomem);
>   }
>   
> +static void ppce500_spin_realize(DeviceState *dev, Error **errp)
> +{
> +    SpinState *s = E500_SPIN(dev);
> +
> +    if (s->cpu_count == 0) {
> +        error_setg(errp, "'cpus-qom-path' property array must be set");
> +        return;
> +    } else if (s->cpu_count > MAX_CPUS) {
> +        error_setg(errp, "at most %d CPUs are supported", MAX_CPUS);
> +        return;
> +    }
> +
> +    s->cpu = g_new(CPUState *, s->cpu_count);
> +    for (unsigned i = 0; i < s->cpu_count; i++) {
> +        bool ambiguous;
> +        Object *obj;
> +
> +        obj = object_resolve_path(s->cpu_canonical_path[i], &ambiguous);
> +        assert(!ambiguous);
> +        s->cpu[i] = CPU(obj);
> +    }
> +}
> +
> +static Property ppce500_spin_properties[] = {
> +    DEFINE_PROP_ARRAY("cpus-qom-path", SpinState, cpu_count,
> +                      cpu_canonical_path, qdev_prop_string, char *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>   static void ppce500_spin_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       dc->reset = spin_reset;
> +    dc->realize = ppce500_spin_realize;
> +    device_class_set_props(dc, ppce500_spin_properties);
>   }
>   
>   static const TypeInfo ppce500_spin_types[] = {

Re: [RFC PATCH 0/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Daniel Henrique Barboza]

On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
> Following the discussion with Peter on my "cpus: Step toward
> removing global 'first_cpu'" series [*], we now pass the array
> of CPUs via property. Since we can not pass array of "link"
> properties, we pass the QOM path of each CPU as a QList(String).
> 
> Tagged as RFC to discuss the idea of using qdev_prop_set_array
> with qlist_append_str(object_get_canonical_path). Personally I
> find it super simple.

I probably misunderstood the concept/problem but "super simple" is not the first
thing that came to my mind in patch 5 hehe

I didn't follow the whole thread, just the [*] message marked and a couple
of replies, but are we planning to deprecate qemu_get_cpu()? Patch 5 mentions
"Devices should avoid calling qemu_get_cpu() because this call doesn't work
as expected with heterogeneous machines". I'll take your word for it. But
e500 isn't an heterogeneous machine - we're creating ppc cpus only. So I'm
a bit confused here. Are you using e500 just as a simple PoC?

Regardless of the reason to use e500 in this RFC, I believe we would benefit
from having common helpers/magic sauce macros to add all this apparently
boilerplate code to replace qemu_get_cpu().

I mean, we're changing this:

-    cpu = qemu_get_cpu(env_idx);
-    if (cpu == NULL) {
-        /* Unknown CPU */
-        return;
-    }
-

For a lot of QOM stuff like this:


+        cpu_qompath = object_get_canonical_path(OBJECT(cs));
+        qlist_append_str(spin_cpu_list, cpu_qompath);
+    qdev_prop_set_array(dev, "cpus-qom-path", spin_cpu_list);


+    if (s->cpu_count == 0) {
+        error_setg(errp, "'cpus-qom-path' property array must be set");
+        return;
+    } else if (s->cpu_count > MAX_CPUS) {
+        error_setg(errp, "at most %d CPUs are supported", MAX_CPUS);
+        return;
+    }
+
+    s->cpu = g_new(CPUState *, s->cpu_count);
+    for (unsigned i = 0; i < s->cpu_count; i++) {
+        bool ambiguous;
+        Object *obj;
+
+        obj = object_resolve_path(s->cpu_canonical_path[i], &ambiguous);
+        assert(!ambiguous);
+        s->cpu[i] = CPU(obj);
+    }
+
+static Property ppce500_spin_properties[] = {
+    DEFINE_PROP_ARRAY("cpus-qom-path", SpinState, cpu_count,
+                      cpu_canonical_path, qdev_prop_string, char *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+

So anything that makes the QOM stuff more palatable to deal with would be
really appreciated. Thanks,


Daniel


> 
> Regards,
> 
> Phil.
> 
> [*] https://lore.kernel.org/qemu-devel/CAFEAcA9FT+QMyQSLCeLjd7tEfaoS9JazmkYWQE++s1AmF7Nfvw@mail.gmail.com/
> 
> Kevin Wolf (1):
>    qdev: Add qdev_prop_set_array()
> 
> Philippe Mathieu-Daudé (4):
>    hw/ppc/e500: Declare CPU QOM types using DEFINE_TYPES() macro
>    hw/ppc/e500: QOM-attach CPUs to the machine container
>    hw/ppc/e500: Inline sysbus_create_simple(E500_SPIN)
>    hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths
> 
>   include/hw/qdev-properties.h |  3 ++
>   hw/core/qdev-properties.c    | 21 +++++++++++
>   hw/ppc/e500.c                | 11 +++++-
>   hw/ppc/ppce500_spin.c        | 69 ++++++++++++++++++++++++++----------
>   4 files changed, 84 insertions(+), 20 deletions(-)
> 

Re: [RFC PATCH 5/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Philippe Mathieu-Daudé]

On 31/10/23 22:16, Daniel Henrique Barboza wrote:
> On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
>> Devices should avoid calling qemu_get_cpu() because this call
>> doesn't work as expected with heterogeneous machines. Such
>> devices often iterate over a cluster of CPUs, which the device's
>> parent has direct access (when creating the child device).
>>
>> We can pass QOM as 'link' between objects, but we can't pass an
>> array of links. Here we exploits QAPI simplicity, by using
>> DEFINE_PROP_ARRAY and a list of strings, each string being the
>> CPU canonical path in QOM tree (which is constant and unique).
>> When the device realizes itself, the original CPU pointer is
>> recovered via a object_resolve_path() call.
>>
>> Inspired-by: Peter Maydell <peter.maydell@linaro.org>
>> Inspired-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> Tested with:
>> $ make check-qtest-ppc{,64}
>> $ make check-avocado AVOCADO_TAGS='machine:ppce500 machine:mpc8544ds'
>>
>> RFC: See cover
>>
>> FIXME: Should we free spin_cpu_list using g_autoptr(QList)?
> 
> By looking at how object_property_set_qobject() works I *think* we can 
> free it.
> Perhaps try using g_autofree and see if something explodes, hehe

In another thread, Peter noticed we then call qdev_prop_set_array(),
which takes the ownership, so no need to use g_autoptr in this
particular case (else we trigger a double-free):
https://lore.kernel.org/qemu-devel/CAFEAcA_GC8ypM2Y94KCU9Q_dntF6Na+igu-+0JZJ+MvPFE_HcA@mail.gmail.com/


> Thanks,
> 
> Daniel
> 
>> ---
>>   hw/ppc/e500.c         |  6 ++++++
>>   hw/ppc/ppce500_spin.c | 48 ++++++++++++++++++++++++++++++++++++-------
>>   2 files changed, 47 insertions(+), 7 deletions(-)

Re: [RFC PATCH 0/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Philippe Mathieu-Daudé]

Hi Daniel,

On 31/10/23 22:49, Daniel Henrique Barboza wrote:
> On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
>> Following the discussion with Peter on my "cpus: Step toward
>> removing global 'first_cpu'" series [*], we now pass the array
>> of CPUs via property. Since we can not pass array of "link"
>> properties, we pass the QOM path of each CPU as a QList(String).
>>
>> Tagged as RFC to discuss the idea of using qdev_prop_set_array
>> with qlist_append_str(object_get_canonical_path). Personally I
>> find it super simple.
> 
> I probably misunderstood the concept/problem but "super simple" is not 
> the first
> thing that came to my mind in patch 5 hehe
> 
> I didn't follow the whole thread, just the [*] message marked and a couple
> of replies, but are we planning to deprecate qemu_get_cpu()? Patch 5 
> mentions
> "Devices should avoid calling qemu_get_cpu() because this call doesn't work
> as expected with heterogeneous machines". I'll take your word for it. But
> e500 isn't an heterogeneous machine - we're creating ppc cpus only. So I'm
> a bit confused here. Are you using e500 just as a simple PoC?

I'm using the e500 as the simplest complex device using qemu_get_cpu :)

Indeed, in [*] Peter said not all qemu_get_cpu() calls are harmful. In
particular in homogeneous machines.

Looking back at the e500, the problem isn't the *machine*, but a device
it is using.

Taking "dynamic machines" as a step toward "heterogeneous machines",
I'm considering all devices potentially creatable dynamically, again
potentially ending being use in some heterogeneous prototype. So I'd
rather have all devices using the same API without worrying whether
the device is designed for heterogeneous machine or not. The simplest
way I found to achieve that, is to restrict qemu_get_cpu() to accel/
and system/ -- where a vCPU arch is irrelevant --, but prohibit it for
hw/ files. Maybe I'm wrong and this isn't the best way to go, which is
why I tried this RFC, expecting constructive comments like yours, thanks
for that!

> Regardless of the reason to use e500 in this RFC, I believe we would 
> benefit
> from having common helpers/magic sauce macros to add all this apparently
> boilerplate code to replace qemu_get_cpu().
> 
> I mean, we're changing this:
> 
> -    cpu = qemu_get_cpu(env_idx);
> -    if (cpu == NULL) {
> -        /* Unknown CPU */
> -        return;
> -    }
> -
> 
> For a lot of QOM stuff like this:
> 
> 
> +        cpu_qompath = object_get_canonical_path(OBJECT(cs));
> +        qlist_append_str(spin_cpu_list, cpu_qompath);
> +    qdev_prop_set_array(dev, "cpus-qom-path", spin_cpu_list);
> 
> 
> +    if (s->cpu_count == 0) {
> +        error_setg(errp, "'cpus-qom-path' property array must be set");
> +        return;
> +    } else if (s->cpu_count > MAX_CPUS) {
> +        error_setg(errp, "at most %d CPUs are supported", MAX_CPUS);
> +        return;
> +    }
> +
> +    s->cpu = g_new(CPUState *, s->cpu_count);
> +    for (unsigned i = 0; i < s->cpu_count; i++) {
> +        bool ambiguous;
> +        Object *obj;
> +
> +        obj = object_resolve_path(s->cpu_canonical_path[i], &ambiguous);
> +        assert(!ambiguous);
> +        s->cpu[i] = CPU(obj);
> +    }
> +
> +static Property ppce500_spin_properties[] = {
> +    DEFINE_PROP_ARRAY("cpus-qom-path", SpinState, cpu_count,
> +                      cpu_canonical_path, qdev_prop_string, char *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> 
> So anything that makes the QOM stuff more palatable to deal with would be
> really appreciated. Thanks,

Yeah, I'll see what I can do here. But first I'd like some feedback
from QOM experts on whether using QOM canonical path is good or not.

Markus, Paolo (which I forgot to Cc...)?

> Daniel
> 
>>
>> Regards,
>>
>> Phil.
>>
>> [*] 
>> https://lore.kernel.org/qemu-devel/CAFEAcA9FT+QMyQSLCeLjd7tEfaoS9JazmkYWQE++s1AmF7Nfvw@mail.gmail.com/
>>
>> Kevin Wolf (1):
>>    qdev: Add qdev_prop_set_array()
>>
>> Philippe Mathieu-Daudé (4):
>>    hw/ppc/e500: Declare CPU QOM types using DEFINE_TYPES() macro
>>    hw/ppc/e500: QOM-attach CPUs to the machine container
>>    hw/ppc/e500: Inline sysbus_create_simple(E500_SPIN)
>>    hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths
>>
>>   include/hw/qdev-properties.h |  3 ++
>>   hw/core/qdev-properties.c    | 21 +++++++++++
>>   hw/ppc/e500.c                | 11 +++++-
>>   hw/ppc/ppce500_spin.c        | 69 ++++++++++++++++++++++++++----------
>>   4 files changed, 84 insertions(+), 20 deletions(-)
>>

Re: [RFC PATCH 0/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Philippe Mathieu-Daudé]

On 31/10/23 22:49, Daniel Henrique Barboza wrote:
> 
> 
> On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
>> Following the discussion with Peter on my "cpus: Step toward
>> removing global 'first_cpu'" series [*], we now pass the array
>> of CPUs via property. Since we can not pass array of "link"
>> properties, we pass the QOM path of each CPU as a QList(String).
>>
>> Tagged as RFC to discuss the idea of using qdev_prop_set_array
>> with qlist_append_str(object_get_canonical_path). Personally I
>> find it super simple.
> 
> I probably misunderstood the concept/problem but "super simple" is not 
> the first
> thing that came to my mind in patch 5 hehe

Right, I probably forgot some paragraph here. I meant, passing QOM
canonical path as a string between (qdev) objects seems much simpler
than declaring a PropertyInfo for each type we want to pass, because
this is within the same QEMU process and we don't need to serialize
anything.

See for example:
$ git grep -h PropertyInfo hw/core/qdev-properties-system.c
219:const PropertyInfo qdev_prop_drive = {
228:const PropertyInfo qdev_prop_drive_iothread = {
295:const PropertyInfo qdev_prop_chr = {
369:const PropertyInfo qdev_prop_macaddr = {
457:const PropertyInfo qdev_prop_netdev = {
495:const PropertyInfo qdev_prop_audiodev = {
585:const PropertyInfo qdev_prop_losttickpolicy = {
615:const PropertyInfo qdev_prop_blocksize = {
628:const PropertyInfo qdev_prop_blockdev_on_error = {
642:const PropertyInfo qdev_prop_bios_chs_trans = {
654:const PropertyInfo qdev_prop_fdc_drive_type = {
666:const PropertyInfo qdev_prop_multifd_compression = {
747:const PropertyInfo qdev_prop_reserved_region = {
810:const PropertyInfo qdev_prop_pci_devfn = {
916:const PropertyInfo qdev_prop_pci_host_devaddr = {
926:const PropertyInfo qdev_prop_off_auto_pcibar = {
996:const PropertyInfo qdev_prop_pcie_link_speed = {
1084:const PropertyInfo qdev_prop_pcie_link_width = {
1134:const PropertyInfo qdev_prop_uuid = {
1147:const PropertyInfo qdev_prop_cpus390entitlement = {

Re: [PATCH 2/5] hw/ppc/e500: Declare CPU QOM types using DEFINE_TYPES() macro

[Kevin Wolf]

Am 30.10.2023 um 15:39 hat Philippe Mathieu-Daudé geschrieben:
> When multiple QOM types are registered in the same file,
> it is simpler to use the the DEFINE_TYPES() macro. In
> particular because type array declared with such macro
> are easier to review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> I'm going to do that for each file I modify, so eventually
> we'll get all converted.

This is probably a standard commit message that you used for every
conversion? Maybe it should be changed here because I only see a single
QOM type in the patch.

Kevin

Re: [PATCH 3/5] hw/ppc/e500: QOM-attach CPUs to the machine container

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Instead of having CPUs dangling in the /unattached/device
> bucket, attach them to the machine container.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ppc/e500.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e04114fb3c..f8177c0280 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -946,6 +946,7 @@ void ppce500_init(MachineState *machine)
>              exit(1);
>          }
>  
> +        object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cs));
>          /*
>           * Secondary CPU starts in halted state for now. Needs to change
>           * when implementing non-kernel boot.

A peek at "info qom-tree" confirms the CPU is in /machine/unattached/.
Along with most onboard devices.  Details below.

Quick test...  I count 563 machines.  394 seem to have CPU(s) in or
below /machine/unattached/, 129 elsewhere, and 40 I can't easily
examine, because they don't start to monitor without additional CLI
arguments.

Where should CPUs be?

Is /machine/unattached/ basically where we dump products of lazy
modelling?

If yes, should we try to empty it out?

If we shouldn't, then why move this one out?



$ qemu-system-ppc -nodefaults -S -display none -M ppce500 -monitor stdio
QEMU 8.1.50 monitor - type 'help' for more information
(qemu) info qom-tree
/machine (ppce500-machine)
  /e500-ccsr (e500-ccsr)
    /e500-ccsr[0] (memory-region)
    /e500-pci-bar0[0] (memory-region)
  /pci-host (e500-pcihost)
    /bm-e500[0] (memory-region)
    /pci bus memory[0] (memory-region)
    /pci-conf-data[0] (memory-region)
    /pci-conf-idx[0] (memory-region)
    /pci-container[0] (memory-region)
    /pci-pio[0] (memory-region)
    /pci.0 (PCI)
    /pci.reg[0] (memory-region)
  /peripheral (container)
  /peripheral-anon (container)
  /pic (openpic)
    /cpu[0] (memory-region)
    /glb[0] (memory-region)
    /msi[0] (memory-region)
    /openpic[0] (memory-region)
    /src[0] (memory-region)
    /summary[0] (memory-region)
    /tmr[0] (memory-region)
    /unnamed-gpio-in[0] (irq)
    /unnamed-gpio-in[100] (irq)
    /unnamed-gpio-in[101] (irq)
    /unnamed-gpio-in[102] (irq)
    /unnamed-gpio-in[103] (irq)
    /unnamed-gpio-in[104] (irq)
    /unnamed-gpio-in[105] (irq)
    /unnamed-gpio-in[106] (irq)
    /unnamed-gpio-in[107] (irq)
    /unnamed-gpio-in[108] (irq)
    /unnamed-gpio-in[109] (irq)
    /unnamed-gpio-in[10] (irq)
    /unnamed-gpio-in[110] (irq)
    /unnamed-gpio-in[111] (irq)
    /unnamed-gpio-in[112] (irq)
    /unnamed-gpio-in[113] (irq)
    /unnamed-gpio-in[114] (irq)
    /unnamed-gpio-in[115] (irq)
    /unnamed-gpio-in[116] (irq)
    /unnamed-gpio-in[117] (irq)
    /unnamed-gpio-in[118] (irq)
    /unnamed-gpio-in[119] (irq)
    /unnamed-gpio-in[11] (irq)
    /unnamed-gpio-in[120] (irq)
    /unnamed-gpio-in[121] (irq)
    /unnamed-gpio-in[122] (irq)
    /unnamed-gpio-in[123] (irq)
    /unnamed-gpio-in[124] (irq)
    /unnamed-gpio-in[125] (irq)
    /unnamed-gpio-in[126] (irq)
    /unnamed-gpio-in[127] (irq)
    /unnamed-gpio-in[128] (irq)
    /unnamed-gpio-in[129] (irq)
    /unnamed-gpio-in[12] (irq)
    /unnamed-gpio-in[130] (irq)
    /unnamed-gpio-in[131] (irq)
    /unnamed-gpio-in[132] (irq)
    /unnamed-gpio-in[133] (irq)
    /unnamed-gpio-in[134] (irq)
    /unnamed-gpio-in[135] (irq)
    /unnamed-gpio-in[136] (irq)
    /unnamed-gpio-in[137] (irq)
    /unnamed-gpio-in[138] (irq)
    /unnamed-gpio-in[139] (irq)
    /unnamed-gpio-in[13] (irq)
    /unnamed-gpio-in[140] (irq)
    /unnamed-gpio-in[141] (irq)
    /unnamed-gpio-in[142] (irq)
    /unnamed-gpio-in[143] (irq)
    /unnamed-gpio-in[144] (irq)
    /unnamed-gpio-in[145] (irq)
    /unnamed-gpio-in[146] (irq)
    /unnamed-gpio-in[147] (irq)
    /unnamed-gpio-in[148] (irq)
    /unnamed-gpio-in[149] (irq)
    /unnamed-gpio-in[14] (irq)
    /unnamed-gpio-in[150] (irq)
    /unnamed-gpio-in[151] (irq)
    /unnamed-gpio-in[152] (irq)
    /unnamed-gpio-in[153] (irq)
    /unnamed-gpio-in[154] (irq)
    /unnamed-gpio-in[155] (irq)
    /unnamed-gpio-in[156] (irq)
    /unnamed-gpio-in[157] (irq)
    /unnamed-gpio-in[158] (irq)
    /unnamed-gpio-in[159] (irq)
    /unnamed-gpio-in[15] (irq)
    /unnamed-gpio-in[160] (irq)
    /unnamed-gpio-in[161] (irq)
    /unnamed-gpio-in[162] (irq)
    /unnamed-gpio-in[163] (irq)
    /unnamed-gpio-in[164] (irq)
    /unnamed-gpio-in[165] (irq)
    /unnamed-gpio-in[166] (irq)
    /unnamed-gpio-in[167] (irq)
    /unnamed-gpio-in[168] (irq)
    /unnamed-gpio-in[169] (irq)
    /unnamed-gpio-in[16] (irq)
    /unnamed-gpio-in[170] (irq)
    /unnamed-gpio-in[171] (irq)
    /unnamed-gpio-in[172] (irq)
    /unnamed-gpio-in[173] (irq)
    /unnamed-gpio-in[174] (irq)
    /unnamed-gpio-in[175] (irq)
    /unnamed-gpio-in[176] (irq)
    /unnamed-gpio-in[177] (irq)
    /unnamed-gpio-in[178] (irq)
    /unnamed-gpio-in[179] (irq)
    /unnamed-gpio-in[17] (irq)
    /unnamed-gpio-in[180] (irq)
    /unnamed-gpio-in[181] (irq)
    /unnamed-gpio-in[182] (irq)
    /unnamed-gpio-in[183] (irq)
    /unnamed-gpio-in[184] (irq)
    /unnamed-gpio-in[185] (irq)
    /unnamed-gpio-in[186] (irq)
    /unnamed-gpio-in[187] (irq)
    /unnamed-gpio-in[188] (irq)
    /unnamed-gpio-in[189] (irq)
    /unnamed-gpio-in[18] (irq)
    /unnamed-gpio-in[190] (irq)
    /unnamed-gpio-in[191] (irq)
    /unnamed-gpio-in[192] (irq)
    /unnamed-gpio-in[193] (irq)
    /unnamed-gpio-in[194] (irq)
    /unnamed-gpio-in[195] (irq)
    /unnamed-gpio-in[196] (irq)
    /unnamed-gpio-in[197] (irq)
    /unnamed-gpio-in[198] (irq)
    /unnamed-gpio-in[199] (irq)
    /unnamed-gpio-in[19] (irq)
    /unnamed-gpio-in[1] (irq)
    /unnamed-gpio-in[200] (irq)
    /unnamed-gpio-in[201] (irq)
    /unnamed-gpio-in[202] (irq)
    /unnamed-gpio-in[203] (irq)
    /unnamed-gpio-in[204] (irq)
    /unnamed-gpio-in[205] (irq)
    /unnamed-gpio-in[206] (irq)
    /unnamed-gpio-in[207] (irq)
    /unnamed-gpio-in[208] (irq)
    /unnamed-gpio-in[209] (irq)
    /unnamed-gpio-in[20] (irq)
    /unnamed-gpio-in[210] (irq)
    /unnamed-gpio-in[211] (irq)
    /unnamed-gpio-in[212] (irq)
    /unnamed-gpio-in[213] (irq)
    /unnamed-gpio-in[214] (irq)
    /unnamed-gpio-in[215] (irq)
    /unnamed-gpio-in[216] (irq)
    /unnamed-gpio-in[217] (irq)
    /unnamed-gpio-in[218] (irq)
    /unnamed-gpio-in[219] (irq)
    /unnamed-gpio-in[21] (irq)
    /unnamed-gpio-in[220] (irq)
    /unnamed-gpio-in[221] (irq)
    /unnamed-gpio-in[222] (irq)
    /unnamed-gpio-in[223] (irq)
    /unnamed-gpio-in[224] (irq)
    /unnamed-gpio-in[225] (irq)
    /unnamed-gpio-in[226] (irq)
    /unnamed-gpio-in[227] (irq)
    /unnamed-gpio-in[228] (irq)
    /unnamed-gpio-in[229] (irq)
    /unnamed-gpio-in[22] (irq)
    /unnamed-gpio-in[230] (irq)
    /unnamed-gpio-in[231] (irq)
    /unnamed-gpio-in[232] (irq)
    /unnamed-gpio-in[233] (irq)
    /unnamed-gpio-in[234] (irq)
    /unnamed-gpio-in[235] (irq)
    /unnamed-gpio-in[236] (irq)
    /unnamed-gpio-in[237] (irq)
    /unnamed-gpio-in[238] (irq)
    /unnamed-gpio-in[239] (irq)
    /unnamed-gpio-in[23] (irq)
    /unnamed-gpio-in[240] (irq)
    /unnamed-gpio-in[241] (irq)
    /unnamed-gpio-in[242] (irq)
    /unnamed-gpio-in[243] (irq)
    /unnamed-gpio-in[244] (irq)
    /unnamed-gpio-in[245] (irq)
    /unnamed-gpio-in[246] (irq)
    /unnamed-gpio-in[247] (irq)
    /unnamed-gpio-in[248] (irq)
    /unnamed-gpio-in[249] (irq)
    /unnamed-gpio-in[24] (irq)
    /unnamed-gpio-in[250] (irq)
    /unnamed-gpio-in[251] (irq)
    /unnamed-gpio-in[252] (irq)
    /unnamed-gpio-in[253] (irq)
    /unnamed-gpio-in[254] (irq)
    /unnamed-gpio-in[255] (irq)
    /unnamed-gpio-in[256] (irq)
    /unnamed-gpio-in[257] (irq)
    /unnamed-gpio-in[258] (irq)
    /unnamed-gpio-in[259] (irq)
    /unnamed-gpio-in[25] (irq)
    /unnamed-gpio-in[260] (irq)
    /unnamed-gpio-in[261] (irq)
    /unnamed-gpio-in[262] (irq)
    /unnamed-gpio-in[263] (irq)
    /unnamed-gpio-in[26] (irq)
    /unnamed-gpio-in[27] (irq)
    /unnamed-gpio-in[28] (irq)
    /unnamed-gpio-in[29] (irq)
    /unnamed-gpio-in[2] (irq)
    /unnamed-gpio-in[30] (irq)
    /unnamed-gpio-in[31] (irq)
    /unnamed-gpio-in[32] (irq)
    /unnamed-gpio-in[33] (irq)
    /unnamed-gpio-in[34] (irq)
    /unnamed-gpio-in[35] (irq)
    /unnamed-gpio-in[36] (irq)
    /unnamed-gpio-in[37] (irq)
    /unnamed-gpio-in[38] (irq)
    /unnamed-gpio-in[39] (irq)
    /unnamed-gpio-in[3] (irq)
    /unnamed-gpio-in[40] (irq)
    /unnamed-gpio-in[41] (irq)
    /unnamed-gpio-in[42] (irq)
    /unnamed-gpio-in[43] (irq)
    /unnamed-gpio-in[44] (irq)
    /unnamed-gpio-in[45] (irq)
    /unnamed-gpio-in[46] (irq)
    /unnamed-gpio-in[47] (irq)
    /unnamed-gpio-in[48] (irq)
    /unnamed-gpio-in[49] (irq)
    /unnamed-gpio-in[4] (irq)
    /unnamed-gpio-in[50] (irq)
    /unnamed-gpio-in[51] (irq)
    /unnamed-gpio-in[52] (irq)
    /unnamed-gpio-in[53] (irq)
    /unnamed-gpio-in[54] (irq)
    /unnamed-gpio-in[55] (irq)
    /unnamed-gpio-in[56] (irq)
    /unnamed-gpio-in[57] (irq)
    /unnamed-gpio-in[58] (irq)
    /unnamed-gpio-in[59] (irq)
    /unnamed-gpio-in[5] (irq)
    /unnamed-gpio-in[60] (irq)
    /unnamed-gpio-in[61] (irq)
    /unnamed-gpio-in[62] (irq)
    /unnamed-gpio-in[63] (irq)
    /unnamed-gpio-in[64] (irq)
    /unnamed-gpio-in[65] (irq)
    /unnamed-gpio-in[66] (irq)
    /unnamed-gpio-in[67] (irq)
    /unnamed-gpio-in[68] (irq)
    /unnamed-gpio-in[69] (irq)
    /unnamed-gpio-in[6] (irq)
    /unnamed-gpio-in[70] (irq)
    /unnamed-gpio-in[71] (irq)
    /unnamed-gpio-in[72] (irq)
    /unnamed-gpio-in[73] (irq)
    /unnamed-gpio-in[74] (irq)
    /unnamed-gpio-in[75] (irq)
    /unnamed-gpio-in[76] (irq)
    /unnamed-gpio-in[77] (irq)
    /unnamed-gpio-in[78] (irq)
    /unnamed-gpio-in[79] (irq)
    /unnamed-gpio-in[7] (irq)
    /unnamed-gpio-in[80] (irq)
    /unnamed-gpio-in[81] (irq)
    /unnamed-gpio-in[82] (irq)
    /unnamed-gpio-in[83] (irq)
    /unnamed-gpio-in[84] (irq)
    /unnamed-gpio-in[85] (irq)
    /unnamed-gpio-in[86] (irq)
    /unnamed-gpio-in[87] (irq)
    /unnamed-gpio-in[88] (irq)
    /unnamed-gpio-in[89] (irq)
    /unnamed-gpio-in[8] (irq)
    /unnamed-gpio-in[90] (irq)
    /unnamed-gpio-in[91] (irq)
    /unnamed-gpio-in[92] (irq)
    /unnamed-gpio-in[93] (irq)
    /unnamed-gpio-in[94] (irq)
    /unnamed-gpio-in[95] (irq)
    /unnamed-gpio-in[96] (irq)
    /unnamed-gpio-in[97] (irq)
    /unnamed-gpio-in[98] (irq)
    /unnamed-gpio-in[99] (irq)
    /unnamed-gpio-in[9] (irq)
  /unattached (container)
    /device[0] (e500v2_v30-powerpc-cpu)
      /unnamed-gpio-in[0] (irq)
      /unnamed-gpio-in[1] (irq)
      /unnamed-gpio-in[2] (irq)
      /unnamed-gpio-in[3] (irq)
      /unnamed-gpio-in[4] (irq)
      /unnamed-gpio-in[5] (irq)
      /unnamed-gpio-in[6] (irq)
    /device[1] (mpc-i2c)
      /i2c (i2c-bus)
      /mpc-i2c[0] (memory-region)
    /device[2] (ds1338)
    /device[3] (unimplemented-device)
      /esdhc[0] (memory-region)
    /device[4] (generic-sdhci)
      /sd-bus (sdhci-bus)
      /sdhci[0] (memory-region)
    /device[5] (mpc8544-guts)
      /mpc8544.guts[0] (memory-region)
    /device[6] (e500-host-bridge)
      /bus master container[0] (memory-region)
      /bus master[0] (memory-region)
    /device[7] (e500-spin)
      /e500 spin pv device[0] (memory-region)
    /device[8] (mpc8xxx_gpio)
      /mpc8xxx_gpio[0] (memory-region)
      /unnamed-gpio-in[0] (irq)
      /unnamed-gpio-in[10] (irq)
      /unnamed-gpio-in[11] (irq)
      /unnamed-gpio-in[12] (irq)
      /unnamed-gpio-in[13] (irq)
      /unnamed-gpio-in[14] (irq)
      /unnamed-gpio-in[15] (irq)
      /unnamed-gpio-in[16] (irq)
      /unnamed-gpio-in[17] (irq)
      /unnamed-gpio-in[18] (irq)
      /unnamed-gpio-in[19] (irq)
      /unnamed-gpio-in[1] (irq)
      /unnamed-gpio-in[20] (irq)
      /unnamed-gpio-in[21] (irq)
      /unnamed-gpio-in[22] (irq)
      /unnamed-gpio-in[23] (irq)
      /unnamed-gpio-in[24] (irq)
      /unnamed-gpio-in[25] (irq)
      /unnamed-gpio-in[26] (irq)
      /unnamed-gpio-in[27] (irq)
      /unnamed-gpio-in[28] (irq)
      /unnamed-gpio-in[29] (irq)
      /unnamed-gpio-in[2] (irq)
      /unnamed-gpio-in[30] (irq)
      /unnamed-gpio-in[31] (irq)
      /unnamed-gpio-in[3] (irq)
      /unnamed-gpio-in[4] (irq)
      /unnamed-gpio-in[5] (irq)
      /unnamed-gpio-in[6] (irq)
      /unnamed-gpio-in[7] (irq)
      /unnamed-gpio-in[8] (irq)
      /unnamed-gpio-in[9] (irq)
    /device[9] (platform-bus-device)
      /platform bus[0] (memory-region)
    /io[0] (memory-region)
    /non-qdev-gpio[0] (irq)
    /sysbus (System)
    /system[0] (memory-region)

Re: [RFC PATCH 5/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Devices should avoid calling qemu_get_cpu() because this call
> doesn't work as expected with heterogeneous machines. Such
> devices often iterate over a cluster of CPUs, which the device's
> parent has direct access (when creating the child device).
>
> We can pass QOM as 'link' between objects, but we can't pass an
> array of links. Here we exploits QAPI simplicity, by using

Do you mean qdev simplicity?

> DEFINE_PROP_ARRAY and a list of strings, each string being the
> CPU canonical path in QOM tree (which is constant and unique).
> When the device realizes itself, the original CPU pointer is
> recovered via a object_resolve_path() call.

We have link properties, see DEFINE_PROP_LINK() and qdev_prop_link.
Would an array of link properties be feasible here?

> Inspired-by: Peter Maydell <peter.maydell@linaro.org>
> Inspired-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Tested with:
> $ make check-qtest-ppc{,64}
> $ make check-avocado AVOCADO_TAGS='machine:ppce500 machine:mpc8544ds'
>
> RFC: See cover
>
> FIXME: Should we free spin_cpu_list using g_autoptr(QList)?

Re: [PATCH 3/5] hw/ppc/e500: QOM-attach CPUs to the machine container

[Philippe Mathieu-Daudé]

On 3/11/23 08:40, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Instead of having CPUs dangling in the /unattached/device
>> bucket, attach them to the machine container.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/ppc/e500.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index e04114fb3c..f8177c0280 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -946,6 +946,7 @@ void ppce500_init(MachineState *machine)
>>               exit(1);
>>           }
>>   
>> +        object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cs));
>>           /*
>>            * Secondary CPU starts in halted state for now. Needs to change
>>            * when implementing non-kernel boot.
> 
> A peek at "info qom-tree" confirms the CPU is in /machine/unattached/.
> Along with most onboard devices.  Details below.
> 
> Quick test...  I count 563 machines.  394 seem to have CPU(s) in or
> below /machine/unattached/, 129 elsewhere, and 40 I can't easily
> examine, because they don't start to monitor without additional CLI
> arguments.
> 
> Where should CPUs be?

It is machine specific.

- For System-on-Chip, it would be in /soc

- For systems that fully model CPU topology, I'd expect a consistent
   topology path. (If it is part of a cluster, in that /cluster).

- For mainframes, it should be part of the CPU cards that can be
   inserted?

- For a single Pentium CPU, maybe /machine is sufficient.

> Is /machine/unattached/ basically where we dump products of lazy
> modelling?

Unfortunately, yes. Also where CLI created devices are I guess.

> If yes, should we try to empty it out?

If it is useful. For components expected to be referenced externally
by humans, probably. If only used by scripts, maybe not, except if
human have to debug.

> If we shouldn't, then why move this one out?

When looking for a component in the tree, I start to look at /machine,
having to fish for it elsewhere is not very natural. I'd change your
question by:
- Why do we need /unattached?
or
- Why do we have 2 different folders, /machine and /unattached?
If it is a headache, why not just simply merge them both?

> $ qemu-system-ppc -nodefaults -S -display none -M ppce500 -monitor stdio
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) info qom-tree
> /machine (ppce500-machine)
>    /e500-ccsr (e500-ccsr)
>      /e500-ccsr[0] (memory-region)
>      /e500-pci-bar0[0] (memory-region)
>    /pci-host (e500-pcihost)
>      /bm-e500[0] (memory-region)
>      /pci bus memory[0] (memory-region)
>      /pci-conf-data[0] (memory-region)
>      /pci-conf-idx[0] (memory-region)
>      /pci-container[0] (memory-region)
>      /pci-pio[0] (memory-region)
>      /pci.0 (PCI)
>      /pci.reg[0] (memory-region)
>    /peripheral (container)
>    /peripheral-anon (container)
[...]

>    /unattached (container)
>      /device[0] (e500v2_v30-powerpc-cpu)
>        /unnamed-gpio-in[0] (irq)
>        /unnamed-gpio-in[1] (irq)
>        /unnamed-gpio-in[2] (irq)
>        /unnamed-gpio-in[3] (irq)
>        /unnamed-gpio-in[4] (irq)
>        /unnamed-gpio-in[5] (irq)
>        /unnamed-gpio-in[6] (irq)
>      /device[1] (mpc-i2c)
>        /i2c (i2c-bus)
>        /mpc-i2c[0] (memory-region)
>      /device[2] (ds1338)
>      /device[3] (unimplemented-device)
>        /esdhc[0] (memory-region)
>      /device[4] (generic-sdhci)
>        /sd-bus (sdhci-bus)
>        /sdhci[0] (memory-region)
>      /device[5] (mpc8544-guts)
>        /mpc8544.guts[0] (memory-region)
>      /device[6] (e500-host-bridge)
>        /bus master container[0] (memory-region)
>        /bus master[0] (memory-region)
>      /device[7] (e500-spin)
>        /e500 spin pv device[0] (memory-region)
>      /device[8] (mpc8xxx_gpio)
>        /mpc8xxx_gpio[0] (memory-region)
>        /unnamed-gpio-in[0] (irq)
>        /unnamed-gpio-in[10] (irq)
>        /unnamed-gpio-in[11] (irq)
>        /unnamed-gpio-in[12] (irq)
>        /unnamed-gpio-in[13] (irq)
>        /unnamed-gpio-in[14] (irq)
>        /unnamed-gpio-in[15] (irq)
>        /unnamed-gpio-in[16] (irq)
>        /unnamed-gpio-in[17] (irq)
>        /unnamed-gpio-in[18] (irq)
>        /unnamed-gpio-in[19] (irq)
>        /unnamed-gpio-in[1] (irq)
>        /unnamed-gpio-in[20] (irq)
>        /unnamed-gpio-in[21] (irq)
>        /unnamed-gpio-in[22] (irq)
>        /unnamed-gpio-in[23] (irq)
>        /unnamed-gpio-in[24] (irq)
>        /unnamed-gpio-in[25] (irq)
>        /unnamed-gpio-in[26] (irq)
>        /unnamed-gpio-in[27] (irq)
>        /unnamed-gpio-in[28] (irq)
>        /unnamed-gpio-in[29] (irq)
>        /unnamed-gpio-in[2] (irq)
>        /unnamed-gpio-in[30] (irq)
>        /unnamed-gpio-in[31] (irq)
>        /unnamed-gpio-in[3] (irq)
>        /unnamed-gpio-in[4] (irq)
>        /unnamed-gpio-in[5] (irq)
>        /unnamed-gpio-in[6] (irq)
>        /unnamed-gpio-in[7] (irq)
>        /unnamed-gpio-in[8] (irq)
>        /unnamed-gpio-in[9] (irq)
>      /device[9] (platform-bus-device)
>        /platform bus[0] (memory-region)

Actually most of these do have a QOM parent.

Correctly placing them in the tree should help when trying to
resolve a component and avoiding an ambiguous match.

>      /io[0] (memory-region)
>      /non-qdev-gpio[0] (irq)
>      /sysbus (System)
>      /system[0] (memory-region)
> 

Re: [RFC PATCH 0/5] hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths

[Daniel Henrique Barboza]

On 11/2/23 04:49, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
> 
> On 31/10/23 22:49, Daniel Henrique Barboza wrote:
>> On 10/30/23 11:39, Philippe Mathieu-Daudé wrote:
>>> Following the discussion with Peter on my "cpus: Step toward
>>> removing global 'first_cpu'" series [*], we now pass the array
>>> of CPUs via property. Since we can not pass array of "link"
>>> properties, we pass the QOM path of each CPU as a QList(String).
>>>
>>> Tagged as RFC to discuss the idea of using qdev_prop_set_array
>>> with qlist_append_str(object_get_canonical_path). Personally I
>>> find it super simple.
>>
>> I probably misunderstood the concept/problem but "super simple" is not the first
>> thing that came to my mind in patch 5 hehe
>>
>> I didn't follow the whole thread, just the [*] message marked and a couple
>> of replies, but are we planning to deprecate qemu_get_cpu()? Patch 5 mentions
>> "Devices should avoid calling qemu_get_cpu() because this call doesn't work
>> as expected with heterogeneous machines". I'll take your word for it. But
>> e500 isn't an heterogeneous machine - we're creating ppc cpus only. So I'm
>> a bit confused here. Are you using e500 just as a simple PoC?
> 
> I'm using the e500 as the simplest complex device using qemu_get_cpu :)

Fair enough :D

> 
> Indeed, in [*] Peter said not all qemu_get_cpu() calls are harmful. In
> particular in homogeneous machines.
> 
> Looking back at the e500, the problem isn't the *machine*, but a device
> it is using.
> 
> Taking "dynamic machines" as a step toward "heterogeneous machines",
> I'm considering all devices potentially creatable dynamically, again
> potentially ending being use in some heterogeneous prototype. So I'd
> rather have all devices using the same API without worrying whether
> the device is designed for heterogeneous machine or not. The simplest
> way I found to achieve that, is to restrict qemu_get_cpu() to accel/
> and system/ -- where a vCPU arch is irrelevant --, but prohibit it for
> hw/ files. Maybe I'm wrong and this isn't the best way to go, which is
> why I tried this RFC, expecting constructive comments like yours, thanks
> for that!

Thanks for the clarification! Let's see what the QOM experts have to say
about it.



Thanks,

Daniel

> 
>> Regardless of the reason to use e500 in this RFC, I believe we would benefit
>> from having common helpers/magic sauce macros to add all this apparently
>> boilerplate code to replace qemu_get_cpu().
>>
>> I mean, we're changing this:
>>
>> -    cpu = qemu_get_cpu(env_idx);
>> -    if (cpu == NULL) {
>> -        /* Unknown CPU */
>> -        return;
>> -    }
>> -
>>
>> For a lot of QOM stuff like this:
>>
>>
>> +        cpu_qompath = object_get_canonical_path(OBJECT(cs));
>> +        qlist_append_str(spin_cpu_list, cpu_qompath);
>> +    qdev_prop_set_array(dev, "cpus-qom-path", spin_cpu_list);
>>
>>
>> +    if (s->cpu_count == 0) {
>> +        error_setg(errp, "'cpus-qom-path' property array must be set");
>> +        return;
>> +    } else if (s->cpu_count > MAX_CPUS) {
>> +        error_setg(errp, "at most %d CPUs are supported", MAX_CPUS);
>> +        return;
>> +    }
>> +
>> +    s->cpu = g_new(CPUState *, s->cpu_count);
>> +    for (unsigned i = 0; i < s->cpu_count; i++) {
>> +        bool ambiguous;
>> +        Object *obj;
>> +
>> +        obj = object_resolve_path(s->cpu_canonical_path[i], &ambiguous);
>> +        assert(!ambiguous);
>> +        s->cpu[i] = CPU(obj);
>> +    }
>> +
>> +static Property ppce500_spin_properties[] = {
>> +    DEFINE_PROP_ARRAY("cpus-qom-path", SpinState, cpu_count,
>> +                      cpu_canonical_path, qdev_prop_string, char *),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>
>> So anything that makes the QOM stuff more palatable to deal with would be
>> really appreciated. Thanks,
> 
> Yeah, I'll see what I can do here. But first I'd like some feedback
> from QOM experts on whether using QOM canonical path is good or not.
> 
> Markus, Paolo (which I forgot to Cc...)?
> 
>> Daniel
>>
>>>
>>> Regards,
>>>
>>> Phil.
>>>
>>> [*] https://lore.kernel.org/qemu-devel/CAFEAcA9FT+QMyQSLCeLjd7tEfaoS9JazmkYWQE++s1AmF7Nfvw@mail.gmail.com/
>>>
>>> Kevin Wolf (1):
>>>    qdev: Add qdev_prop_set_array()
>>>
>>> Philippe Mathieu-Daudé (4):
>>>    hw/ppc/e500: Declare CPU QOM types using DEFINE_TYPES() macro
>>>    hw/ppc/e500: QOM-attach CPUs to the machine container
>>>    hw/ppc/e500: Inline sysbus_create_simple(E500_SPIN)
>>>    hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths
>>>
>>>   include/hw/qdev-properties.h |  3 ++
>>>   hw/core/qdev-properties.c    | 21 +++++++++++
>>>   hw/ppc/e500.c                | 11 +++++-
>>>   hw/ppc/ppce500_spin.c        | 69 ++++++++++++++++++++++++++----------
>>>   4 files changed, 84 insertions(+), 20 deletions(-)
>>>
> 

Re: [PATCH 3/5] hw/ppc/e500: QOM-attach CPUs to the machine container

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 3/11/23 08:40, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> Instead of having CPUs dangling in the /unattached/device
>>> bucket, attach them to the machine container.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/ppc/e500.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index e04114fb3c..f8177c0280 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -946,6 +946,7 @@ void ppce500_init(MachineState *machine)
>>>               exit(1);
>>>           }
>>>   +        object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cs));
>>>           /*
>>>            * Secondary CPU starts in halted state for now. Needs to change
>>>            * when implementing non-kernel boot.
>> A peek at "info qom-tree" confirms the CPU is in /machine/unattached/.
>> Along with most onboard devices.  Details below.
>>
>> Quick test...  I count 563 machines.  394 seem to have CPU(s) in or
>> below /machine/unattached/, 129 elsewhere, and 40 I can't easily
>> examine, because they don't start to monitor without additional CLI
>> arguments.
>>
>> Where should CPUs be?
>
> It is machine specific.
>
> - For System-on-Chip, it would be in /soc
>
> - For systems that fully model CPU topology, I'd expect a consistent
>   topology path. (If it is part of a cluster, in that /cluster).
>
> - For mainframes, it should be part of the CPU cards that can be
>   inserted?
>
> - For a single Pentium CPU, maybe /machine is sufficient.
>
>> Is /machine/unattached/ basically where we dump products of lazy
>> modelling?
>
> Unfortunately, yes. Also where CLI created devices are I guess.

No, these go into /machine/peripheral/ (with id=...) or
/machine/peripheral-anon/ (without).

/unattached has a different role: it's where objects without a parent go
when a parent is needed.  For instance, when a device without a QOM
parent gets realized, device_set_realized() makes it a child of
/unattached/.  Similar logic in hw/core/gpio.c, system/ioport.c and
system/memory.c.

>> If yes, should we try to empty it out?
>
> If it is useful. For components expected to be referenced externally
> by humans, probably. If only used by scripts, maybe not, except if
> human have to debug.
>
>> If we shouldn't, then why move this one out?
>
> When looking for a component in the tree, I start to look at /machine,
> having to fish for it elsewhere is not very natural. I'd change your
> question by:
> - Why do we need /unattached?

Because we can't be bothered to pick parents?

Perhaps an excusable shortcut when we had to convert a big pile of
devices to QOM.  But we take the shortcut for new objects, too.
Surprise, surprise.

> or
> - Why do we have 2 different folders, /machine and /unattached?
> If it is a headache, why not just simply merge them both?

I guess a justification for having both could be:

    /machine/: somebody spent a brain wave or two on the proper parent

    /unattached/: what's a parent, and why should I care?

Merging them would lose information.  Do we care?

>> $ qemu-system-ppc -nodefaults -S -display none -M ppce500 -monitor stdio
>> QEMU 8.1.50 monitor - type 'help' for more information
>> (qemu) info qom-tree
>> /machine (ppce500-machine)
>>    /e500-ccsr (e500-ccsr)
>>      /e500-ccsr[0] (memory-region)
>>      /e500-pci-bar0[0] (memory-region)
>>    /pci-host (e500-pcihost)
>>      /bm-e500[0] (memory-region)
>>      /pci bus memory[0] (memory-region)
>>      /pci-conf-data[0] (memory-region)
>>      /pci-conf-idx[0] (memory-region)
>>      /pci-container[0] (memory-region)
>>      /pci-pio[0] (memory-region)
>>      /pci.0 (PCI)
>>      /pci.reg[0] (memory-region)
>>    /peripheral (container)
>>    /peripheral-anon (container)
> [...]
>
>>    /unattached (container)
>>      /device[0] (e500v2_v30-powerpc-cpu)
>>        /unnamed-gpio-in[0] (irq)
>>        /unnamed-gpio-in[1] (irq)
>>        /unnamed-gpio-in[2] (irq)
>>        /unnamed-gpio-in[3] (irq)
>>        /unnamed-gpio-in[4] (irq)
>>        /unnamed-gpio-in[5] (irq)
>>        /unnamed-gpio-in[6] (irq)
>>      /device[1] (mpc-i2c)
>>        /i2c (i2c-bus)
>>        /mpc-i2c[0] (memory-region)
>>      /device[2] (ds1338)
>>      /device[3] (unimplemented-device)
>>        /esdhc[0] (memory-region)
>>      /device[4] (generic-sdhci)
>>        /sd-bus (sdhci-bus)
>>        /sdhci[0] (memory-region)
>>      /device[5] (mpc8544-guts)
>>        /mpc8544.guts[0] (memory-region)
>>      /device[6] (e500-host-bridge)
>>        /bus master container[0] (memory-region)
>>        /bus master[0] (memory-region)
>>      /device[7] (e500-spin)
>>        /e500 spin pv device[0] (memory-region)
>>      /device[8] (mpc8xxx_gpio)
>>        /mpc8xxx_gpio[0] (memory-region)
>>        /unnamed-gpio-in[0] (irq)
>>        /unnamed-gpio-in[10] (irq)
>>        /unnamed-gpio-in[11] (irq)
>>        /unnamed-gpio-in[12] (irq)
>>        /unnamed-gpio-in[13] (irq)
>>        /unnamed-gpio-in[14] (irq)
>>        /unnamed-gpio-in[15] (irq)
>>        /unnamed-gpio-in[16] (irq)
>>        /unnamed-gpio-in[17] (irq)
>>        /unnamed-gpio-in[18] (irq)
>>        /unnamed-gpio-in[19] (irq)
>>        /unnamed-gpio-in[1] (irq)
>>        /unnamed-gpio-in[20] (irq)
>>        /unnamed-gpio-in[21] (irq)
>>        /unnamed-gpio-in[22] (irq)
>>        /unnamed-gpio-in[23] (irq)
>>        /unnamed-gpio-in[24] (irq)
>>        /unnamed-gpio-in[25] (irq)
>>        /unnamed-gpio-in[26] (irq)
>>        /unnamed-gpio-in[27] (irq)
>>        /unnamed-gpio-in[28] (irq)
>>        /unnamed-gpio-in[29] (irq)
>>        /unnamed-gpio-in[2] (irq)
>>        /unnamed-gpio-in[30] (irq)
>>        /unnamed-gpio-in[31] (irq)
>>        /unnamed-gpio-in[3] (irq)
>>        /unnamed-gpio-in[4] (irq)
>>        /unnamed-gpio-in[5] (irq)
>>        /unnamed-gpio-in[6] (irq)
>>        /unnamed-gpio-in[7] (irq)
>>        /unnamed-gpio-in[8] (irq)
>>        /unnamed-gpio-in[9] (irq)
>>      /device[9] (platform-bus-device)
>>        /platform bus[0] (memory-region)
>
> Actually most of these do have a QOM parent.

They don't or else they wouldn't be here.  Do you mean "the proper
parent is obvious"?

> Correctly placing them in the tree should help when trying to
> resolve a component and avoiding an ambiguous match.

Yes.

>>      /io[0] (memory-region)
>>      /non-qdev-gpio[0] (irq)
>>      /sysbus (System)
>>      /system[0] (memory-region)