| Message ID | 1430421547-18278-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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().
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;
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
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
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
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.
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.
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
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.
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;