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

[Eduardo Habkost]

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?)

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

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

> [...]

-- 
Eduardo