| Message ID | 20231030143957.82988-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/ppc/e500: Pass array of CPUs as array of canonical QOM paths | expand |
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(-) >
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(-) >>
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 = {
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(-) >>> >