| Message ID | 1334619423-13012-7-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 17/04/2012 01:37, Igor Mammedov ha scritto: > From: Igor Mammedov <niallain@gmail.com> > > Signed-off-by: Igor Mammedov <niallain@gmail.com> > --- > target-i386/helper.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/target-i386/helper.c b/target-i386/helper.c > index de7637c..1996b97 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1134,6 +1134,7 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > X86CPU *cpu; > CPUX86State *env; > Error *errp = NULL; > + char cpuname[8]; > > cpu = X86_CPU(object_new(TYPE_X86_CPU)); > env = &cpu->env; > @@ -1146,6 +1147,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > } > } > > + snprintf(cpuname, sizeof(cpuname), "cpu%d", env->cpuid_apic_id); > + object_property_add_child(container_get("/machine"), cpuname, OBJECT(cpu), NULL); > + > object_property_set_bool(OBJECT(cpu), true, "realized", &errp); > if (errp) { > object_delete(OBJECT(cpu)); I think the right name would be /machine/cpu[%d]/cpu. The local APIC for example should reside under /machine/cpu[%d]/apic. Paolo
Am 17.04.2012 09:19, schrieb Paolo Bonzini: > Il 17/04/2012 01:37, Igor Mammedov ha scritto: >> From: Igor Mammedov <niallain@gmail.com> >> >> Signed-off-by: Igor Mammedov <niallain@gmail.com> >> --- >> target-i386/helper.c | 4 ++++ >> 1 files changed, 4 insertions(+), 0 deletions(-) >> >> diff --git a/target-i386/helper.c b/target-i386/helper.c >> index de7637c..1996b97 100644 >> --- a/target-i386/helper.c >> +++ b/target-i386/helper.c >> @@ -1134,6 +1134,7 @@ CPUX86State *cpu_x86_init(const char *cpu_model) >> X86CPU *cpu; >> CPUX86State *env; >> Error *errp = NULL; >> + char cpuname[8]; >> >> cpu = X86_CPU(object_new(TYPE_X86_CPU)); >> env = &cpu->env; >> @@ -1146,6 +1147,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) >> } >> } >> >> + snprintf(cpuname, sizeof(cpuname), "cpu%d", env->cpuid_apic_id); >> + object_property_add_child(container_get("/machine"), cpuname, OBJECT(cpu), NULL); >> + >> object_property_set_bool(OBJECT(cpu), true, "realized", &errp); >> if (errp) { >> object_delete(OBJECT(cpu)); > > I think the right name would be /machine/cpu[%d]/cpu. The local APIC > for example should reside under /machine/cpu[%d]/apic. Depends on how we model the CPU, I was kinda waiting for feedback on that. I would prefer /machine/cpu[%d], with the APIC being a child .../apic, if possible. That however depends on how the device'ification of the CPU for hotplug works out - issue being that the CPU is a cross-target base class where we'd need to change its parent to something that's a device for device_add, on a hot-pluggable bus (for now) and works for all targets. Andreas
----- Original Message ----- > From: "Paolo Bonzini" <pbonzini@redhat.com> > To: "Igor Mammedov" <imammedo@redhat.com> > Cc: qemu-devel@nongnu.org, afaerber@suse.de, aliguori@us.ibm.com, "jan kiszka" <jan.kiszka@siemens.com> > Sent: Tuesday, April 17, 2012 9:19:44 AM > Subject: Re: [PATCH RFC 6/6] target-i386: make cpus childs of /machine > > Il 17/04/2012 01:37, Igor Mammedov ha scritto: > > From: Igor Mammedov <niallain@gmail.com> > > > > Signed-off-by: Igor Mammedov <niallain@gmail.com> > > --- > > target-i386/helper.c | 4 ++++ > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/target-i386/helper.c b/target-i386/helper.c > > index de7637c..1996b97 100644 > > --- a/target-i386/helper.c > > +++ b/target-i386/helper.c > > @@ -1134,6 +1134,7 @@ CPUX86State *cpu_x86_init(const char > > *cpu_model) > > X86CPU *cpu; > > CPUX86State *env; > > Error *errp = NULL; > > + char cpuname[8]; > > > > cpu = X86_CPU(object_new(TYPE_X86_CPU)); > > env = &cpu->env; > > @@ -1146,6 +1147,9 @@ CPUX86State *cpu_x86_init(const char > > *cpu_model) > > } > > } > > > > + snprintf(cpuname, sizeof(cpuname), "cpu%d", > > env->cpuid_apic_id); > > + object_property_add_child(container_get("/machine"), cpuname, > > OBJECT(cpu), NULL); > > + > > object_property_set_bool(OBJECT(cpu), true, "realized", > > &errp); > > if (errp) { > > object_delete(OBJECT(cpu)); > > I think the right name would be /machine/cpu[%d]/cpu. The local APIC > for example should reside under /machine/cpu[%d]/apic. APIC is a child of cpu, see [4/6] > > Paolo >
----- Original Message ----- > From: "Andreas Färber" <afaerber@suse.de> > To: "Paolo Bonzini" <pbonzini@redhat.com>, "Igor Mammedov" <imammedo@redhat.com> > Cc: qemu-devel@nongnu.org, aliguori@us.ibm.com, "jan kiszka" <jan.kiszka@siemens.com> > Sent: Tuesday, April 17, 2012 10:46:14 AM > Subject: Re: [PATCH RFC 6/6] target-i386: make cpus childs of /machine > > Am 17.04.2012 09:19, schrieb Paolo Bonzini: > > Il 17/04/2012 01:37, Igor Mammedov ha scritto: > >> From: Igor Mammedov <niallain@gmail.com> > >> > >> Signed-off-by: Igor Mammedov <niallain@gmail.com> > >> --- > >> target-i386/helper.c | 4 ++++ > >> 1 files changed, 4 insertions(+), 0 deletions(-) > >> > >> diff --git a/target-i386/helper.c b/target-i386/helper.c > >> index de7637c..1996b97 100644 > >> --- a/target-i386/helper.c > >> +++ b/target-i386/helper.c > >> @@ -1134,6 +1134,7 @@ CPUX86State *cpu_x86_init(const char > >> *cpu_model) > >> X86CPU *cpu; > >> CPUX86State *env; > >> Error *errp = NULL; > >> + char cpuname[8]; > >> > >> cpu = X86_CPU(object_new(TYPE_X86_CPU)); > >> env = &cpu->env; > >> @@ -1146,6 +1147,9 @@ CPUX86State *cpu_x86_init(const char > >> *cpu_model) > >> } > >> } > >> > >> + snprintf(cpuname, sizeof(cpuname), "cpu%d", > >> env->cpuid_apic_id); > >> + object_property_add_child(container_get("/machine"), cpuname, > >> OBJECT(cpu), NULL); > >> + > >> object_property_set_bool(OBJECT(cpu), true, "realized", > >> &errp); > >> if (errp) { > >> object_delete(OBJECT(cpu)); > > > > I think the right name would be /machine/cpu[%d]/cpu. The local > > APIC > > for example should reside under /machine/cpu[%d]/apic. > > Depends on how we model the CPU, I was kinda waiting for feedback on > that. > > I would prefer /machine/cpu[%d], with the APIC being a child > .../apic, > if possible. That however depends on how the device'ification of the > CPU Ok, I'll change /machine/cpu%d to /machine/cpu[%d]. > for hotplug works out - issue being that the CPU is a cross-target > base > class where we'd need to change its parent to something that's a > device > for device_add, on a hot-pluggable bus (for now) and works for all > targets. We could just enable hotplugging in sysbus so that it won't prevent hotplugging apic for now. And for cpu hotplug use links as Anthony suggested. However we need storage for keeping link references, cpu list (I mean: first_cpu...) could be used for it now. But I'd like to make shallow conversion of cpu list to a pre-allocated cpu array for maxcpus. that will provide storage for pre-allocated links and fix/simplify cpu_index allocation for repeated cpu add/remove ops. After that probably would be a good time to hack device_add/del for cpu hot-plug. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG > Nürnberg >
Am 17.04.2012 12:28, schrieb Igor Mammedov: > ----- Original Message ----- >> From: "Andreas Färber" <afaerber@suse.de> >> To: "Paolo Bonzini" <pbonzini@redhat.com>, "Igor Mammedov" <imammedo@redhat.com> >> Cc: qemu-devel@nongnu.org, aliguori@us.ibm.com, "jan kiszka" <jan.kiszka@siemens.com> >> Sent: Tuesday, April 17, 2012 10:46:14 AM >> Subject: Re: [PATCH RFC 6/6] target-i386: make cpus childs of /machine >> >> Am 17.04.2012 09:19, schrieb Paolo Bonzini: >>> Il 17/04/2012 01:37, Igor Mammedov ha scritto: >>>> From: Igor Mammedov <niallain@gmail.com> >>>> >>>> Signed-off-by: Igor Mammedov <niallain@gmail.com> >>>> --- >>>> target-i386/helper.c | 4 ++++ >>>> 1 files changed, 4 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/target-i386/helper.c b/target-i386/helper.c >>>> index de7637c..1996b97 100644 >>>> --- a/target-i386/helper.c >>>> +++ b/target-i386/helper.c >>>> @@ -1134,6 +1134,7 @@ CPUX86State *cpu_x86_init(const char >>>> *cpu_model) >>>> X86CPU *cpu; >>>> CPUX86State *env; >>>> Error *errp = NULL; >>>> + char cpuname[8]; >>>> >>>> cpu = X86_CPU(object_new(TYPE_X86_CPU)); >>>> env = &cpu->env; >>>> @@ -1146,6 +1147,9 @@ CPUX86State *cpu_x86_init(const char >>>> *cpu_model) >>>> } >>>> } >>>> >>>> + snprintf(cpuname, sizeof(cpuname), "cpu%d", >>>> env->cpuid_apic_id); >>>> + object_property_add_child(container_get("/machine"), cpuname, >>>> OBJECT(cpu), NULL); >>>> + >>>> object_property_set_bool(OBJECT(cpu), true, "realized", >>>> &errp); >>>> if (errp) { >>>> object_delete(OBJECT(cpu)); >>> >>> I think the right name would be /machine/cpu[%d]/cpu. The local >>> APIC >>> for example should reside under /machine/cpu[%d]/apic. >> >> Depends on how we model the CPU, I was kinda waiting for feedback on >> that. >> >> I would prefer /machine/cpu[%d], with the APIC being a child >> .../apic, >> if possible. That however depends on how the device'ification of the >> CPU > Ok, I'll change /machine/cpu%d to /machine/cpu[%d]. Note that I needed a similar patch recently in my quest to move fields from CPU_COMMON into CPUState: https://github.com/afaerber/qemu-cpu/commits/qom-cpu (WIP) current state: https://github.com/afaerber/qemu-cpu/commit/1318ad13cda52e694caea5cc72293c5879db3f9f I chose to do it in pc.c instead because for other architectures the placement will be a machine-specific decision. I've used cpu_index, but it seems cpuid_apic_id is assigned only once, from cpu_index, so it should be identical. What's the difference? Comparing our code I see that I do an unnecessary NUL termination after snprintf(), whereas your buffer should probably be 9 chars to account for a maximum of 255 CPUs (3 letters, 2 square brackets, 3 digits, NUL). In particular I needed /machine/cpu[n] to pass the X86CPU to the APIC in a sensible way - next commit on that branch, currently: https://github.com/afaerber/qemu-cpu/commit/1134efc143aa1629f8961ef058416e1acfa50f8e Regards, Andreas
Il 10/05/2012 01:15, Andreas Färber ha scritto: > In particular I needed /machine/cpu[n] to pass the X86CPU to the APIC in > a sensible way - next commit on that branch, currently: > https://github.com/afaerber/qemu-cpu/commit/1134efc143aa1629f8961ef058416e1acfa50f8e Yay, thanks for doing that! Paolo
Am 10.05.2012 08:57, schrieb Paolo Bonzini: > Il 10/05/2012 01:15, Andreas Färber ha scritto: >> In particular I needed /machine/cpu[n] to pass the X86CPU to the APIC in >> a sensible way - next commit on that branch, currently: >> https://github.com/afaerber/qemu-cpu/commit/1134efc143aa1629f8961ef058416e1acfa50f8e > > Yay, thanks for doing that! Thought you'd like it. ;) It's the only one related to CPU(Arch)State though, so some more work left for you to do after my series! Andreas
Il 10/05/2012 11:51, Andreas Färber ha scritto: >>> >> In particular I needed /machine/cpu[n] to pass the X86CPU to the APIC in >>> >> a sensible way - next commit on that branch, currently: >>> >> https://github.com/afaerber/qemu-cpu/commit/1134efc143aa1629f8961ef058416e1acfa50f8e >> > >> > Yay, thanks for doing that! > Thought you'd like it. ;) It's the only one related to CPU(Arch)State > though, so some more work left for you to do after my series! There's also one in CRIS I think. :) Paolo
On 05/10/2012 01:15 AM, Andreas Färber wrote: > Am 17.04.2012 12:28, schrieb Igor Mammedov: >> ----- Original Message ----- >>> From: "Andreas Färber"<afaerber@suse.de> ... >>>> I think the right name would be /machine/cpu[%d]/cpu. The local >>>> APIC >>>> for example should reside under /machine/cpu[%d]/apic. >>> >>> Depends on how we model the CPU, I was kinda waiting for feedback on >>> that. >>> >>> I would prefer /machine/cpu[%d], with the APIC being a child >>> .../apic, >>> if possible. That however depends on how the device'ification of the >>> CPU >> Ok, I'll change /machine/cpu%d to /machine/cpu[%d]. > > Note that I needed a similar patch recently in my quest to move fields > from CPU_COMMON into CPUState: > https://github.com/afaerber/qemu-cpu/commits/qom-cpu (WIP) > > current state: > https://github.com/afaerber/qemu-cpu/commit/1318ad13cda52e694caea5cc72293c5879db3f9f > > I chose to do it in pc.c instead because for other architectures the > placement will be a machine-specific decision. I'm trying to re-base your commit on top of this series and doing it at board level might be a problem if you think about doing hotplug in generalized way: 1. create new obj instance for cpu 2. set properties 'might include setting apic with its creation' 3. realize obj instance would it better to split name and cpu creation into 2 stages: - at board level: create links for all possible cpus - in target-XXX/cpu.c:XXX_cpu_initfn set appropriate link to itself (1) and than any property that might need canonical path could be set in general way (2)? Of cause this scheme is screwed up by the fact that there isn't canonical path for links (cpu[xxx]). Any thoughts how to deal with it? - Can we use object_property_add_child at runtime to add new cpu to /machine, instead of link? > I've used cpu_index, but it seems cpuid_apic_id is assigned only once, > from cpu_index, so it should be identical. What's the difference? Once Jan voiced that user visible cpu id, should be apic_id in context of cpu hotplug (i.e. when doing: device_add xxx_cpu,id=12345,...) "Jan, please correct me if I've got you wrong." So QOM tree probably should reflect this id and not cpu_index. However cpu_index and apic_id are the same now and bios assumes it as well. What are possible benefits of using cpuid_apic_id != cpu_index for qemu?
On 2012-05-21 11:50, Igor Mammedov wrote: >> I've used cpu_index, but it seems cpuid_apic_id is assigned only once, >> from cpu_index, so it should be identical. What's the difference? > Once Jan voiced that user visible cpu id, should be apic_id in context of cpu hotplug > (i.e. when doing: device_add xxx_cpu,id=12345,...) > "Jan, please correct me if I've got you wrong." > So QOM tree probably should reflect this id and not cpu_index. > > However cpu_index and apic_id are the same now and bios assumes it as well. > What are possible benefits of using cpuid_apic_id != cpu_index for qemu? From my POV, cpu_index could become equal to the physical APIC ID. As long as we can set it freely (provided it remains unique) and non-continuously, we don't need separate indexes. Jan
diff --git a/target-i386/helper.c b/target-i386/helper.c index de7637c..1996b97 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1134,6 +1134,7 @@ CPUX86State *cpu_x86_init(const char *cpu_model) X86CPU *cpu; CPUX86State *env; Error *errp = NULL; + char cpuname[8]; cpu = X86_CPU(object_new(TYPE_X86_CPU)); env = &cpu->env; @@ -1146,6 +1147,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) } } + snprintf(cpuname, sizeof(cpuname), "cpu%d", env->cpuid_apic_id); + object_property_add_child(container_get("/machine"), cpuname, OBJECT(cpu), NULL); + object_property_set_bool(OBJECT(cpu), true, "realized", &errp); if (errp) { object_delete(OBJECT(cpu));