Re: [PATCH v2 7/7] target/ppc: wrapped some TCG only logic with ifdefs

[David Gibson]

On Tue, May 18, 2021 at 12:05:15PM -0300, Bruno Larsen (billionai) wrote:
> Wrapped some function calls in cpu_init.c, gdbstub.c, mmu-hash64.c and
> excp_helper.c that were TCG only with ifdef CONFIG_TCG, to support
> building without TCG.
> 
> for excp_helper we also moved the function do_rfi higher in the file to
> reduce the ifdef count.

The description's no longer really accurate since some of the fixups
are no longer ifdef based.


> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/cpu_init.c    | 16 +++++---
>  target/ppc/excp_helper.c | 82 +++++++++++++++++++++++-----------------
>  target/ppc/mmu-hash64.c  |  8 ++++
>  3 files changed, 66 insertions(+), 40 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 88a8344eea..5ab4d4ef2b 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -1203,15 +1203,13 @@ static void register_BookE206_sprs(CPUPPCState *env, 
> uint32_t mas_mask,
>      /* TLB assist registers */
>      /* XXX : not implemented */
>      for (i = 0; i < 8; i++) {
> -        void (*uea_write)(DisasContext *ctx, int sprn, int gprn) =
> -            &spr_write_generic32;
> -        if (i == 2 && (mas_mask & (1 << i)) && (env->insns_flags & PPC_64B)) 
> {
> -            uea_write = &spr_write_generic;
> -        }
>          if (mas_mask & (1 << i)) {
>              spr_register(env, mas_sprn[i], mas_names[i],
>                           SPR_NOACCESS, SPR_NOACCESS,
> -                         &spr_read_generic, uea_write,
> +                         &spr_read_generic,
> +                         (i == 2 && (mas_mask & (1 << i)) &&
> +                         (env->insns_flags & PPC_64B))
> +                         ? &spr_write_generic : &spr_write_generic32,


Looks good.
>                           0x00000000);
>          }
>      }
> @@ -8605,11 +8603,13 @@ static void ppc_cpu_realize(DeviceState *dev, Error 
> **errp)
>          }
>      }
>  
> +#ifdef CONFIG_TCG
>      create_ppc_opcodes(cpu, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          goto unrealize;
>      }
> +#endif

In this instance, I think it would be cleaner to create a no-op stub
for create_ppc_opcodes() and destroy_ppc_opcodes() rather than using
ifdefs.

>      init_ppc_proc(cpu);
>  
>      ppc_gdb_init(cs, pcc);
> @@ -8798,7 +8798,9 @@ static void ppc_cpu_unrealize(DeviceState *dev)
>  
>      cpu_remove_sync(CPU(cpu));
>  
> +#ifdef CONFIG_TCG
>      destroy_ppc_opcodes(cpu);
> +#endif
>  }
>  
>  static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
> @@ -9296,7 +9298,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->class_by_name = ppc_cpu_class_by_name;
>      cc->has_work = ppc_cpu_has_work;
>      cc->dump_state = ppc_cpu_dump_state;
> +#ifdef CONFIG_TCG
>      cc->dump_statistics = ppc_cpu_dump_statistics;
> +#endif
>      cc->set_pc = ppc_cpu_set_pc;
>      cc->gdb_read_register = ppc_cpu_gdb_read_register;
>      cc->gdb_write_register = ppc_cpu_gdb_write_register;
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 80bb6e70e9..a14b529722 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -19,9 +19,13 @@
>  #include "qemu/osdep.h"
>  #include "qemu/main-loop.h"
>  #include "cpu.h"
> +#ifdef CONFIG_TCG
>  #include "exec/helper-proto.h"
> +#endif

I don't like the look of ifdefs amongst the includes.  Generally the
headers themselves should be made safe (if unnecessary) to include for
!TCG builds.

>  #include "exec/exec-all.h"
> +#ifdef CONFIG_TCG
>  #include "exec/cpu_ldst.h"
> +#endif
>  #include "internal.h"
>  #include "helper_regs.h"

The remaining ifdef changes look fine.  Some it would be nice to clean
up better in future, but there's no rush.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature