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 <lviv...@redhat.com> >>>> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au> >>>> Reviewed-by: Igor Mammedov <imamm...@redhat.com> >>>> --- >>> [...] >>>> 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