[PATCH 0/5] TriCore instruction bugfixes

[PATCH 0/5] TriCore instruction bugfixes

[Bastian Koppelmann]

Hi,

while resolving [1], I noticed a few more bugs in DEXTR and LD_BU_PREINC which
this patch series fixes. 

I also included the solo patch [1] into this series.

Cheers,
Bastian

[1] https://gitlab.com/qemu-project/qemu/-/issues/653
[2] https://lore.kernel.org/qemu-devel/20230113123759.677960-1-kbastian@mail.uni-paderborn.de/


Bastian Koppelmann (5):
  target/tricore: Fix OPC2_32_RCRW_IMASK translation
  target/tricore: Fix OPC2_32_RCRW_INSERT translation
  target/tricore: Fix RRPW_DEXTR
  target/tricore: Fix OPC2_32_RRRR_DEXTR
  target/tricore: Fix OPC2_32_BO_LD_BU_PREINC

 target/tricore/translate.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

-- 
2.39.1

[PATCH 1/5] target/tricore: Fix OPC2_32_RCRW_IMASK translation

[Bastian Koppelmann]

we were mixing up the "c" and "d" registers. We used "d" as a
destination register und "c" as the source. According to the TriCore ISA
manual 1.6 vol 2 it is the other way round.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/653
---
 target/tricore/translate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index df9e46c649..8de4e56b1f 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -5794,11 +5794,11 @@ static void decode_rcrw_insert(DisasContext *ctx)
 
     switch (op2) {
     case OPC2_32_RCRW_IMASK:
-        tcg_gen_andi_tl(temp, cpu_gpr_d[r4], 0x1f);
+        tcg_gen_andi_tl(temp, cpu_gpr_d[r3], 0x1f);
         tcg_gen_movi_tl(temp2, (1 << width) - 1);
-        tcg_gen_shl_tl(cpu_gpr_d[r3 + 1], temp2, temp);
+        tcg_gen_shl_tl(cpu_gpr_d[r4 + 1], temp2, temp);
         tcg_gen_movi_tl(temp2, const4);
-        tcg_gen_shl_tl(cpu_gpr_d[r3], temp2, temp);
+        tcg_gen_shl_tl(cpu_gpr_d[r4], temp2, temp);
         break;
     case OPC2_32_RCRW_INSERT:
         temp3 = tcg_temp_new();
-- 
2.39.1

[PATCH 2/5] target/tricore: Fix OPC2_32_RCRW_INSERT translation

[Bastian Koppelmann]

we were mixing up the "c" and "d" registers. We used "d" as a
destination register und "c" as the source. According to the TriCore ISA
manual 1.6 vol 2 it is the other way round.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/653
---
 target/tricore/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 8de4e56b1f..6149d4f5c0 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -5805,8 +5805,8 @@ static void decode_rcrw_insert(DisasContext *ctx)
 
         tcg_gen_movi_tl(temp, width);
         tcg_gen_movi_tl(temp2, const4);
-        tcg_gen_andi_tl(temp3, cpu_gpr_d[r4], 0x1f);
-        gen_insert(cpu_gpr_d[r3], cpu_gpr_d[r1], temp2, temp, temp3);
+        tcg_gen_andi_tl(temp3, cpu_gpr_d[r3], 0x1f);
+        gen_insert(cpu_gpr_d[r4], cpu_gpr_d[r1], temp2, temp, temp3);
 
         tcg_temp_free(temp3);
         break;
-- 
2.39.1

[PATCH 3/5] target/tricore: Fix RRPW_DEXTR

[Bastian Koppelmann]

if we used const16 == 0 we would crash qemu with the error:

../tcg/tcg-op.c:196: tcg_gen_shri_i32: Assertion `arg2 >= 0 && arg2 < 32' failed

This is a special case anyways as we can directly return cpu_gpr_d[r1]
as this is the most significant word an nothing is shifted.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
---
 target/tricore/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 6149d4f5c0..62128c6aae 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8708,6 +8708,8 @@ static void decode_32Bit_opc(DisasContext *ctx)
         const16 = MASK_OP_RRPW_POS(ctx->opcode);
         if (r1 == r2) {
             tcg_gen_rotli_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], const16);
+        } else if (const16 == 0) {
+            tcg_gen_mov_tl(cpu_gpr_d[r3], cpu_gpr_d[r1]);
         } else {
             temp = tcg_temp_new();
             tcg_gen_shli_tl(temp, cpu_gpr_d[r1], const16);
-- 
2.39.1

[PATCH 4/5] target/tricore: Fix OPC2_32_RRRR_DEXTR

[Bastian Koppelmann]

if cpu_gpr_d[r3] == 0 then we were shifting the lower register to the
right by 32 which is undefined behaviour. In this case the TriCore would
do nothing an just return the higher register cpu_reg_d[r1]. We fixed
that by detecting whether cpu_gpr_d[r3] was zero and cleared the lower
register.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
---
 target/tricore/translate.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 62128c6aae..b8e0969079 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8245,10 +8245,19 @@ static void decode_rrrr_extract_insert(DisasContext *ctx)
         if (r1 == r2) {
             tcg_gen_rotl_tl(cpu_gpr_d[r4], cpu_gpr_d[r1], tmp_pos);
         } else {
+            TCGv msw = tcg_temp_new();
+            TCGv zero = tcg_const_tl(0);
             tcg_gen_shl_tl(tmp_width, cpu_gpr_d[r1], tmp_pos);
-            tcg_gen_subfi_tl(tmp_pos, 32, tmp_pos);
-            tcg_gen_shr_tl(tmp_pos, cpu_gpr_d[r2], tmp_pos);
-            tcg_gen_or_tl(cpu_gpr_d[r4], tmp_width, tmp_pos);
+            tcg_gen_subfi_tl(msw, 32, tmp_pos);
+            tcg_gen_shr_tl(msw, cpu_gpr_d[r2], msw);
+            /* if pos == 0, then we do cpu_gpr_d[r2] << 32, which is undefined
+             * behaviour. So check that case here and set the low bits to zero
+             * which effectivly returns cpu_gpr_d[r1]
+             */
+            tcg_gen_movcond_tl(TCG_COND_EQ, msw, tmp_pos, zero, zero, msw);
+            tcg_gen_or_tl(cpu_gpr_d[r4], tmp_width, msw);
+            tcg_temp_free(zero);
+            tcg_temp_free(msw);
         }
         break;
     case OPC2_32_RRRR_EXTR:
-- 
2.39.1

[PATCH 5/5] target/tricore: Fix OPC2_32_BO_LD_BU_PREINC

[Bastian Koppelmann]

we were sign extending the result of the load, while the instruction
clearly states that the result should be unsigned.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
---
 target/tricore/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index b8e0969079..c17d19b83e 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -4964,7 +4964,7 @@ static void decode_bo_addrmode_ld_post_pre_base(DisasContext *ctx)
         tcg_gen_addi_tl(cpu_gpr_a[r2], cpu_gpr_a[r2], off10);
         break;
     case OPC2_32_BO_LD_BU_PREINC:
-        gen_ld_preincr(ctx, cpu_gpr_d[r1], cpu_gpr_a[r2], off10, MO_SB);
+        gen_ld_preincr(ctx, cpu_gpr_d[r1], cpu_gpr_a[r2], off10, MO_UB);
         break;
     case OPC2_32_BO_LD_D_SHORTOFF:
         CHECK_REG_PAIR(r1);
-- 
2.39.1

Re: [PATCH 1/5] target/tricore: Fix OPC2_32_RCRW_IMASK translation

[Richard Henderson]

On 1/27/23 02:03, Bastian Koppelmann wrote:
> we were mixing up the "c" and "d" registers. We used "d" as a
> destination register und "c" as the source. According to the TriCore ISA
> manual 1.6 vol 2 it is the other way round.
> 
> Signed-off-by: Bastian Koppelmann<kbastian@mail.uni-paderborn.de>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/653
> ---
>   target/tricore/translate.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/5] target/tricore: Fix OPC2_32_RCRW_INSERT translation

[Richard Henderson]

On 1/27/23 02:03, Bastian Koppelmann wrote:
> we were mixing up the "c" and "d" registers. We used "d" as a
> destination register und "c" as the source. According to the TriCore ISA
> manual 1.6 vol 2 it is the other way round.
> 
> Signed-off-by: Bastian Koppelmann<kbastian@mail.uni-paderborn.de>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/653
> ---
>   target/tricore/translate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/5] target/tricore: Fix RRPW_DEXTR

[Richard Henderson]

On 1/27/23 02:03, Bastian Koppelmann wrote:
> if we used const16 == 0 we would crash qemu with the error:
> 
> ../tcg/tcg-op.c:196: tcg_gen_shri_i32: Assertion `arg2 >= 0 && arg2 < 32' failed
> 
> This is a special case anyways as we can directly return cpu_gpr_d[r1]
> as this is the most significant word an nothing is shifted.
> 
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> ---
>   target/tricore/translate.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index 6149d4f5c0..62128c6aae 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -8708,6 +8708,8 @@ static void decode_32Bit_opc(DisasContext *ctx)
>           const16 = MASK_OP_RRPW_POS(ctx->opcode);
>           if (r1 == r2) {
>               tcg_gen_rotli_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], const16);
> +        } else if (const16 == 0) {
> +            tcg_gen_mov_tl(cpu_gpr_d[r3], cpu_gpr_d[r1]);
>           } else {
>               temp = tcg_temp_new();
>               tcg_gen_shli_tl(temp, cpu_gpr_d[r1], const16);

While correct, this entire operation is

    tcg_gen_extract2_tl(cpu_gpr_d[r3], cpu_gpr_d[r2], cpu_gpr_d[r1], 32 - const16);

which will take care of your two special cases as well.


r~

Re: [PATCH 4/5] target/tricore: Fix OPC2_32_RRRR_DEXTR

[Richard Henderson]

On 1/27/23 02:03, Bastian Koppelmann wrote:
> if cpu_gpr_d[r3] == 0 then we were shifting the lower register to the
> right by 32 which is undefined behaviour. In this case the TriCore would
> do nothing an just return the higher register cpu_reg_d[r1]. We fixed
> that by detecting whether cpu_gpr_d[r3] was zero and cleared the lower
> register.
> 
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> ---
>   target/tricore/translate.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index 62128c6aae..b8e0969079 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -8245,10 +8245,19 @@ static void decode_rrrr_extract_insert(DisasContext *ctx)
>           if (r1 == r2) {
>               tcg_gen_rotl_tl(cpu_gpr_d[r4], cpu_gpr_d[r1], tmp_pos);
>           } else {
> +            TCGv msw = tcg_temp_new();
> +            TCGv zero = tcg_const_tl(0);

tcg_constant_tl(0), which you then don't need to free at the end.

>               tcg_gen_shl_tl(tmp_width, cpu_gpr_d[r1], tmp_pos);
> +            tcg_gen_subfi_tl(msw, 32, tmp_pos);
> +            tcg_gen_shr_tl(msw, cpu_gpr_d[r2], msw);
> +            /* if pos == 0, then we do cpu_gpr_d[r2] << 32, which is undefined

   /*
    * If ...
    */

> +             * behaviour. So check that case here and set the low bits to zero
> +             * which effectivly returns cpu_gpr_d[r1]
> +             */
> +            tcg_gen_movcond_tl(TCG_COND_EQ, msw, tmp_pos, zero, zero, msw);
> +            tcg_gen_or_tl(cpu_gpr_d[r4], tmp_width, msw);
> +            tcg_temp_free(zero);
> +            tcg_temp_free(msw);

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 5/5] target/tricore: Fix OPC2_32_BO_LD_BU_PREINC

[Richard Henderson]

On 1/27/23 02:03, Bastian Koppelmann wrote:
> we were sign extending the result of the load, while the instruction
> clearly states that the result should be unsigned.
> 
> Signed-off-by: Bastian Koppelmann<kbastian@mail.uni-paderborn.de>
> ---
>   target/tricore/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~