Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine
On Mon, 7 Feb 2022 09:14:37 +0100
Igor Mammedov <imammedo(a)redhat.com> wrote:
On Sat, 5 Feb 2022 13:45:24 +0100
Philippe Mathieu-Daudé <f4bug(a)amsat.org> wrote:
> Previously CPUs were exposed in the QOM tree at a path
>
> /machine/unattached/device[nn]
>
> where the 'nn' of the first CPU is usually zero, but can
> vary depending on what devices were already created.
>
> With this change the CPUs are now at
>
> /machine/cpu[nn]
>
> where the 'nn' of the first CPU is always zero.
Could you add to commit message the reason behind the change?
regardless, it looks like unwarranted movement to me
prompted by livirt accessing/expecting a QOM patch which is
not stable ABI. I'd rather get it fixed on libvirt side.
If libvirt needs for some reason access a CPU instance,
it should use @query-hotpluggable-cpus to get a list of CPUs
(which includes QOM path of already present CPUs) instead of
hard-codding some 'well-known' path as there is no any guarantee
that it will stay stable whatsoever.
> Note: This (intentionally) breaks compatibility with current
> libvirt code that looks for "/machine/unattached/device[0]"
> in the assumption it is the first CPU.
Why libvirt does this in the first place?
> Cc: libvir-list(a)redhat.com
> Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug(a)amsat.org>
> ---
> hw/i386/x86.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb9..50bf249c700 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error
**errp)
> {
> Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
>
> + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu));
that will take in account only initial cpus, -device/device_add cpus
will still go to wherever device_add attaches them (see qdev_set_id)
> if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
> goto out;
> }