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>
Resolves: 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;
+
     ctx->hflags |= MIPS_HFLAG_BDS32;

     tcg_temp_free(t0);
-- 
2.30.2

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~

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~
>