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