All of lore.kernel.org
 help / color / mirror / Atom feed
* all class init functions for all types in QEMU are called in select_machine(). Expected?
@ 2021-03-12  9:31 Claudio Fontana
  2021-03-12  9:45 ` Philippe Mathieu-Daudé
  2021-03-12  9:46 ` Paolo Bonzini
  0 siblings, 2 replies; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12  9:31 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Eduardo Habkost, qemu-devel

Hello Paolo and all,

while debugging a class init ordering issue, I noticed that

_all_ class init functions for all types registered in the QEMU QOM are called in select_machine().
Expected?

In particular it happens here:

static MachineClass *select_machine(void)
{
    GSList *machines = object_class_get_list(TYPE_MACHINE, false);


object_class_get_list() ->
  object_class_foreach() ->
    g_hash_table_foreach() ->
      object_class_foreach_tramp ->
        type_initialize(type);


Is this really desired? It looks suspect to me.

(gdb) bt
#0  0x0000555555db613f in arm_v7m_class_init (oc=0x555556dca320, data=0x555556a926e0 <arm_tcg_cpus+288>)
    at ../target/arm/tcg/tcg-cpu-models.c:849
#1  0x0000555555f1deba in type_initialize (ti=0x555556d5b2f0) at ../qom/object.c:364
#2  0x0000555555f1f62a in object_class_foreach_tramp (key=0x555556d5b470, value=0x555556d5b2f0, opaque=0x7fffffffda20)
    at ../qom/object.c:1069
#3  0x00007ffff6562000 in g_hash_table_foreach () at /usr/lib64/libglib-2.0.so.0
#4  0x0000555555f1f709 in object_class_foreach (fn=
    0x555555f1f866 <object_class_get_list_tramp>, implements_type=0x555556381b09 "machine", include_abstract=false, opaque=0x7fffffffda70)
    at ../qom/object.c:1091
#5  0x0000555555f1f8e4 in object_class_get_list (implements_type=0x555556381b09 "machine", include_abstract=false) at ../qom/object.c:1148
#6  0x0000555555debe94 in select_machine () at ../softmmu/vl.c:1607


If not here, where should be the right place, for example, for CPU class inits to be called?

At the very least I would put a comment there around the beginning of select_machine() saying:

/* all types, all classes in QOM are initialized here, as a result of the object_class_get_list call */

Wdyt?


Ciao,

Claudio



-- 
Claudio Fontana
Engineering Manager Virtualization, SUSE Labs Core

SUSE Software Solutions Italy Srl


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12  9:31 all class init functions for all types in QEMU are called in select_machine(). Expected? Claudio Fontana
@ 2021-03-12  9:45 ` Philippe Mathieu-Daudé
  2021-03-12  9:58   ` Claudio Fontana
  2021-03-12  9:46 ` Paolo Bonzini
  1 sibling, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-12  9:45 UTC (permalink / raw)
  To: Claudio Fontana, Paolo Bonzini; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 10:31 AM, Claudio Fontana wrote:
> Hello Paolo and all,
> 
> while debugging a class init ordering issue, I noticed that
> 
> _all_ class init functions for all types registered in the QEMU QOM are called in select_machine().
> Expected?
> 
> In particular it happens here:
> 
> static MachineClass *select_machine(void)
> {
>     GSList *machines = object_class_get_list(TYPE_MACHINE, false);
> 
> 
> object_class_get_list() ->
>   object_class_foreach() ->
>     g_hash_table_foreach() ->
>       object_class_foreach_tramp ->
>         type_initialize(type);
> 
> 
> Is this really desired? It looks suspect to me.
> 
> (gdb) bt
> #0  0x0000555555db613f in arm_v7m_class_init (oc=0x555556dca320, data=0x555556a926e0 <arm_tcg_cpus+288>)
>     at ../target/arm/tcg/tcg-cpu-models.c:849
> #1  0x0000555555f1deba in type_initialize (ti=0x555556d5b2f0) at ../qom/object.c:364
> #2  0x0000555555f1f62a in object_class_foreach_tramp (key=0x555556d5b470, value=0x555556d5b2f0, opaque=0x7fffffffda20)
>     at ../qom/object.c:1069
> #3  0x00007ffff6562000 in g_hash_table_foreach () at /usr/lib64/libglib-2.0.so.0
> #4  0x0000555555f1f709 in object_class_foreach (fn=
>     0x555555f1f866 <object_class_get_list_tramp>, implements_type=0x555556381b09 "machine", include_abstract=false, opaque=0x7fffffffda70)
>     at ../qom/object.c:1091
> #5  0x0000555555f1f8e4 in object_class_get_list (implements_type=0x555556381b09 "machine", include_abstract=false) at ../qom/object.c:1148
> #6  0x0000555555debe94 in select_machine () at ../softmmu/vl.c:1607
> 
> 
> If not here, where should be the right place, for example, for CPU class inits to be called?
> 
> At the very least I would put a comment there around the beginning of select_machine() saying:
> 
> /* all types, all classes in QOM are initialized here, as a result of the object_class_get_list call */
> 
> Wdyt?

Are you trying to register types conditionally?



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12  9:31 all class init functions for all types in QEMU are called in select_machine(). Expected? Claudio Fontana
  2021-03-12  9:45 ` Philippe Mathieu-Daudé
@ 2021-03-12  9:46 ` Paolo Bonzini
  2021-03-12  9:49   ` Claudio Fontana
  1 sibling, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-03-12  9:46 UTC (permalink / raw)
  To: Claudio Fontana; +Cc: Eduardo Habkost, qemu-devel

On 12/03/21 10:31, Claudio Fontana wrote:
> Hello Paolo and all,
> 
> while debugging a class init ordering issue, I noticed that
> 
> _all_ class init functions for all types registered in the QEMU QOM are called in select_machine().
> Expected?
> 
> In particular it happens here:
> 
> static MachineClass *select_machine(void)
> {
>      GSList *machines = object_class_get_list(TYPE_MACHINE, false);
> 
> 
> object_class_get_list() ->
>    object_class_foreach() ->
>      g_hash_table_foreach() ->
>        object_class_foreach_tramp ->
>          type_initialize(type);
> 
> Is this really desired? It looks suspect to me.

It is not a problem because class_init should be idempotent.  Changing 
QEMU to not do this would not be impossible, but most likely not worth 
the effort.  To do this, I think one would have to reimplement all of 
object_class_dynamic_cast to operate on TypeInfos (so for example walk 
all interfaces in the type info instead of using class->interfaces).

> If not here, where should be the right place, for example, for CPU class inits to be called?

The first time they're used, upon a call to one of object_new, 
object_initialize, object_class_get_list or object_class_foreach.

> At the very least I would put a comment there around the beginning of select_machine() saying:
> 
> /* all types, all classes in QOM are initialized here, as a result of the object_class_get_list call */

No, it's just a side effect that is not (or should not) be visible.

Paolo



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12  9:46 ` Paolo Bonzini
@ 2021-03-12  9:49   ` Claudio Fontana
  0 siblings, 0 replies; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12  9:49 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 10:46 AM, Paolo Bonzini wrote:
> On 12/03/21 10:31, Claudio Fontana wrote:
>> Hello Paolo and all,
>>
>> while debugging a class init ordering issue, I noticed that
>>
>> _all_ class init functions for all types registered in the QEMU QOM are called in select_machine().
>> Expected?
>>
>> In particular it happens here:
>>
>> static MachineClass *select_machine(void)
>> {
>>      GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>
>>
>> object_class_get_list() ->
>>    object_class_foreach() ->
>>      g_hash_table_foreach() ->
>>        object_class_foreach_tramp ->
>>          type_initialize(type);
>>
>> Is this really desired? It looks suspect to me.
> 
> It is not a problem because class_init should be idempotent.  Changing 
> QEMU to not do this would not be impossible, but most likely not worth 
> the effort.  To do this, I think one would have to reimplement all of 
> object_class_dynamic_cast to operate on TypeInfos (so for example walk 
> all interfaces in the type info instead of using class->interfaces).
> 
>> If not here, where should be the right place, for example, for CPU class inits to be called?
> 
> The first time they're used, upon a call to one of object_new, 
> object_initialize, object_class_get_list or object_class_foreach.


Right, this is what I would have expected. Instead everything known to QOM at that time (there is more stuff added later on),
is initialized right there.

> 
>> At the very least I would put a comment there around the beginning of select_machine() saying:
>>
>> /* all types, all classes in QOM are initialized here, as a result of the object_class_get_list call */
> 
> No, it's just a side effect that is not (or should not) be visible.
> 
> Paolo
> 

Well it's an interesting side effect to me, seems useful to know (add a comment at least) when debugging.
Can be found out quickly, but why hide it..

Ciao,

Claudio


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12  9:45 ` Philippe Mathieu-Daudé
@ 2021-03-12  9:58   ` Claudio Fontana
  2021-03-12 10:07     ` Paolo Bonzini
  0 siblings, 1 reply; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12  9:58 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Paolo Bonzini; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 10:45 AM, Philippe Mathieu-Daudé wrote:
> On 3/12/21 10:31 AM, Claudio Fontana wrote:
>> Hello Paolo and all,
>>
>> while debugging a class init ordering issue, I noticed that
>>
>> _all_ class init functions for all types registered in the QEMU QOM are called in select_machine().
>> Expected?
>>
>> In particular it happens here:
>>
>> static MachineClass *select_machine(void)
>> {
>>     GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>
>>
>> object_class_get_list() ->
>>   object_class_foreach() ->
>>     g_hash_table_foreach() ->
>>       object_class_foreach_tramp ->
>>         type_initialize(type);
>>
>>
>> Is this really desired? It looks suspect to me.
>>
>> (gdb) bt
>> #0  0x0000555555db613f in arm_v7m_class_init (oc=0x555556dca320, data=0x555556a926e0 <arm_tcg_cpus+288>)
>>     at ../target/arm/tcg/tcg-cpu-models.c:849
>> #1  0x0000555555f1deba in type_initialize (ti=0x555556d5b2f0) at ../qom/object.c:364
>> #2  0x0000555555f1f62a in object_class_foreach_tramp (key=0x555556d5b470, value=0x555556d5b2f0, opaque=0x7fffffffda20)
>>     at ../qom/object.c:1069
>> #3  0x00007ffff6562000 in g_hash_table_foreach () at /usr/lib64/libglib-2.0.so.0
>> #4  0x0000555555f1f709 in object_class_foreach (fn=
>>     0x555555f1f866 <object_class_get_list_tramp>, implements_type=0x555556381b09 "machine", include_abstract=false, opaque=0x7fffffffda70)
>>     at ../qom/object.c:1091
>> #5  0x0000555555f1f8e4 in object_class_get_list (implements_type=0x555556381b09 "machine", include_abstract=false) at ../qom/object.c:1148
>> #6  0x0000555555debe94 in select_machine () at ../softmmu/vl.c:1607
>>
>>
>> If not here, where should be the right place, for example, for CPU class inits to be called?
>>
>> At the very least I would put a comment there around the beginning of select_machine() saying:
>>
>> /* all types, all classes in QOM are initialized here, as a result of the object_class_get_list call */
>>
>> Wdyt?
> 
> Are you trying to register types conditionally?
> 

Not really, but I have been using the accel class init function on x86 to register the TCG OPS,

and this instead requires a bit more thought for ARM,

because we currently register for the ARM M Profile the TCG Ops at arm_v7m_class_init time,
which is called already at select_machine() time,

so when we select the accelerator, and we call the tcg_cpu_class_init, we run the risk of overriding the existing tcg_ops,
with the current result that we have to do:

static void tcg_cpu_class_init(CPUClass *cc)
{
    /*                                                                                                                                      
     * do not overwrite the TCG ops, if already set by the                                                                                  
     * arm cpu class (this is the case for the M profile CPUs).                                                                             
     *                                                                                                                                      
     * Otherwise, provide the default ARM TCG behavior here.                                                                                
     */
    if (!cc->tcg_ops) {
        cc->tcg_ops = &arm_tcg_ops;
    }
}

It's a fine result for me, but we do have an "if" inside a class_init.

Ideas? Looks horrible?

THanks,

Claudio


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12  9:58   ` Claudio Fontana
@ 2021-03-12 10:07     ` Paolo Bonzini
  2021-03-12 10:25       ` Claudio Fontana
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-03-12 10:07 UTC (permalink / raw)
  To: Claudio Fontana, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 12/03/21 10:58, Claudio Fontana wrote:
> Not really, but I have been using the accel class init function on x86 to register the TCG OPS,
> 
> and this instead requires a bit more thought for ARM,
> 
> because we currently register for the ARM M Profile the TCG Ops at arm_v7m_class_init time,
> which is called already at select_machine() time,
> 
> so when we select the accelerator, and we call the tcg_cpu_class_init, we run the risk of overriding the existing tcg_ops
>
> Ideas? Looks horrible?

Not horrible, but wrong.  The class_init function must be idempotent: 
classes have no side effect until they're instantiated (and even then, 
usually we delay that to later, e.g. realized for devices or complete 
for user-creatable objects).

Why can't you register ops in the machine init function for the accelerator?

Paolo



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 10:07     ` Paolo Bonzini
@ 2021-03-12 10:25       ` Claudio Fontana
  2021-03-12 10:39         ` Paolo Bonzini
  0 siblings, 1 reply; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12 10:25 UTC (permalink / raw)
  To: Paolo Bonzini, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 11:07 AM, Paolo Bonzini wrote:
> On 12/03/21 10:58, Claudio Fontana wrote:
>> Not really, but I have been using the accel class init function on x86 to register the TCG OPS,
>>
>> and this instead requires a bit more thought for ARM,
>>
>> because we currently register for the ARM M Profile the TCG Ops at arm_v7m_class_init time,
>> which is called already at select_machine() time,
>>
>> so when we select the accelerator, and we call the tcg_cpu_class_init, we run the risk of overriding the existing tcg_ops
>>
>> Ideas? Looks horrible?
> 
> Not horrible, but wrong.  The class_init function must be idempotent: 
> classes have no side effect until they're instantiated (and even then, 
> usually we delay that to later, e.g. realized for devices or complete 
> for user-creatable objects).
> 
> Why can't you register ops in the machine init function for the accelerator?
> 
> Paolo
> 

We can register them from there if needed I think,,
but where ever we do it, we still have to point to the one set of ops or the other, depending on the cpu model that is finally chosen.

So when we attach the tcg cpu accelerator object to the cpu, we currently should check which cpu it is, and behave accordingly.

Maybe this is better? Ie, not set the tcg ops in different places (in the v7m cpu class init and in the tcg cpu accel init),

but rather set them just in a single place, when we attach the accel cpu object,
checking which cpu profile it is somehow (TBD), and then behave accordingly?

THanks,

Claudio







^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 10:25       ` Claudio Fontana
@ 2021-03-12 10:39         ` Paolo Bonzini
  2021-03-12 11:51           ` Claudio Fontana
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-03-12 10:39 UTC (permalink / raw)
  To: Claudio Fontana, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 12/03/21 11:25, Claudio Fontana wrote:
> We can register them from there if needed I think,, but where ever we
> do it, we still have to point to the one set of ops or the other,
> depending on the cpu model that is finally chosen.
> 
> So when we attach the tcg cpu accelerator object to the cpu, we
> currently should check which cpu it is, and behave accordingly.
> 
> Maybe this is better? Ie, not set the tcg ops in different places (in
> the v7m cpu class init and in the tcg cpu accel init),
> 
> but rather set them just in a single place, when we attach the accel
> cpu object, checking which cpu profile it is somehow (TBD), and then
> behave accordingly?

Take a look at 
https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence#Basic_phases. 
  The phases are:

- creating backends (PHASE_NO_MACHINE)

- creating machine (after which PHASE_MACHINE_CREATED is entered)

- creating accelerator (after which PHASE_ACCEL_CREATED is entered)

- initializing embedded devices (in machine_run_board_init, after which 
PHASE_MACHINE_INITIALIZED is entered), including CPUs

- creating devices (in qmp_x_exit_preconfig, after which 
PHASE_MACHINE_READY is entered)

And the last is where the guest actually starts.

I think that you should have a callback in the accelerator that runs 
after -cpu is processed and before PHASE_MACHINE_INITIALIZED is entered. 
  So the right place to add it would be machine_run_board_init.

Maybe some kind of double dispatch, where the accelerator has an 
acc->init_cpu(acc, cc) method and the CPU has a cc->init_tcg_ops(cc) 
method.  Then TCG's init_cpu calls into the latter.

Paolo



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 10:39         ` Paolo Bonzini
@ 2021-03-12 11:51           ` Claudio Fontana
  2021-03-12 12:02             ` Paolo Bonzini
  0 siblings, 1 reply; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12 11:51 UTC (permalink / raw)
  To: Paolo Bonzini, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 11:39 AM, Paolo Bonzini wrote:
> On 12/03/21 11:25, Claudio Fontana wrote:
>> We can register them from there if needed I think,, but where ever we
>> do it, we still have to point to the one set of ops or the other,
>> depending on the cpu model that is finally chosen.
>>
>> So when we attach the tcg cpu accelerator object to the cpu, we
>> currently should check which cpu it is, and behave accordingly.
>>
>> Maybe this is better? Ie, not set the tcg ops in different places (in
>> the v7m cpu class init and in the tcg cpu accel init),
>>
>> but rather set them just in a single place, when we attach the accel
>> cpu object, checking which cpu profile it is somehow (TBD), and then
>> behave accordingly?
> 
> Take a look at 
> https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence#Basic_phases. 
>   The phases are:
> 
> - creating backends (PHASE_NO_MACHINE)
> 
> - creating machine (after which PHASE_MACHINE_CREATED is entered)
> 
> - creating accelerator (after which PHASE_ACCEL_CREATED is entered)
> 
> - initializing embedded devices (in machine_run_board_init, after which 
> PHASE_MACHINE_INITIALIZED is entered), including CPUs
> 
> - creating devices (in qmp_x_exit_preconfig, after which 
> PHASE_MACHINE_READY is entered)
> 
> And the last is where the guest actually starts.
> 
> I think that you should have a callback in the accelerator that runs 
> after -cpu is processed and before PHASE_MACHINE_INITIALIZED is entered. 
>   So the right place to add it would be machine_run_board_init.
> 
> Maybe some kind of double dispatch, where the accelerator has an 
> acc->init_cpu(acc, cc) method and the CPU has a cc->init_tcg_ops(cc) 
> method.  Then TCG's init_cpu calls into the latter.
> 
> Paolo
> 
> 

Thanks, digesting this.

What you describe as:

acc->init_cpu(acc, cc)

seems to me we already have, as the accel class init, fe, for x86/tcg:

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

#ifndef CONFIG_USER_ONLY
    acc->cpu_realizefn = tcg_cpu_realizefn;
#endif /* CONFIG_USER_ONLY */

    acc->cpu_class_init = tcg_cpu_class_init;
    acc->cpu_instance_init = tcg_cpu_instance_init;
}

acc->cpu_class_init() call would then be the acc->init_cpu call you mention.

The only thing we seem to be missing is the cc->init_tcg_ops(cc)..

Ciao,

Claudio










^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 11:51           ` Claudio Fontana
@ 2021-03-12 12:02             ` Paolo Bonzini
  2021-03-12 13:40               ` Claudio Fontana
  0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-03-12 12:02 UTC (permalink / raw)
  To: Claudio Fontana, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 12/03/21 12:51, Claudio Fontana wrote:
> seems to me we already have, as the accel class init, fe, for x86/tcg:
> 
> static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
> {
>      AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
> 
> #ifndef CONFIG_USER_ONLY
>      acc->cpu_realizefn = tcg_cpu_realizefn;
> #endif /* CONFIG_USER_ONLY */
> 
>      acc->cpu_class_init = tcg_cpu_class_init;
>      acc->cpu_instance_init = tcg_cpu_instance_init;
> }
> 
> acc->cpu_class_init() call would then be the acc->init_cpu call you mention.
> 
> The only thing we seem to be missing is the cc->init_tcg_ops(cc)..

Yes, called by tcg_cpu_class_init or tcg_cpu_instance_init.

Paolo



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 12:02             ` Paolo Bonzini
@ 2021-03-12 13:40               ` Claudio Fontana
  2021-03-12 14:00                 ` Claudio Fontana
  2021-03-12 17:04                 ` Paolo Bonzini
  0 siblings, 2 replies; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12 13:40 UTC (permalink / raw)
  To: Paolo Bonzini, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 1:02 PM, Paolo Bonzini wrote:
> On 12/03/21 12:51, Claudio Fontana wrote:
>> seems to me we already have, as the accel class init, fe, for x86/tcg:
>>
>> static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
>> {
>>      AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
>>
>> #ifndef CONFIG_USER_ONLY
>>      acc->cpu_realizefn = tcg_cpu_realizefn;
>> #endif /* CONFIG_USER_ONLY */
>>
>>      acc->cpu_class_init = tcg_cpu_class_init;
>>      acc->cpu_instance_init = tcg_cpu_instance_init;
>> }
>>
>> acc->cpu_class_init() call would then be the acc->init_cpu call you mention.
>>
>> The only thing we seem to be missing is the cc->init_tcg_ops(cc)..
> 
> Yes, called by tcg_cpu_class_init or tcg_cpu_instance_init.
> 
> Paolo
> 
> 

.. I wonder if we should make it a bit more general like:

static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
{
    CPUClass *cc = CPU_CLASS(klass);
    AccelCPUClass *accel_cpu = opaque;

    /*                                                                                                                                      
     * double dispatch. The first callback allows the accel cpu                                                                             
     * to run initializations for the CPU,                                                                                                  
     * the second one allows the CPU to customize the accel cpu                                                                             
     * behavior according to the CPU.                                                                                                       
     *                                                                                                                                      
     * The second is currently only used by TCG, to specialize the                                                                          
     * TCGCPUOps depending on the CPU type.                                                                                                 
     */
    cc->accel_cpu = accel_cpu;
    if (accel_cpu->cpu_class_init) {
        accel_cpu->cpu_class_init(cc);
    }
    if (cc->init_accel_cpu) {
        cc->init_accel_cpu(accel_cpu, cc);
    }
}

.. but maybe this is premature, and should wait for actual users of this beyond TCG on ARM?

Ciao,

C




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 13:40               ` Claudio Fontana
@ 2021-03-12 14:00                 ` Claudio Fontana
  2021-03-12 17:04                 ` Paolo Bonzini
  1 sibling, 0 replies; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12 14:00 UTC (permalink / raw)
  To: Paolo Bonzini, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 2:40 PM, Claudio Fontana wrote:
> On 3/12/21 1:02 PM, Paolo Bonzini wrote:
>> On 12/03/21 12:51, Claudio Fontana wrote:
>>> seems to me we already have, as the accel class init, fe, for x86/tcg:
>>>
>>> static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
>>> {
>>>      AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
>>>
>>> #ifndef CONFIG_USER_ONLY
>>>      acc->cpu_realizefn = tcg_cpu_realizefn;
>>> #endif /* CONFIG_USER_ONLY */
>>>
>>>      acc->cpu_class_init = tcg_cpu_class_init;
>>>      acc->cpu_instance_init = tcg_cpu_instance_init;
>>> }
>>>
>>> acc->cpu_class_init() call would then be the acc->init_cpu call you mention.
>>>
>>> The only thing we seem to be missing is the cc->init_tcg_ops(cc)..
>>
>> Yes, called by tcg_cpu_class_init or tcg_cpu_instance_init.
>>
>> Paolo
>>
>>
> 
> .. I wonder if we should make it a bit more general like:
> 
> static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
> {
>     CPUClass *cc = CPU_CLASS(klass);
>     AccelCPUClass *accel_cpu = opaque;
> 
>     /*                                                                                                                                      
>      * double dispatch. The first callback allows the accel cpu                                                                             
>      * to run initializations for the CPU,                                                                                                  
>      * the second one allows the CPU to customize the accel cpu                                                                             
>      * behavior according to the CPU.                                                                                                       
>      *                                                                                                                                      
>      * The second is currently only used by TCG, to specialize the                                                                          
>      * TCGCPUOps depending on the CPU type.                                                                                                 
>      */
>     cc->accel_cpu = accel_cpu;
>     if (accel_cpu->cpu_class_init) {
>         accel_cpu->cpu_class_init(cc);
>     }
>     if (cc->init_accel_cpu) {
>         cc->init_accel_cpu(accel_cpu, cc);
>     }
> }
> 
> .. but maybe this is premature, and should wait for actual users of this beyond TCG on ARM?
> 
> Ciao,
> 
> C
> 

I'll put my experiment in the ARM cleanup series.

Ciao and thanks,

Claudio



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 13:40               ` Claudio Fontana
  2021-03-12 14:00                 ` Claudio Fontana
@ 2021-03-12 17:04                 ` Paolo Bonzini
  2021-03-12 17:24                   ` Claudio Fontana
  1 sibling, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-03-12 17:04 UTC (permalink / raw)
  To: Claudio Fontana, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 12/03/21 14:40, Claudio Fontana wrote:
>      /*
>       * double dispatch. The first callback allows the accel cpu
>       * to run initializations for the CPU,
>       * the second one allows the CPU to customize the accel cpu
>       * behavior according to the CPU.
>       *
>       * The second is currently only used by TCG, to specialize the
>       * TCGCPUOps depending on the CPU type.
>       */
>      cc->accel_cpu = accel_cpu;
>      if (accel_cpu->cpu_class_init) {
>          accel_cpu->cpu_class_init(cc);
>      }
>      if (cc->init_accel_cpu) {
>          cc->init_accel_cpu(accel_cpu, cc);
>      }
> }
> 
> .. but maybe this is premature, and should wait for actual users of this beyond TCG on ARM?

I prefer to single out TCG and have the call in cpu_class_init.  The 
idea of double dispatch (as opposed to an if/else chain with checks on 
the class of the argument) is that the first caller uses different 
"method names" to tell its type name to the target.

See for example 
https://en.wikipedia.org/wiki/Double_dispatch#Example_in_Ruby.

Paolo



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 17:04                 ` Paolo Bonzini
@ 2021-03-12 17:24                   ` Claudio Fontana
  2021-03-12 17:30                     ` Paolo Bonzini
  0 siblings, 1 reply; 15+ messages in thread
From: Claudio Fontana @ 2021-03-12 17:24 UTC (permalink / raw)
  To: Paolo Bonzini, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 3/12/21 6:04 PM, Paolo Bonzini wrote:
> On 12/03/21 14:40, Claudio Fontana wrote:
>>      /*
>>       * double dispatch. The first callback allows the accel cpu
>>       * to run initializations for the CPU,
>>       * the second one allows the CPU to customize the accel cpu
>>       * behavior according to the CPU.
>>       *
>>       * The second is currently only used by TCG, to specialize the
>>       * TCGCPUOps depending on the CPU type.
>>       */
>>      cc->accel_cpu = accel_cpu;
>>      if (accel_cpu->cpu_class_init) {
>>          accel_cpu->cpu_class_init(cc);
>>      }
>>      if (cc->init_accel_cpu) {
>>          cc->init_accel_cpu(accel_cpu, cc);
>>      }
>> }
>>
>> .. but maybe this is premature, and should wait for actual users of this beyond TCG on ARM?
> 
> I prefer to single out TCG and have the call in cpu_class_init.  The 
> idea of double dispatch (as opposed to an if/else chain with checks on 
> the class of the argument) is that the first caller uses different 
> "method names" to tell its type name to the target.
> 
> See for example 
> https://en.wikipedia.org/wiki/Double_dispatch#Example_in_Ruby.
> 
> Paolo
> 

Ah, just saw this. I already sent the series, but we can rework and rethink this.

Ciao, thanks,

Claudio


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: all class init functions for all types in QEMU are called in select_machine(). Expected?
  2021-03-12 17:24                   ` Claudio Fontana
@ 2021-03-12 17:30                     ` Paolo Bonzini
  0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2021-03-12 17:30 UTC (permalink / raw)
  To: Claudio Fontana, Philippe Mathieu-Daudé; +Cc: Eduardo Habkost, qemu-devel

On 12/03/21 18:24, Claudio Fontana wrote:
> Ah, just saw this. I already sent the series, but we can rework and rethink this.

No big deal; since you have the assertion it's okay as you posted, too.

Paolo



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-03-12 18:32 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12  9:31 all class init functions for all types in QEMU are called in select_machine(). Expected? Claudio Fontana
2021-03-12  9:45 ` Philippe Mathieu-Daudé
2021-03-12  9:58   ` Claudio Fontana
2021-03-12 10:07     ` Paolo Bonzini
2021-03-12 10:25       ` Claudio Fontana
2021-03-12 10:39         ` Paolo Bonzini
2021-03-12 11:51           ` Claudio Fontana
2021-03-12 12:02             ` Paolo Bonzini
2021-03-12 13:40               ` Claudio Fontana
2021-03-12 14:00                 ` Claudio Fontana
2021-03-12 17:04                 ` Paolo Bonzini
2021-03-12 17:24                   ` Claudio Fontana
2021-03-12 17:30                     ` Paolo Bonzini
2021-03-12  9:46 ` Paolo Bonzini
2021-03-12  9:49   ` Claudio Fontana

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.