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

add a new optional interface to CPUClass,
which allows accelerators to extend the CPUClass
with additional accelerator-specific initializations.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 MAINTAINERS                 |  1 +
 accel/accel-common.c        | 43 +++++++++++++++++++++++++++++++++++++
 include/hw/core/accel-cpu.h | 28 ++++++++++++++++++++++++
 include/hw/core/cpu.h       |  4 ++++
 4 files changed, 76 insertions(+)
 create mode 100644 include/hw/core/accel-cpu.h

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

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,

yes, I thought to pass cpu_type in order to set the interface only for the cpu that is actually used,
instead of looping over all cpu models, just to be a bit quicker, but both things should work.

Ciao,

Claudio

On 11/27/20 9:59 AM, Claudio Fontana wrote:
> 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,
> 
> yes, I thought to pass cpu_type in order to set the interface only for the cpu that is actually used,
> instead of looping over all cpu models, just to be a bit quicker, but both things should work.
> 
> Ciao,
> 
> Claudio
> 

Note that this actually creates a bug that is caught _ONLY_ by

acceptance-system-centos.

The gist of it is that cpu_type (or current_machine->default_cpu_type) is _not_ guaranteed to be set,
the code there is a bit misleading I think.

I'll look into it, but just wanted to warn early about it.

Ciao,

Claudio

On 11/27/20 12:22 PM, Claudio Fontana wrote:
> On 11/27/20 9:59 AM, Claudio Fontana wrote:
>> 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,
>>
>> yes, I thought to pass cpu_type in order to set the interface only for the cpu that is actually used,
>> instead of looping over all cpu models, just to be a bit quicker, but both things should work.
>>
>> Ciao,
>>
>> Claudio
>>
> 
> Note that this actually creates a bug that is caught _ONLY_ by
> 
> acceptance-system-centos.
> 
> The gist of it is that cpu_type (or current_machine->default_cpu_type) is _not_ guaranteed to be set,
> the code there is a bit misleading I think.
> 
> I'll look into it, but just wanted to warn early about it.
> 
> Ciao,
> 
> Claudio
> 

This seems to be due to "-machine none", is machine none supposed to have no default cpu_type?
Is it expected that for machine none current_machine->cpu_type is NULL, or is it a bug?

Thanks,

Claudio

On 27/11/20 12:41, Claudio Fontana wrote:
> This seems to be due to "-machine none", is machine none supposed to
> have no default cpu_type? Is it expected that for machine none
> current_machine->cpu_type is NULL, or is it a bug?

"-machine none" has no CPU at all, so I think anything is acceptable. 
There's also the possibility of emulating big.LITTLE machines in the 
future with >1 cpu_type, so the cop out of doing it on the whole 
hierarchy is easiest.

Paolo

On 11/27/20 2:31 PM, Paolo Bonzini wrote:
> On 27/11/20 12:41, Claudio Fontana wrote:
>> This seems to be due to "-machine none", is machine none supposed to
>> have no default cpu_type? Is it expected that for machine none
>> current_machine->cpu_type is NULL, or is it a bug?
> 
> "-machine none" has no CPU at all, so I think anything is acceptable. 
> There's also the possibility of emulating big.LITTLE machines in the 
> future with >1 cpu_type, so the cop out of doing it on the whole 
> hierarchy is easiest.
> 
> Paolo
> 

Ok will take that route then.

Thanks!

Claudio

On Thu, Nov 26, 2020 at 11:32:17PM +0100, Claudio Fontana wrote:
> add a new optional interface to CPUClass,
> which allows accelerators to extend the CPUClass
> with additional accelerator-specific initializations.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
[...]
> +static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
> +{
> +    CPUClass *cc = CPU_CLASS(klass);
> +    AccelCPUClass *accel_cpu_interface = opaque;
> +
> +    cc->accel_cpu_interface = accel_cpu_interface;
> +    if (accel_cpu_interface->cpu_class_init) {
> +        accel_cpu_interface->cpu_class_init(cc);
> +    }
> +}

So, now that the approach we're following to trigger the
accel_init_cpu*() call is less controversial (thanks for your
patience!), we can try to address the monkey patching issue:

Monkey patching classes like this is acceptable as an initial
solution, but I'd like us to have a plan to eventually get rid of
it.  Monkey patching CPU classes makes querying of CPU model
information less predictable and subtly dependent on QEMU
initialization state.

Removing CPUClass.accel_cpu_interface may be easy, because it
should be possible to just call current_accel() when realizing
CPUs.  Getting rid of CPUClass.cpu_class_init might be more
difficult, depending on what the ->cpu_class_init() function is
doing.

> +
> +/* initialize the arch-specific accel CpuClass interfaces */
> +static void accel_init_cpu_interfaces(AccelClass *ac, const char *cpu_type)
> +{
> +    const char *ac_name; /* AccelClass name */
> +    char *acc_name;      /* AccelCPUClass name */
> +    ObjectClass *acc;    /* AccelCPUClass */
> +
> +    ac_name = object_class_get_name(OBJECT_CLASS(ac));
> +    g_assert(ac_name != NULL);
> +
> +    acc_name = g_strdup_printf("%s-%s", ac_name, CPU_RESOLVING_TYPE);
> +    acc = object_class_by_name(acc_name);
> +    g_free(acc_name);
> +
> +    if (acc) {
> +        object_class_foreach(accel_init_cpu_int_aux, cpu_type, false, acc);
> +    }
> +}
> +
>  void accel_init_interfaces(AccelClass *ac, const char *cpu_type)
>  {
>  #ifndef CONFIG_USER_ONLY
>      accel_init_ops_interfaces(ac);
>  #endif /* !CONFIG_USER_ONLY */
> +
> +    accel_init_cpu_interfaces(ac, cpu_type);
>  }
[...]

On 11/27/20 6:06 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2020 at 11:32:17PM +0100, Claudio Fontana wrote:
>> add a new optional interface to CPUClass,
>> which allows accelerators to extend the CPUClass
>> with additional accelerator-specific initializations.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
> [...]
>> +static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
>> +{
>> +    CPUClass *cc = CPU_CLASS(klass);
>> +    AccelCPUClass *accel_cpu_interface = opaque;
>> +
>> +    cc->accel_cpu_interface = accel_cpu_interface;
>> +    if (accel_cpu_interface->cpu_class_init) {
>> +        accel_cpu_interface->cpu_class_init(cc);
>> +    }
>> +}
> 
> So, now that the approach we're following to trigger the
> accel_init_cpu*() call is less controversial (thanks for your
> patience!), we can try to address the monkey patching issue:
> 
> Monkey patching classes like this is acceptable as an initial
> solution, but I'd like us to have a plan to eventually get rid of
> it.  Monkey patching CPU classes makes querying of CPU model
> information less predictable and subtly dependent on QEMU
> initialization state.


The question of QEMU initialization state and the querying of supported functionality, also in relationship with the loadable modules, is I think a larger discussion.

Regardless of the amount of glue code and lipstick, this is hiding the fact that the fundamentals of the object hierarchy for cpus are wrong,
and are (unfortunately) codified as part of the external interface.


> 
> Removing CPUClass.accel_cpu_interface may be easy, because it
> should be possible to just call current_accel() when realizing
> CPUs.  Getting rid of CPUClass.cpu_class_init might be more
> difficult, depending on what the ->cpu_class_init() function is
> doing.


This seems to be for a next step to me.

Ciao,

Claudio


> 
>> +
>> +/* initialize the arch-specific accel CpuClass interfaces */
>> +static void accel_init_cpu_interfaces(AccelClass *ac, const char *cpu_type)
>> +{
>> +    const char *ac_name; /* AccelClass name */
>> +    char *acc_name;      /* AccelCPUClass name */
>> +    ObjectClass *acc;    /* AccelCPUClass */
>> +
>> +    ac_name = object_class_get_name(OBJECT_CLASS(ac));
>> +    g_assert(ac_name != NULL);
>> +
>> +    acc_name = g_strdup_printf("%s-%s", ac_name, CPU_RESOLVING_TYPE);
>> +    acc = object_class_by_name(acc_name);
>> +    g_free(acc_name);
>> +
>> +    if (acc) {
>> +        object_class_foreach(accel_init_cpu_int_aux, cpu_type, false, acc);
>> +    }
>> +}
>> +
>>  void accel_init_interfaces(AccelClass *ac, const char *cpu_type)
>>  {
>>  #ifndef CONFIG_USER_ONLY
>>      accel_init_ops_interfaces(ac);
>>  #endif /* !CONFIG_USER_ONLY */
>> +
>> +    accel_init_cpu_interfaces(ac, cpu_type);
>>  }
> [...]
>

On Fri, Nov 27, 2020 at 06:58:22PM +0100, Claudio Fontana wrote:
> On 11/27/20 6:06 PM, Eduardo Habkost wrote:
> > On Thu, Nov 26, 2020 at 11:32:17PM +0100, Claudio Fontana wrote:
> >> add a new optional interface to CPUClass,
> >> which allows accelerators to extend the CPUClass
> >> with additional accelerator-specific initializations.
> >>
> >> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> >> ---
> > [...]
> >> +static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
> >> +{
> >> +    CPUClass *cc = CPU_CLASS(klass);
> >> +    AccelCPUClass *accel_cpu_interface = opaque;
> >> +
> >> +    cc->accel_cpu_interface = accel_cpu_interface;
> >> +    if (accel_cpu_interface->cpu_class_init) {
> >> +        accel_cpu_interface->cpu_class_init(cc);
> >> +    }
> >> +}
> > 
> > So, now that the approach we're following to trigger the
> > accel_init_cpu*() call is less controversial (thanks for your
> > patience!), we can try to address the monkey patching issue:
> > 
> > Monkey patching classes like this is acceptable as an initial
> > solution, but I'd like us to have a plan to eventually get rid of
> > it.  Monkey patching CPU classes makes querying of CPU model
> > information less predictable and subtly dependent on QEMU
> > initialization state.
> 
> 
> The question of QEMU initialization state and the querying of supported functionality, also in relationship with the loadable modules, is I think a larger discussion.
> 
> Regardless of the amount of glue code and lipstick, this is hiding the fact that the fundamentals of the object hierarchy for cpus are wrong,
> and are (unfortunately) codified as part of the external interface.

That's probably right, and removal of monkey patching might force
us to change our external interfaces.

> 
> 
> > 
> > Removing CPUClass.accel_cpu_interface may be easy, because it
> > should be possible to just call current_accel() when realizing
> > CPUs.  Getting rid of CPUClass.cpu_class_init might be more
> > difficult, depending on what the ->cpu_class_init() function is
> > doing.
> 
> 
> This seems to be for a next step to me.

Agreed, although I'd like to understand what makes
AccelCPUClass.cpu_class_init() so important in the first version
(considering that existing x86_cpu_class_init() has zero
tcg_enabled() calls today).

On 11/27/20 7:13 PM, Eduardo Habkost wrote:
> On Fri, Nov 27, 2020 at 06:58:22PM +0100, Claudio Fontana wrote:
>> On 11/27/20 6:06 PM, Eduardo Habkost wrote:
>>> On Thu, Nov 26, 2020 at 11:32:17PM +0100, Claudio Fontana wrote:
>>>> add a new optional interface to CPUClass,
>>>> which allows accelerators to extend the CPUClass
>>>> with additional accelerator-specific initializations.
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>> ---
>>> [...]
>>>> +static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
>>>> +{
>>>> +    CPUClass *cc = CPU_CLASS(klass);
>>>> +    AccelCPUClass *accel_cpu_interface = opaque;
>>>> +
>>>> +    cc->accel_cpu_interface = accel_cpu_interface;
>>>> +    if (accel_cpu_interface->cpu_class_init) {
>>>> +        accel_cpu_interface->cpu_class_init(cc);
>>>> +    }
>>>> +}
>>>
>>> So, now that the approach we're following to trigger the
>>> accel_init_cpu*() call is less controversial (thanks for your
>>> patience!), we can try to address the monkey patching issue:
>>>
>>> Monkey patching classes like this is acceptable as an initial
>>> solution, but I'd like us to have a plan to eventually get rid of
>>> it.  Monkey patching CPU classes makes querying of CPU model
>>> information less predictable and subtly dependent on QEMU
>>> initialization state.
>>
>>
>> The question of QEMU initialization state and the querying of supported functionality, also in relationship with the loadable modules, is I think a larger discussion.
>>
>> Regardless of the amount of glue code and lipstick, this is hiding the fact that the fundamentals of the object hierarchy for cpus are wrong,
>> and are (unfortunately) codified as part of the external interface.
> 
> That's probably right, and removal of monkey patching might force
> us to change our external interfaces.
> 
>>
>>
>>>
>>> Removing CPUClass.accel_cpu_interface may be easy, because it
>>> should be possible to just call current_accel() when realizing
>>> CPUs.  Getting rid of CPUClass.cpu_class_init might be more
>>> difficult, depending on what the ->cpu_class_init() function is
>>> doing.
>>
>>
>> This seems to be for a next step to me.
> 
> Agreed, although I'd like to understand what makes
> AccelCPUClass.cpu_class_init() so important in the first version
> (considering that existing x86_cpu_class_init() has zero
> tcg_enabled() calls today).
> 

currently x86_cpu_common_class_init() has

#ifdef CONFIG_TCG

and

#ifdef CONFIG_USER_ONLY

I move this code to a tcg specific module,
and I also move the parts that should have been CONFIG_TCG before but were probably just missed.

Ciao,

Claudio

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

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

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

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


>>
>> This last option seems kinda attractive, but any ideas?

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
> 
> 
>>>
>>> This last option seems kinda attractive, but any ideas?
> 
> 

Oh I see, sadly, only user mode code seem to be guaranteed to go through cpu_create(), so there is probably no single code point,
where we are guaranteed to see the creation of a cpu, everything is duplicated with explict calls to object_new in multiple places.

Hmm...

Ciao,

Claudio

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
>>
>>
>>>>
>>>> This last option seems kinda attractive, but any ideas?
>>
>>
> 
> Oh I see, sadly, only user mode code seem to be guaranteed to go through cpu_create(), so there is probably no single code point,
> where we are guaranteed to see the creation of a cpu, everything is duplicated with explict calls to object_new in multiple places.
> 
> Hmm...

Well we can actually do it in the right place, that is in cpu_common_intfn,
by calling accel_init_cpu_intefaces there, which kinda makes sense anyway... wdyt?

Ciao,

CLaudio

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
>>>
>>>
>>>>>
>>>>> This last option seems kinda attractive, but any ideas?
>>>
>>>
>>
>> Oh I see, sadly, only user mode code seem to be guaranteed to go through cpu_create(), so there is probably no single code point,
>> where we are guaranteed to see the creation of a cpu, everything is duplicated with explict calls to object_new in multiple places.
>>
>> Hmm...
> 
> Well we can actually do it in the right place, that is in cpu_common_intfn,
> by calling accel_init_cpu_intefaces there, which kinda makes sense anyway... wdyt?
> 

But then the mapping becomes difficult.. actually I think we/I need to study carefully all targets to figure out the best way to associate a cpu subclass/model with its accel cpu interface.
it's going to be next year now.

Happy holidays,

Claudio

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

On 1/11/21 5:13 PM, Claudio Fontana wrote:
> 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?
>

Or is the hierarchy magically fixing this for us (ie, we rely on the class initializers being called again when the cpu we are actually going to be used is going to be created?)
That seems the likely solution right?

Thanks,

Claudio

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.

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

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