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.
>
> [*] 
> CAFEAcA9FT+QMyQSLCeLjd7tEfaoS9JazmkYWQE++s1AmF7Nfvw@mail.gmail.com/">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(-)