Re: [Qemu-devel] [PATCH 6/8] [PATCH RFC v3] s390-qemu: cpu hotplug - s390 cpu init improvements for hotplug

["Jason J. Herne" <jjherne@linux.vnet.ibm.com>]

On 09/13/2013 11:24 AM, Jason J. Herne wrote:
> On 09/05/2013 08:28 AM, Andreas Färber wrote:
>> Am 01.08.2013 16:12, schrieb Jason J. Herne:
>>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>>
>>>      s390_new_cpu is created to encapsulate the creation of a new QOM
>>> S390CPU
>>>      object given a cpuid and a model string.
>>>
>>>      All actual cpu initialization code is moved from boot time
>>> specific functions to
>>>      s390_cpu_initfn (qom init routine) or to s390_new_cpu. This is
>>> done to allow us
>>>      to use the same basic code path for a cpu created at boot time
>>> and one created
>>>      during a hotplug operation.
>>
>> Intentionally indented?
>>
>>>
>>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>>> ---
>>>   hw/s390x/s390-virtio.c |   25 ++++++++++++-------------
>>>   target-s390x/cpu.c     |    4 ++--
>>>   target-s390x/cpu.h     |    1 +
>>>   target-s390x/helper.c  |   12 ++++++++++++
>>>   4 files changed, 27 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>>> index 5ad9cf3..103f32e 100644
>>> --- a/hw/s390x/s390-virtio.c
>>> +++ b/hw/s390x/s390-virtio.c
> ...
>>> @@ -197,19 +202,13 @@ void s390_init_cpus(const char *cpu_model)
>>>           cpu_model = "host";
>>>       }
>>>
>>> -    ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>>> -
>>> -    for (i = 0; i < smp_cpus; i++) {
>>> -        S390CPU *cpu;
>>> -        CPUState *cs;
>>> +    ipi_states = g_malloc(sizeof(S390CPU *) * max_cpus);
>>>
>>> -        cpu = cpu_s390x_init(cpu_model);
>>> -        cs = CPU(cpu);
>>> -
>>> -        ipi_states[i] = cpu;
>>> -        cs->halted = 1;
>>> -        cpu->env.exception_index = EXCP_HLT;
>>> -        cpu->env.storage_keys = s390_get_storage_keys();
>>> +    for (i = 0; i < max_cpus; i++) {
>>> +        ipi_states[i] = NULL;
>>
>> Using g_malloc0() above would hopefully be more efficient and would
>> allow to leave the loop untouched for easier review.
>>
>
> I don't follow. I'm completely changing this loop.  I do not believe we
> can obtain the same functionality implemented here while not touching
> the loop.
>
>>> +        if (i < smp_cpus) {
>>> +            s390_new_cpu(cpu_model, i);
>>> +        }
>>>       }
>>>   }
>>>
>>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>>> index 6be6c08..c90a91c 100644
>>> --- a/target-s390x/cpu.c
>>> +++ b/target-s390x/cpu.c
>>> @@ -116,7 +116,6 @@ static void s390_cpu_initfn(Object *obj)
>>>       S390CPU *cpu = S390_CPU(obj);
>>>       CPUS390XState *env = &cpu->env;
>>>       static bool inited;
>>> -    static int cpu_num = 0;
>>>   #if !defined(CONFIG_USER_ONLY)
>>>       struct tm tm;
>>>   #endif
>>> @@ -135,8 +134,9 @@ static void s390_cpu_initfn(Object *obj)
>>>        * cpu counter in s390_cpu_reset to a negative number at
>>>        * initial ipl */
>>>       cs->halted = 1;
>>> +    cpu->env.exception_index = EXCP_HLT;
>>> +    env->storage_keys = s390_get_storage_keys();
>>
>> 4/8?
>>
>
> Are you asking if this belongs in patch #4? if so, I would say no.  It
> does deal with storage keys, yes. But we're not changing storage key
> semantics here (as we are in patch 4), we're just moving where the
> storage key ptr gets set.  This is in support of re-organizing how cpus
> are initialized as per patch title/description.
>

Ping? :)

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)