Re: [Qemu-devel] [PATCH v3 2/3] exec: rename cpu_exec_init() as cpu_exec_realizefn()

[Andrew Jones]

On Tue, Oct 18, 2016 at 11:18:29AM -0200, Eduardo Habkost wrote:
> On Tue, Oct 18, 2016 at 03:00:07PM +0200, Andrew Jones wrote:
> > On Mon, Oct 17, 2016 at 05:20:22PM -0200, Eduardo Habkost wrote:
> > > On Sat, Oct 15, 2016 at 12:52:48AM +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().
> > > > 
> > > > Signed-off-by: Laurent Vivier <address@hidden>
> > > [...]
> > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > > index 1b9540e..364a45d 100644
> > > > --- a/target-arm/cpu.c
> > > > +++ b/target-arm/cpu.c
> > > > @@ -441,22 +441,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()) {
> > > [...]
> > > > @@ -631,6 +628,15 @@ 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.
> > > > +     */
> > > > +    Aff1 = cs->cpu_index / ARM_CPUS_PER_CLUSTER;
> > > > +    Aff0 = cs->cpu_index % ARM_CPUS_PER_CLUSTER;
> > > > +    cpu->mp_affinity = (Aff1 << ARM_AFF1_SHIFT) | Aff0;
> > > > +
> > > 
> > > This will override any value set in the "mp-affinity" property,
> > > The mp-affinity property can be set by the user in the
> > > command-line, and it is also set by machvirt_init() in
> > > hw/arm/virt.c.
> > 
> > I'm glad you noticed this Eduardo. We'd indeed lose changes made to
> > the MPIDRs for mach-virt and Raspberry Pi.
> > 
> > > 
> > > Considering that each CPU is supposed to have a different value,
> > > I doubt there are existing use cases for mp-affinity being set
> > > directly by the user.
> > 
> > Probably not. It was turned into a property in order for the gicv3
> > model to access it. I don't think that's necessary, so we can just
> > un-property it, accessing it directly from the structure instead.
> > 
> > > 
> > > I suggest having a "cluster-size" property, instead of
> > > "mp-affinity". This way the mp_affinity field can be calculated
> > > on realize, based on the configured cluster-size.
> > 
> > This won't work for Raspberry Pi's MPIDR adjustment.
> 
> Where's that code?

hw/arm/bcm2836.c:109

> 
> > Anyway, IMO realize should only set values when it knows
> > nothing has been set.
> 
> Realize could also initialize some (private) fields using other
> (public) fields as input. Instead of making machine code
> calculate mp_affinity, machine code could just provide input for
> realize to calculate the right values.
> 
> But maybe my cluster-size suggestion won't work anyway because of
> other cases where mp_affinity is set (I didn't check all code
> that sets/gets mp_affinity).

Anyway, cluster-size is machine state, not cpu state.

> 
> > If values have been set, it should only sanity check them. In this
> > case it's safe to simply check for uniqueness and zeros;
> > 
> >  1) The MPIDRs are all zero. As they're not all unique that's not
> >     valid. It's pretty safe to assume they just weren't set in this
> >     case though, so we can set the default values without complaining.
> >     If there was only one cpu, then MPIDR=0 is valid, but that's the
> >     default anyway.
> 
> Implementing this check in arm_cpu_realizefn() would be
> difficult, as the CPU realize method don't have access to the
> other CPUs. But maybe we can use some other default value to
> indicate that the field was never set? UINT64_MAX?

I considered that, but if we un-property ARMCPU->mp_affinity, then where
can it be initialized to "MP_AFFINITY_INVALID", which would be ff000000?

> 
> >  2) The MPIDRs are all unique, or there's a single cpu and its MPIDR
> >     is not zero. In this case we shouldn't touch them.
> >  3) The MPIDRs are not all zero and not all unique. This is a failed
> >     sanity check and should abort.
> 
> A sanity check would be useful, but I don't know where exactly it
> could be implemented. Is there any post-machine-init hook you could use?

We do have virt_guest_info_machine_done for mach-virt, but it'd be
sort of ugly to put it there...

This is one of those things where it's true that the set of all cpu
MPIDRs is machine state, and thus should be checked by the machine for
uniqueness. But, the purpose of checking is to ensure each CPU instance
is sane, a CPU internal motivation. Doing the check at the machine level
is ugly. Doing the check at the CPU level is either not possible or
breaks the object model. Sigh... maybe we should just forget it.


OK, to proceed with this patch, since mp-affinity *is* currently a
property, we can just change its default value to ff000000. Then,
when moving the calculation to realize we just need to ensure the
current value is ff000000 before overwriting it.

Another comment on this patch; please rename the definition
ARM_CPUS_PER_CLUSTER to ARM_DEFAULT_CPUS_PER_CLUSTER, and move it
down to just above arm_cpu_realizefn.

Thanks,
drew