Re: [PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors
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
Re: [PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors
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
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
On Wed Jun 28, 2023 at 1:25 AM AEST, Fabiano Rosas wrote:
> Nicholas Piggin 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
> > ---
> > 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_HILEPPC_BIT(4)
> > +#define HID0_ENABLE_ATTNPPC_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, 0xFDFF, 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
Nicholas Piggin 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
> ---
> 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_HILEPPC_BIT(4)
> +#define HID0_ENABLE_ATTNPPC_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, 0xFDFF, 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, 0xF801, PPC_NONE, PPC2_ISA310),
> GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0xF801, PPC_NONE, PPC2_ISA310),
> GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0xF801, PPC_NONE, PPC2_ISA310),
[PATCH v2 4/4] target/ppc: Implement attn instruction on BookS 64-bit processors
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
---
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_HILEPPC_BIT(4)
+#define HID0_ENABLE_ATTNPPC_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, 0xFDFF, PPC_NONE, PPC2_ISA207S),
GEN_HANDLER_E(brd, 0x1F, 0x1B, 0x05, 0xF801, PPC_NONE, PPC2_ISA310),
GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0xF801, PPC_NONE, PPC2_ISA310),
GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0xF801, PPC_NONE, PPC2_ISA310),
--
2.40.1
