qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() ca


From: Andrew Jones
Subject: Re: [Qemu-arm] [Qemu-devel] [PATCH v4 2/3] exec: move cpu_exec_init() calls to realize functions
Date: Wed, 19 Oct 2016 15:46:08 +0200
User-agent: Mutt/1.6.0.1 (2016-04-01)

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)

> > 
> >> +        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
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]