Re: [RFC v6 10/11] accel: introduce AccelCPUClass extending CPUClass

[Claudio Fontana <cfontana@suse.de>]

On 1/11/21 11:35 PM, Eduardo Habkost wrote:
> On Mon, Jan 11, 2021 at 05:13:27PM +0100, Claudio Fontana wrote:
>> Happy new year,
> 
> Hi!
> 
>>
>> picking up this topic again, i am looking at at now a different aspect of this problem, of setting the right tcg ops for the right cpu class.
>>
>> This issue I am highlighting is present because different targets behave differently in this regard.
>>
>> Ie, we have targets for which we always initialize all cpu classes, as a result of different machine definitions.
>>
>> This is the case of arm, for example where we end up with backtraces like:
>>
>> arm_v7m_class_init
>> type_initialize
>> object_class_foreach_tramp
>> g_hash_table_foreach ()
>> object_class_foreach
>> object_class_get_list
>> select_machine ()
>> qemu_init
>> main
>>
>> with the arm_v7m_class_init called even if we are just going to use an aarch64 cpu (so the class initializer for arm_v7m is called even for unused cpus classes),
>>
>> while in other cases we have the target explicitly relying on the fact that only the right cpu class is initialized, for example in cris we have code like:
> 
> This shouldn't matter at all, because class_init is not supposed
> to have any side effects outside the corresponding ObjectClass
> struct.
> 
> So, I don't understand what you mean below:
> 
>>
>> target/cris/cpu.c:
>>
>> static void crisv9_cpu_class_init(ObjectClass *oc, void *data)
>> {
>>     CPUClass *cc = CPU_CLASS(oc);
>>     CRISCPUClass *ccc = CRIS_CPU_CLASS(oc);
>>
>>     ccc->vr = 9;
>>     cc->do_interrupt = crisv10_cpu_do_interrupt;
>>     cc->gdb_read_register = crisv10_cpu_gdb_read_register;
>>     cc->tcg_initialize = cris_initialize_crisv10_tcg;
>> }
>>
>> where the class initialization of the cpu is explicitly setting the methods of CPUClass, therefore implicitly relying on the fact that no other class initializer screws things up.
> 
> I don't see the problem here.  Having all other class
> initializers being called should be completely OK, because each
> class has its own ObjectClass struct.
> 
> 
>>
>> Given this context, which one of these methods is "right"?
>> Should we rework things so that only used cpu classes are actually initialized?
> 
> This option wouldn't make sense.  class_init is supposed to be
> called on demand on class lookup, and can be triggered by
> object_class_get_list(), object_class_by_name(), or similar
> functions.  This is by design.
> 
> 
>> Or should we maybe not do these settings in cpu class_init at all, but rather at cpu initfn time, or at cpu realize time?
> 
> If you are talking about initializing
> ObjectClass/CPUClass/...Class fields, they can always be safely
> initialized in class_init.

Ok, so I can initialize the CPUClass fields for the tcg ops in the CPUClass subclass class inits safely..

Thanks for clearing this up!

Claudio

> 
> If you are talking about touching anything outside the class
> struct (like in CPUState), class_init is not the right place to
> do it.
>