Re: [Qemu-devel] [PATCH RFC 6/6] target-i386: make cpus childs of /machine

[Andreas Färber]

Am 17.04.2012 12:28, schrieb Igor Mammedov:
> ----- Original Message -----
>> From: "Andreas Färber" <address@hidden>
>> To: "Paolo Bonzini" <address@hidden>, "Igor Mammedov" <address@hidden>
>> Cc: address@hidden, address@hidden, "jan kiszka" <address@hidden>
>> 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 <address@hidden>
>>>>
>>>> Signed-off-by: Igor Mammedov <address@hidden>
>>>> ---
>>>>  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

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