Re: [RFC PATCH 15/28] target/ppc: move various conditional branch insns to decodetree

[Chinmay Rath <rathc@linux.ibm.com>]

On 7/15/26 14:29, Ojaswin Mujoo wrote:
> 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<ojaswin@linux.ibm.com>
>>>
>>> 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<ojaswin@linux.ibm.com>
>>> Signed-off-by: Chinmay Rath<rathc@linux.ibm.com>
>>> ---
>>>    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.
Hi Nikhil,
Thanks for catching this !
>>
>> 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.

Hi Ojas,
Since it is just a single line change, I will take care of it while 
posting v2 series.

Regards,
Chinmay

>
> Regards,
> ojaswin
>
>> Thanks
>> ~ Nikhil
>>