Re: [Qemu-devel] [PATCH v2] target-mips: fix broken MIPS16 and microMIPS

[Leon Alrae <leon.alrae@imgtec.com>]

Hi Yongbok,

For me this patch looks fine, just minor nitpicks:

>      if (blink > 0) {
>          int post_delay = insn_bytes;
>          int lowbit = !!(ctx->hflags & MIPS_HFLAG_M16);
>  
> -        if (opc != OPC_JALRC)
> -            post_delay += ((ctx->hflags & MIPS_HFLAG_BDS16) ? 2 : 4);
> -
> +        post_delay += delayslot_size;

Wouldn't it be better to remove this line add delayslot_size during
post_delay initialization?

> +    if (ctx->hflags & MIPS_HFLAG_BDS_STRICT) {
> +        switch (op & 0x7) { /* MSB-3..MSB-5 */
> +        case 0:
> +        /* POOL31A, POOL32B, POOL32I, POOL32C */

POOL32A :)

>      case OPC_J ... OPC_JAL: /* Jump */
>           offset = (int32_t)(ctx->opcode & 0x3FFFFFF) << 2;
> -         gen_compute_branch(ctx, op, 4, rs, rt, offset);
> +         gen_compute_branch(ctx, op, 4, rs, rt, offset, 4);
>           break;
>      case OPC_BEQ ... OPC_BGTZ: /* Branch */
>      case OPC_BEQL ... OPC_BGTZL:
> -         gen_compute_branch(ctx, op, 4, rs, rt, imm << 2);
> +            gen_compute_branch(ctx, op, 4, rs, rt, imm << 2, 4);

Indentation issue?

> @@ -15719,6 +15648,13 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
>              ctx.bstate = BS_STOP;
>              break;
>          }
> +        if (ctx.hflags & MIPS_HFLAG_BMASK) {
> +            if (!(ctx.hflags & MIPS_HFLAG_BDS16) &&
> +                    !(ctx.hflags & MIPS_HFLAG_BDS32)) {

IMHO it would look nicer if you made this condition shorter by ORing BDS
hflags.

Feel free to add:

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

Regards,
Leon