[PATCH v2 00/15] target/ppc: Move TCG code from excp_helper.c to tcg-excp_helper.c

[PATCH v2 00/15] target/ppc: Move TCG code from excp_helper.c to tcg-excp_helper.c

[Philippe Mathieu-Daudé]

Since v1:
- Keep ppc_tcg_hv_emu() within TARGET_PPC64 (patch #10)

Hi,

This series is a simply a cleanup restricting TCG specific
exception-related code to TCG, by moving code to a new unit
named 'tcg-excp_helper.c'.

I ended doing it as a preliminary cleanup for the "Extract
TCG state from CPUState".

Diffstat shows 1K lines moved, but the patches are trivial
to review using 'git-diff --color-moved=dimmed-zebra' option.

Branch published as:
 https://gitlab.com/philmd/qemu/-/tree/ppc_excp_extract_tcg

Regards,

Phil.

Philippe Mathieu-Daudé (15):
  hw/ppc/spapr: Restrict CONFER hypercall to TCG
  hw/ppc/spapr: Restrict part of PAGE_INIT hypercall to TCG
  target/ppc: Make ppc_ldl_code() declaration public
  target/ppc: Move TCG specific exception handlers to tcg-excp_helper.c
  target/ppc: Move ppc_ldl_code() to tcg-excp_helper.c
  target/ppc: Ensure powerpc_checkstop() is only called under TCG
  target/ppc: Restrict powerpc_checkstop() to TCG
  target/ppc: Remove raise_exception_ra()
  target/ppc: Restrict exception helpers to TCG
  target/ppc: Restrict ppc_tcg_hv_emu() to TCG
  target/ppc: Restrict various common helpers to TCG
  target/ppc: Fix style in excp_helper.c
  target/ppc: Make powerpc_excp() prototype public
  target/ppc: Restrict ATTN / SCV / PMINSN helpers to TCG
  target/ppc: Restrict various system helpers to TCG

 target/ppc/cpu.h             |   5 -
 target/ppc/internal.h        |  11 +-
 hw/ppc/spapr_hcall.c         |   6 +-
 target/ppc/excp_helper.c     | 943 +----------------------------------
 target/ppc/tcg-excp_helper.c | 924 ++++++++++++++++++++++++++++++++++
 target/ppc/meson.build       |   1 +
 6 files changed, 955 insertions(+), 935 deletions(-)
 create mode 100644 target/ppc/tcg-excp_helper.c

-- 
2.47.1

[PATCH v2 01/15] hw/ppc/spapr: Restrict CONFER hypercall to TCG

[Philippe Mathieu-Daudé]

TODO: Add PPC folks why :)

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr_hcall.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f8ab7670630..dbf30358a1a 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -578,6 +578,8 @@ static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
     CPUState *cs = CPU(cpu);
     SpaprCpuState *spapr_cpu;
 
+    assert(tcg_enabled()); /* KVM will have handled this */
+
     /*
      * -1 means confer to all other CPUs without dispatch counter check,
      *  otherwise it's a targeted confer.
-- 
2.47.1

Re: [PATCH v2 01/15] hw/ppc/spapr: Restrict CONFER hypercall to TCG

[Harsh Prateek Bora]

Hi Philippe,

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> TODO: Add PPC folks why :)

While this appear be TCG specific, may I know what caused you to bring 
this change? Usually we have blanks stubs for hcalls in KVM mode which 
are only TCG specific. That helps in avoiding building TCG specific code 
when building with TCG disabled.

Nick, thoughts?

regards,
Harsh

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/spapr_hcall.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f8ab7670630..dbf30358a1a 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -578,6 +578,8 @@ static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       CPUState *cs = CPU(cpu);
>       SpaprCpuState *spapr_cpu;
>   
> +    assert(tcg_enabled()); /* KVM will have handled this */
> +
>       /*
>        * -1 means confer to all other CPUs without dispatch counter check,
>        *  otherwise it's a targeted confer.

[PATCH v2 02/15] hw/ppc/spapr: Restrict part of PAGE_INIT hypercall to TCG

[Philippe Mathieu-Daudé]

Restrict the tb_flush() call to TCG. Assert we are using KVM or TCG.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr_hcall.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index dbf30358a1a..4f1933b8da6 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -299,8 +299,10 @@ static target_ulong h_page_init(PowerPCCPU *cpu, SpaprMachineState *spapr,
     if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
         if (kvm_enabled()) {
             kvmppc_icbi_range(cpu, pdst, len);
-        } else {
+        } else if (tcg_enabled()) {
             tb_flush(CPU(cpu));
+        } else {
+            g_assert_not_reached();
         }
     }
 
-- 
2.47.1

Re: [PATCH v2 02/15] hw/ppc/spapr: Restrict part of PAGE_INIT hypercall to TCG

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Restrict the tb_flush() call to TCG. Assert we are using KVM or TCG.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   hw/ppc/spapr_hcall.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index dbf30358a1a..4f1933b8da6 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -299,8 +299,10 @@ static target_ulong h_page_init(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
>           if (kvm_enabled()) {
>               kvmppc_icbi_range(cpu, pdst, len);
> -        } else {
> +        } else if (tcg_enabled()) {
>               tb_flush(CPU(cpu));
> +        } else {
> +            g_assert_not_reached();
>           }
>       }
>   

[PATCH v2 03/15] target/ppc: Make ppc_ldl_code() declaration public

[Philippe Mathieu-Daudé]

We are going to move code calling ppc_ldl_code() out of
excp_helper.c where it is defined. Expose its declaration
for few commits, until eventually making it static again
once everything is moved.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/internal.h    | 2 ++
 target/ppc/excp_helper.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 20fb2ec593c..46db6adfcf6 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -268,6 +268,8 @@ static inline void pte_invalidate(target_ulong *pte0)
 #define PTE_PTEM_MASK 0x7FFFFFBF
 #define PTE_CHECK_MASK (TARGET_PAGE_MASK | 0x7B)
 
+uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr);
+
 #ifdef CONFIG_USER_ONLY
 void ppc_cpu_record_sigsegv(CPUState *cs, vaddr addr,
                             MMUAccessType access_type,
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index fde9912230e..7ed4bbec035 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -144,7 +144,7 @@ static inline bool insn_need_byteswap(CPUArchState *env)
     return !!(env->msr & ((target_ulong)1 << MSR_LE));
 }
 
-static uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
+uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
 {
     uint32_t insn = cpu_ldl_code(env, addr);
 
-- 
2.47.1

Re: [PATCH v2 03/15] target/ppc: Make ppc_ldl_code() declaration public

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> We are going to move code calling ppc_ldl_code() out of
> excp_helper.c where it is defined. Expose its declaration
> for few commits, until eventually making it static again
> once everything is moved.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   target/ppc/internal.h    | 2 ++
>   target/ppc/excp_helper.c | 2 +-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index 20fb2ec593c..46db6adfcf6 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -268,6 +268,8 @@ static inline void pte_invalidate(target_ulong *pte0)
>   #define PTE_PTEM_MASK 0x7FFFFFBF
>   #define PTE_CHECK_MASK (TARGET_PAGE_MASK | 0x7B)
>   
> +uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr);
> +
>   #ifdef CONFIG_USER_ONLY
>   void ppc_cpu_record_sigsegv(CPUState *cs, vaddr addr,
>                               MMUAccessType access_type,
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index fde9912230e..7ed4bbec035 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -144,7 +144,7 @@ static inline bool insn_need_byteswap(CPUArchState *env)
>       return !!(env->msr & ((target_ulong)1 << MSR_LE));
>   }
>   
> -static uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
> +uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
>   {
>       uint32_t insn = cpu_ldl_code(env, addr);
>   

[PATCH v2 04/15] target/ppc: Move TCG specific exception handlers to tcg-excp_helper.c

[Philippe Mathieu-Daudé]

Move the TCGCPUOps handlers to a new unit: tcg-excp_helper.c,
only built when TCG is selected.

See in target/ppc/cpu_init.c:

    #ifdef CONFIG_TCG
    static const TCGCPUOps ppc_tcg_ops = {
      ...
      .do_unaligned_access = ppc_cpu_do_unaligned_access,
      .do_transaction_failed = ppc_cpu_do_transaction_failed,
      .debug_excp_handler = ppc_cpu_debug_excp_handler,
      .debug_check_breakpoint = ppc_cpu_debug_check_breakpoint,
      .debug_check_watchpoint = ppc_cpu_debug_check_watchpoint,
    };
    #endif /* CONFIG_TCG */

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/excp_helper.c     | 173 ------------------------------
 target/ppc/tcg-excp_helper.c | 202 +++++++++++++++++++++++++++++++++++
 target/ppc/meson.build       |   1 +
 3 files changed, 203 insertions(+), 173 deletions(-)
 create mode 100644 target/ppc/tcg-excp_helper.c

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 7ed4bbec035..b05eb7f5aec 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -3144,178 +3144,5 @@ void helper_book3s_trace(CPUPPCState *env, target_ulong prev_ip)
     raise_exception_err(env, POWERPC_EXCP_TRACE, error_code);
 }
 
-void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
-                                 MMUAccessType access_type,
-                                 int mmu_idx, uintptr_t retaddr)
-{
-    CPUPPCState *env = cpu_env(cs);
-    uint32_t insn;
-
-    /* Restore state and reload the insn we executed, for filling in DSISR.  */
-    cpu_restore_state(cs, retaddr);
-    insn = ppc_ldl_code(env, env->nip);
-
-    switch (env->mmu_model) {
-    case POWERPC_MMU_SOFT_4xx:
-        env->spr[SPR_40x_DEAR] = vaddr;
-        break;
-    case POWERPC_MMU_BOOKE:
-    case POWERPC_MMU_BOOKE206:
-        env->spr[SPR_BOOKE_DEAR] = vaddr;
-        break;
-    default:
-        env->spr[SPR_DAR] = vaddr;
-        break;
-    }
-
-    cs->exception_index = POWERPC_EXCP_ALIGN;
-    env->error_code = insn & 0x03FF0000;
-    cpu_loop_exit(cs);
-}
-
-void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
-                                   vaddr vaddr, unsigned size,
-                                   MMUAccessType access_type,
-                                   int mmu_idx, MemTxAttrs attrs,
-                                   MemTxResult response, uintptr_t retaddr)
-{
-    CPUPPCState *env = cpu_env(cs);
-
-    switch (env->excp_model) {
-#if defined(TARGET_PPC64)
-    case POWERPC_EXCP_POWER8:
-    case POWERPC_EXCP_POWER9:
-    case POWERPC_EXCP_POWER10:
-    case POWERPC_EXCP_POWER11:
-        /*
-         * Machine check codes can be found in processor User Manual or
-         * Linux or skiboot source.
-         */
-        if (access_type == MMU_DATA_LOAD) {
-            env->spr[SPR_DAR] = vaddr;
-            env->spr[SPR_DSISR] = PPC_BIT(57);
-            env->error_code = PPC_BIT(42);
-
-        } else if (access_type == MMU_DATA_STORE) {
-            /*
-             * MCE for stores in POWER is asynchronous so hardware does
-             * not set DAR, but QEMU can do better.
-             */
-            env->spr[SPR_DAR] = vaddr;
-            env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45);
-            env->error_code |= PPC_BIT(42);
-
-        } else { /* Fetch */
-            /*
-             * is_prefix_insn_excp() tests !PPC_BIT(42) to avoid fetching
-             * the instruction, so that must always be clear for fetches.
-             */
-            env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45);
-        }
-        break;
-#endif
-    default:
-        /*
-         * TODO: Check behaviour for other CPUs, for now do nothing.
-         * Could add a basic MCE even if real hardware ignores.
-         */
-        return;
-    }
-
-    cs->exception_index = POWERPC_EXCP_MCHECK;
-    cpu_loop_exit_restore(cs, retaddr);
-}
-
-void ppc_cpu_debug_excp_handler(CPUState *cs)
-{
-#if defined(TARGET_PPC64)
-    CPUPPCState *env = cpu_env(cs);
-
-    if (env->insns_flags2 & PPC2_ISA207S) {
-        if (cs->watchpoint_hit) {
-            if (cs->watchpoint_hit->flags & BP_CPU) {
-                env->spr[SPR_DAR] = cs->watchpoint_hit->hitaddr;
-                env->spr[SPR_DSISR] = PPC_BIT(41);
-                cs->watchpoint_hit = NULL;
-                raise_exception(env, POWERPC_EXCP_DSI);
-            }
-            cs->watchpoint_hit = NULL;
-        } else if (cpu_breakpoint_test(cs, env->nip, BP_CPU)) {
-            raise_exception_err(env, POWERPC_EXCP_TRACE,
-                                PPC_BIT(33) | PPC_BIT(43));
-        }
-    }
-#endif
-}
-
-bool ppc_cpu_debug_check_breakpoint(CPUState *cs)
-{
-#if defined(TARGET_PPC64)
-    CPUPPCState *env = cpu_env(cs);
-
-    if (env->insns_flags2 & PPC2_ISA207S) {
-        target_ulong priv;
-
-        priv = env->spr[SPR_CIABR] & PPC_BITMASK(62, 63);
-        switch (priv) {
-        case 0x1: /* problem */
-            return env->msr & ((target_ulong)1 << MSR_PR);
-        case 0x2: /* supervisor */
-            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
-                    !(env->msr & ((target_ulong)1 << MSR_HV)));
-        case 0x3: /* hypervisor */
-            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
-                     (env->msr & ((target_ulong)1 << MSR_HV)));
-        default:
-            g_assert_not_reached();
-        }
-    }
-#endif
-
-    return false;
-}
-
-bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
-{
-#if defined(TARGET_PPC64)
-    CPUPPCState *env = cpu_env(cs);
-
-    if (env->insns_flags2 & PPC2_ISA207S) {
-        if (wp == env->dawr0_watchpoint) {
-            uint32_t dawrx = env->spr[SPR_DAWRX0];
-            bool wt = extract32(dawrx, PPC_BIT_NR(59), 1);
-            bool wti = extract32(dawrx, PPC_BIT_NR(60), 1);
-            bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
-            bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
-            bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
-
-            if ((env->msr & ((target_ulong)1 << MSR_PR)) && !pr) {
-                return false;
-            } else if ((env->msr & ((target_ulong)1 << MSR_HV)) && !hv) {
-                return false;
-            } else if (!sv) {
-                return false;
-            }
-
-            if (!wti) {
-                if (env->msr & ((target_ulong)1 << MSR_DR)) {
-                    if (!wt) {
-                        return false;
-                    }
-                } else {
-                    if (wt) {
-                        return false;
-                    }
-                }
-            }
-
-            return true;
-        }
-    }
-#endif
-
-    return false;
-}
-
 #endif /* !CONFIG_USER_ONLY */
 #endif /* CONFIG_TCG */
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
new file mode 100644
index 00000000000..3402dbe05ee
--- /dev/null
+++ b/target/ppc/tcg-excp_helper.c
@@ -0,0 +1,202 @@
+/*
+ *  PowerPC exception emulation helpers for QEMU (TCG specific)
+ *
+ *  Copyright (c) 2003-2007 Jocelyn Mayer
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include "qemu/osdep.h"
+#include "exec/cpu_ldst.h"
+
+#include "hw/ppc/ppc.h"
+#include "internal.h"
+#include "cpu.h"
+#include "trace.h"
+
+#ifndef CONFIG_USER_ONLY
+
+void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
+                                 MMUAccessType access_type,
+                                 int mmu_idx, uintptr_t retaddr)
+{
+    CPUPPCState *env = cpu_env(cs);
+    uint32_t insn;
+
+    /* Restore state and reload the insn we executed, for filling in DSISR.  */
+    cpu_restore_state(cs, retaddr);
+    insn = ppc_ldl_code(env, env->nip);
+
+    switch (env->mmu_model) {
+    case POWERPC_MMU_SOFT_4xx:
+        env->spr[SPR_40x_DEAR] = vaddr;
+        break;
+    case POWERPC_MMU_BOOKE:
+    case POWERPC_MMU_BOOKE206:
+        env->spr[SPR_BOOKE_DEAR] = vaddr;
+        break;
+    default:
+        env->spr[SPR_DAR] = vaddr;
+        break;
+    }
+
+    cs->exception_index = POWERPC_EXCP_ALIGN;
+    env->error_code = insn & 0x03FF0000;
+    cpu_loop_exit(cs);
+}
+
+void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                   vaddr vaddr, unsigned size,
+                                   MMUAccessType access_type,
+                                   int mmu_idx, MemTxAttrs attrs,
+                                   MemTxResult response, uintptr_t retaddr)
+{
+    CPUPPCState *env = cpu_env(cs);
+
+    switch (env->excp_model) {
+#if defined(TARGET_PPC64)
+    case POWERPC_EXCP_POWER8:
+    case POWERPC_EXCP_POWER9:
+    case POWERPC_EXCP_POWER10:
+    case POWERPC_EXCP_POWER11:
+        /*
+         * Machine check codes can be found in processor User Manual or
+         * Linux or skiboot source.
+         */
+        if (access_type == MMU_DATA_LOAD) {
+            env->spr[SPR_DAR] = vaddr;
+            env->spr[SPR_DSISR] = PPC_BIT(57);
+            env->error_code = PPC_BIT(42);
+
+        } else if (access_type == MMU_DATA_STORE) {
+            /*
+             * MCE for stores in POWER is asynchronous so hardware does
+             * not set DAR, but QEMU can do better.
+             */
+            env->spr[SPR_DAR] = vaddr;
+            env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45);
+            env->error_code |= PPC_BIT(42);
+
+        } else { /* Fetch */
+            /*
+             * is_prefix_insn_excp() tests !PPC_BIT(42) to avoid fetching
+             * the instruction, so that must always be clear for fetches.
+             */
+            env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45);
+        }
+        break;
+#endif
+    default:
+        /*
+         * TODO: Check behaviour for other CPUs, for now do nothing.
+         * Could add a basic MCE even if real hardware ignores.
+         */
+        return;
+    }
+
+    cs->exception_index = POWERPC_EXCP_MCHECK;
+    cpu_loop_exit_restore(cs, retaddr);
+}
+
+void ppc_cpu_debug_excp_handler(CPUState *cs)
+{
+#if defined(TARGET_PPC64)
+    CPUPPCState *env = cpu_env(cs);
+
+    if (env->insns_flags2 & PPC2_ISA207S) {
+        if (cs->watchpoint_hit) {
+            if (cs->watchpoint_hit->flags & BP_CPU) {
+                env->spr[SPR_DAR] = cs->watchpoint_hit->hitaddr;
+                env->spr[SPR_DSISR] = PPC_BIT(41);
+                cs->watchpoint_hit = NULL;
+                raise_exception(env, POWERPC_EXCP_DSI);
+            }
+            cs->watchpoint_hit = NULL;
+        } else if (cpu_breakpoint_test(cs, env->nip, BP_CPU)) {
+            raise_exception_err(env, POWERPC_EXCP_TRACE,
+                                PPC_BIT(33) | PPC_BIT(43));
+        }
+    }
+#endif
+}
+
+bool ppc_cpu_debug_check_breakpoint(CPUState *cs)
+{
+#if defined(TARGET_PPC64)
+    CPUPPCState *env = cpu_env(cs);
+
+    if (env->insns_flags2 & PPC2_ISA207S) {
+        target_ulong priv;
+
+        priv = env->spr[SPR_CIABR] & PPC_BITMASK(62, 63);
+        switch (priv) {
+        case 0x1: /* problem */
+            return env->msr & ((target_ulong)1 << MSR_PR);
+        case 0x2: /* supervisor */
+            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
+                    !(env->msr & ((target_ulong)1 << MSR_HV)));
+        case 0x3: /* hypervisor */
+            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
+                     (env->msr & ((target_ulong)1 << MSR_HV)));
+        default:
+            g_assert_not_reached();
+        }
+    }
+#endif
+
+    return false;
+}
+
+bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
+{
+#if defined(TARGET_PPC64)
+    CPUPPCState *env = cpu_env(cs);
+
+    if (env->insns_flags2 & PPC2_ISA207S) {
+        if (wp == env->dawr0_watchpoint) {
+            uint32_t dawrx = env->spr[SPR_DAWRX0];
+            bool wt = extract32(dawrx, PPC_BIT_NR(59), 1);
+            bool wti = extract32(dawrx, PPC_BIT_NR(60), 1);
+            bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
+            bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
+            bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
+
+            if ((env->msr & ((target_ulong)1 << MSR_PR)) && !pr) {
+                return false;
+            } else if ((env->msr & ((target_ulong)1 << MSR_HV)) && !hv) {
+                return false;
+            } else if (!sv) {
+                return false;
+            }
+
+            if (!wti) {
+                if (env->msr & ((target_ulong)1 << MSR_DR)) {
+                    if (!wt) {
+                        return false;
+                    }
+                } else {
+                    if (wt) {
+                        return false;
+                    }
+                }
+            }
+
+            return true;
+        }
+    }
+#endif
+
+    return false;
+}
+
+#endif /* !CONFIG_USER_ONLY */
diff --git a/target/ppc/meson.build b/target/ppc/meson.build
index db3b7a0c33b..8eed1fa40ca 100644
--- a/target/ppc/meson.build
+++ b/target/ppc/meson.build
@@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
   'int_helper.c',
   'mem_helper.c',
   'misc_helper.c',
+  'tcg-excp_helper.c',
   'timebase_helper.c',
   'translate.c',
   'power8-pmu.c',
-- 
2.47.1

Re: [PATCH v2 04/15] target/ppc: Move TCG specific exception handlers to tcg-excp_helper.c

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Move the TCGCPUOps handlers to a new unit: tcg-excp_helper.c,
> only built when TCG is selected.
> 

Nice.
Just a thought - will the filename look better as excp_helper-tcg.c ?
That naming usually help developers when using tab completion.

> See in target/ppc/cpu_init.c:
> 
>      #ifdef CONFIG_TCG
>      static const TCGCPUOps ppc_tcg_ops = {
>        ...
>        .do_unaligned_access = ppc_cpu_do_unaligned_access,
>        .do_transaction_failed = ppc_cpu_do_transaction_failed,
>        .debug_excp_handler = ppc_cpu_debug_excp_handler,
>        .debug_check_breakpoint = ppc_cpu_debug_check_breakpoint,
>        .debug_check_watchpoint = ppc_cpu_debug_check_watchpoint,
>      };
>      #endif /* CONFIG_TCG */
> 

Thanks for capturing this in commit log.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   target/ppc/excp_helper.c     | 173 ------------------------------
>   target/ppc/tcg-excp_helper.c | 202 +++++++++++++++++++++++++++++++++++
>   target/ppc/meson.build       |   1 +
>   3 files changed, 203 insertions(+), 173 deletions(-)
>   create mode 100644 target/ppc/tcg-excp_helper.c
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 7ed4bbec035..b05eb7f5aec 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -3144,178 +3144,5 @@ void helper_book3s_trace(CPUPPCState *env, target_ulong prev_ip)
>       raise_exception_err(env, POWERPC_EXCP_TRACE, error_code);
>   }
>   
> -void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> -                                 MMUAccessType access_type,
> -                                 int mmu_idx, uintptr_t retaddr)
> -{
> -    CPUPPCState *env = cpu_env(cs);
> -    uint32_t insn;
> -
> -    /* Restore state and reload the insn we executed, for filling in DSISR.  */
> -    cpu_restore_state(cs, retaddr);
> -    insn = ppc_ldl_code(env, env->nip);
> -
> -    switch (env->mmu_model) {
> -    case POWERPC_MMU_SOFT_4xx:
> -        env->spr[SPR_40x_DEAR] = vaddr;
> -        break;
> -    case POWERPC_MMU_BOOKE:
> -    case POWERPC_MMU_BOOKE206:
> -        env->spr[SPR_BOOKE_DEAR] = vaddr;
> -        break;
> -    default:
> -        env->spr[SPR_DAR] = vaddr;
> -        break;
> -    }
> -
> -    cs->exception_index = POWERPC_EXCP_ALIGN;
> -    env->error_code = insn & 0x03FF0000;
> -    cpu_loop_exit(cs);
> -}
> -
> -void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> -                                   vaddr vaddr, unsigned size,
> -                                   MMUAccessType access_type,
> -                                   int mmu_idx, MemTxAttrs attrs,
> -                                   MemTxResult response, uintptr_t retaddr)
> -{
> -    CPUPPCState *env = cpu_env(cs);
> -
> -    switch (env->excp_model) {
> -#if defined(TARGET_PPC64)
> -    case POWERPC_EXCP_POWER8:
> -    case POWERPC_EXCP_POWER9:
> -    case POWERPC_EXCP_POWER10:
> -    case POWERPC_EXCP_POWER11:
> -        /*
> -         * Machine check codes can be found in processor User Manual or
> -         * Linux or skiboot source.
> -         */
> -        if (access_type == MMU_DATA_LOAD) {
> -            env->spr[SPR_DAR] = vaddr;
> -            env->spr[SPR_DSISR] = PPC_BIT(57);
> -            env->error_code = PPC_BIT(42);
> -
> -        } else if (access_type == MMU_DATA_STORE) {
> -            /*
> -             * MCE for stores in POWER is asynchronous so hardware does
> -             * not set DAR, but QEMU can do better.
> -             */
> -            env->spr[SPR_DAR] = vaddr;
> -            env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45);
> -            env->error_code |= PPC_BIT(42);
> -
> -        } else { /* Fetch */
> -            /*
> -             * is_prefix_insn_excp() tests !PPC_BIT(42) to avoid fetching
> -             * the instruction, so that must always be clear for fetches.
> -             */
> -            env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45);
> -        }
> -        break;
> -#endif
> -    default:
> -        /*
> -         * TODO: Check behaviour for other CPUs, for now do nothing.
> -         * Could add a basic MCE even if real hardware ignores.
> -         */
> -        return;
> -    }
> -
> -    cs->exception_index = POWERPC_EXCP_MCHECK;
> -    cpu_loop_exit_restore(cs, retaddr);
> -}
> -
> -void ppc_cpu_debug_excp_handler(CPUState *cs)
> -{
> -#if defined(TARGET_PPC64)
> -    CPUPPCState *env = cpu_env(cs);
> -
> -    if (env->insns_flags2 & PPC2_ISA207S) {
> -        if (cs->watchpoint_hit) {
> -            if (cs->watchpoint_hit->flags & BP_CPU) {
> -                env->spr[SPR_DAR] = cs->watchpoint_hit->hitaddr;
> -                env->spr[SPR_DSISR] = PPC_BIT(41);
> -                cs->watchpoint_hit = NULL;
> -                raise_exception(env, POWERPC_EXCP_DSI);
> -            }
> -            cs->watchpoint_hit = NULL;
> -        } else if (cpu_breakpoint_test(cs, env->nip, BP_CPU)) {
> -            raise_exception_err(env, POWERPC_EXCP_TRACE,
> -                                PPC_BIT(33) | PPC_BIT(43));
> -        }
> -    }
> -#endif
> -}
> -
> -bool ppc_cpu_debug_check_breakpoint(CPUState *cs)
> -{
> -#if defined(TARGET_PPC64)
> -    CPUPPCState *env = cpu_env(cs);
> -
> -    if (env->insns_flags2 & PPC2_ISA207S) {
> -        target_ulong priv;
> -
> -        priv = env->spr[SPR_CIABR] & PPC_BITMASK(62, 63);
> -        switch (priv) {
> -        case 0x1: /* problem */
> -            return env->msr & ((target_ulong)1 << MSR_PR);
> -        case 0x2: /* supervisor */
> -            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
> -                    !(env->msr & ((target_ulong)1 << MSR_HV)));
> -        case 0x3: /* hypervisor */
> -            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
> -                     (env->msr & ((target_ulong)1 << MSR_HV)));
> -        default:
> -            g_assert_not_reached();
> -        }
> -    }
> -#endif
> -
> -    return false;
> -}
> -
> -bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
> -{
> -#if defined(TARGET_PPC64)
> -    CPUPPCState *env = cpu_env(cs);
> -
> -    if (env->insns_flags2 & PPC2_ISA207S) {
> -        if (wp == env->dawr0_watchpoint) {
> -            uint32_t dawrx = env->spr[SPR_DAWRX0];
> -            bool wt = extract32(dawrx, PPC_BIT_NR(59), 1);
> -            bool wti = extract32(dawrx, PPC_BIT_NR(60), 1);
> -            bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
> -            bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
> -            bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
> -
> -            if ((env->msr & ((target_ulong)1 << MSR_PR)) && !pr) {
> -                return false;
> -            } else if ((env->msr & ((target_ulong)1 << MSR_HV)) && !hv) {
> -                return false;
> -            } else if (!sv) {
> -                return false;
> -            }
> -
> -            if (!wti) {
> -                if (env->msr & ((target_ulong)1 << MSR_DR)) {
> -                    if (!wt) {
> -                        return false;
> -                    }
> -                } else {
> -                    if (wt) {
> -                        return false;
> -                    }
> -                }
> -            }
> -
> -            return true;
> -        }
> -    }
> -#endif
> -
> -    return false;
> -}
> -
>   #endif /* !CONFIG_USER_ONLY */
>   #endif /* CONFIG_TCG */
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> new file mode 100644
> index 00000000000..3402dbe05ee
> --- /dev/null
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -0,0 +1,202 @@
> +/*
> + *  PowerPC exception emulation helpers for QEMU (TCG specific)
> + *
> + *  Copyright (c) 2003-2007 Jocelyn Mayer
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "qemu/osdep.h"
> +#include "exec/cpu_ldst.h"
> +
> +#include "hw/ppc/ppc.h"
> +#include "internal.h"
> +#include "cpu.h"
> +#include "trace.h"
> +
> +#ifndef CONFIG_USER_ONLY
> +
> +void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> +                                 MMUAccessType access_type,
> +                                 int mmu_idx, uintptr_t retaddr)
> +{
> +    CPUPPCState *env = cpu_env(cs);
> +    uint32_t insn;
> +
> +    /* Restore state and reload the insn we executed, for filling in DSISR.  */
> +    cpu_restore_state(cs, retaddr);
> +    insn = ppc_ldl_code(env, env->nip);
> +
> +    switch (env->mmu_model) {
> +    case POWERPC_MMU_SOFT_4xx:
> +        env->spr[SPR_40x_DEAR] = vaddr;
> +        break;
> +    case POWERPC_MMU_BOOKE:
> +    case POWERPC_MMU_BOOKE206:
> +        env->spr[SPR_BOOKE_DEAR] = vaddr;
> +        break;
> +    default:
> +        env->spr[SPR_DAR] = vaddr;
> +        break;
> +    }
> +
> +    cs->exception_index = POWERPC_EXCP_ALIGN;
> +    env->error_code = insn & 0x03FF0000;
> +    cpu_loop_exit(cs);
> +}
> +
> +void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> +                                   vaddr vaddr, unsigned size,
> +                                   MMUAccessType access_type,
> +                                   int mmu_idx, MemTxAttrs attrs,
> +                                   MemTxResult response, uintptr_t retaddr)
> +{
> +    CPUPPCState *env = cpu_env(cs);
> +
> +    switch (env->excp_model) {
> +#if defined(TARGET_PPC64)
> +    case POWERPC_EXCP_POWER8:
> +    case POWERPC_EXCP_POWER9:
> +    case POWERPC_EXCP_POWER10:
> +    case POWERPC_EXCP_POWER11:
> +        /*
> +         * Machine check codes can be found in processor User Manual or
> +         * Linux or skiboot source.
> +         */
> +        if (access_type == MMU_DATA_LOAD) {
> +            env->spr[SPR_DAR] = vaddr;
> +            env->spr[SPR_DSISR] = PPC_BIT(57);
> +            env->error_code = PPC_BIT(42);
> +
> +        } else if (access_type == MMU_DATA_STORE) {
> +            /*
> +             * MCE for stores in POWER is asynchronous so hardware does
> +             * not set DAR, but QEMU can do better.
> +             */
> +            env->spr[SPR_DAR] = vaddr;
> +            env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45);
> +            env->error_code |= PPC_BIT(42);
> +
> +        } else { /* Fetch */
> +            /*
> +             * is_prefix_insn_excp() tests !PPC_BIT(42) to avoid fetching
> +             * the instruction, so that must always be clear for fetches.
> +             */
> +            env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45);
> +        }
> +        break;
> +#endif
> +    default:
> +        /*
> +         * TODO: Check behaviour for other CPUs, for now do nothing.
> +         * Could add a basic MCE even if real hardware ignores.
> +         */
> +        return;
> +    }
> +
> +    cs->exception_index = POWERPC_EXCP_MCHECK;
> +    cpu_loop_exit_restore(cs, retaddr);
> +}
> +
> +void ppc_cpu_debug_excp_handler(CPUState *cs)
> +{
> +#if defined(TARGET_PPC64)
> +    CPUPPCState *env = cpu_env(cs);
> +
> +    if (env->insns_flags2 & PPC2_ISA207S) {
> +        if (cs->watchpoint_hit) {
> +            if (cs->watchpoint_hit->flags & BP_CPU) {
> +                env->spr[SPR_DAR] = cs->watchpoint_hit->hitaddr;
> +                env->spr[SPR_DSISR] = PPC_BIT(41);
> +                cs->watchpoint_hit = NULL;
> +                raise_exception(env, POWERPC_EXCP_DSI);
> +            }
> +            cs->watchpoint_hit = NULL;
> +        } else if (cpu_breakpoint_test(cs, env->nip, BP_CPU)) {
> +            raise_exception_err(env, POWERPC_EXCP_TRACE,
> +                                PPC_BIT(33) | PPC_BIT(43));
> +        }
> +    }
> +#endif
> +}
> +
> +bool ppc_cpu_debug_check_breakpoint(CPUState *cs)
> +{
> +#if defined(TARGET_PPC64)
> +    CPUPPCState *env = cpu_env(cs);
> +
> +    if (env->insns_flags2 & PPC2_ISA207S) {
> +        target_ulong priv;
> +
> +        priv = env->spr[SPR_CIABR] & PPC_BITMASK(62, 63);
> +        switch (priv) {
> +        case 0x1: /* problem */
> +            return env->msr & ((target_ulong)1 << MSR_PR);
> +        case 0x2: /* supervisor */
> +            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
> +                    !(env->msr & ((target_ulong)1 << MSR_HV)));
> +        case 0x3: /* hypervisor */
> +            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
> +                     (env->msr & ((target_ulong)1 << MSR_HV)));
> +        default:
> +            g_assert_not_reached();
> +        }
> +    }
> +#endif
> +
> +    return false;
> +}
> +
> +bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
> +{
> +#if defined(TARGET_PPC64)
> +    CPUPPCState *env = cpu_env(cs);
> +
> +    if (env->insns_flags2 & PPC2_ISA207S) {
> +        if (wp == env->dawr0_watchpoint) {
> +            uint32_t dawrx = env->spr[SPR_DAWRX0];
> +            bool wt = extract32(dawrx, PPC_BIT_NR(59), 1);
> +            bool wti = extract32(dawrx, PPC_BIT_NR(60), 1);
> +            bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
> +            bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
> +            bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
> +
> +            if ((env->msr & ((target_ulong)1 << MSR_PR)) && !pr) {
> +                return false;
> +            } else if ((env->msr & ((target_ulong)1 << MSR_HV)) && !hv) {
> +                return false;
> +            } else if (!sv) {
> +                return false;
> +            }
> +
> +            if (!wti) {
> +                if (env->msr & ((target_ulong)1 << MSR_DR)) {
> +                    if (!wt) {
> +                        return false;
> +                    }
> +                } else {
> +                    if (wt) {
> +                        return false;
> +                    }
> +                }
> +            }
> +
> +            return true;
> +        }
> +    }
> +#endif
> +
> +    return false;
> +}
> +
> +#endif /* !CONFIG_USER_ONLY */
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index db3b7a0c33b..8eed1fa40ca 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
>     'int_helper.c',
>     'mem_helper.c',
>     'misc_helper.c',
> +  'tcg-excp_helper.c',
>     'timebase_helper.c',
>     'translate.c',
>     'power8-pmu.c',

Re: [PATCH v2 04/15] target/ppc: Move TCG specific exception handlers to tcg-excp_helper.c

[BALATON Zoltan]

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

On Tue, 28 Jan 2025, Harsh Prateek Bora wrote:
> On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
>> Move the TCGCPUOps handlers to a new unit: tcg-excp_helper.c,
>> only built when TCG is selected.
>> 
>
> Nice.
> Just a thought - will the filename look better as excp_helper-tcg.c ?
> That naming usually help developers when using tab completion.

Or maybe stick to either _ or - in the filename and not mix both in one 
name? If you want to use -tcg or tcg- then also rename to excp-helper. We 
already have a mix of _ and - names but at least try to be consistent 
within one name.

Regards,
BALATON Zoltan

>> See in target/ppc/cpu_init.c:
>>
>>      #ifdef CONFIG_TCG
>>      static const TCGCPUOps ppc_tcg_ops = {
>>        ...
>>        .do_unaligned_access = ppc_cpu_do_unaligned_access,
>>        .do_transaction_failed = ppc_cpu_do_transaction_failed,
>>        .debug_excp_handler = ppc_cpu_debug_excp_handler,
>>        .debug_check_breakpoint = ppc_cpu_debug_check_breakpoint,
>>        .debug_check_watchpoint = ppc_cpu_debug_check_watchpoint,
>>      };
>>      #endif /* CONFIG_TCG */
>> 
>
> Thanks for capturing this in commit log.
>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
>> ---
>>   target/ppc/excp_helper.c     | 173 ------------------------------
>>   target/ppc/tcg-excp_helper.c | 202 +++++++++++++++++++++++++++++++++++
>>   target/ppc/meson.build       |   1 +
>>   3 files changed, 203 insertions(+), 173 deletions(-)
>>   create mode 100644 target/ppc/tcg-excp_helper.c
>> 
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 7ed4bbec035..b05eb7f5aec 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -3144,178 +3144,5 @@ void helper_book3s_trace(CPUPPCState *env, 
>> target_ulong prev_ip)
>>       raise_exception_err(env, POWERPC_EXCP_TRACE, error_code);
>>   }
>>   -void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>> -                                 MMUAccessType access_type,
>> -                                 int mmu_idx, uintptr_t retaddr)
>> -{
>> -    CPUPPCState *env = cpu_env(cs);
>> -    uint32_t insn;
>> -
>> -    /* Restore state and reload the insn we executed, for filling in 
>> DSISR.  */
>> -    cpu_restore_state(cs, retaddr);
>> -    insn = ppc_ldl_code(env, env->nip);
>> -
>> -    switch (env->mmu_model) {
>> -    case POWERPC_MMU_SOFT_4xx:
>> -        env->spr[SPR_40x_DEAR] = vaddr;
>> -        break;
>> -    case POWERPC_MMU_BOOKE:
>> -    case POWERPC_MMU_BOOKE206:
>> -        env->spr[SPR_BOOKE_DEAR] = vaddr;
>> -        break;
>> -    default:
>> -        env->spr[SPR_DAR] = vaddr;
>> -        break;
>> -    }
>> -
>> -    cs->exception_index = POWERPC_EXCP_ALIGN;
>> -    env->error_code = insn & 0x03FF0000;
>> -    cpu_loop_exit(cs);
>> -}
>> -
>> -void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>> -                                   vaddr vaddr, unsigned size,
>> -                                   MMUAccessType access_type,
>> -                                   int mmu_idx, MemTxAttrs attrs,
>> -                                   MemTxResult response, uintptr_t 
>> retaddr)
>> -{
>> -    CPUPPCState *env = cpu_env(cs);
>> -
>> -    switch (env->excp_model) {
>> -#if defined(TARGET_PPC64)
>> -    case POWERPC_EXCP_POWER8:
>> -    case POWERPC_EXCP_POWER9:
>> -    case POWERPC_EXCP_POWER10:
>> -    case POWERPC_EXCP_POWER11:
>> -        /*
>> -         * Machine check codes can be found in processor User Manual or
>> -         * Linux or skiboot source.
>> -         */
>> -        if (access_type == MMU_DATA_LOAD) {
>> -            env->spr[SPR_DAR] = vaddr;
>> -            env->spr[SPR_DSISR] = PPC_BIT(57);
>> -            env->error_code = PPC_BIT(42);
>> -
>> -        } else if (access_type == MMU_DATA_STORE) {
>> -            /*
>> -             * MCE for stores in POWER is asynchronous so hardware does
>> -             * not set DAR, but QEMU can do better.
>> -             */
>> -            env->spr[SPR_DAR] = vaddr;
>> -            env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45);
>> -            env->error_code |= PPC_BIT(42);
>> -
>> -        } else { /* Fetch */
>> -            /*
>> -             * is_prefix_insn_excp() tests !PPC_BIT(42) to avoid fetching
>> -             * the instruction, so that must always be clear for fetches.
>> -             */
>> -            env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45);
>> -        }
>> -        break;
>> -#endif
>> -    default:
>> -        /*
>> -         * TODO: Check behaviour for other CPUs, for now do nothing.
>> -         * Could add a basic MCE even if real hardware ignores.
>> -         */
>> -        return;
>> -    }
>> -
>> -    cs->exception_index = POWERPC_EXCP_MCHECK;
>> -    cpu_loop_exit_restore(cs, retaddr);
>> -}
>> -
>> -void ppc_cpu_debug_excp_handler(CPUState *cs)
>> -{
>> -#if defined(TARGET_PPC64)
>> -    CPUPPCState *env = cpu_env(cs);
>> -
>> -    if (env->insns_flags2 & PPC2_ISA207S) {
>> -        if (cs->watchpoint_hit) {
>> -            if (cs->watchpoint_hit->flags & BP_CPU) {
>> -                env->spr[SPR_DAR] = cs->watchpoint_hit->hitaddr;
>> -                env->spr[SPR_DSISR] = PPC_BIT(41);
>> -                cs->watchpoint_hit = NULL;
>> -                raise_exception(env, POWERPC_EXCP_DSI);
>> -            }
>> -            cs->watchpoint_hit = NULL;
>> -        } else if (cpu_breakpoint_test(cs, env->nip, BP_CPU)) {
>> -            raise_exception_err(env, POWERPC_EXCP_TRACE,
>> -                                PPC_BIT(33) | PPC_BIT(43));
>> -        }
>> -    }
>> -#endif
>> -}
>> -
>> -bool ppc_cpu_debug_check_breakpoint(CPUState *cs)
>> -{
>> -#if defined(TARGET_PPC64)
>> -    CPUPPCState *env = cpu_env(cs);
>> -
>> -    if (env->insns_flags2 & PPC2_ISA207S) {
>> -        target_ulong priv;
>> -
>> -        priv = env->spr[SPR_CIABR] & PPC_BITMASK(62, 63);
>> -        switch (priv) {
>> -        case 0x1: /* problem */
>> -            return env->msr & ((target_ulong)1 << MSR_PR);
>> -        case 0x2: /* supervisor */
>> -            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
>> -                    !(env->msr & ((target_ulong)1 << MSR_HV)));
>> -        case 0x3: /* hypervisor */
>> -            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
>> -                     (env->msr & ((target_ulong)1 << MSR_HV)));
>> -        default:
>> -            g_assert_not_reached();
>> -        }
>> -    }
>> -#endif
>> -
>> -    return false;
>> -}
>> -
>> -bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>> -{
>> -#if defined(TARGET_PPC64)
>> -    CPUPPCState *env = cpu_env(cs);
>> -
>> -    if (env->insns_flags2 & PPC2_ISA207S) {
>> -        if (wp == env->dawr0_watchpoint) {
>> -            uint32_t dawrx = env->spr[SPR_DAWRX0];
>> -            bool wt = extract32(dawrx, PPC_BIT_NR(59), 1);
>> -            bool wti = extract32(dawrx, PPC_BIT_NR(60), 1);
>> -            bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
>> -            bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
>> -            bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
>> -
>> -            if ((env->msr & ((target_ulong)1 << MSR_PR)) && !pr) {
>> -                return false;
>> -            } else if ((env->msr & ((target_ulong)1 << MSR_HV)) && !hv) {
>> -                return false;
>> -            } else if (!sv) {
>> -                return false;
>> -            }
>> -
>> -            if (!wti) {
>> -                if (env->msr & ((target_ulong)1 << MSR_DR)) {
>> -                    if (!wt) {
>> -                        return false;
>> -                    }
>> -                } else {
>> -                    if (wt) {
>> -                        return false;
>> -                    }
>> -                }
>> -            }
>> -
>> -            return true;
>> -        }
>> -    }
>> -#endif
>> -
>> -    return false;
>> -}
>> -
>>   #endif /* !CONFIG_USER_ONLY */
>>   #endif /* CONFIG_TCG */
>> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
>> new file mode 100644
>> index 00000000000..3402dbe05ee
>> --- /dev/null
>> +++ b/target/ppc/tcg-excp_helper.c
>> @@ -0,0 +1,202 @@
>> +/*
>> + *  PowerPC exception emulation helpers for QEMU (TCG specific)
>> + *
>> + *  Copyright (c) 2003-2007 Jocelyn Mayer
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see 
>> <http://www.gnu.org/licenses/>.
>> + */
>> +#include "qemu/osdep.h"
>> +#include "exec/cpu_ldst.h"
>> +
>> +#include "hw/ppc/ppc.h"
>> +#include "internal.h"
>> +#include "cpu.h"
>> +#include "trace.h"
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +
>> +void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>> +                                 MMUAccessType access_type,
>> +                                 int mmu_idx, uintptr_t retaddr)
>> +{
>> +    CPUPPCState *env = cpu_env(cs);
>> +    uint32_t insn;
>> +
>> +    /* Restore state and reload the insn we executed, for filling in 
>> DSISR.  */
>> +    cpu_restore_state(cs, retaddr);
>> +    insn = ppc_ldl_code(env, env->nip);
>> +
>> +    switch (env->mmu_model) {
>> +    case POWERPC_MMU_SOFT_4xx:
>> +        env->spr[SPR_40x_DEAR] = vaddr;
>> +        break;
>> +    case POWERPC_MMU_BOOKE:
>> +    case POWERPC_MMU_BOOKE206:
>> +        env->spr[SPR_BOOKE_DEAR] = vaddr;
>> +        break;
>> +    default:
>> +        env->spr[SPR_DAR] = vaddr;
>> +        break;
>> +    }
>> +
>> +    cs->exception_index = POWERPC_EXCP_ALIGN;
>> +    env->error_code = insn & 0x03FF0000;
>> +    cpu_loop_exit(cs);
>> +}
>> +
>> +void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>> +                                   vaddr vaddr, unsigned size,
>> +                                   MMUAccessType access_type,
>> +                                   int mmu_idx, MemTxAttrs attrs,
>> +                                   MemTxResult response, uintptr_t 
>> retaddr)
>> +{
>> +    CPUPPCState *env = cpu_env(cs);
>> +
>> +    switch (env->excp_model) {
>> +#if defined(TARGET_PPC64)
>> +    case POWERPC_EXCP_POWER8:
>> +    case POWERPC_EXCP_POWER9:
>> +    case POWERPC_EXCP_POWER10:
>> +    case POWERPC_EXCP_POWER11:
>> +        /*
>> +         * Machine check codes can be found in processor User Manual or
>> +         * Linux or skiboot source.
>> +         */
>> +        if (access_type == MMU_DATA_LOAD) {
>> +            env->spr[SPR_DAR] = vaddr;
>> +            env->spr[SPR_DSISR] = PPC_BIT(57);
>> +            env->error_code = PPC_BIT(42);
>> +
>> +        } else if (access_type == MMU_DATA_STORE) {
>> +            /*
>> +             * MCE for stores in POWER is asynchronous so hardware does
>> +             * not set DAR, but QEMU can do better.
>> +             */
>> +            env->spr[SPR_DAR] = vaddr;
>> +            env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45);
>> +            env->error_code |= PPC_BIT(42);
>> +
>> +        } else { /* Fetch */
>> +            /*
>> +             * is_prefix_insn_excp() tests !PPC_BIT(42) to avoid fetching
>> +             * the instruction, so that must always be clear for fetches.
>> +             */
>> +            env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45);
>> +        }
>> +        break;
>> +#endif
>> +    default:
>> +        /*
>> +         * TODO: Check behaviour for other CPUs, for now do nothing.
>> +         * Could add a basic MCE even if real hardware ignores.
>> +         */
>> +        return;
>> +    }
>> +
>> +    cs->exception_index = POWERPC_EXCP_MCHECK;
>> +    cpu_loop_exit_restore(cs, retaddr);
>> +}
>> +
>> +void ppc_cpu_debug_excp_handler(CPUState *cs)
>> +{
>> +#if defined(TARGET_PPC64)
>> +    CPUPPCState *env = cpu_env(cs);
>> +
>> +    if (env->insns_flags2 & PPC2_ISA207S) {
>> +        if (cs->watchpoint_hit) {
>> +            if (cs->watchpoint_hit->flags & BP_CPU) {
>> +                env->spr[SPR_DAR] = cs->watchpoint_hit->hitaddr;
>> +                env->spr[SPR_DSISR] = PPC_BIT(41);
>> +                cs->watchpoint_hit = NULL;
>> +                raise_exception(env, POWERPC_EXCP_DSI);
>> +            }
>> +            cs->watchpoint_hit = NULL;
>> +        } else if (cpu_breakpoint_test(cs, env->nip, BP_CPU)) {
>> +            raise_exception_err(env, POWERPC_EXCP_TRACE,
>> +                                PPC_BIT(33) | PPC_BIT(43));
>> +        }
>> +    }
>> +#endif
>> +}
>> +
>> +bool ppc_cpu_debug_check_breakpoint(CPUState *cs)
>> +{
>> +#if defined(TARGET_PPC64)
>> +    CPUPPCState *env = cpu_env(cs);
>> +
>> +    if (env->insns_flags2 & PPC2_ISA207S) {
>> +        target_ulong priv;
>> +
>> +        priv = env->spr[SPR_CIABR] & PPC_BITMASK(62, 63);
>> +        switch (priv) {
>> +        case 0x1: /* problem */
>> +            return env->msr & ((target_ulong)1 << MSR_PR);
>> +        case 0x2: /* supervisor */
>> +            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
>> +                    !(env->msr & ((target_ulong)1 << MSR_HV)));
>> +        case 0x3: /* hypervisor */
>> +            return (!(env->msr & ((target_ulong)1 << MSR_PR)) &&
>> +                     (env->msr & ((target_ulong)1 << MSR_HV)));
>> +        default:
>> +            g_assert_not_reached();
>> +        }
>> +    }
>> +#endif
>> +
>> +    return false;
>> +}
>> +
>> +bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>> +{
>> +#if defined(TARGET_PPC64)
>> +    CPUPPCState *env = cpu_env(cs);
>> +
>> +    if (env->insns_flags2 & PPC2_ISA207S) {
>> +        if (wp == env->dawr0_watchpoint) {
>> +            uint32_t dawrx = env->spr[SPR_DAWRX0];
>> +            bool wt = extract32(dawrx, PPC_BIT_NR(59), 1);
>> +            bool wti = extract32(dawrx, PPC_BIT_NR(60), 1);
>> +            bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
>> +            bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
>> +            bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
>> +
>> +            if ((env->msr & ((target_ulong)1 << MSR_PR)) && !pr) {
>> +                return false;
>> +            } else if ((env->msr & ((target_ulong)1 << MSR_HV)) && !hv) {
>> +                return false;
>> +            } else if (!sv) {
>> +                return false;
>> +            }
>> +
>> +            if (!wti) {
>> +                if (env->msr & ((target_ulong)1 << MSR_DR)) {
>> +                    if (!wt) {
>> +                        return false;
>> +                    }
>> +                } else {
>> +                    if (wt) {
>> +                        return false;
>> +                    }
>> +                }
>> +            }
>> +
>> +            return true;
>> +        }
>> +    }
>> +#endif
>> +
>> +    return false;
>> +}
>> +
>> +#endif /* !CONFIG_USER_ONLY */
>> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
>> index db3b7a0c33b..8eed1fa40ca 100644
>> --- a/target/ppc/meson.build
>> +++ b/target/ppc/meson.build
>> @@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
>>     'int_helper.c',
>>     'mem_helper.c',
>>     'misc_helper.c',
>> +  'tcg-excp_helper.c',
>>     'timebase_helper.c',
>>     'translate.c',
>>     'power8-pmu.c',
>
>

Re: [PATCH v2 04/15] target/ppc: Move TCG specific exception handlers to tcg-excp_helper.c

[Philippe Mathieu-Daudé]

On 28/1/25 13:41, BALATON Zoltan wrote:
> On Tue, 28 Jan 2025, Harsh Prateek Bora wrote:
>> On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
>>> Move the TCGCPUOps handlers to a new unit: tcg-excp_helper.c,
>>> only built when TCG is selected.
>>>
>>
>> Nice.
>> Just a thought - will the filename look better as excp_helper-tcg.c ?
>> That naming usually help developers when using tab completion.
> 
> Or maybe stick to either _ or - in the filename and not mix both in one 
> name? If you want to use -tcg or tcg- then also rename to excp-helper. 
> We already have a mix of _ and - names but at least try to be consistent 
> within one name.

Other targets use a sub-directory:

$ ls -1d target/*/tcg
target/arm/tcg
target/i386/tcg
target/loongarch/tcg
target/mips/tcg
target/riscv/tcg
target/s390x/tcg

I'm not interested in particular with this target and ready to work a
lot on it, this is why I went with a surgical approach.

> 
> Regards,
> BALATON Zoltan

[PATCH v2 05/15] target/ppc: Move ppc_ldl_code() to tcg-excp_helper.c

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/excp_helper.c     | 21 ---------------------
 target/ppc/tcg-excp_helper.c | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index b05eb7f5aec..8956466db1d 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -136,27 +136,6 @@ static void dump_hcall(CPUPPCState *env)
                   env->nip);
 }
 
-#ifdef CONFIG_TCG
-/* Return true iff byteswap is needed to load instruction */
-static inline bool insn_need_byteswap(CPUArchState *env)
-{
-    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
-    return !!(env->msr & ((target_ulong)1 << MSR_LE));
-}
-
-uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
-{
-    uint32_t insn = cpu_ldl_code(env, addr);
-
-    if (insn_need_byteswap(env)) {
-        insn = bswap32(insn);
-    }
-
-    return insn;
-}
-
-#endif
-
 static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
 {
     const char *es;
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
index 3402dbe05ee..6950b78774d 100644
--- a/target/ppc/tcg-excp_helper.c
+++ b/target/ppc/tcg-excp_helper.c
@@ -199,4 +199,22 @@ bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
     return false;
 }
 
+/* Return true iff byteswap is needed to load instruction */
+static inline bool insn_need_byteswap(CPUArchState *env)
+{
+    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
+    return !!(env->msr & ((target_ulong)1 << MSR_LE));
+}
+
+uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
+{
+    uint32_t insn = cpu_ldl_code(env, addr);
+
+    if (insn_need_byteswap(env)) {
+        insn = bswap32(insn);
+    }
+
+    return insn;
+}
+
 #endif /* !CONFIG_USER_ONLY */
-- 
2.47.1

Re: [PATCH v2 05/15] target/ppc: Move ppc_ldl_code() to tcg-excp_helper.c

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/excp_helper.c     | 21 ---------------------
>   target/ppc/tcg-excp_helper.c | 18 ++++++++++++++++++
>   2 files changed, 18 insertions(+), 21 deletions(-)
> 

This patch also needs to remove the function declaration introduced in 
internal.h in patch 3.

Otherwise,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index b05eb7f5aec..8956466db1d 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -136,27 +136,6 @@ static void dump_hcall(CPUPPCState *env)
>                     env->nip);
>   }
>   
> -#ifdef CONFIG_TCG
> -/* Return true iff byteswap is needed to load instruction */
> -static inline bool insn_need_byteswap(CPUArchState *env)
> -{
> -    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
> -    return !!(env->msr & ((target_ulong)1 << MSR_LE));
> -}
> -
> -uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
> -{
> -    uint32_t insn = cpu_ldl_code(env, addr);
> -
> -    if (insn_need_byteswap(env)) {
> -        insn = bswap32(insn);
> -    }
> -
> -    return insn;
> -}
> -
> -#endif
> -
>   static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
>   {
>       const char *es;
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index 3402dbe05ee..6950b78774d 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -199,4 +199,22 @@ bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>       return false;
>   }
>   
> +/* Return true iff byteswap is needed to load instruction */
> +static inline bool insn_need_byteswap(CPUArchState *env)
> +{
> +    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
> +    return !!(env->msr & ((target_ulong)1 << MSR_LE));
> +}
> +
> +uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
> +{
> +    uint32_t insn = cpu_ldl_code(env, addr);
> +
> +    if (insn_need_byteswap(env)) {
> +        insn = bswap32(insn);
> +    }
> +
> +    return insn;
> +}
> +
>   #endif /* !CONFIG_USER_ONLY */

Re: [PATCH v2 05/15] target/ppc: Move ppc_ldl_code() to tcg-excp_helper.c

[Philippe Mathieu-Daudé]

On 28/1/25 07:13, Harsh Prateek Bora wrote:
> 
> 
> On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/ppc/excp_helper.c     | 21 ---------------------
>>   target/ppc/tcg-excp_helper.c | 18 ++++++++++++++++++
>>   2 files changed, 18 insertions(+), 21 deletions(-)
>>
> 
> This patch also needs to remove the function declaration introduced in 
> internal.h in patch 3.

No, it is still used in 2 distinct units:

$ git grep ppc_ldl_code a09cb1ead
a09cb1ead:target/ppc/excp_helper.c:1291:    return is_prefix_insn(env, 
ppc_ldl_code(env, env->nip));
a09cb1ead:target/ppc/excp_helper.c:1516:        uint32_t insn = 
ppc_ldl_code(env, env->nip);
a09cb1ead:target/ppc/excp_helper.c:1519:            uint32_t insn2 = 
ppc_ldl_code(env, env->nip + 4);
a09cb1ead:target/ppc/internal.h:271:uint32_t ppc_ldl_code(CPUArchState 
*env, target_ulong addr);
a09cb1ead:target/ppc/tcg-excp_helper.c:38:    insn = ppc_ldl_code(env, 
env->nip);
a09cb1ead:target/ppc/tcg-excp_helper.c:209:uint32_t 
ppc_ldl_code(CPUArchState *env, target_ulong addr)

> Otherwise,
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

Thanks!

[PATCH v2 06/15] target/ppc: Ensure powerpc_checkstop() is only called under TCG

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/excp_helper.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 8956466db1d..b08cd53688c 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include "qemu/main-loop.h"
 #include "qemu/log.h"
+#include "system/tcg.h"
 #include "system/system.h"
 #include "system/runstate.h"
 #include "cpu.h"
@@ -30,7 +31,6 @@
 #include "trace.h"
 
 #ifdef CONFIG_TCG
-#include "system/tcg.h"
 #include "exec/helper-proto.h"
 #include "exec/cpu_ldst.h"
 #endif
@@ -443,13 +443,11 @@ void helper_attn(CPUPPCState *env)
 static void powerpc_mcheck_checkstop(CPUPPCState *env)
 {
     /* KVM guests always have MSR[ME] enabled */
-#ifdef CONFIG_TCG
     if (FIELD_EX64(env->msr, MSR, ME)) {
         return;
     }
-
+    assert(tcg_enabled());
     powerpc_checkstop(env, "machine check with MSR[ME]=0");
-#endif
 }
 
 static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
-- 
2.47.1

Re: [PATCH v2 06/15] target/ppc: Ensure powerpc_checkstop() is only called under TCG

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/excp_helper.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 8956466db1d..b08cd53688c 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -19,6 +19,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/main-loop.h"
>   #include "qemu/log.h"
> +#include "system/tcg.h"
>   #include "system/system.h"
>   #include "system/runstate.h"
>   #include "cpu.h"
> @@ -30,7 +31,6 @@
>   #include "trace.h"
>   
>   #ifdef CONFIG_TCG
> -#include "system/tcg.h"
>   #include "exec/helper-proto.h"
>   #include "exec/cpu_ldst.h"
>   #endif
> @@ -443,13 +443,11 @@ void helper_attn(CPUPPCState *env)
>   static void powerpc_mcheck_checkstop(CPUPPCState *env)
>   {
>       /* KVM guests always have MSR[ME] enabled */
> -#ifdef CONFIG_TCG
>       if (FIELD_EX64(env->msr, MSR, ME)) {
>           return;
>       }
> -
> +    assert(tcg_enabled());

Shouldn't this be a no-op if not TCG ?

Nick, please advise ?

regards,
Harsh
>       powerpc_checkstop(env, "machine check with MSR[ME]=0");
> -#endif
>   }
>   
>   static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)

Re: [PATCH v2 06/15] target/ppc: Ensure powerpc_checkstop() is only called under TCG

[Harsh Prateek Bora]

On 1/28/25 12:13, Harsh Prateek Bora wrote:
> 
> 
> On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/ppc/excp_helper.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 8956466db1d..b08cd53688c 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -19,6 +19,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/main-loop.h"
>>   #include "qemu/log.h"
>> +#include "system/tcg.h"
>>   #include "system/system.h"
>>   #include "system/runstate.h"
>>   #include "cpu.h"
>> @@ -30,7 +31,6 @@
>>   #include "trace.h"
>>   #ifdef CONFIG_TCG
>> -#include "system/tcg.h"
>>   #include "exec/helper-proto.h"
>>   #include "exec/cpu_ldst.h"
>>   #endif
>> @@ -443,13 +443,11 @@ void helper_attn(CPUPPCState *env)
>>   static void powerpc_mcheck_checkstop(CPUPPCState *env)
>>   {
>>       /* KVM guests always have MSR[ME] enabled */
>> -#ifdef CONFIG_TCG
>>       if (FIELD_EX64(env->msr, MSR, ME)) {
>>           return;
>>       }
>> -
>> +    assert(tcg_enabled());
> 
> Shouldn't this be a no-op if not TCG ?
> 
> Nick, please advise ?
> 

Also, patch title needs update - it's powerpc_mcheck_checkstop.

> regards,
> Harsh
>>       powerpc_checkstop(env, "machine check with MSR[ME]=0");
>> -#endif
>>   }
>>   static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
> 

Re: [PATCH v2 06/15] target/ppc: Ensure powerpc_checkstop() is only called under TCG

[Nicholas Piggin]

On Tue Jan 28, 2025 at 4:43 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/ppc/excp_helper.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 8956466db1d..b08cd53688c 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -19,6 +19,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/main-loop.h"
>>   #include "qemu/log.h"
>> +#include "system/tcg.h"
>>   #include "system/system.h"
>>   #include "system/runstate.h"
>>   #include "cpu.h"
>> @@ -30,7 +31,6 @@
>>   #include "trace.h"
>>   
>>   #ifdef CONFIG_TCG
>> -#include "system/tcg.h"
>>   #include "exec/helper-proto.h"
>>   #include "exec/cpu_ldst.h"
>>   #endif
>> @@ -443,13 +443,11 @@ void helper_attn(CPUPPCState *env)
>>   static void powerpc_mcheck_checkstop(CPUPPCState *env)
>>   {
>>       /* KVM guests always have MSR[ME] enabled */
>> -#ifdef CONFIG_TCG
>>       if (FIELD_EX64(env->msr, MSR, ME)) {
>>           return;
>>       }
>> -
>> +    assert(tcg_enabled());
>
> Shouldn't this be a no-op if not TCG ?
>
> Nick, please advise ?

If KVM, I think we would rather catch that it got called instead of
no-op.

At this point the guest is crashed, so it's not overly rude to
assert. I'm okay with this.

Thanks,
Nick

[PATCH v2 07/15] target/ppc: Restrict powerpc_checkstop() to TCG

[Philippe Mathieu-Daudé]

Expose powerpc_checkstop() prototype, and move it to
tcg-excp_helper.c, only built when TCG is available.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/internal.h        |  4 +++-
 target/ppc/excp_helper.c     | 26 --------------------------
 target/ppc/tcg-excp_helper.c | 28 ++++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 46db6adfcf6..62186bc1e61 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -289,7 +289,9 @@ void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
 void ppc_cpu_debug_excp_handler(CPUState *cs);
 bool ppc_cpu_debug_check_breakpoint(CPUState *cs);
 bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
-#endif
+
+G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason);
+#endif /* !CONFIG_USER_ONLY */
 
 FIELD(GER_MSK, XMSK, 0, 4)
 FIELD(GER_MSK, YMSK, 4, 4)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index b08cd53688c..236e5078f56 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -400,32 +400,6 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
 }
 
 #ifdef CONFIG_TCG
-/*
- * This stops the machine and logs CPU state without killing QEMU (like
- * cpu_abort()) because it is often a guest error as opposed to a QEMU error,
- * so the machine can still be debugged.
- */
-static G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason)
-{
-    CPUState *cs = env_cpu(env);
-    FILE *f;
-
-    f = qemu_log_trylock();
-    if (f) {
-        fprintf(f, "Entering checkstop state: %s\n", reason);
-        cpu_dump_state(cs, f, CPU_DUMP_FPU | CPU_DUMP_CCOP);
-        qemu_log_unlock(f);
-    }
-
-    /*
-     * This stops the machine and logs CPU state without killing QEMU
-     * (like cpu_abort()) so the machine can still be debugged (because
-     * it is often a guest error).
-     */
-    qemu_system_guest_panicked(NULL);
-    cpu_loop_exit_noexc(cs);
-}
-
 #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
 void helper_attn(CPUPPCState *env)
 {
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
index 6950b78774d..93c2d6b5a03 100644
--- a/target/ppc/tcg-excp_helper.c
+++ b/target/ppc/tcg-excp_helper.c
@@ -17,7 +17,9 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "exec/cpu_ldst.h"
+#include "system/runstate.h"
 
 #include "hw/ppc/ppc.h"
 #include "internal.h"
@@ -199,6 +201,32 @@ bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
     return false;
 }
 
+/*
+ * This stops the machine and logs CPU state without killing QEMU (like
+ * cpu_abort()) because it is often a guest error as opposed to a QEMU error,
+ * so the machine can still be debugged.
+ */
+G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason)
+{
+    CPUState *cs = env_cpu(env);
+    FILE *f;
+
+    f = qemu_log_trylock();
+    if (f) {
+        fprintf(f, "Entering checkstop state: %s\n", reason);
+        cpu_dump_state(cs, f, CPU_DUMP_FPU | CPU_DUMP_CCOP);
+        qemu_log_unlock(f);
+    }
+
+    /*
+     * This stops the machine and logs CPU state without killing QEMU
+     * (like cpu_abort()) so the machine can still be debugged (because
+     * it is often a guest error).
+     */
+    qemu_system_guest_panicked(NULL);
+    cpu_loop_exit_noexc(cs);
+}
+
 /* Return true iff byteswap is needed to load instruction */
 static inline bool insn_need_byteswap(CPUArchState *env)
 {
-- 
2.47.1

Re: [PATCH v2 07/15] target/ppc: Restrict powerpc_checkstop() to TCG

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Expose powerpc_checkstop() prototype, and move it to
> tcg-excp_helper.c, only built when TCG is available.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   target/ppc/internal.h        |  4 +++-
>   target/ppc/excp_helper.c     | 26 --------------------------
>   target/ppc/tcg-excp_helper.c | 28 ++++++++++++++++++++++++++++
>   3 files changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index 46db6adfcf6..62186bc1e61 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -289,7 +289,9 @@ void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>   void ppc_cpu_debug_excp_handler(CPUState *cs);
>   bool ppc_cpu_debug_check_breakpoint(CPUState *cs);
>   bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
> -#endif
> +
> +G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason);
> +#endif /* !CONFIG_USER_ONLY */
>   
>   FIELD(GER_MSK, XMSK, 0, 4)
>   FIELD(GER_MSK, YMSK, 4, 4)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index b08cd53688c..236e5078f56 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -400,32 +400,6 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
>   }
>   
>   #ifdef CONFIG_TCG
> -/*
> - * This stops the machine and logs CPU state without killing QEMU (like
> - * cpu_abort()) because it is often a guest error as opposed to a QEMU error,
> - * so the machine can still be debugged.
> - */
> -static G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason)
> -{
> -    CPUState *cs = env_cpu(env);
> -    FILE *f;
> -
> -    f = qemu_log_trylock();
> -    if (f) {
> -        fprintf(f, "Entering checkstop state: %s\n", reason);
> -        cpu_dump_state(cs, f, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> -        qemu_log_unlock(f);
> -    }
> -
> -    /*
> -     * This stops the machine and logs CPU state without killing QEMU
> -     * (like cpu_abort()) so the machine can still be debugged (because
> -     * it is often a guest error).
> -     */
> -    qemu_system_guest_panicked(NULL);
> -    cpu_loop_exit_noexc(cs);
> -}
> -
>   #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>   void helper_attn(CPUPPCState *env)
>   {
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index 6950b78774d..93c2d6b5a03 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -17,7 +17,9 @@
>    * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>    */
>   #include "qemu/osdep.h"
> +#include "qemu/log.h"
>   #include "exec/cpu_ldst.h"
> +#include "system/runstate.h"
>   
>   #include "hw/ppc/ppc.h"
>   #include "internal.h"
> @@ -199,6 +201,32 @@ bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>       return false;
>   }
>   
> +/*
> + * This stops the machine and logs CPU state without killing QEMU (like
> + * cpu_abort()) because it is often a guest error as opposed to a QEMU error,
> + * so the machine can still be debugged.
> + */
> +G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason)
> +{
> +    CPUState *cs = env_cpu(env);
> +    FILE *f;
> +
> +    f = qemu_log_trylock();
> +    if (f) {
> +        fprintf(f, "Entering checkstop state: %s\n", reason);
> +        cpu_dump_state(cs, f, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> +        qemu_log_unlock(f);
> +    }
> +
> +    /*
> +     * This stops the machine and logs CPU state without killing QEMU
> +     * (like cpu_abort()) so the machine can still be debugged (because
> +     * it is often a guest error).
> +     */
> +    qemu_system_guest_panicked(NULL);
> +    cpu_loop_exit_noexc(cs);
> +}
> +
>   /* Return true iff byteswap is needed to load instruction */
>   static inline bool insn_need_byteswap(CPUArchState *env)
>   {

[PATCH v2 08/15] target/ppc: Remove raise_exception_ra()

[Philippe Mathieu-Daudé]

Introduced in commit db789c6cd33 ("ppc: Provide basic
raise_exception_* functions"), raise_exception_ra() has
never been used. Remove as dead code.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/cpu.h         | 2 --
 target/ppc/excp_helper.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 0b8b4c05172..4ca27d6b389 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2753,8 +2753,6 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, vaddr *pc,
 #endif
 
 G_NORETURN void raise_exception(CPUPPCState *env, uint32_t exception);
-G_NORETURN void raise_exception_ra(CPUPPCState *env, uint32_t exception,
-                                   uintptr_t raddr);
 G_NORETURN void raise_exception_err(CPUPPCState *env, uint32_t exception,
                                     uint32_t error_code);
 G_NORETURN void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 236e5078f56..9e1a2ecc36f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2528,12 +2528,6 @@ void raise_exception(CPUPPCState *env, uint32_t exception)
     raise_exception_err_ra(env, exception, 0, 0);
 }
 
-void raise_exception_ra(CPUPPCState *env, uint32_t exception,
-                        uintptr_t raddr)
-{
-    raise_exception_err_ra(env, exception, 0, raddr);
-}
-
 #ifdef CONFIG_TCG
 void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
                                 uint32_t error_code)
-- 
2.47.1

Re: [PATCH v2 08/15] target/ppc: Remove raise_exception_ra()

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Introduced in commit db789c6cd33 ("ppc: Provide basic
> raise_exception_* functions"), raise_exception_ra() has
> never been used. Remove as dead code.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Although there are a bunch of raise_exception_err_ra calls passing
error_code as 0, I hope removing this is fine as still unused.

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   target/ppc/cpu.h         | 2 --
>   target/ppc/excp_helper.c | 6 ------
>   2 files changed, 8 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 0b8b4c05172..4ca27d6b389 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2753,8 +2753,6 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, vaddr *pc,
>   #endif
>   
>   G_NORETURN void raise_exception(CPUPPCState *env, uint32_t exception);
> -G_NORETURN void raise_exception_ra(CPUPPCState *env, uint32_t exception,
> -                                   uintptr_t raddr);
>   G_NORETURN void raise_exception_err(CPUPPCState *env, uint32_t exception,
>                                       uint32_t error_code);
>   G_NORETURN void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 236e5078f56..9e1a2ecc36f 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -2528,12 +2528,6 @@ void raise_exception(CPUPPCState *env, uint32_t exception)
>       raise_exception_err_ra(env, exception, 0, 0);
>   }
>   
> -void raise_exception_ra(CPUPPCState *env, uint32_t exception,
> -                        uintptr_t raddr)
> -{
> -    raise_exception_err_ra(env, exception, 0, raddr);
> -}
> -
>   #ifdef CONFIG_TCG
>   void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
>                                   uint32_t error_code)

Re: [PATCH v2 08/15] target/ppc: Remove raise_exception_ra()

[Philippe Mathieu-Daudé]

On 28/1/25 10:46, Harsh Prateek Bora wrote:
> 
> 
> On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
>> Introduced in commit db789c6cd33 ("ppc: Provide basic
>> raise_exception_* functions"), raise_exception_ra() has
>> never been used. Remove as dead code.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Although there are a bunch of raise_exception_err_ra calls passing
> error_code as 0, I hope removing this is fine as still unused.

I dunno about error_code=0, where I'm suspicious is raddr=0,
I'd expect raddr=GETPC(), but I haven't bothered to check yet.

> 
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> 
>> ---
>>   target/ppc/cpu.h         | 2 --
>>   target/ppc/excp_helper.c | 6 ------
>>   2 files changed, 8 deletions(-)

[PATCH v2 09/15] target/ppc: Restrict exception helpers to TCG

[Philippe Mathieu-Daudé]

Move exception helpers to tcg-excp_helper.c so they are
only built when TCG is selected.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/excp_helper.c     | 34 --------------------------------
 target/ppc/tcg-excp_helper.c | 38 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 9e1a2ecc36f..6a12402b23a 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2504,41 +2504,7 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 
 #endif /* !CONFIG_USER_ONLY */
 
-/*****************************************************************************/
-/* Exceptions processing helpers */
-
-void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
-                            uint32_t error_code, uintptr_t raddr)
-{
-    CPUState *cs = env_cpu(env);
-
-    cs->exception_index = exception;
-    env->error_code = error_code;
-    cpu_loop_exit_restore(cs, raddr);
-}
-
-void raise_exception_err(CPUPPCState *env, uint32_t exception,
-                         uint32_t error_code)
-{
-    raise_exception_err_ra(env, exception, error_code, 0);
-}
-
-void raise_exception(CPUPPCState *env, uint32_t exception)
-{
-    raise_exception_err_ra(env, exception, 0, 0);
-}
-
 #ifdef CONFIG_TCG
-void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
-                                uint32_t error_code)
-{
-    raise_exception_err_ra(env, exception, error_code, 0);
-}
-
-void helper_raise_exception(CPUPPCState *env, uint32_t exception)
-{
-    raise_exception_err_ra(env, exception, 0, 0);
-}
 
 #ifndef CONFIG_USER_ONLY
 void helper_store_msr(CPUPPCState *env, target_ulong val)
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
index 93c2d6b5a03..268a1614597 100644
--- a/target/ppc/tcg-excp_helper.c
+++ b/target/ppc/tcg-excp_helper.c
@@ -19,15 +19,53 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "exec/cpu_ldst.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
 #include "system/runstate.h"
 
+#include "helper_regs.h"
 #include "hw/ppc/ppc.h"
 #include "internal.h"
 #include "cpu.h"
 #include "trace.h"
 
+/*****************************************************************************/
+/* Exceptions processing helpers */
+
+void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
+                            uint32_t error_code, uintptr_t raddr)
+{
+    CPUState *cs = env_cpu(env);
+
+    cs->exception_index = exception;
+    env->error_code = error_code;
+    cpu_loop_exit_restore(cs, raddr);
+}
+
+void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
+                                uint32_t error_code)
+{
+    raise_exception_err_ra(env, exception, error_code, 0);
+}
+
+void helper_raise_exception(CPUPPCState *env, uint32_t exception)
+{
+    raise_exception_err_ra(env, exception, 0, 0);
+}
+
 #ifndef CONFIG_USER_ONLY
 
+void raise_exception_err(CPUPPCState *env, uint32_t exception,
+                                           uint32_t error_code)
+{
+    raise_exception_err_ra(env, exception, error_code, 0);
+}
+
+void raise_exception(CPUPPCState *env, uint32_t exception)
+{
+    raise_exception_err_ra(env, exception, 0, 0);
+}
+
 void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
                                  MMUAccessType access_type,
                                  int mmu_idx, uintptr_t retaddr)
-- 
2.47.1

Re: [PATCH v2 09/15] target/ppc: Restrict exception helpers to TCG

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Move exception helpers to tcg-excp_helper.c so they are
> only built when TCG is selected.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/excp_helper.c     | 34 --------------------------------
>   target/ppc/tcg-excp_helper.c | 38 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 38 insertions(+), 34 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 9e1a2ecc36f..6a12402b23a 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -2504,41 +2504,7 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>   
>   #endif /* !CONFIG_USER_ONLY */
>   
> -/*****************************************************************************/
> -/* Exceptions processing helpers */
> -
> -void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
> -                            uint32_t error_code, uintptr_t raddr)
> -{
> -    CPUState *cs = env_cpu(env);
> -
> -    cs->exception_index = exception;
> -    env->error_code = error_code;
> -    cpu_loop_exit_restore(cs, raddr);
> -}
> -
> -void raise_exception_err(CPUPPCState *env, uint32_t exception,
> -                         uint32_t error_code)
> -{
> -    raise_exception_err_ra(env, exception, error_code, 0);
> -}
> -
> -void raise_exception(CPUPPCState *env, uint32_t exception)
> -{
> -    raise_exception_err_ra(env, exception, 0, 0);
> -}
> -
>   #ifdef CONFIG_TCG
> -void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
> -                                uint32_t error_code)
> -{
> -    raise_exception_err_ra(env, exception, error_code, 0);
> -}
> -
> -void helper_raise_exception(CPUPPCState *env, uint32_t exception)
> -{
> -    raise_exception_err_ra(env, exception, 0, 0);
> -}
>   
>   #ifndef CONFIG_USER_ONLY
>   void helper_store_msr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index 93c2d6b5a03..268a1614597 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -19,15 +19,53 @@
>   #include "qemu/osdep.h"
>   #include "qemu/log.h"
>   #include "exec/cpu_ldst.h"
> +#include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
>   #include "system/runstate.h"
>   
> +#include "helper_regs.h"
>   #include "hw/ppc/ppc.h"
>   #include "internal.h"
>   #include "cpu.h"
>   #include "trace.h"
>   
> +/*****************************************************************************/
> +/* Exceptions processing helpers */
> +
> +void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
> +                            uint32_t error_code, uintptr_t raddr)
> +{
> +    CPUState *cs = env_cpu(env);
> +
> +    cs->exception_index = exception;
> +    env->error_code = error_code;
> +    cpu_loop_exit_restore(cs, raddr);
> +}
> +
> +void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
> +                                uint32_t error_code)
> +{
> +    raise_exception_err_ra(env, exception, error_code, 0);
> +}
> +
> +void helper_raise_exception(CPUPPCState *env, uint32_t exception)
> +{
> +    raise_exception_err_ra(env, exception, 0, 0);
> +}
> +
>   #ifndef CONFIG_USER_ONLY
>   

In excp_helper.c, below helpers were getting built when CONFIG_USER_ONLY 
is defined. Is this change to move under above ifndef intentional?

regards,
Harsh

> +void raise_exception_err(CPUPPCState *env, uint32_t exception,
> +                                           uint32_t error_code)
> +{
> +    raise_exception_err_ra(env, exception, error_code, 0);
> +}
> +
> +void raise_exception(CPUPPCState *env, uint32_t exception)
> +{
> +    raise_exception_err_ra(env, exception, 0, 0);
> +}
> +
>   void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>                                    MMUAccessType access_type,
>                                    int mmu_idx, uintptr_t retaddr)

Re: [PATCH v2 09/15] target/ppc: Restrict exception helpers to TCG

[Philippe Mathieu-Daudé]

On 28/1/25 10:59, Harsh Prateek Bora wrote:
> 
> 
> On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
>> Move exception helpers to tcg-excp_helper.c so they are
>> only built when TCG is selected.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/ppc/excp_helper.c     | 34 --------------------------------
>>   target/ppc/tcg-excp_helper.c | 38 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 38 insertions(+), 34 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 9e1a2ecc36f..6a12402b23a 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -2504,41 +2504,7 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int 
>> interrupt_request)
>>   #endif /* !CONFIG_USER_ONLY */
>> -/ 
>> *****************************************************************************/
>> -/* Exceptions processing helpers */
>> -
>> -void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
>> -                            uint32_t error_code, uintptr_t raddr)
>> -{
>> -    CPUState *cs = env_cpu(env);
>> -
>> -    cs->exception_index = exception;
>> -    env->error_code = error_code;
>> -    cpu_loop_exit_restore(cs, raddr);
>> -}
>> -
>> -void raise_exception_err(CPUPPCState *env, uint32_t exception,
>> -                         uint32_t error_code)
>> -{
>> -    raise_exception_err_ra(env, exception, error_code, 0);
>> -}
>> -
>> -void raise_exception(CPUPPCState *env, uint32_t exception)
>> -{
>> -    raise_exception_err_ra(env, exception, 0, 0);
>> -}
>> -
>>   #ifdef CONFIG_TCG
>> -void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
>> -                                uint32_t error_code)
>> -{
>> -    raise_exception_err_ra(env, exception, error_code, 0);
>> -}
>> -
>> -void helper_raise_exception(CPUPPCState *env, uint32_t exception)
>> -{
>> -    raise_exception_err_ra(env, exception, 0, 0);
>> -}
>>   #ifndef CONFIG_USER_ONLY
>>   void helper_store_msr(CPUPPCState *env, target_ulong val)
>> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
>> index 93c2d6b5a03..268a1614597 100644
>> --- a/target/ppc/tcg-excp_helper.c
>> +++ b/target/ppc/tcg-excp_helper.c
>> @@ -19,15 +19,53 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/log.h"
>>   #include "exec/cpu_ldst.h"
>> +#include "exec/exec-all.h"
>> +#include "exec/helper-proto.h"
>>   #include "system/runstate.h"
>> +#include "helper_regs.h"
>>   #include "hw/ppc/ppc.h"
>>   #include "internal.h"
>>   #include "cpu.h"
>>   #include "trace.h"
>> +/ 
>> *****************************************************************************/
>> +/* Exceptions processing helpers */
>> +
>> +void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
>> +                            uint32_t error_code, uintptr_t raddr)
>> +{
>> +    CPUState *cs = env_cpu(env);
>> +
>> +    cs->exception_index = exception;
>> +    env->error_code = error_code;
>> +    cpu_loop_exit_restore(cs, raddr);
>> +}
>> +
>> +void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
>> +                                uint32_t error_code)
>> +{
>> +    raise_exception_err_ra(env, exception, error_code, 0);
>> +}
>> +
>> +void helper_raise_exception(CPUPPCState *env, uint32_t exception)
>> +{
>> +    raise_exception_err_ra(env, exception, 0, 0);
>> +}
>> +
>>   #ifndef CONFIG_USER_ONLY
> 
> In excp_helper.c, below helpers were getting built when CONFIG_USER_ONLY 
> is defined. Is this change to move under above ifndef intentional?

Yes, they are unused otherwise. I'll mention in commit description.

> 
> regards,
> Harsh

[PATCH v2 10/15] target/ppc: Restrict ppc_tcg_hv_emu() to TCG

[Philippe Mathieu-Daudé]

Make is_prefix_insn_excp() prototype but have it guarded by
a tcg_enabled() check. Inline part of it in powerpc_excp_books().

Extract POWERPC_EXCP_HV_EMU handling code to ppc_tcg_hv_emu(),
also exposing its prototype in "internal.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/internal.h        |   6 +++
 target/ppc/excp_helper.c     | 101 +++++------------------------------
 target/ppc/tcg-excp_helper.c |  75 ++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 89 deletions(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 62186bc1e61..0e66b29ec68 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -291,6 +291,12 @@ bool ppc_cpu_debug_check_breakpoint(CPUState *cs);
 bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
 
 G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason);
+
+#if defined(TARGET_PPC64)
+bool is_prefix_insn_excp(CPUPPCState *env, int excp);
+void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
+                    int *srr0, int *srr1);
+#endif /* TARGET_PPC64 */
 #endif /* !CONFIG_USER_ONLY */
 
 FIELD(GER_MSK, XMSK, 0, 4)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 6a12402b23a..56a56148a40 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include "qemu/main-loop.h"
 #include "qemu/log.h"
+#include "system/kvm.h"
 #include "system/tcg.h"
 #include "system/system.h"
 #include "system/runstate.h"
@@ -1194,81 +1195,6 @@ static bool books_vhyp_handles_hv_excp(PowerPCCPU *cpu)
     return false;
 }
 
-#ifdef CONFIG_TCG
-static bool is_prefix_insn(CPUPPCState *env, uint32_t insn)
-{
-    if (!(env->insns_flags2 & PPC2_ISA310)) {
-        return false;
-    }
-    return ((insn & 0xfc000000) == 0x04000000);
-}
-
-static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
-{
-    CPUPPCState *env = &cpu->env;
-
-    if (!(env->insns_flags2 & PPC2_ISA310)) {
-        return false;
-    }
-
-    if (!tcg_enabled()) {
-        /*
-         * This does not load instructions and set the prefix bit correctly
-         * for injected interrupts with KVM. That may have to be discovered
-         * and set by the KVM layer before injecting.
-         */
-        return false;
-    }
-
-    switch (excp) {
-    case POWERPC_EXCP_MCHECK:
-        if (!(env->error_code & PPC_BIT(42))) {
-            /*
-             * Fetch attempt caused a machine check, so attempting to fetch
-             * again would cause a recursive machine check.
-             */
-            return false;
-        }
-        break;
-    case POWERPC_EXCP_HDSI:
-        /* HDSI PRTABLE_FAULT has the originating access type in error_code */
-        if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
-            (env->error_code == MMU_INST_FETCH)) {
-            /*
-             * Fetch failed due to partition scope translation, so prefix
-             * indication is not relevant (and attempting to load the
-             * instruction at NIP would cause recursive faults with the same
-             * translation).
-             */
-            return false;
-        }
-        break;
-
-    case POWERPC_EXCP_DSI:
-    case POWERPC_EXCP_DSEG:
-    case POWERPC_EXCP_ALIGN:
-    case POWERPC_EXCP_PROGRAM:
-    case POWERPC_EXCP_FPU:
-    case POWERPC_EXCP_TRACE:
-    case POWERPC_EXCP_HV_EMU:
-    case POWERPC_EXCP_VPU:
-    case POWERPC_EXCP_VSXU:
-    case POWERPC_EXCP_FU:
-    case POWERPC_EXCP_HV_FU:
-        break;
-    default:
-        return false;
-    }
-
-    return is_prefix_insn(env, ppc_ldl_code(env, env->nip));
-}
-#else
-static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
-{
-    return false;
-}
-#endif
-
 static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
 {
     CPUPPCState *env = &cpu->env;
@@ -1310,7 +1236,15 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     }
     vector |= env->excp_prefix;
 
-    if (is_prefix_insn_excp(cpu, excp)) {
+    if (env->insns_flags2 & PPC2_ISA310) {
+        /* nothing to do */
+    } else if (kvm_enabled()) {
+        /*
+         * This does not load instructions and set the prefix bit correctly
+         * for injected interrupts with KVM. That may have to be discovered
+         * and set by the KVM layer before injecting.
+         */
+    } else if (tcg_enabled() && is_prefix_insn_excp(env, excp)) {
         msr |= PPC_BIT(34);
     }
 
@@ -1484,20 +1418,9 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
         new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
         break;
 #ifdef CONFIG_TCG
-    case POWERPC_EXCP_HV_EMU: {
-        uint32_t insn = ppc_ldl_code(env, env->nip);
-        env->spr[SPR_HEIR] = insn;
-        if (is_prefix_insn(env, insn)) {
-            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
-            env->spr[SPR_HEIR] <<= 32;
-            env->spr[SPR_HEIR] |= insn2;
-        }
-        srr0 = SPR_HSRR0;
-        srr1 = SPR_HSRR1;
-        new_msr |= (target_ulong)MSR_HVB;
-        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
+    case POWERPC_EXCP_HV_EMU:
+        ppc_tcg_hv_emu(env, &new_msr, &srr0, &srr1);
         break;
-    }
 #endif
     case POWERPC_EXCP_VPU:       /* Vector unavailable exception             */
     case POWERPC_EXCP_VSXU:       /* VSX unavailable exception               */
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
index 268a1614597..dc5601a4577 100644
--- a/target/ppc/tcg-excp_helper.c
+++ b/target/ppc/tcg-excp_helper.c
@@ -283,4 +283,79 @@ uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
     return insn;
 }
 
+#if defined(TARGET_PPC64)
+
+static bool is_prefix_insn(CPUPPCState *env, uint32_t insn)
+{
+    if (!(env->insns_flags2 & PPC2_ISA310)) {
+        return false;
+    }
+    return ((insn & 0xfc000000) == 0x04000000);
+}
+
+bool is_prefix_insn_excp(CPUPPCState *env, int excp)
+{
+    switch (excp) {
+    case POWERPC_EXCP_MCHECK:
+        if (!(env->error_code & PPC_BIT(42))) {
+            /*
+             * Fetch attempt caused a machine check, so attempting to fetch
+             * again would cause a recursive machine check.
+             */
+            return false;
+        }
+        break;
+    case POWERPC_EXCP_HDSI:
+        /* HDSI PRTABLE_FAULT has the originating access type in error_code */
+        if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
+            (env->error_code == MMU_INST_FETCH)) {
+            /*
+             * Fetch failed due to partition scope translation, so prefix
+             * indication is not relevant (and attempting to load the
+             * instruction at NIP would cause recursive faults with the same
+             * translation).
+             */
+            return false;
+        }
+        break;
+
+    case POWERPC_EXCP_DSI:
+    case POWERPC_EXCP_DSEG:
+    case POWERPC_EXCP_ALIGN:
+    case POWERPC_EXCP_PROGRAM:
+    case POWERPC_EXCP_FPU:
+    case POWERPC_EXCP_TRACE:
+    case POWERPC_EXCP_HV_EMU:
+    case POWERPC_EXCP_VPU:
+    case POWERPC_EXCP_VSXU:
+    case POWERPC_EXCP_FU:
+    case POWERPC_EXCP_HV_FU:
+        break;
+    default:
+        return false;
+    }
+
+    return is_prefix_insn(env, ppc_ldl_code(env, env->nip));
+}
+
+void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
+                    int *srr0, int *srr1)
+{
+    uint32_t insn = ppc_ldl_code(env, env->nip);
+
+    env->spr[SPR_HEIR] = insn;
+    if (is_prefix_insn(env, insn)) {
+        uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
+
+        env->spr[SPR_HEIR] <<= 32;
+        env->spr[SPR_HEIR] |= insn2;
+    }
+    *srr0 = SPR_HSRR0;
+    *srr1 = SPR_HSRR1;
+    *new_msr |= (target_ulong)MSR_HVB;
+    *new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
+}
+
+#endif /* TARGET_PPC64 */
+
 #endif /* !CONFIG_USER_ONLY */
-- 
2.47.1

Re: [PATCH v2 10/15] target/ppc: Restrict ppc_tcg_hv_emu() to TCG

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Make is_prefix_insn_excp() prototype but have it guarded by
> a tcg_enabled() check. Inline part of it in powerpc_excp_books().
> 
> Extract POWERPC_EXCP_HV_EMU handling code to ppc_tcg_hv_emu(),
> also exposing its prototype in "internal.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/internal.h        |   6 +++
>   target/ppc/excp_helper.c     | 101 +++++------------------------------
>   target/ppc/tcg-excp_helper.c |  75 ++++++++++++++++++++++++++
>   3 files changed, 93 insertions(+), 89 deletions(-)
> 
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index 62186bc1e61..0e66b29ec68 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -291,6 +291,12 @@ bool ppc_cpu_debug_check_breakpoint(CPUState *cs);
>   bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
>   
>   G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason);
> +
> +#if defined(TARGET_PPC64)
> +bool is_prefix_insn_excp(CPUPPCState *env, int excp);
> +void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
> +                    int *srr0, int *srr1);
> +#endif /* TARGET_PPC64 */
>   #endif /* !CONFIG_USER_ONLY */
>   
>   FIELD(GER_MSK, XMSK, 0, 4)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 6a12402b23a..56a56148a40 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -19,6 +19,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/main-loop.h"
>   #include "qemu/log.h"
> +#include "system/kvm.h"
>   #include "system/tcg.h"
>   #include "system/system.h"
>   #include "system/runstate.h"
> @@ -1194,81 +1195,6 @@ static bool books_vhyp_handles_hv_excp(PowerPCCPU *cpu)
>       return false;
>   }
>   
> -#ifdef CONFIG_TCG
> -static bool is_prefix_insn(CPUPPCState *env, uint32_t insn)
> -{
> -    if (!(env->insns_flags2 & PPC2_ISA310)) {
> -        return false;
> -    }
> -    return ((insn & 0xfc000000) == 0x04000000);
> -}
> -
> -static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
> -{
> -    CPUPPCState *env = &cpu->env;
> -
> -    if (!(env->insns_flags2 & PPC2_ISA310)) {
> -        return false;
> -    }
> -
> -    if (!tcg_enabled()) {
> -        /*
> -         * This does not load instructions and set the prefix bit correctly
> -         * for injected interrupts with KVM. That may have to be discovered
> -         * and set by the KVM layer before injecting.
> -         */
> -        return false;
> -    }
> -
> -    switch (excp) {
> -    case POWERPC_EXCP_MCHECK:
> -        if (!(env->error_code & PPC_BIT(42))) {
> -            /*
> -             * Fetch attempt caused a machine check, so attempting to fetch
> -             * again would cause a recursive machine check.
> -             */
> -            return false;
> -        }
> -        break;
> -    case POWERPC_EXCP_HDSI:
> -        /* HDSI PRTABLE_FAULT has the originating access type in error_code */
> -        if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
> -            (env->error_code == MMU_INST_FETCH)) {
> -            /*
> -             * Fetch failed due to partition scope translation, so prefix
> -             * indication is not relevant (and attempting to load the
> -             * instruction at NIP would cause recursive faults with the same
> -             * translation).
> -             */
> -            return false;
> -        }
> -        break;
> -
> -    case POWERPC_EXCP_DSI:
> -    case POWERPC_EXCP_DSEG:
> -    case POWERPC_EXCP_ALIGN:
> -    case POWERPC_EXCP_PROGRAM:
> -    case POWERPC_EXCP_FPU:
> -    case POWERPC_EXCP_TRACE:
> -    case POWERPC_EXCP_HV_EMU:
> -    case POWERPC_EXCP_VPU:
> -    case POWERPC_EXCP_VSXU:
> -    case POWERPC_EXCP_FU:
> -    case POWERPC_EXCP_HV_FU:
> -        break;
> -    default:
> -        return false;
> -    }
> -
> -    return is_prefix_insn(env, ppc_ldl_code(env, env->nip));
> -}
> -#else
> -static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
> -{
> -    return false;
> -}
> -#endif
> -
>   static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>   {
>       CPUPPCState *env = &cpu->env;
> @@ -1310,7 +1236,15 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>       }
>       vector |= env->excp_prefix;
>   
> -    if (is_prefix_insn_excp(cpu, excp)) {
> +    if (env->insns_flags2 & PPC2_ISA310) {

I guess you meant checking for ! of above.
is_prefix_insn_excp() returns false for ! of above.

> +        /* nothing to do */
> +    } else if (kvm_enabled()) {
> +        /*
> +         * This does not load instructions and set the prefix bit correctly
> +         * for injected interrupts with KVM. That may have to be discovered
> +         * and set by the KVM layer before injecting.
> +         */
> +    } else if (tcg_enabled() && is_prefix_insn_excp(env, excp)) {
>           msr |= PPC_BIT(34);
>       }
>   
> @@ -1484,20 +1418,9 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>           new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>           break;
>   #ifdef CONFIG_TCG
> -    case POWERPC_EXCP_HV_EMU: {
> -        uint32_t insn = ppc_ldl_code(env, env->nip);
> -        env->spr[SPR_HEIR] = insn;
> -        if (is_prefix_insn(env, insn)) {
> -            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
> -            env->spr[SPR_HEIR] <<= 32;
> -            env->spr[SPR_HEIR] |= insn2;
> -        }
> -        srr0 = SPR_HSRR0;
> -        srr1 = SPR_HSRR1;
> -        new_msr |= (target_ulong)MSR_HVB;
> -        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> +    case POWERPC_EXCP_HV_EMU:
> +        ppc_tcg_hv_emu(env, &new_msr, &srr0, &srr1);

Naming suggestion: ppc_excp_hv_emu may be more apt.

Thanks,
Harsh

>           break;
> -    }
>   #endif
>       case POWERPC_EXCP_VPU:       /* Vector unavailable exception             */
>       case POWERPC_EXCP_VSXU:       /* VSX unavailable exception               */
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index 268a1614597..dc5601a4577 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -283,4 +283,79 @@ uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
>       return insn;
>   }
>   
> +#if defined(TARGET_PPC64)
> +
> +static bool is_prefix_insn(CPUPPCState *env, uint32_t insn)
> +{
> +    if (!(env->insns_flags2 & PPC2_ISA310)) {
> +        return false;
> +    }
> +    return ((insn & 0xfc000000) == 0x04000000);
> +}
> +
> +bool is_prefix_insn_excp(CPUPPCState *env, int excp)
> +{
> +    switch (excp) {
> +    case POWERPC_EXCP_MCHECK:
> +        if (!(env->error_code & PPC_BIT(42))) {
> +            /*
> +             * Fetch attempt caused a machine check, so attempting to fetch
> +             * again would cause a recursive machine check.
> +             */
> +            return false;
> +        }
> +        break;
> +    case POWERPC_EXCP_HDSI:
> +        /* HDSI PRTABLE_FAULT has the originating access type in error_code */
> +        if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
> +            (env->error_code == MMU_INST_FETCH)) {
> +            /*
> +             * Fetch failed due to partition scope translation, so prefix
> +             * indication is not relevant (and attempting to load the
> +             * instruction at NIP would cause recursive faults with the same
> +             * translation).
> +             */
> +            return false;
> +        }
> +        break;
> +
> +    case POWERPC_EXCP_DSI:
> +    case POWERPC_EXCP_DSEG:
> +    case POWERPC_EXCP_ALIGN:
> +    case POWERPC_EXCP_PROGRAM:
> +    case POWERPC_EXCP_FPU:
> +    case POWERPC_EXCP_TRACE:
> +    case POWERPC_EXCP_HV_EMU:
> +    case POWERPC_EXCP_VPU:
> +    case POWERPC_EXCP_VSXU:
> +    case POWERPC_EXCP_FU:
> +    case POWERPC_EXCP_HV_FU:
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    return is_prefix_insn(env, ppc_ldl_code(env, env->nip));
> +}
> +
> +void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
> +                    int *srr0, int *srr1)
> +{
> +    uint32_t insn = ppc_ldl_code(env, env->nip);
> +
> +    env->spr[SPR_HEIR] = insn;
> +    if (is_prefix_insn(env, insn)) {
> +        uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
> +
> +        env->spr[SPR_HEIR] <<= 32;
> +        env->spr[SPR_HEIR] |= insn2;
> +    }
> +    *srr0 = SPR_HSRR0;
> +    *srr1 = SPR_HSRR1;
> +    *new_msr |= (target_ulong)MSR_HVB;
> +    *new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> +}
> +
> +#endif /* TARGET_PPC64 */
> +
>   #endif /* !CONFIG_USER_ONLY */

[PATCH v2 11/15] target/ppc: Restrict various common helpers to TCG

[Philippe Mathieu-Daudé]

Move helpers common to system/user emulation to tcg-excp_helper.c.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/excp_helper.c     | 141 ----------------------------------
 target/ppc/tcg-excp_helper.c | 143 +++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+), 141 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 56a56148a40..48e08d65bd7 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2634,148 +2634,7 @@ void helper_rfmci(CPUPPCState *env)
     /* FIXME: choose CSRR1 or MCSRR1 based on cpu type */
     do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
 }
-#endif /* !CONFIG_USER_ONLY */
 
-void helper_TW(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
-               uint32_t flags)
-{
-    if (!likely(!(((int32_t)arg1 < (int32_t)arg2 && (flags & 0x10)) ||
-                  ((int32_t)arg1 > (int32_t)arg2 && (flags & 0x08)) ||
-                  ((int32_t)arg1 == (int32_t)arg2 && (flags & 0x04)) ||
-                  ((uint32_t)arg1 < (uint32_t)arg2 && (flags & 0x02)) ||
-                  ((uint32_t)arg1 > (uint32_t)arg2 && (flags & 0x01))))) {
-        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
-                               POWERPC_EXCP_TRAP, GETPC());
-    }
-}
-
-#ifdef TARGET_PPC64
-void helper_TD(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
-               uint32_t flags)
-{
-    if (!likely(!(((int64_t)arg1 < (int64_t)arg2 && (flags & 0x10)) ||
-                  ((int64_t)arg1 > (int64_t)arg2 && (flags & 0x08)) ||
-                  ((int64_t)arg1 == (int64_t)arg2 && (flags & 0x04)) ||
-                  ((uint64_t)arg1 < (uint64_t)arg2 && (flags & 0x02)) ||
-                  ((uint64_t)arg1 > (uint64_t)arg2 && (flags & 0x01))))) {
-        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
-                               POWERPC_EXCP_TRAP, GETPC());
-    }
-}
-#endif /* TARGET_PPC64 */
-
-static uint32_t helper_SIMON_LIKE_32_64(uint32_t x, uint64_t key, uint32_t lane)
-{
-    const uint16_t c = 0xfffc;
-    const uint64_t z0 = 0xfa2561cdf44ac398ULL;
-    uint16_t z = 0, temp;
-    uint16_t k[32], eff_k[32], xleft[33], xright[33], fxleft[32];
-
-    for (int i = 3; i >= 0; i--) {
-        k[i] = key & 0xffff;
-        key >>= 16;
-    }
-    xleft[0] = x & 0xffff;
-    xright[0] = (x >> 16) & 0xffff;
-
-    for (int i = 0; i < 28; i++) {
-        z = (z0 >> (63 - i)) & 1;
-        temp = ror16(k[i + 3], 3) ^ k[i + 1];
-        k[i + 4] = c ^ z ^ k[i] ^ temp ^ ror16(temp, 1);
-    }
-
-    for (int i = 0; i < 8; i++) {
-        eff_k[4 * i + 0] = k[4 * i + ((0 + lane) % 4)];
-        eff_k[4 * i + 1] = k[4 * i + ((1 + lane) % 4)];
-        eff_k[4 * i + 2] = k[4 * i + ((2 + lane) % 4)];
-        eff_k[4 * i + 3] = k[4 * i + ((3 + lane) % 4)];
-    }
-
-    for (int i = 0; i < 32; i++) {
-        fxleft[i] = (rol16(xleft[i], 1) &
-            rol16(xleft[i], 8)) ^ rol16(xleft[i], 2);
-        xleft[i + 1] = xright[i] ^ fxleft[i] ^ eff_k[i];
-        xright[i + 1] = xleft[i];
-    }
-
-    return (((uint32_t)xright[32]) << 16) | xleft[32];
-}
-
-static uint64_t hash_digest(uint64_t ra, uint64_t rb, uint64_t key)
-{
-    uint64_t stage0_h = 0ULL, stage0_l = 0ULL;
-    uint64_t stage1_h, stage1_l;
-
-    for (int i = 0; i < 4; i++) {
-        stage0_h |= ror64(rb & 0xff, 8 * (2 * i + 1));
-        stage0_h |= ((ra >> 32) & 0xff) << (8 * 2 * i);
-        stage0_l |= ror64((rb >> 32) & 0xff, 8 * (2 * i + 1));
-        stage0_l |= (ra & 0xff) << (8 * 2 * i);
-        rb >>= 8;
-        ra >>= 8;
-    }
-
-    stage1_h = (uint64_t)helper_SIMON_LIKE_32_64(stage0_h >> 32, key, 0) << 32;
-    stage1_h |= helper_SIMON_LIKE_32_64(stage0_h, key, 1);
-    stage1_l = (uint64_t)helper_SIMON_LIKE_32_64(stage0_l >> 32, key, 2) << 32;
-    stage1_l |= helper_SIMON_LIKE_32_64(stage0_l, key, 3);
-
-    return stage1_h ^ stage1_l;
-}
-
-static void do_hash(CPUPPCState *env, target_ulong ea, target_ulong ra,
-                    target_ulong rb, uint64_t key, bool store)
-{
-    uint64_t calculated_hash = hash_digest(ra, rb, key), loaded_hash;
-
-    if (store) {
-        cpu_stq_data_ra(env, ea, calculated_hash, GETPC());
-    } else {
-        loaded_hash = cpu_ldq_data_ra(env, ea, GETPC());
-        if (loaded_hash != calculated_hash) {
-            raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
-                POWERPC_EXCP_TRAP, GETPC());
-        }
-    }
-}
-
-#include "qemu/guest-random.h"
-
-#ifdef TARGET_PPC64
-#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
-void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
-                 target_ulong rb)                                             \
-{                                                                             \
-    if (env->msr & R_MSR_PR_MASK) {                                           \
-        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PRO_##dexcr_aspect##_MASK ||      \
-            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
-            return;                                                           \
-    } else if (!(env->msr & R_MSR_HV_MASK)) {                                 \
-        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PNH_##dexcr_aspect##_MASK ||      \
-            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
-            return;                                                           \
-    } else if (!(env->msr & R_MSR_S_MASK)) {                                  \
-        if (!(env->spr[SPR_HDEXCR] & R_HDEXCR_HNU_##dexcr_aspect##_MASK))     \
-            return;                                                           \
-    }                                                                         \
-                                                                              \
-    do_hash(env, ea, ra, rb, key, store);                                     \
-}
-#else
-#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
-void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
-                 target_ulong rb)                                             \
-{                                                                             \
-    do_hash(env, ea, ra, rb, key, store);                                     \
-}
-#endif /* TARGET_PPC64 */
-
-HELPER_HASH(HASHST, env->spr[SPR_HASHKEYR], true, NPHIE)
-HELPER_HASH(HASHCHK, env->spr[SPR_HASHKEYR], false, NPHIE)
-HELPER_HASH(HASHSTP, env->spr[SPR_HASHPKEYR], true, PHIE)
-HELPER_HASH(HASHCHKP, env->spr[SPR_HASHPKEYR], false, PHIE)
-
-#ifndef CONFIG_USER_ONLY
 /* Embedded.Processor Control */
 static int dbell2irq(target_ulong rb)
 {
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
index dc5601a4577..5ad39cacc92 100644
--- a/target/ppc/tcg-excp_helper.c
+++ b/target/ppc/tcg-excp_helper.c
@@ -66,6 +66,149 @@ void raise_exception(CPUPPCState *env, uint32_t exception)
     raise_exception_err_ra(env, exception, 0, 0);
 }
 
+#endif /* CONFIG_USER_ONLY */
+
+void helper_TW(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
+               uint32_t flags)
+{
+    if (!likely(!(((int32_t)arg1 < (int32_t)arg2 && (flags & 0x10)) ||
+                  ((int32_t)arg1 > (int32_t)arg2 && (flags & 0x08)) ||
+                  ((int32_t)arg1 == (int32_t)arg2 && (flags & 0x04)) ||
+                  ((uint32_t)arg1 < (uint32_t)arg2 && (flags & 0x02)) ||
+                  ((uint32_t)arg1 > (uint32_t)arg2 && (flags & 0x01))))) {
+        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
+                               POWERPC_EXCP_TRAP, GETPC());
+    }
+}
+
+#ifdef TARGET_PPC64
+void helper_TD(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
+               uint32_t flags)
+{
+    if (!likely(!(((int64_t)arg1 < (int64_t)arg2 && (flags & 0x10)) ||
+                  ((int64_t)arg1 > (int64_t)arg2 && (flags & 0x08)) ||
+                  ((int64_t)arg1 == (int64_t)arg2 && (flags & 0x04)) ||
+                  ((uint64_t)arg1 < (uint64_t)arg2 && (flags & 0x02)) ||
+                  ((uint64_t)arg1 > (uint64_t)arg2 && (flags & 0x01))))) {
+        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
+                               POWERPC_EXCP_TRAP, GETPC());
+    }
+}
+#endif /* TARGET_PPC64 */
+
+static uint32_t helper_SIMON_LIKE_32_64(uint32_t x, uint64_t key, uint32_t lane)
+{
+    const uint16_t c = 0xfffc;
+    const uint64_t z0 = 0xfa2561cdf44ac398ULL;
+    uint16_t z = 0, temp;
+    uint16_t k[32], eff_k[32], xleft[33], xright[33], fxleft[32];
+
+    for (int i = 3; i >= 0; i--) {
+        k[i] = key & 0xffff;
+        key >>= 16;
+    }
+    xleft[0] = x & 0xffff;
+    xright[0] = (x >> 16) & 0xffff;
+
+    for (int i = 0; i < 28; i++) {
+        z = (z0 >> (63 - i)) & 1;
+        temp = ror16(k[i + 3], 3) ^ k[i + 1];
+        k[i + 4] = c ^ z ^ k[i] ^ temp ^ ror16(temp, 1);
+    }
+
+    for (int i = 0; i < 8; i++) {
+        eff_k[4 * i + 0] = k[4 * i + ((0 + lane) % 4)];
+        eff_k[4 * i + 1] = k[4 * i + ((1 + lane) % 4)];
+        eff_k[4 * i + 2] = k[4 * i + ((2 + lane) % 4)];
+        eff_k[4 * i + 3] = k[4 * i + ((3 + lane) % 4)];
+    }
+
+    for (int i = 0; i < 32; i++) {
+        fxleft[i] = (rol16(xleft[i], 1) &
+            rol16(xleft[i], 8)) ^ rol16(xleft[i], 2);
+        xleft[i + 1] = xright[i] ^ fxleft[i] ^ eff_k[i];
+        xright[i + 1] = xleft[i];
+    }
+
+    return (((uint32_t)xright[32]) << 16) | xleft[32];
+}
+
+static uint64_t hash_digest(uint64_t ra, uint64_t rb, uint64_t key)
+{
+    uint64_t stage0_h = 0ULL, stage0_l = 0ULL;
+    uint64_t stage1_h, stage1_l;
+
+    for (int i = 0; i < 4; i++) {
+        stage0_h |= ror64(rb & 0xff, 8 * (2 * i + 1));
+        stage0_h |= ((ra >> 32) & 0xff) << (8 * 2 * i);
+        stage0_l |= ror64((rb >> 32) & 0xff, 8 * (2 * i + 1));
+        stage0_l |= (ra & 0xff) << (8 * 2 * i);
+        rb >>= 8;
+        ra >>= 8;
+    }
+
+    stage1_h = (uint64_t)helper_SIMON_LIKE_32_64(stage0_h >> 32, key, 0) << 32;
+    stage1_h |= helper_SIMON_LIKE_32_64(stage0_h, key, 1);
+    stage1_l = (uint64_t)helper_SIMON_LIKE_32_64(stage0_l >> 32, key, 2) << 32;
+    stage1_l |= helper_SIMON_LIKE_32_64(stage0_l, key, 3);
+
+    return stage1_h ^ stage1_l;
+}
+
+static void do_hash(CPUPPCState *env, target_ulong ea, target_ulong ra,
+                    target_ulong rb, uint64_t key, bool store)
+{
+    uint64_t calculated_hash = hash_digest(ra, rb, key), loaded_hash;
+
+    if (store) {
+        cpu_stq_data_ra(env, ea, calculated_hash, GETPC());
+    } else {
+        loaded_hash = cpu_ldq_data_ra(env, ea, GETPC());
+        if (loaded_hash != calculated_hash) {
+            raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
+                POWERPC_EXCP_TRAP, GETPC());
+        }
+    }
+}
+
+#include "qemu/guest-random.h"
+
+#ifdef TARGET_PPC64
+#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
+void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
+                 target_ulong rb)                                             \
+{                                                                             \
+    if (env->msr & R_MSR_PR_MASK) {                                           \
+        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PRO_##dexcr_aspect##_MASK ||      \
+            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
+            return;                                                           \
+    } else if (!(env->msr & R_MSR_HV_MASK)) {                                 \
+        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PNH_##dexcr_aspect##_MASK ||      \
+            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
+            return;                                                           \
+    } else if (!(env->msr & R_MSR_S_MASK)) {                                  \
+        if (!(env->spr[SPR_HDEXCR] & R_HDEXCR_HNU_##dexcr_aspect##_MASK))     \
+            return;                                                           \
+    }                                                                         \
+                                                                              \
+    do_hash(env, ea, ra, rb, key, store);                                     \
+}
+#else
+#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
+void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
+                 target_ulong rb)                                             \
+{                                                                             \
+    do_hash(env, ea, ra, rb, key, store);                                     \
+}
+#endif /* TARGET_PPC64 */
+
+HELPER_HASH(HASHST, env->spr[SPR_HASHKEYR], true, NPHIE)
+HELPER_HASH(HASHCHK, env->spr[SPR_HASHKEYR], false, NPHIE)
+HELPER_HASH(HASHSTP, env->spr[SPR_HASHPKEYR], true, PHIE)
+HELPER_HASH(HASHCHKP, env->spr[SPR_HASHPKEYR], false, PHIE)
+
+#ifndef CONFIG_USER_ONLY
+
 void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
                                  MMUAccessType access_type,
                                  int mmu_idx, uintptr_t retaddr)
-- 
2.47.1

Re: [PATCH v2 11/15] target/ppc: Restrict various common helpers to TCG

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Move helpers common to system/user emulation to tcg-excp_helper.c.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/excp_helper.c     | 141 ----------------------------------
>   target/ppc/tcg-excp_helper.c | 143 +++++++++++++++++++++++++++++++++++
>   2 files changed, 143 insertions(+), 141 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 56a56148a40..48e08d65bd7 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -2634,148 +2634,7 @@ void helper_rfmci(CPUPPCState *env)
>       /* FIXME: choose CSRR1 or MCSRR1 based on cpu type */
>       do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
>   }
> -#endif /* !CONFIG_USER_ONLY */
>   
> -void helper_TW(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
> -               uint32_t flags)
> -{
> -    if (!likely(!(((int32_t)arg1 < (int32_t)arg2 && (flags & 0x10)) ||
> -                  ((int32_t)arg1 > (int32_t)arg2 && (flags & 0x08)) ||
> -                  ((int32_t)arg1 == (int32_t)arg2 && (flags & 0x04)) ||
> -                  ((uint32_t)arg1 < (uint32_t)arg2 && (flags & 0x02)) ||
> -                  ((uint32_t)arg1 > (uint32_t)arg2 && (flags & 0x01))))) {
> -        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
> -                               POWERPC_EXCP_TRAP, GETPC());
> -    }
> -}
> -
> -#ifdef TARGET_PPC64
> -void helper_TD(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
> -               uint32_t flags)
> -{
> -    if (!likely(!(((int64_t)arg1 < (int64_t)arg2 && (flags & 0x10)) ||
> -                  ((int64_t)arg1 > (int64_t)arg2 && (flags & 0x08)) ||
> -                  ((int64_t)arg1 == (int64_t)arg2 && (flags & 0x04)) ||
> -                  ((uint64_t)arg1 < (uint64_t)arg2 && (flags & 0x02)) ||
> -                  ((uint64_t)arg1 > (uint64_t)arg2 && (flags & 0x01))))) {
> -        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
> -                               POWERPC_EXCP_TRAP, GETPC());
> -    }
> -}
> -#endif /* TARGET_PPC64 */
> -
> -static uint32_t helper_SIMON_LIKE_32_64(uint32_t x, uint64_t key, uint32_t lane)
> -{
> -    const uint16_t c = 0xfffc;
> -    const uint64_t z0 = 0xfa2561cdf44ac398ULL;
> -    uint16_t z = 0, temp;
> -    uint16_t k[32], eff_k[32], xleft[33], xright[33], fxleft[32];
> -
> -    for (int i = 3; i >= 0; i--) {
> -        k[i] = key & 0xffff;
> -        key >>= 16;
> -    }
> -    xleft[0] = x & 0xffff;
> -    xright[0] = (x >> 16) & 0xffff;
> -
> -    for (int i = 0; i < 28; i++) {
> -        z = (z0 >> (63 - i)) & 1;
> -        temp = ror16(k[i + 3], 3) ^ k[i + 1];
> -        k[i + 4] = c ^ z ^ k[i] ^ temp ^ ror16(temp, 1);
> -    }
> -
> -    for (int i = 0; i < 8; i++) {
> -        eff_k[4 * i + 0] = k[4 * i + ((0 + lane) % 4)];
> -        eff_k[4 * i + 1] = k[4 * i + ((1 + lane) % 4)];
> -        eff_k[4 * i + 2] = k[4 * i + ((2 + lane) % 4)];
> -        eff_k[4 * i + 3] = k[4 * i + ((3 + lane) % 4)];
> -    }
> -
> -    for (int i = 0; i < 32; i++) {
> -        fxleft[i] = (rol16(xleft[i], 1) &
> -            rol16(xleft[i], 8)) ^ rol16(xleft[i], 2);
> -        xleft[i + 1] = xright[i] ^ fxleft[i] ^ eff_k[i];
> -        xright[i + 1] = xleft[i];
> -    }
> -
> -    return (((uint32_t)xright[32]) << 16) | xleft[32];
> -}
> -
> -static uint64_t hash_digest(uint64_t ra, uint64_t rb, uint64_t key)
> -{
> -    uint64_t stage0_h = 0ULL, stage0_l = 0ULL;
> -    uint64_t stage1_h, stage1_l;
> -
> -    for (int i = 0; i < 4; i++) {
> -        stage0_h |= ror64(rb & 0xff, 8 * (2 * i + 1));
> -        stage0_h |= ((ra >> 32) & 0xff) << (8 * 2 * i);
> -        stage0_l |= ror64((rb >> 32) & 0xff, 8 * (2 * i + 1));
> -        stage0_l |= (ra & 0xff) << (8 * 2 * i);
> -        rb >>= 8;
> -        ra >>= 8;
> -    }
> -
> -    stage1_h = (uint64_t)helper_SIMON_LIKE_32_64(stage0_h >> 32, key, 0) << 32;
> -    stage1_h |= helper_SIMON_LIKE_32_64(stage0_h, key, 1);
> -    stage1_l = (uint64_t)helper_SIMON_LIKE_32_64(stage0_l >> 32, key, 2) << 32;
> -    stage1_l |= helper_SIMON_LIKE_32_64(stage0_l, key, 3);
> -
> -    return stage1_h ^ stage1_l;
> -}
> -
> -static void do_hash(CPUPPCState *env, target_ulong ea, target_ulong ra,
> -                    target_ulong rb, uint64_t key, bool store)
> -{
> -    uint64_t calculated_hash = hash_digest(ra, rb, key), loaded_hash;
> -
> -    if (store) {
> -        cpu_stq_data_ra(env, ea, calculated_hash, GETPC());
> -    } else {
> -        loaded_hash = cpu_ldq_data_ra(env, ea, GETPC());
> -        if (loaded_hash != calculated_hash) {
> -            raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
> -                POWERPC_EXCP_TRAP, GETPC());
> -        }
> -    }
> -}
> -
> -#include "qemu/guest-random.h"
> -
> -#ifdef TARGET_PPC64
> -#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
> -void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
> -                 target_ulong rb)                                             \
> -{                                                                             \
> -    if (env->msr & R_MSR_PR_MASK) {                                           \
> -        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PRO_##dexcr_aspect##_MASK ||      \
> -            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
> -            return;                                                           \
> -    } else if (!(env->msr & R_MSR_HV_MASK)) {                                 \
> -        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PNH_##dexcr_aspect##_MASK ||      \
> -            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
> -            return;                                                           \
> -    } else if (!(env->msr & R_MSR_S_MASK)) {                                  \
> -        if (!(env->spr[SPR_HDEXCR] & R_HDEXCR_HNU_##dexcr_aspect##_MASK))     \
> -            return;                                                           \
> -    }                                                                         \
> -                                                                              \
> -    do_hash(env, ea, ra, rb, key, store);                                     \
> -}
> -#else
> -#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
> -void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
> -                 target_ulong rb)                                             \
> -{                                                                             \
> -    do_hash(env, ea, ra, rb, key, store);                                     \
> -}
> -#endif /* TARGET_PPC64 */
> -
> -HELPER_HASH(HASHST, env->spr[SPR_HASHKEYR], true, NPHIE)
> -HELPER_HASH(HASHCHK, env->spr[SPR_HASHKEYR], false, NPHIE)
> -HELPER_HASH(HASHSTP, env->spr[SPR_HASHPKEYR], true, PHIE)
> -HELPER_HASH(HASHCHKP, env->spr[SPR_HASHPKEYR], false, PHIE)
> -
> -#ifndef CONFIG_USER_ONLY
>   /* Embedded.Processor Control */
>   static int dbell2irq(target_ulong rb)
>   {
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index dc5601a4577..5ad39cacc92 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -66,6 +66,149 @@ void raise_exception(CPUPPCState *env, uint32_t exception)
>       raise_exception_err_ra(env, exception, 0, 0);
>   }
>   
> +#endif /* CONFIG_USER_ONLY */

Comment update needed: /* !CONFIG_USER_ONLY */

Otherwise,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> +
> +void helper_TW(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
> +               uint32_t flags)
> +{
> +    if (!likely(!(((int32_t)arg1 < (int32_t)arg2 && (flags & 0x10)) ||
> +                  ((int32_t)arg1 > (int32_t)arg2 && (flags & 0x08)) ||
> +                  ((int32_t)arg1 == (int32_t)arg2 && (flags & 0x04)) ||
> +                  ((uint32_t)arg1 < (uint32_t)arg2 && (flags & 0x02)) ||
> +                  ((uint32_t)arg1 > (uint32_t)arg2 && (flags & 0x01))))) {
> +        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
> +                               POWERPC_EXCP_TRAP, GETPC());
> +    }
> +}
> +
> +#ifdef TARGET_PPC64
> +void helper_TD(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
> +               uint32_t flags)
> +{
> +    if (!likely(!(((int64_t)arg1 < (int64_t)arg2 && (flags & 0x10)) ||
> +                  ((int64_t)arg1 > (int64_t)arg2 && (flags & 0x08)) ||
> +                  ((int64_t)arg1 == (int64_t)arg2 && (flags & 0x04)) ||
> +                  ((uint64_t)arg1 < (uint64_t)arg2 && (flags & 0x02)) ||
> +                  ((uint64_t)arg1 > (uint64_t)arg2 && (flags & 0x01))))) {
> +        raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
> +                               POWERPC_EXCP_TRAP, GETPC());
> +    }
> +}
> +#endif /* TARGET_PPC64 */
> +
> +static uint32_t helper_SIMON_LIKE_32_64(uint32_t x, uint64_t key, uint32_t lane)
> +{
> +    const uint16_t c = 0xfffc;
> +    const uint64_t z0 = 0xfa2561cdf44ac398ULL;
> +    uint16_t z = 0, temp;
> +    uint16_t k[32], eff_k[32], xleft[33], xright[33], fxleft[32];
> +
> +    for (int i = 3; i >= 0; i--) {
> +        k[i] = key & 0xffff;
> +        key >>= 16;
> +    }
> +    xleft[0] = x & 0xffff;
> +    xright[0] = (x >> 16) & 0xffff;
> +
> +    for (int i = 0; i < 28; i++) {
> +        z = (z0 >> (63 - i)) & 1;
> +        temp = ror16(k[i + 3], 3) ^ k[i + 1];
> +        k[i + 4] = c ^ z ^ k[i] ^ temp ^ ror16(temp, 1);
> +    }
> +
> +    for (int i = 0; i < 8; i++) {
> +        eff_k[4 * i + 0] = k[4 * i + ((0 + lane) % 4)];
> +        eff_k[4 * i + 1] = k[4 * i + ((1 + lane) % 4)];
> +        eff_k[4 * i + 2] = k[4 * i + ((2 + lane) % 4)];
> +        eff_k[4 * i + 3] = k[4 * i + ((3 + lane) % 4)];
> +    }
> +
> +    for (int i = 0; i < 32; i++) {
> +        fxleft[i] = (rol16(xleft[i], 1) &
> +            rol16(xleft[i], 8)) ^ rol16(xleft[i], 2);
> +        xleft[i + 1] = xright[i] ^ fxleft[i] ^ eff_k[i];
> +        xright[i + 1] = xleft[i];
> +    }
> +
> +    return (((uint32_t)xright[32]) << 16) | xleft[32];
> +}
> +
> +static uint64_t hash_digest(uint64_t ra, uint64_t rb, uint64_t key)
> +{
> +    uint64_t stage0_h = 0ULL, stage0_l = 0ULL;
> +    uint64_t stage1_h, stage1_l;
> +
> +    for (int i = 0; i < 4; i++) {
> +        stage0_h |= ror64(rb & 0xff, 8 * (2 * i + 1));
> +        stage0_h |= ((ra >> 32) & 0xff) << (8 * 2 * i);
> +        stage0_l |= ror64((rb >> 32) & 0xff, 8 * (2 * i + 1));
> +        stage0_l |= (ra & 0xff) << (8 * 2 * i);
> +        rb >>= 8;
> +        ra >>= 8;
> +    }
> +
> +    stage1_h = (uint64_t)helper_SIMON_LIKE_32_64(stage0_h >> 32, key, 0) << 32;
> +    stage1_h |= helper_SIMON_LIKE_32_64(stage0_h, key, 1);
> +    stage1_l = (uint64_t)helper_SIMON_LIKE_32_64(stage0_l >> 32, key, 2) << 32;
> +    stage1_l |= helper_SIMON_LIKE_32_64(stage0_l, key, 3);
> +
> +    return stage1_h ^ stage1_l;
> +}
> +
> +static void do_hash(CPUPPCState *env, target_ulong ea, target_ulong ra,
> +                    target_ulong rb, uint64_t key, bool store)
> +{
> +    uint64_t calculated_hash = hash_digest(ra, rb, key), loaded_hash;
> +
> +    if (store) {
> +        cpu_stq_data_ra(env, ea, calculated_hash, GETPC());
> +    } else {
> +        loaded_hash = cpu_ldq_data_ra(env, ea, GETPC());
> +        if (loaded_hash != calculated_hash) {
> +            raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
> +                POWERPC_EXCP_TRAP, GETPC());
> +        }
> +    }
> +}
> +
> +#include "qemu/guest-random.h"
> +
> +#ifdef TARGET_PPC64
> +#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
> +void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
> +                 target_ulong rb)                                             \
> +{                                                                             \
> +    if (env->msr & R_MSR_PR_MASK) {                                           \
> +        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PRO_##dexcr_aspect##_MASK ||      \
> +            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
> +            return;                                                           \
> +    } else if (!(env->msr & R_MSR_HV_MASK)) {                                 \
> +        if (!(env->spr[SPR_DEXCR] & R_DEXCR_PNH_##dexcr_aspect##_MASK ||      \
> +            env->spr[SPR_HDEXCR] & R_HDEXCR_ENF_##dexcr_aspect##_MASK))       \
> +            return;                                                           \
> +    } else if (!(env->msr & R_MSR_S_MASK)) {                                  \
> +        if (!(env->spr[SPR_HDEXCR] & R_HDEXCR_HNU_##dexcr_aspect##_MASK))     \
> +            return;                                                           \
> +    }                                                                         \
> +                                                                              \
> +    do_hash(env, ea, ra, rb, key, store);                                     \
> +}
> +#else
> +#define HELPER_HASH(op, key, store, dexcr_aspect)                             \
> +void helper_##op(CPUPPCState *env, target_ulong ea, target_ulong ra,          \
> +                 target_ulong rb)                                             \
> +{                                                                             \
> +    do_hash(env, ea, ra, rb, key, store);                                     \
> +}
> +#endif /* TARGET_PPC64 */
> +
> +HELPER_HASH(HASHST, env->spr[SPR_HASHKEYR], true, NPHIE)
> +HELPER_HASH(HASHCHK, env->spr[SPR_HASHKEYR], false, NPHIE)
> +HELPER_HASH(HASHSTP, env->spr[SPR_HASHPKEYR], true, PHIE)
> +HELPER_HASH(HASHCHKP, env->spr[SPR_HASHPKEYR], false, PHIE)
> +
> +#ifndef CONFIG_USER_ONLY
> +
>   void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>                                    MMUAccessType access_type,
>                                    int mmu_idx, uintptr_t retaddr)

[PATCH v2 12/15] target/ppc: Fix style in excp_helper.c

[Philippe Mathieu-Daudé]

Fix style in do_rfi() before moving the code around.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/excp_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 48e08d65bd7..661d9650d9f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2481,8 +2481,9 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
     msr &= ~(1ULL << MSR_POW);
 
     /* MSR:TGPR cannot be set by any form of rfi */
-    if (env->flags & POWERPC_FLAG_TGPR)
+    if (env->flags & POWERPC_FLAG_TGPR) {
         msr &= ~(1ULL << MSR_TGPR);
+    }
 
 #ifdef TARGET_PPC64
     /* Switching to 32-bit ? Crop the nip */
-- 
2.47.1

Re: [PATCH v2 12/15] target/ppc: Fix style in excp_helper.c

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Fix style in do_rfi() before moving the code around.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   target/ppc/excp_helper.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 48e08d65bd7..661d9650d9f 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -2481,8 +2481,9 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
>       msr &= ~(1ULL << MSR_POW);
>   
>       /* MSR:TGPR cannot be set by any form of rfi */
> -    if (env->flags & POWERPC_FLAG_TGPR)
> +    if (env->flags & POWERPC_FLAG_TGPR) {
>           msr &= ~(1ULL << MSR_TGPR);
> +    }
>   
>   #ifdef TARGET_PPC64
>       /* Switching to 32-bit ? Crop the nip */

[PATCH v2 13/15] target/ppc: Make powerpc_excp() prototype public

[Philippe Mathieu-Daudé]

In order to move TCG specific code dependent on powerpc_excp()
in the next commit, expose its prototype in "internal.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/internal.h    | 1 +
 target/ppc/excp_helper.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 0e66b29ec68..b8997ba31db 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -291,6 +291,7 @@ bool ppc_cpu_debug_check_breakpoint(CPUState *cs);
 bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
 
 G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason);
+void powerpc_excp(PowerPCCPU *cpu, int excp);
 
 #if defined(TARGET_PPC64)
 bool is_prefix_insn_excp(CPUPPCState *env, int excp);
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 661d9650d9f..f0e734e1412 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1494,7 +1494,7 @@ static inline void powerpc_excp_books(PowerPCCPU *cpu, int excp)
 }
 #endif /* TARGET_PPC64 */
 
-static void powerpc_excp(PowerPCCPU *cpu, int excp)
+void powerpc_excp(PowerPCCPU *cpu, int excp)
 {
     CPUPPCState *env = &cpu->env;
 
-- 
2.47.1

Re: [PATCH v2 13/15] target/ppc: Make powerpc_excp() prototype public

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> In order to move TCG specific code dependent on powerpc_excp()
> in the next commit, expose its prototype in "internal.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   target/ppc/internal.h    | 1 +
>   target/ppc/excp_helper.c | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index 0e66b29ec68..b8997ba31db 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -291,6 +291,7 @@ bool ppc_cpu_debug_check_breakpoint(CPUState *cs);
>   bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
>   
>   G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason);
> +void powerpc_excp(PowerPCCPU *cpu, int excp);
>   
>   #if defined(TARGET_PPC64)
>   bool is_prefix_insn_excp(CPUPPCState *env, int excp);
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 661d9650d9f..f0e734e1412 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1494,7 +1494,7 @@ static inline void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>   }
>   #endif /* TARGET_PPC64 */
>   
> -static void powerpc_excp(PowerPCCPU *cpu, int excp)
> +void powerpc_excp(PowerPCCPU *cpu, int excp)
>   {
>       CPUPPCState *env = &cpu->env;
>   

[PATCH v2 14/15] target/ppc: Restrict ATTN / SCV / PMINSN helpers to TCG

[Philippe Mathieu-Daudé]

Move helper_attn(), helper_scv() and helper_pminsn() to
tcg-excp_helper.c.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/excp_helper.c     | 45 ------------------------------------
 target/ppc/tcg-excp_helper.c | 39 +++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 45 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index f0e734e1412..2deed155987 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -400,21 +400,6 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
     env->reserve_addr = -1;
 }
 
-#ifdef CONFIG_TCG
-#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
-void helper_attn(CPUPPCState *env)
-{
-    /* POWER attn is unprivileged when enabled by HID, otherwise illegal */
-    if ((*env->check_attn)(env)) {
-        powerpc_checkstop(env, "host executed attn");
-    } else {
-        raise_exception_err(env, POWERPC_EXCP_HV_EMU,
-                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
-    }
-}
-#endif
-#endif /* CONFIG_TCG */
-
 static void powerpc_mcheck_checkstop(CPUPPCState *env)
 {
     /* KVM guests always have MSR[ME] enabled */
@@ -2445,36 +2430,6 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
     ppc_maybe_interrupt(env);
 }
 
-#ifdef TARGET_PPC64
-void helper_scv(CPUPPCState *env, uint32_t lev)
-{
-    if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
-        raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
-    } else {
-        raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
-    }
-}
-
-void helper_pminsn(CPUPPCState *env, uint32_t insn)
-{
-    CPUState *cs = env_cpu(env);
-
-    cs->halted = 1;
-
-    /* Condition for waking up at 0x100 */
-    env->resume_as_sreset = (insn != PPC_PM_STOP) ||
-        (env->spr[SPR_PSSCR] & PSSCR_EC);
-
-    /* HDECR is not to wake from PM state, it may have already fired */
-    if (env->resume_as_sreset) {
-        PowerPCCPU *cpu = env_archcpu(env);
-        ppc_set_irq(cpu, PPC_INTERRUPT_HDECR, 0);
-    }
-
-    ppc_maybe_interrupt(env);
-}
-#endif /* TARGET_PPC64 */
-
 static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
 {
     /* MSR:POW cannot be set by any form of rfi */
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
index 5ad39cacc92..4517b458b79 100644
--- a/target/ppc/tcg-excp_helper.c
+++ b/target/ppc/tcg-excp_helper.c
@@ -499,6 +499,45 @@ void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
     *new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
 }
 
+void helper_attn(CPUPPCState *env)
+{
+    /* POWER attn is unprivileged when enabled by HID, otherwise illegal */
+    if ((*env->check_attn)(env)) {
+        powerpc_checkstop(env, "host executed attn");
+    } else {
+        raise_exception_err(env, POWERPC_EXCP_HV_EMU,
+                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
+    }
+}
+
+void helper_scv(CPUPPCState *env, uint32_t lev)
+{
+    if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
+        raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
+    } else {
+        raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
+    }
+}
+
+void helper_pminsn(CPUPPCState *env, uint32_t insn)
+{
+    CPUState *cs = env_cpu(env);
+
+    cs->halted = 1;
+
+    /* Condition for waking up at 0x100 */
+    env->resume_as_sreset = (insn != PPC_PM_STOP) ||
+        (env->spr[SPR_PSSCR] & PSSCR_EC);
+
+    /* HDECR is not to wake from PM state, it may have already fired */
+    if (env->resume_as_sreset) {
+        PowerPCCPU *cpu = env_archcpu(env);
+        ppc_set_irq(cpu, PPC_INTERRUPT_HDECR, 0);
+    }
+
+    ppc_maybe_interrupt(env);
+}
+
 #endif /* TARGET_PPC64 */
 
 #endif /* !CONFIG_USER_ONLY */
-- 
2.47.1

Re: [PATCH v2 14/15] target/ppc: Restrict ATTN / SCV / PMINSN helpers to TCG

[Harsh Prateek Bora]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Move helper_attn(), helper_scv() and helper_pminsn() to
> tcg-excp_helper.c.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   target/ppc/excp_helper.c     | 45 ------------------------------------
>   target/ppc/tcg-excp_helper.c | 39 +++++++++++++++++++++++++++++++
>   2 files changed, 39 insertions(+), 45 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index f0e734e1412..2deed155987 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -400,21 +400,6 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
>       env->reserve_addr = -1;
>   }
>   
> -#ifdef CONFIG_TCG
> -#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> -void helper_attn(CPUPPCState *env)
> -{
> -    /* POWER attn is unprivileged when enabled by HID, otherwise illegal */
> -    if ((*env->check_attn)(env)) {
> -        powerpc_checkstop(env, "host executed attn");
> -    } else {
> -        raise_exception_err(env, POWERPC_EXCP_HV_EMU,
> -                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
> -    }
> -}
> -#endif
> -#endif /* CONFIG_TCG */
> -
>   static void powerpc_mcheck_checkstop(CPUPPCState *env)
>   {
>       /* KVM guests always have MSR[ME] enabled */
> @@ -2445,36 +2430,6 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
>       ppc_maybe_interrupt(env);
>   }
>   
> -#ifdef TARGET_PPC64
> -void helper_scv(CPUPPCState *env, uint32_t lev)
> -{
> -    if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> -        raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
> -    } else {
> -        raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> -    }
> -}
> -
> -void helper_pminsn(CPUPPCState *env, uint32_t insn)
> -{
> -    CPUState *cs = env_cpu(env);
> -
> -    cs->halted = 1;
> -
> -    /* Condition for waking up at 0x100 */
> -    env->resume_as_sreset = (insn != PPC_PM_STOP) ||
> -        (env->spr[SPR_PSSCR] & PSSCR_EC);
> -
> -    /* HDECR is not to wake from PM state, it may have already fired */
> -    if (env->resume_as_sreset) {
> -        PowerPCCPU *cpu = env_archcpu(env);
> -        ppc_set_irq(cpu, PPC_INTERRUPT_HDECR, 0);
> -    }
> -
> -    ppc_maybe_interrupt(env);
> -}
> -#endif /* TARGET_PPC64 */
> -
>   static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
>   {
>       /* MSR:POW cannot be set by any form of rfi */
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index 5ad39cacc92..4517b458b79 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -499,6 +499,45 @@ void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
>       *new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>   }
>   
> +void helper_attn(CPUPPCState *env)
> +{
> +    /* POWER attn is unprivileged when enabled by HID, otherwise illegal */
> +    if ((*env->check_attn)(env)) {
> +        powerpc_checkstop(env, "host executed attn");
> +    } else {
> +        raise_exception_err(env, POWERPC_EXCP_HV_EMU,
> +                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
> +    }
> +}
> +
> +void helper_scv(CPUPPCState *env, uint32_t lev)
> +{
> +    if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> +        raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
> +    } else {
> +        raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> +    }
> +}
> +
> +void helper_pminsn(CPUPPCState *env, uint32_t insn)
> +{
> +    CPUState *cs = env_cpu(env);
> +
> +    cs->halted = 1;
> +
> +    /* Condition for waking up at 0x100 */
> +    env->resume_as_sreset = (insn != PPC_PM_STOP) ||
> +        (env->spr[SPR_PSSCR] & PSSCR_EC);
> +
> +    /* HDECR is not to wake from PM state, it may have already fired */
> +    if (env->resume_as_sreset) {
> +        PowerPCCPU *cpu = env_archcpu(env);
> +        ppc_set_irq(cpu, PPC_INTERRUPT_HDECR, 0);
> +    }
> +
> +    ppc_maybe_interrupt(env);
> +}
> +
>   #endif /* TARGET_PPC64 */
>   
>   #endif /* !CONFIG_USER_ONLY */

[PATCH v2 15/15] target/ppc: Restrict various system helpers to TCG

[Philippe Mathieu-Daudé]

Move various TCG system helpers to tcg-excp_helper.c.

ppc_ldl_code(), raise_exception() and raise_exception_err()
are only used within this file, restrict their declaration
scope.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/cpu.h             |   3 -
 target/ppc/internal.h        |   2 -
 target/ppc/excp_helper.c     | 389 -----------------------------------
 target/ppc/tcg-excp_helper.c | 387 +++++++++++++++++++++++++++++++++-
 4 files changed, 384 insertions(+), 397 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 4ca27d6b389..35b56368eac 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2752,9 +2752,6 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, vaddr *pc,
 }
 #endif
 
-G_NORETURN void raise_exception(CPUPPCState *env, uint32_t exception);
-G_NORETURN void raise_exception_err(CPUPPCState *env, uint32_t exception,
-                                    uint32_t error_code);
 G_NORETURN void raise_exception_err_ra(CPUPPCState *env, uint32_t exception,
                                        uint32_t error_code, uintptr_t raddr);
 
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index b8997ba31db..aedc94d1a1e 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -268,8 +268,6 @@ static inline void pte_invalidate(target_ulong *pte0)
 #define PTE_PTEM_MASK 0x7FFFFFBF
 #define PTE_CHECK_MASK (TARGET_PAGE_MASK | 0x7B)
 
-uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr);
-
 #ifdef CONFIG_USER_ONLY
 void ppc_cpu_record_sigsegv(CPUState *cs, vaddr addr,
                             MMUAccessType access_type,
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 2deed155987..eedfa2d1de1 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -31,11 +31,6 @@
 
 #include "trace.h"
 
-#ifdef CONFIG_TCG
-#include "exec/helper-proto.h"
-#include "exec/cpu_ldst.h"
-#endif
-
 /*****************************************************************************/
 /* Exception processing */
 #ifndef CONFIG_USER_ONLY
@@ -2411,387 +2406,3 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 }
 
 #endif /* !CONFIG_USER_ONLY */
-
-#ifdef CONFIG_TCG
-
-#ifndef CONFIG_USER_ONLY
-void helper_store_msr(CPUPPCState *env, target_ulong val)
-{
-    uint32_t excp = hreg_store_msr(env, val, 0);
-
-    if (excp != 0) {
-        cpu_interrupt_exittb(env_cpu(env));
-        raise_exception(env, excp);
-    }
-}
-
-void helper_ppc_maybe_interrupt(CPUPPCState *env)
-{
-    ppc_maybe_interrupt(env);
-}
-
-static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
-{
-    /* MSR:POW cannot be set by any form of rfi */
-    msr &= ~(1ULL << MSR_POW);
-
-    /* MSR:TGPR cannot be set by any form of rfi */
-    if (env->flags & POWERPC_FLAG_TGPR) {
-        msr &= ~(1ULL << MSR_TGPR);
-    }
-
-#ifdef TARGET_PPC64
-    /* Switching to 32-bit ? Crop the nip */
-    if (!msr_is_64bit(env, msr)) {
-        nip = (uint32_t)nip;
-    }
-#else
-    nip = (uint32_t)nip;
-#endif
-    /* XXX: beware: this is false if VLE is supported */
-    env->nip = nip & ~((target_ulong)0x00000003);
-    hreg_store_msr(env, msr, 1);
-    trace_ppc_excp_rfi(env->nip, env->msr);
-    /*
-     * No need to raise an exception here, as rfi is always the last
-     * insn of a TB
-     */
-    cpu_interrupt_exittb(env_cpu(env));
-    /* Reset the reservation */
-    env->reserve_addr = -1;
-
-    /* Context synchronizing: check if TCG TLB needs flush */
-    check_tlb_flush(env, false);
-}
-
-void helper_rfi(CPUPPCState *env)
-{
-    do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xfffffffful);
-}
-
-#ifdef TARGET_PPC64
-void helper_rfid(CPUPPCState *env)
-{
-    /*
-     * The architecture defines a number of rules for which bits can
-     * change but in practice, we handle this in hreg_store_msr()
-     * which will be called by do_rfi(), so there is no need to filter
-     * here
-     */
-    do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1]);
-}
-
-void helper_rfscv(CPUPPCState *env)
-{
-    do_rfi(env, env->lr, env->ctr);
-}
-
-void helper_hrfid(CPUPPCState *env)
-{
-    do_rfi(env, env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
-}
-
-void helper_rfebb(CPUPPCState *env, target_ulong s)
-{
-    target_ulong msr = env->msr;
-
-    /*
-     * Handling of BESCR bits 32:33 according to PowerISA v3.1:
-     *
-     * "If BESCR 32:33 != 0b00 the instruction is treated as if
-     *  the instruction form were invalid."
-     */
-    if (env->spr[SPR_BESCR] & BESCR_INVALID) {
-        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
-    }
-
-    env->nip = env->spr[SPR_EBBRR];
-
-    /* Switching to 32-bit ? Crop the nip */
-    if (!msr_is_64bit(env, msr)) {
-        env->nip = (uint32_t)env->spr[SPR_EBBRR];
-    }
-
-    if (s) {
-        env->spr[SPR_BESCR] |= BESCR_GE;
-    } else {
-        env->spr[SPR_BESCR] &= ~BESCR_GE;
-    }
-}
-
-/*
- * Triggers or queues an 'ebb_excp' EBB exception. All checks
- * but FSCR, HFSCR and msr_pr must be done beforehand.
- *
- * PowerISA v3.1 isn't clear about whether an EBB should be
- * postponed or cancelled if the EBB facility is unavailable.
- * Our assumption here is that the EBB is cancelled if both
- * FSCR and HFSCR EBB facilities aren't available.
- */
-static void do_ebb(CPUPPCState *env, int ebb_excp)
-{
-    PowerPCCPU *cpu = env_archcpu(env);
-
-    /*
-     * FSCR_EBB and FSCR_IC_EBB are the same bits used with
-     * HFSCR.
-     */
-    helper_fscr_facility_check(env, FSCR_EBB, 0, FSCR_IC_EBB);
-    helper_hfscr_facility_check(env, FSCR_EBB, "EBB", FSCR_IC_EBB);
-
-    if (ebb_excp == POWERPC_EXCP_PERFM_EBB) {
-        env->spr[SPR_BESCR] |= BESCR_PMEO;
-    } else if (ebb_excp == POWERPC_EXCP_EXTERNAL_EBB) {
-        env->spr[SPR_BESCR] |= BESCR_EEO;
-    }
-
-    if (FIELD_EX64(env->msr, MSR, PR)) {
-        powerpc_excp(cpu, ebb_excp);
-    } else {
-        ppc_set_irq(cpu, PPC_INTERRUPT_EBB, 1);
-    }
-}
-
-void raise_ebb_perfm_exception(CPUPPCState *env)
-{
-    bool perfm_ebb_enabled = env->spr[SPR_POWER_MMCR0] & MMCR0_EBE &&
-                             env->spr[SPR_BESCR] & BESCR_PME &&
-                             env->spr[SPR_BESCR] & BESCR_GE;
-
-    if (!perfm_ebb_enabled) {
-        return;
-    }
-
-    do_ebb(env, POWERPC_EXCP_PERFM_EBB);
-}
-#endif /* TARGET_PPC64 */
-
-/*****************************************************************************/
-/* Embedded PowerPC specific helpers */
-void helper_40x_rfci(CPUPPCState *env)
-{
-    do_rfi(env, env->spr[SPR_40x_SRR2], env->spr[SPR_40x_SRR3]);
-}
-
-void helper_rfci(CPUPPCState *env)
-{
-    do_rfi(env, env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1]);
-}
-
-void helper_rfdi(CPUPPCState *env)
-{
-    /* FIXME: choose CSRR1 or DSRR1 based on cpu type */
-    do_rfi(env, env->spr[SPR_BOOKE_DSRR0], env->spr[SPR_BOOKE_DSRR1]);
-}
-
-void helper_rfmci(CPUPPCState *env)
-{
-    /* FIXME: choose CSRR1 or MCSRR1 based on cpu type */
-    do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
-}
-
-/* Embedded.Processor Control */
-static int dbell2irq(target_ulong rb)
-{
-    int msg = rb & DBELL_TYPE_MASK;
-    int irq = -1;
-
-    switch (msg) {
-    case DBELL_TYPE_DBELL:
-        irq = PPC_INTERRUPT_DOORBELL;
-        break;
-    case DBELL_TYPE_DBELL_CRIT:
-        irq = PPC_INTERRUPT_CDOORBELL;
-        break;
-    case DBELL_TYPE_G_DBELL:
-    case DBELL_TYPE_G_DBELL_CRIT:
-    case DBELL_TYPE_G_DBELL_MC:
-        /* XXX implement */
-    default:
-        break;
-    }
-
-    return irq;
-}
-
-void helper_msgclr(CPUPPCState *env, target_ulong rb)
-{
-    int irq = dbell2irq(rb);
-
-    if (irq < 0) {
-        return;
-    }
-
-    ppc_set_irq(env_archcpu(env), irq, 0);
-}
-
-void helper_msgsnd(target_ulong rb)
-{
-    int irq = dbell2irq(rb);
-    int pir = rb & DBELL_PIRTAG_MASK;
-    CPUState *cs;
-
-    if (irq < 0) {
-        return;
-    }
-
-    bql_lock();
-    CPU_FOREACH(cs) {
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-        CPUPPCState *cenv = &cpu->env;
-
-        if ((rb & DBELL_BRDCAST_MASK) || (cenv->spr[SPR_BOOKE_PIR] == pir)) {
-            ppc_set_irq(cpu, irq, 1);
-        }
-    }
-    bql_unlock();
-}
-
-/* Server Processor Control */
-
-static bool dbell_type_server(target_ulong rb)
-{
-    /*
-     * A Directed Hypervisor Doorbell message is sent only if the
-     * message type is 5. All other types are reserved and the
-     * instruction is a no-op
-     */
-    return (rb & DBELL_TYPE_MASK) == DBELL_TYPE_DBELL_SERVER;
-}
-
-static inline bool dbell_bcast_core(target_ulong rb)
-{
-    return (rb & DBELL_BRDCAST_MASK) == DBELL_BRDCAST_CORE;
-}
-
-static inline bool dbell_bcast_subproc(target_ulong rb)
-{
-    return (rb & DBELL_BRDCAST_MASK) == DBELL_BRDCAST_SUBPROC;
-}
-
-/*
- * Send an interrupt to a thread in the same core as env).
- */
-static void msgsnd_core_tir(CPUPPCState *env, uint32_t target_tir, int irq)
-{
-    PowerPCCPU *cpu = env_archcpu(env);
-    CPUState *cs = env_cpu(env);
-
-    if (ppc_cpu_lpar_single_threaded(cs)) {
-        if (target_tir == 0) {
-            ppc_set_irq(cpu, irq, 1);
-        }
-    } else {
-        CPUState *ccs;
-
-        /* Does iothread need to be locked for walking CPU list? */
-        bql_lock();
-        THREAD_SIBLING_FOREACH(cs, ccs) {
-            PowerPCCPU *ccpu = POWERPC_CPU(ccs);
-            if (target_tir == ppc_cpu_tir(ccpu)) {
-                ppc_set_irq(ccpu, irq, 1);
-                break;
-            }
-        }
-        bql_unlock();
-    }
-}
-
-void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
-{
-    if (!dbell_type_server(rb)) {
-        return;
-    }
-
-    ppc_set_irq(env_archcpu(env), PPC_INTERRUPT_HDOORBELL, 0);
-}
-
-void helper_book3s_msgsnd(CPUPPCState *env, target_ulong rb)
-{
-    int pir = rb & DBELL_PROCIDTAG_MASK;
-    bool brdcast = false;
-    CPUState *cs, *ccs;
-    PowerPCCPU *cpu;
-
-    if (!dbell_type_server(rb)) {
-        return;
-    }
-
-    /* POWER8 msgsnd is like msgsndp (targets a thread within core) */
-    if (!(env->insns_flags2 & PPC2_ISA300)) {
-        msgsnd_core_tir(env, rb & PPC_BITMASK(57, 63), PPC_INTERRUPT_HDOORBELL);
-        return;
-    }
-
-    /* POWER9 and later msgsnd is a global (targets any thread) */
-    cpu = ppc_get_vcpu_by_pir(pir);
-    if (!cpu) {
-        return;
-    }
-    cs = CPU(cpu);
-
-    if (dbell_bcast_core(rb) || (dbell_bcast_subproc(rb) &&
-                                 (env->flags & POWERPC_FLAG_SMT_1LPAR))) {
-        brdcast = true;
-    }
-
-    if (ppc_cpu_core_single_threaded(cs) || !brdcast) {
-        ppc_set_irq(cpu, PPC_INTERRUPT_HDOORBELL, 1);
-        return;
-    }
-
-    /*
-     * Why is bql needed for walking CPU list? Answer seems to be because ppc
-     * irq handling needs it, but ppc_set_irq takes the lock itself if needed,
-     * so could this be removed?
-     */
-    bql_lock();
-    THREAD_SIBLING_FOREACH(cs, ccs) {
-        ppc_set_irq(POWERPC_CPU(ccs), PPC_INTERRUPT_HDOORBELL, 1);
-    }
-    bql_unlock();
-}
-
-#ifdef TARGET_PPC64
-void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
-{
-    helper_hfscr_facility_check(env, HFSCR_MSGP, "msgclrp", HFSCR_IC_MSGP);
-
-    if (!dbell_type_server(rb)) {
-        return;
-    }
-
-    ppc_set_irq(env_archcpu(env), PPC_INTERRUPT_DOORBELL, 0);
-}
-
-/*
- * sends a message to another thread  on the same
- * multi-threaded processor
- */
-void helper_book3s_msgsndp(CPUPPCState *env, target_ulong rb)
-{
-    helper_hfscr_facility_check(env, HFSCR_MSGP, "msgsndp", HFSCR_IC_MSGP);
-
-    if (!dbell_type_server(rb)) {
-        return;
-    }
-
-    msgsnd_core_tir(env, rb & PPC_BITMASK(57, 63), PPC_INTERRUPT_DOORBELL);
-}
-#endif /* TARGET_PPC64 */
-
-/* Single-step tracing */
-void helper_book3s_trace(CPUPPCState *env, target_ulong prev_ip)
-{
-    uint32_t error_code = 0;
-    if (env->insns_flags2 & PPC2_ISA207S) {
-        /* Load/store reporting, SRR1[35, 36] and SDAR, are not implemented. */
-        env->spr[SPR_POWER_SIAR] = prev_ip;
-        error_code = PPC_BIT(33);
-    }
-    raise_exception_err(env, POWERPC_EXCP_TRACE, error_code);
-}
-
-#endif /* !CONFIG_USER_ONLY */
-#endif /* CONFIG_TCG */
diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
index 4517b458b79..348dd8c6bff 100644
--- a/target/ppc/tcg-excp_helper.c
+++ b/target/ppc/tcg-excp_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "qemu/log.h"
 #include "exec/cpu_ldst.h"
 #include "exec/exec-all.h"
@@ -55,13 +56,13 @@ void helper_raise_exception(CPUPPCState *env, uint32_t exception)
 
 #ifndef CONFIG_USER_ONLY
 
-void raise_exception_err(CPUPPCState *env, uint32_t exception,
+static G_NORETURN void raise_exception_err(CPUPPCState *env, uint32_t exception,
                                            uint32_t error_code)
 {
     raise_exception_err_ra(env, exception, error_code, 0);
 }
 
-void raise_exception(CPUPPCState *env, uint32_t exception)
+static G_NORETURN void raise_exception(CPUPPCState *env, uint32_t exception)
 {
     raise_exception_err_ra(env, exception, 0, 0);
 }
@@ -209,6 +210,8 @@ HELPER_HASH(HASHCHKP, env->spr[SPR_HASHPKEYR], false, PHIE)
 
 #ifndef CONFIG_USER_ONLY
 
+static uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr);
+
 void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
                                  MMUAccessType access_type,
                                  int mmu_idx, uintptr_t retaddr)
@@ -415,7 +418,7 @@ static inline bool insn_need_byteswap(CPUArchState *env)
     return !!(env->msr & ((target_ulong)1 << MSR_LE));
 }
 
-uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
+static uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
 {
     uint32_t insn = cpu_ldl_code(env, addr);
 
@@ -540,4 +543,382 @@ void helper_pminsn(CPUPPCState *env, uint32_t insn)
 
 #endif /* TARGET_PPC64 */
 
+void helper_store_msr(CPUPPCState *env, target_ulong val)
+{
+    uint32_t excp = hreg_store_msr(env, val, 0);
+
+    if (excp != 0) {
+        cpu_interrupt_exittb(env_cpu(env));
+        raise_exception(env, excp);
+    }
+}
+
+void helper_ppc_maybe_interrupt(CPUPPCState *env)
+{
+    ppc_maybe_interrupt(env);
+}
+
+static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
+{
+    /* MSR:POW cannot be set by any form of rfi */
+    msr &= ~(1ULL << MSR_POW);
+
+    /* MSR:TGPR cannot be set by any form of rfi */
+    if (env->flags & POWERPC_FLAG_TGPR) {
+        msr &= ~(1ULL << MSR_TGPR);
+    }
+
+#ifdef TARGET_PPC64
+    /* Switching to 32-bit ? Crop the nip */
+    if (!msr_is_64bit(env, msr)) {
+        nip = (uint32_t)nip;
+    }
+#else
+    nip = (uint32_t)nip;
+#endif
+    /* XXX: beware: this is false if VLE is supported */
+    env->nip = nip & ~((target_ulong)0x00000003);
+    hreg_store_msr(env, msr, 1);
+    trace_ppc_excp_rfi(env->nip, env->msr);
+    /*
+     * No need to raise an exception here, as rfi is always the last
+     * insn of a TB
+     */
+    cpu_interrupt_exittb(env_cpu(env));
+    /* Reset the reservation */
+    env->reserve_addr = -1;
+
+    /* Context synchronizing: check if TCG TLB needs flush */
+    check_tlb_flush(env, false);
+}
+
+void helper_rfi(CPUPPCState *env)
+{
+    do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xfffffffful);
+}
+
+#ifdef TARGET_PPC64
+void helper_rfid(CPUPPCState *env)
+{
+    /*
+     * The architecture defines a number of rules for which bits can
+     * change but in practice, we handle this in hreg_store_msr()
+     * which will be called by do_rfi(), so there is no need to filter
+     * here
+     */
+    do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1]);
+}
+
+void helper_rfscv(CPUPPCState *env)
+{
+    do_rfi(env, env->lr, env->ctr);
+}
+
+void helper_hrfid(CPUPPCState *env)
+{
+    do_rfi(env, env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
+}
+
+void helper_rfebb(CPUPPCState *env, target_ulong s)
+{
+    target_ulong msr = env->msr;
+
+    /*
+     * Handling of BESCR bits 32:33 according to PowerISA v3.1:
+     *
+     * "If BESCR 32:33 != 0b00 the instruction is treated as if
+     *  the instruction form were invalid."
+     */
+    if (env->spr[SPR_BESCR] & BESCR_INVALID) {
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
+    }
+
+    env->nip = env->spr[SPR_EBBRR];
+
+    /* Switching to 32-bit ? Crop the nip */
+    if (!msr_is_64bit(env, msr)) {
+        env->nip = (uint32_t)env->spr[SPR_EBBRR];
+    }
+
+    if (s) {
+        env->spr[SPR_BESCR] |= BESCR_GE;
+    } else {
+        env->spr[SPR_BESCR] &= ~BESCR_GE;
+    }
+}
+
+/*
+ * Triggers or queues an 'ebb_excp' EBB exception. All checks
+ * but FSCR, HFSCR and msr_pr must be done beforehand.
+ *
+ * PowerISA v3.1 isn't clear about whether an EBB should be
+ * postponed or cancelled if the EBB facility is unavailable.
+ * Our assumption here is that the EBB is cancelled if both
+ * FSCR and HFSCR EBB facilities aren't available.
+ */
+static void do_ebb(CPUPPCState *env, int ebb_excp)
+{
+    PowerPCCPU *cpu = env_archcpu(env);
+
+    /*
+     * FSCR_EBB and FSCR_IC_EBB are the same bits used with
+     * HFSCR.
+     */
+    helper_fscr_facility_check(env, FSCR_EBB, 0, FSCR_IC_EBB);
+    helper_hfscr_facility_check(env, FSCR_EBB, "EBB", FSCR_IC_EBB);
+
+    if (ebb_excp == POWERPC_EXCP_PERFM_EBB) {
+        env->spr[SPR_BESCR] |= BESCR_PMEO;
+    } else if (ebb_excp == POWERPC_EXCP_EXTERNAL_EBB) {
+        env->spr[SPR_BESCR] |= BESCR_EEO;
+    }
+
+    if (FIELD_EX64(env->msr, MSR, PR)) {
+        powerpc_excp(cpu, ebb_excp);
+    } else {
+        ppc_set_irq(cpu, PPC_INTERRUPT_EBB, 1);
+    }
+}
+
+void raise_ebb_perfm_exception(CPUPPCState *env)
+{
+    bool perfm_ebb_enabled = env->spr[SPR_POWER_MMCR0] & MMCR0_EBE &&
+                             env->spr[SPR_BESCR] & BESCR_PME &&
+                             env->spr[SPR_BESCR] & BESCR_GE;
+
+    if (!perfm_ebb_enabled) {
+        return;
+    }
+
+    do_ebb(env, POWERPC_EXCP_PERFM_EBB);
+}
+#endif /* TARGET_PPC64 */
+
+/*****************************************************************************/
+/* Embedded PowerPC specific helpers */
+void helper_40x_rfci(CPUPPCState *env)
+{
+    do_rfi(env, env->spr[SPR_40x_SRR2], env->spr[SPR_40x_SRR3]);
+}
+
+void helper_rfci(CPUPPCState *env)
+{
+    do_rfi(env, env->spr[SPR_BOOKE_CSRR0], env->spr[SPR_BOOKE_CSRR1]);
+}
+
+void helper_rfdi(CPUPPCState *env)
+{
+    /* FIXME: choose CSRR1 or DSRR1 based on cpu type */
+    do_rfi(env, env->spr[SPR_BOOKE_DSRR0], env->spr[SPR_BOOKE_DSRR1]);
+}
+
+void helper_rfmci(CPUPPCState *env)
+{
+    /* FIXME: choose CSRR1 or MCSRR1 based on cpu type */
+    do_rfi(env, env->spr[SPR_BOOKE_MCSRR0], env->spr[SPR_BOOKE_MCSRR1]);
+}
+
+/* Embedded.Processor Control */
+static int dbell2irq(target_ulong rb)
+{
+    int msg = rb & DBELL_TYPE_MASK;
+    int irq = -1;
+
+    switch (msg) {
+    case DBELL_TYPE_DBELL:
+        irq = PPC_INTERRUPT_DOORBELL;
+        break;
+    case DBELL_TYPE_DBELL_CRIT:
+        irq = PPC_INTERRUPT_CDOORBELL;
+        break;
+    case DBELL_TYPE_G_DBELL:
+    case DBELL_TYPE_G_DBELL_CRIT:
+    case DBELL_TYPE_G_DBELL_MC:
+        /* XXX implement */
+    default:
+        break;
+    }
+
+    return irq;
+}
+
+void helper_msgclr(CPUPPCState *env, target_ulong rb)
+{
+    int irq = dbell2irq(rb);
+
+    if (irq < 0) {
+        return;
+    }
+
+    ppc_set_irq(env_archcpu(env), irq, 0);
+}
+
+void helper_msgsnd(target_ulong rb)
+{
+    int irq = dbell2irq(rb);
+    int pir = rb & DBELL_PIRTAG_MASK;
+    CPUState *cs;
+
+    if (irq < 0) {
+        return;
+    }
+
+    bql_lock();
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        CPUPPCState *cenv = &cpu->env;
+
+        if ((rb & DBELL_BRDCAST_MASK) || (cenv->spr[SPR_BOOKE_PIR] == pir)) {
+            ppc_set_irq(cpu, irq, 1);
+        }
+    }
+    bql_unlock();
+}
+
+/* Server Processor Control */
+
+static bool dbell_type_server(target_ulong rb)
+{
+    /*
+     * A Directed Hypervisor Doorbell message is sent only if the
+     * message type is 5. All other types are reserved and the
+     * instruction is a no-op
+     */
+    return (rb & DBELL_TYPE_MASK) == DBELL_TYPE_DBELL_SERVER;
+}
+
+static inline bool dbell_bcast_core(target_ulong rb)
+{
+    return (rb & DBELL_BRDCAST_MASK) == DBELL_BRDCAST_CORE;
+}
+
+static inline bool dbell_bcast_subproc(target_ulong rb)
+{
+    return (rb & DBELL_BRDCAST_MASK) == DBELL_BRDCAST_SUBPROC;
+}
+
+/*
+ * Send an interrupt to a thread in the same core as env).
+ */
+static void msgsnd_core_tir(CPUPPCState *env, uint32_t target_tir, int irq)
+{
+    PowerPCCPU *cpu = env_archcpu(env);
+    CPUState *cs = env_cpu(env);
+
+    if (ppc_cpu_lpar_single_threaded(cs)) {
+        if (target_tir == 0) {
+            ppc_set_irq(cpu, irq, 1);
+        }
+    } else {
+        CPUState *ccs;
+
+        /* Does iothread need to be locked for walking CPU list? */
+        bql_lock();
+        THREAD_SIBLING_FOREACH(cs, ccs) {
+            PowerPCCPU *ccpu = POWERPC_CPU(ccs);
+            if (target_tir == ppc_cpu_tir(ccpu)) {
+                ppc_set_irq(ccpu, irq, 1);
+                break;
+            }
+        }
+        bql_unlock();
+    }
+}
+
+void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
+{
+    if (!dbell_type_server(rb)) {
+        return;
+    }
+
+    ppc_set_irq(env_archcpu(env), PPC_INTERRUPT_HDOORBELL, 0);
+}
+
+void helper_book3s_msgsnd(CPUPPCState *env, target_ulong rb)
+{
+    int pir = rb & DBELL_PROCIDTAG_MASK;
+    bool brdcast = false;
+    CPUState *cs, *ccs;
+    PowerPCCPU *cpu;
+
+    if (!dbell_type_server(rb)) {
+        return;
+    }
+
+    /* POWER8 msgsnd is like msgsndp (targets a thread within core) */
+    if (!(env->insns_flags2 & PPC2_ISA300)) {
+        msgsnd_core_tir(env, rb & PPC_BITMASK(57, 63), PPC_INTERRUPT_HDOORBELL);
+        return;
+    }
+
+    /* POWER9 and later msgsnd is a global (targets any thread) */
+    cpu = ppc_get_vcpu_by_pir(pir);
+    if (!cpu) {
+        return;
+    }
+    cs = CPU(cpu);
+
+    if (dbell_bcast_core(rb) || (dbell_bcast_subproc(rb) &&
+                                 (env->flags & POWERPC_FLAG_SMT_1LPAR))) {
+        brdcast = true;
+    }
+
+    if (ppc_cpu_core_single_threaded(cs) || !brdcast) {
+        ppc_set_irq(cpu, PPC_INTERRUPT_HDOORBELL, 1);
+        return;
+    }
+
+    /*
+     * Why is bql needed for walking CPU list? Answer seems to be because ppc
+     * irq handling needs it, but ppc_set_irq takes the lock itself if needed,
+     * so could this be removed?
+     */
+    bql_lock();
+    THREAD_SIBLING_FOREACH(cs, ccs) {
+        ppc_set_irq(POWERPC_CPU(ccs), PPC_INTERRUPT_HDOORBELL, 1);
+    }
+    bql_unlock();
+}
+
+#ifdef TARGET_PPC64
+void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
+{
+    helper_hfscr_facility_check(env, HFSCR_MSGP, "msgclrp", HFSCR_IC_MSGP);
+
+    if (!dbell_type_server(rb)) {
+        return;
+    }
+
+    ppc_set_irq(env_archcpu(env), PPC_INTERRUPT_DOORBELL, 0);
+}
+
+/*
+ * sends a message to another thread  on the same
+ * multi-threaded processor
+ */
+void helper_book3s_msgsndp(CPUPPCState *env, target_ulong rb)
+{
+    helper_hfscr_facility_check(env, HFSCR_MSGP, "msgsndp", HFSCR_IC_MSGP);
+
+    if (!dbell_type_server(rb)) {
+        return;
+    }
+
+    msgsnd_core_tir(env, rb & PPC_BITMASK(57, 63), PPC_INTERRUPT_DOORBELL);
+}
+#endif /* TARGET_PPC64 */
+
+/* Single-step tracing */
+void helper_book3s_trace(CPUPPCState *env, target_ulong prev_ip)
+{
+    uint32_t error_code = 0;
+    if (env->insns_flags2 & PPC2_ISA207S) {
+        /* Load/store reporting, SRR1[35, 36] and SDAR, are not implemented. */
+        env->spr[SPR_POWER_SIAR] = prev_ip;
+        error_code = PPC_BIT(33);
+    }
+    raise_exception_err(env, POWERPC_EXCP_TRACE, error_code);
+}
+
 #endif /* !CONFIG_USER_ONLY */
-- 
2.47.1

Re: [PATCH v2 00/15] target/ppc: Move TCG code from excp_helper.c to tcg-excp_helper.c

[Nicholas Piggin]

On Mon Jan 27, 2025 at 8:26 PM AEST, Philippe Mathieu-Daudé wrote:
> Since v1:
> - Keep ppc_tcg_hv_emu() within TARGET_PPC64 (patch #10)
>
> Hi,
>
> This series is a simply a cleanup restricting TCG specific
> exception-related code to TCG, by moving code to a new unit
> named 'tcg-excp_helper.c'.
>
> I ended doing it as a preliminary cleanup for the "Extract
> TCG state from CPUState".
>
> Diffstat shows 1K lines moved, but the patches are trivial
> to review using 'git-diff --color-moved=dimmed-zebra' option.
>
> Branch published as:
>  https://gitlab.com/philmd/qemu/-/tree/ppc_excp_extract_tcg
>
> Regards,
>
> Phil.

I pulled this in and just added some minor changelog and comment
and subject line tweaks noticed in review.

Patch 2 was already merged, patch 10 I left out because it had a
few possible issues (but it generally looks good if you plan to
resubmit it some time).

Thanks very much, it's a nice cleanup. Sorry I have been slow
with things...

Thanks,
Nick

>
> Philippe Mathieu-Daudé (15):
>   hw/ppc/spapr: Restrict CONFER hypercall to TCG
>   hw/ppc/spapr: Restrict part of PAGE_INIT hypercall to TCG
>   target/ppc: Make ppc_ldl_code() declaration public
>   target/ppc: Move TCG specific exception handlers to tcg-excp_helper.c
>   target/ppc: Move ppc_ldl_code() to tcg-excp_helper.c
>   target/ppc: Ensure powerpc_checkstop() is only called under TCG
>   target/ppc: Restrict powerpc_checkstop() to TCG
>   target/ppc: Remove raise_exception_ra()
>   target/ppc: Restrict exception helpers to TCG
>   target/ppc: Restrict ppc_tcg_hv_emu() to TCG
>   target/ppc: Restrict various common helpers to TCG
>   target/ppc: Fix style in excp_helper.c
>   target/ppc: Make powerpc_excp() prototype public
>   target/ppc: Restrict ATTN / SCV / PMINSN helpers to TCG
>   target/ppc: Restrict various system helpers to TCG
>
>  target/ppc/cpu.h             |   5 -
>  target/ppc/internal.h        |  11 +-
>  hw/ppc/spapr_hcall.c         |   6 +-
>  target/ppc/excp_helper.c     | 943 +----------------------------------
>  target/ppc/tcg-excp_helper.c | 924 ++++++++++++++++++++++++++++++++++
>  target/ppc/meson.build       |   1 +
>  6 files changed, 955 insertions(+), 935 deletions(-)
>  create mode 100644 target/ppc/tcg-excp_helper.c

Re: [PATCH v2 00/15] target/ppc: Move TCG code from excp_helper.c to tcg-excp_helper.c

[Philippe Mathieu-Daudé]

On 11/3/25 07:22, Nicholas Piggin wrote:
> On Mon Jan 27, 2025 at 8:26 PM AEST, Philippe Mathieu-Daudé wrote:

>> This series is a simply a cleanup restricting TCG specific
>> exception-related code to TCG, by moving code to a new unit
>> named 'tcg-excp_helper.c'.


> I pulled this in and just added some minor changelog and comment
> and subject line tweaks noticed in review.
> 
> Patch 2 was already merged, patch 10 I left out because it had a
> few possible issues (but it generally looks good if you plan to
> resubmit it some time).

Thank you Nick, I will!

> 
> Thanks very much, it's a nice cleanup. Sorry I have been slow
> with things...
> 
> Thanks,
> Nick