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

[Claudio Fontana <cfontana@suse.de>]

On 12/19/20 12:00 AM, Claudio Fontana wrote:
> On 12/18/20 11:30 PM, Claudio Fontana wrote:
>> On 12/18/20 10:55 PM, Claudio Fontana wrote:
>>> On 12/18/20 7:04 PM, Claudio Fontana wrote:
>>>> On 12/18/20 7:01 PM, Paolo Bonzini wrote:
>>>>> On 18/12/20 18:51, Claudio Fontana wrote:
>>>>>> But with things like cris/ for example,
>>>>>> the tcg functions to use are actually versioned per each subclass of TYPE_CRIS_CPU.
>>>>>>
>>>>>> Different tcg_ops need to be used for different subclasses of the CPU_RESOLVING_TYPE.
>>>>>
>>>>> CRIS is not that bad since it's TCG only.  You can just make it a field 
>>>>> in CRISCPUClass and copy it over to tcg_ops.
>>>>>
>>>>> I think ARM had something similar though, with different do_interrupt 
>>>>> implementations for M and A processors.  Somebody from Linaro was 
>>>>> cleaning it up as part of some BQL work, but it was never merged.  But 
>>>>> even in that case, do_interrupt is somewhat special for ARM so making it 
>>>>> an xxxCPUClass field makes sense.
>>>>>
>>>>> Paolo
>>>>
>>>> Ok that's a good alternative,
>>>>
>>>>>
>>>>>> So in order to avoid code in the class initialization like this:
>>>>>>
>>>>>> if (version1) { then set the tcg ops for version 1; }
>>>>>> if (version2) { then set the tcg ops for version 2; ...} etc,
>>>>>>
>>>>>> we could define the right tcg op variants corresponding to the cpu variants, so that everything can be matched automatically.
>>>>>>
>>>>>> But I think we'd need to pass explicitly the cpu type in accel_init_cpu_interfaces for this to work..
>>>>>> we could still in the future call accel_init_cpu_interfaces multiple times, once for each cpu model we want to use.
>>>>>>
>>>>>> Or, we could do something else: we could delay the accel cpu interface initialization and call it in cpu_create(const char *typename),
>>>>>> where typename needs to be known for sure.
>>>>
>>>>
>>>> I take you don't like this idea to initialize the accel cpu interface in cpu_create()?
>>>> It seems to make sense to me, but any drawbacks?
>>>>
>>>> Ciao thanks!
>>>>
>>>> Claudio
>>>>
>>>>


Happy new year,

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:

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.

Given this context, which one of these methods is "right"? Should we rework things so that only used cpu classes are actually initialized?
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?

any comments/preferences?

Thanks,

Claudio