[PATCH v2 0/7] target/ppc: Misc motion to support disabling TCG

[PATCH v2 0/7] target/ppc: Misc motion to support disabling TCG

[Bruno Larsen (billionai)]

This patch series does a lot of the missing misc work to get the
disable-tcg flag working for the ppc architecture. most of the work is
code motion, with the notable exception of overhauling the logic of
dealing with fpscr.

The mmu logic overhaul has been removed from this patch series as it is
pretty complex, and this way we can test the patches a bit better. 

One caveat is that while testing the commits for regressions, one test
of the check-acceptance has failed on patch 2, but my automated script
didn't give me any info and I couldn't reproduce it manually. Given how
small that change is, I don't think that patch is the culprit, but
rather there is a non deterministic edge case that may cause a problem
in one of the acceptance tests.

based on dgibson's ppc-for-6.1 tree

Changelog for v2:
 * split the patch series
 * added a fix for 5d145639e, which no longer compiles with linux-user
 * removed patches ther were already accepted
 * applied rth's cleanup to ppc_store_sdr1
 * changed destination of ppc_store_msr
 * undone change to helper-proto, now fewer files include it

Bruno Larsen (billionai) (7):
  target/ppc: fix ppc_store_sdr1 for user-only compilation
  target/ppc: moved ppc_store_lpcr and ppc_store_msr to cpu.c
  target/ppc: reduce usage of fpscr_set_rounding_mode
  target/ppc: overhaul and moved logic of storing fpscr
  target/ppc: removed unnecessary inclusion of helper-proto.h
  target/ppc: moved ppc_cpu_do_interrupt to cpu.c
  target/ppc: wrapped some TCG only logic with ifdefs

 target/ppc/cpu.c         |  90 +++++++++++++-
 target/ppc/cpu.h         |  13 ++-
 target/ppc/cpu_init.c    |  17 +--
 target/ppc/excp_helper.c | 101 ++++++++--------
 target/ppc/fpu_helper.c  | 246 +++------------------------------------
 target/ppc/gdbstub.c     |   7 +-
 target/ppc/misc_helper.c |  16 ---
 target/ppc/mmu-hash32.c  |   1 -
 target/ppc/mmu-hash64.c  |   8 ++
 target/ppc/mmu-radix64.c |   1 -
 10 files changed, 181 insertions(+), 319 deletions(-)

-- 
2.17.1

[PATCH v2 1/7] target/ppc: fix ppc_store_sdr1 for user-only compilation

[Bruno Larsen (billionai)]

When the function was moved to cpu.c, it should have been wrapped with
 #if !defined(CONFIG_USER_ONLY), otherwise linux-user builds don't work.

The cleanup suggested by Richard Henderson has also been applied,
changing error_report(...) to qemu_log_mask(LOG_GUEST_ERROR, ...) in
that function.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 0ab7ac1af1..82e276349a 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -65,6 +65,7 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
     return env->vscr | (sat << VSCR_SAT);
 }
 
+#if !defined(CONFIG_USER_ONLY)
 void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
 {
     PowerPCCPU *cpu = env_archcpu(env);
@@ -76,13 +77,13 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
         target_ulong htabsize = value & SDR_64_HTABSIZE;
 
         if (value & ~sdr_mask) {
-            error_report("Invalid bits 0x"TARGET_FMT_lx" set in SDR1",
-                         value & ~sdr_mask);
+            qemu_log_mask(LOG_GUEST_ERROR, "Invalid bits 0x"TARGET_FMT_lx
+                     " set in SDR1", value & ~sdr_mask);
             value &= sdr_mask;
         }
         if (htabsize > 28) {
-            error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
-                         htabsize);
+            qemu_log_mask(LOG_GUEST_ERROR, "Invalid HTABSIZE 0x" TARGET_FMT_lx
+                     " stored in SDR1", htabsize);
             return;
         }
     }
@@ -90,3 +91,4 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
     /* FIXME: Should check for valid HTABMASK values in 32-bit case */
     env->spr[SPR_SDR1] = value;
 }
+#endif
-- 
2.17.1

Re: [PATCH v2 1/7] target/ppc: fix ppc_store_sdr1 for user-only compilation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

On Tue, May 18, 2021 at 12:05:09PM -0300, Bruno Larsen (billionai) wrote:
> When the function was moved to cpu.c, it should have been wrapped with
>  #if !defined(CONFIG_USER_ONLY), otherwise linux-user builds don't work.

I already corrected this in my tree, folding it into the original
change to avoid breaking bisect.

> The cleanup suggested by Richard Henderson has also been applied,
> changing error_report(...) to qemu_log_mask(LOG_GUEST_ERROR, ...) in
> that function.

That's a reasonable change, but doesn't belong with the compile fix.
Please rebase and update the commit message.

> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/cpu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index 0ab7ac1af1..82e276349a 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -65,6 +65,7 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
>      return env->vscr | (sat << VSCR_SAT);
>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
>  void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>  {
>      PowerPCCPU *cpu = env_archcpu(env);
> @@ -76,13 +77,13 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>          target_ulong htabsize = value & SDR_64_HTABSIZE;
>  
>          if (value & ~sdr_mask) {
> -            error_report("Invalid bits 0x"TARGET_FMT_lx" set in SDR1",
> -                         value & ~sdr_mask);
> +            qemu_log_mask(LOG_GUEST_ERROR, "Invalid bits 0x"TARGET_FMT_lx
> +                     " set in SDR1", value & ~sdr_mask);
>              value &= sdr_mask;
>          }
>          if (htabsize > 28) {
> -            error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> -                         htabsize);
> +            qemu_log_mask(LOG_GUEST_ERROR, "Invalid HTABSIZE 0x" TARGET_FMT_lx
> +                     " stored in SDR1", htabsize);
>              return;
>          }
>      }
> @@ -90,3 +91,4 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>      /* FIXME: Should check for valid HTABMASK values in 32-bit case */
>      env->spr[SPR_SDR1] = value;
>  }
> +#endif

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/7] target/ppc: moved ppc_store_lpcr and ppc_store_msr to cpu.c

[Bruno Larsen (billionai)]

These functions are used in hw/ppc logic, during machine startup, which
means it must be compiled when --disable-tcg is selected, and so it has
been moved into a common code file

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/cpu.c         | 17 +++++++++++++++++
 target/ppc/misc_helper.c | 16 ----------------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 82e276349a..8a39cba5be 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -24,6 +24,7 @@
 #include "exec/log.h"
 #include "fpu/softfloat-helpers.h"
 #include "mmu-hash64.h"
+#include "helper_regs.h"
 
 target_ulong cpu_read_xer(CPUPPCState *env)
 {
@@ -92,3 +93,19 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
     env->spr[SPR_SDR1] = value;
 }
 #endif
+
+/* GDBstub can read and write MSR... */
+void ppc_store_msr(CPUPPCState *env, target_ulong value)
+{
+    hreg_store_msr(env, value, 0);
+}
+
+void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    CPUPPCState *env = &cpu->env;
+
+    env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
+    /* The gtse bit affects hflags */
+    hreg_compute_hflags(env);
+}
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 08a31da289..442b12652c 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -255,22 +255,6 @@ target_ulong helper_clcs(CPUPPCState *env, uint32_t arg)
 /*****************************************************************************/
 /* Special registers manipulation */
 
-/* GDBstub can read and write MSR... */
-void ppc_store_msr(CPUPPCState *env, target_ulong value)
-{
-    hreg_store_msr(env, value, 0);
-}
-
-void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
-{
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-    CPUPPCState *env = &cpu->env;
-
-    env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
-    /* The gtse bit affects hflags */
-    hreg_compute_hflags(env);
-}
-
 /*
  * This code is lifted from MacOnLinux. It is called whenever THRM1,2
  * or 3 is read an fixes up the values in such a way that will make
-- 
2.17.1

[PATCH v2 3/7] target/ppc: reduce usage of fpscr_set_rounding_mode

[Bruno Larsen (billionai)]

It is preferable to store the current rounding mode and restore from that
than recalculating from fpscr, so we changed the behavior of do_fri and
VSX_ROUND to do it like that.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/fpu_helper.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 44315fca0b..4799d5f5e4 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -822,6 +822,7 @@ static inline uint64_t do_fri(CPUPPCState *env, uint64_t arg,
                               int rounding_mode)
 {
     CPU_DoubleU farg;
+    int old_rounding_mode = get_float_rounding_mode(&env->fp_status);
 
     farg.ll = arg;
 
@@ -834,8 +835,7 @@ static inline uint64_t do_fri(CPUPPCState *env, uint64_t arg,
                       float_flag_inexact;
         set_float_rounding_mode(rounding_mode, &env->fp_status);
         farg.ll = float64_round_to_int(farg.d, &env->fp_status);
-        /* Restore rounding mode from FPSCR */
-        fpscr_set_rounding_mode(env);
+        set_float_rounding_mode(old_rounding_mode, &env->fp_status);
 
         /* fri* does not set FPSCR[XX] */
         if (!inexact) {
@@ -3136,8 +3136,10 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb)       \
 {                                                                      \
     ppc_vsr_t t = *xt;                                                 \
     int i;                                                             \
+    int curr_rounding_mode;                                            \
                                                                        \
     if (rmode != FLOAT_ROUND_CURRENT) {                                \
+        curr_rounding_mode = get_float_rounding_mode(&env->fp_status); \
         set_float_rounding_mode(rmode, &env->fp_status);               \
     }                                                                  \
                                                                        \
@@ -3160,7 +3162,7 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb)       \
      * mode from FPSCR                                                 \
      */                                                                \
     if (rmode != FLOAT_ROUND_CURRENT) {                                \
-        fpscr_set_rounding_mode(env);                                  \
+        set_float_rounding_mode(curr_rounding_mode, &env->fp_status);  \
         env->fp_status.float_exception_flags &= ~float_flag_inexact;   \
     }                                                                  \
                                                                        \
-- 
2.17.1

Re: [PATCH v2 3/7] target/ppc: reduce usage of fpscr_set_rounding_mode

[Richard Henderson]

On 5/18/21 10:05 AM, Bruno Larsen (billionai) wrote:
> +    int old_rounding_mode = get_float_rounding_mode(&env->fp_status);

FloatRoundMode is a better type for the variable.

> @@ -3136,8 +3136,10 @@ void helper_##op(CPUPPCState *env, ppc_vsr_t *xt, ppc_vsr_t *xb)       \
>  {                                                                      \
>      ppc_vsr_t t = *xt;                                                 \
>      int i;                                                             \
> +    int curr_rounding_mode;                                            \

Likewise.  Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 4/7] target/ppc: overhauled and moved logic of storing fpscr

[Bruno Larsen (billionai)]

Followed the suggested overhaul to store_fpscr logic, and moved it to
cpu.c where it can be accessed in !TCG builds.

The overhaul was suggesting because storing a value to fpscr should
never raise an exception, so we could remove all the mess that happened
with POWERPC_EXCP_FP.

We also moved fpscr_set_rounding_mode into cpu.c as it could now be moved
there, and it is needed when a value for the fpscr is being stored
directly.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu.c        |  43 ++++++++
 target/ppc/cpu.h        |  12 +-
 target/ppc/fpu_helper.c | 238 +++-------------------------------------
 target/ppc/gdbstub.c    |   6 +-
 4 files changed, 65 insertions(+), 234 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 8a39cba5be..b0ac22b5ce 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -25,6 +25,7 @@
 #include "fpu/softfloat-helpers.h"
 #include "mmu-hash64.h"
 #include "helper_regs.h"
+#include "sysemu/tcg.h"
 
 target_ulong cpu_read_xer(CPUPPCState *env)
 {
@@ -109,3 +110,45 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
     /* The gtse bit affects hflags */
     hreg_compute_hflags(env);
 }
+
+static inline void fpscr_set_rounding_mode(CPUPPCState *env)
+{
+    int rnd_type;
+
+    /* Set rounding mode */
+    switch (fpscr_rn) {
+    case 0:
+        /* Best approximation (round to nearest) */
+        rnd_type = float_round_nearest_even;
+        break;
+    case 1:
+        /* Smaller magnitude (round toward zero) */
+        rnd_type = float_round_to_zero;
+        break;
+    case 2:
+        /* Round toward +infinite */
+        rnd_type = float_round_up;
+        break;
+    default:
+    case 3:
+        /* Round toward -infinite */
+        rnd_type = float_round_down;
+        break;
+    }
+    set_float_rounding_mode(rnd_type, &env->fp_status);
+}
+
+void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
+{
+    val &= ~(FP_VX | FP_FEX);
+    if (val & FPSCR_IX) {
+        val |= FP_VX;
+    }
+    if ((val >> FPSCR_XX) & (val >> FPSCR_XE) & 0x1f) {
+        val |= FP_FEX;
+    }
+    env->fpscr = val;
+    if (tcg_enabled()) {
+        fpscr_set_rounding_mode(env);
+    }
+}
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index f43ceec5cb..b9eb2690cf 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -679,11 +679,11 @@ enum {
 #define fpscr_ni     (((env->fpscr) >> FPSCR_NI)     & 0x1)
 #define fpscr_rn     (((env->fpscr) >> FPSCR_RN0)    & 0x3)
 /* Invalid operation exception summary */
-#define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) | (1 << FPSCR_VXISI)  | \
-                                  (1 << FPSCR_VXIDI)  | (1 << FPSCR_VXZDZ)  | \
-                                  (1 << FPSCR_VXIMZ)  | (1 << FPSCR_VXVC)   | \
-                                  (1 << FPSCR_VXSOFT) | (1 << FPSCR_VXSQRT) | \
-                                  (1 << FPSCR_VXCVI)))
+#define FPSCR_IX     ((1 << FPSCR_VXSNAN) | (1 << FPSCR_VXISI)  | \
+                      (1 << FPSCR_VXIDI)  | (1 << FPSCR_VXZDZ)  | \
+                      (1 << FPSCR_VXIMZ)  | (1 << FPSCR_VXVC)   | \
+                      (1 << FPSCR_VXSOFT) | (1 << FPSCR_VXSQRT) | \
+                      (1 << FPSCR_VXCVI))
 /* exception summary */
 #define fpscr_ex  (((env->fpscr) >> FPSCR_XX) & 0x1F)
 /* enabled exception summary */
@@ -1338,7 +1338,7 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
 #endif
 #endif
 
-void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
+void ppc_store_fpscr(CPUPPCState *env, target_ulong val);
 void helper_hfscr_facility_check(CPUPPCState *env, uint32_t bit,
                                  const char *caller, uint32_t cause);
 
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 4799d5f5e4..cc03f8d16a 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -383,247 +383,35 @@ static inline void float_inexact_excp(CPUPPCState *env)
     }
 }
 
-static inline void fpscr_set_rounding_mode(CPUPPCState *env)
-{
-    int rnd_type;
-
-    /* Set rounding mode */
-    switch (fpscr_rn) {
-    case 0:
-        /* Best approximation (round to nearest) */
-        rnd_type = float_round_nearest_even;
-        break;
-    case 1:
-        /* Smaller magnitude (round toward zero) */
-        rnd_type = float_round_to_zero;
-        break;
-    case 2:
-        /* Round toward +infinite */
-        rnd_type = float_round_up;
-        break;
-    default:
-    case 3:
-        /* Round toward -infinite */
-        rnd_type = float_round_down;
-        break;
-    }
-    set_float_rounding_mode(rnd_type, &env->fp_status);
-}
-
 void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
 {
-    int prev;
-
-    prev = (env->fpscr >> bit) & 1;
-    env->fpscr &= ~(1 << bit);
-    if (prev == 1) {
-        switch (bit) {
-        case FPSCR_RN1:
-        case FPSCR_RN0:
-            fpscr_set_rounding_mode(env);
-            break;
-        case FPSCR_VXSNAN:
-        case FPSCR_VXISI:
-        case FPSCR_VXIDI:
-        case FPSCR_VXZDZ:
-        case FPSCR_VXIMZ:
-        case FPSCR_VXVC:
-        case FPSCR_VXSOFT:
-        case FPSCR_VXSQRT:
-        case FPSCR_VXCVI:
-            if (!fpscr_ix) {
-                /* Set VX bit to zero */
-                env->fpscr &= ~FP_VX;
-            }
-            break;
-        case FPSCR_OX:
-        case FPSCR_UX:
-        case FPSCR_ZX:
-        case FPSCR_XX:
-        case FPSCR_VE:
-        case FPSCR_OE:
-        case FPSCR_UE:
-        case FPSCR_ZE:
-        case FPSCR_XE:
-            if (!fpscr_eex) {
-                /* Set the FEX bit */
-                env->fpscr &= ~FP_FEX;
-            }
-            break;
-        default:
-            break;
-        }
+    uint32_t mask = 1u << bit;
+    if (env->fpscr & mask) {
+        ppc_store_fpscr(env, env->fpscr & ~mask);
     }
 }
 
 void helper_fpscr_setbit(CPUPPCState *env, uint32_t bit)
 {
-    CPUState *cs = env_cpu(env);
-    int prev;
-
-    prev = (env->fpscr >> bit) & 1;
-    env->fpscr |= 1 << bit;
-    if (prev == 0) {
-        switch (bit) {
-        case FPSCR_VX:
-            env->fpscr |= FP_FX;
-            if (fpscr_ve) {
-                goto raise_ve;
-            }
-            break;
-        case FPSCR_OX:
-            env->fpscr |= FP_FX;
-            if (fpscr_oe) {
-                goto raise_oe;
-            }
-            break;
-        case FPSCR_UX:
-            env->fpscr |= FP_FX;
-            if (fpscr_ue) {
-                goto raise_ue;
-            }
-            break;
-        case FPSCR_ZX:
-            env->fpscr |= FP_FX;
-            if (fpscr_ze) {
-                goto raise_ze;
-            }
-            break;
-        case FPSCR_XX:
-            env->fpscr |= FP_FX;
-            if (fpscr_xe) {
-                goto raise_xe;
-            }
-            break;
-        case FPSCR_VXSNAN:
-        case FPSCR_VXISI:
-        case FPSCR_VXIDI:
-        case FPSCR_VXZDZ:
-        case FPSCR_VXIMZ:
-        case FPSCR_VXVC:
-        case FPSCR_VXSOFT:
-        case FPSCR_VXSQRT:
-        case FPSCR_VXCVI:
-            env->fpscr |= FP_VX;
-            env->fpscr |= FP_FX;
-            if (fpscr_ve != 0) {
-                goto raise_ve;
-            }
-            break;
-        case FPSCR_VE:
-            if (fpscr_vx != 0) {
-            raise_ve:
-                env->error_code = POWERPC_EXCP_FP;
-                if (fpscr_vxsnan) {
-                    env->error_code |= POWERPC_EXCP_FP_VXSNAN;
-                }
-                if (fpscr_vxisi) {
-                    env->error_code |= POWERPC_EXCP_FP_VXISI;
-                }
-                if (fpscr_vxidi) {
-                    env->error_code |= POWERPC_EXCP_FP_VXIDI;
-                }
-                if (fpscr_vxzdz) {
-                    env->error_code |= POWERPC_EXCP_FP_VXZDZ;
-                }
-                if (fpscr_vximz) {
-                    env->error_code |= POWERPC_EXCP_FP_VXIMZ;
-                }
-                if (fpscr_vxvc) {
-                    env->error_code |= POWERPC_EXCP_FP_VXVC;
-                }
-                if (fpscr_vxsoft) {
-                    env->error_code |= POWERPC_EXCP_FP_VXSOFT;
-                }
-                if (fpscr_vxsqrt) {
-                    env->error_code |= POWERPC_EXCP_FP_VXSQRT;
-                }
-                if (fpscr_vxcvi) {
-                    env->error_code |= POWERPC_EXCP_FP_VXCVI;
-                }
-                goto raise_excp;
-            }
-            break;
-        case FPSCR_OE:
-            if (fpscr_ox != 0) {
-            raise_oe:
-                env->error_code = POWERPC_EXCP_FP | POWERPC_EXCP_FP_OX;
-                goto raise_excp;
-            }
-            break;
-        case FPSCR_UE:
-            if (fpscr_ux != 0) {
-            raise_ue:
-                env->error_code = POWERPC_EXCP_FP | POWERPC_EXCP_FP_UX;
-                goto raise_excp;
-            }
-            break;
-        case FPSCR_ZE:
-            if (fpscr_zx != 0) {
-            raise_ze:
-                env->error_code = POWERPC_EXCP_FP | POWERPC_EXCP_FP_ZX;
-                goto raise_excp;
-            }
-            break;
-        case FPSCR_XE:
-            if (fpscr_xx != 0) {
-            raise_xe:
-                env->error_code = POWERPC_EXCP_FP | POWERPC_EXCP_FP_XX;
-                goto raise_excp;
-            }
-            break;
-        case FPSCR_RN1:
-        case FPSCR_RN0:
-            fpscr_set_rounding_mode(env);
-            break;
-        default:
-            break;
-        raise_excp:
-            /* Update the floating-point enabled exception summary */
-            env->fpscr |= FP_FEX;
-            /* We have to update Rc1 before raising the exception */
-            cs->exception_index = POWERPC_EXCP_PROGRAM;
-            break;
-        }
+    uint32_t mask = 1u << bit;
+    if (!(env->fpscr & mask)) {
+        ppc_store_fpscr(env, env->fpscr | mask);
     }
 }
 
-void helper_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
+void helper_store_fpscr(CPUPPCState *env, uint64_t val, uint32_t nibbles)
 {
-    CPUState *cs = env_cpu(env);
-    target_ulong prev, new;
+    target_ulong mask = 0;
     int i;
 
-    prev = env->fpscr;
-    new = (target_ulong)arg;
-    new &= ~(FP_FEX | FP_VX);
-    new |= prev & (FP_FEX | FP_VX);
+    /* TODO: push this extension back to translation time */
     for (i = 0; i < sizeof(target_ulong) * 2; i++) {
-        if (mask & (1 << i)) {
-            env->fpscr &= ~(0xFLL << (4 * i));
-            env->fpscr |= new & (0xFLL << (4 * i));
+        if (nibbles & (1 << i)) {
+            mask |= (target_ulong) 0xf << (4 * i);
         }
     }
-    /* Update VX and FEX */
-    if (fpscr_ix != 0) {
-        env->fpscr |= FP_VX;
-    } else {
-        env->fpscr &= ~FP_VX;
-    }
-    if ((fpscr_ex & fpscr_eex) != 0) {
-        env->fpscr |= FP_FEX;
-        cs->exception_index = POWERPC_EXCP_PROGRAM;
-        /* XXX: we should compute it properly */
-        env->error_code = POWERPC_EXCP_FP;
-    } else {
-        env->fpscr &= ~FP_FEX;
-    }
-    fpscr_set_rounding_mode(env);
-}
-
-void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
-{
-    helper_store_fpscr(env, arg, mask);
+    val = (val & mask) | (env->fpscr & ~mask);
+    ppc_store_fpscr(env, val);
 }
 
 static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 9339e7eafe..c7d866cfcc 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -272,7 +272,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
             break;
         case 70:
             /* fpscr */
-            store_fpscr(env, ldtul_p(mem_buf), 0xffffffff);
+            ppc_store_fpscr(env, ldtul_p(mem_buf));
             break;
         }
     }
@@ -322,7 +322,7 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
             break;
         case 70 + 32:
             /* fpscr */
-            store_fpscr(env, ldq_p(mem_buf), 0xffffffff);
+            ppc_store_fpscr(env, ldq_p(mem_buf));
             break;
         }
     }
@@ -475,7 +475,7 @@ static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
     }
     if (n == 32) {
         ppc_maybe_bswap_register(env, mem_buf, 4);
-        store_fpscr(env, ldl_p(mem_buf), 0xffffffff);
+        ppc_store_fpscr(env, ldl_p(mem_buf));
         return 4;
     }
     return 0;
-- 
2.17.1

Re: [PATCH v2 4/7] target/ppc: overhauled and moved logic of storing fpscr

[Richard Henderson]

On 5/18/21 10:05 AM, Bruno Larsen (billionai) wrote:
> Followed the suggested overhaul to store_fpscr logic, and moved it to
> cpu.c where it can be accessed in !TCG builds.
> 
> The overhaul was suggesting because storing a value to fpscr should
> never raise an exception, so we could remove all the mess that happened
> with POWERPC_EXCP_FP.
> 
> We also moved fpscr_set_rounding_mode into cpu.c as it could now be moved
> there, and it is needed when a value for the fpscr is being stored
> directly.
> 
> Suggested-by: Richard Henderson<richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/cpu.c        |  43 ++++++++
>   target/ppc/cpu.h        |  12 +-
>   target/ppc/fpu_helper.c | 238 +++-------------------------------------
>   target/ppc/gdbstub.c    |   6 +-
>   4 files changed, 65 insertions(+), 234 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 5/7] target/ppc: removed unnecessary inclusion of helper-proto.h

[Bruno Larsen (billionai)]

These files included helper-proto.h, but didn't use or declare any
helpers, so the #include has been removed

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu_init.c    | 1 -
 target/ppc/gdbstub.c     | 1 -
 target/ppc/mmu-hash32.c  | 1 -
 target/ppc/mmu-radix64.c | 1 -
 4 files changed, 4 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index d0fa219880..88a8344eea 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -43,7 +43,6 @@
 #include "fpu/softfloat.h"
 #include "qapi/qapi-commands-machine-target.h"
 
-#include "exec/helper-proto.h"
 #include "helper_regs.h"
 #include "internal.h"
 #include "spr_tcg.h"
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index c7d866cfcc..09ff1328d4 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -20,7 +20,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "exec/gdbstub.h"
-#include "exec/helper-proto.h"
 #include "internal.h"
 
 static int ppc_gdb_register_len_apple(int n)
diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
index 178cf090b7..3d693a4fd4 100644
--- a/target/ppc/mmu-hash32.c
+++ b/target/ppc/mmu-hash32.c
@@ -21,7 +21,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
-#include "exec/helper-proto.h"
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "mmu-hash32.h"
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 30fcfcf11f..a6de6bf606 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -20,7 +20,6 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
-#include "exec/helper-proto.h"
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
-- 
2.17.1

Re: [PATCH v2 5/7] target/ppc: removed unnecessary inclusion of helper-proto.h

[Richard Henderson]

On 5/18/21 10:05 AM, Bruno Larsen (billionai) wrote:
> These files included helper-proto.h, but didn't use or declare any
> helpers, so the #include has been removed
> 
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/cpu_init.c    | 1 -
>   target/ppc/gdbstub.c     | 1 -
>   target/ppc/mmu-hash32.c  | 1 -
>   target/ppc/mmu-radix64.c | 1 -
>   4 files changed, 4 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 6/7] target/ppc: moved ppc_cpu_do_interrupt to cpu.c

[Bruno Larsen (billionai)]

Moved the ppc_cpu_do_interrupt function to cpu.c file, where it makes
more sense, and turned powerpc_excp not static, as it now needs to be
accessed from outside of excp_helper.c

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu.c         | 20 ++++++++++++++++++++
 target/ppc/cpu.h         |  1 +
 target/ppc/excp_helper.c | 19 +------------------
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index a00ff2d5a3..eb4b705089 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -152,3 +152,23 @@ void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
         fpscr_set_rounding_mode(env);
     }
 }
+
+/* Exception processing */
+#if defined(CONFIG_USER_ONLY)
+void ppc_cpu_do_interrupt(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    cs->exception_index = POWERPC_EXCP_NONE;
+    env->error_code = 0;
+}
+#else
+void ppc_cpu_do_interrupt(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    powerpc_excp(cpu, env->excp_model, cs->exception_index);
+}
+#endif
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index b9eb2690cf..01dac2b12d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1258,6 +1258,7 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass,
 #endif /* CONFIG_USER_ONLY */
 
 void ppc_cpu_do_interrupt(CPUState *cpu);
+void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp);
 bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
 void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index f4f15279eb..80bb6e70e9 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -38,15 +38,6 @@
 /*****************************************************************************/
 /* Exception processing */
 #if defined(CONFIG_USER_ONLY)
-void ppc_cpu_do_interrupt(CPUState *cs)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-
-    cs->exception_index = POWERPC_EXCP_NONE;
-    env->error_code = 0;
-}
-
 static void ppc_hw_interrupt(CPUPPCState *env)
 {
     CPUState *cs = env_cpu(env);
@@ -324,7 +315,7 @@ static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
  * Note that this function should be greatly optimized when called
  * with a constant excp, from ppc_hw_interrupt
  */
-static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
+inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
@@ -968,14 +959,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     powerpc_set_excp_state(cpu, vector, new_msr);
 }
 
-void ppc_cpu_do_interrupt(CPUState *cs)
-{
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-
-    powerpc_excp(cpu, env->excp_model, cs->exception_index);
-}
-
 static void ppc_hw_interrupt(CPUPPCState *env)
 {
     PowerPCCPU *cpu = env_archcpu(env);
-- 
2.17.1

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

[Bruno Larsen (billionai)]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4153 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bruno Piazera Larsen]

[-- Attachment #1: Type: text/plain, Size: 3793 bytes --]


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>

[-- Attachment #2: Type: text/html, Size: 5153 bytes --]

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

[Richard Henderson]

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~