Re: [PATCH 19/29] target/ppc: Refactor sleep and its variants to use a common helper
Miles Glenn <[email protected]>
| Newsgroups | org.nongnu.qemu-devel |
|---|---|
| Organization | IBM |
| Message-ID | <[email protected]> |
Hi Chinmay, I was only suggesting that it might be good to add a comment to the commit message stating that this commit was preparatory to moving the sleep instructions to decodetree. No need to squash them. On Fri, 2026-08-21 at 12:57 +0530, Chinmay Rath wrote: > On 8/20/26 20:45, Miles Glenn wrote: > > Chinmay, > > > > This commit seems out of the scope of moving instructions to > > decodetree. Was this change necessary in order to move the > > instructions to decodetree? If so, it would be helpful to add that to > > the commit message. Otherwise, I would submit this separately from the > > decodetree commits. > Hi Glenn, > > It is common practice to merge helper functions in helper files and/or > translation functions in the translate.c or translate directory files > during instruction movement to decodetree, if they have common logic. > Speaking of which, ideally this commit should have been squashed along > with the previous commit where those insns were actually moved. The > author of the patch has instead created two separate patches. Though it > is not technically wrong, since neither of the patch are incomplete > functionally, let me know if you think this should be squashed with the > previous commit, will do that. > > PS: Please feel free to review other patches as well. It would be of > great help :) > > Regards, > Chinmay Hi Chinmay, I was only suggesting that it might be good to add a comment to the commit message stating that this commit was preparatory to moving the sleep instructions to decodetree. No need to squash the two commits. Thanks, Glenn > > Thanks, > > > > Glenn > > > > On Thu, 2026-08-20 at 19:30 +0530, Chinmay Rath wrote: > > > From: Ojaswin Mujoo <[email protected]> > > > > > > Refactor sleep, doze, nap, rvwinkle and stop to use a common helper > > > since they share majority of the code. Also refactor gen_hlper_pminsn() > > > to gen_helper_PMINSN() > > > > > > Signed-off-by: Ojaswin Mujoo <[email protected]> > > > Signed-off-by: Chinmay Rath <[email protected]> > > > --- > > > target/ppc/helper.h | 2 +- > > > target/ppc/tcg-excp_helper.c | 2 +- > > > .../ppc/translate/processor-ctrl-impl.c.inc | 99 +++---------------- > > > 3 files changed, 16 insertions(+), 87 deletions(-) > > > > > > diff --git a/target/ppc/helper.h b/target/ppc/helper.h > > > index cd2a0948a3..7e15676122 100644 > > > --- a/target/ppc/helper.h > > > +++ b/target/ppc/helper.h > > > @@ -18,7 +18,7 @@ DEF_HELPER_1(rfdi, void, env) > > > DEF_HELPER_1(rfmci, void, env) > > > #if defined(TARGET_PPC64) > > > DEF_HELPER_2(scv, noreturn, env, i32) > > > -DEF_HELPER_2(pminsn, void, env, i32) > > > +DEF_HELPER_2(PMINSN, void, env, i32) > > > DEF_HELPER_1(rfid, void, env) > > > DEF_HELPER_1(rfscv, void, env) > > > DEF_HELPER_1(hrfid, void, env) > > > diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c > > > index b04f07a637..c4ffa2dfbb 100644 > > > --- a/target/ppc/tcg-excp_helper.c > > > +++ b/target/ppc/tcg-excp_helper.c > > > @@ -459,7 +459,7 @@ void helper_scv(CPUPPCState *env, uint32_t lev) > > > } > > > } > > > > > > -void helper_pminsn(CPUPPCState *env, uint32_t insn) > > > +void helper_PMINSN(CPUPPCState *env, uint32_t insn) > > > { > > > CPUState *cs = env_cpu(env); > > > > > > diff --git a/target/ppc/translate/processor-ctrl-impl.c.inc b/target/ppc/translate/processor-ctrl-impl.c.inc > > > index bdbc195294..3b3ed3019a 100644 > > > --- a/target/ppc/translate/processor-ctrl-impl.c.inc > > > +++ b/target/ppc/translate/processor-ctrl-impl.c.inc > > > @@ -104,103 +104,32 @@ static bool trans_MSGSYNC(DisasContext *ctx, arg_MSGSYNC *a) > > > return true; > > > } > > > > > > -static bool do_doze(DisasContext *ctx, arg_DOZE *a) > > > -{ > > > - REQUIRE_64BIT(ctx); > > > - > > > -#if defined(CONFIG_USER_ONLY) > > > - gen_priv_opc(ctx); > > > -#else > > > - TCGv_i32 t; > > > - > > > - REQUIRE_HV(ctx); > > > - translator_io_start(&ctx->base); > > > - t = tcg_constant_i32(PPC_PM_DOZE); > > > - gen_helper_pminsn(tcg_env, t); > > > - /* Stop translation, as the CPU is supposed to sleep from now */ > > > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > > > -#endif /* defined(CONFIG_USER_ONLY) */ > > > - > > > - return true; > > > -} > > > -TRANS_FLAGS2(PM_ISA206, DOZE, do_doze); > > > - > > > -static bool do_nap(DisasContext *ctx, arg_NAP *a) > > > -{ > > > - REQUIRE_64BIT(ctx); > > > - > > > -#if defined(CONFIG_USER_ONLY) > > > - gen_priv_opc(ctx); > > > -#else > > > - TCGv_i32 t; > > > - > > > - REQUIRE_HV(ctx); > > > - translator_io_start(&ctx->base); > > > - t = tcg_constant_i32(PPC_PM_NAP); > > > - gen_helper_pminsn(tcg_env, t); > > > - /* Stop translation, as the CPU is supposed to sleep from now */ > > > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > > > -#endif /* defined(CONFIG_USER_ONLY) */ > > > - return true; > > > -} > > > -TRANS_FLAGS2(PM_ISA206, NAP, do_nap); > > > - > > > -static bool do_sleep(DisasContext *ctx, arg_SLEEP *a) > > > -{ > > > - REQUIRE_64BIT(ctx); > > > - > > > -#if defined(CONFIG_USER_ONLY) > > > - gen_priv_opc(ctx); > > > -#else > > > - TCGv_i32 t; > > > - > > > - REQUIRE_HV(ctx); > > > - translator_io_start(&ctx->base); > > > - t = tcg_constant_i32(PPC_PM_SLEEP); > > > - gen_helper_pminsn(tcg_env, t); > > > - /* Stop translation, as the CPU is supposed to sleep from now */ > > > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > > > -#endif /* defined(CONFIG_USER_ONLY) */ > > > - return true; > > > -} > > > -TRANS_FLAGS2(PM_ISA206, SLEEP, do_sleep); > > > - > > > -static bool do_rvwinkle(DisasContext *ctx, arg_RVWINKLE *a) > > > +/* > > > + * Helper to handle DOZE, NAP, SLEEP, RVWINKLE & STOP. Since none of them use > > > + * any arguments just use a placeholder arg_SLEEP. > > > + */ > > > +static bool do_sleep(DisasContext *ctx, arg_SLEEP *a, powerpc_pm_insn_t type) > > > { > > > REQUIRE_64BIT(ctx); > > > > > > #if defined(CONFIG_USER_ONLY) > > > gen_priv_opc(ctx); > > > -#else > > > +#elif defined(TARGET_PPC64) > > > TCGv_i32 t; > > > > > > REQUIRE_HV(ctx); > > > translator_io_start(&ctx->base); > > > - t = tcg_constant_i32(PPC_PM_RVWINKLE); > > > - gen_helper_pminsn(tcg_env, t); > > > + t = tcg_constant_i32(type); > > > + gen_helper_PMINSN(tcg_env, t); > > > /* Stop translation, as the CPU is supposed to sleep from now */ > > > gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > > > -#endif /* defined(CONFIG_USER_ONLY) */ > > > - return true; > > > -} > > > -TRANS_FLAGS2(PM_ISA206, RVWINKLE, do_rvwinkle); > > > - > > > -static bool do_stop(DisasContext *ctx, arg_STOP *a) > > > -{ > > > - REQUIRE_64BIT(ctx); > > > - > > > -#if defined(CONFIG_USER_ONLY) > > > - gen_priv_opc(ctx); > > > #else > > > - TCGv_i32 t; > > > - > > > - REQUIRE_HV(ctx); > > > - translator_io_start(&ctx->base); > > > - t = tcg_constant_i32(PPC_PM_STOP); > > > - gen_helper_pminsn(tcg_env, t); > > > - /* Stop translation, as the CPU is supposed to sleep from now */ > > > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > > > + qemu_build_not_reached(); > > > #endif > > > return true; > > > } > > > -TRANS_FLAGS2(ISA300, STOP, do_stop); > > > +TRANS_FLAGS2(PM_ISA206, DOZE, do_sleep, PPC_PM_DOZE); > > > +TRANS_FLAGS2(PM_ISA206, NAP, do_sleep, PPC_PM_NAP); > > > +TRANS_FLAGS2(PM_ISA206, SLEEP, do_sleep, PPC_PM_SLEEP); > > > +TRANS_FLAGS2(PM_ISA206, RVWINKLE, do_sleep, PPC_PM_RVWINKLE); > > > +TRANS_FLAGS2(ISA300, STOP, do_sleep, PPC_PM_STOP);