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