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

[Ruinland ChuanTzu Tsai]

On Thu, Oct 21, 2021 at 09:11:44AM -0700, Richard Henderson wrote:
> 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.

Wilco, thanks for the tips.

Cordially yours,
Ruinland

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