On Mon, 13 Jan 2025 17:00:55 +0100 Philippe Mathieu-Daudé <[email protected]> wrote:
> On 13/1/25 13:28, Igor Mammedov wrote: > > On Sun, 12 Jan 2025 23:16:40 +0100 > > Philippe Mathieu-Daudé <[email protected]> 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é <[email protected]> > >> Reviewed-by: Zhao Liu <[email protected]> > >> Message-Id: <[email protected]> > >> --- > >> 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/[email protected]/ 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: >
