Re: [RFC PATCH v1 1/2] riscv: Add preliminary infra for custom instrcution handling

[Richard Henderson]

On 10/21/21 8:11 AM, Ruinland Chuan-Tzu Tsai wrote:
> -static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> +/* Custom insn related definitions/prototypes */
> +extern __thread bool cpu_has_custom_insns;
> +/*
> + * These 2 are for indication if decode fails.
> + * We don't want to interfere the following regular extension decoding
> + * We assume MTTCG is 1-1 mapping for now.
> + */
> +__thread bool custom_c_insn_decoded;
> +__thread bool custom_insn_decoded;
> +
> +extern bool (*custom_c_insn_handler)(DisasContext *, uint16_t);
> +extern bool (*custom_insn_handler)(DisasContext *, uint32_t);
> +
> +static void try_decode_custom_insn(CPURISCVState *env, DisasContext *ctx,
> +                                   uint16_t opcode)


This is way, way over-engineered.

You seem to be trying to design something that can be plugged in, which the rest of QEMU 
knows nothing of.  I think that's a mistake.  Your custom cpu extensions should be treated 
as any other RISC-V extension, since it is in fact built in to QEMU.

Begin with adding the "bool ext_andes" field in RISCVCPU.  Propagate that into the 
DisasContext just like the other extensions.

Changes to decode_opc should be quite simple:

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 6d7fbca1fa..ea1e159259 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -473,13 +473,16 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, 
uint16_t opcode)
 {
     /* check for compressed insn */
     if (extract16(opcode, 0, 2) != 3) {
+        ctx->pc_succ_insn = ctx->base.pc_next + 2;
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
-        } else {
-            ctx->pc_succ_insn = ctx->base.pc_next + 2;
-            if (!decode_insn16(ctx, opcode)) {
-                gen_exception_illegal(ctx);
-            }
+            return;
+        }
+        if (ctx->ext_andes && decode_andes16(ctx, opcode)) {
+            return;
+        }
+        if (!decode_insn16(ctx, opcode)) {
+            gen_exception_illegal(ctx);
         }
     } else {
         uint32_t opcode32 = opcode;
@@ -487,6 +490,9 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t 
opcode)
                              translator_lduw(env, &ctx->base,
                                              ctx->base.pc_next + 2));
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
+        if (ctx->ext_andes && decode_andes32(ctx, opcode)) {
+            return;
+        }
         if (!decode_insn32(ctx, opcode32)) {
             gen_exception_illegal(ctx);
         }


r~