Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Andreas Färber]

First I did not participate in that discussion, second nack to that self pointer. Please hold off on this until I'm back. Andreas

Eduardo Habkost <ehabkost@redhat.com> schrieb:

>This will provide a predictable path for the CPU objects, and a more
>powerful alternative for the query-cpus QMP command, as now every QOM
>property on CPU objects can be easily queried.
>
>Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>---
>Note that this doesn't replace any future topology enumeration
>mechanisms we may choose to implement. It just replaces the existing
>topology-unaware VCPU enumeration mechanism that is query-cpus.
>
>Reference to previous discussion:
>
>  Date: Thu, 23 Apr 2015 10:17:36 -0300
>  From: Eduardo Habkost <ehabkost@redhat.com>
>  Subject: Re: [Qemu-devel] cpu modelling and hotplug (was: [PATCH RFC 0/4] target-i386: PC socket/core/thread modeling, part 1)
>  Message-ID: <20150423131736.GA17796@thinpad.lan.raisama.net>
>---
> exec.c            | 16 ++++++++++++++++
> include/qom/cpu.h |  3 +++
> 2 files changed, 19 insertions(+)
>
>diff --git a/exec.c b/exec.c
>index ae37b98..8bdfa65 100644
>--- a/exec.c
>+++ b/exec.c
>@@ -519,6 +519,20 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
> }
> #endif
> 
>+static void cpu_add_qom_link(CPUState *cpu)
>+{
>+#if !defined(CONFIG_USER_ONLY)
>+    Object *cobj = OBJECT(cpu);
>+    Object *cpu_container = container_get(OBJECT(current_machine), "/cpus");
>+    char *path = g_strdup_printf("%d", cpu->cpu_index);
>+
>+    cpu->self = cobj;
>+    object_property_add_link(cpu_container, path, TYPE_CPU, &cpu->self, NULL,
>+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
>+    g_free(path);
>+#endif
>+}
>+
> void cpu_exec_init(CPUArchState *env)
> {
>     CPUState *cpu = ENV_GET_CPU(env);
>@@ -558,6 +572,8 @@ void cpu_exec_init(CPUArchState *env)
>     if (cc->vmsd != NULL) {
>         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>     }
>+
>+    cpu_add_qom_link(cpu);
> }
> 
> #if defined(CONFIG_USER_ONLY)
>diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>index 39f0f19..c253420 100644
>--- a/include/qom/cpu.h
>+++ b/include/qom/cpu.h
>@@ -246,6 +246,9 @@ struct CPUState {
>     DeviceState parent_obj;
>     /*< public >*/
> 
>+    /* Needed for the /machine/cpus/<index> link */
>+    Object *self;
>+
>     int nr_cores;
>     int nr_threads;
>     int numa_node;
>-- 
>2.1.0
>

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Paolo Bonzini]

On 30/04/2015 22:21, Andreas Färber wrote:
>>+    cpu->self = cobj;
>>+    object_property_add_link(cpu_container, path, TYPE_CPU, &cpu->self, NULL,
>>+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);

Doesn't this leak the CPU object?

I have a patch to add "." and ".." properties.  You can use them to add
an alias to an object.

Paolo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Peter Crosthwaite]

On Thu, Apr 30, 2015 at 2:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 30/04/2015 22:21, Andreas Färber wrote:
>>>+    cpu->self = cobj;
>>>+    object_property_add_link(cpu_container, path, TYPE_CPU, &cpu->self, NULL,
>>>+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
>
> Doesn't this leak the CPU object?
>
> I have a patch to add "." and ".." properties.  You can use them to add
> an alias to an object.
>

Curious, are we ultimately going for full canonical path navigation by
string? Going the other way, I still have application of setting a
property of a child object from reference to a parent that might fit
in:

http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03281.html

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Eduardo Habkost]

On Thu, Apr 30, 2015 at 11:47:09PM +0200, Paolo Bonzini wrote:
> 
> 
> On 30/04/2015 22:21, Andreas Färber wrote:
> >>+    cpu->self = cobj;
> >>+    object_property_add_link(cpu_container, path, TYPE_CPU, &cpu->self, NULL,
> >>+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
> 
> Doesn't this leak the CPU object?

I am not sure I follow. CPUs can't be destroyed today because there's no
code to remove them from the global cpus list.  The day we implement
cpu_exec_uninit(), it should undo everything done by cpu_exec_init()
including removing the link above and removing the CPU from the global
cpus list.

> 
> I have a patch to add "." and ".." properties.  You can use them to add
> an alias to an object.

What's the difference between a link and an alias to an object?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Paolo Bonzini]

On 01/05/2015 13:59, Eduardo Habkost wrote:
>> I have a patch to add "." and ".." properties.  You can use them to add
>> an alias to an object.
> 
> What's the difference between a link and an alias to an object?

A (read-only) link adds a reference to the target, an alias doesn't.

Paolo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Eduardo Habkost]

On Thu, Apr 30, 2015 at 10:21:55PM +0200, Andreas Färber wrote:
> First I did not participate in that discussion, second nack to that
> self pointer. Please hold off on this until I'm back. Andreas

I suggested it 3 times before. The message mentioned in the patch has
pointers to the other 2 discussions.

I wasn't happy with the self pointer, either. I will try to implement a
object_property_add_readonly_link() function that doesn't need Object**
as argument.

-- 
Eduardo

[Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Eduardo Habkost]

This will provide a predictable path for the CPU objects, and a more
powerful alternative for the query-cpus QMP command, as now every QOM
property on CPU objects can be easily queried.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Note that this doesn't replace any future topology enumeration
mechanisms we may choose to implement. It just replaces the existing
topology-unaware VCPU enumeration mechanism that is query-cpus.

Reference to previous discussion:

  Date: Thu, 23 Apr 2015 10:17:36 -0300
  From: Eduardo Habkost <ehabkost@redhat.com>
  Subject: Re: [Qemu-devel] cpu modelling and hotplug (was: [PATCH RFC 0/4] target-i386: PC socket/core/thread modeling, part 1)
  Message-ID: <20150423131736.GA17796@thinpad.lan.raisama.net>
---
 exec.c            | 16 ++++++++++++++++
 include/qom/cpu.h |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/exec.c b/exec.c
index ae37b98..8bdfa65 100644
--- a/exec.c
+++ b/exec.c
@@ -519,6 +519,20 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 }
 #endif
 
+static void cpu_add_qom_link(CPUState *cpu)
+{
+#if !defined(CONFIG_USER_ONLY)
+    Object *cobj = OBJECT(cpu);
+    Object *cpu_container = container_get(OBJECT(current_machine), "/cpus");
+    char *path = g_strdup_printf("%d", cpu->cpu_index);
+
+    cpu->self = cobj;
+    object_property_add_link(cpu_container, path, TYPE_CPU, &cpu->self, NULL,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
+    g_free(path);
+#endif
+}
+
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
@@ -558,6 +572,8 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+
+    cpu_add_qom_link(cpu);
 }
 
 #if defined(CONFIG_USER_ONLY)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39f0f19..c253420 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -246,6 +246,9 @@ struct CPUState {
     DeviceState parent_obj;
     /*< public >*/
 
+    /* Needed for the /machine/cpus/<index> link */
+    Object *self;
+
     int nr_cores;
     int nr_threads;
     int numa_node;
-- 
2.1.0

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Peter Crosthwaite]

On Thu, Apr 30, 2015 at 12:19 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> This will provide a predictable path for the CPU objects, and a more
> powerful alternative for the query-cpus QMP command, as now every QOM
> property on CPU objects can be easily queried.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Note that this doesn't replace any future topology enumeration
> mechanisms we may choose to implement. It just replaces the existing
> topology-unaware VCPU enumeration mechanism that is query-cpus.
>
> Reference to previous discussion:
>
>   Date: Thu, 23 Apr 2015 10:17:36 -0300
>   From: Eduardo Habkost <ehabkost@redhat.com>
>   Subject: Re: [Qemu-devel] cpu modelling and hotplug (was: [PATCH RFC 0/4] target-i386: PC socket/core/thread modeling, part 1)
>   Message-ID: <20150423131736.GA17796@thinpad.lan.raisama.net>
> ---
>  exec.c            | 16 ++++++++++++++++
>  include/qom/cpu.h |  3 +++
>  2 files changed, 19 insertions(+)
>
> diff --git a/exec.c b/exec.c
> index ae37b98..8bdfa65 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -519,6 +519,20 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
>  }
>  #endif
>
> +static void cpu_add_qom_link(CPUState *cpu)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    Object *cobj = OBJECT(cpu);
> +    Object *cpu_container = container_get(OBJECT(current_machine), "/cpus");
> +    char *path = g_strdup_printf("%d", cpu->cpu_index);
> +

This pathing is inconsistent with other arrayification efforts where
we use ../foo[N].
Do we need the container, can we just use /cpu[0], /cpu[1]?

> +    cpu->self = cobj;

Paolo's . property ideal is probably a winner, but see this discussion:

https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03557.html

for how to create RYO links with open coded setters and getters.
You could use the object itself as an opaque data and a trivial getter,
no setter and then you can get rid of the self pointer.

Regards,
Peter

> +    object_property_add_link(cpu_container, path, TYPE_CPU, &cpu->self, NULL,
> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
> +    g_free(path);
> +#endif
> +}
> +
>  void cpu_exec_init(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> @@ -558,6 +572,8 @@ void cpu_exec_init(CPUArchState *env)
>      if (cc->vmsd != NULL) {
>          vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>      }
> +
> +    cpu_add_qom_link(cpu);
>  }
>
>  #if defined(CONFIG_USER_ONLY)
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 39f0f19..c253420 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -246,6 +246,9 @@ struct CPUState {
>      DeviceState parent_obj;
>      /*< public >*/
>
> +    /* Needed for the /machine/cpus/<index> link */
> +    Object *self;
> +
>      int nr_cores;
>      int nr_threads;
>      int numa_node;
> --
> 2.1.0
>
>

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Eduardo Habkost]

On Thu, Apr 30, 2015 at 06:51:43PM -0700, Peter Crosthwaite wrote:
> On Thu, Apr 30, 2015 at 12:19 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > This will provide a predictable path for the CPU objects, and a more
> > powerful alternative for the query-cpus QMP command, as now every QOM
> > property on CPU objects can be easily queried.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Note that this doesn't replace any future topology enumeration
> > mechanisms we may choose to implement. It just replaces the existing
> > topology-unaware VCPU enumeration mechanism that is query-cpus.
> >
> > Reference to previous discussion:
> >
> >   Date: Thu, 23 Apr 2015 10:17:36 -0300
> >   From: Eduardo Habkost <ehabkost@redhat.com>
> >   Subject: Re: [Qemu-devel] cpu modelling and hotplug (was: [PATCH RFC 0/4] target-i386: PC socket/core/thread modeling, part 1)
> >   Message-ID: <20150423131736.GA17796@thinpad.lan.raisama.net>
> > ---
> >  exec.c            | 16 ++++++++++++++++
> >  include/qom/cpu.h |  3 +++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/exec.c b/exec.c
> > index ae37b98..8bdfa65 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -519,6 +519,20 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
> >  }
> >  #endif
> >
> > +static void cpu_add_qom_link(CPUState *cpu)
> > +{
> > +#if !defined(CONFIG_USER_ONLY)
> > +    Object *cobj = OBJECT(cpu);
> > +    Object *cpu_container = container_get(OBJECT(current_machine), "/cpus");
> > +    char *path = g_strdup_printf("%d", cpu->cpu_index);
> > +
> 
> This pathing is inconsistent with other arrayification efforts where
> we use ../foo[N].
> Do we need the container, can we just use /cpu[0], /cpu[1]?

Having a container looked simpler and cleaner than using cpu[0], cpu[1],
but I don't have a strong preference either way.

> 
> > +    cpu->self = cobj;
> 
> Paolo's . property ideal is probably a winner, but see this discussion:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03557.html
> 
> for how to create RYO links with open coded setters and getters.
> You could use the object itself as an opaque data and a trivial getter,
> no setter and then you can get rid of the self pointer.

True. But I will try to implement that in a generic way instead of
duplicating what's already implemented by object_get_link_property() and
object_resolve_link_property().

-- 
Eduardo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Igor Mammedov]

On Thu, 30 Apr 2015 16:19:07 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This will provide a predictable path for the CPU objects, and a more
> powerful alternative for the query-cpus QMP command, as now every QOM
> property on CPU objects can be easily queried.
provided the way cpu_index is generated, path won't be predictable/stable
with CPU unplug. I'd rather use DEVICE->id instead of cpu_index.


> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Note that this doesn't replace any future topology enumeration
> mechanisms we may choose to implement. It just replaces the existing
> topology-unaware VCPU enumeration mechanism that is query-cpus.
> 
> Reference to previous discussion:
> 
>   Date: Thu, 23 Apr 2015 10:17:36 -0300
>   From: Eduardo Habkost <ehabkost@redhat.com>
>   Subject: Re: [Qemu-devel] cpu modelling and hotplug (was: [PATCH RFC 0/4] target-i386: PC socket/core/thread modeling, part 1)
>   Message-ID: <20150423131736.GA17796@thinpad.lan.raisama.net>
> ---
>  exec.c            | 16 ++++++++++++++++
>  include/qom/cpu.h |  3 +++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index ae37b98..8bdfa65 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -519,6 +519,20 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
>  }
>  #endif
>  
> +static void cpu_add_qom_link(CPUState *cpu)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    Object *cobj = OBJECT(cpu);
> +    Object *cpu_container = container_get(OBJECT(current_machine), "/cpus");
> +    char *path = g_strdup_printf("%d", cpu->cpu_index);
> +
> +    cpu->self = cobj;
> +    object_property_add_link(cpu_container, path, TYPE_CPU, &cpu->self, NULL,
> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
> +    g_free(path);
> +#endif
> +}
> +
>  void cpu_exec_init(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> @@ -558,6 +572,8 @@ void cpu_exec_init(CPUArchState *env)
>      if (cc->vmsd != NULL) {
>          vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>      }
> +
> +    cpu_add_qom_link(cpu);
>  }
>  
>  #if defined(CONFIG_USER_ONLY)
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 39f0f19..c253420 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -246,6 +246,9 @@ struct CPUState {
>      DeviceState parent_obj;
>      /*< public >*/
>  
> +    /* Needed for the /machine/cpus/<index> link */
> +    Object *self;
> +
>      int nr_cores;
>      int nr_threads;
>      int numa_node;

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Paolo Bonzini]

On 04/05/2015 11:47, Igor Mammedov wrote:
> On Thu, 30 Apr 2015 16:19:07 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This will provide a predictable path for the CPU objects, and a more
> > powerful alternative for the query-cpus QMP command, as now every QOM
> > property on CPU objects can be easily queried.
> 
> provided the way cpu_index is generated, path won't be predictable/stable
> with CPU unplug. I'd rather use DEVICE->id instead of cpu_index.

Can we use the APIC id then?  Perhaps wrapped with a CPUState-level
method get_stable_processor_id()?

Paolo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Igor Mammedov]

On Mon, 04 May 2015 11:59:52 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 04/05/2015 11:47, Igor Mammedov wrote:
> > On Thu, 30 Apr 2015 16:19:07 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > This will provide a predictable path for the CPU objects, and a
> > > more powerful alternative for the query-cpus QMP command, as now
> > > every QOM property on CPU objects can be easily queried.
> > 
> > provided the way cpu_index is generated, path won't be
> > predictable/stable with CPU unplug. I'd rather use DEVICE->id
> > instead of cpu_index.
> 
> Can we use the APIC id then?  Perhaps wrapped with a CPUState-level
> method get_stable_processor_id()?
We have CPUClass->get_arch_id() which results in APIC id for
target-i386.
But I'd rather see an arbitrary DEVICE->id as index/name, that way
when -device cpu-foo,id=cpuXXX becomes functional we would have
1:1 mapping between CLI and /machine/cpus/ view.

> 
> Paolo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Paolo Bonzini]

On 04/05/2015 15:16, Igor Mammedov wrote:
>> > Can we use the APIC id then?  Perhaps wrapped with a CPUState-level
>> > method get_stable_processor_id()?
> We have CPUClass->get_arch_id() which results in APIC id for
> target-i386.
> But I'd rather see an arbitrary DEVICE->id as index/name, that way
> when -device cpu-foo,id=cpuXXX becomes functional we would have
> 1:1 mapping between CLI and /machine/cpus/ view.

CPUs would already be available at /machine/peripheral.  I think aliases
should provide alternative indexing whenever possible---not simply
filter by device type.

Paolo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Eduardo Habkost]

On Mon, May 04, 2015 at 03:19:32PM +0200, Paolo Bonzini wrote:
> 
> 
> On 04/05/2015 15:16, Igor Mammedov wrote:
> >> > Can we use the APIC id then?  Perhaps wrapped with a CPUState-level
> >> > method get_stable_processor_id()?
> > We have CPUClass->get_arch_id() which results in APIC id for
> > target-i386.
> > But I'd rather see an arbitrary DEVICE->id as index/name, that way
> > when -device cpu-foo,id=cpuXXX becomes functional we would have
> > 1:1 mapping between CLI and /machine/cpus/ view.
> 
> CPUs would already be available at /machine/peripheral.  I think aliases
> should provide alternative indexing whenever possible---not simply
> filter by device type.

Could you clarify what you mean by "alternative indexing"?

All I am trying to provide right now is having a predictable path for
CPUs, it doesn't matter if using -device, device_add, -smp, or cpu-add.
Filtering by device type is not what I need, here. If we can make the
CPU QOM path predictable inside /machine/peripheral or anywhere else,
that would be enough to me[1].

device IDs are predictable when using -device because they are in the
command-line. And they are predictable for -smp CPUs if we use cpu_index
as default ID.

Making the path depend on guest-visible bits that can change depending
on the architeture or machine would make the path less predictable.

I have an alternative patch that simply adds a "qom-path" field to
query-cpus. If we find out that making commitments about QOM paths is
too hard, I can submit it instead.

[1] Is there anybody or any document that can explain to me what all the
    containers inside /machine mean? I see /machine/peripheral,
    /machine/peripheral-anon, /machine/unattached, here, and I don't
    know what they mean.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Paolo Bonzini]

On 04/05/2015 16:05, Eduardo Habkost wrote:
> On Mon, May 04, 2015 at 03:19:32PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 04/05/2015 15:16, Igor Mammedov wrote:
>>>>> Can we use the APIC id then?  Perhaps wrapped with a CPUState-level
>>>>> method get_stable_processor_id()?
>>> We have CPUClass->get_arch_id() which results in APIC id for
>>> target-i386.
>>> But I'd rather see an arbitrary DEVICE->id as index/name, that way
>>> when -device cpu-foo,id=cpuXXX becomes functional we would have
>>> 1:1 mapping between CLI and /machine/cpus/ view.
>>
>> CPUs would already be available at /machine/peripheral.  I think aliases
>> should provide alternative indexing whenever possible---not simply
>> filter by device type.
> 
> [1] Is there anybody or any document that can explain to me what all the
>     containers inside /machine mean? I see /machine/peripheral,
>     /machine/peripheral-anon, /machine/unattached, here, and I don't
>     know what they mean.

/machine/peripheral/XYZ holds devices created with -device id=XYZ

/machine/peripheral-anon/device[NN] holds devices created without an id

/machine/unattached holds devices created by the board and not added
elsewhere through object_property_add_child.

> Could you clarify what you mean by "alternative indexing"?

A way to lookup devices of a particular kind.  An example of
"alternative indexing" is using pci.0/child[NN] to look up children of
the first PCI bus.

> All I am trying to provide right now is having a predictable path for
> CPUs, it doesn't matter if using -device, device_add, -smp, or cpu-add.
> Filtering by device type is not what I need, here.

Ok, so we're on the same page.  I would use any of:

- /machine/cpus/NN (your choice)

- /machine/cpu[NN] (Peter's choice)

- /machine/cpus/cpu[NN] (hybrid, resembles /machine/peripheral-anon or
/machine/unattached more)

I'm not sure if "NN" should be a random progressive number (in that case
you can use cpu[*] to let the QOM core pick the number) or the APIC ID.
 You know the domain better than I do.

> Making the path depend on guest-visible bits that can change depending
> on the architeture or machine would make the path less predictable.

You can still list all children of /machine/cpus.  The disadvantage of
the APIC ID is that IDs may have holes even without doing any
hot-unplug; the advantage is that, from a set of online CPUs in the
guest, you can predict the paths in /machine/cpus.

With cpu[*] instead you can have different contents of /machine/cpus
after for example

   cpu_add 3           # adds /machine/cpus/cpu[2] pointing to CPU 3
   cpu_add 2           # adds /machine/cpus/cpu[3] pointing to CPU 2

vs.

   cpu_add 2           # adds /machine/cpus/cpu[2] pointing to CPU 2
   cpu_add 3           # adds /machine/cpus/cpu[3] pointing to CPU 3


> I have an alternative patch that simply adds a "qom-path" field to
> query-cpus. If we find out that making commitments about QOM paths is
> too hard, I can submit it instead.

I don't think it's too hard, but this alternative patch may also make sense.

Paolo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Eduardo Habkost]

On Mon, May 04, 2015 at 05:53:59PM +0200, Paolo Bonzini wrote:
> 
> 
> On 04/05/2015 16:05, Eduardo Habkost wrote:
> > On Mon, May 04, 2015 at 03:19:32PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 04/05/2015 15:16, Igor Mammedov wrote:
> >>>>> Can we use the APIC id then?  Perhaps wrapped with a CPUState-level
> >>>>> method get_stable_processor_id()?
> >>> We have CPUClass->get_arch_id() which results in APIC id for
> >>> target-i386.
> >>> But I'd rather see an arbitrary DEVICE->id as index/name, that way
> >>> when -device cpu-foo,id=cpuXXX becomes functional we would have
> >>> 1:1 mapping between CLI and /machine/cpus/ view.
> >>
> >> CPUs would already be available at /machine/peripheral.  I think aliases
> >> should provide alternative indexing whenever possible---not simply
> >> filter by device type.
> > 
> > [1] Is there anybody or any document that can explain to me what all the
> >     containers inside /machine mean? I see /machine/peripheral,
> >     /machine/peripheral-anon, /machine/unattached, here, and I don't
> >     know what they mean.
> 
> /machine/peripheral/XYZ holds devices created with -device id=XYZ

So, if you know the ID of the CPU, we already have an easy mechanism to
look it up, and having device ID on /machine/cpus would be pointless
(and make the solution more complex because today the CPUs don't have
any device ID set).

> 
> /machine/peripheral-anon/device[NN] holds devices created without an id
> 
> /machine/unattached holds devices created by the board and not added
> elsewhere through object_property_add_child.
> 
> > Could you clarify what you mean by "alternative indexing"?
> 
> A way to lookup devices of a particular kind.  An example of
> "alternative indexing" is using pci.0/child[NN] to look up children of
> the first PCI bus.
> 
> > All I am trying to provide right now is having a predictable path for
> > CPUs, it doesn't matter if using -device, device_add, -smp, or cpu-add.
> > Filtering by device type is not what I need, here.
> 
> Ok, so we're on the same page.  I would use any of:
> 
> - /machine/cpus/NN (your choice)
> 
> - /machine/cpu[NN] (Peter's choice)
> 
> - /machine/cpus/cpu[NN] (hybrid, resembles /machine/peripheral-anon or
> /machine/unattached more)
> 
> I'm not sure if "NN" should be a random progressive number (in that case
> you can use cpu[*] to let the QOM core pick the number) or the APIC ID.
>  You know the domain better than I do.

You made a good point below, which make me want to use the APIC ID:

> 
> > Making the path depend on guest-visible bits that can change depending
> > on the architeture or machine would make the path less predictable.
> 
> You can still list all children of /machine/cpus.

That's true, and that's enough for clients that don't want/need to be
aware of the arch-specific ID. Most clients should treat the QOM path as
an arbitrary string, and in this case get_arch_id() is the simplest (and
more well-behaved) identifier we have.

> The disadvantage of
> the APIC ID is that IDs may have holes even without doing any
> hot-unplug; the advantage is that, from a set of online CPUs in the
> guest, you can predict the paths in /machine/cpus.

The other disavantage I was worrying about is that it doesn't let
clients predict the full QOM path of a CPU without knowing how to
calculate the arch-specific ID. But this is solved by simply listing all
children of /machine/cpus.

> 
> With cpu[*] instead you can have different contents of /machine/cpus
> after for example
> 
>    cpu_add 3           # adds /machine/cpus/cpu[2] pointing to CPU 3
>    cpu_add 2           # adds /machine/cpus/cpu[3] pointing to CPU 2
> 
> vs.
> 
>    cpu_add 2           # adds /machine/cpus/cpu[2] pointing to CPU 2
>    cpu_add 3           # adds /machine/cpus/cpu[3] pointing to CPU 3
> 
> 
> > I have an alternative patch that simply adds a "qom-path" field to
> > query-cpus. If we find out that making commitments about QOM paths is
> > too hard, I can submit it instead.
> 
> I don't think it's too hard, but this alternative patch may also make sense.

Well, the patch may be useful even with predictable paths: query-cpus
doesn't show the APIC ID, so adding a "qom-path" field would be useful
to correctly match QOM objects and information from query-cpus.

And if we add the qom-path field to query-cpus, we don't have the
immediate need for predictable QOM paths for CPUs anymore. Maybe we
should forget about /machine/cpus (because it will be osboleted by
topology aware CPU enumeration mechanisms in the future) and just apply
the qom-path patch.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>

[Eduardo Habkost]

On Mon, May 04, 2015 at 03:16:16PM +0200, Igor Mammedov wrote:
> On Mon, 04 May 2015 11:59:52 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > 
> > 
> > On 04/05/2015 11:47, Igor Mammedov wrote:
> > > On Thu, 30 Apr 2015 16:19:07 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > This will provide a predictable path for the CPU objects, and a
> > > > more powerful alternative for the query-cpus QMP command, as now
> > > > every QOM property on CPU objects can be easily queried.
> > > 
> > > provided the way cpu_index is generated, path won't be
> > > predictable/stable with CPU unplug. I'd rather use DEVICE->id
> > > instead of cpu_index.
> > 
> > Can we use the APIC id then?  Perhaps wrapped with a CPUState-level
> > method get_stable_processor_id()?
> We have CPUClass->get_arch_id() which results in APIC id for
> target-i386.
> But I'd rather see an arbitrary DEVICE->id as index/name, that way
> when -device cpu-foo,id=cpuXXX becomes functional we would have
> 1:1 mapping between CLI and /machine/cpus/ view.

An arbitrary device ID sounds better to me, because it allows us to
change guest-visible behavior without breaking QMP client expectations.

The only question is what should be the default device ID for the CPUs
created by -smp and cpu-add. I was going to suggest get_arch_id(), but
cpu_index may be a better candidate for the same reason above: it is
truly an arbitrary ID that doesn't depend on guest-visible bits.

-- 
Eduardo