diff mbox series

[RFC,v7,12/22] cpu: Introduce TCGCpuOperations struct

Message ID 20201130023535.16689-13-cfontana@suse.de
State New
Headers show
Series i386 cleanup | expand

Commit Message

Claudio Fontana Nov. 30, 2020, 2:35 a.m. UTC
From: Eduardo Habkost <ehabkost@redhat.com>

The TCG-specific CPU methods will be moved to a separate struct,
to make it easier to move accel-specific code outside generic CPU
code in the future.  Start by moving tcg_initialize().

The new CPUClass.tcg_opts field may eventually become a pointer,
but keep it an embedded struct for now, to make code conversion
easier.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 MAINTAINERS                     |  1 +
 cpu.c                           |  2 +-
 include/hw/core/cpu.h           |  9 ++++++++-
 include/hw/core/tcg-cpu-ops.h   | 25 +++++++++++++++++++++++++
 target/alpha/cpu.c              |  2 +-
 target/arm/cpu.c                |  2 +-
 target/avr/cpu.c                |  2 +-
 target/cris/cpu.c               | 12 ++++++------
 target/hppa/cpu.c               |  2 +-
 target/i386/tcg-cpu.c           |  2 +-
 target/lm32/cpu.c               |  2 +-
 target/m68k/cpu.c               |  2 +-
 target/microblaze/cpu.c         |  2 +-
 target/mips/cpu.c               |  2 +-
 target/moxie/cpu.c              |  2 +-
 target/nios2/cpu.c              |  2 +-
 target/openrisc/cpu.c           |  2 +-
 target/ppc/translate_init.c.inc |  2 +-
 target/riscv/cpu.c              |  2 +-
 target/rx/cpu.c                 |  2 +-
 target/s390x/cpu.c              |  2 +-
 target/sh4/cpu.c                |  2 +-
 target/sparc/cpu.c              |  2 +-
 target/tilegx/cpu.c             |  2 +-
 target/tricore/cpu.c            |  2 +-
 target/unicore32/cpu.c          |  2 +-
 target/xtensa/cpu.c             |  2 +-
 27 files changed, 63 insertions(+), 30 deletions(-)
 create mode 100644 include/hw/core/tcg-cpu-ops.h

Comments

Philippe Mathieu-Daudé Dec. 4, 2020, 5:10 p.m. UTC | #1
On 11/30/20 3:35 AM, Claudio Fontana wrote:
> From: Eduardo Habkost <ehabkost@redhat.com>
> 
> The TCG-specific CPU methods will be moved to a separate struct,
> to make it easier to move accel-specific code outside generic CPU
> code in the future.  Start by moving tcg_initialize().

Good idea! One minor comment below.

> 
> The new CPUClass.tcg_opts field may eventually become a pointer,
> but keep it an embedded struct for now, to make code conversion
> easier.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  MAINTAINERS                     |  1 +
>  cpu.c                           |  2 +-
>  include/hw/core/cpu.h           |  9 ++++++++-
>  include/hw/core/tcg-cpu-ops.h   | 25 +++++++++++++++++++++++++
>  target/alpha/cpu.c              |  2 +-
>  target/arm/cpu.c                |  2 +-
>  target/avr/cpu.c                |  2 +-
>  target/cris/cpu.c               | 12 ++++++------
>  target/hppa/cpu.c               |  2 +-
>  target/i386/tcg-cpu.c           |  2 +-
>  target/lm32/cpu.c               |  2 +-
>  target/m68k/cpu.c               |  2 +-
>  target/microblaze/cpu.c         |  2 +-
>  target/mips/cpu.c               |  2 +-
>  target/moxie/cpu.c              |  2 +-
>  target/nios2/cpu.c              |  2 +-
>  target/openrisc/cpu.c           |  2 +-
>  target/ppc/translate_init.c.inc |  2 +-
>  target/riscv/cpu.c              |  2 +-
>  target/rx/cpu.c                 |  2 +-
>  target/s390x/cpu.c              |  2 +-
>  target/sh4/cpu.c                |  2 +-
>  target/sparc/cpu.c              |  2 +-
>  target/tilegx/cpu.c             |  2 +-
>  target/tricore/cpu.c            |  2 +-
>  target/unicore32/cpu.c          |  2 +-
>  target/xtensa/cpu.c             |  2 +-
>  27 files changed, 63 insertions(+), 30 deletions(-)
>  create mode 100644 include/hw/core/tcg-cpu-ops.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f53f2678d8..d876f504a6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1535,6 +1535,7 @@ F: qapi/machine.json
>  F: qapi/machine-target.json
>  F: include/hw/boards.h
>  F: include/hw/core/cpu.h
> +F: include/hw/core/tcg-cpu-ops.h
>  F: include/hw/cpu/cluster.h
>  F: include/sysemu/numa.h
>  T: git https://github.com/ehabkost/qemu.git machine-next
> diff --git a/cpu.c b/cpu.c
> index 0be5dcb6f3..d02c2a17f1 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -180,7 +180,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>  
>      if (tcg_enabled() && !tcg_target_initialized) {
>          tcg_target_initialized = true;
> -        cc->tcg_initialize();
> +        cc->tcg_ops.initialize();
>      }
>      tlb_init(cpu);
>  
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 3d92c967ff..c93b08a0fb 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -76,6 +76,10 @@ typedef struct CPUWatchpoint CPUWatchpoint;
>  
>  struct TranslationBlock;
>  
> +#ifdef CONFIG_TCG
> +#include "tcg-cpu-ops.h"
> +#endif /* CONFIG_TCG */
> +
>  /**
>   * CPUClass:
>   * @class_by_name: Callback to map -cpu command line model name to an
> @@ -221,12 +225,15 @@ struct CPUClass {
>  
>      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
>      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
> -    void (*tcg_initialize)(void);
>  
>      const char *deprecation_note;
>      /* Keep non-pointer data at the end to minimize holes.  */
>      int gdb_num_core_regs;
>      bool gdb_stop_before_watchpoint;
> +
> +#ifdef CONFIG_TCG
> +    TcgCpuOperations tcg_ops;
> +#endif /* CONFIG_TCG */
>  };
>  
>  /*
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> new file mode 100644
> index 0000000000..4475ef0996
> --- /dev/null
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -0,0 +1,25 @@
> +/*
> + * TCG-Specific operations that are not meaningful for hardware accelerators
> + *
> + * Copyright 2020 SUSE LLC
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef TCG_CPU_OPS_H
> +#define TCG_CPU_OPS_H
> +
> +/**
> + * struct TcgCpuOperations: TCG operations specific to a CPU class
> + */
> +typedef struct TcgCpuOperations {
> +    /**
> +     * @initialize: Initalize TCG state
> +     *
> +     * Called when the first CPU is realized.
> +     */
> +    void (*initialize)(void);
> +} TcgCpuOperations;
> +
> +#endif /* TCG_CPU_OPS_H */
...

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 07492e9f9a..1fa9382a7c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2261,7 +2261,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_stop_before_watchpoint = true;
>      cc->disas_set_info = arm_disas_set_info;
>  #ifdef CONFIG_TCG
> -    cc->tcg_initialize = arm_translate_init;
> +    cc->tcg_ops.initialize = arm_translate_init;

This one is correctly guarded by '#ifdef CONFIG_TCG'.

For the other targets, can you either place it within
the '#ifdef CONFIG_TCG' block or if there is none, add
one please?

With that change:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks,

Phil.
Eduardo Habkost Dec. 4, 2020, 5:28 p.m. UTC | #2
On Fri, Dec 04, 2020 at 06:10:49PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/30/20 3:35 AM, Claudio Fontana wrote:
> > From: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > The TCG-specific CPU methods will be moved to a separate struct,
> > to make it easier to move accel-specific code outside generic CPU
> > code in the future.  Start by moving tcg_initialize().
> 
> Good idea! One minor comment below.
> 
> > 
> > The new CPUClass.tcg_opts field may eventually become a pointer,
> > but keep it an embedded struct for now, to make code conversion
> > easier.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  MAINTAINERS                     |  1 +
> >  cpu.c                           |  2 +-
> >  include/hw/core/cpu.h           |  9 ++++++++-
> >  include/hw/core/tcg-cpu-ops.h   | 25 +++++++++++++++++++++++++
> >  target/alpha/cpu.c              |  2 +-
> >  target/arm/cpu.c                |  2 +-
> >  target/avr/cpu.c                |  2 +-
> >  target/cris/cpu.c               | 12 ++++++------
> >  target/hppa/cpu.c               |  2 +-
> >  target/i386/tcg-cpu.c           |  2 +-
> >  target/lm32/cpu.c               |  2 +-
> >  target/m68k/cpu.c               |  2 +-
> >  target/microblaze/cpu.c         |  2 +-
> >  target/mips/cpu.c               |  2 +-
> >  target/moxie/cpu.c              |  2 +-
> >  target/nios2/cpu.c              |  2 +-
> >  target/openrisc/cpu.c           |  2 +-
> >  target/ppc/translate_init.c.inc |  2 +-
> >  target/riscv/cpu.c              |  2 +-
> >  target/rx/cpu.c                 |  2 +-
> >  target/s390x/cpu.c              |  2 +-
> >  target/sh4/cpu.c                |  2 +-
> >  target/sparc/cpu.c              |  2 +-
> >  target/tilegx/cpu.c             |  2 +-
> >  target/tricore/cpu.c            |  2 +-
> >  target/unicore32/cpu.c          |  2 +-
> >  target/xtensa/cpu.c             |  2 +-
> >  27 files changed, 63 insertions(+), 30 deletions(-)
> >  create mode 100644 include/hw/core/tcg-cpu-ops.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f53f2678d8..d876f504a6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1535,6 +1535,7 @@ F: qapi/machine.json
> >  F: qapi/machine-target.json
> >  F: include/hw/boards.h
> >  F: include/hw/core/cpu.h
> > +F: include/hw/core/tcg-cpu-ops.h
> >  F: include/hw/cpu/cluster.h
> >  F: include/sysemu/numa.h
> >  T: git https://github.com/ehabkost/qemu.git machine-next
> > diff --git a/cpu.c b/cpu.c
> > index 0be5dcb6f3..d02c2a17f1 100644
> > --- a/cpu.c
> > +++ b/cpu.c
> > @@ -180,7 +180,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
> >  
> >      if (tcg_enabled() && !tcg_target_initialized) {
> >          tcg_target_initialized = true;
> > -        cc->tcg_initialize();
> > +        cc->tcg_ops.initialize();
> >      }
> >      tlb_init(cpu);
> >  
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index 3d92c967ff..c93b08a0fb 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -76,6 +76,10 @@ typedef struct CPUWatchpoint CPUWatchpoint;
> >  
> >  struct TranslationBlock;
> >  
> > +#ifdef CONFIG_TCG
> > +#include "tcg-cpu-ops.h"
> > +#endif /* CONFIG_TCG */
> > +
> >  /**
> >   * CPUClass:
> >   * @class_by_name: Callback to map -cpu command line model name to an
> > @@ -221,12 +225,15 @@ struct CPUClass {
> >  
> >      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
> >      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
> > -    void (*tcg_initialize)(void);
> >  
> >      const char *deprecation_note;
> >      /* Keep non-pointer data at the end to minimize holes.  */
> >      int gdb_num_core_regs;
> >      bool gdb_stop_before_watchpoint;
> > +
> > +#ifdef CONFIG_TCG
> > +    TcgCpuOperations tcg_ops;
> > +#endif /* CONFIG_TCG */
> >  };

I'm not a fan of #ifdefs in struct definitions (especially in
generic code like hw/cpu), because there's risk the same header
generate different struct layout when used by different .c files.
I would prefer to gradually refactor the code so that tcg_ops is
eventually removed from CPUClass.

This is not a dealbreaker, because both approaches are steps in
the same direction.  But the #ifdef here makes review harder and
has more risks of unwanted side effects.

> >  
> >  /*
> > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> > new file mode 100644
> > index 0000000000..4475ef0996
> > --- /dev/null
> > +++ b/include/hw/core/tcg-cpu-ops.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * TCG-Specific operations that are not meaningful for hardware accelerators
> > + *
> > + * Copyright 2020 SUSE LLC
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef TCG_CPU_OPS_H
> > +#define TCG_CPU_OPS_H
> > +
> > +/**
> > + * struct TcgCpuOperations: TCG operations specific to a CPU class
> > + */
> > +typedef struct TcgCpuOperations {
> > +    /**
> > +     * @initialize: Initalize TCG state
> > +     *
> > +     * Called when the first CPU is realized.
> > +     */
> > +    void (*initialize)(void);
> > +} TcgCpuOperations;
> > +
> > +#endif /* TCG_CPU_OPS_H */
> ...
> 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 07492e9f9a..1fa9382a7c 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -2261,7 +2261,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
> >      cc->gdb_stop_before_watchpoint = true;
> >      cc->disas_set_info = arm_disas_set_info;
> >  #ifdef CONFIG_TCG
> > -    cc->tcg_initialize = arm_translate_init;
> > +    cc->tcg_ops.initialize = arm_translate_init;
> 
> This one is correctly guarded by '#ifdef CONFIG_TCG'.
> 
> For the other targets, can you either place it within
> the '#ifdef CONFIG_TCG' block or if there is none, add
> one please?

As a new #ifdef risks having additional unwanted side effects, I
would prefer to do it in separate patch, just in case.

This also applies to the #ifdef Claudio added to hw/core/cpu.h
above.  In case we really want to do it, I would do it in a
separate patch.

> 
> With that change:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!
Claudio Fontana Dec. 4, 2020, 6:02 p.m. UTC | #3
On 12/4/20 6:10 PM, Philippe Mathieu-Daudé wrote:
> On 11/30/20 3:35 AM, Claudio Fontana wrote:
>> From: Eduardo Habkost <ehabkost@redhat.com>
>>
>> The TCG-specific CPU methods will be moved to a separate struct,
>> to make it easier to move accel-specific code outside generic CPU
>> code in the future.  Start by moving tcg_initialize().
> 
> Good idea! One minor comment below.
> 
>>
>> The new CPUClass.tcg_opts field may eventually become a pointer,
>> but keep it an embedded struct for now, to make code conversion
>> easier.
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  MAINTAINERS                     |  1 +
>>  cpu.c                           |  2 +-
>>  include/hw/core/cpu.h           |  9 ++++++++-
>>  include/hw/core/tcg-cpu-ops.h   | 25 +++++++++++++++++++++++++
>>  target/alpha/cpu.c              |  2 +-
>>  target/arm/cpu.c                |  2 +-
>>  target/avr/cpu.c                |  2 +-
>>  target/cris/cpu.c               | 12 ++++++------
>>  target/hppa/cpu.c               |  2 +-
>>  target/i386/tcg-cpu.c           |  2 +-
>>  target/lm32/cpu.c               |  2 +-
>>  target/m68k/cpu.c               |  2 +-
>>  target/microblaze/cpu.c         |  2 +-
>>  target/mips/cpu.c               |  2 +-
>>  target/moxie/cpu.c              |  2 +-
>>  target/nios2/cpu.c              |  2 +-
>>  target/openrisc/cpu.c           |  2 +-
>>  target/ppc/translate_init.c.inc |  2 +-
>>  target/riscv/cpu.c              |  2 +-
>>  target/rx/cpu.c                 |  2 +-
>>  target/s390x/cpu.c              |  2 +-
>>  target/sh4/cpu.c                |  2 +-
>>  target/sparc/cpu.c              |  2 +-
>>  target/tilegx/cpu.c             |  2 +-
>>  target/tricore/cpu.c            |  2 +-
>>  target/unicore32/cpu.c          |  2 +-
>>  target/xtensa/cpu.c             |  2 +-
>>  27 files changed, 63 insertions(+), 30 deletions(-)
>>  create mode 100644 include/hw/core/tcg-cpu-ops.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f53f2678d8..d876f504a6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1535,6 +1535,7 @@ F: qapi/machine.json
>>  F: qapi/machine-target.json
>>  F: include/hw/boards.h
>>  F: include/hw/core/cpu.h
>> +F: include/hw/core/tcg-cpu-ops.h
>>  F: include/hw/cpu/cluster.h
>>  F: include/sysemu/numa.h
>>  T: git https://github.com/ehabkost/qemu.git machine-next
>> diff --git a/cpu.c b/cpu.c
>> index 0be5dcb6f3..d02c2a17f1 100644
>> --- a/cpu.c
>> +++ b/cpu.c
>> @@ -180,7 +180,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>>  
>>      if (tcg_enabled() && !tcg_target_initialized) {
>>          tcg_target_initialized = true;
>> -        cc->tcg_initialize();
>> +        cc->tcg_ops.initialize();
>>      }
>>      tlb_init(cpu);
>>  
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index 3d92c967ff..c93b08a0fb 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -76,6 +76,10 @@ typedef struct CPUWatchpoint CPUWatchpoint;
>>  
>>  struct TranslationBlock;
>>  
>> +#ifdef CONFIG_TCG
>> +#include "tcg-cpu-ops.h"
>> +#endif /* CONFIG_TCG */
>> +
>>  /**
>>   * CPUClass:
>>   * @class_by_name: Callback to map -cpu command line model name to an
>> @@ -221,12 +225,15 @@ struct CPUClass {
>>  
>>      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
>>      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
>> -    void (*tcg_initialize)(void);
>>  
>>      const char *deprecation_note;
>>      /* Keep non-pointer data at the end to minimize holes.  */
>>      int gdb_num_core_regs;
>>      bool gdb_stop_before_watchpoint;
>> +
>> +#ifdef CONFIG_TCG
>> +    TcgCpuOperations tcg_ops;
>> +#endif /* CONFIG_TCG */
>>  };
>>  
>>  /*
>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
>> new file mode 100644
>> index 0000000000..4475ef0996
>> --- /dev/null
>> +++ b/include/hw/core/tcg-cpu-ops.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * TCG-Specific operations that are not meaningful for hardware accelerators
>> + *
>> + * Copyright 2020 SUSE LLC
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef TCG_CPU_OPS_H
>> +#define TCG_CPU_OPS_H
>> +
>> +/**
>> + * struct TcgCpuOperations: TCG operations specific to a CPU class
>> + */
>> +typedef struct TcgCpuOperations {
>> +    /**
>> +     * @initialize: Initalize TCG state
>> +     *
>> +     * Called when the first CPU is realized.
>> +     */
>> +    void (*initialize)(void);
>> +} TcgCpuOperations;
>> +
>> +#endif /* TCG_CPU_OPS_H */
> ...
> 
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 07492e9f9a..1fa9382a7c 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -2261,7 +2261,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->gdb_stop_before_watchpoint = true;
>>      cc->disas_set_info = arm_disas_set_info;
>>  #ifdef CONFIG_TCG
>> -    cc->tcg_initialize = arm_translate_init;
>> +    cc->tcg_ops.initialize = arm_translate_init;
> 
> This one is correctly guarded by '#ifdef CONFIG_TCG'.
> 
> For the other targets, can you either place it within
> the '#ifdef CONFIG_TCG' block or if there is none, add
> one please?
> 
> With that change:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks,
> 
> Phil.
> 

Ciao,

yes, I am doing that,

Thanks!

Claudio
Claudio Fontana Dec. 4, 2020, 6:04 p.m. UTC | #4
On 12/4/20 6:28 PM, Eduardo Habkost wrote:
> On Fri, Dec 04, 2020 at 06:10:49PM +0100, Philippe Mathieu-Daudé wrote:
>> On 11/30/20 3:35 AM, Claudio Fontana wrote:
>>> From: Eduardo Habkost <ehabkost@redhat.com>
>>>
>>> The TCG-specific CPU methods will be moved to a separate struct,
>>> to make it easier to move accel-specific code outside generic CPU
>>> code in the future.  Start by moving tcg_initialize().
>>
>> Good idea! One minor comment below.
>>
>>>
>>> The new CPUClass.tcg_opts field may eventually become a pointer,
>>> but keep it an embedded struct for now, to make code conversion
>>> easier.
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  MAINTAINERS                     |  1 +
>>>  cpu.c                           |  2 +-
>>>  include/hw/core/cpu.h           |  9 ++++++++-
>>>  include/hw/core/tcg-cpu-ops.h   | 25 +++++++++++++++++++++++++
>>>  target/alpha/cpu.c              |  2 +-
>>>  target/arm/cpu.c                |  2 +-
>>>  target/avr/cpu.c                |  2 +-
>>>  target/cris/cpu.c               | 12 ++++++------
>>>  target/hppa/cpu.c               |  2 +-
>>>  target/i386/tcg-cpu.c           |  2 +-
>>>  target/lm32/cpu.c               |  2 +-
>>>  target/m68k/cpu.c               |  2 +-
>>>  target/microblaze/cpu.c         |  2 +-
>>>  target/mips/cpu.c               |  2 +-
>>>  target/moxie/cpu.c              |  2 +-
>>>  target/nios2/cpu.c              |  2 +-
>>>  target/openrisc/cpu.c           |  2 +-
>>>  target/ppc/translate_init.c.inc |  2 +-
>>>  target/riscv/cpu.c              |  2 +-
>>>  target/rx/cpu.c                 |  2 +-
>>>  target/s390x/cpu.c              |  2 +-
>>>  target/sh4/cpu.c                |  2 +-
>>>  target/sparc/cpu.c              |  2 +-
>>>  target/tilegx/cpu.c             |  2 +-
>>>  target/tricore/cpu.c            |  2 +-
>>>  target/unicore32/cpu.c          |  2 +-
>>>  target/xtensa/cpu.c             |  2 +-
>>>  27 files changed, 63 insertions(+), 30 deletions(-)
>>>  create mode 100644 include/hw/core/tcg-cpu-ops.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index f53f2678d8..d876f504a6 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1535,6 +1535,7 @@ F: qapi/machine.json
>>>  F: qapi/machine-target.json
>>>  F: include/hw/boards.h
>>>  F: include/hw/core/cpu.h
>>> +F: include/hw/core/tcg-cpu-ops.h
>>>  F: include/hw/cpu/cluster.h
>>>  F: include/sysemu/numa.h
>>>  T: git https://github.com/ehabkost/qemu.git machine-next
>>> diff --git a/cpu.c b/cpu.c
>>> index 0be5dcb6f3..d02c2a17f1 100644
>>> --- a/cpu.c
>>> +++ b/cpu.c
>>> @@ -180,7 +180,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>>>  
>>>      if (tcg_enabled() && !tcg_target_initialized) {
>>>          tcg_target_initialized = true;
>>> -        cc->tcg_initialize();
>>> +        cc->tcg_ops.initialize();
>>>      }
>>>      tlb_init(cpu);
>>>  
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index 3d92c967ff..c93b08a0fb 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -76,6 +76,10 @@ typedef struct CPUWatchpoint CPUWatchpoint;
>>>  
>>>  struct TranslationBlock;
>>>  
>>> +#ifdef CONFIG_TCG
>>> +#include "tcg-cpu-ops.h"
>>> +#endif /* CONFIG_TCG */
>>> +
>>>  /**
>>>   * CPUClass:
>>>   * @class_by_name: Callback to map -cpu command line model name to an
>>> @@ -221,12 +225,15 @@ struct CPUClass {
>>>  
>>>      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
>>>      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
>>> -    void (*tcg_initialize)(void);
>>>  
>>>      const char *deprecation_note;
>>>      /* Keep non-pointer data at the end to minimize holes.  */
>>>      int gdb_num_core_regs;
>>>      bool gdb_stop_before_watchpoint;
>>> +
>>> +#ifdef CONFIG_TCG
>>> +    TcgCpuOperations tcg_ops;
>>> +#endif /* CONFIG_TCG */
>>>  };
> 
> I'm not a fan of #ifdefs in struct definitions (especially in
> generic code like hw/cpu), because there's risk the same header
> generate different struct layout when used by different .c files.
> I would prefer to gradually refactor the code so that tcg_ops is
> eventually removed from CPUClass.
> 
> This is not a dealbreaker, because both approaches are steps in
> the same direction.  But the #ifdef here makes review harder and
> has more risks of unwanted side effects.
> 
>>>  
>>>  /*
>>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
>>> new file mode 100644
>>> index 0000000000..4475ef0996
>>> --- /dev/null
>>> +++ b/include/hw/core/tcg-cpu-ops.h
>>> @@ -0,0 +1,25 @@
>>> +/*
>>> + * TCG-Specific operations that are not meaningful for hardware accelerators
>>> + *
>>> + * Copyright 2020 SUSE LLC
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef TCG_CPU_OPS_H
>>> +#define TCG_CPU_OPS_H
>>> +
>>> +/**
>>> + * struct TcgCpuOperations: TCG operations specific to a CPU class
>>> + */
>>> +typedef struct TcgCpuOperations {
>>> +    /**
>>> +     * @initialize: Initalize TCG state
>>> +     *
>>> +     * Called when the first CPU is realized.
>>> +     */
>>> +    void (*initialize)(void);
>>> +} TcgCpuOperations;
>>> +
>>> +#endif /* TCG_CPU_OPS_H */
>> ...
>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 07492e9f9a..1fa9382a7c 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -2261,7 +2261,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>>>      cc->gdb_stop_before_watchpoint = true;
>>>      cc->disas_set_info = arm_disas_set_info;
>>>  #ifdef CONFIG_TCG
>>> -    cc->tcg_initialize = arm_translate_init;
>>> +    cc->tcg_ops.initialize = arm_translate_init;
>>
>> This one is correctly guarded by '#ifdef CONFIG_TCG'.
>>
>> For the other targets, can you either place it within
>> the '#ifdef CONFIG_TCG' block or if there is none, add
>> one please?
> 
> As a new #ifdef risks having additional unwanted side effects, I
> would prefer to do it in separate patch, just in case.
> 
> This also applies to the #ifdef Claudio added to hw/core/cpu.h
> above.  In case we really want to do it, I would do it in a
> separate patch.

Hi Eduardo,

yes, once things are settling we should dispose of as many #ifdefs are possible

Ciao,

Claudio

> 
>>
>> With that change:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks!
>
Claudio Fontana Dec. 4, 2020, 6:07 p.m. UTC | #5
On 12/4/20 7:04 PM, Claudio Fontana wrote:
> On 12/4/20 6:28 PM, Eduardo Habkost wrote:
>> On Fri, Dec 04, 2020 at 06:10:49PM +0100, Philippe Mathieu-Daudé wrote:
>>> On 11/30/20 3:35 AM, Claudio Fontana wrote:
>>>> From: Eduardo Habkost <ehabkost@redhat.com>
>>>>
>>>> The TCG-specific CPU methods will be moved to a separate struct,
>>>> to make it easier to move accel-specific code outside generic CPU
>>>> code in the future.  Start by moving tcg_initialize().
>>>
>>> Good idea! One minor comment below.
>>>
>>>>
>>>> The new CPUClass.tcg_opts field may eventually become a pointer,
>>>> but keep it an embedded struct for now, to make code conversion
>>>> easier.
>>>>
>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> ---
>>>>  MAINTAINERS                     |  1 +
>>>>  cpu.c                           |  2 +-
>>>>  include/hw/core/cpu.h           |  9 ++++++++-
>>>>  include/hw/core/tcg-cpu-ops.h   | 25 +++++++++++++++++++++++++
>>>>  target/alpha/cpu.c              |  2 +-
>>>>  target/arm/cpu.c                |  2 +-
>>>>  target/avr/cpu.c                |  2 +-
>>>>  target/cris/cpu.c               | 12 ++++++------
>>>>  target/hppa/cpu.c               |  2 +-
>>>>  target/i386/tcg-cpu.c           |  2 +-
>>>>  target/lm32/cpu.c               |  2 +-
>>>>  target/m68k/cpu.c               |  2 +-
>>>>  target/microblaze/cpu.c         |  2 +-
>>>>  target/mips/cpu.c               |  2 +-
>>>>  target/moxie/cpu.c              |  2 +-
>>>>  target/nios2/cpu.c              |  2 +-
>>>>  target/openrisc/cpu.c           |  2 +-
>>>>  target/ppc/translate_init.c.inc |  2 +-
>>>>  target/riscv/cpu.c              |  2 +-
>>>>  target/rx/cpu.c                 |  2 +-
>>>>  target/s390x/cpu.c              |  2 +-
>>>>  target/sh4/cpu.c                |  2 +-
>>>>  target/sparc/cpu.c              |  2 +-
>>>>  target/tilegx/cpu.c             |  2 +-
>>>>  target/tricore/cpu.c            |  2 +-
>>>>  target/unicore32/cpu.c          |  2 +-
>>>>  target/xtensa/cpu.c             |  2 +-
>>>>  27 files changed, 63 insertions(+), 30 deletions(-)
>>>>  create mode 100644 include/hw/core/tcg-cpu-ops.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index f53f2678d8..d876f504a6 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -1535,6 +1535,7 @@ F: qapi/machine.json
>>>>  F: qapi/machine-target.json
>>>>  F: include/hw/boards.h
>>>>  F: include/hw/core/cpu.h
>>>> +F: include/hw/core/tcg-cpu-ops.h
>>>>  F: include/hw/cpu/cluster.h
>>>>  F: include/sysemu/numa.h
>>>>  T: git https://github.com/ehabkost/qemu.git machine-next
>>>> diff --git a/cpu.c b/cpu.c
>>>> index 0be5dcb6f3..d02c2a17f1 100644
>>>> --- a/cpu.c
>>>> +++ b/cpu.c
>>>> @@ -180,7 +180,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>>>>  
>>>>      if (tcg_enabled() && !tcg_target_initialized) {
>>>>          tcg_target_initialized = true;
>>>> -        cc->tcg_initialize();
>>>> +        cc->tcg_ops.initialize();
>>>>      }
>>>>      tlb_init(cpu);
>>>>  
>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>>> index 3d92c967ff..c93b08a0fb 100644
>>>> --- a/include/hw/core/cpu.h
>>>> +++ b/include/hw/core/cpu.h
>>>> @@ -76,6 +76,10 @@ typedef struct CPUWatchpoint CPUWatchpoint;
>>>>  
>>>>  struct TranslationBlock;
>>>>  
>>>> +#ifdef CONFIG_TCG
>>>> +#include "tcg-cpu-ops.h"
>>>> +#endif /* CONFIG_TCG */
>>>> +
>>>>  /**
>>>>   * CPUClass:
>>>>   * @class_by_name: Callback to map -cpu command line model name to an
>>>> @@ -221,12 +225,15 @@ struct CPUClass {
>>>>  
>>>>      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
>>>>      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
>>>> -    void (*tcg_initialize)(void);
>>>>  
>>>>      const char *deprecation_note;
>>>>      /* Keep non-pointer data at the end to minimize holes.  */
>>>>      int gdb_num_core_regs;
>>>>      bool gdb_stop_before_watchpoint;
>>>> +
>>>> +#ifdef CONFIG_TCG
>>>> +    TcgCpuOperations tcg_ops;
>>>> +#endif /* CONFIG_TCG */
>>>>  };
>>
>> I'm not a fan of #ifdefs in struct definitions (especially in
>> generic code like hw/cpu), because there's risk the same header
>> generate different struct layout when used by different .c files.
>> I would prefer to gradually refactor the code so that tcg_ops is
>> eventually removed from CPUClass.
>>
>> This is not a dealbreaker, because both approaches are steps in
>> the same direction.  But the #ifdef here makes review harder and
>> has more risks of unwanted side effects.
>>
>>>>  
>>>>  /*
>>>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
>>>> new file mode 100644
>>>> index 0000000000..4475ef0996
>>>> --- /dev/null
>>>> +++ b/include/hw/core/tcg-cpu-ops.h
>>>> @@ -0,0 +1,25 @@
>>>> +/*
>>>> + * TCG-Specific operations that are not meaningful for hardware accelerators
>>>> + *
>>>> + * Copyright 2020 SUSE LLC
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#ifndef TCG_CPU_OPS_H
>>>> +#define TCG_CPU_OPS_H
>>>> +
>>>> +/**
>>>> + * struct TcgCpuOperations: TCG operations specific to a CPU class
>>>> + */
>>>> +typedef struct TcgCpuOperations {
>>>> +    /**
>>>> +     * @initialize: Initalize TCG state
>>>> +     *
>>>> +     * Called when the first CPU is realized.
>>>> +     */
>>>> +    void (*initialize)(void);
>>>> +} TcgCpuOperations;
>>>> +
>>>> +#endif /* TCG_CPU_OPS_H */
>>> ...
>>>
>>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>>> index 07492e9f9a..1fa9382a7c 100644
>>>> --- a/target/arm/cpu.c
>>>> +++ b/target/arm/cpu.c
>>>> @@ -2261,7 +2261,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>>>>      cc->gdb_stop_before_watchpoint = true;
>>>>      cc->disas_set_info = arm_disas_set_info;
>>>>  #ifdef CONFIG_TCG
>>>> -    cc->tcg_initialize = arm_translate_init;
>>>> +    cc->tcg_ops.initialize = arm_translate_init;
>>>
>>> This one is correctly guarded by '#ifdef CONFIG_TCG'.
>>>
>>> For the other targets, can you either place it within
>>> the '#ifdef CONFIG_TCG' block or if there is none, add
>>> one please?
>>
>> As a new #ifdef risks having additional unwanted side effects, I
>> would prefer to do it in separate patch, just in case.
>>
>> This also applies to the #ifdef Claudio added to hw/core/cpu.h
>> above.  In case we really want to do it, I would do it in a
>> separate patch.
> 
> Hi Eduardo,
> 
> yes, once things are settling we should dispose of as many #ifdefs are possible


By that I mean, as targets are adapted (arm, s390, ...) things can be refactored in a similar was as for x86,
so that the ifdefs disappear, and meson instead controls which pieces are built.

When it comes to the extra field, it was already very useful to have it #ifdef ed,
because it identified quite a few problem with the existing patches, ie, code that was trying to use the field even though it was not there.

So this step helps with finding all the right pieces to refactor away,
and then once things are in their proper place, we can take away the ifdef I think.

Ciao,

Claudio


> 
> Ciao,
> 
> Claudio
> 
>>
>>>
>>> With that change:
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Thanks!
>>
>
Eduardo Habkost Dec. 4, 2020, 6:29 p.m. UTC | #6
On Fri, Dec 04, 2020 at 07:07:09PM +0100, Claudio Fontana wrote:
> On 12/4/20 7:04 PM, Claudio Fontana wrote:
> > On 12/4/20 6:28 PM, Eduardo Habkost wrote:
> >> On Fri, Dec 04, 2020 at 06:10:49PM +0100, Philippe Mathieu-Daudé wrote:
> >>> On 11/30/20 3:35 AM, Claudio Fontana wrote:
> >>>> From: Eduardo Habkost <ehabkost@redhat.com>
> >>>>
> >>>> The TCG-specific CPU methods will be moved to a separate struct,
> >>>> to make it easier to move accel-specific code outside generic CPU
> >>>> code in the future.  Start by moving tcg_initialize().
> >>>
> >>> Good idea! One minor comment below.
> >>>
> >>>>
> >>>> The new CPUClass.tcg_opts field may eventually become a pointer,
> >>>> but keep it an embedded struct for now, to make code conversion
> >>>> easier.
> >>>>
> >>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>>> ---
> >>>>  MAINTAINERS                     |  1 +
> >>>>  cpu.c                           |  2 +-
> >>>>  include/hw/core/cpu.h           |  9 ++++++++-
> >>>>  include/hw/core/tcg-cpu-ops.h   | 25 +++++++++++++++++++++++++
> >>>>  target/alpha/cpu.c              |  2 +-
> >>>>  target/arm/cpu.c                |  2 +-
> >>>>  target/avr/cpu.c                |  2 +-
> >>>>  target/cris/cpu.c               | 12 ++++++------
> >>>>  target/hppa/cpu.c               |  2 +-
> >>>>  target/i386/tcg-cpu.c           |  2 +-
> >>>>  target/lm32/cpu.c               |  2 +-
> >>>>  target/m68k/cpu.c               |  2 +-
> >>>>  target/microblaze/cpu.c         |  2 +-
> >>>>  target/mips/cpu.c               |  2 +-
> >>>>  target/moxie/cpu.c              |  2 +-
> >>>>  target/nios2/cpu.c              |  2 +-
> >>>>  target/openrisc/cpu.c           |  2 +-
> >>>>  target/ppc/translate_init.c.inc |  2 +-
> >>>>  target/riscv/cpu.c              |  2 +-
> >>>>  target/rx/cpu.c                 |  2 +-
> >>>>  target/s390x/cpu.c              |  2 +-
> >>>>  target/sh4/cpu.c                |  2 +-
> >>>>  target/sparc/cpu.c              |  2 +-
> >>>>  target/tilegx/cpu.c             |  2 +-
> >>>>  target/tricore/cpu.c            |  2 +-
> >>>>  target/unicore32/cpu.c          |  2 +-
> >>>>  target/xtensa/cpu.c             |  2 +-
> >>>>  27 files changed, 63 insertions(+), 30 deletions(-)
> >>>>  create mode 100644 include/hw/core/tcg-cpu-ops.h
> >>>>
> >>>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>>> index f53f2678d8..d876f504a6 100644
> >>>> --- a/MAINTAINERS
> >>>> +++ b/MAINTAINERS
> >>>> @@ -1535,6 +1535,7 @@ F: qapi/machine.json
> >>>>  F: qapi/machine-target.json
> >>>>  F: include/hw/boards.h
> >>>>  F: include/hw/core/cpu.h
> >>>> +F: include/hw/core/tcg-cpu-ops.h
> >>>>  F: include/hw/cpu/cluster.h
> >>>>  F: include/sysemu/numa.h
> >>>>  T: git https://github.com/ehabkost/qemu.git machine-next
> >>>> diff --git a/cpu.c b/cpu.c
> >>>> index 0be5dcb6f3..d02c2a17f1 100644
> >>>> --- a/cpu.c
> >>>> +++ b/cpu.c
> >>>> @@ -180,7 +180,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
> >>>>  
> >>>>      if (tcg_enabled() && !tcg_target_initialized) {
> >>>>          tcg_target_initialized = true;
> >>>> -        cc->tcg_initialize();
> >>>> +        cc->tcg_ops.initialize();
> >>>>      }
> >>>>      tlb_init(cpu);
> >>>>  
> >>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> >>>> index 3d92c967ff..c93b08a0fb 100644
> >>>> --- a/include/hw/core/cpu.h
> >>>> +++ b/include/hw/core/cpu.h
> >>>> @@ -76,6 +76,10 @@ typedef struct CPUWatchpoint CPUWatchpoint;
> >>>>  
> >>>>  struct TranslationBlock;
> >>>>  
> >>>> +#ifdef CONFIG_TCG
> >>>> +#include "tcg-cpu-ops.h"
> >>>> +#endif /* CONFIG_TCG */
> >>>> +
> >>>>  /**
> >>>>   * CPUClass:
> >>>>   * @class_by_name: Callback to map -cpu command line model name to an
> >>>> @@ -221,12 +225,15 @@ struct CPUClass {
> >>>>  
> >>>>      void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
> >>>>      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
> >>>> -    void (*tcg_initialize)(void);
> >>>>  
> >>>>      const char *deprecation_note;
> >>>>      /* Keep non-pointer data at the end to minimize holes.  */
> >>>>      int gdb_num_core_regs;
> >>>>      bool gdb_stop_before_watchpoint;
> >>>> +
> >>>> +#ifdef CONFIG_TCG
> >>>> +    TcgCpuOperations tcg_ops;
> >>>> +#endif /* CONFIG_TCG */
> >>>>  };
> >>
> >> I'm not a fan of #ifdefs in struct definitions (especially in
> >> generic code like hw/cpu), because there's risk the same header
> >> generate different struct layout when used by different .c files.
> >> I would prefer to gradually refactor the code so that tcg_ops is
> >> eventually removed from CPUClass.
> >>
> >> This is not a dealbreaker, because both approaches are steps in
> >> the same direction.  But the #ifdef here makes review harder and
> >> has more risks of unwanted side effects.
> >>
> >>>>  
> >>>>  /*
> >>>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> >>>> new file mode 100644
> >>>> index 0000000000..4475ef0996
> >>>> --- /dev/null
> >>>> +++ b/include/hw/core/tcg-cpu-ops.h
> >>>> @@ -0,0 +1,25 @@
> >>>> +/*
> >>>> + * TCG-Specific operations that are not meaningful for hardware accelerators
> >>>> + *
> >>>> + * Copyright 2020 SUSE LLC
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>>> + * See the COPYING file in the top-level directory.
> >>>> + */
> >>>> +
> >>>> +#ifndef TCG_CPU_OPS_H
> >>>> +#define TCG_CPU_OPS_H
> >>>> +
> >>>> +/**
> >>>> + * struct TcgCpuOperations: TCG operations specific to a CPU class
> >>>> + */
> >>>> +typedef struct TcgCpuOperations {
> >>>> +    /**
> >>>> +     * @initialize: Initalize TCG state
> >>>> +     *
> >>>> +     * Called when the first CPU is realized.
> >>>> +     */
> >>>> +    void (*initialize)(void);
> >>>> +} TcgCpuOperations;
> >>>> +
> >>>> +#endif /* TCG_CPU_OPS_H */
> >>> ...
> >>>
> >>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> >>>> index 07492e9f9a..1fa9382a7c 100644
> >>>> --- a/target/arm/cpu.c
> >>>> +++ b/target/arm/cpu.c
> >>>> @@ -2261,7 +2261,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
> >>>>      cc->gdb_stop_before_watchpoint = true;
> >>>>      cc->disas_set_info = arm_disas_set_info;
> >>>>  #ifdef CONFIG_TCG
> >>>> -    cc->tcg_initialize = arm_translate_init;
> >>>> +    cc->tcg_ops.initialize = arm_translate_init;
> >>>
> >>> This one is correctly guarded by '#ifdef CONFIG_TCG'.
> >>>
> >>> For the other targets, can you either place it within
> >>> the '#ifdef CONFIG_TCG' block or if there is none, add
> >>> one please?
> >>
> >> As a new #ifdef risks having additional unwanted side effects, I
> >> would prefer to do it in separate patch, just in case.
> >>
> >> This also applies to the #ifdef Claudio added to hw/core/cpu.h
> >> above.  In case we really want to do it, I would do it in a
> >> separate patch.
> > 
> > Hi Eduardo,
> > 
> > yes, once things are settling we should dispose of as many #ifdefs are possible
> 
> 
> By that I mean, as targets are adapted (arm, s390, ...) things can be refactored in a similar was as for x86,
> so that the ifdefs disappear, and meson instead controls which pieces are built.
> 
> When it comes to the extra field, it was already very useful to have it #ifdef ed,
> because it identified quite a few problem with the existing patches, ie, code that was trying to use the field even though it was not there.
> 
> So this step helps with finding all the right pieces to refactor away,
> and then once things are in their proper place, we can take away the ifdef I think.

That's a good point.  Grepping for CONFIG_TCG will help us find
out cases where the code still has to be moved to a separate
file.

I was probably being overly cautious about the ifdef.  CONFIG_TCG
is not as tricky as target-specific macros that can't be used in
any header.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index f53f2678d8..d876f504a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1535,6 +1535,7 @@  F: qapi/machine.json
 F: qapi/machine-target.json
 F: include/hw/boards.h
 F: include/hw/core/cpu.h
+F: include/hw/core/tcg-cpu-ops.h
 F: include/hw/cpu/cluster.h
 F: include/sysemu/numa.h
 T: git https://github.com/ehabkost/qemu.git machine-next
diff --git a/cpu.c b/cpu.c
index 0be5dcb6f3..d02c2a17f1 100644
--- a/cpu.c
+++ b/cpu.c
@@ -180,7 +180,7 @@  void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 
     if (tcg_enabled() && !tcg_target_initialized) {
         tcg_target_initialized = true;
-        cc->tcg_initialize();
+        cc->tcg_ops.initialize();
     }
     tlb_init(cpu);
 
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 3d92c967ff..c93b08a0fb 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -76,6 +76,10 @@  typedef struct CPUWatchpoint CPUWatchpoint;
 
 struct TranslationBlock;
 
+#ifdef CONFIG_TCG
+#include "tcg-cpu-ops.h"
+#endif /* CONFIG_TCG */
+
 /**
  * CPUClass:
  * @class_by_name: Callback to map -cpu command line model name to an
@@ -221,12 +225,15 @@  struct CPUClass {
 
     void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
     vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
-    void (*tcg_initialize)(void);
 
     const char *deprecation_note;
     /* Keep non-pointer data at the end to minimize holes.  */
     int gdb_num_core_regs;
     bool gdb_stop_before_watchpoint;
+
+#ifdef CONFIG_TCG
+    TcgCpuOperations tcg_ops;
+#endif /* CONFIG_TCG */
 };
 
 /*
diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
new file mode 100644
index 0000000000..4475ef0996
--- /dev/null
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -0,0 +1,25 @@ 
+/*
+ * TCG-Specific operations that are not meaningful for hardware accelerators
+ *
+ * Copyright 2020 SUSE LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef TCG_CPU_OPS_H
+#define TCG_CPU_OPS_H
+
+/**
+ * struct TcgCpuOperations: TCG operations specific to a CPU class
+ */
+typedef struct TcgCpuOperations {
+    /**
+     * @initialize: Initalize TCG state
+     *
+     * Called when the first CPU is realized.
+     */
+    void (*initialize)(void);
+} TcgCpuOperations;
+
+#endif /* TCG_CPU_OPS_H */
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index b3fd6643e8..d66f0351a9 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -231,7 +231,7 @@  static void alpha_cpu_class_init(ObjectClass *oc, void *data)
     dc->vmsd = &vmstate_alpha_cpu;
 #endif
     cc->disas_set_info = alpha_cpu_disas_set_info;
-    cc->tcg_initialize = alpha_translate_init;
+    cc->tcg_ops.initialize = alpha_translate_init;
 
     cc->gdb_num_core_regs = 67;
 }
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 07492e9f9a..1fa9382a7c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2261,7 +2261,7 @@  static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_stop_before_watchpoint = true;
     cc->disas_set_info = arm_disas_set_info;
 #ifdef CONFIG_TCG
-    cc->tcg_initialize = arm_translate_init;
+    cc->tcg_ops.initialize = arm_translate_init;
     cc->tlb_fill = arm_cpu_tlb_fill;
     cc->debug_excp_handler = arm_debug_excp_handler;
     cc->debug_check_watchpoint = arm_debug_check_watchpoint;
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 5d9c4ad5bf..94306a2aa0 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -206,7 +206,7 @@  static void avr_cpu_class_init(ObjectClass *oc, void *data)
     cc->tlb_fill = avr_cpu_tlb_fill;
     cc->vmsd = &vms_avr_cpu;
     cc->disas_set_info = avr_cpu_disas_set_info;
-    cc->tcg_initialize = avr_cpu_tcg_init;
+    cc->tcg_ops.initialize = avr_cpu_tcg_init;
     cc->synchronize_from_tb = avr_cpu_synchronize_from_tb;
     cc->gdb_read_register = avr_cpu_gdb_read_register;
     cc->gdb_write_register = avr_cpu_gdb_write_register;
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index cff6b9eabf..4328f8e6c9 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -201,7 +201,7 @@  static void crisv8_cpu_class_init(ObjectClass *oc, void *data)
     ccc->vr = 8;
     cc->do_interrupt = crisv10_cpu_do_interrupt;
     cc->gdb_read_register = crisv10_cpu_gdb_read_register;
-    cc->tcg_initialize = cris_initialize_crisv10_tcg;
+    cc->tcg_ops.initialize = cris_initialize_crisv10_tcg;
 }
 
 static void crisv9_cpu_class_init(ObjectClass *oc, void *data)
@@ -212,7 +212,7 @@  static void crisv9_cpu_class_init(ObjectClass *oc, void *data)
     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;
+    cc->tcg_ops.initialize = cris_initialize_crisv10_tcg;
 }
 
 static void crisv10_cpu_class_init(ObjectClass *oc, void *data)
@@ -223,7 +223,7 @@  static void crisv10_cpu_class_init(ObjectClass *oc, void *data)
     ccc->vr = 10;
     cc->do_interrupt = crisv10_cpu_do_interrupt;
     cc->gdb_read_register = crisv10_cpu_gdb_read_register;
-    cc->tcg_initialize = cris_initialize_crisv10_tcg;
+    cc->tcg_ops.initialize = cris_initialize_crisv10_tcg;
 }
 
 static void crisv11_cpu_class_init(ObjectClass *oc, void *data)
@@ -234,7 +234,7 @@  static void crisv11_cpu_class_init(ObjectClass *oc, void *data)
     ccc->vr = 11;
     cc->do_interrupt = crisv10_cpu_do_interrupt;
     cc->gdb_read_register = crisv10_cpu_gdb_read_register;
-    cc->tcg_initialize = cris_initialize_crisv10_tcg;
+    cc->tcg_ops.initialize = cris_initialize_crisv10_tcg;
 }
 
 static void crisv17_cpu_class_init(ObjectClass *oc, void *data)
@@ -245,7 +245,7 @@  static void crisv17_cpu_class_init(ObjectClass *oc, void *data)
     ccc->vr = 17;
     cc->do_interrupt = crisv10_cpu_do_interrupt;
     cc->gdb_read_register = crisv10_cpu_gdb_read_register;
-    cc->tcg_initialize = cris_initialize_crisv10_tcg;
+    cc->tcg_ops.initialize = cris_initialize_crisv10_tcg;
 }
 
 static void crisv32_cpu_class_init(ObjectClass *oc, void *data)
@@ -284,7 +284,7 @@  static void cris_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_stop_before_watchpoint = true;
 
     cc->disas_set_info = cris_disas_set_info;
-    cc->tcg_initialize = cris_initialize_tcg;
+    cc->tcg_ops.initialize = cris_initialize_tcg;
 }
 
 #define DEFINE_CRIS_CPU_TYPE(cpu_model, initfn) \
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 71b6aca45d..4c778966c2 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -153,7 +153,7 @@  static void hppa_cpu_class_init(ObjectClass *oc, void *data)
 #endif
     cc->do_unaligned_access = hppa_cpu_do_unaligned_access;
     cc->disas_set_info = hppa_cpu_disas_set_info;
-    cc->tcg_initialize = hppa_translate_init;
+    cc->tcg_ops.initialize = hppa_translate_init;
 
     cc->gdb_num_core_regs = 128;
 }
diff --git a/target/i386/tcg-cpu.c b/target/i386/tcg-cpu.c
index 628dd29fe7..1f2a3e881a 100644
--- a/target/i386/tcg-cpu.c
+++ b/target/i386/tcg-cpu.c
@@ -63,7 +63,7 @@  void tcg_cpu_common_class_init(CPUClass *cc)
     cc->synchronize_from_tb = x86_cpu_synchronize_from_tb;
     cc->cpu_exec_enter = x86_cpu_exec_enter;
     cc->cpu_exec_exit = x86_cpu_exec_exit;
-    cc->tcg_initialize = tcg_x86_init;
+    cc->tcg_ops.initialize = tcg_x86_init;
     cc->tlb_fill = x86_cpu_tlb_fill;
 #ifndef CONFIG_USER_ONLY
     cc->debug_excp_handler = breakpoint_handler;
diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
index c50ad5fa15..ef795b81a4 100644
--- a/target/lm32/cpu.c
+++ b/target/lm32/cpu.c
@@ -237,7 +237,7 @@  static void lm32_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_stop_before_watchpoint = true;
     cc->debug_excp_handler = lm32_debug_excp_handler;
     cc->disas_set_info = lm32_cpu_disas_set_info;
-    cc->tcg_initialize = lm32_translate_init;
+    cc->tcg_ops.initialize = lm32_translate_init;
 }
 
 #define DEFINE_LM32_CPU_TYPE(cpu_model, initfn) \
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 72c545149e..b66d86c353 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -289,7 +289,7 @@  static void m68k_cpu_class_init(ObjectClass *c, void *data)
     cc->get_phys_page_debug = m68k_cpu_get_phys_page_debug;
 #endif
     cc->disas_set_info = m68k_cpu_disas_set_info;
-    cc->tcg_initialize = m68k_tcg_init;
+    cc->tcg_ops.initialize = m68k_tcg_init;
 
     cc->gdb_num_core_regs = 18;
 
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 9b2482159d..bc10518fa3 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -335,7 +335,7 @@  static void mb_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_num_core_regs = 32 + 27;
 
     cc->disas_set_info = mb_disas_set_info;
-    cc->tcg_initialize = mb_tcg_init;
+    cc->tcg_ops.initialize = mb_tcg_init;
 }
 
 static const TypeInfo mb_cpu_type_info = {
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 76d50b00b4..bc48573763 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -249,7 +249,7 @@  static void mips_cpu_class_init(ObjectClass *c, void *data)
 #endif
     cc->disas_set_info = mips_cpu_disas_set_info;
 #ifdef CONFIG_TCG
-    cc->tcg_initialize = mips_tcg_init;
+    cc->tcg_ops.initialize = mips_tcg_init;
     cc->tlb_fill = mips_cpu_tlb_fill;
 #endif
 
diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c
index 6e0443ccb7..224cfc8361 100644
--- a/target/moxie/cpu.c
+++ b/target/moxie/cpu.c
@@ -116,7 +116,7 @@  static void moxie_cpu_class_init(ObjectClass *oc, void *data)
     cc->vmsd = &vmstate_moxie_cpu;
 #endif
     cc->disas_set_info = moxie_cpu_disas_set_info;
-    cc->tcg_initialize = moxie_translate_init;
+    cc->tcg_ops.initialize = moxie_translate_init;
 }
 
 static void moxielite_initfn(Object *obj)
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 8f7011fcb9..29c9c6f634 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -205,7 +205,7 @@  static void nios2_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_read_register = nios2_cpu_gdb_read_register;
     cc->gdb_write_register = nios2_cpu_gdb_write_register;
     cc->gdb_num_core_regs = 49;
-    cc->tcg_initialize = nios2_tcg_init;
+    cc->tcg_ops.initialize = nios2_tcg_init;
 }
 
 static const TypeInfo nios2_cpu_type_info = {
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 5528c0918f..e442f4f97c 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -166,7 +166,7 @@  static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
     dc->vmsd = &vmstate_openrisc_cpu;
 #endif
     cc->gdb_num_core_regs = 32 + 3;
-    cc->tcg_initialize = openrisc_translate_init;
+    cc->tcg_ops.initialize = openrisc_translate_init;
     cc->disas_set_info = openrisc_disas_set_info;
 }
 
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index 78cc8f043b..9a6932b774 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -10945,7 +10945,7 @@  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
 #endif
 #ifdef CONFIG_TCG
-    cc->tcg_initialize = ppc_translate_init;
+    cc->tcg_ops.initialize = ppc_translate_init;
     cc->tlb_fill = ppc_cpu_tlb_fill;
 #endif
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 6a0264fc6b..a52e0ce466 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -562,7 +562,7 @@  static void riscv_cpu_class_init(ObjectClass *c, void *data)
     cc->vmsd = &vmstate_riscv_cpu;
 #endif
 #ifdef CONFIG_TCG
-    cc->tcg_initialize = riscv_translate_init;
+    cc->tcg_ops.initialize = riscv_translate_init;
     cc->tlb_fill = riscv_cpu_tlb_fill;
 #endif
     device_class_set_props(dc, riscv_cpu_properties);
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 23ee17a701..a701a09b11 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -194,7 +194,7 @@  static void rx_cpu_class_init(ObjectClass *klass, void *data)
     cc->gdb_write_register = rx_cpu_gdb_write_register;
     cc->get_phys_page_debug = rx_cpu_get_phys_page_debug;
     cc->disas_set_info = rx_cpu_disas_set_info;
-    cc->tcg_initialize = rx_translate_init;
+    cc->tcg_ops.initialize = rx_translate_init;
     cc->tlb_fill = rx_cpu_tlb_fill;
 
     cc->gdb_num_core_regs = 26;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 7b66718c44..697b94ff7b 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -512,7 +512,7 @@  static void s390_cpu_class_init(ObjectClass *oc, void *data)
 #endif
     cc->disas_set_info = s390_cpu_disas_set_info;
 #ifdef CONFIG_TCG
-    cc->tcg_initialize = s390x_translate_init;
+    cc->tcg_ops.initialize = s390x_translate_init;
     cc->tlb_fill = s390_cpu_tlb_fill;
 #endif
 
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 3c68021c56..bdc5c9d90b 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -231,7 +231,7 @@  static void superh_cpu_class_init(ObjectClass *oc, void *data)
     cc->get_phys_page_debug = superh_cpu_get_phys_page_debug;
 #endif
     cc->disas_set_info = superh_cpu_disas_set_info;
-    cc->tcg_initialize = sh4_translate_init;
+    cc->tcg_ops.initialize = sh4_translate_init;
 
     cc->gdb_num_core_regs = 59;
 
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 701e794eac..07e48b86d1 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -879,7 +879,7 @@  static void sparc_cpu_class_init(ObjectClass *oc, void *data)
     cc->vmsd = &vmstate_sparc_cpu;
 #endif
     cc->disas_set_info = cpu_sparc_disas_set_info;
-    cc->tcg_initialize = sparc_tcg_init;
+    cc->tcg_ops.initialize = sparc_tcg_init;
 
 #if defined(TARGET_SPARC64) && !defined(TARGET_ABI32)
     cc->gdb_num_core_regs = 86;
diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
index 1fee87c094..cd24d0eb9d 100644
--- a/target/tilegx/cpu.c
+++ b/target/tilegx/cpu.c
@@ -153,7 +153,7 @@  static void tilegx_cpu_class_init(ObjectClass *oc, void *data)
     cc->set_pc = tilegx_cpu_set_pc;
     cc->tlb_fill = tilegx_cpu_tlb_fill;
     cc->gdb_num_core_regs = 0;
-    cc->tcg_initialize = tilegx_tcg_init;
+    cc->tcg_ops.initialize = tilegx_tcg_init;
 }
 
 static const TypeInfo tilegx_cpu_type_info = {
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 2f2e5b029f..78b2925955 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -164,7 +164,7 @@  static void tricore_cpu_class_init(ObjectClass *c, void *data)
     cc->set_pc = tricore_cpu_set_pc;
     cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
     cc->get_phys_page_debug = tricore_cpu_get_phys_page_debug;
-    cc->tcg_initialize = tricore_tcg_init;
+    cc->tcg_ops.initialize = tricore_tcg_init;
     cc->tlb_fill = tricore_cpu_tlb_fill;
 }
 
diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
index b27fb9689f..226bf4226e 100644
--- a/target/unicore32/cpu.c
+++ b/target/unicore32/cpu.c
@@ -137,7 +137,7 @@  static void uc32_cpu_class_init(ObjectClass *oc, void *data)
     cc->set_pc = uc32_cpu_set_pc;
     cc->tlb_fill = uc32_cpu_tlb_fill;
     cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug;
-    cc->tcg_initialize = uc32_translate_init;
+    cc->tcg_ops.initialize = uc32_translate_init;
     dc->vmsd = &vmstate_uc32_cpu;
 }
 
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 88a32268a1..5a6f5bf88b 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -209,7 +209,7 @@  static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
 #endif
     cc->debug_excp_handler = xtensa_breakpoint_handler;
     cc->disas_set_info = xtensa_cpu_disas_set_info;
-    cc->tcg_initialize = xtensa_translate_init;
+    cc->tcg_ops.initialize = xtensa_translate_init;
     dc->vmsd = &vmstate_xtensa_cpu;
 }