Re: [PULL 04/49] hw: Add QOM parentship relation with CPUs

[Igor Mammedov]

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:  
>