| Message ID | 20230627134644.260663-5-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Catch invalid real address accesses | expand |
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),
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
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~
On 6/27/23 15:46, Nicholas Piggin wrote:
> +DEF_HELPER_1(attn, void, env)
s/void/noreturn/
r~
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
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),
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(+)