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

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(-)

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[] = {

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(-)

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)?