[RFC PATCH 00/11] target/ppc: add support to disable-tcg

[RFC PATCH 00/11] target/ppc: add support to disable-tcg

[Bruno Larsen (billionai)]

This is a new version of the patch series that adds support for the
disable-tcg build flag. It is not marked as v2 because so much has
changed between them that it didn't feel like it made sense to relate to
that first RFC.

Most of the patches here are meant to be final, but 2 of them near the
end are a bit of a shot in the dark, so we figured RFC would be a better
way to tag this patch series.

Bruno Larsen (billionai) (10):
  target/ppc: created ppc_{store,get}_vscr for generic vscr usage
  target/ppc: moved ppc_store_sdr1 to cpu.c
  target/ppc: moved ppc_cpu_dump_state to cpu_init.c
  target/ppc: moved ppc_store_msr into gdbstub.c
  target/ppc: moved ppc_store_lpcr to cpu.c
  target/ppc: updated vscr manipulation in machine.c
  target/ppc: added KVM fallback to fpscr manipulation
  target/ppc: wrapped some TCG only logic with ifdefs
  target/ppc: created tcg-stub.c file
  target/ppc: updated meson.build to support disable-tcg

Lucas Mateus Castro (alqotel) (1):
  include/exec: added functions to the stubs in exec-all.h

 include/exec/exec-all.h     |  10 ++
 include/exec/helper-proto.h |   2 +
 target/ppc/arch_dump.c      |   3 +-
 target/ppc/cpu.c            |  56 +++++++++++
 target/ppc/cpu.h            |   2 +
 target/ppc/cpu_init.c       | 192 +++++++++++++++++++++++++++++++++++-
 target/ppc/excp_helper.c    |   6 +-
 target/ppc/gdbstub.c        |  12 ++-
 target/ppc/int_helper.c     |   9 +-
 target/ppc/kvm.c            |  14 +++
 target/ppc/kvm_ppc.h        |   6 ++
 target/ppc/machine.c        |   7 +-
 target/ppc/meson.build      |  16 ++-
 target/ppc/misc_helper.c    |  16 ---
 target/ppc/mmu_helper.c     |  26 -----
 target/ppc/tcg-stub.c       |  33 +++++++
 target/ppc/translate.c      | 187 -----------------------------------
 17 files changed, 347 insertions(+), 250 deletions(-)
 create mode 100644 target/ppc/tcg-stub.c

-- 
2.17.1

[PATCH 01/11] target/ppc: created ppc_{store, get}_vscr for generic vscr usage

[Bruno Larsen (billionai)]

Some functions unrelated to TCG use helper_m{t,f}vscr, so generic versions
of those functions were added to cpu.c, in preparation for compilation
without TCG

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/arch_dump.c  |  3 +--
 target/ppc/cpu.c        | 16 ++++++++++++++++
 target/ppc/cpu.h        |  2 ++
 target/ppc/cpu_init.c   |  2 +-
 target/ppc/gdbstub.c    |  4 ++--
 target/ppc/int_helper.c |  9 ++-------
 6 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
index 9ab04b2c38..9210e61ef4 100644
--- a/target/ppc/arch_dump.c
+++ b/target/ppc/arch_dump.c
@@ -17,7 +17,6 @@
 #include "elf.h"
 #include "sysemu/dump.h"
 #include "sysemu/kvm.h"
-#include "exec/helper-proto.h"
 
 #ifdef TARGET_PPC64
 #define ELFCLASS ELFCLASS64
@@ -176,7 +175,7 @@ static void ppc_write_elf_vmxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
             vmxregset->avr[i].u64[1] = avr->u64[1];
         }
     }
-    vmxregset->vscr.u32[3] = cpu_to_dump32(s, helper_mfvscr(&cpu->env));
+    vmxregset->vscr.u32[3] = cpu_to_dump32(s, ppc_get_vscr(&cpu->env));
 }
 
 static void ppc_write_elf_vsxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index e501a7ff6f..cb794e9f4f 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "cpu-models.h"
+#include "fpu/softfloat-helpers.h"
 
 target_ulong cpu_read_xer(CPUPPCState *env)
 {
@@ -45,3 +46,18 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer)
                        (1ul << XER_OV) | (1ul << XER_CA) |
                        (1ul << XER_OV32) | (1ul << XER_CA32));
 }
+
+void ppc_store_vscr(CPUPPCState *env, uint32_t vscr)
+{
+    env->vscr = vscr & ~(1u << VSCR_SAT);
+    /* Which bit we set is completely arbitrary, but clear the rest.  */
+    env->vscr_sat.u64[0] = vscr & (1u << VSCR_SAT);
+    env->vscr_sat.u64[1] = 0;
+    set_flush_to_zero((vscr >> VSCR_NJ) & 1, &env->vec_status);
+}
+
+uint32_t ppc_get_vscr(CPUPPCState *env)
+{
+    uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0;
+    return env->vscr | (sat << VSCR_SAT);
+}
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 98fcf1c4d6..f43ceec5cb 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2651,4 +2651,6 @@ static inline bool ppc_has_spr(PowerPCCPU *cpu, int spr)
 void dump_mmu(CPUPPCState *env);
 
 void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
+void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
+uint32_t ppc_get_vscr(CPUPPCState *env);
 #endif /* PPC_CPU_H */
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index faece1dca2..b4a2d15c6a 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -55,7 +55,7 @@ static inline void vscr_init(CPUPPCState *env, uint32_t val)
 {
     /* Altivec always uses round-to-nearest */
     set_float_rounding_mode(float_round_nearest_even, &env->vec_status);
-    helper_mtvscr(env, val);
+    ppc_store_vscr(env, val);
 }
 
 /**
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 94a7273ee0..9339e7eafe 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -498,7 +498,7 @@ static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
         return 16;
     }
     if (n == 32) {
-        gdb_get_reg32(buf, helper_mfvscr(env));
+        gdb_get_reg32(buf, ppc_get_vscr(env));
         mem_buf = gdb_get_reg_ptr(buf, 4);
         ppc_maybe_bswap_register(env, mem_buf, 4);
         return 4;
@@ -529,7 +529,7 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
     }
     if (n == 32) {
         ppc_maybe_bswap_register(env, mem_buf, 4);
-        helper_mtvscr(env, ldl_p(mem_buf));
+        ppc_store_vscr(env, ldl_p(mem_buf));
         return 4;
     }
     if (n == 33) {
diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index a44c2d90ea..41f8477d4b 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -462,17 +462,12 @@ SATCVT(sd, uw, int64_t, uint32_t, 0, UINT32_MAX)
 
 void helper_mtvscr(CPUPPCState *env, uint32_t vscr)
 {
-    env->vscr = vscr & ~(1u << VSCR_SAT);
-    /* Which bit we set is completely arbitrary, but clear the rest.  */
-    env->vscr_sat.u64[0] = vscr & (1u << VSCR_SAT);
-    env->vscr_sat.u64[1] = 0;
-    set_flush_to_zero((vscr >> VSCR_NJ) & 1, &env->vec_status);
+    ppc_store_vscr(env, vscr);
 }
 
 uint32_t helper_mfvscr(CPUPPCState *env)
 {
-    uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0;
-    return env->vscr | (sat << VSCR_SAT);
+    return ppc_get_vscr(env);
 }
 
 static inline void set_vscr_sat(CPUPPCState *env)
-- 
2.17.1

Re: [PATCH 01/11] target/ppc: created ppc_{store,get}_vscr for generic vscr usage

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> Some functions unrelated to TCG use helper_m{t,f}vscr, so generic versions
> of those functions were added to cpu.c, in preparation for compilation
> without TCG
> 
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/arch_dump.c  |  3 +--
>   target/ppc/cpu.c        | 16 ++++++++++++++++
>   target/ppc/cpu.h        |  2 ++
>   target/ppc/cpu_init.c   |  2 +-
>   target/ppc/gdbstub.c    |  4 ++--
>   target/ppc/int_helper.c |  9 ++-------
>   6 files changed, 24 insertions(+), 12 deletions(-)

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

r~

Re: [PATCH 01/11] target/ppc: created ppc_{store,get}_vscr for generic vscr usage

[David Gibson]

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

On Wed, May 12, 2021 at 11:08:03AM -0300, Bruno Larsen (billionai) wrote:
> Some functions unrelated to TCG use helper_m{t,f}vscr, so generic versions
> of those functions were added to cpu.c, in preparation for compilation
> without TCG
> 
> Signed-off-by: Bruno Larsen (billionai)
> <bruno.larsen@eldorado.org.br>

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/arch_dump.c  |  3 +--
>  target/ppc/cpu.c        | 16 ++++++++++++++++
>  target/ppc/cpu.h        |  2 ++
>  target/ppc/cpu_init.c   |  2 +-
>  target/ppc/gdbstub.c    |  4 ++--
>  target/ppc/int_helper.c |  9 ++-------
>  6 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index 9ab04b2c38..9210e61ef4 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -17,7 +17,6 @@
>  #include "elf.h"
>  #include "sysemu/dump.h"
>  #include "sysemu/kvm.h"
> -#include "exec/helper-proto.h"
>  
>  #ifdef TARGET_PPC64
>  #define ELFCLASS ELFCLASS64
> @@ -176,7 +175,7 @@ static void ppc_write_elf_vmxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
>              vmxregset->avr[i].u64[1] = avr->u64[1];
>          }
>      }
> -    vmxregset->vscr.u32[3] = cpu_to_dump32(s, helper_mfvscr(&cpu->env));
> +    vmxregset->vscr.u32[3] = cpu_to_dump32(s, ppc_get_vscr(&cpu->env));
>  }
>  
>  static void ppc_write_elf_vsxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index e501a7ff6f..cb794e9f4f 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -20,6 +20,7 @@
>  #include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "cpu-models.h"
> +#include "fpu/softfloat-helpers.h"
>  
>  target_ulong cpu_read_xer(CPUPPCState *env)
>  {
> @@ -45,3 +46,18 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer)
>                         (1ul << XER_OV) | (1ul << XER_CA) |
>                         (1ul << XER_OV32) | (1ul << XER_CA32));
>  }
> +
> +void ppc_store_vscr(CPUPPCState *env, uint32_t vscr)
> +{
> +    env->vscr = vscr & ~(1u << VSCR_SAT);
> +    /* Which bit we set is completely arbitrary, but clear the rest.  */
> +    env->vscr_sat.u64[0] = vscr & (1u << VSCR_SAT);
> +    env->vscr_sat.u64[1] = 0;
> +    set_flush_to_zero((vscr >> VSCR_NJ) & 1, &env->vec_status);
> +}
> +
> +uint32_t ppc_get_vscr(CPUPPCState *env)
> +{
> +    uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0;
> +    return env->vscr | (sat << VSCR_SAT);
> +}
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 98fcf1c4d6..f43ceec5cb 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2651,4 +2651,6 @@ static inline bool ppc_has_spr(PowerPCCPU *cpu, int spr)
>  void dump_mmu(CPUPPCState *env);
>  
>  void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
> +void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
> +uint32_t ppc_get_vscr(CPUPPCState *env);
>  #endif /* PPC_CPU_H */
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index faece1dca2..b4a2d15c6a 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -55,7 +55,7 @@ static inline void vscr_init(CPUPPCState *env, uint32_t val)
>  {
>      /* Altivec always uses round-to-nearest */
>      set_float_rounding_mode(float_round_nearest_even, &env->vec_status);
> -    helper_mtvscr(env, val);
> +    ppc_store_vscr(env, val);
>  }
>  
>  /**
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 94a7273ee0..9339e7eafe 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -498,7 +498,7 @@ static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
>          return 16;
>      }
>      if (n == 32) {
> -        gdb_get_reg32(buf, helper_mfvscr(env));
> +        gdb_get_reg32(buf, ppc_get_vscr(env));
>          mem_buf = gdb_get_reg_ptr(buf, 4);
>          ppc_maybe_bswap_register(env, mem_buf, 4);
>          return 4;
> @@ -529,7 +529,7 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>      }
>      if (n == 32) {
>          ppc_maybe_bswap_register(env, mem_buf, 4);
> -        helper_mtvscr(env, ldl_p(mem_buf));
> +        ppc_store_vscr(env, ldl_p(mem_buf));
>          return 4;
>      }
>      if (n == 33) {
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index a44c2d90ea..41f8477d4b 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -462,17 +462,12 @@ SATCVT(sd, uw, int64_t, uint32_t, 0, UINT32_MAX)
>  
>  void helper_mtvscr(CPUPPCState *env, uint32_t vscr)
>  {
> -    env->vscr = vscr & ~(1u << VSCR_SAT);
> -    /* Which bit we set is completely arbitrary, but clear the rest.  */
> -    env->vscr_sat.u64[0] = vscr & (1u << VSCR_SAT);
> -    env->vscr_sat.u64[1] = 0;
> -    set_flush_to_zero((vscr >> VSCR_NJ) & 1, &env->vec_status);
> +    ppc_store_vscr(env, vscr);
>  }
>  
>  uint32_t helper_mfvscr(CPUPPCState *env)
>  {
> -    uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0;
> -    return env->vscr | (sat << VSCR_SAT);
> +    return ppc_get_vscr(env);
>  }
>  
>  static inline void set_vscr_sat(CPUPPCState *env)

-- 
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 02/11] target/ppc: moved ppc_store_sdr1 to cpu.c

[Bruno Larsen (billionai)]

Moved this function that is required in !TCG cases into a
common code file

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu.c        | 29 +++++++++++++++++++++++++++++
 target/ppc/mmu_helper.c | 26 --------------------------
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index cb794e9f4f..0ab7ac1af1 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -20,7 +20,10 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "cpu-models.h"
+#include "cpu-qom.h"
+#include "exec/log.h"
 #include "fpu/softfloat-helpers.h"
+#include "mmu-hash64.h"
 
 target_ulong cpu_read_xer(CPUPPCState *env)
 {
@@ -61,3 +64,29 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
     uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0;
     return env->vscr | (sat << VSCR_SAT);
 }
+
+void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
+{
+    PowerPCCPU *cpu = env_archcpu(env);
+    qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
+    assert(!cpu->vhyp);
+#if defined(TARGET_PPC64)
+    if (mmu_is_64bit(env->mmu_model)) {
+        target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
+        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);
+            value &= sdr_mask;
+        }
+        if (htabsize > 28) {
+            error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
+                         htabsize);
+            return;
+        }
+    }
+#endif /* defined(TARGET_PPC64) */
+    /* FIXME: Should check for valid HTABMASK values in 32-bit case */
+    env->spr[SPR_SDR1] = value;
+}
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index ca88658cba..06e1ebdcbc 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -2085,32 +2085,6 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
 
 /*****************************************************************************/
 /* Special registers manipulation */
-void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
-{
-    PowerPCCPU *cpu = env_archcpu(env);
-    qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
-    assert(!cpu->vhyp);
-#if defined(TARGET_PPC64)
-    if (mmu_is_64bit(env->mmu_model)) {
-        target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
-        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);
-            value &= sdr_mask;
-        }
-        if (htabsize > 28) {
-            error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
-                         htabsize);
-            return;
-        }
-    }
-#endif /* defined(TARGET_PPC64) */
-    /* FIXME: Should check for valid HTABMASK values in 32-bit case */
-    env->spr[SPR_SDR1] = value;
-}
-
 #if defined(TARGET_PPC64)
 void ppc_store_ptcr(CPUPPCState *env, target_ulong value)
 {
-- 
2.17.1

Re: [PATCH 02/11] target/ppc: moved ppc_store_sdr1 to cpu.c

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> Moved this function that is required in !TCG cases into a
> common code file
> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>

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


> +void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
> +{
> +    PowerPCCPU *cpu = env_archcpu(env);
> +    qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
> +    assert(!cpu->vhyp);
> +#if defined(TARGET_PPC64)
> +    if (mmu_is_64bit(env->mmu_model)) {
> +        target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
> +        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);

As a separate cleanup patch, this should not use error_report but 
qemu_log(LOG_GUEST_ERROR, ...).


r~

Re: [PATCH 02/11] target/ppc: moved ppc_store_sdr1 to cpu.c

[David Gibson]

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

On Wed, May 12, 2021 at 11:08:04AM -0300, Bruno Larsen (billionai) wrote:
> Moved this function that is required in !TCG cases into a
> common code file

The reasons it's needed by !TCG are kind of bogus, related to
weirdness in the way KVM PR works.  But it's fair not to care about
that right now, so, applied to ppc-for-6.1.

> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/cpu.c        | 29 +++++++++++++++++++++++++++++
>  target/ppc/mmu_helper.c | 26 --------------------------
>  2 files changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index cb794e9f4f..0ab7ac1af1 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -20,7 +20,10 @@
>  #include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "cpu-models.h"
> +#include "cpu-qom.h"
> +#include "exec/log.h"
>  #include "fpu/softfloat-helpers.h"
> +#include "mmu-hash64.h"
>  
>  target_ulong cpu_read_xer(CPUPPCState *env)
>  {
> @@ -61,3 +64,29 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
>      uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0;
>      return env->vscr | (sat << VSCR_SAT);
>  }
> +
> +void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
> +{
> +    PowerPCCPU *cpu = env_archcpu(env);
> +    qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
> +    assert(!cpu->vhyp);
> +#if defined(TARGET_PPC64)
> +    if (mmu_is_64bit(env->mmu_model)) {
> +        target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
> +        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);
> +            value &= sdr_mask;
> +        }
> +        if (htabsize > 28) {
> +            error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> +                         htabsize);
> +            return;
> +        }
> +    }
> +#endif /* defined(TARGET_PPC64) */
> +    /* FIXME: Should check for valid HTABMASK values in 32-bit case */
> +    env->spr[SPR_SDR1] = value;
> +}
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index ca88658cba..06e1ebdcbc 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2085,32 +2085,6 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
>  
>  /*****************************************************************************/
>  /* Special registers manipulation */
> -void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
> -{
> -    PowerPCCPU *cpu = env_archcpu(env);
> -    qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
> -    assert(!cpu->vhyp);
> -#if defined(TARGET_PPC64)
> -    if (mmu_is_64bit(env->mmu_model)) {
> -        target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
> -        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);
> -            value &= sdr_mask;
> -        }
> -        if (htabsize > 28) {
> -            error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
> -                         htabsize);
> -            return;
> -        }
> -    }
> -#endif /* defined(TARGET_PPC64) */
> -    /* FIXME: Should check for valid HTABMASK values in 32-bit case */
> -    env->spr[SPR_SDR1] = value;
> -}
> -
>  #if defined(TARGET_PPC64)
>  void ppc_store_ptcr(CPUPPCState *env, target_ulong value)
>  {

-- 
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 02/11] target/ppc: moved ppc_store_sdr1 to cpu.c

[Bruno Piazera Larsen]

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


On 13/05/2021 00:50, David Gibson wrote:
> On Wed, May 12, 2021 at 11:08:04AM -0300, Bruno Larsen (billionai) wrote:
>> Moved this function that is required in !TCG cases into a
>> common code file
> The reasons it's needed by !TCG are kind of bogus, related to
> weirdness in the way KVM PR works.  But it's fair not to care about
> that right now, so, applied to ppc-for-6.1.
Now that the future is here, I was looking into why might the reasons be 
bogus. From what I can see, what should be happening is just storing 
what was retrieved by the kvm ioctl, right? Am I missing something?
--
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: 1560 bytes --]

Re: [PATCH 02/11] target/ppc: moved ppc_store_sdr1 to cpu.c

[David Gibson]

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

On Mon, May 31, 2021 at 03:17:47PM -0300, Bruno Piazera Larsen wrote:
> 
> On 13/05/2021 00:50, David Gibson wrote:
> > On Wed, May 12, 2021 at 11:08:04AM -0300, Bruno Larsen (billionai) wrote:
> > > Moved this function that is required in !TCG cases into a
> > > common code file
> > The reasons it's needed by !TCG are kind of bogus, related to
> > weirdness in the way KVM PR works.  But it's fair not to care about
> > that right now, so, applied to ppc-for-6.1.
> Now that the future is here, I was looking into why might the reasons be
> bogus. From what I can see, what should be happening is just storing what
> was retrieved by the kvm ioctl, right? Am I missing something?

Actually, I was mixing this up with something else.  We invoke
ppc_store_sdr1() in several places from !TCG code.  From explicitly
KVM code in kvmppc_book_get_sregs() - since we more or less trust KVM
we could in theory just store directly to the value.  However we also
use it in common code in machine.c in the loadvm path.  I don't want
to bypass ppc_store_sdr1() there so that we have a common place for
all SDR1 updates, for sanity checking and any secondary alterations we
need.

So having this in common code is the right thing to do.

(In case you care, the "bogus reason" thing I was thinking of in
connection to SDR1 handling is actually the special
encode_hpt_for_kvm_pr() case in kvmppc_but_books_sregs.  That's there
because KVM PR requires us to inform us of the *qemu userspace*
location of the hash table for PAPR guests via the SDR1 value, which
doesn't really make sense)

-- 
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 03/11] target/ppc: moved ppc_cpu_dump_state to cpu_init.c

[Bruno Larsen (billionai)]

This function was forgotten in the cpu_init code motion series, but it
seems to be used regardless of TCG, and so needs to be moved to support
disabling TCG.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu_init.c  | 182 +++++++++++++++++++++++++++++++++++++++
 target/ppc/translate.c | 187 -----------------------------------------
 2 files changed, 182 insertions(+), 187 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index b4a2d15c6a..d0fa219880 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -9366,4 +9366,186 @@ static void ppc_cpu_register_types(void)
 #endif
 }
 
+void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
+{
+#define RGPL  4
+#define RFPL  4
+
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    int i;
+
+    qemu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "
+                 TARGET_FMT_lx " XER " TARGET_FMT_lx " CPU#%d\n",
+                 env->nip, env->lr, env->ctr, cpu_read_xer(env),
+                 cs->cpu_index);
+    qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
+                 "%08x iidx %d didx %d\n",
+                 env->msr, env->spr[SPR_HID0], env->hflags,
+                 cpu_mmu_index(env, true), cpu_mmu_index(env, false));
+#if !defined(NO_TIMER_DUMP)
+    qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
+#if !defined(CONFIG_USER_ONLY)
+                 " DECR " TARGET_FMT_lu
+#endif
+                 "\n",
+                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
+#if !defined(CONFIG_USER_ONLY)
+                 , cpu_ppc_load_decr(env)
+#endif
+        );
+#endif
+    for (i = 0; i < 32; i++) {
+        if ((i & (RGPL - 1)) == 0) {
+            qemu_fprintf(f, "GPR%02d", i);
+        }
+        qemu_fprintf(f, " %016" PRIx64, ppc_dump_gpr(env, i));
+        if ((i & (RGPL - 1)) == (RGPL - 1)) {
+            qemu_fprintf(f, "\n");
+        }
+    }
+    qemu_fprintf(f, "CR ");
+    for (i = 0; i < 8; i++)
+        qemu_fprintf(f, "%01x", env->crf[i]);
+    qemu_fprintf(f, "  [");
+    for (i = 0; i < 8; i++) {
+        char a = '-';
+        if (env->crf[i] & 0x08) {
+            a = 'L';
+        } else if (env->crf[i] & 0x04) {
+            a = 'G';
+        } else if (env->crf[i] & 0x02) {
+            a = 'E';
+        }
+        qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
+    }
+    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
+                 env->reserve_addr);
+
+    if (flags & CPU_DUMP_FPU) {
+        for (i = 0; i < 32; i++) {
+            if ((i & (RFPL - 1)) == 0) {
+                qemu_fprintf(f, "FPR%02d", i);
+            }
+            qemu_fprintf(f, " %016" PRIx64, *cpu_fpr_ptr(env, i));
+            if ((i & (RFPL - 1)) == (RFPL - 1)) {
+                qemu_fprintf(f, "\n");
+            }
+        }
+        qemu_fprintf(f, "FPSCR " TARGET_FMT_lx "\n", env->fpscr);
+    }
+
+#if !defined(CONFIG_USER_ONLY)
+    qemu_fprintf(f, " SRR0 " TARGET_FMT_lx "  SRR1 " TARGET_FMT_lx
+                 "    PVR " TARGET_FMT_lx " VRSAVE " TARGET_FMT_lx "\n",
+                 env->spr[SPR_SRR0], env->spr[SPR_SRR1],
+                 env->spr[SPR_PVR], env->spr[SPR_VRSAVE]);
+
+    qemu_fprintf(f, "SPRG0 " TARGET_FMT_lx " SPRG1 " TARGET_FMT_lx
+                 "  SPRG2 " TARGET_FMT_lx "  SPRG3 " TARGET_FMT_lx "\n",
+                 env->spr[SPR_SPRG0], env->spr[SPR_SPRG1],
+                 env->spr[SPR_SPRG2], env->spr[SPR_SPRG3]);
+
+    qemu_fprintf(f, "SPRG4 " TARGET_FMT_lx " SPRG5 " TARGET_FMT_lx
+                 "  SPRG6 " TARGET_FMT_lx "  SPRG7 " TARGET_FMT_lx "\n",
+                 env->spr[SPR_SPRG4], env->spr[SPR_SPRG5],
+                 env->spr[SPR_SPRG6], env->spr[SPR_SPRG7]);
+
+#if defined(TARGET_PPC64)
+    if (env->excp_model == POWERPC_EXCP_POWER7 ||
+        env->excp_model == POWERPC_EXCP_POWER8 ||
+        env->excp_model == POWERPC_EXCP_POWER9 ||
+        env->excp_model == POWERPC_EXCP_POWER10)  {
+        qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n",
+                     env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
+    }
+#endif
+    if (env->excp_model == POWERPC_EXCP_BOOKE) {
+        qemu_fprintf(f, "CSRR0 " TARGET_FMT_lx " CSRR1 " TARGET_FMT_lx
+                     " MCSRR0 " TARGET_FMT_lx " MCSRR1 " TARGET_FMT_lx "\n",
+                     env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1],
+                     env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
+
+        qemu_fprintf(f, "  TCR " TARGET_FMT_lx "   TSR " TARGET_FMT_lx
+                     "    ESR " TARGET_FMT_lx "   DEAR " TARGET_FMT_lx "\n",
+                     env->spr[SPR_BOOKE_TCR], env->spr[SPR_BOOKE_TSR],
+                     env->spr[SPR_BOOKE_ESR], env->spr[SPR_BOOKE_DEAR]);
+
+        qemu_fprintf(f, "  PIR " TARGET_FMT_lx " DECAR " TARGET_FMT_lx
+                     "   IVPR " TARGET_FMT_lx "   EPCR " TARGET_FMT_lx "\n",
+                     env->spr[SPR_BOOKE_PIR], env->spr[SPR_BOOKE_DECAR],
+                     env->spr[SPR_BOOKE_IVPR], env->spr[SPR_BOOKE_EPCR]);
+
+        qemu_fprintf(f, " MCSR " TARGET_FMT_lx " SPRG8 " TARGET_FMT_lx
+                     "    EPR " TARGET_FMT_lx "\n",
+                     env->spr[SPR_BOOKE_MCSR], env->spr[SPR_BOOKE_SPRG8],
+                     env->spr[SPR_BOOKE_EPR]);
+
+        /* FSL-specific */
+        qemu_fprintf(f, " MCAR " TARGET_FMT_lx "  PID1 " TARGET_FMT_lx
+                     "   PID2 " TARGET_FMT_lx "    SVR " TARGET_FMT_lx "\n",
+                     env->spr[SPR_Exxx_MCAR], env->spr[SPR_BOOKE_PID1],
+                     env->spr[SPR_BOOKE_PID2], env->spr[SPR_E500_SVR]);
+
+        /*
+         * IVORs are left out as they are large and do not change often --
+         * they can be read with "p $ivor0", "p $ivor1", etc.
+         */
+    }
+
+#if defined(TARGET_PPC64)
+    if (env->flags & POWERPC_FLAG_CFAR) {
+        qemu_fprintf(f, " CFAR " TARGET_FMT_lx"\n", env->cfar);
+    }
+#endif
+
+    if (env->spr_cb[SPR_LPCR].name) {
+        qemu_fprintf(f, " LPCR " TARGET_FMT_lx "\n", env->spr[SPR_LPCR]);
+    }
+
+    switch (env->mmu_model) {
+    case POWERPC_MMU_32B:
+    case POWERPC_MMU_601:
+    case POWERPC_MMU_SOFT_6xx:
+    case POWERPC_MMU_SOFT_74xx:
+#if defined(TARGET_PPC64)
+    case POWERPC_MMU_64B:
+    case POWERPC_MMU_2_03:
+    case POWERPC_MMU_2_06:
+    case POWERPC_MMU_2_07:
+    case POWERPC_MMU_3_00:
+#endif
+        if (env->spr_cb[SPR_SDR1].name) { /* SDR1 Exists */
+            qemu_fprintf(f, " SDR1 " TARGET_FMT_lx " ", env->spr[SPR_SDR1]);
+        }
+        if (env->spr_cb[SPR_PTCR].name) { /* PTCR Exists */
+            qemu_fprintf(f, " PTCR " TARGET_FMT_lx " ", env->spr[SPR_PTCR]);
+        }
+        qemu_fprintf(f, "  DAR " TARGET_FMT_lx "  DSISR " TARGET_FMT_lx "\n",
+                     env->spr[SPR_DAR], env->spr[SPR_DSISR]);
+        break;
+    case POWERPC_MMU_BOOKE206:
+        qemu_fprintf(f, " MAS0 " TARGET_FMT_lx "  MAS1 " TARGET_FMT_lx
+                     "   MAS2 " TARGET_FMT_lx "   MAS3 " TARGET_FMT_lx "\n",
+                     env->spr[SPR_BOOKE_MAS0], env->spr[SPR_BOOKE_MAS1],
+                     env->spr[SPR_BOOKE_MAS2], env->spr[SPR_BOOKE_MAS3]);
+
+        qemu_fprintf(f, " MAS4 " TARGET_FMT_lx "  MAS6 " TARGET_FMT_lx
+                     "   MAS7 " TARGET_FMT_lx "    PID " TARGET_FMT_lx "\n",
+                     env->spr[SPR_BOOKE_MAS4], env->spr[SPR_BOOKE_MAS6],
+                     env->spr[SPR_BOOKE_MAS7], env->spr[SPR_BOOKE_PID]);
+
+        qemu_fprintf(f, "MMUCFG " TARGET_FMT_lx " TLB0CFG " TARGET_FMT_lx
+                     " TLB1CFG " TARGET_FMT_lx "\n",
+                     env->spr[SPR_MMUCFG], env->spr[SPR_BOOKE_TLB0CFG],
+                     env->spr[SPR_BOOKE_TLB1CFG]);
+        break;
+    default:
+        break;
+    }
+#endif
+
+#undef RGPL
+#undef RFPL
+}
 type_init(ppc_cpu_register_types)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 5e3495e018..6c68d7006a 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -8617,193 +8617,6 @@ GEN_HANDLER2_E(trechkpt, "trechkpt", 0x1F, 0x0E, 0x1F, 0x03FFF800, \
 #include "translate/spe-ops.c.inc"
 };
 
-#include "helper_regs.h"
-
-/*****************************************************************************/
-/* Misc PowerPC helpers */
-void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
-{
-#define RGPL  4
-#define RFPL  4
-
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-    int i;
-
-    qemu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "
-                 TARGET_FMT_lx " XER " TARGET_FMT_lx " CPU#%d\n",
-                 env->nip, env->lr, env->ctr, cpu_read_xer(env),
-                 cs->cpu_index);
-    qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
-                 "%08x iidx %d didx %d\n",
-                 env->msr, env->spr[SPR_HID0], env->hflags,
-                 cpu_mmu_index(env, true), cpu_mmu_index(env, false));
-#if !defined(NO_TIMER_DUMP)
-    qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
-#if !defined(CONFIG_USER_ONLY)
-                 " DECR " TARGET_FMT_lu
-#endif
-                 "\n",
-                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
-#if !defined(CONFIG_USER_ONLY)
-                 , cpu_ppc_load_decr(env)
-#endif
-        );
-#endif
-    for (i = 0; i < 32; i++) {
-        if ((i & (RGPL - 1)) == 0) {
-            qemu_fprintf(f, "GPR%02d", i);
-        }
-        qemu_fprintf(f, " %016" PRIx64, ppc_dump_gpr(env, i));
-        if ((i & (RGPL - 1)) == (RGPL - 1)) {
-            qemu_fprintf(f, "\n");
-        }
-    }
-    qemu_fprintf(f, "CR ");
-    for (i = 0; i < 8; i++)
-        qemu_fprintf(f, "%01x", env->crf[i]);
-    qemu_fprintf(f, "  [");
-    for (i = 0; i < 8; i++) {
-        char a = '-';
-        if (env->crf[i] & 0x08) {
-            a = 'L';
-        } else if (env->crf[i] & 0x04) {
-            a = 'G';
-        } else if (env->crf[i] & 0x02) {
-            a = 'E';
-        }
-        qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
-    }
-    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
-                 env->reserve_addr);
-
-    if (flags & CPU_DUMP_FPU) {
-        for (i = 0; i < 32; i++) {
-            if ((i & (RFPL - 1)) == 0) {
-                qemu_fprintf(f, "FPR%02d", i);
-            }
-            qemu_fprintf(f, " %016" PRIx64, *cpu_fpr_ptr(env, i));
-            if ((i & (RFPL - 1)) == (RFPL - 1)) {
-                qemu_fprintf(f, "\n");
-            }
-        }
-        qemu_fprintf(f, "FPSCR " TARGET_FMT_lx "\n", env->fpscr);
-    }
-
-#if !defined(CONFIG_USER_ONLY)
-    qemu_fprintf(f, " SRR0 " TARGET_FMT_lx "  SRR1 " TARGET_FMT_lx
-                 "    PVR " TARGET_FMT_lx " VRSAVE " TARGET_FMT_lx "\n",
-                 env->spr[SPR_SRR0], env->spr[SPR_SRR1],
-                 env->spr[SPR_PVR], env->spr[SPR_VRSAVE]);
-
-    qemu_fprintf(f, "SPRG0 " TARGET_FMT_lx " SPRG1 " TARGET_FMT_lx
-                 "  SPRG2 " TARGET_FMT_lx "  SPRG3 " TARGET_FMT_lx "\n",
-                 env->spr[SPR_SPRG0], env->spr[SPR_SPRG1],
-                 env->spr[SPR_SPRG2], env->spr[SPR_SPRG3]);
-
-    qemu_fprintf(f, "SPRG4 " TARGET_FMT_lx " SPRG5 " TARGET_FMT_lx
-                 "  SPRG6 " TARGET_FMT_lx "  SPRG7 " TARGET_FMT_lx "\n",
-                 env->spr[SPR_SPRG4], env->spr[SPR_SPRG5],
-                 env->spr[SPR_SPRG6], env->spr[SPR_SPRG7]);
-
-#if defined(TARGET_PPC64)
-    if (env->excp_model == POWERPC_EXCP_POWER7 ||
-        env->excp_model == POWERPC_EXCP_POWER8 ||
-        env->excp_model == POWERPC_EXCP_POWER9 ||
-        env->excp_model == POWERPC_EXCP_POWER10)  {
-        qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n",
-                     env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
-    }
-#endif
-    if (env->excp_model == POWERPC_EXCP_BOOKE) {
-        qemu_fprintf(f, "CSRR0 " TARGET_FMT_lx " CSRR1 " TARGET_FMT_lx
-                     " MCSRR0 " TARGET_FMT_lx " MCSRR1 " TARGET_FMT_lx "\n",
-                     env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1],
-                     env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
-
-        qemu_fprintf(f, "  TCR " TARGET_FMT_lx "   TSR " TARGET_FMT_lx
-                     "    ESR " TARGET_FMT_lx "   DEAR " TARGET_FMT_lx "\n",
-                     env->spr[SPR_BOOKE_TCR], env->spr[SPR_BOOKE_TSR],
-                     env->spr[SPR_BOOKE_ESR], env->spr[SPR_BOOKE_DEAR]);
-
-        qemu_fprintf(f, "  PIR " TARGET_FMT_lx " DECAR " TARGET_FMT_lx
-                     "   IVPR " TARGET_FMT_lx "   EPCR " TARGET_FMT_lx "\n",
-                     env->spr[SPR_BOOKE_PIR], env->spr[SPR_BOOKE_DECAR],
-                     env->spr[SPR_BOOKE_IVPR], env->spr[SPR_BOOKE_EPCR]);
-
-        qemu_fprintf(f, " MCSR " TARGET_FMT_lx " SPRG8 " TARGET_FMT_lx
-                     "    EPR " TARGET_FMT_lx "\n",
-                     env->spr[SPR_BOOKE_MCSR], env->spr[SPR_BOOKE_SPRG8],
-                     env->spr[SPR_BOOKE_EPR]);
-
-        /* FSL-specific */
-        qemu_fprintf(f, " MCAR " TARGET_FMT_lx "  PID1 " TARGET_FMT_lx
-                     "   PID2 " TARGET_FMT_lx "    SVR " TARGET_FMT_lx "\n",
-                     env->spr[SPR_Exxx_MCAR], env->spr[SPR_BOOKE_PID1],
-                     env->spr[SPR_BOOKE_PID2], env->spr[SPR_E500_SVR]);
-
-        /*
-         * IVORs are left out as they are large and do not change often --
-         * they can be read with "p $ivor0", "p $ivor1", etc.
-         */
-    }
-
-#if defined(TARGET_PPC64)
-    if (env->flags & POWERPC_FLAG_CFAR) {
-        qemu_fprintf(f, " CFAR " TARGET_FMT_lx"\n", env->cfar);
-    }
-#endif
-
-    if (env->spr_cb[SPR_LPCR].name) {
-        qemu_fprintf(f, " LPCR " TARGET_FMT_lx "\n", env->spr[SPR_LPCR]);
-    }
-
-    switch (env->mmu_model) {
-    case POWERPC_MMU_32B:
-    case POWERPC_MMU_601:
-    case POWERPC_MMU_SOFT_6xx:
-    case POWERPC_MMU_SOFT_74xx:
-#if defined(TARGET_PPC64)
-    case POWERPC_MMU_64B:
-    case POWERPC_MMU_2_03:
-    case POWERPC_MMU_2_06:
-    case POWERPC_MMU_2_07:
-    case POWERPC_MMU_3_00:
-#endif
-        if (env->spr_cb[SPR_SDR1].name) { /* SDR1 Exists */
-            qemu_fprintf(f, " SDR1 " TARGET_FMT_lx " ", env->spr[SPR_SDR1]);
-        }
-        if (env->spr_cb[SPR_PTCR].name) { /* PTCR Exists */
-            qemu_fprintf(f, " PTCR " TARGET_FMT_lx " ", env->spr[SPR_PTCR]);
-        }
-        qemu_fprintf(f, "  DAR " TARGET_FMT_lx "  DSISR " TARGET_FMT_lx "\n",
-                     env->spr[SPR_DAR], env->spr[SPR_DSISR]);
-        break;
-    case POWERPC_MMU_BOOKE206:
-        qemu_fprintf(f, " MAS0 " TARGET_FMT_lx "  MAS1 " TARGET_FMT_lx
-                     "   MAS2 " TARGET_FMT_lx "   MAS3 " TARGET_FMT_lx "\n",
-                     env->spr[SPR_BOOKE_MAS0], env->spr[SPR_BOOKE_MAS1],
-                     env->spr[SPR_BOOKE_MAS2], env->spr[SPR_BOOKE_MAS3]);
-
-        qemu_fprintf(f, " MAS4 " TARGET_FMT_lx "  MAS6 " TARGET_FMT_lx
-                     "   MAS7 " TARGET_FMT_lx "    PID " TARGET_FMT_lx "\n",
-                     env->spr[SPR_BOOKE_MAS4], env->spr[SPR_BOOKE_MAS6],
-                     env->spr[SPR_BOOKE_MAS7], env->spr[SPR_BOOKE_PID]);
-
-        qemu_fprintf(f, "MMUCFG " TARGET_FMT_lx " TLB0CFG " TARGET_FMT_lx
-                     " TLB1CFG " TARGET_FMT_lx "\n",
-                     env->spr[SPR_MMUCFG], env->spr[SPR_BOOKE_TLB0CFG],
-                     env->spr[SPR_BOOKE_TLB1CFG]);
-        break;
-    default:
-        break;
-    }
-#endif
-
-#undef RGPL
-#undef RFPL
-}
-
 /*****************************************************************************/
 /* Opcode types */
 enum {
-- 
2.17.1

Re: [PATCH 03/11] target/ppc: moved ppc_cpu_dump_state to cpu_init.c

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> This function was forgotten in the cpu_init code motion series, but it
> seems to be used regardless of TCG, and so needs to be moved to support
> disabling TCG.
> 
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/cpu_init.c  | 182 +++++++++++++++++++++++++++++++++++++++
>   target/ppc/translate.c | 187 -----------------------------------------
>   2 files changed, 182 insertions(+), 187 deletions(-)

Not just "seems to be" -- "is".

Indeed, cpu_dump_state is called from accel/kvm/kvm-all.c, and the monitor.


r~

Re: [PATCH 03/11] target/ppc: moved ppc_cpu_dump_state to cpu_init.c

[Richard Henderson]

On 5/12/21 11:58 AM, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>> This function was forgotten in the cpu_init code motion series, but it
>> seems to be used regardless of TCG, and so needs to be moved to support
>> disabling TCG.
>>
>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
>> ---
>>   target/ppc/cpu_init.c  | 182 +++++++++++++++++++++++++++++++++++++++
>>   target/ppc/translate.c | 187 -----------------------------------------
>>   2 files changed, 182 insertions(+), 187 deletions(-)
> 
> Not just "seems to be" -- "is".
> 
> Indeed, cpu_dump_state is called from accel/kvm/kvm-all.c, and the monitor.

... and I meant to include

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

r~

Re: [PATCH 03/11] target/ppc: moved ppc_cpu_dump_state to cpu_init.c

[David Gibson]

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

On Wed, May 12, 2021 at 11:08:05AM -0300, Bruno Larsen (billionai) wrote:
> This function was forgotten in the cpu_init code motion series, but it
> seems to be used regardless of TCG, and so needs to be moved to support
> disabling TCG.
> 
> Signed-off-by: Bruno Larsen (billionai)
> <bruno.larsen@eldorado.org.br>

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/cpu_init.c  | 182 +++++++++++++++++++++++++++++++++++++++
>  target/ppc/translate.c | 187 -----------------------------------------
>  2 files changed, 182 insertions(+), 187 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index b4a2d15c6a..d0fa219880 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -9366,4 +9366,186 @@ static void ppc_cpu_register_types(void)
>  #endif
>  }
>  
> +void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> +{
> +#define RGPL  4
> +#define RFPL  4
> +
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    int i;
> +
> +    qemu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "
> +                 TARGET_FMT_lx " XER " TARGET_FMT_lx " CPU#%d\n",
> +                 env->nip, env->lr, env->ctr, cpu_read_xer(env),
> +                 cs->cpu_index);
> +    qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
> +                 "%08x iidx %d didx %d\n",
> +                 env->msr, env->spr[SPR_HID0], env->hflags,
> +                 cpu_mmu_index(env, true), cpu_mmu_index(env, false));
> +#if !defined(NO_TIMER_DUMP)
> +    qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> +#if !defined(CONFIG_USER_ONLY)
> +                 " DECR " TARGET_FMT_lu
> +#endif
> +                 "\n",
> +                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> +#if !defined(CONFIG_USER_ONLY)
> +                 , cpu_ppc_load_decr(env)
> +#endif
> +        );
> +#endif
> +    for (i = 0; i < 32; i++) {
> +        if ((i & (RGPL - 1)) == 0) {
> +            qemu_fprintf(f, "GPR%02d", i);
> +        }
> +        qemu_fprintf(f, " %016" PRIx64, ppc_dump_gpr(env, i));
> +        if ((i & (RGPL - 1)) == (RGPL - 1)) {
> +            qemu_fprintf(f, "\n");
> +        }
> +    }
> +    qemu_fprintf(f, "CR ");
> +    for (i = 0; i < 8; i++)
> +        qemu_fprintf(f, "%01x", env->crf[i]);
> +    qemu_fprintf(f, "  [");
> +    for (i = 0; i < 8; i++) {
> +        char a = '-';
> +        if (env->crf[i] & 0x08) {
> +            a = 'L';
> +        } else if (env->crf[i] & 0x04) {
> +            a = 'G';
> +        } else if (env->crf[i] & 0x02) {
> +            a = 'E';
> +        }
> +        qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
> +    }
> +    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
> +                 env->reserve_addr);
> +
> +    if (flags & CPU_DUMP_FPU) {
> +        for (i = 0; i < 32; i++) {
> +            if ((i & (RFPL - 1)) == 0) {
> +                qemu_fprintf(f, "FPR%02d", i);
> +            }
> +            qemu_fprintf(f, " %016" PRIx64, *cpu_fpr_ptr(env, i));
> +            if ((i & (RFPL - 1)) == (RFPL - 1)) {
> +                qemu_fprintf(f, "\n");
> +            }
> +        }
> +        qemu_fprintf(f, "FPSCR " TARGET_FMT_lx "\n", env->fpscr);
> +    }
> +
> +#if !defined(CONFIG_USER_ONLY)
> +    qemu_fprintf(f, " SRR0 " TARGET_FMT_lx "  SRR1 " TARGET_FMT_lx
> +                 "    PVR " TARGET_FMT_lx " VRSAVE " TARGET_FMT_lx "\n",
> +                 env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> +                 env->spr[SPR_PVR], env->spr[SPR_VRSAVE]);
> +
> +    qemu_fprintf(f, "SPRG0 " TARGET_FMT_lx " SPRG1 " TARGET_FMT_lx
> +                 "  SPRG2 " TARGET_FMT_lx "  SPRG3 " TARGET_FMT_lx "\n",
> +                 env->spr[SPR_SPRG0], env->spr[SPR_SPRG1],
> +                 env->spr[SPR_SPRG2], env->spr[SPR_SPRG3]);
> +
> +    qemu_fprintf(f, "SPRG4 " TARGET_FMT_lx " SPRG5 " TARGET_FMT_lx
> +                 "  SPRG6 " TARGET_FMT_lx "  SPRG7 " TARGET_FMT_lx "\n",
> +                 env->spr[SPR_SPRG4], env->spr[SPR_SPRG5],
> +                 env->spr[SPR_SPRG6], env->spr[SPR_SPRG7]);
> +
> +#if defined(TARGET_PPC64)
> +    if (env->excp_model == POWERPC_EXCP_POWER7 ||
> +        env->excp_model == POWERPC_EXCP_POWER8 ||
> +        env->excp_model == POWERPC_EXCP_POWER9 ||
> +        env->excp_model == POWERPC_EXCP_POWER10)  {
> +        qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
> +    }
> +#endif
> +    if (env->excp_model == POWERPC_EXCP_BOOKE) {
> +        qemu_fprintf(f, "CSRR0 " TARGET_FMT_lx " CSRR1 " TARGET_FMT_lx
> +                     " MCSRR0 " TARGET_FMT_lx " MCSRR1 " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1],
> +                     env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
> +
> +        qemu_fprintf(f, "  TCR " TARGET_FMT_lx "   TSR " TARGET_FMT_lx
> +                     "    ESR " TARGET_FMT_lx "   DEAR " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_BOOKE_TCR], env->spr[SPR_BOOKE_TSR],
> +                     env->spr[SPR_BOOKE_ESR], env->spr[SPR_BOOKE_DEAR]);
> +
> +        qemu_fprintf(f, "  PIR " TARGET_FMT_lx " DECAR " TARGET_FMT_lx
> +                     "   IVPR " TARGET_FMT_lx "   EPCR " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_BOOKE_PIR], env->spr[SPR_BOOKE_DECAR],
> +                     env->spr[SPR_BOOKE_IVPR], env->spr[SPR_BOOKE_EPCR]);
> +
> +        qemu_fprintf(f, " MCSR " TARGET_FMT_lx " SPRG8 " TARGET_FMT_lx
> +                     "    EPR " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_BOOKE_MCSR], env->spr[SPR_BOOKE_SPRG8],
> +                     env->spr[SPR_BOOKE_EPR]);
> +
> +        /* FSL-specific */
> +        qemu_fprintf(f, " MCAR " TARGET_FMT_lx "  PID1 " TARGET_FMT_lx
> +                     "   PID2 " TARGET_FMT_lx "    SVR " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_Exxx_MCAR], env->spr[SPR_BOOKE_PID1],
> +                     env->spr[SPR_BOOKE_PID2], env->spr[SPR_E500_SVR]);
> +
> +        /*
> +         * IVORs are left out as they are large and do not change often --
> +         * they can be read with "p $ivor0", "p $ivor1", etc.
> +         */
> +    }
> +
> +#if defined(TARGET_PPC64)
> +    if (env->flags & POWERPC_FLAG_CFAR) {
> +        qemu_fprintf(f, " CFAR " TARGET_FMT_lx"\n", env->cfar);
> +    }
> +#endif
> +
> +    if (env->spr_cb[SPR_LPCR].name) {
> +        qemu_fprintf(f, " LPCR " TARGET_FMT_lx "\n", env->spr[SPR_LPCR]);
> +    }
> +
> +    switch (env->mmu_model) {
> +    case POWERPC_MMU_32B:
> +    case POWERPC_MMU_601:
> +    case POWERPC_MMU_SOFT_6xx:
> +    case POWERPC_MMU_SOFT_74xx:
> +#if defined(TARGET_PPC64)
> +    case POWERPC_MMU_64B:
> +    case POWERPC_MMU_2_03:
> +    case POWERPC_MMU_2_06:
> +    case POWERPC_MMU_2_07:
> +    case POWERPC_MMU_3_00:
> +#endif
> +        if (env->spr_cb[SPR_SDR1].name) { /* SDR1 Exists */
> +            qemu_fprintf(f, " SDR1 " TARGET_FMT_lx " ", env->spr[SPR_SDR1]);
> +        }
> +        if (env->spr_cb[SPR_PTCR].name) { /* PTCR Exists */
> +            qemu_fprintf(f, " PTCR " TARGET_FMT_lx " ", env->spr[SPR_PTCR]);
> +        }
> +        qemu_fprintf(f, "  DAR " TARGET_FMT_lx "  DSISR " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_DAR], env->spr[SPR_DSISR]);
> +        break;
> +    case POWERPC_MMU_BOOKE206:
> +        qemu_fprintf(f, " MAS0 " TARGET_FMT_lx "  MAS1 " TARGET_FMT_lx
> +                     "   MAS2 " TARGET_FMT_lx "   MAS3 " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_BOOKE_MAS0], env->spr[SPR_BOOKE_MAS1],
> +                     env->spr[SPR_BOOKE_MAS2], env->spr[SPR_BOOKE_MAS3]);
> +
> +        qemu_fprintf(f, " MAS4 " TARGET_FMT_lx "  MAS6 " TARGET_FMT_lx
> +                     "   MAS7 " TARGET_FMT_lx "    PID " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_BOOKE_MAS4], env->spr[SPR_BOOKE_MAS6],
> +                     env->spr[SPR_BOOKE_MAS7], env->spr[SPR_BOOKE_PID]);
> +
> +        qemu_fprintf(f, "MMUCFG " TARGET_FMT_lx " TLB0CFG " TARGET_FMT_lx
> +                     " TLB1CFG " TARGET_FMT_lx "\n",
> +                     env->spr[SPR_MMUCFG], env->spr[SPR_BOOKE_TLB0CFG],
> +                     env->spr[SPR_BOOKE_TLB1CFG]);
> +        break;
> +    default:
> +        break;
> +    }
> +#endif
> +
> +#undef RGPL
> +#undef RFPL
> +}
>  type_init(ppc_cpu_register_types)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 5e3495e018..6c68d7006a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -8617,193 +8617,6 @@ GEN_HANDLER2_E(trechkpt, "trechkpt", 0x1F, 0x0E, 0x1F, 0x03FFF800, \
>  #include "translate/spe-ops.c.inc"
>  };
>  
> -#include "helper_regs.h"
> -
> -/*****************************************************************************/
> -/* Misc PowerPC helpers */
> -void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> -{
> -#define RGPL  4
> -#define RFPL  4
> -
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    CPUPPCState *env = &cpu->env;
> -    int i;
> -
> -    qemu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "
> -                 TARGET_FMT_lx " XER " TARGET_FMT_lx " CPU#%d\n",
> -                 env->nip, env->lr, env->ctr, cpu_read_xer(env),
> -                 cs->cpu_index);
> -    qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
> -                 "%08x iidx %d didx %d\n",
> -                 env->msr, env->spr[SPR_HID0], env->hflags,
> -                 cpu_mmu_index(env, true), cpu_mmu_index(env, false));
> -#if !defined(NO_TIMER_DUMP)
> -    qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> -#if !defined(CONFIG_USER_ONLY)
> -                 " DECR " TARGET_FMT_lu
> -#endif
> -                 "\n",
> -                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> -#if !defined(CONFIG_USER_ONLY)
> -                 , cpu_ppc_load_decr(env)
> -#endif
> -        );
> -#endif
> -    for (i = 0; i < 32; i++) {
> -        if ((i & (RGPL - 1)) == 0) {
> -            qemu_fprintf(f, "GPR%02d", i);
> -        }
> -        qemu_fprintf(f, " %016" PRIx64, ppc_dump_gpr(env, i));
> -        if ((i & (RGPL - 1)) == (RGPL - 1)) {
> -            qemu_fprintf(f, "\n");
> -        }
> -    }
> -    qemu_fprintf(f, "CR ");
> -    for (i = 0; i < 8; i++)
> -        qemu_fprintf(f, "%01x", env->crf[i]);
> -    qemu_fprintf(f, "  [");
> -    for (i = 0; i < 8; i++) {
> -        char a = '-';
> -        if (env->crf[i] & 0x08) {
> -            a = 'L';
> -        } else if (env->crf[i] & 0x04) {
> -            a = 'G';
> -        } else if (env->crf[i] & 0x02) {
> -            a = 'E';
> -        }
> -        qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
> -    }
> -    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
> -                 env->reserve_addr);
> -
> -    if (flags & CPU_DUMP_FPU) {
> -        for (i = 0; i < 32; i++) {
> -            if ((i & (RFPL - 1)) == 0) {
> -                qemu_fprintf(f, "FPR%02d", i);
> -            }
> -            qemu_fprintf(f, " %016" PRIx64, *cpu_fpr_ptr(env, i));
> -            if ((i & (RFPL - 1)) == (RFPL - 1)) {
> -                qemu_fprintf(f, "\n");
> -            }
> -        }
> -        qemu_fprintf(f, "FPSCR " TARGET_FMT_lx "\n", env->fpscr);
> -    }
> -
> -#if !defined(CONFIG_USER_ONLY)
> -    qemu_fprintf(f, " SRR0 " TARGET_FMT_lx "  SRR1 " TARGET_FMT_lx
> -                 "    PVR " TARGET_FMT_lx " VRSAVE " TARGET_FMT_lx "\n",
> -                 env->spr[SPR_SRR0], env->spr[SPR_SRR1],
> -                 env->spr[SPR_PVR], env->spr[SPR_VRSAVE]);
> -
> -    qemu_fprintf(f, "SPRG0 " TARGET_FMT_lx " SPRG1 " TARGET_FMT_lx
> -                 "  SPRG2 " TARGET_FMT_lx "  SPRG3 " TARGET_FMT_lx "\n",
> -                 env->spr[SPR_SPRG0], env->spr[SPR_SPRG1],
> -                 env->spr[SPR_SPRG2], env->spr[SPR_SPRG3]);
> -
> -    qemu_fprintf(f, "SPRG4 " TARGET_FMT_lx " SPRG5 " TARGET_FMT_lx
> -                 "  SPRG6 " TARGET_FMT_lx "  SPRG7 " TARGET_FMT_lx "\n",
> -                 env->spr[SPR_SPRG4], env->spr[SPR_SPRG5],
> -                 env->spr[SPR_SPRG6], env->spr[SPR_SPRG7]);
> -
> -#if defined(TARGET_PPC64)
> -    if (env->excp_model == POWERPC_EXCP_POWER7 ||
> -        env->excp_model == POWERPC_EXCP_POWER8 ||
> -        env->excp_model == POWERPC_EXCP_POWER9 ||
> -        env->excp_model == POWERPC_EXCP_POWER10)  {
> -        qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
> -    }
> -#endif
> -    if (env->excp_model == POWERPC_EXCP_BOOKE) {
> -        qemu_fprintf(f, "CSRR0 " TARGET_FMT_lx " CSRR1 " TARGET_FMT_lx
> -                     " MCSRR0 " TARGET_FMT_lx " MCSRR1 " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1],
> -                     env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
> -
> -        qemu_fprintf(f, "  TCR " TARGET_FMT_lx "   TSR " TARGET_FMT_lx
> -                     "    ESR " TARGET_FMT_lx "   DEAR " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_BOOKE_TCR], env->spr[SPR_BOOKE_TSR],
> -                     env->spr[SPR_BOOKE_ESR], env->spr[SPR_BOOKE_DEAR]);
> -
> -        qemu_fprintf(f, "  PIR " TARGET_FMT_lx " DECAR " TARGET_FMT_lx
> -                     "   IVPR " TARGET_FMT_lx "   EPCR " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_BOOKE_PIR], env->spr[SPR_BOOKE_DECAR],
> -                     env->spr[SPR_BOOKE_IVPR], env->spr[SPR_BOOKE_EPCR]);
> -
> -        qemu_fprintf(f, " MCSR " TARGET_FMT_lx " SPRG8 " TARGET_FMT_lx
> -                     "    EPR " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_BOOKE_MCSR], env->spr[SPR_BOOKE_SPRG8],
> -                     env->spr[SPR_BOOKE_EPR]);
> -
> -        /* FSL-specific */
> -        qemu_fprintf(f, " MCAR " TARGET_FMT_lx "  PID1 " TARGET_FMT_lx
> -                     "   PID2 " TARGET_FMT_lx "    SVR " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_Exxx_MCAR], env->spr[SPR_BOOKE_PID1],
> -                     env->spr[SPR_BOOKE_PID2], env->spr[SPR_E500_SVR]);
> -
> -        /*
> -         * IVORs are left out as they are large and do not change often --
> -         * they can be read with "p $ivor0", "p $ivor1", etc.
> -         */
> -    }
> -
> -#if defined(TARGET_PPC64)
> -    if (env->flags & POWERPC_FLAG_CFAR) {
> -        qemu_fprintf(f, " CFAR " TARGET_FMT_lx"\n", env->cfar);
> -    }
> -#endif
> -
> -    if (env->spr_cb[SPR_LPCR].name) {
> -        qemu_fprintf(f, " LPCR " TARGET_FMT_lx "\n", env->spr[SPR_LPCR]);
> -    }
> -
> -    switch (env->mmu_model) {
> -    case POWERPC_MMU_32B:
> -    case POWERPC_MMU_601:
> -    case POWERPC_MMU_SOFT_6xx:
> -    case POWERPC_MMU_SOFT_74xx:
> -#if defined(TARGET_PPC64)
> -    case POWERPC_MMU_64B:
> -    case POWERPC_MMU_2_03:
> -    case POWERPC_MMU_2_06:
> -    case POWERPC_MMU_2_07:
> -    case POWERPC_MMU_3_00:
> -#endif
> -        if (env->spr_cb[SPR_SDR1].name) { /* SDR1 Exists */
> -            qemu_fprintf(f, " SDR1 " TARGET_FMT_lx " ", env->spr[SPR_SDR1]);
> -        }
> -        if (env->spr_cb[SPR_PTCR].name) { /* PTCR Exists */
> -            qemu_fprintf(f, " PTCR " TARGET_FMT_lx " ", env->spr[SPR_PTCR]);
> -        }
> -        qemu_fprintf(f, "  DAR " TARGET_FMT_lx "  DSISR " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_DAR], env->spr[SPR_DSISR]);
> -        break;
> -    case POWERPC_MMU_BOOKE206:
> -        qemu_fprintf(f, " MAS0 " TARGET_FMT_lx "  MAS1 " TARGET_FMT_lx
> -                     "   MAS2 " TARGET_FMT_lx "   MAS3 " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_BOOKE_MAS0], env->spr[SPR_BOOKE_MAS1],
> -                     env->spr[SPR_BOOKE_MAS2], env->spr[SPR_BOOKE_MAS3]);
> -
> -        qemu_fprintf(f, " MAS4 " TARGET_FMT_lx "  MAS6 " TARGET_FMT_lx
> -                     "   MAS7 " TARGET_FMT_lx "    PID " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_BOOKE_MAS4], env->spr[SPR_BOOKE_MAS6],
> -                     env->spr[SPR_BOOKE_MAS7], env->spr[SPR_BOOKE_PID]);
> -
> -        qemu_fprintf(f, "MMUCFG " TARGET_FMT_lx " TLB0CFG " TARGET_FMT_lx
> -                     " TLB1CFG " TARGET_FMT_lx "\n",
> -                     env->spr[SPR_MMUCFG], env->spr[SPR_BOOKE_TLB0CFG],
> -                     env->spr[SPR_BOOKE_TLB1CFG]);
> -        break;
> -    default:
> -        break;
> -    }
> -#endif
> -
> -#undef RGPL
> -#undef RFPL
> -}
> -
>  /*****************************************************************************/
>  /* Opcode types */
>  enum {

-- 
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 04/11] target/ppc: moved ppc_store_msr into gdbstub.c

[Bruno Larsen (billionai)]

This function is used by !TCG cases, so it was moved to a common code
file. We chose gdbstub.c since it was the one giving us grief over it.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/gdbstub.c     | 7 +++++++
 target/ppc/misc_helper.c | 6 ------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 9339e7eafe..17e41fc113 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -22,6 +22,7 @@
 #include "exec/gdbstub.h"
 #include "exec/helper-proto.h"
 #include "internal.h"
+#include "helper_regs.h"
 
 static int ppc_gdb_register_len_apple(int n)
 {
@@ -622,6 +623,12 @@ gchar *ppc_gdb_arch_name(CPUState *cs)
 #endif
 }
 
+/* GDBstub can read and write MSR... */
+void ppc_store_msr(CPUPPCState *env, target_ulong value)
+{
+    hreg_store_msr(env, value, 0);
+}
+
 void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *pcc)
 {
     if (pcc->insns_flags & PPC_FLOAT) {
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 08a31da289..b910ac6479 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -255,12 +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);
-- 
2.17.1

Re: [PATCH 04/11] target/ppc: moved ppc_store_msr into gdbstub.c

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> This function is used by !TCG cases, so it was moved to a common code
> file. We chose gdbstub.c since it was the one giving us grief over it.
> 
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/gdbstub.c     | 7 +++++++
>   target/ppc/misc_helper.c | 6 ------
>   2 files changed, 7 insertions(+), 6 deletions(-)

gdbstub.c is not the only !tcg user; e.g. machine.c.

I think this should go in cpu.c, next to the other special register read/write 
functions.


r~

Re: [PATCH 04/11] target/ppc: moved ppc_store_msr into gdbstub.c

[David Gibson]

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

On Wed, May 12, 2021 at 12:05:04PM -0500, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> > This function is used by !TCG cases, so it was moved to a common code
> > file. We chose gdbstub.c since it was the one giving us grief over it.
> > 
> > Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> > ---
> >   target/ppc/gdbstub.c     | 7 +++++++
> >   target/ppc/misc_helper.c | 6 ------
> >   2 files changed, 7 insertions(+), 6 deletions(-)
> 
> gdbstub.c is not the only !tcg user; e.g. machine.c.
> 
> I think this should go in cpu.c, next to the other special register
> read/write functions.

I agree.

-- 
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 05/11] target/ppc: moved ppc_store_lpcr to cpu.c

[Bruno Larsen (billionai)]

This function is used in !TCG cases, so it has been moved into a file
that is compiled when --disable-tcg is selected.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/cpu.c         | 11 +++++++++++
 target/ppc/misc_helper.c | 10 ----------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 0ab7ac1af1..8a82e2ffa8 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)
 {
@@ -90,3 +91,13 @@ 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;
 }
+
+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 b910ac6479..442b12652c 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -255,16 +255,6 @@ target_ulong helper_clcs(CPUPPCState *env, uint32_t arg)
 /*****************************************************************************/
 /* Special registers manipulation */
 
-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

Re: [PATCH 05/11] target/ppc: moved ppc_store_lpcr to cpu.c

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> This function is used in !TCG cases, so it has been moved into a file
> that is compiled when --disable-tcg is selected.
> 
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/cpu.c         | 11 +++++++++++
>   target/ppc/misc_helper.c | 10 ----------
>   2 files changed, 11 insertions(+), 10 deletions(-)

It's helpful to be more specific about where it's used -- not just !tcg, but 
the location.  In this case, hw/ppc/ during machine startup.

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


r~

Re: [PATCH 05/11] target/ppc: moved ppc_store_lpcr to cpu.c

[David Gibson]

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

On Wed, May 12, 2021 at 11:08:07AM -0300, Bruno Larsen (billionai) wrote:
> This function is used in !TCG cases, so it has been moved into a file
> that is compiled when --disable-tcg is selected.
> 
> Signed-off-by: Bruno Larsen (billionai)
> <bruno.larsen@eldorado.org.br>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target/ppc/cpu.c         | 11 +++++++++++
>  target/ppc/misc_helper.c | 10 ----------
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index 0ab7ac1af1..8a82e2ffa8 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)
>  {
> @@ -90,3 +91,13 @@ 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;
>  }
> +
> +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 b910ac6479..442b12652c 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -255,16 +255,6 @@ target_ulong helper_clcs(CPUPPCState *env, uint32_t arg)
>  /*****************************************************************************/
>  /* Special registers manipulation */
>  
> -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

-- 
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 06/11] target/ppc: updated vscr manipulation in machine.c

[Bruno Larsen (billionai)]

Updated the code in machine.c to use the generic ppc_{store,get}_vscr
instead of helper style functions, so it can build without TCG

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

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index e5bffbe365..93972df58e 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -8,7 +8,6 @@
 #include "qapi/error.h"
 #include "qemu/main-loop.h"
 #include "kvm_ppc.h"
-#include "exec/helper-proto.h"
 
 static void post_load_update_msr(CPUPPCState *env)
 {
@@ -107,7 +106,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
         ppc_store_sdr1(env, sdr1);
     }
     qemu_get_be32s(f, &vscr);
-    helper_mtvscr(env, vscr);
+    ppc_store_vscr(env, vscr);
     qemu_get_be64s(f, &env->spe_acc);
     qemu_get_be32s(f, &env->spe_fscr);
     qemu_get_betls(f, &env->msr_mask);
@@ -456,7 +455,7 @@ static int get_vscr(QEMUFile *f, void *opaque, size_t size,
                     const VMStateField *field)
 {
     PowerPCCPU *cpu = opaque;
-    helper_mtvscr(&cpu->env, qemu_get_be32(f));
+    ppc_store_vscr(&cpu->env, qemu_get_be32(f));
     return 0;
 }
 
@@ -464,7 +463,7 @@ static int put_vscr(QEMUFile *f, void *opaque, size_t size,
                     const VMStateField *field, JSONWriter *vmdesc)
 {
     PowerPCCPU *cpu = opaque;
-    qemu_put_be32(f, helper_mfvscr(&cpu->env));
+    qemu_put_be32(f, ppc_get_vscr(&cpu->env));
     return 0;
 }
 
-- 
2.17.1

Re: [PATCH 06/11] target/ppc: updated vscr manipulation in machine.c

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> Updated the code in machine.c to use the generic ppc_{store,get}_vscr
> instead of helper style functions, so it can build without TCG
> 
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/machine.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)

Squash this into patch 1.


r~

Re: [PATCH 06/11] target/ppc: updated vscr manipulation in machine.c

[David Gibson]

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

On Wed, May 12, 2021 at 12:08:52PM -0500, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> > Updated the code in machine.c to use the generic ppc_{store,get}_vscr
> > instead of helper style functions, so it can build without TCG
> > 
> > Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> > ---
> >   target/ppc/machine.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> Squash this into patch 1.

Or don't since I already applied patch 1.  I've applied this one as
well.

-- 
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 07/11] target/ppc: added KVM fallback to fpscr manipulation

[Bruno Larsen (billionai)]

some common code needs to store information in fpscr, but this function
relies on TCG cde to work. This patch adds a kvm way to do it, and a
transparent way to call it when TCG is not compiled

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/gdbstub.c |  1 +
 target/ppc/kvm.c     | 14 ++++++++++++++
 target/ppc/kvm_ppc.h |  6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 17e41fc113..65759e0c1c 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -23,6 +23,7 @@
 #include "exec/helper-proto.h"
 #include "internal.h"
 #include "helper_regs.h"
+#include "kvm_ppc.h"
 
 static int ppc_gdb_register_len_apple(int n)
 {
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb..a8a720eb48 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2947,3 +2947,17 @@ bool kvm_arch_cpu_check_are_resettable(void)
 {
     return true;
 }
+
+void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
+{
+    CPUState *cs = env_cpu(env);
+    struct kvm_one_reg reg;
+    int ret;
+    reg.id = KVM_REG_PPC_FPSCR;
+    reg.addr = (uintptr_t) &env->fpscr;
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret < 0)
+    {
+        fprintf(stderr, "Unable to set FPSCR to KVM: %s", strerror(errno));
+    }
+}
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 989f61ace0..ff365e57a6 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -86,6 +86,12 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t tb_offset);
 
 int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);
 
+#ifndef CONFIG_TCG
+# define store_fpscr kvmppc_store_fpscr
+#endif
+void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
+
+
 #else
 
 static inline uint32_t kvmppc_get_tbfreq(void)
-- 
2.17.1

Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..a8a720eb48 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2947,3 +2947,17 @@ bool kvm_arch_cpu_check_are_resettable(void)
>   {
>       return true;
>   }
> +
> +void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
> +{
> +    CPUState *cs = env_cpu(env);
> +    struct kvm_one_reg reg;
> +    int ret;
> +    reg.id = KVM_REG_PPC_FPSCR;
> +    reg.addr = (uintptr_t) &env->fpscr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret < 0)
> +    {
> +        fprintf(stderr, "Unable to set FPSCR to KVM: %s", strerror(errno));
> +    }
> +}

This is all unnecessary.  All you need to do is store to env->fpscr and the 
value will be synced back with kvm_put_fp.

I'll note that some of the trouble you may be having with extracting 
helper_store_fpscr to a ppc_store_fpscr function is due to an existing bug in 
the tcg code:

Storing to fpscr should *never* raise an exception -- see MTFSF, MTFSB0, 
MTFSB1.  Thus the mucking about with cs->exception_index and env->error_code is 
incorrect.

In addition, the masking is being done weirdly and could use a complete overhaul.

This could all be rewritten as

-- %< -- cpu.h

  /* Invalid operation exception summary */
- #define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) ...
+ #define FPSCR_IX  ((1 << FPSCR_VXSNAN) | ...)

-- %< -- cpu.c

// move fpscr_set_rounding_mode here

void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
{
     /* Recompute exception summary state. */
     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);
     }
}

-- %< -- fpu_helper.c

void helper_store_fpscr(CPUPPCState *env, target_ulong val,
                         uint32_t nibbles)
{
     target_ulong mask = 0;

     /* TODO: Push this expansion back to translation time. */
     for (int i = 0; i < sizeof(target_ulong) * 2; ++i) {
         if (nibbles & (1 << i)) {
             mask |= (target_ulong)0xf << (4 * i);
         }
     }

     val = (val & mask) | (env->fpscr & ~mask);
     ppc_store_fpscr(env, val);
}

void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
{
     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)
{
     uint32_t mask = 1u << bit;
     if (!(env->fpscr & mask)) {
         ppc_store_fpscr(env, env->fpscr | mask);
     }
}

There are a couple of other uses of fpscr_set_rounding_mode, where the 
softfloat value is changed temporarily (do_fri, VSX_ROUND).  These should 
simply save the previous softfloat value (using get_float_rounding_mode) around 
the operation instead of re-computing from fpscr.

Which leaves us with exactly one use of fpscr_set_rounding_mode, which can then 
be moved to cpu.c next to ppc_store_fpscr.


r~

Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation

[Bruno Piazera Larsen]

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


On 12/05/2021 15:20, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 104a308abb..a8a720eb48 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2947,3 +2947,17 @@ bool kvm_arch_cpu_check_are_resettable(void)
>>   {
>>       return true;
>>   }
>> +
>> +void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
>> +{
>> +    CPUState *cs = env_cpu(env);
>> +    struct kvm_one_reg reg;
>> +    int ret;
>> +    reg.id = KVM_REG_PPC_FPSCR;
>> +    reg.addr = (uintptr_t) &env->fpscr;
>> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>> +    if (ret < 0)
>> +    {
>> +        fprintf(stderr, "Unable to set FPSCR to KVM: %s", 
>> strerror(errno));
>> +    }
>> +}
>
> This is all unnecessary.  All you need to do is store to env->fpscr 
> and the value will be synced back with kvm_put_fp.
I figured this was too complicated again, but didn't have a better idea
>
> I'll note that some of the trouble you may be having with extracting 
> helper_store_fpscr to a ppc_store_fpscr function is due to an existing 
> bug in the tcg code:
>
> Storing to fpscr should *never* raise an exception -- see MTFSF, 
> MTFSB0, MTFSB1.  Thus the mucking about with cs->exception_index and 
> env->error_code is incorrect.
>
> In addition, the masking is being done weirdly and could use a 
> complete overhaul.
>
> This could all be rewritten as
>
> -- %< -- cpu.h
>
>  /* Invalid operation exception summary */
> - #define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) ...
> + #define FPSCR_IX  ((1 << FPSCR_VXSNAN) | ...)
>
> -- %< -- cpu.c
>
> // move fpscr_set_rounding_mode here
>
> void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
> {
>     /* Recompute exception summary state. */
>     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);
>     }
> }
>
> -- %< -- fpu_helper.c
>
> void helper_store_fpscr(CPUPPCState *env, target_ulong val,
>                         uint32_t nibbles)
> {
>     target_ulong mask = 0;
>
>     /* TODO: Push this expansion back to translation time. */
>     for (int i = 0; i < sizeof(target_ulong) * 2; ++i) {
>         if (nibbles & (1 << i)) {
>             mask |= (target_ulong)0xf << (4 * i);
>         }
>     }
>
>     val = (val & mask) | (env->fpscr & ~mask);
>     ppc_store_fpscr(env, val);
> }
>
> void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
> {
>     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)
> {
>     uint32_t mask = 1u << bit;
>     if (!(env->fpscr & mask)) {
>         ppc_store_fpscr(env, env->fpscr | mask);
>     }
> }
>
> There are a couple of other uses of fpscr_set_rounding_mode, where the 
> softfloat value is changed temporarily (do_fri, VSX_ROUND). These 
> should simply save the previous softfloat value (using 
> get_float_rounding_mode) around the operation instead of re-computing 
> from fpscr.
>
> Which leaves us with exactly one use of fpscr_set_rounding_mode, which 
> can then be moved to cpu.c next to ppc_store_fpscr.
>
ok, yeah, I can definitely try this.
>
> r~
-- 
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: 6325 bytes --]

Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation

[Bruno Piazera Larsen]

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


On 12/05/2021 15:20, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 104a308abb..a8a720eb48 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2947,3 +2947,17 @@ bool kvm_arch_cpu_check_are_resettable(void)
>>   {
>>       return true;
>>   }
>> +
>> +void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
>> +{
>> +    CPUState *cs = env_cpu(env);
>> +    struct kvm_one_reg reg;
>> +    int ret;
>> +    reg.id = KVM_REG_PPC_FPSCR;
>> +    reg.addr = (uintptr_t) &env->fpscr;
>> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>> +    if (ret < 0)
>> +    {
>> +        fprintf(stderr, "Unable to set FPSCR to KVM: %s", 
>> strerror(errno));
>> +    }
>> +}
>
> This is all unnecessary.  All you need to do is store to env->fpscr 
> and the value will be synced back with kvm_put_fp.
>
> I'll note that some of the trouble you may be having with extracting 
> helper_store_fpscr to a ppc_store_fpscr function is due to an existing 
> bug in the tcg code:
>
> Storing to fpscr should *never* raise an exception -- see MTFSF, 
> MTFSB0, MTFSB1.  Thus the mucking about with cs->exception_index and 
> env->error_code is incorrect.
>
> In addition, the masking is being done weirdly and could use a 
> complete overhaul.
>
> This could all be rewritten as
>
> -- %< -- cpu.h
>
>  /* Invalid operation exception summary */
> - #define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) ...
> + #define FPSCR_IX  ((1 << FPSCR_VXSNAN) | ...)
>
> -- %< -- cpu.c
>
> // move fpscr_set_rounding_mode here
>
> void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
> {
>     /* Recompute exception summary state. */
>     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);
>     }
> }
>
> -- %< -- fpu_helper.c
>
> void helper_store_fpscr(CPUPPCState *env, target_ulong val,
>                         uint32_t nibbles)
> {
>     target_ulong mask = 0;
>
>     /* TODO: Push this expansion back to translation time. */
>     for (int i = 0; i < sizeof(target_ulong) * 2; ++i) {
>         if (nibbles & (1 << i)) {
>             mask |= (target_ulong)0xf << (4 * i);
>         }
>     }
>
>     val = (val & mask) | (env->fpscr & ~mask);
>     ppc_store_fpscr(env, val);
> }
That expansion can't be moved to translation time, because gdbstub would 
also need that code in a variety of functions, so better to keep it in 
that central location,
>
> void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
> {
>     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)
> {
>     uint32_t mask = 1u << bit;
>     if (!(env->fpscr & mask)) {
>         ppc_store_fpscr(env, env->fpscr | mask);
>     }
> }
>
> There are a couple of other uses of fpscr_set_rounding_mode, where the 
> softfloat value is changed temporarily (do_fri, VSX_ROUND). These 
> should simply save the previous softfloat value (using 
> get_float_rounding_mode) around the operation instead of re-computing 
> from fpscr.
>
> Which leaves us with exactly one use of fpscr_set_rounding_mode, which 
> can then be moved to cpu.c next to ppc_store_fpscr.
>
>
> r~

I was implementing this solution, but ran into a problem: We needed 
store_fpscr for gdbstub.c, that's the original reason we made that new 
function to begin with. This solution, although it improves the handling 
of fpscr, doesn't fix the original problem.

What I think we can do is put the logic that is in helper_store_fpscr 
into store_fpscr, move it to cpu.c, and have the helper call the 
non-helper function. That way we conserve helper_* as TCG-specific and 
have the overhaul.

Toughts?

-- 
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: 6846 bytes --]

Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation

[Richard Henderson]

On 5/13/21 11:36 AM, Bruno Piazera Larsen wrote:
> 
> On 12/05/2021 15:20, Richard Henderson wrote:
>> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>> index 104a308abb..a8a720eb48 100644
>>> --- a/target/ppc/kvm.c
>>> +++ b/target/ppc/kvm.c
>>> @@ -2947,3 +2947,17 @@ bool kvm_arch_cpu_check_are_resettable(void)
>>>   {
>>>       return true;
>>>   }
>>> +
>>> +void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
>>> +{
>>> +    CPUState *cs = env_cpu(env);
>>> +    struct kvm_one_reg reg;
>>> +    int ret;
>>> +    reg.id = KVM_REG_PPC_FPSCR;
>>> +    reg.addr = (uintptr_t) &env->fpscr;
>>> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>>> +    if (ret < 0)
>>> +    {
>>> +        fprintf(stderr, "Unable to set FPSCR to KVM: %s", strerror(errno));
>>> +    }
>>> +}
>>
>> This is all unnecessary.  All you need to do is store to env->fpscr and the 
>> value will be synced back with kvm_put_fp.
>>
>> I'll note that some of the trouble you may be having with extracting 
>> helper_store_fpscr to a ppc_store_fpscr function is due to an existing bug in 
>> the tcg code:
>>
>> Storing to fpscr should *never* raise an exception -- see MTFSF, MTFSB0, 
>> MTFSB1.  Thus the mucking about with cs->exception_index and env->error_code 
>> is incorrect.
>>
>> In addition, the masking is being done weirdly and could use a complete 
>> overhaul.
>>
>> This could all be rewritten as
>>
>> -- %< -- cpu.h
>>
>>  /* Invalid operation exception summary */
>> - #define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) ...
>> + #define FPSCR_IX  ((1 << FPSCR_VXSNAN) | ...)
>>
>> -- %< -- cpu.c
>>
>> // move fpscr_set_rounding_mode here
>>
>> void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
>> {
>>     /* Recompute exception summary state. */
>>     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);
>>     }
>> }
>>
>> -- %< -- fpu_helper.c
>>
>> void helper_store_fpscr(CPUPPCState *env, target_ulong val,
>>                         uint32_t nibbles)
>> {
>>     target_ulong mask = 0;
>>
>>     /* TODO: Push this expansion back to translation time. */
>>     for (int i = 0; i < sizeof(target_ulong) * 2; ++i) {
>>         if (nibbles & (1 << i)) {
>>             mask |= (target_ulong)0xf << (4 * i);
>>         }
>>     }
>>
>>     val = (val & mask) | (env->fpscr & ~mask);
>>     ppc_store_fpscr(env, val);
>> }
> That expansion can't be moved to translation time, because gdbstub would also 
> need that code in a variety of functions, so better to keep it in that central 
> location,
>>
>> void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
>> {
>>     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)
>> {
>>     uint32_t mask = 1u << bit;
>>     if (!(env->fpscr & mask)) {
>>         ppc_store_fpscr(env, env->fpscr | mask);
>>     }
>> }
>>
>> There are a couple of other uses of fpscr_set_rounding_mode, where the 
>> softfloat value is changed temporarily (do_fri, VSX_ROUND). These should 
>> simply save the previous softfloat value (using get_float_rounding_mode) 
>> around the operation instead of re-computing from fpscr.
>>
>> Which leaves us with exactly one use of fpscr_set_rounding_mode, which can 
>> then be moved to cpu.c next to ppc_store_fpscr.
>>
>>
>> r~
> 
> I was implementing this solution, but ran into a problem: We needed store_fpscr 
> for gdbstub.c, that's the original reason we made that new function to begin 
> with. This solution, although it improves the handling of fpscr, doesn't fix 
> the original problem.

Why not?  Did you miss the cpu.c cut at the very top?

> What I think we can do is put the logic that is in helper_store_fpscr into 
> store_fpscr, move it to cpu.c, and have the helper call the non-helper 
> function. That way we conserve helper_* as TCG-specific and have the overhaul.

That is exactly what I have written above.


r~

Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation

[Bruno Piazera Larsen]

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


On 13/05/2021 19:45, Richard Henderson wrote:
> On 5/13/21 11:36 AM, Bruno Piazera Larsen wrote:
>>
>> On 12/05/2021 15:20, Richard Henderson wrote:
>>> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 104a308abb..a8a720eb48 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -2947,3 +2947,17 @@ bool kvm_arch_cpu_check_are_resettable(void)
>>>>   {
>>>>       return true;
>>>>   }
>>>> +
>>>> +void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t 
>>>> mask)
>>>> +{
>>>> +    CPUState *cs = env_cpu(env);
>>>> +    struct kvm_one_reg reg;
>>>> +    int ret;
>>>> +    reg.id = KVM_REG_PPC_FPSCR;
>>>> +    reg.addr = (uintptr_t) &env->fpscr;
>>>> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>>>> +    if (ret < 0)
>>>> +    {
>>>> +        fprintf(stderr, "Unable to set FPSCR to KVM: %s", 
>>>> strerror(errno));
>>>> +    }
>>>> +}
>>>
>>> This is all unnecessary.  All you need to do is store to env->fpscr 
>>> and the value will be synced back with kvm_put_fp.
>>>
>>> I'll note that some of the trouble you may be having with extracting 
>>> helper_store_fpscr to a ppc_store_fpscr function is due to an 
>>> existing bug in the tcg code:
>>>
>>> Storing to fpscr should *never* raise an exception -- see MTFSF, 
>>> MTFSB0, MTFSB1.  Thus the mucking about with cs->exception_index and 
>>> env->error_code is incorrect.
>>>
>>> In addition, the masking is being done weirdly and could use a 
>>> complete overhaul.
>>>
>>> This could all be rewritten as
>>>
>>> -- %< -- cpu.h
>>>
>>>  /* Invalid operation exception summary */
>>> - #define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) ...
>>> + #define FPSCR_IX  ((1 << FPSCR_VXSNAN) | ...)
>>>
>>> -- %< -- cpu.c
>>>
>>> // move fpscr_set_rounding_mode here
>>>
>>> void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
>>> {
>>>     /* Recompute exception summary state. */
>>>     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);
>>>     }
>>> }
>>>
>>> -- %< -- fpu_helper.c
>>>
>>> void helper_store_fpscr(CPUPPCState *env, target_ulong val,
>>>                         uint32_t nibbles)
>>> {
>>>     target_ulong mask = 0;
>>>
>>>     /* TODO: Push this expansion back to translation time. */
>>>     for (int i = 0; i < sizeof(target_ulong) * 2; ++i) {
>>>         if (nibbles & (1 << i)) {
>>>             mask |= (target_ulong)0xf << (4 * i);
>>>         }
>>>     }
>>>
>>>     val = (val & mask) | (env->fpscr & ~mask);
>>>     ppc_store_fpscr(env, val);
>>> }
>> That expansion can't be moved to translation time, because gdbstub 
>> would also need that code in a variety of functions, so better to 
>> keep it in that central location,
>>>
>>> void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
>>> {
>>>     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)
>>> {
>>>     uint32_t mask = 1u << bit;
>>>     if (!(env->fpscr & mask)) {
>>>         ppc_store_fpscr(env, env->fpscr | mask);
>>>     }
>>> }
>>>
>>> There are a couple of other uses of fpscr_set_rounding_mode, where 
>>> the softfloat value is changed temporarily (do_fri, VSX_ROUND). 
>>> These should simply save the previous softfloat value (using 
>>> get_float_rounding_mode) around the operation instead of 
>>> re-computing from fpscr.
>>>
>>> Which leaves us with exactly one use of fpscr_set_rounding_mode, 
>>> which can then be moved to cpu.c next to ppc_store_fpscr.
>>>
>>>
>>> r~
>>
>> I was implementing this solution, but ran into a problem: We needed 
>> store_fpscr for gdbstub.c, that's the original reason we made that 
>> new function to begin with. This solution, although it improves the 
>> handling of fpscr, doesn't fix the original problem.
>
> Why not?  Did you miss the cpu.c cut at the very top?
So the plan was to have gdbstub call ppc_store_fpscr? I assumed it 
wasn't since there is one less parameter in the new function. Now that I 
took another look, gdbstub has the mask as 0xffffffff, so it's easier 
than I thought.
>
>> What I think we can do is put the logic that is in helper_store_fpscr 
>> into store_fpscr, move it to cpu.c, and have the helper call the 
>> non-helper function. That way we conserve helper_* as TCG-specific 
>> and have the overhaul.
>
> That is exactly what I have written above.

Not exactly, because the expansion of nibbles into mask is still in 
helper_store_fpscr, and that's what I meant.

-- 
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: 8588 bytes --]

[RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Bruno Larsen (billionai)]

Wrapped some function calls in cpu_init.c and gdbstub.c that were TCG only
with ifdef CONFIG_TCG, to make it easier to build without it.

This seemed to be the minimum amount of changes to make the project
compile, but we wanted some feedback about wether we excluded something
important or if we should reorder some code to minimize ifdef count

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 include/exec/helper-proto.h | 2 ++
 target/ppc/cpu_init.c       | 8 ++++++++
 target/ppc/excp_helper.c    | 6 +++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h
index ba100793a7..ce287222ee 100644
--- a/include/exec/helper-proto.h
+++ b/include/exec/helper-proto.h
@@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), dh_ctype(t2), dh_ctype(t3), \
 #define IN_HELPER_PROTO
 
 #include "helper.h"
+#ifdef CONFIG_TCG
 #include "trace/generated-helpers.h"
+#endif
 #include "accel/tcg/tcg-runtime.h"
 #include "accel/tcg/plugin-helpers.h"
 
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index d0fa219880..9d72dc49cf 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -1204,11 +1204,13 @@ static void register_BookE206_sprs(CPUPPCState *env, uint32_t mas_mask,
     /* TLB assist registers */
     /* XXX : not implemented */
     for (i = 0; i < 8; i++) {
+#ifdef CONFIG_TCG
         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;
         }
+#endif
         if (mas_mask & (1 << i)) {
             spr_register(env, mas_sprn[i], mas_names[i],
                          SPR_NOACCESS, SPR_NOACCESS,
@@ -8606,7 +8608,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
         }
     }
 
+#ifdef CONFIG_TCG
     create_ppc_opcodes(cpu, &local_err);
+#endif
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         goto unrealize;
@@ -8799,7 +8803,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)
@@ -9297,7 +9303,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 f4f15279eb..824e16a32a 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -21,7 +21,9 @@
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
+#ifdef CONFIG_TCG
 #include "exec/cpu_ldst.h"
+#endif
 #include "internal.h"
 #include "helper_regs.h"
 
@@ -1492,7 +1494,7 @@ void helper_book3s_msgsnd(target_ulong rb)
     book3s_msgsnd_common(pir, PPC_INTERRUPT_HDOORBELL);
 }
 
-#if defined(TARGET_PPC64)
+#if defined(TARGET_PPC64) && defined(CONFIG_TCG)
 void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
 {
     helper_hfscr_facility_check(env, HFSCR_MSGP, "msgclrp", HFSCR_IC_MSGP);
@@ -1525,6 +1527,7 @@ void helper_book3s_msgsndp(CPUPPCState *env, target_ulong rb)
 #endif
 #endif
 
+#ifdef CONFIG_TCG
 void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
                                  MMUAccessType access_type,
                                  int mmu_idx, uintptr_t retaddr)
@@ -1540,3 +1543,4 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
     env->error_code = insn & 0x03FF0000;
     cpu_loop_exit(cs);
 }
+#endif
-- 
2.17.1

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h
> index ba100793a7..ce287222ee 100644
> --- a/include/exec/helper-proto.h
> +++ b/include/exec/helper-proto.h
> @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), dh_ctype(t2), dh_ctype(t3), \
>   #define IN_HELPER_PROTO
>   
>   #include "helper.h"
> +#ifdef CONFIG_TCG
>   #include "trace/generated-helpers.h"
> +#endif
>   #include "accel/tcg/tcg-runtime.h"
>   #include "accel/tcg/plugin-helpers.h"
>   

Um.. this file is exclusively TCG already.
Are you missing some use of helper_foo()?


> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index d0fa219880..9d72dc49cf 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -1204,11 +1204,13 @@ static void register_BookE206_sprs(CPUPPCState *env, uint32_t mas_mask,
>       /* TLB assist registers */
>       /* XXX : not implemented */
>       for (i = 0; i < 8; i++) {
> +#ifdef CONFIG_TCG
>           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;
>           }
> +#endif

You could move this condition into

>           if (mas_mask & (1 << i)) {
>               spr_register(env, mas_sprn[i], mas_names[i],
>                            SPR_NOACCESS, SPR_NOACCESS,

... the call here, so that it all automatically compiles out:

   spr_register(env, ...,
                spr_read_generic,
                (i == 2 && (env->insns_flags & PPC_64B)
                 ? spr_write_generic : spr_write_generic32),
                0x00000000);

> @@ -8606,7 +8608,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
>           }
>       }
>   
> +#ifdef CONFIG_TCG
>       create_ppc_opcodes(cpu, &local_err);
> +#endif
>       if (local_err != NULL) {
>           error_propagate(errp, local_err);
>           goto unrealize;

Include the error checking into the ifdef.


> @@ -8799,7 +8803,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)
> @@ -9297,7 +9303,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

We should just drop this entirely.  It's supposedly a generic thing, but only 
used by ppc.  But even then only with source modification to enable 
DO_PPC_STATISTICS.  And even then as we convert to decodetree, said statistics 
will not be collected.

We should delete everything from cpu_dump_statistics on down.


r~

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Bruno Piazera Larsen]

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

On 12/05/2021 15:33, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>> diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h
>> index ba100793a7..ce287222ee 100644
>> --- a/include/exec/helper-proto.h
>> +++ b/include/exec/helper-proto.h
>> @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), 
>> dh_ctype(t2), dh_ctype(t3), \
>>   #define IN_HELPER_PROTO
>>     #include "helper.h"
>> +#ifdef CONFIG_TCG
>>   #include "trace/generated-helpers.h"
>> +#endif
>>   #include "accel/tcg/tcg-runtime.h"
>>   #include "accel/tcg/plugin-helpers.h"
>
> Um.. this file is exclusively TCG already.
> Are you missing some use of helper_foo()?
A lot of files that we are compiling (mainly mmu-*, excp_helper and 
gdbstub IIRC). We could comb through all of them and remove all 
declarations of helpers and wrap the inclusion of helper-proto itself in 
ifdefs, but it felt unnecessarily long. If it is preferable, we can do it.
>
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index d0fa219880..9d72dc49cf 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -1204,11 +1204,13 @@ static void 
>> register_BookE206_sprs(CPUPPCState *env, uint32_t mas_mask,
>>       /* TLB assist registers */
>>       /* XXX : not implemented */
>>       for (i = 0; i < 8; i++) {
>> +#ifdef CONFIG_TCG
>>           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;
>>           }
>> +#endif
>
> You could move this condition into
>
>>           if (mas_mask & (1 << i)) {
>>               spr_register(env, mas_sprn[i], mas_names[i],
>>                            SPR_NOACCESS, SPR_NOACCESS,
>
> ... the call here, so that it all automatically compiles out:
>
>   spr_register(env, ...,
>                spr_read_generic,
>                (i == 2 && (env->insns_flags & PPC_64B)
>                 ? spr_write_generic : spr_write_generic32),
>                0x00000000);
>
Yeah, sounds like a better plan.
>> @@ -8606,7 +8608,9 @@ static void ppc_cpu_realize(DeviceState *dev, 
>> Error **errp)
>>           }
>>       }
>>   +#ifdef CONFIG_TCG
>>       create_ppc_opcodes(cpu, &local_err);
>> +#endif
>>       if (local_err != NULL) {
>>           error_propagate(errp, local_err);
>>           goto unrealize;
>
> Include the error checking into the ifdef.
Ah, yeah, good idea
>
>
>> @@ -8799,7 +8803,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)
>> @@ -9297,7 +9303,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
>
> We should just drop this entirely.  It's supposedly a generic thing, 
> but only used by ppc.  But even then only with source modification to 
> enable DO_PPC_STATISTICS.  And even then as we convert to decodetree, 
> said statistics will not be collected.
>
> We should delete everything from cpu_dump_statistics on down.
uhm, sure. We can do it, but I think should be left as future cleanup 
once we get disable-tcg working
>
>
> r~
-- 
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: 6712 bytes --]

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Bruno Piazera Larsen]

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


On 12/05/2021 15:57, Bruno Piazera Larsen wrote:
> On 12/05/2021 15:33, Richard Henderson wrote:
>> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>>> diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h
>>> index ba100793a7..ce287222ee 100644
>>> --- a/include/exec/helper-proto.h
>>> +++ b/include/exec/helper-proto.h
>>> @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), 
>>> dh_ctype(t2), dh_ctype(t3), \
>>>   #define IN_HELPER_PROTO
>>>     #include "helper.h"
>>> +#ifdef CONFIG_TCG
>>>   #include "trace/generated-helpers.h"
>>> +#endif
>>>   #include "accel/tcg/tcg-runtime.h"
>>>   #include "accel/tcg/plugin-helpers.h"
>>
>> Um.. this file is exclusively TCG already.
>> Are you missing some use of helper_foo()?
> A lot of files that we are compiling (mainly mmu-*, excp_helper and 
> gdbstub IIRC). We could comb through all of them and remove all 
> declarations of helpers and wrap the inclusion of helper-proto itself 
> in ifdefs, but it felt unnecessarily long. If it is preferable, we can 
> do it.
>
So, I just looked and we'd need to change excp_helper.c and 
mmu-hash64.c, encasing 14 and 8 helper_foo() declarations. Is it better 
to work on those 2 files, or to change helper-proto?
-- 
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: 2523 bytes --]

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Richard Henderson]

On 5/14/21 8:29 AM, Bruno Piazera Larsen wrote:
> 
> On 12/05/2021 15:57, Bruno Piazera Larsen wrote:
>> On 12/05/2021 15:33, Richard Henderson wrote:
>>> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>>>> diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h
>>>> index ba100793a7..ce287222ee 100644
>>>> --- a/include/exec/helper-proto.h
>>>> +++ b/include/exec/helper-proto.h
>>>> @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), dh_ctype(t2), 
>>>> dh_ctype(t3), \
>>>>   #define IN_HELPER_PROTO
>>>>     #include "helper.h"
>>>> +#ifdef CONFIG_TCG
>>>>   #include "trace/generated-helpers.h"
>>>> +#endif
>>>>   #include "accel/tcg/tcg-runtime.h"
>>>>   #include "accel/tcg/plugin-helpers.h"
>>>
>>> Um.. this file is exclusively TCG already.
>>> Are you missing some use of helper_foo()?
>> A lot of files that we are compiling (mainly mmu-*, excp_helper and gdbstub 
>> IIRC). We could comb through all of them and remove all declarations of 
>> helpers and wrap the inclusion of helper-proto itself in ifdefs, but it felt 
>> unnecessarily long. If it is preferable, we can do it.
>>
> So, I just looked and we'd need to change excp_helper.c and mmu-hash64.c, 
> encasing 14 and 8 helper_foo() declarations. Is it better to work on those 2 
> files, or to change helper-proto?

Let's work on excp_helper.c and mmu-hash64.c.

For excp_helper.c, ideally everything in there would be tcg related.  Either 
explicitly as helper_foo() or by being one of the TCGCPUOps functions like 
ppc_cpu_exec_interrupt.

For mmu-hash64.c... I guess the easiest thing in the short term is to put big 
ifdefs around helper_slbi{a,e,eg}.  Or they could be moved to mmu_helper.c, 
with slb_lookup declared in mmu-hash64.h.


r~

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Bruno Piazera Larsen]

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


On 14/05/2021 11:44, Richard Henderson wrote:
> On 5/14/21 8:29 AM, Bruno Piazera Larsen wrote:
>>
>> On 12/05/2021 15:57, Bruno Piazera Larsen wrote:
>>> On 12/05/2021 15:33, Richard Henderson wrote:
>>>> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>>>>> diff --git a/include/exec/helper-proto.h 
>>>>> b/include/exec/helper-proto.h
>>>>> index ba100793a7..ce287222ee 100644
>>>>> --- a/include/exec/helper-proto.h
>>>>> +++ b/include/exec/helper-proto.h
>>>>> @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), 
>>>>> dh_ctype(t2), dh_ctype(t3), \
>>>>>   #define IN_HELPER_PROTO
>>>>>     #include "helper.h"
>>>>> +#ifdef CONFIG_TCG
>>>>>   #include "trace/generated-helpers.h"
>>>>> +#endif
>>>>>   #include "accel/tcg/tcg-runtime.h"
>>>>>   #include "accel/tcg/plugin-helpers.h"
>>>>
>>>> Um.. this file is exclusively TCG already.
>>>> Are you missing some use of helper_foo()?
>>> A lot of files that we are compiling (mainly mmu-*, excp_helper and 
>>> gdbstub IIRC). We could comb through all of them and remove all 
>>> declarations of helpers and wrap the inclusion of helper-proto 
>>> itself in ifdefs, but it felt unnecessarily long. If it is 
>>> preferable, we can do it.
>>>
>> So, I just looked and we'd need to change excp_helper.c and 
>> mmu-hash64.c, encasing 14 and 8 helper_foo() declarations. Is it 
>> better to work on those 2 files, or to change helper-proto?
>
> Let's work on excp_helper.c and mmu-hash64.c.
>
> For excp_helper.c, ideally everything in there would be tcg related.  
> Either explicitly as helper_foo() or by being one of the TCGCPUOps 
> functions like ppc_cpu_exec_interrupt.

Removing excp_helper.c gives linker errors for the functions:

* ppc_cpu_do_system_reset, on hw/ppc/pnv.c and hw/ppc/spapr.c

* ppc_do_cpu_interrupt, on hw/ppc/spapr_events.c and target/ppc/kvm.c, 
and from what was discussed on the IRC this should still be compiled

* ppc_cpu_do_fwnmi_machine_check, on hw/ppc/spapr_events.c


I think ppc_do_cpu_interrupt could go to cpu.c, but no clue where the 
others should go.

>
> For mmu-hash64.c... I guess the easiest thing in the short term is to 
> put big ifdefs around helper_slbi{a,e,eg}.  Or they could be moved to 
> mmu_helper.c, with slb_lookup declared in mmu-hash64.h.
>
The ifdefs are easy enough, and not compiling it gives way too many 
linker errors. Moving the helpers and static functions sounds the 
cleanest though, so I'll look into it while I wait for the excp_helper help
>
> r~
-- 
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: 4482 bytes --]

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[David Gibson]

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

On Fri, May 14, 2021 at 01:22:48PM -0300, Bruno Piazera Larsen wrote:
> 
> On 14/05/2021 11:44, Richard Henderson wrote:
> > On 5/14/21 8:29 AM, Bruno Piazera Larsen wrote:
> > > 
> > > On 12/05/2021 15:57, Bruno Piazera Larsen wrote:
> > > > On 12/05/2021 15:33, Richard Henderson wrote:
> > > > > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> > > > > > diff --git a/include/exec/helper-proto.h
> > > > > > b/include/exec/helper-proto.h
> > > > > > index ba100793a7..ce287222ee 100644
> > > > > > --- a/include/exec/helper-proto.h
> > > > > > +++ b/include/exec/helper-proto.h
> > > > > > @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name)
> > > > > > (dh_ctype(t1), dh_ctype(t2), dh_ctype(t3), \
> > > > > >   #define IN_HELPER_PROTO
> > > > > >     #include "helper.h"
> > > > > > +#ifdef CONFIG_TCG
> > > > > >   #include "trace/generated-helpers.h"
> > > > > > +#endif
> > > > > >   #include "accel/tcg/tcg-runtime.h"
> > > > > >   #include "accel/tcg/plugin-helpers.h"
> > > > > 
> > > > > Um.. this file is exclusively TCG already.
> > > > > Are you missing some use of helper_foo()?
> > > > A lot of files that we are compiling (mainly mmu-*, excp_helper
> > > > and gdbstub IIRC). We could comb through all of them and remove
> > > > all declarations of helpers and wrap the inclusion of
> > > > helper-proto itself in ifdefs, but it felt unnecessarily long.
> > > > If it is preferable, we can do it.
> > > > 
> > > So, I just looked and we'd need to change excp_helper.c and
> > > mmu-hash64.c, encasing 14 and 8 helper_foo() declarations. Is it
> > > better to work on those 2 files, or to change helper-proto?
> > 
> > Let's work on excp_helper.c and mmu-hash64.c.
> > 
> > For excp_helper.c, ideally everything in there would be tcg related. 
> > Either explicitly as helper_foo() or by being one of the TCGCPUOps
> > functions like ppc_cpu_exec_interrupt.
> 
> Removing excp_helper.c gives linker errors for the functions:
> 
> * ppc_cpu_do_system_reset, on hw/ppc/pnv.c and hw/ppc/spapr.c

Oof, that's a bit tricky.  We definitely do need this system reset
injection for KVM as well as TCG.  Unfortunately it calls into
powerpc_excp() which I think has a bunch of TCG specific stuff as
well.

Long term, I think the thing would be to remove the giant ugly
multiplexer in powerpc_excp() in favour of different entry points.
But that's a big job.

Short term, littering it with ifdefs might be the least worst we can
do.  Richard, any better ideas?


> * ppc_do_cpu_interrupt, on hw/ppc/spapr_events.c and target/ppc/kvm.c, and
> from what was discussed on the IRC this should still be compiled

Similar situation to above, but much easier because it doesn't go
through the giant multiplexer.
> 
> 
> I think ppc_do_cpu_interrupt could go to cpu.c, but no clue where the others
> should go.

Yes, moving ppc_cpu_do_interrupt() to common code is the right call.

> 
> > 
> > For mmu-hash64.c... I guess the easiest thing in the short term is to
> > put big ifdefs around helper_slbi{a,e,eg}.  Or they could be moved to
> > mmu_helper.c, with slb_lookup declared in mmu-hash64.h.
> > 
> The ifdefs are easy enough, and not compiling it gives way too many linker
> errors. Moving the helpers and static functions sounds the cleanest though,
> so I'll look into it while I wait for the excp_helper help
> > 
> > r~

-- 
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: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Richard Henderson]

On 5/16/21 11:10 PM, David Gibson wrote:
>> Removing excp_helper.c gives linker errors for the functions:
>>
>> * ppc_cpu_do_system_reset, on hw/ppc/pnv.c and hw/ppc/spapr.c
> 
> Oof, that's a bit tricky.  We definitely do need this system reset
> injection for KVM as well as TCG.  Unfortunately it calls into
> powerpc_excp() which I think has a bunch of TCG specific stuff as
> well.
> 
> Long term, I think the thing would be to remove the giant ugly
> multiplexer in powerpc_excp() in favour of different entry points.
> But that's a big job.
> 
> Short term, littering it with ifdefs might be the least worst we can
> do.  Richard, any better ideas?

Nope, no better ideas here.


r~

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[David Gibson]

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

On Fri, May 14, 2021 at 09:44:36AM -0500, Richard Henderson wrote:
> On 5/14/21 8:29 AM, Bruno Piazera Larsen wrote:
> > 
> > On 12/05/2021 15:57, Bruno Piazera Larsen wrote:
> > > On 12/05/2021 15:33, Richard Henderson wrote:
> > > > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> > > > > diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h
> > > > > index ba100793a7..ce287222ee 100644
> > > > > --- a/include/exec/helper-proto.h
> > > > > +++ b/include/exec/helper-proto.h
> > > > > @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1),
> > > > > dh_ctype(t2), dh_ctype(t3), \
> > > > >   #define IN_HELPER_PROTO
> > > > >     #include "helper.h"
> > > > > +#ifdef CONFIG_TCG
> > > > >   #include "trace/generated-helpers.h"
> > > > > +#endif
> > > > >   #include "accel/tcg/tcg-runtime.h"
> > > > >   #include "accel/tcg/plugin-helpers.h"
> > > > 
> > > > Um.. this file is exclusively TCG already.
> > > > Are you missing some use of helper_foo()?
> > > A lot of files that we are compiling (mainly mmu-*, excp_helper and
> > > gdbstub IIRC). We could comb through all of them and remove all
> > > declarations of helpers and wrap the inclusion of helper-proto
> > > itself in ifdefs, but it felt unnecessarily long. If it is
> > > preferable, we can do it.
> > > 
> > So, I just looked and we'd need to change excp_helper.c and
> > mmu-hash64.c, encasing 14 and 8 helper_foo() declarations. Is it better
> > to work on those 2 files, or to change helper-proto?
> 
> Let's work on excp_helper.c and mmu-hash64.c.
> 
> For excp_helper.c, ideally everything in there would be tcg related.  Either
> explicitly as helper_foo() or by being one of the TCGCPUOps functions like
> ppc_cpu_exec_interrupt.
> 
> For mmu-hash64.c... I guess the easiest thing in the short term is to put
> big ifdefs around helper_slbi{a,e,eg}.  Or they could be moved to
> mmu_helper.c, with slb_lookup declared in mmu-hash64.h.

I think the ifdefs are the lesser evil here.  I created mmu-hash64.*
because I think dividing things by MMU family is much more useful than
dividing by how it fits into the TCG/SOFTMMU model which how things
kind of were done historically (and still are for the many MMU familes
I've never had time or knowledge to rework).

-- 
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: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Bruno Piazera Larsen]

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


>> @@ -8799,7 +8803,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)
>> @@ -9297,7 +9303,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
>
> We should just drop this entirely.  It's supposedly a generic thing, 
> but only used by ppc.  But even then only with source modification to 
> enable DO_PPC_STATISTICS.  And even then as we convert to decodetree, 
> said statistics will not be collected.
>
> We should delete everything from cpu_dump_statistics on down.

So, now that we have a version of disable-tcg that is functional, I'm 
inclined to look at this cleanup. Just to make sure I got it right: 
everything related to ppc_cpu_dump_statistics and the stuff related to 
ifdef DO_PPC_STATISTICS can be removed, yeah?

-- 
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: 2305 bytes --]

Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[Richard Henderson]

On 5/24/21 11:01 AM, Bruno Piazera Larsen wrote:
>>> +#ifdef CONFIG_TCG
>>>       cc->dump_statistics = ppc_cpu_dump_statistics;
>>> +#endif
>>
>> We should just drop this entirely.  It's supposedly a generic thing, but only 
>> used by ppc.  But even then only with source modification to enable 
>> DO_PPC_STATISTICS.  And even then as we convert to decodetree, said 
>> statistics will not be collected.
>>
>> We should delete everything from cpu_dump_statistics on down.
> 
> So, now that we have a version of disable-tcg that is functional, I'm inclined 
> to look at this cleanup. Just to make sure I got it right: everything related 
> to ppc_cpu_dump_statistics and the stuff related to ifdef DO_PPC_STATISTICS can 
> be removed, yeah?

I would imagine a set of patches:

(1) DO_PPC_STATISTICS
(2) ppc_cpu_dump_statistics (which will be empty now).
(3) CPUClass.dump_statistics (which now has no setters),
     and cpu_dump_statistics (which is closely related).
(4) hmp_info_cpustats (which now does nothing).


r~

[PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[Bruno Larsen (billionai)]

From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>

Added tlb_set_page and tlb_set_page_with_attrs to the
stubbed functions in exec-all.h  as it is needed
in some functions when compiling without TCG

Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
 include/exec/exec-all.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 6b036cae8f..f287c88282 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -365,6 +365,16 @@ tlb_flush_page_bits_by_mmuidx_all_cpus_synced(CPUState *cpu, target_ulong addr,
                                               uint16_t idxmap, unsigned bits)
 {
 }
+static inline void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
+                             hwaddr paddr, MemTxAttrs attrs,
+                             int prot, int mmu_idx, target_ulong size)
+{
+}
+static inline void tlb_set_page(CPUState *cpu, target_ulong vaddr,
+                  hwaddr paddr, int prot,
+                  int mmu_idx, target_ulong size)
+{
+}
 #endif
 /**
  * probe_access:
-- 
2.17.1

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br>
> 
> Added tlb_set_page and tlb_set_page_with_attrs to the
> stubbed functions in exec-all.h  as it is needed
> in some functions when compiling without TCG
> 
> Signed-off-by: Lucas Mateus Castro (alqotel)<lucas.araujo@eldorado.org.br>
> ---
>   include/exec/exec-all.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)

No, the caller is tcg-specific already.


r~

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[Lucas Mateus Martins Araujo e Castro]

On 12/05/2021 15:34, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>> From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br>
>>
>> Added tlb_set_page and tlb_set_page_with_attrs to the
>> stubbed functions in exec-all.h  as it is needed
>> in some functions when compiling without TCG
>>
>> Signed-off-by: Lucas Mateus Castro 
>> (alqotel)<lucas.araujo@eldorado.org.br>
>> ---
>>   include/exec/exec-all.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>
> No, the caller is tcg-specific already.
>
>
> r~

tlb_set_page is called by many ppc_hash64_handle_mmu_fault, 
ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of 
which from what I've seen are only used inside #if 
defined(CONFIG_SOFTMMU). So what is the best way to deal with these 
tlb_set_page calls? Should these part of the _handle_mmu_fault functions 
never be reached or should these functions never be called?

If it's the latter then should we change the #if defined to #if 
defined(CONFIG_SOFTMMU) && (CONFIG_TCG)?


P.S: There was a miscommunication between me and Bruno, this should've 
been a RFC.

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[Richard Henderson]

On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote:
> tlb_set_page is called by many ppc_hash64_handle_mmu_fault, 
> ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which from 
> what I've seen are only used inside #if defined(CONFIG_SOFTMMU).

tlb_set_page should only be called from one place: ppc_cpu_tlb_fill.  The other 
functions should fill in data, much like get_physical_address.


> So what is the 
> best way to deal with these tlb_set_page calls? Should these part of the 
> _handle_mmu_fault functions never be reached or should these functions never be 
> called?

There is some duplication between get_physical_address* and *handle_mmu_fault 
that should be fixed.

What should be happening is that you have one function (per mmu type) that 
takes a virtual address and resolves a physical address. This bit of code 
should be written so that it is usable by both 
CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill.  It appears as if 
ppc_radix64_xlate is the right interface for this.

It appears that real mode handling is duplicated between hash64 and radix64, 
which could be unified.

You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka 
ppc_cpu_tlb_fill.  TCGCPUOps.tlb_fill is obviously TCG only.

The version you are looking at here is system emulation specific (sysemu, 
!defined(CONFIG_USER_ONLY)).  There is a second version of this function, with 
the same signature, that is used for user emulation in the helpfully named 
user_only_helper.c.


r~

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[David Gibson]

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

On Thu, May 13, 2021 at 06:44:01PM -0500, Richard Henderson wrote:
65;6401;1c> On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote:
> > tlb_set_page is called by many ppc_hash64_handle_mmu_fault,
> > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of
> > which from what I've seen are only used inside #if
> > defined(CONFIG_SOFTMMU).
> 
> tlb_set_page should only be called from one place: ppc_cpu_tlb_fill.  The
> other functions should fill in data, much like get_physical_address.
> 
> 
> > So what is the best way to deal with these tlb_set_page calls? Should
> > these part of the _handle_mmu_fault functions never be reached or should
> > these functions never be called?
> 
> There is some duplication between get_physical_address* and
> *handle_mmu_fault that should be fixed.
> 
> What should be happening is that you have one function (per mmu type) that
> takes a virtual address and resolves a physical address. This bit of code
> should be written so that it is usable by both
> CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill.  It appears as if
> ppc_radix64_xlate is the right interface for this.
> 
> It appears that real mode handling is duplicated between hash64 and radix64,
> which could be unified.

Any common handling between the hash and radix MMUs should go in
mmu-book3s-v3.*  That covers common things across the v3 (POWER9 and
later) MMUs which includes both hash and radix mode.

> You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka
> ppc_cpu_tlb_fill.  TCGCPUOps.tlb_fill is obviously TCG only.
> 
> The version you are looking at here is system emulation specific (sysemu,
> !defined(CONFIG_USER_ONLY)).  There is a second version of this function,
> with the same signature, that is used for user emulation in the helpfully
> named user_only_helper.c.
> 
> 
> r~
> 

-- 
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 09/11] include/exec: added functions to the stubs in exec-all.h

[Lucas Mateus Martins Araujo e Castro]

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


On 17/05/2021 00:58, David Gibson wrote:
> On Thu, May 13, 2021 at 06:44:01PM -0500, Richard Henderson wrote:
> 65;6401;1c> On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote:
>>> tlb_set_page is called by many ppc_hash64_handle_mmu_fault,
>>> ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of
>>> which from what I've seen are only used inside #if
>>> defined(CONFIG_SOFTMMU).
>> tlb_set_page should only be called from one place: ppc_cpu_tlb_fill.  The
>> other functions should fill in data, much like get_physical_address.
>>
>>
>>> So what is the best way to deal with these tlb_set_page calls? Should
>>> these part of the _handle_mmu_fault functions never be reached or should
>>> these functions never be called?
>> There is some duplication between get_physical_address* and
>> *handle_mmu_fault that should be fixed.
>>
>> What should be happening is that you have one function (per mmu type) that
>> takes a virtual address and resolves a physical address. This bit of code
>> should be written so that it is usable by both
>> CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill.  It appears as if
>> ppc_radix64_xlate is the right interface for this.
>>
>> It appears that real mode handling is duplicated between hash64 and radix64,
>> which could be unified.
> Any common handling between the hash and radix MMUs should go in
> mmu-book3s-v3.*  That covers common things across the v3 (POWER9 and
> later) MMUs which includes both hash and radix mode.

I'm not completely sure how this should be handled, there's a 
get_physical_address in mmu_helper.c but it's a static function and 
divided by processor families instead of MMU types, so 
get_physical_address_* should be a new function?

The new get_physical_address_* function would be a mmu-hash(32|64) that 
do something like ppc_radix64_xlate and add a function to mmu-book3s-v3 
that call either the radix64 or the hash64 function and also handle real 
mode access.

Also should the tlb_set_page calls in *_handle_mmu_fault be changed to 
ppc_cpu_tlb_fill or the function should themselves fill it?

>
>> You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka
>> ppc_cpu_tlb_fill.  TCGCPUOps.tlb_fill is obviously TCG only.
>>
>> The version you are looking at here is system emulation specific (sysemu,
>> !defined(CONFIG_USER_ONLY)).  There is a second version of this function,
>> with the same signature, that is used for user emulation in the helpfully
>> named user_only_helper.c.
>>
>>
>> r~
>>
-- 
Lucas Mateus M. Araujo e Castro
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
Estagiario
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

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

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[Richard Henderson]

On 5/17/21 11:59 AM, Lucas Mateus Martins Araujo e Castro wrote:
> I'm not completely sure how this should be handled, there's a 
> get_physical_address in mmu_helper.c but it's a static function and divided by 
> processor families instead of MMU types, so get_physical_address_* should be a 
> new function?
> 
> The new get_physical_address_* function would be a mmu-hash(32|64) that do 
> something like ppc_radix64_xlate and add a function to mmu-book3s-v3 that call 
> either the radix64 or the hash64 function and also handle real mode access.

The entry points that we are concerned about are:
   ppc_cpu_get_phys_page_debug
   ppc_cpu_tlb_fill

Currently there is a hook, pcc->handle_mmu_fault, which is used by 
ppc_cpu_tlb_fill, but is insufficiently general.  We're going to remove that hook.

We're going to add a new hook with the same interface as ppc_radix64_xlate that 
will be used by both ppc_cpu_get_phys_page_debug and ppc_cpu_tlb_fill.

> Also should the tlb_set_page calls in *_handle_mmu_fault be changed to 
> ppc_cpu_tlb_fill or the function should themselves fill it?

Only ppc_cpu_tlb_fill should call tlb_set_page.


r~

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[Bruno Piazera Larsen]

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


On 17/05/2021 15:02, Richard Henderson wrote:
> On 5/17/21 11:59 AM, Lucas Mateus Martins Araujo e Castro wrote:
>> I'm not completely sure how this should be handled, there's a 
>> get_physical_address in mmu_helper.c but it's a static function and 
>> divided by processor families instead of MMU types, so 
>> get_physical_address_* should be a new function?
>>
>> The new get_physical_address_* function would be a mmu-hash(32|64) 
>> that do something like ppc_radix64_xlate and add a function to 
>> mmu-book3s-v3 that call either the radix64 or the hash64 function and 
>> also handle real mode access.
>
> The entry points that we are concerned about are:
>   ppc_cpu_get_phys_page_debug
>   ppc_cpu_tlb_fill
>
> Currently there is a hook, pcc->handle_mmu_fault, which is used by 
> ppc_cpu_tlb_fill, but is insufficiently general.  We're going to 
> remove that hook.
>
> We're going to add a new hook with the same interface as 
> ppc_radix64_xlate that will be used by both 
> ppc_cpu_get_phys_page_debug and ppc_cpu_tlb_fill.
>
So, just to make sure we understand, what we want to do is:

* take all the common code from *_handle_mmu_fault and put it in 
ppc_cpu_tlb_fill

* take whatever is not common and hide it in an equivalent of 
ppc_radix64_xlate

* make the 2 entry points only use these new functions, so we can 
compile ppc_cpu_get_phys_page_debug

* move get_physical_address and all functions called by it somewhere 
that will compile when we disable tcg (again, to compile 
get_phys_page_debug)

Is that it? Sorry if this is very obvious, we never dealt with hardware 
and mmu stuff before...

-- 
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: 2726 bytes --]

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[David Gibson]

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

On Mon, May 17, 2021 at 01:59:35PM -0300, Lucas Mateus Martins Araujo e Castro wrote:
> 
> On 17/05/2021 00:58, David Gibson wrote:
> > On Thu, May 13, 2021 at 06:44:01PM -0500, Richard Henderson wrote:
> > 65;6401;1c> On 5/13/21 9:03 AM, Lucas Mateus Martins Araujo e Castro wrote:
> > > > tlb_set_page is called by many ppc_hash64_handle_mmu_fault,
> > > > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of
> > > > which from what I've seen are only used inside #if
> > > > defined(CONFIG_SOFTMMU).
> > > tlb_set_page should only be called from one place: ppc_cpu_tlb_fill.  The
> > > other functions should fill in data, much like get_physical_address.
> > > 
> > > 
> > > > So what is the best way to deal with these tlb_set_page calls? Should
> > > > these part of the _handle_mmu_fault functions never be reached or should
> > > > these functions never be called?
> > > There is some duplication between get_physical_address* and
> > > *handle_mmu_fault that should be fixed.
> > > 
> > > What should be happening is that you have one function (per mmu type) that
> > > takes a virtual address and resolves a physical address. This bit of code
> > > should be written so that it is usable by both
> > > CPUClass.get_phys_page_attrs_debug and TCGCPUOps.tlb_fill.  It appears as if
> > > ppc_radix64_xlate is the right interface for this.
> > > 
> > > It appears that real mode handling is duplicated between hash64 and radix64,
> > > which could be unified.
> > Any common handling between the hash and radix MMUs should go in
> > mmu-book3s-v3.*  That covers common things across the v3 (POWER9 and
> > later) MMUs which includes both hash and radix mode.
> 
> I'm not completely sure how this should be handled, there's a
> get_physical_address in mmu_helper.c but it's a static function and divided
> by processor families instead of MMU types, so get_physical_address_* should
> be a new function?

Sorry, I wasn't clear.  mmu-v3 is only for things specifically common
to the hash64 model (as implemented in mmu-hash64.c) and the radix
model (as implemented in mmu-radix64.c).  Which basically means things
related to the POWER9 MMU which can switch between those modes.

Things common to *more* MMU models (hash32, 4xx, 8xx, BookE, etc.)
which includes most of what's in mmu-helper.c go elsewhere.

> The new get_physical_address_* function would be a mmu-hash(32|64) that do
> something like ppc_radix64_xlate and add a function to mmu-book3s-v3 that
> call either the radix64 or the hash64 function and also handle real mode
> access.
> 
> Also should the tlb_set_page calls in *_handle_mmu_fault be changed to
> ppc_cpu_tlb_fill or the function should themselves fill it?
> 
> > 
> > > You should only call tlb_set_page from TCGCPUOps.tlb_fill, aka
> > > ppc_cpu_tlb_fill.  TCGCPUOps.tlb_fill is obviously TCG only.
> > > 
> > > The version you are looking at here is system emulation specific (sysemu,
> > > !defined(CONFIG_USER_ONLY)).  There is a second version of this function,
> > > with the same signature, that is used for user emulation in the helpfully
> > > named user_only_helper.c.
> > > 
> > > 
> > > r~
> > > 

-- 
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 09/11] include/exec: added functions to the stubs in exec-all.h

[David Gibson]

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

On Thu, May 13, 2021 at 11:03:24AM -0300, Lucas Mateus Martins Araujo e Castro wrote:
> 
> On 12/05/2021 15:34, Richard Henderson wrote:
> > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> > > From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br>
> > > 
> > > Added tlb_set_page and tlb_set_page_with_attrs to the
> > > stubbed functions in exec-all.h  as it is needed
> > > in some functions when compiling without TCG
> > > 
> > > Signed-off-by: Lucas Mateus Castro
> > > (alqotel)<lucas.araujo@eldorado.org.br>
> > > ---
> > >   include/exec/exec-all.h | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > 
> > No, the caller is tcg-specific already.
> > 
> > 
> > r~
> 
> tlb_set_page is called by many ppc_hash64_handle_mmu_fault,
> ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which
> from what I've seen are only used inside #if defined(CONFIG_SOFTMMU). So
> what is the best way to deal with these tlb_set_page calls? Should these
> part of the _handle_mmu_fault functions never be reached or should
> these

The handle_mmu_fault() functions per se shouldn't be included in a
!SOFTMMU build.  We might have to extract some of their internal logic
for the gdb path, though.

> functions never be called?
> 
> If it's the latter then should we change the #if defined to #if
> defined(CONFIG_SOFTMMU) && (CONFIG_TCG)?

That definitely doesn't make sense.  In practice CONFIG_SOFTMMU == CONFIG_TCG.

> 
> 
> P.S: There was a miscommunication between me and Bruno, this should've been
> a RFC.
> 

-- 
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 09/11] include/exec: added functions to the stubs in exec-all.h

[Bruno Piazera Larsen]

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


On 17/05/2021 00:55, David Gibson wrote:
> On Thu, May 13, 2021 at 11:03:24AM -0300, Lucas Mateus Martins Araujo e Castro wrote:
>> On 12/05/2021 15:34, Richard Henderson wrote:
>>> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>>>> From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br>
>>>>
>>>> Added tlb_set_page and tlb_set_page_with_attrs to the
>>>> stubbed functions in exec-all.h  as it is needed
>>>> in some functions when compiling without TCG
>>>>
>>>> Signed-off-by: Lucas Mateus Castro
>>>> (alqotel)<lucas.araujo@eldorado.org.br>
>>>> ---
>>>>    include/exec/exec-all.h | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>> No, the caller is tcg-specific already.
>>>
>>>
>>> r~
>> tlb_set_page is called by many ppc_hash64_handle_mmu_fault,
>> ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which
>> from what I've seen are only used inside #if defined(CONFIG_SOFTMMU). So
>> what is the best way to deal with these tlb_set_page calls? Should these
>> part of the _handle_mmu_fault functions never be reached or should
>> these
> The handle_mmu_fault() functions per se shouldn't be included in a
> !SOFTMMU build.  We might have to extract some of their internal logic
> for the gdb path, though.
>
>> functions never be called?
>>
>> If it's the latter then should we change the #if defined to #if
>> defined(CONFIG_SOFTMMU) && (CONFIG_TCG)?
> That definitely doesn't make sense.  In practice CONFIG_SOFTMMU == CONFIG_TCG.
We figured it was the case, but from what I can tell, CONFIG_SOFTMMU is 
set when parsing the target list (in the configure script) and 
CONFIG_TCG is set later, when parsing which accelerators were requested. 
So even though SOFTMMU should imply TCG, the way it is coded right now 
doesn't. We could also try and change the configure script, but neither 
of us is really good with bash scripts, so this was the next best 
solution we came up with.
>
>>
>> P.S: There was a miscommunication between me and Bruno, this should've been
>> a RFC.
>>
-- 
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: 3793 bytes --]

Re: [PATCH 09/11] include/exec: added functions to the stubs in exec-all.h

[David Gibson]

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

On Mon, May 17, 2021 at 08:07:24AM -0300, Bruno Piazera Larsen wrote:
> 
> On 17/05/2021 00:55, David Gibson wrote:
> > On Thu, May 13, 2021 at 11:03:24AM -0300, Lucas Mateus Martins Araujo e Castro wrote:
> > > On 12/05/2021 15:34, Richard Henderson wrote:
> > > > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> > > > > From: "Lucas Mateus Castro (alqotel)"<lucas.araujo@eldorado.org.br>
> > > > > 
> > > > > Added tlb_set_page and tlb_set_page_with_attrs to the
> > > > > stubbed functions in exec-all.h  as it is needed
> > > > > in some functions when compiling without TCG
> > > > > 
> > > > > Signed-off-by: Lucas Mateus Castro
> > > > > (alqotel)<lucas.araujo@eldorado.org.br>
> > > > > ---
> > > > >    include/exec/exec-all.h | 10 ++++++++++
> > > > >    1 file changed, 10 insertions(+)
> > > > No, the caller is tcg-specific already.
> > > > 
> > > > 
> > > > r~
> > > tlb_set_page is called by many ppc_hash64_handle_mmu_fault,
> > > ppc_radix64_handle_mmu_fault and ppc_hash32_handle_mmu_fault, all of which
> > > from what I've seen are only used inside #if defined(CONFIG_SOFTMMU). So
> > > what is the best way to deal with these tlb_set_page calls? Should these
> > > part of the _handle_mmu_fault functions never be reached or should
> > > these
> > The handle_mmu_fault() functions per se shouldn't be included in a
> > !SOFTMMU build.  We might have to extract some of their internal logic
> > for the gdb path, though.
> > 
> > > functions never be called?
> > > 
> > > If it's the latter then should we change the #if defined to #if
> > > defined(CONFIG_SOFTMMU) && (CONFIG_TCG)?
> > That definitely doesn't make sense.  In practice CONFIG_SOFTMMU == CONFIG_TCG.

Ugh.. sorry.. I was confused again.  In practice CONFIG_SOFTMMU ==
!CONFIG_USER_ONLY.


> We figured it was the case, but from what I can tell, CONFIG_SOFTMMU is set
> when parsing the target list (in the configure script) and CONFIG_TCG is set
> later, when parsing which accelerators were requested. So even though
> SOFTMMU should imply TCG, the way it is coded right now doesn't. We could
> also try and change the configure script, but neither of us is really good
> with bash scripts, so this was the next best solution we came up with.
> > 
> > > 
> > > P.S: There was a miscommunication between me and Bruno, this should've been
> > > a RFC.
> > > 

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

[RFC PATCH 10/11] target/ppc: created tcg-stub.c file

[Bruno Larsen (billionai)]

Created a file with stubs needed to compile disabling TCG.

We're not sure about keeping the softmmu stubs in this file. if there is
a better place to put them, please let us know.

The other 3 functions have been stubbed because we didn't know what to
do with them. Making the file compile in the !TCG case would create an
ifdef hell, but extracting the functions meant moving many others as
well, and there weren't any good places to put them.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/tcg-stub.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 target/ppc/tcg-stub.c

diff --git a/target/ppc/tcg-stub.c b/target/ppc/tcg-stub.c
new file mode 100644
index 0000000000..67099e2676
--- /dev/null
+++ b/target/ppc/tcg-stub.c
@@ -0,0 +1,33 @@
+
+#include "qemu/osdep.h"
+#include "exec/hwaddr.h"
+#include "cpu.h"
+#include "hw/ppc/spapr.h"
+
+hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
+{
+    return 0;
+}
+
+void dump_mmu(CPUPPCState *env)
+{
+}
+
+void ppc_tlb_invalidate_all(CPUPPCState *env)
+{
+}
+
+target_ulong softmmu_resize_hpt_prepare(PowerPCCPU *cpu,
+                                        SpaprMachineState *spapr,
+                                        target_ulong shift)
+{
+    g_assert_not_reached();
+}
+
+target_ulong softmmu_resize_hpt_commit(PowerPCCPU* cpu,
+                                       SpaprMachineState *spapr,
+                                       target_ulong flags,
+                                       target_ulong shift)
+{
+    g_assert_not_reached();
+}
-- 
2.17.1

Re: [RFC PATCH 10/11] target/ppc: created tcg-stub.c file

[Richard Henderson]

On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> +++ b/target/ppc/tcg-stub.c
> @@ -0,0 +1,33 @@
> +
> +#include "qemu/osdep.h"

All files get copyright boilerplate.

> +#include "exec/hwaddr.h"
> +#include "cpu.h"
> +#include "hw/ppc/spapr.h"
> +
> +hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> +{
> +    return 0;
> +}

This is used by gdbstub.

If there's a way for kvm to convert a virtual address to a physical address 
using the hardware, then use that.  I suspect there is not.

Otherwise, you have to keep all of the mmu page table walking stuff for kvm as 
well as tcg.  Which probably means that all of the other stuff that you're 
stubbing out is used or usable as well.


r~

Re: [RFC PATCH 10/11] target/ppc: created tcg-stub.c file

[Bruno Piazera Larsen]

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


On 12/05/2021 15:39, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>> +++ b/target/ppc/tcg-stub.c
>> @@ -0,0 +1,33 @@
>> +
>> +#include "qemu/osdep.h"
>
> All files get copyright boilerplate.
yeah, I didn't expect this file to stick around, though, as the last 
time we made a stub file, it ended up not being used, so I decided to go 
the quick route
>
>> +#include "exec/hwaddr.h"
>> +#include "cpu.h"
>> +#include "hw/ppc/spapr.h"
>> +
>> +hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>> +{
>> +    return 0;
>> +}
>
> This is used by gdbstub.
>
> If there's a way for kvm to convert a virtual address to a physical 
> address using the hardware, then use that.  I suspect there is not.
>
> Otherwise, you have to keep all of the mmu page table walking stuff 
> for kvm as well as tcg.  Which probably means that all of the other 
> stuff that you're stubbing out is used or usable as well.
ah, this probably means we'll need to compile mmu_helper.c too... that 
was something we were hoping to avoid, because of the sheer size.
>
>
> r~
-- 
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: 2566 bytes --]

Re: [RFC PATCH 10/11] target/ppc: created tcg-stub.c file

[Bruno Piazera Larsen]

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


On 12/05/2021 15:39, Richard Henderson wrote:
> On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
>> +++ b/target/ppc/tcg-stub.c
>> @@ -0,0 +1,33 @@
>> +
>> +#include "qemu/osdep.h"
>
> All files get copyright boilerplate.
>
>> +#include "exec/hwaddr.h"
>> +#include "cpu.h"
>> +#include "hw/ppc/spapr.h"
>> +
>> +hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>> +{
>> +    return 0;
>> +}
>
> This is used by gdbstub.
>
> If there's a way for kvm to convert a virtual address to a physical 
> address using the hardware, then use that.  I suspect there is not.
>
> Otherwise, you have to keep all of the mmu page table walking stuff 
> for kvm as well as tcg.  Which probably means that all of the other 
> stuff that you're stubbing out is used or usable as well.

 From what I can tell, KVM can't do it, so we'll have to extract the 
function. Looking at it, the main problem is that it might call 
get_physical_address and use struct mmu_ctx_t, if the mmu_model isn't 
one of: POWERPC_MMU_64B, POWERPC_MMU_2_03, POWERPC_MMU_2_06, 
POWERPC_MMU_2_07, POWERPC_MMU_3_00, POWERPC_MMU_32B, POWERPC_MMU_601.

Is it possible that a machine with an mmu not listed in here could build 
a !TCG version of qemu? if it's not possible, we can separate that part 
into a separate function left in mmu_helper.c and move the rest to 
somewhere else.

Looking at dump_mmu and ppc_tlb_invalidate_all, looks like we need to 
move enough code that it make sense to create an mmu_common.c for common 
code. Otherwise, it's probably easier to compile all of mmu_helper.c 
instead of picking those functions out.

-- 

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: 2920 bytes --]

Re: [RFC PATCH 10/11] target/ppc: created tcg-stub.c file

[David Gibson]

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

On Wed, May 12, 2021 at 11:08:12AM -0300, Bruno Larsen (billionai) wrote:
> Created a file with stubs needed to compile disabling TCG.
> 
> We're not sure about keeping the softmmu stubs in this file. if there is
> a better place to put them, please let us know.
> 
> The other 3 functions have been stubbed because we didn't know what to
> do with them. Making the file compile in the !TCG case would create an
> ifdef hell, but extracting the functions meant moving many others as
> well, and there weren't any good places to put them.
> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/tcg-stub.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 target/ppc/tcg-stub.c
> 
> diff --git a/target/ppc/tcg-stub.c b/target/ppc/tcg-stub.c
> new file mode 100644
> index 0000000000..67099e2676
> --- /dev/null
> +++ b/target/ppc/tcg-stub.c
> @@ -0,0 +1,33 @@
> +
> +#include "qemu/osdep.h"
> +#include "exec/hwaddr.h"
> +#include "cpu.h"
> +#include "hw/ppc/spapr.h"
> +
> +hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> +{
> +    return 0;
> +}
> +
> +void dump_mmu(CPUPPCState *env)
> +{
> +}
> +
> +void ppc_tlb_invalidate_all(CPUPPCState *env)
> +{
> +}
> +
> +target_ulong softmmu_resize_hpt_prepare(PowerPCCPU *cpu,
> +                                        SpaprMachineState *spapr,
> +                                        target_ulong shift)
> +{
> +    g_assert_not_reached();
> +}
> +
> +target_ulong softmmu_resize_hpt_commit(PowerPCCPU* cpu,
> +                                       SpaprMachineState *spapr,
> +                                       target_ulong flags,
> +                                       target_ulong shift)
> +{
> +    g_assert_not_reached();
> +}

I think these last two stubs should be obsoleted by the patch from
Lucas I already merged "hw/ppc: moved hcalls that depend on softmmu".

-- 
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: [RFC PATCH 10/11] target/ppc: created tcg-stub.c file

[Bruno Piazera Larsen]

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


On 13/05/2021 00:59, David Gibson wrote:
> On Wed, May 12, 2021 at 11:08:12AM -0300, Bruno Larsen (billionai) wrote:
>> Created a file with stubs needed to compile disabling TCG.
>>
>> We're not sure about keeping the softmmu stubs in this file. if there is
>> a better place to put them, please let us know.
>>
>> The other 3 functions have been stubbed because we didn't know what to
>> do with them. Making the file compile in the !TCG case would create an
>> ifdef hell, but extracting the functions meant moving many others as
>> well, and there weren't any good places to put them.
>>
>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
>> ---
>>   target/ppc/tcg-stub.c | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>   create mode 100644 target/ppc/tcg-stub.c
>>
>> diff --git a/target/ppc/tcg-stub.c b/target/ppc/tcg-stub.c
>> new file mode 100644
>> index 0000000000..67099e2676
>> --- /dev/null
>> +++ b/target/ppc/tcg-stub.c
>> @@ -0,0 +1,33 @@
>> +
>> +#include "qemu/osdep.h"
>> +#include "exec/hwaddr.h"
>> +#include "cpu.h"
>> +#include "hw/ppc/spapr.h"
>> +
>> +hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>> +{
>> +    return 0;
>> +}
>> +
>> +void dump_mmu(CPUPPCState *env)
>> +{
>> +}
>> +
>> +void ppc_tlb_invalidate_all(CPUPPCState *env)
>> +{
>> +}
>> +
>> +target_ulong softmmu_resize_hpt_prepare(PowerPCCPU *cpu,
>> +                                        SpaprMachineState *spapr,
>> +                                        target_ulong shift)
>> +{
>> +    g_assert_not_reached();
>> +}
>> +
>> +target_ulong softmmu_resize_hpt_commit(PowerPCCPU* cpu,
>> +                                       SpaprMachineState *spapr,
>> +                                       target_ulong flags,
>> +                                       target_ulong shift)
>> +{
>> +    g_assert_not_reached();
>> +}
> I think these last two stubs should be obsoleted by the patch from
> Lucas I already merged "hw/ppc: moved hcalls that depend on softmmu".

They aren't, when talking to him he said he wanted to use as few ifdefs 
as possible. Which do you think is better, to go back and ifdef away 
those calls, or keep the stubs? And if we keep the stubs, do we keep 
them here or in hw/ppc/spapr_hcall.c, along with other stubs?

-- 

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: 3197 bytes --]

Re: [RFC PATCH 10/11] target/ppc: created tcg-stub.c file

[David Gibson]

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

On Thu, May 13, 2021 at 09:56:27AM -0300, Bruno Piazera Larsen wrote:
> 
> On 13/05/2021 00:59, David Gibson wrote:
> > On Wed, May 12, 2021 at 11:08:12AM -0300, Bruno Larsen (billionai) wrote:
> > > Created a file with stubs needed to compile disabling TCG.
> > > 
> > > We're not sure about keeping the softmmu stubs in this file. if there is
> > > a better place to put them, please let us know.
> > > 
> > > The other 3 functions have been stubbed because we didn't know what to
> > > do with them. Making the file compile in the !TCG case would create an
> > > ifdef hell, but extracting the functions meant moving many others as
> > > well, and there weren't any good places to put them.
> > > 
> > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > > ---
> > >   target/ppc/tcg-stub.c | 33 +++++++++++++++++++++++++++++++++
> > >   1 file changed, 33 insertions(+)
> > >   create mode 100644 target/ppc/tcg-stub.c
> > > 
> > > diff --git a/target/ppc/tcg-stub.c b/target/ppc/tcg-stub.c
> > > new file mode 100644
> > > index 0000000000..67099e2676
> > > --- /dev/null
> > > +++ b/target/ppc/tcg-stub.c
> > > @@ -0,0 +1,33 @@
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "exec/hwaddr.h"
> > > +#include "cpu.h"
> > > +#include "hw/ppc/spapr.h"
> > > +
> > > +hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > > +void dump_mmu(CPUPPCState *env)
> > > +{
> > > +}
> > > +
> > > +void ppc_tlb_invalidate_all(CPUPPCState *env)
> > > +{
> > > +}
> > > +
> > > +target_ulong softmmu_resize_hpt_prepare(PowerPCCPU *cpu,
> > > +                                        SpaprMachineState *spapr,
> > > +                                        target_ulong shift)
> > > +{
> > > +    g_assert_not_reached();
> > > +}
> > > +
> > > +target_ulong softmmu_resize_hpt_commit(PowerPCCPU* cpu,
> > > +                                       SpaprMachineState *spapr,
> > > +                                       target_ulong flags,
> > > +                                       target_ulong shift)
> > > +{
> > > +    g_assert_not_reached();
> > > +}
> > I think these last two stubs should be obsoleted by the patch from
> > Lucas I already merged "hw/ppc: moved hcalls that depend on softmmu".
> 
> They aren't,

Ah, sorry.  I forgot (again) that the resize_hpt stuff is a bit
different from the other kvm-implemented mmu hypercalls.

> when talking to him he said he wanted to use as few ifdefs as
> possible. Which do you think is better, to go back and ifdef away those
> calls, or keep the stubs? And if we keep the stubs, do we keep them here or
> in hw/ppc/spapr_hcall.c, along with other stubs?

Hmm.. I don't think you should need to do either.  IIUC, when in a
!TCG build, kvm_enabled() should evaluate to true at compile time.  In
which case as long as the calls to these functions are protected by an
if (!kvm_enabled()) the compiler should be able to just figure it out
without stubs.

-- 
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 11/11] target/ppc: updated meson.build to support disable-tcg

[Bruno Larsen (billionai)]

updated build file to not compile some sources that are unnecessary if
TCG is disabled on the system.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/meson.build | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/target/ppc/meson.build b/target/ppc/meson.build
index d1aa7d5d39..d9e599b00f 100644
--- a/target/ppc/meson.build
+++ b/target/ppc/meson.build
@@ -3,11 +3,14 @@ ppc_ss.add(files(
   'cpu-models.c',
   'cpu.c',
   'cpu_init.c',
-  'dfp_helper.c',
   'excp_helper.c',
-  'fpu_helper.c',
   'gdbstub.c',
   'helper_regs.c',
+))
+
+ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
+  'dfp_helper.c',
+  'fpu_helper.c',
   'int_helper.c',
   'mem_helper.c',
   'misc_helper.c',
@@ -25,9 +28,14 @@ ppc_softmmu_ss.add(files(
   'arch_dump.c',
   'machine.c',
   'mmu-hash32.c',
-  'mmu_helper.c',
   'monitor.c',
 ))
+
+ppc_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files(
+  'mmu_helper.c',
+), if_false: files(
+  'tcg-stub.c',
+))
 ppc_softmmu_ss.add(when: 'TARGET_PPC64', if_true: files(
   'compat.c',
   'mmu-book3s-v3.c',
-- 
2.17.1