[v1] target/riscv: Prevent lost illegal instruction exceptions

[PATCH] target/riscv: Prevent lost illegal instruction exceptions

Posted by Georg Kotheimer 3 years, 1 month ago

When decode_insn16() fails, we fall back to decode_RV32_64C() for
further compressed instruction decoding. However, prior to this change,
we did not raise an illegal instruction exception, if decode_RV32_64C()
fails to decode the instruction. This means that we skipped illegal
compressed instructions instead of raising an illegal instruction
exception.

Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>
---
 target/riscv/translate.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0f28b5f41e..8c00734252 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -537,7 +537,7 @@ static void gen_set_rm(DisasContext *ctx, int rm)
     tcg_temp_free_i32(t0);
 }
 
-static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
+static bool decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
 {
     uint8_t funct3 = extract16(opcode, 13, 3);
     uint8_t rd_rs2 = GET_C_RS2S(opcode);
@@ -554,7 +554,7 @@ static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
         gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
                     GET_C_LW_IMM(opcode));
 #endif
-        break;
+        return true;
     case 7:
 #if defined(TARGET_RISCV64)
         /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
@@ -565,18 +565,21 @@ static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
         gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
                      GET_C_LW_IMM(opcode));
 #endif
-        break;
+        return true;
+    default:
+        return false;
     }
 }
 
-static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
+static bool decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
 {
     uint8_t op = extract16(opcode, 0, 2);
 
     switch (op) {
     case 0:
-        decode_RV32_64C0(ctx, opcode);
-        break;
+        return decode_RV32_64C0(ctx, opcode);
+    default:
+        return false;
     }
 }
 
@@ -780,7 +783,9 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
             ctx->pc_succ_insn = ctx->base.pc_next + 2;
             if (!decode_insn16(ctx, opcode)) {
                 /* fall back to old decoder */
-                decode_RV32_64C(ctx, opcode);
+                if (!decode_RV32_64C(ctx, opcode)) {
+                    gen_exception_illegal(ctx);
+                }
             }
         }
     } else {
-- 
2.30.1

Re: [PATCH] target/riscv: Prevent lost illegal instruction exceptions

Posted by Richard Henderson 3 years, 1 month ago

On 3/16/21 9:03 AM, Georg Kotheimer wrote:
> When decode_insn16() fails, we fall back to decode_RV32_64C() for
> further compressed instruction decoding.

I think this is all dead code now.  Certainly c.ld/c.sd are in insn16-64.decode 
and c.flw/c.fsw are in insn16-32.decode.

Digging, we failed to remove these functions here: f330433b363.

You are absolutely right there's a missing

>               if (!decode_insn16(ctx, opcode)) {
>                   /* fall back to old decoder */
> -                decode_RV32_64C(ctx, opcode);
> +                if (!decode_RV32_64C(ctx, opcode)) {
> +                    gen_exception_illegal(ctx);

exception here, but we can remove the last remnants of the old decoder instead 
of patching them.

r~

Re: [PATCH] target/riscv: Prevent lost illegal instruction exceptions

Posted by Alistair Francis 3 years, 1 month ago

On Tue, Mar 16, 2021 at 11:05 AM Georg Kotheimer
<georg.kotheimer@kernkonzept.com> wrote:
>
> When decode_insn16() fails, we fall back to decode_RV32_64C() for
> further compressed instruction decoding. However, prior to this change,
> we did not raise an illegal instruction exception, if decode_RV32_64C()
> fails to decode the instruction. This means that we skipped illegal
> compressed instructions instead of raising an illegal instruction
> exception.
>
> Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 0f28b5f41e..8c00734252 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -537,7 +537,7 @@ static void gen_set_rm(DisasContext *ctx, int rm)
>      tcg_temp_free_i32(t0);
>  }
>
> -static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
> +static bool decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>  {
>      uint8_t funct3 = extract16(opcode, 13, 3);
>      uint8_t rd_rs2 = GET_C_RS2S(opcode);
> @@ -554,7 +554,7 @@ static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>          gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
>                      GET_C_LW_IMM(opcode));
>  #endif
> -        break;
> +        return true;
>      case 7:
>  #if defined(TARGET_RISCV64)
>          /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
> @@ -565,18 +565,21 @@ static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>          gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
>                       GET_C_LW_IMM(opcode));
>  #endif
> -        break;
> +        return true;
> +    default:
> +        return false;
>      }
>  }
>
> -static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
> +static bool decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
>  {
>      uint8_t op = extract16(opcode, 0, 2);
>
>      switch (op) {
>      case 0:
> -        decode_RV32_64C0(ctx, opcode);
> -        break;
> +        return decode_RV32_64C0(ctx, opcode);
> +    default:
> +        return false;
>      }
>  }
>
> @@ -780,7 +783,9 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
>              ctx->pc_succ_insn = ctx->base.pc_next + 2;
>              if (!decode_insn16(ctx, opcode)) {
>                  /* fall back to old decoder */
> -                decode_RV32_64C(ctx, opcode);
> +                if (!decode_RV32_64C(ctx, opcode)) {
> +                    gen_exception_illegal(ctx);
> +                }
>              }
>          }
>      } else {
> --
> 2.30.1
>
>