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

[Andrew Jones]

On Thu, Oct 20, 2016 at 01:26:03PM +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>
> Reviewed-by: Eduardo Habkost <address@hidden>
> ---
[...]
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 3991173..a42495b 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -80,9 +80,11 @@ void arm_gt_stimer_cb(void *opaque);
>  #define ARM_AFF2_MASK  (0xFFULL << ARM_AFF2_SHIFT)
>  #define ARM_AFF3_SHIFT 32
>  #define ARM_AFF3_MASK  (0xFFULL << ARM_AFF3_SHIFT)
> +#define ARM_DEFAULT_CPUS_PER_CLUSTER 8
>  
>  #define ARM32_AFFINITY_MASK (ARM_AFF0_MASK|ARM_AFF1_MASK|ARM_AFF2_MASK)
>  #define ARM64_AFFINITY_MASK \
>      (ARM_AFF0_MASK|ARM_AFF1_MASK|ARM_AFF2_MASK|ARM_AFF3_MASK)
> +#define ARM64_AFFINITY_INVALID (~ARM64_AFFINITY_MASK)
>  
>  #endif
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1b9540e..fb272a8 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -434,29 +434,16 @@ static void arm_disas_set_info(CPUState *cpu, 
> disassemble_info *info)
>      }
>  }
>  
> -#define ARM_CPUS_PER_CLUSTER 8
> -
>  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 +563,13 @@ 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;
> +
> +    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 +625,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 == ARM64_AFFINITY_INVALID) {
> +        uint32_t Aff1 = cs->cpu_index / ARM_DEFAULT_CPUS_PER_CLUSTER;
> +        uint32_t Aff0 = cs->cpu_index % ARM_DEFAULT_CPUS_PER_CLUSTER;

These should have been uint8_t, but I should have pointed that out before
and it probably doesn't really matter (it was uint32_t before...)

> +        cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> +    }
> +
>      if (cpu->reset_hivecs) {
>              cpu->reset_sctlr |= (1 << 13);
>      }
> @@ -1461,7 +1466,8 @@ 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, ARM64_AFFINITY_INVALID),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> @@ -1533,17 +1539,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;
>  }

For the ARM part

Reviewed-by: Andrew Jones <address@hidden>

Thanks,
drew