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

[Laurent Vivier]

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.

>> +
>>  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.
> 
>> +        cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
>> +    }
>> +
>>      if (cpu->reset_hivecs) {
>>              cpu->reset_sctlr |= (1 << 13);
>>      }
>> @@ -1461,7 +1471,7 @@ static Property arm_cpu_properties[] = {
>>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
>>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>> -    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 0),
>> +    DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, 
>> MP_AFFINITY_INVALID),
>>      DEFINE_PROP_END_OF_LIST()
>>  };
>>  
>> @@ -1533,17 +1543,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>      cc->debug_check_watchpoint = arm_debug_check_watchpoint;
>>  
>>      cc->disas_set_info = arm_disas_set_info;
>> -
>> -    /*
>> -     * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves
>> -     * the object in cpus -> dangling pointer after final
>> -     * object_unref().
>> -     *
>> -     * Once this is fixed, the devices that create ARM CPUs should be
>> -     * updated not to set cannot_destroy_with_object_finalize_yet,
>> -     * unless they still screw up something else.
>> -     */
>> -    dc->cannot_destroy_with_object_finalize_yet = true;
>>  }
>>  
>>  static void cpu_register(const ARMCPUInfo *info)
> [...]
> 
> Otherwise looks good to me.

Thanks,
Laurent