[PATCH v2 0/4] target/ppc: Catch invalid real address accesses

[PATCH v2 0/4] target/ppc: Catch invalid real address accesses

[Nicholas Piggin]

Since v1 I squashed a patch (and wrapper function) out, removed 970
to POWER8 invalid real address checkstop, added attn implementation to
demonstrate sharing of checkstop, and made some fixes to checkstop
mechanism so it stops on the instruction causing it.

Thanks,
Nick


BALATON Zoltan (1):
  target/ppc: Move common check in machine check handlers to a function

Nicholas Piggin (3):
  target/ppc: Machine check on invalid real address access on POWER9/10
  target/ppc: Make checkstop actually stop the system
  target/ppc: Implement attn instruction on BookS 64-bit processors

 target/ppc/cpu.h         |   2 +
 target/ppc/cpu_init.c    |   1 +
 target/ppc/excp_helper.c | 181 ++++++++++++++++++++++-----------------
 target/ppc/helper.h      |   2 +
 target/ppc/internal.h    |   5 ++
 target/ppc/translate.c   |   7 ++
 6 files changed, 121 insertions(+), 77 deletions(-)

-- 
2.40.1

[PATCH v2 1/4] target/ppc: Machine check on invalid real address access on POWER9/10

[Nicholas Piggin]

ppc currently silently accepts invalid real address access. Catch
these and turn them into machine checks on POWER9/10 machines.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v1:
- Only implement this for POWER9/10. Seems like previous IBM processors
  may not catch this after all, trying to get info.

 target/ppc/cpu_init.c    |  1 +
 target/ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++
 target/ppc/internal.h    |  5 ++++
 3 files changed, 55 insertions(+)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index aeff71d063..a97fb7fe10 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7295,6 +7295,7 @@ static const struct TCGCPUOps ppc_tcg_ops = {
   .cpu_exec_enter = ppc_cpu_exec_enter,
   .cpu_exec_exit = ppc_cpu_exec_exit,
   .do_unaligned_access = ppc_cpu_do_unaligned_access,
+  .do_transaction_failed = ppc_cpu_do_transaction_failed,
 #endif /* !CONFIG_USER_ONLY */
 };
 #endif /* CONFIG_TCG */
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 2158390e27..13318fbbb9 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1493,7 +1493,9 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
+        msr |= env->error_code;
         break;
+
     case POWERPC_EXCP_DSI:       /* Data storage exception                   */
         trace_ppc_excp_dsi(env->spr[SPR_DSISR], env->spr[SPR_DAR]);
         break;
@@ -3253,5 +3255,52 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
     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 = cs->env_ptr;
+
+    switch (env->excp_model) {
+#if defined(TARGET_PPC64)
+    case POWERPC_EXCP_POWER9:
+    case POWERPC_EXCP_POWER10:
+        /*
+         * Machine check codes can be found in 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 */
+
+            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);
+}
 #endif /* CONFIG_TCG */
 #endif /* !CONFIG_USER_ONLY */
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 901bae6d39..57acb3212c 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -296,6 +296,11 @@ bool ppc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
 G_NORETURN void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                             MMUAccessType access_type, int mmu_idx,
                                             uintptr_t retaddr);
+void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                   vaddr addr, unsigned size,
+                                   MMUAccessType access_type,
+                                   int mmu_idx, MemTxAttrs attrs,
+                                   MemTxResult response, uintptr_t retaddr);
 #endif
 
 FIELD(GER_MSK, XMSK, 0, 4)
-- 
2.40.1

[PATCH v2 2/4] target/ppc: Move common check in machine check handlers to a function

[Nicholas Piggin]

From: BALATON Zoltan <balaton@eik.bme.hu>

All powerpc exception handlers share some code when handling machine
check exceptions. Move this to a common function.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
[np: adapted to make checkstop generally usable]
Signed-off-by-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v1:
- Adjusted to avoid additional trivial wrapper function when
  making checkstop usable by other callers.

 target/ppc/excp_helper.c | 98 +++++++++-------------------------------
 1 file changed, 21 insertions(+), 77 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 13318fbbb9..5beda973ce 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -186,6 +186,21 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
              env->error_code);
 }
 
+static void powerpc_checkstop(CPUPPCState *env)
+{
+    CPUState *cs = env_cpu(env);
+
+    /* Machine check exception is not enabled. Enter checkstop state. */
+    fprintf(stderr, "Machine check while not allowed. "
+            "Entering checkstop state\n");
+    if (qemu_log_separate()) {
+        qemu_log("Machine check while not allowed. "
+                 "Entering checkstop state\n");
+    }
+    cs->halted = 1;
+    cpu_interrupt_exittb(cs);
+}
+
 #if defined(TARGET_PPC64)
 static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
                                 target_ulong *msr)
@@ -468,20 +483,8 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
         break;
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            /*
-             * Machine check exception is not enabled.  Enter
-             * checkstop state.
-             */
-            fprintf(stderr, "Machine check while not allowed. "
-                    "Entering checkstop state\n");
-            if (qemu_log_separate()) {
-                qemu_log("Machine check while not allowed. "
-                        "Entering checkstop state\n");
-            }
-            cs->halted = 1;
-            cpu_interrupt_exittb(cs);
+            powerpc_checkstop(env);
         }
-
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -599,20 +602,8 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
         break;
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            /*
-             * Machine check exception is not enabled.  Enter
-             * checkstop state.
-             */
-            fprintf(stderr, "Machine check while not allowed. "
-                    "Entering checkstop state\n");
-            if (qemu_log_separate()) {
-                qemu_log("Machine check while not allowed. "
-                        "Entering checkstop state\n");
-            }
-            cs->halted = 1;
-            cpu_interrupt_exittb(cs);
+            powerpc_checkstop(env);
         }
-
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -772,20 +763,8 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            /*
-             * Machine check exception is not enabled.  Enter
-             * checkstop state.
-             */
-            fprintf(stderr, "Machine check while not allowed. "
-                    "Entering checkstop state\n");
-            if (qemu_log_separate()) {
-                qemu_log("Machine check while not allowed. "
-                        "Entering checkstop state\n");
-            }
-            cs->halted = 1;
-            cpu_interrupt_exittb(cs);
+            powerpc_checkstop(env);
         }
-
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -957,20 +936,8 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            /*
-             * Machine check exception is not enabled.  Enter
-             * checkstop state.
-             */
-            fprintf(stderr, "Machine check while not allowed. "
-                    "Entering checkstop state\n");
-            if (qemu_log_separate()) {
-                qemu_log("Machine check while not allowed. "
-                        "Entering checkstop state\n");
-            }
-            cs->halted = 1;
-            cpu_interrupt_exittb(cs);
+            powerpc_checkstop(env);
         }
-
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -1152,20 +1119,8 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
         break;
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            /*
-             * Machine check exception is not enabled.  Enter
-             * checkstop state.
-             */
-            fprintf(stderr, "Machine check while not allowed. "
-                    "Entering checkstop state\n");
-            if (qemu_log_separate()) {
-                qemu_log("Machine check while not allowed. "
-                        "Entering checkstop state\n");
-            }
-            cs->halted = 1;
-            cpu_interrupt_exittb(cs);
+            powerpc_checkstop(env);
         }
-
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -1469,18 +1424,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            /*
-             * Machine check exception is not enabled.  Enter
-             * checkstop state.
-             */
-            fprintf(stderr, "Machine check while not allowed. "
-                    "Entering checkstop state\n");
-            if (qemu_log_separate()) {
-                qemu_log("Machine check while not allowed. "
-                        "Entering checkstop state\n");
-            }
-            cs->halted = 1;
-            cpu_interrupt_exittb(cs);
+            powerpc_checkstop(env);
         }
         if (env->msr_mask & MSR_HVB) {
             /*
-- 
2.40.1

[PATCH v2 3/4] target/ppc: Make checkstop actually stop the system

[Nicholas Piggin]

checkstop state does not halt the system, interrupts continue to be
serviced, and other CPUs run.

Stop the machine with vm_stop(), and print a register dump too.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v1:
- Fix loop exit so it stops on the attn instruction, rather than
  after it.

 target/ppc/excp_helper.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 5beda973ce..28d8a9b212 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 "sysemu/runstate.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "internal.h"
@@ -186,19 +187,24 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
              env->error_code);
 }
 
-static void powerpc_checkstop(CPUPPCState *env)
+static void powerpc_checkstop(CPUPPCState *env, const char *reason)
 {
     CPUState *cs = env_cpu(env);
 
-    /* Machine check exception is not enabled. Enter checkstop state. */
-    fprintf(stderr, "Machine check while not allowed. "
-            "Entering checkstop state\n");
+    vm_stop(RUN_STATE_GUEST_PANICKED);
+
+    fprintf(stderr, "Entering checkstop state: %s\n", reason);
+    cpu_dump_state(cs, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP);
     if (qemu_log_separate()) {
-        qemu_log("Machine check while not allowed. "
-                 "Entering checkstop state\n");
+        FILE *logfile = qemu_log_trylock();
+        if (logfile) {
+            fprintf(logfile, "Entering checkstop state: %s\n", reason);
+            cpu_dump_state(cs, logfile, CPU_DUMP_FPU | CPU_DUMP_CCOP);
+            qemu_log_unlock(logfile);
+        }
     }
-    cs->halted = 1;
-    cpu_interrupt_exittb(cs);
+
+    cpu_loop_exit_noexc(cs);
 }
 
 #if defined(TARGET_PPC64)
@@ -483,7 +489,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
         break;
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            powerpc_checkstop(env);
+            powerpc_checkstop(env, "machine check with MSR[ME]=0");
         }
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -602,7 +608,7 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
         break;
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            powerpc_checkstop(env);
+            powerpc_checkstop(env, "machine check with MSR[ME]=0");
         }
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -763,7 +769,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            powerpc_checkstop(env);
+            powerpc_checkstop(env, "machine check with MSR[ME]=0");
         }
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -936,7 +942,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            powerpc_checkstop(env);
+            powerpc_checkstop(env, "machine check with MSR[ME]=0");
         }
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -1119,7 +1125,7 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
         break;
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            powerpc_checkstop(env);
+            powerpc_checkstop(env, "machine check with MSR[ME]=0");
         }
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -1424,7 +1430,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-            powerpc_checkstop(env);
+            powerpc_checkstop(env, "machine check with MSR[ME]=0");
         }
         if (env->msr_mask & MSR_HVB) {
             /*
-- 
2.40.1

Re: [PATCH v2 3/4] target/ppc: Make checkstop actually stop the system

[BALATON Zoltan]

On Tue, 27 Jun 2023, Nicholas Piggin wrote:
> checkstop state does not halt the system, interrupts continue to be
> serviced, and other CPUs run.
>
> Stop the machine with vm_stop(), and print a register dump too.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Since v1:
> - Fix loop exit so it stops on the attn instruction, rather than
>  after it.
>
> target/ppc/excp_helper.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 5beda973ce..28d8a9b212 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 "sysemu/runstate.h"
> #include "cpu.h"
> #include "exec/exec-all.h"
> #include "internal.h"
> @@ -186,19 +187,24 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
>              env->error_code);
> }
>
> -static void powerpc_checkstop(CPUPPCState *env)
> +static void powerpc_checkstop(CPUPPCState *env, const char *reason)
> {
>     CPUState *cs = env_cpu(env);
>
> -    /* Machine check exception is not enabled. Enter checkstop state. */
> -    fprintf(stderr, "Machine check while not allowed. "
> -            "Entering checkstop state\n");
> +    vm_stop(RUN_STATE_GUEST_PANICKED);
> +
> +    fprintf(stderr, "Entering checkstop state: %s\n", reason);
> +    cpu_dump_state(cs, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP);
>     if (qemu_log_separate()) {
> -        qemu_log("Machine check while not allowed. "
> -                 "Entering checkstop state\n");
> +        FILE *logfile = qemu_log_trylock();
> +        if (logfile) {
> +            fprintf(logfile, "Entering checkstop state: %s\n", reason);

I don't think you should have fprintfs here. Is this remnants of debug 
code left here by mistake? The fprintf that was there before may also need 
to be converted to some qemI_log or error_report but I did not know what 
these are for and did not address that. But if you want to add more then 
it may need to be solved first.

> +            cpu_dump_state(cs, logfile, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> +            qemu_log_unlock(logfile);
> +        }
>     }
> -    cs->halted = 1;
> -    cpu_interrupt_exittb(cs);
> +

Excess blank line?

> +    cpu_loop_exit_noexc(cs);
> }
>
> #if defined(TARGET_PPC64)
> @@ -483,7 +489,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>         break;
>     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
>         if (!FIELD_EX64(env->msr, MSR, ME)) {
> -            powerpc_checkstop(env);
> +            powerpc_checkstop(env, "machine check with MSR[ME]=0");

If the message is always the same why pass it from here If the only other 
option not used yet would be MSR[ME]=1 then that could also be checked in 
the func so no need to pass the message. So is there any other possible 
reason here?

Regards,
BALATON Zoltan

>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -602,7 +608,7 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
>         break;
>     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
>         if (!FIELD_EX64(env->msr, MSR, ME)) {
> -            powerpc_checkstop(env);
> +            powerpc_checkstop(env, "machine check with MSR[ME]=0");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -763,7 +769,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>     switch (excp) {
>     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
>         if (!FIELD_EX64(env->msr, MSR, ME)) {
> -            powerpc_checkstop(env);
> +            powerpc_checkstop(env, "machine check with MSR[ME]=0");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -936,7 +942,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>     switch (excp) {
>     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
>         if (!FIELD_EX64(env->msr, MSR, ME)) {
> -            powerpc_checkstop(env);
> +            powerpc_checkstop(env, "machine check with MSR[ME]=0");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -1119,7 +1125,7 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
>         break;
>     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
>         if (!FIELD_EX64(env->msr, MSR, ME)) {
> -            powerpc_checkstop(env);
> +            powerpc_checkstop(env, "machine check with MSR[ME]=0");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -1424,7 +1430,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>     switch (excp) {
>     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
>         if (!FIELD_EX64(env->msr, MSR, ME)) {
> -            powerpc_checkstop(env);
> +            powerpc_checkstop(env, "machine check with MSR[ME]=0");
>         }
>         if (env->msr_mask & MSR_HVB) {
>             /*
>

Re: [PATCH v2 3/4] target/ppc: Make checkstop actually stop the system

[Nicholas Piggin]

On Wed Jun 28, 2023 at 3:38 AM AEST, BALATON Zoltan wrote:
> On Tue, 27 Jun 2023, Nicholas Piggin wrote:
> > checkstop state does not halt the system, interrupts continue to be
> > serviced, and other CPUs run.
> >
> > Stop the machine with vm_stop(), and print a register dump too.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > Since v1:
> > - Fix loop exit so it stops on the attn instruction, rather than
> >  after it.
> >
> > target/ppc/excp_helper.c | 34 ++++++++++++++++++++--------------
> > 1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 5beda973ce..28d8a9b212 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 "sysemu/runstate.h"
> > #include "cpu.h"
> > #include "exec/exec-all.h"
> > #include "internal.h"
> > @@ -186,19 +187,24 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
> >              env->error_code);
> > }
> >
> > -static void powerpc_checkstop(CPUPPCState *env)
> > +static void powerpc_checkstop(CPUPPCState *env, const char *reason)
> > {
> >     CPUState *cs = env_cpu(env);
> >
> > -    /* Machine check exception is not enabled. Enter checkstop state. */
> > -    fprintf(stderr, "Machine check while not allowed. "
> > -            "Entering checkstop state\n");
> > +    vm_stop(RUN_STATE_GUEST_PANICKED);
> > +
> > +    fprintf(stderr, "Entering checkstop state: %s\n", reason);
> > +    cpu_dump_state(cs, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> >     if (qemu_log_separate()) {
> > -        qemu_log("Machine check while not allowed. "
> > -                 "Entering checkstop state\n");
> > +        FILE *logfile = qemu_log_trylock();
> > +        if (logfile) {
> > +            fprintf(logfile, "Entering checkstop state: %s\n", reason);
>
> I don't think you should have fprintfs here. Is this remnants of debug 
> code left here by mistake? The fprintf that was there before may also need 
> to be converted to some qemI_log or error_report but I did not know what 
> these are for and did not address that. But if you want to add more then 
> it may need to be solved first.

I just followed existing fprintf use. Changing that should be separate
patch indeed.

> > +            cpu_dump_state(cs, logfile, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> > +            qemu_log_unlock(logfile);
> > +        }
> >     }
> > -    cs->halted = 1;
> > -    cpu_interrupt_exittb(cs);
> > +
>
> Excess blank line?

No, it separates the logging block from function.

>
> > +    cpu_loop_exit_noexc(cs);
> > }
> >
> > #if defined(TARGET_PPC64)
> > @@ -483,7 +489,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
> >         break;
> >     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
> >         if (!FIELD_EX64(env->msr, MSR, ME)) {
> > -            powerpc_checkstop(env);
> > +            powerpc_checkstop(env, "machine check with MSR[ME]=0");
>
> If the message is always the same why pass it from here If the only other 
> option not used yet would be MSR[ME]=1 then that could also be checked in 
> the func so no need to pass the message. So is there any other possible 
> reason here?

To make the checkstop function more general (e.g., used by the next patch).

Thanks,
Nick

Re: [PATCH v2 3/4] target/ppc: Make checkstop actually stop the system

[BALATON Zoltan]

On Wed, 28 Jun 2023, Nicholas Piggin wrote:
> On Wed Jun 28, 2023 at 3:38 AM AEST, BALATON Zoltan wrote:
>> On Tue, 27 Jun 2023, Nicholas Piggin wrote:
>>> checkstop state does not halt the system, interrupts continue to be
>>> serviced, and other CPUs run.
>>>
>>> Stop the machine with vm_stop(), and print a register dump too.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> Since v1:
>>> - Fix loop exit so it stops on the attn instruction, rather than
>>>  after it.
>>>
>>> target/ppc/excp_helper.c | 34 ++++++++++++++++++++--------------
>>> 1 file changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>> index 5beda973ce..28d8a9b212 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 "sysemu/runstate.h"
>>> #include "cpu.h"
>>> #include "exec/exec-all.h"
>>> #include "internal.h"
>>> @@ -186,19 +187,24 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
>>>              env->error_code);
>>> }
>>>
>>> -static void powerpc_checkstop(CPUPPCState *env)
>>> +static void powerpc_checkstop(CPUPPCState *env, const char *reason)
>>> {
>>>     CPUState *cs = env_cpu(env);
>>>
>>> -    /* Machine check exception is not enabled. Enter checkstop state. */
>>> -    fprintf(stderr, "Machine check while not allowed. "
>>> -            "Entering checkstop state\n");
>>> +    vm_stop(RUN_STATE_GUEST_PANICKED);
>>> +
>>> +    fprintf(stderr, "Entering checkstop state: %s\n", reason);
>>> +    cpu_dump_state(cs, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP);
>>>     if (qemu_log_separate()) {
>>> -        qemu_log("Machine check while not allowed. "
>>> -                 "Entering checkstop state\n");
>>> +        FILE *logfile = qemu_log_trylock();
>>> +        if (logfile) {
>>> +            fprintf(logfile, "Entering checkstop state: %s\n", reason);
>>
>> I don't think you should have fprintfs here. Is this remnants of debug
>> code left here by mistake? The fprintf that was there before may also need
>> to be converted to some qemI_log or error_report but I did not know what
>> these are for and did not address that. But if you want to add more then
>> it may need to be solved first.
>
> I just followed existing fprintf use. Changing that should be separate
> patch indeed.

As this is the only printf here maybe cleaning it up should come before 
adding more.

>>> +            cpu_dump_state(cs, logfile, CPU_DUMP_FPU | CPU_DUMP_CCOP);
>>> +            qemu_log_unlock(logfile);
>>> +        }
>>>     }
>>> -    cs->halted = 1;
>>> -    cpu_interrupt_exittb(cs);
>>> +
>>
>> Excess blank line?
>
> No, it separates the logging block from function.
>
>>
>>> +    cpu_loop_exit_noexc(cs);
>>> }
>>>
>>> #if defined(TARGET_PPC64)
>>> @@ -483,7 +489,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>>>         break;
>>>     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
>>>         if (!FIELD_EX64(env->msr, MSR, ME)) {
>>> -            powerpc_checkstop(env);
>>> +            powerpc_checkstop(env, "machine check with MSR[ME]=0");
>>
>> If the message is always the same why pass it from here If the only other
>> option not used yet would be MSR[ME]=1 then that could also be checked in
>> the func so no need to pass the message. So is there any other possible
>> reason here?
>
> To make the checkstop function more general (e.g., used by the next patch).

OK, I did look where it's needed but missed that line in the next patch. 
That's not practical to check in the checkstop function then.

Regards,
BALATON Zoltan

Re: [PATCH v2 3/4] target/ppc: Make checkstop actually stop the system

[Richard Henderson]

On 6/27/23 15:46, Nicholas Piggin wrote:
> +    vm_stop(RUN_STATE_GUEST_PANICKED);

Calling qemu_system_guest_panicked(NULL) seems to be more correct.

Though I'm not really sure the difference from cpu_abort(), which would also care for 
dumping cpu state.


r~

Re: [PATCH v2 3/4] target/ppc: Make checkstop actually stop the system

[Nicholas Piggin]

On Wed Jun 28, 2023 at 7:33 PM AEST, Richard Henderson wrote:
> On 6/27/23 15:46, Nicholas Piggin wrote:
> > +    vm_stop(RUN_STATE_GUEST_PANICKED);
>
> Calling qemu_system_guest_panicked(NULL) seems to be more correct.

I'll have a look.

> Though I'm not really sure the difference from cpu_abort(), which would also care for 
> dumping cpu state.

cpu_abort() just kills qemu, so you can't inspect anything. This way
e.g., gdb server and monitor stay up. Seems like you can even re-start
it(?). This is close to what we want short of models for BMC and host
debug logic.

Thanks,
Nick

[PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors

[Nicholas Piggin]

attn is an implementation-specific instruction that on POWER (and G5/
970) can be enabled with a HID bit (disabled = illegal), and executing
it causes the host processor to stop and the service processor to be
notified. Generally used for debugging.

Implement attn and make it checkstop the system, which should be good
enough for QEMU debugging.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v1:
- New patch that also uses checkstop function. Works with skiboot.

 target/ppc/cpu.h         |  2 ++
 target/ppc/excp_helper.c | 28 ++++++++++++++++++++++++++++
 target/ppc/helper.h      |  2 ++
 target/ppc/translate.c   |  7 +++++++
 4 files changed, 39 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 94497aa115..f6e93dec5f 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2116,6 +2116,8 @@ void ppc_compat_add_property(Object *obj, const char *name,
 #define HID0_NAP            (1 << 22)           /* pre-2.06 */
 #define HID0_HILE           PPC_BIT(19) /* POWER8 */
 #define HID0_POWER9_HILE    PPC_BIT(4)
+#define HID0_ENABLE_ATTN    PPC_BIT(31) /* POWER8 */
+#define HID0_POWER9_ENABLE_ATTN PPC_BIT(3)
 
 /*****************************************************************************/
 /* PowerPC Instructions types definitions                                    */
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 28d8a9b212..f46fdd2ee6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -208,6 +208,34 @@ static void powerpc_checkstop(CPUPPCState *env, const char *reason)
 }
 
 #if defined(TARGET_PPC64)
+void helper_attn(CPUPPCState *env)
+{
+    CPUState *cs = env_cpu(env);
+    target_ulong hid0_attn = 0;
+
+    switch (env->excp_model) {
+    case POWERPC_EXCP_970:
+    case POWERPC_EXCP_POWER7:
+    case POWERPC_EXCP_POWER8:
+        hid0_attn = HID0_ENABLE_ATTN;
+        break;
+    case POWERPC_EXCP_POWER9:
+    case POWERPC_EXCP_POWER10:
+        hid0_attn = HID0_POWER9_ENABLE_ATTN;
+        break;
+    default:
+        break;
+    }
+
+    if (env->spr[SPR_HID0] & hid0_attn) {
+        powerpc_checkstop(env, "host executed attn");
+        cpu_loop_exit_noexc(cs);
+    } else {
+        raise_exception_err(env, POWERPC_EXCP_HV_EMU,
+                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
+    }
+}
+
 static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
                                 target_ulong *msr)
 {
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index fda40b8a60..50bb105c08 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -812,3 +812,5 @@ DEF_HELPER_4(DSCLIQ, void, env, fprp, fprp, i32)
 
 DEF_HELPER_1(tbegin, void, env)
 DEF_HELPER_FLAGS_1(fixup_thrm, TCG_CALL_NO_RWG, void, env)
+
+DEF_HELPER_1(attn, void, env)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 372ee600b2..4e9e606d77 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6382,6 +6382,12 @@ static void gen_dform3D(DisasContext *ctx)
 }
 
 #if defined(TARGET_PPC64)
+/* attn */
+static void gen_attn(DisasContext *ctx)
+{
+    gen_helper_attn(cpu_env);
+}
+
 /* brd */
 static void gen_brd(DisasContext *ctx)
 {
@@ -6413,6 +6419,7 @@ static void gen_brh(DisasContext *ctx)
 
 static opcode_t opcodes[] = {
 #if defined(TARGET_PPC64)
+GEN_HANDLER_E(attn, 0x00, 0x00, 0x08, 0xFFFFFDFF, PPC_NONE, PPC2_ISA207S),
 GEN_HANDLER_E(brd, 0x1F, 0x1B, 0x05, 0x0000F801, PPC_NONE, PPC2_ISA310),
 GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0x0000F801, PPC_NONE, PPC2_ISA310),
 GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0x0000F801, PPC_NONE, PPC2_ISA310),
-- 
2.40.1

Re: [PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> attn is an implementation-specific instruction that on POWER (and G5/
> 970) can be enabled with a HID bit (disabled = illegal), and executing
> it causes the host processor to stop and the service processor to be
> notified. Generally used for debugging.
>
> Implement attn and make it checkstop the system, which should be good
> enough for QEMU debugging.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Since v1:
> - New patch that also uses checkstop function. Works with skiboot.
>
>  target/ppc/cpu.h         |  2 ++
>  target/ppc/excp_helper.c | 28 ++++++++++++++++++++++++++++
>  target/ppc/helper.h      |  2 ++
>  target/ppc/translate.c   |  7 +++++++
>  4 files changed, 39 insertions(+)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 94497aa115..f6e93dec5f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2116,6 +2116,8 @@ void ppc_compat_add_property(Object *obj, const char *name,
>  #define HID0_NAP            (1 << 22)           /* pre-2.06 */
>  #define HID0_HILE           PPC_BIT(19) /* POWER8 */
>  #define HID0_POWER9_HILE    PPC_BIT(4)
> +#define HID0_ENABLE_ATTN    PPC_BIT(31) /* POWER8 */
> +#define HID0_POWER9_ENABLE_ATTN PPC_BIT(3)
>  
>  /*****************************************************************************/
>  /* PowerPC Instructions types definitions                                    */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 28d8a9b212..f46fdd2ee6 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -208,6 +208,34 @@ static void powerpc_checkstop(CPUPPCState *env, const char *reason)
>  }
>  
>  #if defined(TARGET_PPC64)
> +void helper_attn(CPUPPCState *env)
> +{
> +    CPUState *cs = env_cpu(env);
> +    target_ulong hid0_attn = 0;
> +
> +    switch (env->excp_model) {
> +    case POWERPC_EXCP_970:
> +    case POWERPC_EXCP_POWER7:
> +    case POWERPC_EXCP_POWER8:
> +        hid0_attn = HID0_ENABLE_ATTN;
> +        break;
> +    case POWERPC_EXCP_POWER9:
> +    case POWERPC_EXCP_POWER10:
> +        hid0_attn = HID0_POWER9_ENABLE_ATTN;
> +        break;
> +    default:
> +        break;
> +    }

There's some precedent for checking HID bits using a cpu class
function. See pcc->check_pow, check_pow_hid0() and
check_pow_hid0_74xx(). I find it a bit nicer because the class carries
all the data so it's easier to move code around.

> +
> +    if (env->spr[SPR_HID0] & hid0_attn) {
> +        powerpc_checkstop(env, "host executed attn");
> +        cpu_loop_exit_noexc(cs);
> +    } else {
> +        raise_exception_err(env, POWERPC_EXCP_HV_EMU,
> +                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
> +    }
> +}
> +
>  static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
>                                  target_ulong *msr)
>  {
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index fda40b8a60..50bb105c08 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -812,3 +812,5 @@ DEF_HELPER_4(DSCLIQ, void, env, fprp, fprp, i32)
>  
>  DEF_HELPER_1(tbegin, void, env)
>  DEF_HELPER_FLAGS_1(fixup_thrm, TCG_CALL_NO_RWG, void, env)
> +
> +DEF_HELPER_1(attn, void, env)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 372ee600b2..4e9e606d77 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6382,6 +6382,12 @@ static void gen_dform3D(DisasContext *ctx)
>  }
>  
>  #if defined(TARGET_PPC64)
> +/* attn */
> +static void gen_attn(DisasContext *ctx)
> +{
> +    gen_helper_attn(cpu_env);

In another incarnation of this patch, Cédric had a check for the
privilege level and linux-user:

+static void gen_attn(DisasContext *ctx)
+{
+ #if defined(CONFIG_USER_ONLY)
+    GEN_PRIV;
+#else
+    CHK_SV;
+
+    gen_helper_attn(cpu_env, cpu_gpr[3]);
+    ctx->base.is_jmp = DISAS_NORETURN;
+#endif
+}

> +}
> +
>  /* brd */
>  static void gen_brd(DisasContext *ctx)
>  {
> @@ -6413,6 +6419,7 @@ static void gen_brh(DisasContext *ctx)
>  
>  static opcode_t opcodes[] = {
>  #if defined(TARGET_PPC64)
> +GEN_HANDLER_E(attn, 0x00, 0x00, 0x08, 0xFFFFFDFF, PPC_NONE,
>  PPC2_ISA207S),

Aren't you missing the 970 with this? Maybe worth a insns_flag2 flag
just for the attn?

>  GEN_HANDLER_E(brd, 0x1F, 0x1B, 0x05, 0x0000F801, PPC_NONE, PPC2_ISA310),
>  GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0x0000F801, PPC_NONE, PPC2_ISA310),
>  GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0x0000F801, PPC_NONE, PPC2_ISA310),

Re: [PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors

[Nicholas Piggin]

On Wed Jun 28, 2023 at 1:25 AM AEST, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > attn is an implementation-specific instruction that on POWER (and G5/
> > 970) can be enabled with a HID bit (disabled = illegal), and executing
> > it causes the host processor to stop and the service processor to be
> > notified. Generally used for debugging.
> >
> > Implement attn and make it checkstop the system, which should be good
> > enough for QEMU debugging.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > Since v1:
> > - New patch that also uses checkstop function. Works with skiboot.
> >
> >  target/ppc/cpu.h         |  2 ++
> >  target/ppc/excp_helper.c | 28 ++++++++++++++++++++++++++++
> >  target/ppc/helper.h      |  2 ++
> >  target/ppc/translate.c   |  7 +++++++
> >  4 files changed, 39 insertions(+)
> >
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 94497aa115..f6e93dec5f 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -2116,6 +2116,8 @@ void ppc_compat_add_property(Object *obj, const char *name,
> >  #define HID0_NAP            (1 << 22)           /* pre-2.06 */
> >  #define HID0_HILE           PPC_BIT(19) /* POWER8 */
> >  #define HID0_POWER9_HILE    PPC_BIT(4)
> > +#define HID0_ENABLE_ATTN    PPC_BIT(31) /* POWER8 */
> > +#define HID0_POWER9_ENABLE_ATTN PPC_BIT(3)
> >  
> >  /*****************************************************************************/
> >  /* PowerPC Instructions types definitions                                    */
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 28d8a9b212..f46fdd2ee6 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -208,6 +208,34 @@ static void powerpc_checkstop(CPUPPCState *env, const char *reason)
> >  }
> >  
> >  #if defined(TARGET_PPC64)
> > +void helper_attn(CPUPPCState *env)
> > +{
> > +    CPUState *cs = env_cpu(env);
> > +    target_ulong hid0_attn = 0;
> > +
> > +    switch (env->excp_model) {
> > +    case POWERPC_EXCP_970:
> > +    case POWERPC_EXCP_POWER7:
> > +    case POWERPC_EXCP_POWER8:
> > +        hid0_attn = HID0_ENABLE_ATTN;
> > +        break;
> > +    case POWERPC_EXCP_POWER9:
> > +    case POWERPC_EXCP_POWER10:
> > +        hid0_attn = HID0_POWER9_ENABLE_ATTN;
> > +        break;
> > +    default:
> > +        break;
> > +    }
>
> There's some precedent for checking HID bits using a cpu class
> function. See pcc->check_pow, check_pow_hid0() and
> check_pow_hid0_74xx(). I find it a bit nicer because the class carries
> all the data so it's easier to move code around.

Good suggestion, thanks.

> > +
> > +    if (env->spr[SPR_HID0] & hid0_attn) {
> > +        powerpc_checkstop(env, "host executed attn");
> > +        cpu_loop_exit_noexc(cs);
> > +    } else {
> > +        raise_exception_err(env, POWERPC_EXCP_HV_EMU,
> > +                            POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
> > +    }
> > +}
> > +
> >  static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
> >                                  target_ulong *msr)
> >  {
> > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > index fda40b8a60..50bb105c08 100644
> > --- a/target/ppc/helper.h
> > +++ b/target/ppc/helper.h
> > @@ -812,3 +812,5 @@ DEF_HELPER_4(DSCLIQ, void, env, fprp, fprp, i32)
> >  
> >  DEF_HELPER_1(tbegin, void, env)
> >  DEF_HELPER_FLAGS_1(fixup_thrm, TCG_CALL_NO_RWG, void, env)
> > +
> > +DEF_HELPER_1(attn, void, env)
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 372ee600b2..4e9e606d77 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -6382,6 +6382,12 @@ static void gen_dform3D(DisasContext *ctx)
> >  }
> >  
> >  #if defined(TARGET_PPC64)
> > +/* attn */
> > +static void gen_attn(DisasContext *ctx)
> > +{
> > +    gen_helper_attn(cpu_env);
>
> In another incarnation of this patch, Cédric had a check for the
> privilege level and linux-user:
>
> +static void gen_attn(DisasContext *ctx)
> +{
> + #if defined(CONFIG_USER_ONLY)
> +    GEN_PRIV;
> +#else
> +    CHK_SV;
> +
> +    gen_helper_attn(cpu_env, cpu_gpr[3]);
> +    ctx->base.is_jmp = DISAS_NORETURN;
> +#endif
> +}

User only could be checked... On priv, I thought that attn is
unprivileged (so long as it is enabled in HID). I could check
what hardware does.

>
> > +}
> > +
> >  /* brd */
> >  static void gen_brd(DisasContext *ctx)
> >  {
> > @@ -6413,6 +6419,7 @@ static void gen_brh(DisasContext *ctx)
> >  
> >  static opcode_t opcodes[] = {
> >  #if defined(TARGET_PPC64)
> > +GEN_HANDLER_E(attn, 0x00, 0x00, 0x08, 0xFFFFFDFF, PPC_NONE,
> >  PPC2_ISA207S),
>
> Aren't you missing the 970 with this? Maybe worth a insns_flag2 flag
> just for the attn?

Yes good catch, I started out just doing powernv but found 970 manual
and it has attn. Will update.

Thanks,
Nick

Re: [PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors

[Richard Henderson]

On 6/27/23 15:46, Nicholas Piggin wrote:
> +    if (env->spr[SPR_HID0] & hid0_attn) {
> +        powerpc_checkstop(env, "host executed attn");
> +        cpu_loop_exit_noexc(cs);

checkstop already does the cpu_loop_exit_noexc.



r~

Re: [PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors

[Richard Henderson]

On 6/27/23 15:46, Nicholas Piggin wrote:
> +DEF_HELPER_1(attn, void, env)

s/void/noreturn/


r~

Re: [PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors

[Nicholas Piggin]

On Wed Jun 28, 2023 at 7:38 PM AEST, Richard Henderson wrote:
> On 6/27/23 15:46, Nicholas Piggin wrote:
> > +DEF_HELPER_1(attn, void, env)
>
> s/void/noreturn/

Thank you x2, agree.

Thanks,
Nick