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

[Igor Mammedov]

On Tue, 14 Jan 2025 13:38:59 +0100
Markus Armbruster <armbru@redhat.com> wrote:

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

well, unless draw a line somewhere it will never stop.
Perhaps we should find on some border where QOM exposure stops
and document it. So whenever question pops up again, one could be
sent there.

all x- commands could be ignored, prefix tells no promises whatsoever,
cxl- group all new and doesn't have excuse to expose QOM, but not
many pay attention it subsystem considering it as platform bring up effort

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

hotpluggable-cpu command might be an example (it returns vcpu path,
which is valid within vcpu lifetime). But then again it's for
devs convenience. 

What I don't like about exposing QOM is 

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

no argument here

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

nope, that's why I'm not arguing against it (modulo voicing my doubts)

PS:
Another question is if it's safe to move/rename device withing QOM tree
wrt migration (i.e. when 1st instance has old QOM tree and 2nd a modified one)

quick smoke test works (migrating from old qemu to a new one with this patch)
But it's better to ask to be safe.

> > (ps: don't we have exactly the same for peripheral-anon container)  
> 
> Yes, but users can avoid that by passing an @id argument.
> 
> [...]
>