* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
@ 2015-04-30 20:21 Andreas Färber
2015-04-30 21:47 ` Paolo Bonzini
2015-05-01 12:08 ` Eduardo Habkost
0 siblings, 2 replies; 17+ messages in thread
From: Andreas Färber @ 2015-04-30 20:21 UTC (permalink / raw)
To: Eduardo Habkost
Cc: peter.maydell, mimu, bharata, agraf, qemu-devel, borntraeger,
Paolo Bonzini, cornelia.huck, Igor Mammedov, david
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-04-30 20:21 [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index> Andreas Färber
@ 2015-04-30 21:47 ` Paolo Bonzini
2015-05-01 1:54 ` Peter Crosthwaite
2015-05-01 11:59 ` Eduardo Habkost
2015-05-01 12:08 ` Eduardo Habkost
1 sibling, 2 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-04-30 21:47 UTC (permalink / raw)
To: Andreas Färber, Eduardo Habkost
Cc: peter.maydell, mimu, qemu-devel, agraf, borntraeger, bharata,
cornelia.huck, Igor Mammedov, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-04-30 21:47 ` Paolo Bonzini
@ 2015-05-01 1:54 ` Peter Crosthwaite
2015-05-01 11:59 ` Eduardo Habkost
1 sibling, 0 replies; 17+ messages in thread
From: Peter Crosthwaite @ 2015-05-01 1:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, mimu, qemu-devel@nongnu.org Developers,
Alexander Graf, Christian Borntraeger (s390x),
bharata, cornelia.huck, Igor Mammedov, David Gibson,
Andreas Färber, Eduardo Habkost
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-04-30 21:47 ` Paolo Bonzini
2015-05-01 1:54 ` Peter Crosthwaite
@ 2015-05-01 11:59 ` Eduardo Habkost
2015-05-05 10:23 ` Paolo Bonzini
1 sibling, 1 reply; 17+ messages in thread
From: Eduardo Habkost @ 2015-05-01 11:59 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, mimu, qemu-devel, agraf, borntraeger, bharata,
cornelia.huck, Igor Mammedov, Andreas Färber, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-04-30 20:21 [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index> Andreas Färber
2015-04-30 21:47 ` Paolo Bonzini
@ 2015-05-01 12:08 ` Eduardo Habkost
1 sibling, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2015-05-01 12:08 UTC (permalink / raw)
To: Andreas Färber
Cc: peter.maydell, mimu, bharata, agraf, qemu-devel, borntraeger,
Paolo Bonzini, cornelia.huck, Igor Mammedov, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-01 11:59 ` Eduardo Habkost
@ 2015-05-05 10:23 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-05 10:23 UTC (permalink / raw)
To: Eduardo Habkost
Cc: peter.maydell, mimu, qemu-devel, agraf, borntraeger, bharata,
cornelia.huck, Igor Mammedov, Andreas Färber, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-04 15:53 ` Paolo Bonzini
@ 2015-05-04 18:37 ` Eduardo Habkost
0 siblings, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2015-05-04 18:37 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, Igor Mammedov, Andreas Färber, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-04 14:05 ` Eduardo Habkost
@ 2015-05-04 15:53 ` Paolo Bonzini
2015-05-04 18:37 ` Eduardo Habkost
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-04 15:53 UTC (permalink / raw)
To: Eduardo Habkost
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, Igor Mammedov, Andreas Färber, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-04 13:19 ` Paolo Bonzini
@ 2015-05-04 14:05 ` Eduardo Habkost
2015-05-04 15:53 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Eduardo Habkost @ 2015-05-04 14:05 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, Igor Mammedov, Andreas Färber, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-04 13:16 ` Igor Mammedov
2015-05-04 13:19 ` Paolo Bonzini
@ 2015-05-04 13:37 ` Eduardo Habkost
1 sibling, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2015-05-04 13:37 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, Paolo Bonzini, Andreas Färber, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-04 13:16 ` Igor Mammedov
@ 2015-05-04 13:19 ` Paolo Bonzini
2015-05-04 14:05 ` Eduardo Habkost
2015-05-04 13:37 ` Eduardo Habkost
1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-04 13:19 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, david, Andreas Färber, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-04 9:59 ` Paolo Bonzini
@ 2015-05-04 13:16 ` Igor Mammedov
2015-05-04 13:19 ` Paolo Bonzini
2015-05-04 13:37 ` Eduardo Habkost
0 siblings, 2 replies; 17+ messages in thread
From: Igor Mammedov @ 2015-05-04 13:16 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, david, Andreas Färber, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-04 9:47 ` Igor Mammedov
@ 2015-05-04 9:59 ` Paolo Bonzini
2015-05-04 13:16 ` Igor Mammedov
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-04 9:59 UTC (permalink / raw)
To: Igor Mammedov, Eduardo Habkost
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, Andreas Färber, david
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-04-30 19:19 Eduardo Habkost
2015-05-01 1:51 ` Peter Crosthwaite
@ 2015-05-04 9:47 ` Igor Mammedov
2015-05-04 9:59 ` Paolo Bonzini
1 sibling, 1 reply; 17+ messages in thread
From: Igor Mammedov @ 2015-05-04 9:47 UTC (permalink / raw)
To: Eduardo Habkost
Cc: peter.maydell, mimu, agraf, qemu-devel, borntraeger, bharata,
cornelia.huck, Paolo Bonzini, Andreas Färber, david
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;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-05-01 1:51 ` Peter Crosthwaite
@ 2015-05-01 12:17 ` Eduardo Habkost
0 siblings, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2015-05-01 12:17 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, mimu, Igor Mammedov, Alexander Graf,
qemu-devel@nongnu.org Developers, Christian Borntraeger (s390x),
bharata, cornelia.huck, Paolo Bonzini, Andreas Färber,
David Gibson
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
2015-04-30 19:19 Eduardo Habkost
@ 2015-05-01 1:51 ` Peter Crosthwaite
2015-05-01 12:17 ` Eduardo Habkost
2015-05-04 9:47 ` Igor Mammedov
1 sibling, 1 reply; 17+ messages in thread
From: Peter Crosthwaite @ 2015-05-01 1:51 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Peter Maydell, mimu, Igor Mammedov, Alexander Graf,
qemu-devel@nongnu.org Developers, Christian Borntraeger (s390x),
bharata, cornelia.huck, Paolo Bonzini, Andreas Färber,
David Gibson
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
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index>
@ 2015-04-30 19:19 Eduardo Habkost
2015-05-01 1:51 ` Peter Crosthwaite
2015-05-04 9:47 ` Igor Mammedov
0 siblings, 2 replies; 17+ messages in thread
From: Eduardo Habkost @ 2015-04-30 19:19 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, mimu, bharata, agraf, borntraeger, Paolo Bonzini,
cornelia.huck, Igor Mammedov, Andreas Färber, david
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-05-05 10:23 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-30 20:21 [Qemu-devel] [PATCH] cpu: Register QOM links at /machine/cpus/<index> Andreas Färber
2015-04-30 21:47 ` Paolo Bonzini
2015-05-01 1:54 ` Peter Crosthwaite
2015-05-01 11:59 ` Eduardo Habkost
2015-05-05 10:23 ` Paolo Bonzini
2015-05-01 12:08 ` Eduardo Habkost
-- strict thread matches above, loose matches on Subject: below --
2015-04-30 19:19 Eduardo Habkost
2015-05-01 1:51 ` Peter Crosthwaite
2015-05-01 12:17 ` Eduardo Habkost
2015-05-04 9:47 ` Igor Mammedov
2015-05-04 9:59 ` Paolo Bonzini
2015-05-04 13:16 ` Igor Mammedov
2015-05-04 13:19 ` Paolo Bonzini
2015-05-04 14:05 ` Eduardo Habkost
2015-05-04 15:53 ` Paolo Bonzini
2015-05-04 18:37 ` Eduardo Habkost
2015-05-04 13:37 ` Eduardo Habkost
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.