On Mon, Jul 06, 2026 at 10:50:39PM +0530, Nikhil Kumar Singh wrote: > > On 20/05/26 21:37, Chinmay Rath wrote: > > From: Ojaswin Mujoo<[email protected]> > > > > Convert the following instrunctions to decodetree specification: > The word should be spelled `instructions` and not `instrunctions`. Please > fix this in the > commit message.
Hi Nikhil, thanks for the review. I'll make the change. > > bc > > bca > > bcl > > bcla > > > > bclr > > bclrl > > > > bcctr > > bcctrl > > > > bctar > > bctarl > > > > The branch was tested by comparing the qemu -D log -d op,in_asm output for > > each instruction and also for the various combinations of branch option > > (BO) bits for the conditional branch instructions. Additionally, gdb > > was used to confirm the LR and other register's behavior is consistent > > to legacy behavior. > > > > Further, the changes also pass a boot test into Fedora distro. > > > > Signed-off-by: Ojaswin Mujoo<[email protected]> > > Signed-off-by: Chinmay Rath<[email protected]> > > --- > > target/ppc/insn32.decode | 13 ++ > > target/ppc/translate.c | 148 ----------------------- > > target/ppc/translate/branch-impl.c.inc | 157 +++++++++++++++++++++++++ > > 3 files changed, 170 insertions(+), 148 deletions(-) > > > > diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode > > index b98fe01a84..34b36cbef6 100644 > > --- a/target/ppc/insn32.decode > > +++ b/target/ppc/insn32.decode > > @@ -1485,6 +1485,19 @@ CLRBHRB 011111 ----- ----- ----- 0110101110 - > > B 010010 ........................ . . @I_b > > +%bd 2:14 !function=times_4 > > +&bcond bo:uint32_t bi bd aa:bool lk:bool > > +@B_bcond ...... bo:5 bi:5 .............. aa:1 lk:1 &bcond bd=%bd > > + > > +BC 010000 ..... ..... .............. . . @B_bcond > > + > > +&bclr bo bi bh lk:bool > > +@XL_bclr ...... bo:5 bi:5 --- bh:2 .......... lk:1 &bclr > > + > > +BCLR 010011 ..... ..... ---.. 0000010000 . @XL_bclr > > +BCCTR 010011 ..... ..... ---.. 1000010000 . @XL_bclr > > +BCTAR 010011 ..... ..... ---.. 1000110000 . @XL_bclr > > + > > ## Misc POWER instructions > > ATTN 000000 00000 00000 00000 0100000000 0 > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > > index 37a164951f..22125c30a5 100644 > > --- a/target/ppc/translate.c > > +++ b/target/ppc/translate.c > > @@ -3089,150 +3089,6 @@ static inline void gen_setlr(DisasContext *ctx, > > target_ulong nip) > > tcg_gen_movi_tl(cpu_lr, nip); > > } > > -#define BCOND_IM 0 > > -#define BCOND_LR 1 > > -#define BCOND_CTR 2 > > -#define BCOND_TAR 3 > > - > > -static void gen_bcond(DisasContext *ctx, int type) > > -{ > > - uint32_t bo = BO(ctx->opcode); > > - TCGLabel *l1; > > - TCGv target; > > - target_long bhrb_type = BHRB_TYPE_OTHER; > > - > > - if (type == BCOND_LR || type == BCOND_CTR || type == BCOND_TAR) { > > - target = tcg_temp_new(); > > - if (type == BCOND_CTR) { > > - tcg_gen_mov_tl(target, cpu_ctr); > > - } else if (type == BCOND_TAR) { > > - gen_load_spr(target, SPR_TAR); > > - } else { > > - tcg_gen_mov_tl(target, cpu_lr); > > - } > > - if (!LK(ctx->opcode)) { > > - bhrb_type |= BHRB_TYPE_INDIRECT; > > - } > > - bhrb_type |= BHRB_TYPE_XL_FORM; > > - } else { > > - target = NULL; > > - } > > - if (LK(ctx->opcode)) { > > - gen_setlr(ctx, ctx->base.pc_next); > > - bhrb_type |= BHRB_TYPE_CALL; > > - } > > - l1 = gen_new_label(); > > - if ((bo & 0x4) == 0) { > > - /* Decrement and test CTR */ > > - TCGv temp = tcg_temp_new(); > > - > > - if (type == BCOND_CTR) { > > - /* > > - * All ISAs up to v3 describe this form of bcctr as invalid but > > - * some processors, ie. 64-bit server processors compliant with > > - * arch 2.x, do implement a "test and decrement" logic instead, > > - * as described in their respective UMs. This logic involves > > CTR > > - * to act as both the branch target and a counter, which makes > > - * it basically useless and thus never used in real code. > > - * > > - * This form was hence chosen to trigger extra > > micro-architectural > > - * side-effect on real HW needed for the Spectre v2 workaround. > > - * It is up to guests that implement such workaround, ie. > > linux, to > > - * use this form in a way it just triggers the side-effect > > without > > - * doing anything else harmful. > > - */ > > - if (unlikely(!is_book3s_arch2x(ctx))) { > > - gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); > > - return; > > - } > > - > > - if (NARROW_MODE(ctx)) { > > - tcg_gen_ext32u_tl(temp, cpu_ctr); > > - } else { > > - tcg_gen_mov_tl(temp, cpu_ctr); > > - } > > - if (bo & 0x2) { > > - tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1); > > - } else { > > - tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1); > > - } > > - tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1); > > - } else { > > - tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1); > > - if (NARROW_MODE(ctx)) { > > - tcg_gen_ext32u_tl(temp, cpu_ctr); > > - } else { > > - tcg_gen_mov_tl(temp, cpu_ctr); > > - } > > - if (bo & 0x2) { > > - tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1); > > - } else { > > - tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1); > > - } > > - } > > - bhrb_type |= BHRB_TYPE_COND; > > - } > > - if ((bo & 0x10) == 0) { > > - /* Test CR */ > > - uint32_t bi = BI(ctx->opcode); > > - uint32_t mask = 0x08 >> (bi & 0x03); > > - TCGv_i32 temp = tcg_temp_new_i32(); > > - > > - if (bo & 0x8) { > > - tcg_gen_andi_i32(temp, cpu_crf[bi >> 2], mask); > > - tcg_gen_brcondi_i32(TCG_COND_EQ, temp, 0, l1); > > - } else { > > - tcg_gen_andi_i32(temp, cpu_crf[bi >> 2], mask); > > - tcg_gen_brcondi_i32(TCG_COND_NE, temp, 0, l1); > > - } > > - bhrb_type |= BHRB_TYPE_COND; > > - } > > - > > - gen_update_branch_history(ctx, ctx->cia, target, bhrb_type); > > - > > - if (type == BCOND_IM) { > > - target_ulong li = (target_long)((int16_t)(BD(ctx->opcode))); > > - if (likely(AA(ctx->opcode) == 0)) { > > - gen_goto_tb(ctx, 0, ctx->cia + li); > > - } else { > > - gen_goto_tb(ctx, 0, li); > > - } > > - } else { > > - if (NARROW_MODE(ctx)) { > > - tcg_gen_andi_tl(cpu_nip, target, (uint32_t)~3); > > - } else { > > - tcg_gen_andi_tl(cpu_nip, target, ~3); > > - } > > - gen_lookup_and_goto_ptr(ctx); > > - } > > - if ((bo & 0x14) != 0x14) { > > - /* fallthrough case */ > > - gen_set_label(l1); > > - gen_goto_tb(ctx, 1, ctx->base.pc_next); > > - } > > - ctx->base.is_jmp = DISAS_NORETURN; > > -} > > - > > -static void gen_bc(DisasContext *ctx) > > -{ > > - gen_bcond(ctx, BCOND_IM); > > -} > > - > > -static void gen_bcctr(DisasContext *ctx) > > -{ > > - gen_bcond(ctx, BCOND_CTR); > > -} > > - > > -static void gen_bclr(DisasContext *ctx) > > -{ > > - gen_bcond(ctx, BCOND_LR); > > -} > > - > > -static void gen_bctar(DisasContext *ctx) > > -{ > > - gen_bcond(ctx, BCOND_TAR); > > -} > > - > > /*** Condition register logical > > ***/ > > #define GEN_CRLOGIC(name, tcg_op, opc) > > \ > > static void glue(gen_, name)(DisasContext *ctx) > > \ > > @@ -5367,10 +5223,6 @@ GEN_HANDLER(stswx, 0x1F, 0x15, 0x14, 0x00000001, > > PPC_STRING), > > /* ISA v3.0 changed the extended opcode from 62 to 30 */ > > GEN_HANDLER(wait, 0x1F, 0x1E, 0x01, 0x039FF801, PPC_WAIT), > > GEN_HANDLER_E(wait, 0x1F, 0x1E, 0x00, 0x039CF801, PPC_NONE, PPC2_ISA300), > > -GEN_HANDLER(bc, 0x10, 0xFF, 0xFF, 0x00000000, PPC_FLOW), > > -GEN_HANDLER(bcctr, 0x13, 0x10, 0x10, 0x00000000, PPC_FLOW), > > -GEN_HANDLER(bclr, 0x13, 0x10, 0x00, 0x00000000, PPC_FLOW), > > -GEN_HANDLER_E(bctar, 0x13, 0x10, 0x11, 0x0000E000, PPC_NONE, > > PPC2_BCTAR_ISA207), > > GEN_HANDLER(rfi, 0x13, 0x12, 0x01, 0x03FF8001, PPC_FLOW), > > #if defined(TARGET_PPC64) > > GEN_HANDLER(rfid, 0x13, 0x12, 0x00, 0x03FF8001, PPC_64B), > > diff --git a/target/ppc/translate/branch-impl.c.inc > > b/target/ppc/translate/branch-impl.c.inc > > index 745f71afd1..44ed40422e 100644 > > --- a/target/ppc/translate/branch-impl.c.inc > > +++ b/target/ppc/translate/branch-impl.c.inc > > @@ -55,3 +55,160 @@ static bool trans_B(DisasContext *ctx, arg_I_b *a) > > return true; > > } > > + > > +#define BCOND_IM 0 > > +#define BCOND_LR 1 > > +#define BCOND_CTR 2 > > +#define BCOND_TAR 3 > > + > > +static bool bcond_helper(DisasContext *ctx, int type, uint32_t bo, int bi, > > + int bd, int bh, bool aa, bool lk) > > +{ > > + TCGLabel *l1; > > + TCGv target; > > + target_long bhrb_type = BHRB_TYPE_OTHER; > > + > > + if (type == BCOND_IM && bh != -1) { > > + /* BCOND_IM should never use bh */ > > + return false; > > + } else if (type != BCOND_IM && (bd != -1 || aa != 0)) { > > + /* Other BCOND types should never use bd or aa */ > > + return false; > > + } > > + > > + if (type == BCOND_LR || type == BCOND_CTR || type == BCOND_TAR) { > > + target = tcg_temp_new(); > > + if (type == BCOND_CTR) { > > + tcg_gen_mov_tl(target, cpu_ctr); > > + } else if (type == BCOND_TAR) { > > + gen_load_spr(target, SPR_TAR); > > + } else { > > + tcg_gen_mov_tl(target, cpu_lr); > > + } > > + if (!lk) { > > + bhrb_type |= BHRB_TYPE_INDIRECT; > > + } > > + bhrb_type |= BHRB_TYPE_XL_FORM; > > + } else { > > + target = NULL; > > + } > > + if (lk) { > > + gen_setlr(ctx, ctx->base.pc_next); > > + bhrb_type |= BHRB_TYPE_CALL; > > + } > > + l1 = gen_new_label(); > > + if ((bo & 0x4) == 0) { > > + /* Decrement and test CTR */ > > + TCGv temp = tcg_temp_new(); > > + > > + if (type == BCOND_CTR) { > > + /* > > + * All ISAs up to v3 describe this form of bcctr as invalid but > > + * some processors, ie. 64-bit server processors compliant with > > + * arch 2.x, do implement a "test and decrement" logic instead, > > + * as described in their respective UMs. This logic involves > > CTR > > + * to act as both the branch target and a counter, which makes > > + * it basically useless and thus never used in real code. > > + * > > + * This form was hence chosen to trigger extra > > micro-architectural > > + * side-effect on real HW needed for the Spectre v2 workaround. > > + * It is up to guests that implement such workaround, ie. > > linux, to > > + * use this form in a way it just triggers the side-effect > > without > > + * doing anything else harmful. > > + */ > > + if (unlikely(!is_book3s_arch2x(ctx))) { > > + gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); > > + return true; > > + } > > + > > + if (NARROW_MODE(ctx)) { > > + tcg_gen_ext32u_tl(temp, cpu_ctr); > > + } else { > > + tcg_gen_mov_tl(temp, cpu_ctr); > > + } > > + if (bo & 0x2) { > > + tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1); > > + } else { > > + tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1); > > + } > > + tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1); > > + } else { > > + tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1); > > + if (NARROW_MODE(ctx)) { > > + tcg_gen_ext32u_tl(temp, cpu_ctr); > > + } else { > > + tcg_gen_mov_tl(temp, cpu_ctr); > > + } > > + if (bo & 0x2) { > > + tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1); > > + } else { > > + tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1); > > + } > > + } > > + bhrb_type |= BHRB_TYPE_COND; > > + } > > + if ((bo & 0x10) == 0) { > > + /* Test CR */ > > + uint32_t mask = 0x08 >> (bi & 0x03); > > + TCGv_i32 temp = tcg_temp_new_i32(); > > + > > + if (bo & 0x8) { > > + tcg_gen_andi_i32(temp, cpu_crf[bi >> 2], mask); > > + tcg_gen_brcondi_i32(TCG_COND_EQ, temp, 0, l1); > > + } else { > > + tcg_gen_andi_i32(temp, cpu_crf[bi >> 2], mask); > > + tcg_gen_brcondi_i32(TCG_COND_NE, temp, 0, l1); > > + } > > + bhrb_type |= BHRB_TYPE_COND; > > + } > > + > > + gen_update_branch_history(ctx, ctx->cia, target, bhrb_type); > > + > > + if (type == BCOND_IM) { > > + target_ulong li = (target_long)((int16_t)(bd)); > > + if (likely(aa == 0)) { > > + gen_goto_tb(ctx, 0, ctx->cia + li); > > + } else { > > + gen_goto_tb(ctx, 0, li); > > + } > > + } else { > > + if (NARROW_MODE(ctx)) { > > + tcg_gen_andi_tl(cpu_nip, target, (uint32_t)~3); > > + } else { > > + tcg_gen_andi_tl(cpu_nip, target, ~3); > > + } > > + gen_lookup_and_goto_ptr(ctx); > > + } > > + if ((bo & 0x14) != 0x14) { > > + /* fallthrough case */ > > + gen_set_label(l1); > > + gen_goto_tb(ctx, 1, ctx->base.pc_next); > > + } > > + ctx->base.is_jmp = DISAS_NORETURN; > > + > > + return true; > > +} > > + > > +static bool trans_BC(DisasContext *ctx, arg_bcond *a) > > +{ > > + /* > > + * bh is not used for bc variants hence we pass -1 > > + */ > > + return bcond_helper(ctx, BCOND_IM, a->bo, a->bi, a->bd, -1, a->aa, > > a->lk); > > +} > > + > > +/* > > + * This helper is shared by bclr, bcctr and bctar. > > + */ > > +static bool bclr_helper(DisasContext *ctx, arg_bclr *a, int type) > > +{ > > + /* > > + * bd and aa is not used for bc variants hence we pass -1 and 0 > > respectively > > + */ > > + return bcond_helper(ctx, type, a->bo, a->bi, -1, a->bh, 0, a->lk); > > +} > > + > > +TRANS(BCLR, bclr_helper, BCOND_LR) > > +TRANS(BCCTR, bclr_helper, BCOND_CTR) > > +TRANS(BCTAR, bclr_helper, BCOND_TAR) > > + > The patch looks mostly fine, but I noticed one small issue regarding the > bctar instruction. > > Actually, in the legacy implementation, bctar was properly guarded by the > ISA 2.07 feature flag like this: > GEN_HANDLER_E(bctar, 0x13, 0x10, 0x11, 0x0000E000, PPC_NONE, > PPC2_BCTAR_ISA207) > > But in the new decodetree implementation, it is getting mapped directly > using the standard macro > TRANS(BCTAR, bclr_helper, BCOND_TAR) Yes you are right, I missed this. Thanks for pointing it out. > > Since the TRANS macro doesn't apply any secondary instruction flags (and > even bclr_helper / bcond_helper are not checking for it), > this check is being missed out. > > It could be better if you can just write a dedicated translation handler for > the same to enforce the BCTAR_ISA207 check. > I guess Chinmay can tell better on this. I think we can just do the following and it should be good enough: -TRANS(BCTAR, bclr_helper, BCOND_TAR) +TRANS_FLAGS2(BCTAR_ISA207, BCTAR, bclr_helper, BCOND_TAR) If this looks okay, I can send a v2. Regards, ojaswin > > Thanks > ~ Nikhil >
