Re: [Qemu-devel] [PATCH qom-next 04/59] pc: Add CPU as /machine/cpu[n]

[Andreas Färber]

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 <address@hidden>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg