Re: [PATCH v2] Properly sign extend BBIT branch offset during calculation

[Christopher Wrogg]

I agree. Here is the corrected patch.

Signed-off-by: Christopher Wrogg <cwrogg@umich.edu>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1251

---
 target/mips/tcg/octeon.decode | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/mips/tcg/octeon.decode b/target/mips/tcg/octeon.decode
index 8929ad088e..0c787cb498 100644
--- a/target/mips/tcg/octeon.decode
+++ b/target/mips/tcg/octeon.decode
@@ -12,7 +12,7 @@
 # BBIT132    111110 ..... ..... ................
 
 %bbit_p      28:1 16:5
-BBIT         11 set:1 . 10 rs:5 ..... offset:16 p=%bbit_p
+BBIT         11 set:1 . 10 rs:5 ..... offset:s16 p=%bbit_p
 
 # Arithmetic
 # BADDU rd, rs, rt
--
2.30.2

On Thu, Oct 13, 2022 at 2:33 PM Richard Henderson <richard.henderson@linaro.org> wrote:

On 10/13/22 15:08, Christopher Wrogg wrote:
> The Octeon specific BBIT instruction incorrectly computes
> the branch offset. The 16 bit value is not sign extended.
>
> Signed-off-by: Christopher Wrogg <cwrogg@umich.edu <mailto:cwrogg@umich.edu>>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1251
> <https://gitlab.com/qemu-project/qemu/-/issues/1251>
> ---
>   target/mips/tcg/octeon_translate.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target/mips/tcg/octeon_translate.c b/target/mips/tcg/octeon_translate.c
> index 6a207d2e7e..90f7b105cb 100644
> --- a/target/mips/tcg/octeon_translate.c
> +++ b/target/mips/tcg/octeon_translate.c
> @@ -38,7 +38,10 @@ static bool trans_BBIT(DisasContext *ctx, arg_BBIT *a)
>       }
>
>       ctx->hflags |= MIPS_HFLAG_BC;
> -    ctx->btarget = ctx->base.pc_next + 4 + a->offset * 4;
> +    a->offset *= 4;
> +    a->offset = (target_long)(int16_t)a->offset;
> +    ctx->btarget = ctx->base.pc_next + 4 + a->offset;

This looks wrong, because you're sign-extending after scaling, which gives you only 14
bits of offset instead of 16.

The correct fix should be earlier in decode:

- BBIT         11 set:1 . 10 rs:5 ..... offset:16 p=%bbit_p
+ BBIT         11 set:1 . 10 rs:5 ..... offset:s16 p=%bbit_p

to indicate a extract a signed field from the instruction.


r~