| Message ID | 20250112221726.30206-5-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/49] pc-bios/meson.build: Silent unuseful DTC warnings | expand |
On Sun, 12 Jan 2025 23:16:40 +0100 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > QDev objects created with object_new() need to manually add > their parent relationship with object_property_add_child(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Zhao Liu <zhao1.liu@intel.com> > Message-Id: <20240216110313.17039-22-philmd@linaro.org> > --- > hw/i386/x86-common.c | 1 + > hw/microblaze/petalogix_ml605_mmu.c | 1 + > hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + > hw/mips/cps.c | 1 + > hw/ppc/e500.c | 1 + > hw/ppc/spapr.c | 1 + > 6 files changed, 6 insertions(+) > > diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c > index 97b4f7d4a0d..9c9ffb3484a 100644 > --- a/hw/i386/x86-common.c > +++ b/hw/i386/x86-common.c > @@ -60,6 +60,7 @@ static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) > if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { > goto out; > } > + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); I might be missing something but why it needs to be done manually? device_set_realized() will place any parent-less device under (1) /machine/unattached while devices created with device_add() are be placed under /machine/peripheral[-anon] The commit message unfortunately doesn't explain why [1] shall be replaced by direct cpu[*] array property directly under machine. Granted, those paths aren't any kind of ABI and wrt x86 cpus nothing should break (or I'd say it shouldn't break our promises) But I'd rather not do this without a good reason/explanation. > qdev_realize(DEVICE(cpu), NULL, errp); > > out: > diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c > index 8b44be75a22..b6be40915ac 100644 > --- a/hw/microblaze/petalogix_ml605_mmu.c > +++ b/hw/microblaze/petalogix_ml605_mmu.c > @@ -83,6 +83,7 @@ petalogix_ml605_init(MachineState *machine) > > /* init CPUs */ > cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU)); > + object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu)); > object_property_set_str(OBJECT(cpu), "version", "8.10.a", &error_abort); > /* Use FPU but don't use floating point conversion and square > * root instructions > diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c > index 2c0d8c34cd2..29629310ba2 100644 > --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c > +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c > @@ -73,6 +73,7 @@ petalogix_s3adsp1800_init(MachineState *machine) > MemoryRegion *sysmem = get_system_memory(); > > cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU)); > + object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu)); > object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort); > object_property_set_bool(OBJECT(cpu), "little-endian", > !TARGET_BIG_ENDIAN, &error_abort); > diff --git a/hw/mips/cps.c b/hw/mips/cps.c > index 0d8cbdc8924..293b405b965 100644 > --- a/hw/mips/cps.c > +++ b/hw/mips/cps.c > @@ -87,6 +87,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) > /* All cores use the same clock tree */ > qdev_connect_clock_in(DEVICE(cpu), "clk-in", s->clock); > > + object_property_add_child(OBJECT(dev), "cpu[*]", OBJECT(cpu)); > if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) { > return; > } > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c > index 4551157c011..17d63ced907 100644 > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -955,6 +955,7 @@ void ppce500_init(MachineState *machine) > */ > object_property_set_bool(OBJECT(cs), "start-powered-off", i != 0, > &error_abort); > + object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cpu)); > qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal); > > if (!firstenv) { > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 623842f8064..125be6d29fd 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2705,6 +2705,7 @@ static void spapr_init_cpus(SpaprMachineState *spapr) > &error_fatal); > object_property_set_int(core, CPU_CORE_PROP_CORE_ID, core_id, > &error_fatal); > + object_property_add_child(OBJECT(spapr), "cpu[*]", OBJECT(core)); > qdev_realize(DEVICE(core), NULL, &error_fatal); > > object_unref(core);
On 13/1/25 13:28, Igor Mammedov wrote: > On Sun, 12 Jan 2025 23:16:40 +0100 > Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> QDev objects created with object_new() need to manually add >> their parent relationship with object_property_add_child(). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Zhao Liu <zhao1.liu@intel.com> >> Message-Id: <20240216110313.17039-22-philmd@linaro.org> >> --- >> hw/i386/x86-common.c | 1 + >> hw/microblaze/petalogix_ml605_mmu.c | 1 + >> hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + >> hw/mips/cps.c | 1 + >> hw/ppc/e500.c | 1 + >> hw/ppc/spapr.c | 1 + >> 6 files changed, 6 insertions(+) >> >> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c >> index 97b4f7d4a0d..9c9ffb3484a 100644 >> --- a/hw/i386/x86-common.c >> +++ b/hw/i386/x86-common.c >> @@ -60,6 +60,7 @@ static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) >> if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { >> goto out; >> } >> + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); > > I might be missing something but why it needs to be done manually? > > device_set_realized() will place any parent-less device under (1) /machine/unattached This is exactly what we want to avoid, to eventually remove the "/machine/unattached" container for good. See "= Problem 4: The /machine/unattached/ orphanage =" in: https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/ > while devices created with device_add() are be placed under /machine/peripheral[-anon] > > The commit message unfortunately doesn't explain why [1] shall be replaced > by direct cpu[*] array property directly under machine. Right. I'll drop for now and respin once reworded. > Granted, those paths aren't any kind of ABI and wrt x86 cpus > nothing should break (or I'd say it shouldn't break our promises) > But I'd rather not do this without a good reason/explanation. > >> qdev_realize(DEVICE(cpu), NULL, errp); >> >> out:
On Mon, 13 Jan 2025 17:00:55 +0100 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 13/1/25 13:28, Igor Mammedov wrote: > > On Sun, 12 Jan 2025 23:16:40 +0100 > > Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > >> QDev objects created with object_new() need to manually add > >> their parent relationship with object_property_add_child(). > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> Reviewed-by: Zhao Liu <zhao1.liu@intel.com> > >> Message-Id: <20240216110313.17039-22-philmd@linaro.org> > >> --- > >> hw/i386/x86-common.c | 1 + > >> hw/microblaze/petalogix_ml605_mmu.c | 1 + > >> hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + > >> hw/mips/cps.c | 1 + > >> hw/ppc/e500.c | 1 + > >> hw/ppc/spapr.c | 1 + > >> 6 files changed, 6 insertions(+) > >> > >> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c > >> index 97b4f7d4a0d..9c9ffb3484a 100644 > >> --- a/hw/i386/x86-common.c > >> +++ b/hw/i386/x86-common.c > >> @@ -60,6 +60,7 @@ static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) > >> if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { > >> goto out; > >> } > >> + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); > > > > I might be missing something but why it needs to be done manually? > > > > device_set_realized() will place any parent-less device under (1) /machine/unattached > > This is exactly what we want to avoid, to eventually remove > the "/machine/unattached" container for good. > > See "= Problem 4: The /machine/unattached/ orphanage =" in: > https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/ QOM paths as far as I'm aware were never part ABI nor I'm aware of of any proposal to make it or some parts of it a public interface. IMHO for public ABI, QEMU provides explicit QMP commands while QOM should stay a playground for developers. I this specific case, one basically replaces /machine/unattached orphanage with explicit /machine one and many 'cpuN' children, which ain't any better than device[N]. and in future I can imagine that at least in x86 case vcpus might have another parent depending on configuration. (i.e. being parented to cores instead) If goal is to get rid of /machine/unattached, that's fine. But please not make brittle naming under /machine/unattached as a reason as 'cpu[N]' is the same just in different place and scattered all over code (hence doubts if it's any better than current way). (ps: don't we have exactly the same for peripheral-anon container) > > while devices created with device_add() are be placed under /machine/peripheral[-anon] > > > > The commit message unfortunately doesn't explain why [1] shall be replaced > > by direct cpu[*] array property directly under machine. > > Right. I'll drop for now and respin once reworded. > > > Granted, those paths aren't any kind of ABI and wrt x86 cpus > > nothing should break (or I'd say it shouldn't break our promises) > > But I'd rather not do this without a good reason/explanation. > > > >> qdev_realize(DEVICE(cpu), NULL, errp); > >> > >> out: >
Igor Mammedov <imammedo@redhat.com> writes: > On Mon, 13 Jan 2025 17:00:55 +0100 > Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> On 13/1/25 13:28, Igor Mammedov wrote: >> > On Sun, 12 Jan 2025 23:16:40 +0100 >> > Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> > >> >> QDev objects created with object_new() need to manually add >> >> their parent relationship with object_property_add_child(). >> >> >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> Reviewed-by: Zhao Liu <zhao1.liu@intel.com> >> >> Message-Id: <20240216110313.17039-22-philmd@linaro.org> >> >> --- >> >> hw/i386/x86-common.c | 1 + >> >> hw/microblaze/petalogix_ml605_mmu.c | 1 + >> >> hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + >> >> hw/mips/cps.c | 1 + >> >> hw/ppc/e500.c | 1 + >> >> hw/ppc/spapr.c | 1 + >> >> 6 files changed, 6 insertions(+) >> >> >> >> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c >> >> index 97b4f7d4a0d..9c9ffb3484a 100644 >> >> --- a/hw/i386/x86-common.c >> >> +++ b/hw/i386/x86-common.c >> >> @@ -60,6 +60,7 @@ static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) >> >> if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { >> >> goto out; >> >> } >> >> + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); >> > >> > I might be missing something but why it needs to be done manually? >> > >> > device_set_realized() will place any parent-less device under (1) /machine/unattached >> >> This is exactly what we want to avoid, to eventually remove >> the "/machine/unattached" container for good. >> >> See "= Problem 4: The /machine/unattached/ orphanage =" in: >> https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/ > > > QOM paths as far as I'm aware were never part ABI nor I'm aware of > of any proposal to make it or some parts of it a public interface. We've been waffling on this since forever. QOM is not a public interface except when it is, and it is when somebody says so, and it isn't when somebody says so, resulting in a wave function that wobbles like an underdone souffle, but never quite collapses. > IMHO for public ABI, QEMU provides explicit QMP commands while > QOM should stay a playground for developers. Plenty of commands take QOM paths as arguments: eject, blockdev-open-tray, blockdev-close-tray, blockdev-remove-medium, blockdev-insert-medium, blockdev-change-medium, block-latency-histogram-set, cxl-inject-general-media-event, cxl-inject-dram-event, cxl-inject-memory-module-event, cxl-inject-poison, cxl-inject-uncorrectable-errors, cxl-inject-correctable-error, device_del, device-sync-config, query-stats, x-query-virtio-status, x-query-virtio-queue-status, x-query-virtio-vhost-queue-status, x-query-virtio-queue-element, and possibly more. The only way their QOM path arguments can be used without relying on QOM paths being ABI would be obtaining the argument value with a command or from an event. I doubt that would be possible even if we tried it, which we haven't. > I this specific case, one basically replaces /machine/unattached > orphanage with explicit /machine one and many 'cpuN' children, > which ain't any better than device[N]. > > and in future I can imagine that at least in x86 case vcpus > might have another parent depending on configuration. > (i.e. being parented to cores instead) > > If goal is to get rid of /machine/unattached, that's fine. /machine/unattached was a lazy mistake. > But please not make brittle naming under /machine/unattached > as a reason as 'cpu[N]' is the same just in different place > and scattered all over code (hence doubts if it's any better than current way). Can you suggest a better, workable naming scheme? > (ps: don't we have exactly the same for peripheral-anon container) Yes, but users can avoid that by passing an @id argument. [...]
> I this specific case, one basically replaces /machine/unattached > orphanage with explicit /machine one and many 'cpuN' children, > which ain't any better than device[N]. > > and in future I can imagine that at least in x86 case vcpus > might have another parent depending on configuration. > (i.e. being parented to cores instead) I remember that this was your idea all along, and I'm not sure if you're also referring to my previous patches about hybrid topology :-), which I'll continue to refresh afterward in future (after all, the hybrid architecture will continue in x86). And I think, since socket/core/thread are the three default QEMU topology hierarchies, I understand that it would be best for thread to always have core as parent.
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c index 97b4f7d4a0d..9c9ffb3484a 100644 --- a/hw/i386/x86-common.c +++ b/hw/i386/x86-common.c @@ -60,6 +60,7 @@ static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { goto out; } + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); qdev_realize(DEVICE(cpu), NULL, errp); out: diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 8b44be75a22..b6be40915ac 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -83,6 +83,7 @@ petalogix_ml605_init(MachineState *machine) /* init CPUs */ cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU)); + object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu)); object_property_set_str(OBJECT(cpu), "version", "8.10.a", &error_abort); /* Use FPU but don't use floating point conversion and square * root instructions diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index 2c0d8c34cd2..29629310ba2 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -73,6 +73,7 @@ petalogix_s3adsp1800_init(MachineState *machine) MemoryRegion *sysmem = get_system_memory(); cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU)); + object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu)); object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort); object_property_set_bool(OBJECT(cpu), "little-endian", !TARGET_BIG_ENDIAN, &error_abort); diff --git a/hw/mips/cps.c b/hw/mips/cps.c index 0d8cbdc8924..293b405b965 100644 --- a/hw/mips/cps.c +++ b/hw/mips/cps.c @@ -87,6 +87,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) /* All cores use the same clock tree */ qdev_connect_clock_in(DEVICE(cpu), "clk-in", s->clock); + object_property_add_child(OBJECT(dev), "cpu[*]", OBJECT(cpu)); if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) { return; } diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 4551157c011..17d63ced907 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -955,6 +955,7 @@ void ppce500_init(MachineState *machine) */ object_property_set_bool(OBJECT(cs), "start-powered-off", i != 0, &error_abort); + object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cpu)); qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal); if (!firstenv) { diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 623842f8064..125be6d29fd 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2705,6 +2705,7 @@ static void spapr_init_cpus(SpaprMachineState *spapr) &error_fatal); object_property_set_int(core, CPU_CORE_PROP_CORE_ID, core_id, &error_fatal); + object_property_add_child(OBJECT(spapr), "cpu[*]", OBJECT(core)); qdev_realize(DEVICE(core), NULL, &error_fatal); object_unref(core);