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.

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,
                          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
     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
 #include "exec/exec-all.h"
+#ifdef CONFIG_TCG
 #include "exec/cpu_ldst.h"
+#endif
 #include "internal.h"
 #include "helper_regs.h"
 
@@ -1191,6 +1195,7 @@ void raise_exception_ra(CPUPPCState *env, uint32_t exception,
     raise_exception_err_ra(env, exception, 0, raddr);
 }
 
+#ifdef CONFIG_TCG
 void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
                                 uint32_t error_code)
 {
@@ -1201,8 +1206,43 @@ void helper_raise_exception(CPUPPCState *env, uint32_t exception)
 {
     raise_exception_err_ra(env, exception, 0, 0);
 }
+#endif
 
 #if !defined(CONFIG_USER_ONLY)
+static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
+{
+    CPUState *cs = env_cpu(env);
+
+    /* MSR:POW cannot be set by any form of rfi */
+    msr &= ~(1ULL << MSR_POW);
+
+#if defined(TARGET_PPC64)
+    /* Switching to 32-bit ? Crop the nip */
+    if (!msr_is_64bit(env, msr)) {
+        nip = (uint32_t)nip;
+    }
+#else
+    nip = (uint32_t)nip;
+#endif
+    /* XXX: beware: this is false if VLE is supported */
+    env->nip = nip & ~((target_ulong)0x00000003);
+    hreg_store_msr(env, msr, 1);
+#if defined(DEBUG_OP)
+    cpu_dump_rfi(env->nip, env->msr);
+#endif
+    /*
+     * No need to raise an exception here, as rfi is always the last
+     * insn of a TB
+     */
+    cpu_interrupt_exittb(cs);
+    /* Reset the reservation */
+    env->reserve_addr = -1;
+
+    /* Context synchronizing: check if TCG TLB needs flush */
+    check_tlb_flush(env, false);
+}
+
+#ifdef CONFIG_TCG
 void helper_store_msr(CPUPPCState *env, target_ulong val)
 {
     uint32_t excp = hreg_store_msr(env, val, 0);
@@ -1243,39 +1283,6 @@ void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
 }
 #endif /* defined(TARGET_PPC64) */
 
-static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
-{
-    CPUState *cs = env_cpu(env);
-
-    /* MSR:POW cannot be set by any form of rfi */
-    msr &= ~(1ULL << MSR_POW);
-
-#if defined(TARGET_PPC64)
-    /* Switching to 32-bit ? Crop the nip */
-    if (!msr_is_64bit(env, msr)) {
-        nip = (uint32_t)nip;
-    }
-#else
-    nip = (uint32_t)nip;
-#endif
-    /* XXX: beware: this is false if VLE is supported */
-    env->nip = nip & ~((target_ulong)0x00000003);
-    hreg_store_msr(env, msr, 1);
-#if defined(DEBUG_OP)
-    cpu_dump_rfi(env->nip, env->msr);
-#endif
-    /*
-     * No need to raise an exception here, as rfi is always the last
-     * insn of a TB
-     */
-    cpu_interrupt_exittb(cs);
-    /* Reset the reservation */
-    env->reserve_addr = -1;
-
-    /* Context synchronizing: check if TCG TLB needs flush */
-    check_tlb_flush(env, false);
-}
-
 void helper_rfi(CPUPPCState *env)
 {
     do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xfffffffful);
@@ -1328,8 +1335,10 @@ void helper_rfmci(CPUPPCState *env)
     /* FIXME: choose CSRR1 or MCSRR1 based on cpu type */
     do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
 }
-#endif
+#endif /* CONFIG_TCG */
+#endif /* !defined(CONFIG_USER_ONLY) */
 
+#ifdef CONFIG_TCG
 void helper_tw(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
                uint32_t flags)
 {
@@ -1357,11 +1366,13 @@ void helper_td(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
     }
 }
 #endif
+#endif
 
 #if !defined(CONFIG_USER_ONLY)
 /*****************************************************************************/
 /* PowerPC 601 specific instructions (POWER bridge) */
 
+#ifdef CONFIG_TCG
 void helper_rfsvc(CPUPPCState *env)
 {
     do_rfi(env, env->lr, env->ctr & 0x0000FFFF);
@@ -1506,8 +1517,10 @@ void helper_book3s_msgsndp(CPUPPCState *env, target_ulong rb)
     book3s_msgsnd_common(pir, PPC_INTERRUPT_DOORBELL);
 }
 #endif
+#endif /* CONFIG_TCG */
 #endif
 
+#ifdef CONFIG_TCG
 void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
                                  MMUAccessType access_type,
                                  int mmu_idx, uintptr_t retaddr)
@@ -1523,3 +1536,4 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
     env->error_code = insn & 0x03FF0000;
     cpu_loop_exit(cs);
 }
+#endif
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index c4a4bc7cd2..5f589c9690 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -21,7 +21,9 @@
 #include "qemu/units.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
+#ifdef CONFIG_TCG
 #include "exec/helper-proto.h"
+#endif
 #include "qemu/error-report.h"
 #include "qemu/qemu-print.h"
 #include "sysemu/hw_accel.h"
@@ -96,6 +98,7 @@ void dump_slb(PowerPCCPU *cpu)
     }
 }
 
+#ifdef CONFIG_TCG
 void helper_slbia(CPUPPCState *env, uint32_t ih)
 {
     PowerPCCPU *cpu = env_archcpu(env);
@@ -201,6 +204,7 @@ void helper_slbieg(CPUPPCState *env, target_ulong addr)
 {
     __helper_slbie(env, addr, true);
 }
+#endif
 
 int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
                   target_ulong esid, target_ulong vsid)
@@ -254,6 +258,7 @@ int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
     return 0;
 }
 
+#ifdef CONFIG_TCG
 static int ppc_load_slb_esid(PowerPCCPU *cpu, target_ulong rb,
                              target_ulong *rt)
 {
@@ -347,6 +352,7 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, target_ulong rb)
     }
     return rt;
 }
+#endif
 
 /* Check No-Execute or Guarded Storage */
 static inline int ppc_hash64_pte_noexec_guard(PowerPCCPU *cpu,
@@ -1120,12 +1126,14 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
     cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
 }
 
+#ifdef CONFIG_TCG
 void helper_store_lpcr(CPUPPCState *env, target_ulong val)
 {
     PowerPCCPU *cpu = env_archcpu(env);
 
     ppc_store_lpcr(cpu, val);
 }
+#endif
 
 void ppc_hash64_init(PowerPCCPU *cpu)
 {
-- 
2.17.1

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

On 18/05/2021 23:02, David Gibson wrote:
> 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.
Sure, will do
> <snip>
>>                            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.
Ok. will add the stubs in tcg-stubs.c
>
>>       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.

The problems that happen with exec/helper-proto.h and exec/cpu_ldst.h is 
that they include headers themselves, trace/generated-helpers.h and 
tcg-target.h respectively, which are in folders that are not included as 
-iquote in the gcc CLI call.

So the problem is that it is trying to include headers that gcc doesn't 
see as part of the project anymore. The best option (I think) would be 
to fix the gcc command generation so headers are safe to include, but 
since I was very confused with all the scripts related to generating 
everything, I ended up going with not including the files. Should I 
change the configure script, so that we can include headers from 
tcg/ppc? I can also just separate headers that can be ifdef'd away from 
the rest with a blank line, so it's more visible later

>
>>   #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.
>
-- 
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

On 5/18/21 10:05 AM, Bruno Larsen (billionai) wrote:
> -        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,

The mas_mask test is replicated, and the second version inside the ?: can be 
dropped.


r~