On Wed, 19 Jan 2022 at 12:30, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
>
> On 13/1/22 21:20, Philipp Tomsich wrote:
> > To split up the decoder into multiple functions (both to support
> > vendor-specific opcodes in separate files and to simplify maintenance
> > of orthogonal extensions), this changes decode_op to iterate over a
> > table of decoders predicated on guard functions.
> >
> > This commit only adds the new structure and the table, allowing for
> > the easy addition of additional decoders in the future.
> >
> > Signed-off-by: Philipp Tomsich <philipp.toms...@vrull.eu>
> > ---
> >
> > Changes in v2:
> > - (new patch) iterate over a table of guarded decoder functions
> >
> > target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
> > 1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index 615048ec87..2cbf9cbb6f 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t
> > ext)
> > return ctx->misa_ext & ext;
> > }
> >
> > +static inline bool always_true_p(CPURISCVState *env
> > __attribute__((__unused__)),
> > + DisasContext *ctx
> > __attribute__((__unused__)))
> > +{
> > + return true;
> > +}
> > +
> > #ifdef TARGET_RISCV32
> > #define get_xl(ctx) MXL_RV32
> > #elif defined(CONFIG_USER_ONLY)
> > @@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase,
> > target_ulong pc)
> >
> > static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t
> > opcode)
> > {
> > - /* check for compressed insn */
> > + /* If not handled, we'll raise an illegal instruction exception */
> > + bool handled = false;
> > +
> > + /*
> > + * A table with predicate (i.e., guard) functions and decoder functions
> > + * that are tested in-order until a decoder matches onto the opcode.
> > + */
> > + const struct {
> > + bool (*guard_func)(CPURISCVState *, DisasContext *);
> > + bool (*decode_func)(DisasContext *, uint32_t);
> > + } decoders[] = {
> > + { always_true_p, decode_insn32 },
> > + };
> > +
> > + /* Check for compressed insn */
> > if (extract16(opcode, 0, 2) != 3) {
> > if (!has_ext(ctx, RVC)) {
> > gen_exception_illegal(ctx);
> > } else {
> > ctx->opcode = opcode;
> > ctx->pc_succ_insn = ctx->base.pc_next + 2;
> > - if (!decode_insn16(ctx, opcode)) {
> > - gen_exception_illegal(ctx);
> > - }
> > + handled = decode_insn16(ctx, opcode);
> > }
> > } else {
> > uint32_t opcode32 = opcode;
> > @@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env,
> > DisasContext *ctx, uint16_t opcode)
> > ctx->base.pc_next + 2));
> > ctx->opcode = opcode32;
> > ctx->pc_succ_insn = ctx->base.pc_next + 4;
> > - if (!decode_insn32(ctx, opcode32)) {
> > - gen_exception_illegal(ctx);
> > +
> > + for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> > + if (!decoders[i].guard_func(env, ctx))
> > + continue;
> > +
> > + if ((handled = decoders[i].decode_func(ctx, opcode32)))
> > + break;
>
> Again, while we might check whether "Vendor Extensions" are enabled or
> not at runtime, they are specific to a (vendor) core model, so we know
> their availability at instantiation time.
>
> I don't understand the need to iterate. You can check for vendor
> extensions in riscv_tr_init_disas_context() and set a vendor_decoder()
> handler in DisasContext, which ends calling the generic decode_opc()
> one.
While the design you propose is a valid variation that will achieve
most of the functionality, I don't believe that this is the best way
forward.
A key issue is that it will interfere with using the command-line to
enable/disable such vendor-defined extensions easily (i.e., "-cpu
any,XVentanaCondOps=true" will not work).
It also looks like there is a misunderstanding of how vendor-defined
extensions work: these will not be the same for every vendor core and
may be implemented by multiple vendors (after all: these are
vendor-defined, not vendor-specific). Trying to force the RISC-V
vendors down the route of handling this via a specialized decoder
function set up in riscv_tr_init_disas_context(), will eventually
force them to have multiple decode functions for
chip-families/generations — this is not conducive to easy
maintainability of the codebase.
Regards,
Philipp.
>
> > }
> > }
> > +
> > + if (!handled)
> > + gen_exception_illegal(ctx);
> > }
> >
> > static void riscv_tr_init_disas_context(DisasContextBase *dcbase,
> > CPUState *cs)
>
