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

[Markus Armbruster]

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.

[...]