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

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" <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

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

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

[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!

Paolo

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

[Andreas Färber]

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

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

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

[Paolo Bonzini]

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

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

[Igor Mammedov]

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?

-- 
-----
  Igor

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

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux