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

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

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>