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