Hi,
On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.ben...@linaro.org> wrote:
> -static void decode_RV32_64C0(DisasContext *ctx)
> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
> {
> - uint8_t funct3 = extract32(ctx->opcode, 13, 3);
> - uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
> - uint8_t rs1s = GET_C_RS1S(ctx->opcode);
> + uint8_t funct3 = extract32(opcode, 13, 3);
We noticed that a uint16_t opcode is passed into this function and
then passed on to extract32().
This is a minor point, but the extract32() will validate the start and
length values passed in according to 32 bits, not 16 bits.
static inline uint32_t extract32(uint32_t value, int start, int length)
{
assert(start >= 0 && length > 0 && length <= 32 - start);
Since we have an extract32() and extract64(), maybe we could add an
extract16() and use it here?
Thanks & Regards,
-Rob
> + uint8_t rd_rs2 = GET_C_RS2S(opcode);
> + uint8_t rs1s = GET_C_RS1S(opcode);
>
> switch (funct3) {
> case 3:
> #if defined(TARGET_RISCV64)
> /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
> gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
> - GET_C_LD_IMM(ctx->opcode));
> + GET_C_LD_IMM(opcode));
> #else
> /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
> gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
> - GET_C_LW_IMM(ctx->opcode));
> + GET_C_LW_IMM(opcode));
> #endif
> break;
> case 7:
> #if defined(TARGET_RISCV64)
> /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
> gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
> - GET_C_LD_IMM(ctx->opcode));
> + GET_C_LD_IMM(opcode));
> #else
> /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
> gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
> - GET_C_LW_IMM(ctx->opcode));
> + GET_C_LW_IMM(opcode));
> #endif
> break;
> }
> }
>
> -static void decode_RV32_64C(DisasContext *ctx)
> +static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
> {
> - uint8_t op = extract32(ctx->opcode, 0, 2);
> + uint8_t op = extract32(opcode, 0, 2);
>
> switch (op) {
> case 0:
> - decode_RV32_64C0(ctx);
> + decode_RV32_64C0(ctx, opcode);
> break;
> }
> }
> @@ -709,22 +708,24 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
> /* Include the auto-generated decoder for 16 bit insn */
> #include "decode_insn16.inc.c"
>
> -static void decode_opc(DisasContext *ctx)
> +static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t
> opcode)
> {
> /* check for compressed insn */
> - if (extract32(ctx->opcode, 0, 2) != 3) {
> + if (extract32(opcode, 0, 2) != 3) {
> if (!has_ext(ctx, RVC)) {
> gen_exception_illegal(ctx);
> } else {
> ctx->pc_succ_insn = ctx->base.pc_next + 2;
> - if (!decode_insn16(ctx, ctx->opcode)) {
> + if (!decode_insn16(ctx, opcode)) {
> /* fall back to old decoder */
> - decode_RV32_64C(ctx);
> + decode_RV32_64C(ctx, opcode);
> }
> }
> } else {
> + uint32_t opcode32 = opcode;
> + opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env,
> ctx->base.pc_next + 2));
> ctx->pc_succ_insn = ctx->base.pc_next + 4;
> - if (!decode_insn32(ctx, ctx->opcode)) {
> + if (!decode_insn32(ctx, opcode32)) {
> gen_exception_illegal(ctx);
> }
> }
> @@ -776,9 +777,9 @@ static void riscv_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cpu)
> {
> DisasContext *ctx = container_of(dcbase, DisasContext, base);
> CPURISCVState *env = cpu->env_ptr;
> + uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
>
> - ctx->opcode = translator_ldl(env, ctx->base.pc_next);
> - decode_opc(ctx);
> + decode_opc(env, ctx, opcode16);
> ctx->base.pc_next = ctx->pc_succ_insn;
>
> if (ctx->base.is_jmp == DISAS_NEXT) {
> --
> 2.20.1
>
