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

[Andrew Jones]

On Tue, Oct 18, 2016 at 01:22:07PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 18, 2016 at 04:22:47PM +0200, Andrew Jones wrote:
> > 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
> 
> Thanks!
> 
> > 
> > > 
> > > > 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.
> 
> Right: if in this case the realize function can query
> current_machine for input instead of requiring a new
> field/property to be used as input, that's even better. But
> sometimes we can't easily avoid adding fields/properties that
> aren't device state, to be used as input to realize (e.g.
> nr_cores/nr_threads).
> 
> (This is just theoretical discussion, anyway, as just using
> cluster-size as input probably doesn't solve Raspberry Pi's
> problem?)

Agreed it's theoretical, but still interesting. I'm always unsure
what good practice with the object model is. I'll note that it's
accepted to reach into machine state through current_machine from
the machine's devices. I'll need to apply that when I get around
to respinning the 'smp rework' series.

> 
> > 
> > > 
> > > > 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?
> 
> Property defaults are normally initialized on instance_init.

OK, so after this series goes through we can un-property mp_affinity and
add an mp_affinity initialization back to arm_cpu_initfn, but one that
only sets the initial invalid value.

> 
> > 
> > > 
> > > >  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.
> 
> Sounds good to me.

Good. Laurent, to be clear, MP_AFFINITY_INVALID doesn't currently exist,
and it should be defined as ~ARM64_AFFINITY_MASK

Thanks,
drew