Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes

[Richard Henderson]

On 2/22/19 8:25 AM, address@hidden wrote:
> @@ -373,9 +373,10 @@ static int write_misa(CPURISCVState *env, int csrno, 
> target_ulong val)
>      }
>  
>      /* Suppress 'C' if next instruction is not aligned
> -       TODO: this should check next_pc */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> +       check next target pc */
> +    if ((val & RVC) && (env->pc_next & 3) != 0) {
>          val &= ~RVC;
> +        env->pending_rvc = 1;
>      }
>  
>      /* misa.MXL writes are not supported by QEMU */
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 2321bba..c9d84ea 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1999,20 +1999,26 @@ static void decode_RV32_64G(DisasContext *ctx)
>      }
>  }
>  
> -static void decode_opc(DisasContext *ctx)
> +static void decode_opc(DisasContext *ctx, CPUState *cpu)
>  {
> +    CPURISCVState *env = cpu->env_ptr;
>      /* check for compressed insn */
>      if (extract32(ctx->opcode, 0, 2) != 3) {
>          if (!has_ext(ctx, RVC)) {
>              gen_exception_illegal(ctx);
>          } else {
> -            ctx->pc_succ_insn = ctx->base.pc_next + 2;
> +            env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 2;
>              decode_RV32_64C(ctx);
>          }
>      } else {
> -        ctx->pc_succ_insn = ctx->base.pc_next + 4;
> +        env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 4;
>          decode_RV32_64G(ctx);
>      }
> +    /* check pending RVC */
> +    if (env->pending_rvc && ((env->pc_next & 3) != 0)) {
> +        env->misa |= RVC;
> +        env->pending_rvc = 0;

You cannot manipulate env like this during translation.

Neither the write to env->pc_next nor the read from env->pending_rvc here will
be in any synchronization with the execution of write_misa.

What semantics are you attempting to implement wrt setting/clearing RVC from 
MISA?

> @@ -2061,7 +2067,7 @@ static void riscv_tr_translate_insn
>      CPURISCVState *env = cpu->env_ptr;
>  
>      ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
> -    decode_opc(ctx);
> +    decode_opc(ctx, cpu);

This is exactly the reason why cpu is *not* passed down to decode_opc, so that
you cannot make this kind of mistake.


r~