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

[Claudio Fontana <cfontana@suse.de>]

On 11/27/20 7:21 AM, Paolo Bonzini wrote:
> On 26/11/20 23:32, Claudio Fontana wrote:
>> +    if (acc) {
>> +        object_class_foreach(accel_init_cpu_int_aux, cpu_type, false, acc);
>> +    }
> 
> Any reason to do it for cpu_type only, rather than for all subclasses of 
> CPU_RESOLVING_TYPE?  This would remove the cpu_type argument to 
> accel_init_cpu_interfaces and accel_init_interfaces.
> 
> Otherwise I haven't done a careful review yet but it looks very nice, 
> thanks!
> 
> Paolo
> 

Hi Paolo,

going back to this topic:

while doing some experiments in applying the ACCEL_CPU_TYPE classes for all targets (TCG for now, not KVM or other hw accels),
and merging all tcg cpu ops,

I did encounter some issue.

For the simplest of targets like hppa for example, it works just fine, we can do:

static void hppa_tcg_cpu_class_init(CPUClass *cc)
{
    cc->tcg_ops.initialize = hppa_translate_init;
    cc->tcg_ops.do_interrupt = hppa_cpu_do_interrupt;
    cc->tcg_ops.cpu_exec_interrupt = hppa_cpu_exec_interrupt;
    cc->tcg_ops.synchronize_from_tb = hppa_cpu_synchronize_from_tb;
    cc->tcg_ops.tlb_fill = hppa_cpu_tlb_fill;
#ifndef CONFIG_USER_ONLY
    cc->tcg_ops.do_unaligned_access = hppa_cpu_do_unaligned_access;
#endif
}

*(later on I will try to change the cc->tcg_ops thing to something that ends up in cc->accel_cpu->tcg_ops->initialize, as I try to merge tcg_ops with accel_cpu, but this is spoiler of the future)*

static void hppa_tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
{
    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);

    acc->cpu_class_init = hppa_tcg_cpu_class_init;
}

static const TypeInfo hppa_tcg_cpu_accel_type = {
    .name = ACCEL_CPU_NAME("tcg"),

    .parent = TYPE_ACCEL_CPU,
    .class_init = hppa_tcg_cpu_accel_class_init,
};

static void hppa_cpu_register_types(void)
{
    type_register_static(&hppa_cpu_type_info);
    type_register_static(&hppa_tcg_cpu_accel_type);
}

type_init(hppa_cpu_register_types)

So this is good.

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.

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.

This last option seems kinda attractive, but any ideas?

Thanks, ciao,

Claudio