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