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/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.

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