Re: [Qemu-ppc] [Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() calls to realize functions

[Laurent Vivier]

On 19/10/2016 15:46, Andrew Jones wrote:
> On Wed, Oct 19, 2016 at 10:30:52AM +0200, Laurent Vivier wrote:
>>
>>
>> On 19/10/2016 10:13, Andrew Jones wrote:
>>> On Tue, Oct 18, 2016 at 09:22:52PM +0200, Laurent Vivier wrote:
>>>> Modify all CPUs to call it from XXX_cpu_realizefn() function.
>>>>
>>>> Remove all the cannot_destroy_with_object_finalize_yet as
>>>> unsafe references have been moved to cpu_exec_realizefn().
>>>> (tested with QOM command provided by commit 4c315c27)
>>>>
>>>> for arm:
>>>>
>>>> Setting of cpu->mp_affinity is moved from arm_cpu_initfn()
>>>> to arm_cpu_realizefn() as setting of cpu_index is now done
>>>> in cpu_exec_realizefn(). To avoid to overwrite an user defined
>>>> value, we set it to an invalid value by default, and update
>>>> it in realize function only if the value is still invalid.
>>>>
>>>> Signed-off-by: Laurent Vivier <address@hidden>
>>>> Reviewed-by: David Gibson <address@hidden>
>>>> Reviewed-by: Igor Mammedov <address@hidden>
>>>> ---
>>> [...]
>>>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>>>> index 1b9540e..f0d2074 100644
>>>> --- a/target-arm/cpu.c
>>>> +++ b/target-arm/cpu.c
>>>> @@ -33,6 +33,8 @@
>>>>  #include "sysemu/kvm.h"
>>>>  #include "kvm_arm.h"
>>>>  
>>>> +#define MP_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
>>>
>>> I would have defined this next to ARM64_AFFINITY_MASK in
>>> target-arm/cpu-qom.h
>>
>> It was my first idea, but all macros in cpu-qom.h start by ARM_ or
>> ARM64_, this one looks like a local macro (the name is too generic), so
>> I put it in cpu.c.
> 
> Feel free to prefix it with ARM64_ :-) How about
> ARM64_AFFINITY_INVALID
> 
>>
>>>> +
>>>>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>>>>  {
>>>>      ARMCPU *cpu = ARM_CPU(cs);
>>>> @@ -441,22 +443,11 @@ static void arm_cpu_initfn(Object *obj)
>>>>      CPUState *cs = CPU(obj);
>>>>      ARMCPU *cpu = ARM_CPU(obj);
>>>>      static bool inited;
>>>> -    uint32_t Aff1, Aff0;
>>>>  
>>>>      cs->env_ptr = &cpu->env;
>>>> -    cpu_exec_init(cs, &error_abort);
>>>>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>>>>                                           g_free, g_free);
>>>>  
>>>> -    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will 
>>>> override it.
>>>> -     * We don't support setting cluster ID ([16..23]) (known as Aff2
>>>> -     * in later ARM ARM versions), or any of the higher affinity level 
>>>> fields,
>>>> -     * so these bits always RAZ.
>>>> -     */
>>>> -    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
>>>> -    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
>>>> -    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
>>>> -
>>>>  #ifndef CONFIG_USER_ONLY
>>>>      /* Our inbound IRQ and FIQ lines */
>>>>      if (kvm_enabled()) {
>>>> @@ -576,6 +567,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>>>> **errp)
>>>>      ARMCPU *cpu = ARM_CPU(dev);
>>>>      ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
>>>>      CPUARMState *env = &cpu->env;
>>>> +    Error *local_err = NULL;
>>>> +    uint32_t Aff1, Aff0;
>>>> +
>>>> +    cpu_exec_realizefn(cs, &local_err);
>>>> +    if (local_err != NULL) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>>  
>>>>      /* Some features automatically imply others: */
>>>>      if (arm_feature(env, ARM_FEATURE_V8)) {
>>>> @@ -631,6 +630,17 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>>>> **errp)
>>>>          set_feature(env, ARM_FEATURE_THUMB_DSP);
>>>>      }
>>>>  
>>>> +    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will 
>>>> override it.
>>>> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
>>>> +     * in later ARM ARM versions), or any of the higher affinity level 
>>>> fields,
>>>> +     * so these bits always RAZ.
>>>> +     */
>>>> +    if (cpu->mp_affinity == MP_AFFINITY_INVALID) {
>>>> +        Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
>>>> +        Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
>>>
>>> I think ARM_CPUS_PER_CLUSTER should be renamed to
>>> ARM_DEFAULT_CPUS_PER_CLUSTER and either moved from where
>>> it's currently defined (above arm_cpu_initfn) to just above
>>> arm_cpu_realizefn, or to the same place as ARM64_AFFINITY_MASK
>>> and MP_AFFINITY_INVALID.
>>>
>>> Aff0 and Aff1 could be declared in this scope, as they're only
>>> used here.
>>
>> The goal was to have as less diff as possible with my previous patch, so
>> I didn't move the declaration. This will not change the generated code.
> 
> I don't care much, but it's not just about generated code. It's also
> about reviewers being able to dismiss those variables when they get
> to the end of the scope (avoids too much brain clutter)

OK, I will modify the patch as you say.

Thanks,
Laurent