| Message ID | 1337742502-28565-5-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Wed, May 23, 2012 at 05:07:27AM +0200, Andreas Färber wrote: > Using the cpu_index, give the X86CPU a canonical path. > This must be done before initializing the APIC. > > Signed-off-by: Igor Mammedov <niallain@gmail.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > hw/pc.c | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 4167782..e9d7e05 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -945,6 +945,8 @@ static X86CPU *pc_new_cpu(const char *cpu_model) > { > X86CPU *cpu; > CPUX86State *env; > + char *name; > + Error *error = NULL; > > cpu = cpu_x86_init(cpu_model); > if (cpu == NULL) { > @@ -952,6 +954,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model) > exit(1); > } > env = &cpu->env; > + > + name = g_strdup_printf("cpu[%d]", env->cpu_index); > + object_property_add_child(OBJECT(qdev_get_machine()), name, > + OBJECT(cpu), &error); This call might be too late. Imagine if before this call a property/child of this CPU would set link on on it. Then it would assert in object_property_set_link -> object_get_canonical_path since CPU would not have parent a that time. Wouldn't it better to make it child in CPU's initfn? This way CPU object could be used as a value for link anywhere once it's been created. > + g_free(name); > + if (error_is_set(&error)) { > + qerror_report_err(error); > + exit(1); > + } > + > if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > env->apic_state = apic_init(env, env->cpuid_apic_id); > } > -- > 1.7.7 > >
Am 08.06.2012 10:20, schrieb Igor Mammedov: > On Wed, May 23, 2012 at 05:07:27AM +0200, Andreas Färber wrote: >> Using the cpu_index, give the X86CPU a canonical path. >> This must be done before initializing the APIC. >> >> Signed-off-by: Igor Mammedov <niallain@gmail.com> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> hw/pc.c | 12 ++++++++++++ >> 1 files changed, 12 insertions(+), 0 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 4167782..e9d7e05 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -945,6 +945,8 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >> { >> X86CPU *cpu; >> CPUX86State *env; >> + char *name; >> + Error *error = NULL; >> >> cpu = cpu_x86_init(cpu_model); >> if (cpu == NULL) { >> @@ -952,6 +954,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model) >> exit(1); >> } >> env = &cpu->env; >> + >> + name = g_strdup_printf("cpu[%d]", env->cpu_index); >> + object_property_add_child(OBJECT(qdev_get_machine()), name, >> + OBJECT(cpu), &error); > This call might be too late. This series here is mostly not going to go through qom-next btw, it is just based on qom-next, so it's not too late to discuss such issues. :) > Imagine if before this call a property/child of this CPU > would set link on on it. Then it would assert in object_property_set_link -> > object_get_canonical_path since CPU would not have parent a that time. > Wouldn't it better to make it child in CPU's initfn? This way CPU object > could be used as a value for link anywhere once it's been created. I've seen that issue in your series. This is touching on a core QOM question: Can we link<> during initfn? That's what you're trying to do for the APIC and why this may be too late in your case. I believe the answer to that question must be no. From what I understand about the x86 modeling, the only case this matters is if you hot-unplug CPU 0, right? Question is, what happens with the APIC then? I would guess the remaining n-1 CPUs still want to access the APIC - then it would need to stay and if CPU 0 is hot-replugged it would not need to be recreated but reconnected. The alternative would be that CPU 0 cannot be hot-unplugged at all, in which case the APIC would be irrelevant to hot-plugging and the remodelling would be moot; or all remaining CPUs would suddenly loose the APIC feature on hot-unplug of CPU 0. Again, I don't know how this works in hardware. Another factor that is making this slightly difficult is that there are three APIC subclasses. Currently they all have an instance_size of sizeof(APICCommonState) so it could be created in-place if it actually is a part (child<>) of the CPU wrt hot-plug. Creating objects with object_new() in QOM instance_init is forbidden. Also I have a broader view than the PC in mind: Depending on whether you have a mainboard with CPU sockets or some SoC or module, you may desire different modelings. My above modeling is for a PC, in hw/pc.c, using /machine/cpu[n]. For a QSeven module the parent would be the Container or type for the module, e.g. /machine/qseven/cpu[n]. Another example might be AMD Fusion. Therefore I think that tying the modeling to the CPU initfn is conceptually wrong. Maybe this CPU hot-plug business would be a good topic for a KVM call? Regards, Andreas
On 2012-06-08 11:11, Andreas Färber wrote: >>From what I understand about the x86 modeling, the only case this > matters is if you hot-unplug CPU 0, right? Question is, what happens > with the APIC then? I would guess the remaining n-1 CPUs still want to > access the APIC APICs are per-CPU, each has its own. Jan
Am 08.06.2012 12:21, schrieb Jan Kiszka: > On 2012-06-08 11:11, Andreas Färber wrote: >> >From what I understand about the x86 modeling, the only case this >> matters is if you hot-unplug CPU 0, right? Question is, what happens >> with the APIC then? I would guess the remaining n-1 CPUs still want to >> access the APIC > > APICs are per-CPU, each has its own. Uh, seems I'm seriously confusing APIC with something else then... Anyway, if each CPU always has its own APIC there's no reason to link<> them. It should be a child<> and it should be initialized in-place. Igor, can you please look into that? In that case the only open issue would be whether to use cpu_index or the APIC ID as the property name in this patch. Andreas
Am 08.06.2012 12:36, schrieb Andreas Färber: > Am 08.06.2012 12:21, schrieb Jan Kiszka: >> On 2012-06-08 11:11, Andreas Färber wrote: >>> >From what I understand about the x86 modeling, the only case this >>> matters is if you hot-unplug CPU 0, right? Question is, what happens >>> with the APIC then? I would guess the remaining n-1 CPUs still want to >>> access the APIC >> >> APICs are per-CPU, each has its own. > > Uh, seems I'm seriously confusing APIC with something else then... Guess I misread this as apic_init() in Igor's patch: + if (env->cpu_index == 0) { + apic_designate_bsp(env->apic_state); + } Shame on me. :/ /-F
On Fri, Jun 08, 2012 at 12:36:18PM +0200, Andreas Färber wrote: > Am 08.06.2012 12:21, schrieb Jan Kiszka: > > On 2012-06-08 11:11, Andreas Färber wrote: > >> >From what I understand about the x86 modeling, the only case this > >> matters is if you hot-unplug CPU 0, right? Question is, what happens > >> with the APIC then? I would guess the remaining n-1 CPUs still want to > >> access the APIC > > > > APICs are per-CPU, each has its own. > > Uh, seems I'm seriously confusing APIC with something else then... > > Anyway, if each CPU always has its own APIC there's no reason to link<> > them. It should be a child<> and it should be initialized in-place. in [5/59] you create a back_link<> from APIC to parent cpu to replace cpu_env pointer (i.e. something that could be ptr property). And from what I've read link<> is kind of strong typed replacement for ptr properties, correct me if I wrong. So having link<> there should be ok, except of that CPU should have parent before it will set back_link<> "cpu" in APIC. > > Igor, can you please look into that? Sure, Could you point to an example of creating a QOMified object in place, please? > > In that case the only open issue would be whether to use cpu_index or > the APIC ID as the property name in this patch. not only, cpu_x86_init() now represents create/set_props/realize sequence and making cpu a child after set_props/realize is wrong. But if cpu_x86_init() were replaced by its contents in pc_new_cpu and CPU were made a child right after object_new(X86CPU) then problem would be avoided. and the rest of properties (including APIC) could be set after that and at the end realizefn() is called. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: > Am 08.06.2012 10:20, schrieb Igor Mammedov: > > On Wed, May 23, 2012 at 05:07:27AM +0200, Andreas Färber wrote: > > This is touching on a core QOM question: Can we link<> during initfn? > That's what you're trying to do for the APIC and why this may be too > late in your case. I believe the answer to that question must be no. Yep, it's more of general question. Potentially any property could be set in initfn to intialize defaults and a property setter could create a link causing chicken/egg conflict. If making link<> to object is not permited till its initfn is done then when it is permited to be made? Maybe object_new() should take parent as parameter or maybe due to limitation we should revise purpose of link<>s /if they are replacement of ptr properties/? [...] > Another factor that is making this slightly difficult is that there are > three APIC subclasses. Currently they all have an instance_size of > sizeof(APICCommonState) so it could be created in-place if it actually > is a part (child<>) of the CPU wrt hot-plug. Creating objects with > object_new() in QOM instance_init is forbidden. Any particular reason why object_new() in intifn is not acceptable? > [...] > > Maybe this CPU hot-plug business would be a good topic for a KVM call? It's more that I'm unhappy about wrong cpu creation order in pc_new_cpu() at the present code. For hotplug qdev_device_add makes new object as a child<> when it's created and it just needs to be placed before other properties are set. > > Regards, > 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 08.06.2012 13:36, schrieb Igor Mammedov: > On Fri, Jun 08, 2012 at 12:36:18PM +0200, Andreas Färber wrote: >> Am 08.06.2012 12:21, schrieb Jan Kiszka: >>> On 2012-06-08 11:11, Andreas Färber wrote: >>>> >From what I understand about the x86 modeling, the only case this >>>> matters is if you hot-unplug CPU 0, right? Question is, what happens >>>> with the APIC then? I would guess the remaining n-1 CPUs still want to >>>> access the APIC >>> >>> APICs are per-CPU, each has its own. >> >> [...] if each CPU always has its own APIC there's no reason to link<> >> them. It should be a child<> and it should be initialized in-place. >> >> Igor, can you please look into that? > Sure, Could you point to an example of creating a QOMified object in place, please? http://patchwork.ozlabs.org/patch/161497/ and Anthony's i440fx series. If I'm reading the code correctly then we'd need to add the APIC as a child of the CPU before its qdev initfn is called, i.e. in place of the current qdev pointer property. Andreas
Am 08.06.2012 14:05, schrieb Igor Mammedov: > On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: >> Another factor that is making this slightly difficult is that there are >> three APIC subclasses. Currently they all have an instance_size of >> sizeof(APICCommonState) so it could be created in-place if it actually >> is a part (child<>) of the CPU wrt hot-plug. Creating objects with >> object_new() in QOM instance_init is forbidden. > Any particular reason why object_new() in intifn is not acceptable? It allocates memory, which may fail. The initfn must not fail, the realizefn may return an Error object. Andreas
On 2012-06-08 14:34, Andreas Färber wrote: > Am 08.06.2012 14:05, schrieb Igor Mammedov: >> On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: >>> Another factor that is making this slightly difficult is that there are >>> three APIC subclasses. Currently they all have an instance_size of >>> sizeof(APICCommonState) so it could be created in-place if it actually >>> is a part (child<>) of the CPU wrt hot-plug. Creating objects with >>> object_new() in QOM instance_init is forbidden. >> Any particular reason why object_new() in intifn is not acceptable? > > It allocates memory, which may fail. The initfn must not fail, the > realizefn may return an Error object. Since when do we fail gracefully on OOM again? Jan
----- Original Message ----- > From: "Jan Kiszka" <jan.kiszka@siemens.com> > To: "Andreas Färber" <afaerber@suse.de> > Cc: "Igor Mammedov" <imammedo@redhat.com>, "Anthony Liguori" <aliguori@us.ibm.com>, qemu-devel@nongnu.org, "Igor > Mammedov" <niallain@gmail.com>, "Paolo Bonzini" <pbonzini@redhat.com> > Sent: Friday, June 8, 2012 2:36:53 PM > Subject: Re: [Qemu-devel] [PATCH qom-next 04/59] pc: Add CPU as /machine/cpu[n] > > On 2012-06-08 14:34, Andreas Färber wrote: > > Am 08.06.2012 14:05, schrieb Igor Mammedov: > >> On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: > >>> Another factor that is making this slightly difficult is that > >>> there are > >>> three APIC subclasses. Currently they all have an instance_size > >>> of > >>> sizeof(APICCommonState) so it could be created in-place if it > >>> actually > >>> is a part (child<>) of the CPU wrt hot-plug. Creating objects > >>> with > >>> object_new() in QOM instance_init is forbidden. > >> Any particular reason why object_new() in intifn is not > >> acceptable? > > > > It allocates memory, which may fail. The initfn must not fail, the > > realizefn may return an Error object. > > Since when do we fail gracefully on OOM again? Maybe Andreas means that we cannot report error to caller? If it's a case then lets pass error to object_new() and fail gracefully or simply abort on OOM. BTW: in-place creation looks ugly compared to object_new(), not mentioning complications if being initialized object could be of different sizes. > > Jan > > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux > >
On 2012-06-08 14:47, Igor Mammedov wrote: > ----- Original Message ----- >> From: "Jan Kiszka" <jan.kiszka@siemens.com> >> To: "Andreas Färber" <afaerber@suse.de> >> Cc: "Igor Mammedov" <imammedo@redhat.com>, "Anthony Liguori" <aliguori@us.ibm.com>, qemu-devel@nongnu.org, "Igor >> Mammedov" <niallain@gmail.com>, "Paolo Bonzini" <pbonzini@redhat.com> >> Sent: Friday, June 8, 2012 2:36:53 PM >> Subject: Re: [Qemu-devel] [PATCH qom-next 04/59] pc: Add CPU as /machine/cpu[n] >> >> On 2012-06-08 14:34, Andreas Färber wrote: >>> Am 08.06.2012 14:05, schrieb Igor Mammedov: >>>> On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: >>>>> Another factor that is making this slightly difficult is that >>>>> there are >>>>> three APIC subclasses. Currently they all have an instance_size >>>>> of >>>>> sizeof(APICCommonState) so it could be created in-place if it >>>>> actually >>>>> is a part (child<>) of the CPU wrt hot-plug. Creating objects >>>>> with >>>>> object_new() in QOM instance_init is forbidden. >>>> Any particular reason why object_new() in intifn is not >>>> acceptable? >>> >>> It allocates memory, which may fail. The initfn must not fail, the >>> realizefn may return an Error object. >> >> Since when do we fail gracefully on OOM again? > Maybe Andreas means that we cannot report error to caller? > If it's a case then lets pass error to object_new() and fail gracefully > or simply abort on OOM. QEMU's policy on OOM is abort (that's what glib already does for us theses days). Jan
On Fri, Jun 8, 2012 at 2:52 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2012-06-08 14:47, Igor Mammedov wrote: >> ----- Original Message ----- >>> From: "Jan Kiszka" <jan.kiszka@siemens.com> >>> To: "Andreas Färber" <afaerber@suse.de> >>> Cc: "Igor Mammedov" <imammedo@redhat.com>, "Anthony Liguori" <aliguori@us.ibm.com>, qemu-devel@nongnu.org, "Igor >>> Mammedov" <niallain@gmail.com>, "Paolo Bonzini" <pbonzini@redhat.com> >>> Sent: Friday, June 8, 2012 2:36:53 PM >>> Subject: Re: [Qemu-devel] [PATCH qom-next 04/59] pc: Add CPU as /machine/cpu[n] >>> >>> On 2012-06-08 14:34, Andreas Färber wrote: >>>> Am 08.06.2012 14:05, schrieb Igor Mammedov: >>>>> On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: >>>>>> Another factor that is making this slightly difficult is that >>>>>> there are >>>>>> three APIC subclasses. Currently they all have an instance_size >>>>>> of >>>>>> sizeof(APICCommonState) so it could be created in-place if it >>>>>> actually >>>>>> is a part (child<>) of the CPU wrt hot-plug. Creating objects >>>>>> with >>>>>> object_new() in QOM instance_init is forbidden. >>>>> Any particular reason why object_new() in intifn is not >>>>> acceptable? >>>> >>>> It allocates memory, which may fail. The initfn must not fail, the >>>> realizefn may return an Error object. >>> >>> Since when do we fail gracefully on OOM again? >> Maybe Andreas means that we cannot report error to caller? >> If it's a case then lets pass error to object_new() and fail gracefully >> or simply abort on OOM. > > QEMU's policy on OOM is abort (that's what glib already does for us > theses days). > then there is little merit in playing in-place game since allocation of containing object may fail as well resulting in abort just a bit earlier. > Jan > > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux
Am 08.06.2012 14:52, schrieb Jan Kiszka: > On 2012-06-08 14:47, Igor Mammedov wrote: >> ----- Original Message ----- >>> From: "Jan Kiszka" <jan.kiszka@siemens.com> >>> To: "Andreas Färber" <afaerber@suse.de> >>> Cc: "Igor Mammedov" <imammedo@redhat.com>, "Anthony Liguori" <aliguori@us.ibm.com>, qemu-devel@nongnu.org, "Igor >>> Mammedov" <niallain@gmail.com>, "Paolo Bonzini" <pbonzini@redhat.com> >>> Sent: Friday, June 8, 2012 2:36:53 PM >>> Subject: Re: [Qemu-devel] [PATCH qom-next 04/59] pc: Add CPU as /machine/cpu[n] >>> >>> On 2012-06-08 14:34, Andreas Färber wrote: >>>> Am 08.06.2012 14:05, schrieb Igor Mammedov: >>>>> On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: >>>>>> Another factor that is making this slightly difficult is that >>>>>> there are >>>>>> three APIC subclasses. Currently they all have an instance_size >>>>>> of >>>>>> sizeof(APICCommonState) so it could be created in-place if it >>>>>> actually >>>>>> is a part (child<>) of the CPU wrt hot-plug. Creating objects >>>>>> with >>>>>> object_new() in QOM instance_init is forbidden. >>>>> Any particular reason why object_new() in intifn is not >>>>> acceptable? >>>> >>>> It allocates memory, which may fail. The initfn must not fail, the >>>> realizefn may return an Error object. >>> >>> Since when do we fail gracefully on OOM again? >> Maybe Andreas means that we cannot report error to caller? >> If it's a case then lets pass error to object_new() and fail gracefully >> or simply abort on OOM. > > QEMU's policy on OOM is abort (that's what glib already does for us > theses days). Nah, that's not the whole truth. (More on that when I've finished fixing my series.) Andreas
On 06/08/2012 03:04 PM, Andreas Färber wrote: > Am 08.06.2012 14:52, schrieb Jan Kiszka: >> On 2012-06-08 14:47, Igor Mammedov wrote: >>> ----- Original Message ----- >>>> From: "Jan Kiszka" <jan.kiszka@siemens.com> >>>> To: "Andreas Färber" <afaerber@suse.de> >>>> Cc: "Igor Mammedov" <imammedo@redhat.com>, "Anthony Liguori" <aliguori@us.ibm.com>, qemu-devel@nongnu.org, "Igor >>>> Mammedov" <niallain@gmail.com>, "Paolo Bonzini" <pbonzini@redhat.com> >>>> Sent: Friday, June 8, 2012 2:36:53 PM >>>> Subject: Re: [Qemu-devel] [PATCH qom-next 04/59] pc: Add CPU as /machine/cpu[n] >>>> >>>> On 2012-06-08 14:34, Andreas Färber wrote: >>>>> Am 08.06.2012 14:05, schrieb Igor Mammedov: >>>>>> On Fri, Jun 08, 2012 at 11:11:11AM +0200, Andreas Färber wrote: >>>>>>> Another factor that is making this slightly difficult is that >>>>>>> there are >>>>>>> three APIC subclasses. Currently they all have an instance_size >>>>>>> of >>>>>>> sizeof(APICCommonState) so it could be created in-place if it >>>>>>> actually >>>>>>> is a part (child<>) of the CPU wrt hot-plug. Creating objects >>>>>>> with >>>>>>> object_new() in QOM instance_init is forbidden. >>>>>> Any particular reason why object_new() in intifn is not >>>>>> acceptable? >>>>> >>>>> It allocates memory, which may fail. The initfn must not fail, the >>>>> realizefn may return an Error object. >>>> >>>> Since when do we fail gracefully on OOM again? >>> Maybe Andreas means that we cannot report error to caller? >>> If it's a case then lets pass error to object_new() and fail gracefully >>> or simply abort on OOM. >> >> QEMU's policy on OOM is abort (that's what glib already does for us >> theses days). > > Nah, that's not the whole truth. Could you elaborate more on subj? I've looked at different initfns, many of them call object_property_add() which may cause OOM as well. So if object_property_add() is permitted then why not object_new()? > > (More on that when I've finished fixing my series.) > > Andreas >
diff --git a/hw/pc.c b/hw/pc.c index 4167782..e9d7e05 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -945,6 +945,8 @@ static X86CPU *pc_new_cpu(const char *cpu_model) { X86CPU *cpu; CPUX86State *env; + char *name; + Error *error = NULL; cpu = cpu_x86_init(cpu_model); if (cpu == NULL) { @@ -952,6 +954,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model) exit(1); } env = &cpu->env; + + name = g_strdup_printf("cpu[%d]", env->cpu_index); + object_property_add_child(OBJECT(qdev_get_machine()), name, + OBJECT(cpu), &error); + g_free(name); + if (error_is_set(&error)) { + qerror_report_err(error); + exit(1); + } + if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { env->apic_state = apic_init(env, env->cpuid_apic_id); }