Re: [PATCH v1 1/2] target/riscv: Implement the stval/mtval illegal instruction

[Bin Meng]

On Fri, Sep 3, 2021 at 7:23 AM Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> The stval and mtval registers can optionally contain the faulting
> instruction on an illegal instruction exception. This patch adds support
> for setting the stval and mtval registers based on the CPU feature.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.h        |  5 ++++-
>  target/riscv/cpu_helper.c |  9 +++++++++
>  target/riscv/translate.c  | 33 +++++++++++++++++++--------------
>  3 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index bf1c899c00..6d41a16ae3 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -77,7 +77,8 @@ enum {
>      RISCV_FEATURE_MMU,
>      RISCV_FEATURE_PMP,
>      RISCV_FEATURE_EPMP,
> -    RISCV_FEATURE_MISA
> +    RISCV_FEATURE_MISA,
> +    RISCV_FEATURE_MTVAL_INST,
>  };
>
>  #define PRIV_VERSION_1_10_0 0x00011000
> @@ -130,6 +131,8 @@ struct CPURISCVState {
>      target_ulong frm;
>
>      target_ulong badaddr;
> +    target_ulong bins;
> +
>      target_ulong guest_phys_fault_addr;
>
>      target_ulong priv_ver;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 968cb8046f..42edd71c1e 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -967,6 +967,15 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>              write_tval  = true;
>              tval = env->badaddr;
>              break;
> +        case RISCV_EXCP_ILLEGAL_INST:
> +            if (riscv_feature(env, RISCV_FEATURE_MTVAL_INST)) {
> +                /* The stval/mtval register can optionally also be used to

nits: incorrect multi-line comment format

> +                 * return the faulting instruction bits on an illegal
> +                 * instruction exception.
> +                 */
> +                tval = env->bins;
> +            }
> +            break;
>          default:
>              break;
>          }
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index e356fc6c46..4221d8e2d5 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -173,8 +173,27 @@ static void lookup_and_goto_ptr(DisasContext *ctx)
>      }
>  }
>
> +/*
> + * Wrappers for getting reg values.
> + *
> + * The $zero register does not have cpu_gpr[0] allocated -- we supply the
> + * constant zero as a source, and an uninitialized sink as destination.
> + *
> + * Further, we may provide an extension for word operations.
> + */
> +static TCGv temp_new(DisasContext *ctx)
> +{
> +    assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
> +    return ctx->temp[ctx->ntemp++] = tcg_temp_new();
> +}
> +
>  static void gen_exception_illegal(DisasContext *ctx)
>  {
> +    TCGv tmp = temp_new(ctx);
> +
> +    tcg_gen_movi_tl(tmp, ctx->opcode);

ctx->opcode is not initialized anywhere.

> +    tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, bins));
> +
>      generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
>  }
>
> @@ -195,20 +214,6 @@ static void gen_goto_tb(DisasContext *ctx, int n, 
> target_ulong dest)
>      }
>  }
>
> -/*
> - * Wrappers for getting reg values.
> - *
> - * The $zero register does not have cpu_gpr[0] allocated -- we supply the
> - * constant zero as a source, and an uninitialized sink as destination.
> - *
> - * Further, we may provide an extension for word operations.
> - */
> -static TCGv temp_new(DisasContext *ctx)
> -{
> -    assert(ctx->ntemp < ARRAY_SIZE(ctx->temp));
> -    return ctx->temp[ctx->ntemp++] = tcg_temp_new();
> -}
> -
>  static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
>  {
>      TCGv t;
> --

Regards,
Bin