Re: [PATCH v2 1/2] target/riscv: Convert sdtrig functionality from property to an extension

[Anup Patel]

On Wed, Jan 17, 2024 at 7:54 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> The debug trigger (sdtrig) capability is controlled using the debug property.
> The sdtrig is an ISA extension and should be treated so. The sdtrig extension
> may or may not be implemented in a system. Therefore, it must raise an illegal
> instruction exception when it is disabled and its CSRs are accessed.
>
> This patch removes the "debug" property and replaces it with ext_sdtrig 
> extension.
> It also raises an illegal instruction exception when the extension is 
> disabled and
> its CSRs are accessed.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  target/riscv/cpu.c        | 7 +++----
>  target/riscv/cpu_cfg.h    | 2 +-
>  target/riscv/cpu_helper.c | 2 +-
>  target/riscv/csr.c        | 2 +-
>  target/riscv/machine.c    | 2 +-
>  5 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..c770a7e506 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -909,7 +909,7 @@ static void riscv_cpu_reset_hold(Object *obj)
>      set_default_nan_mode(1, &env->fp_status);
>
>  #ifndef CONFIG_USER_ONLY
> -    if (cpu->cfg.debug) {
> +    if (cpu->cfg.ext_sdtrig) {
>          riscv_trigger_reset_hold(env);
>      }
>
> @@ -1068,7 +1068,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>      riscv_cpu_register_gdb_regs_for_features(cs);
>
>  #ifndef CONFIG_USER_ONLY
> -    if (cpu->cfg.debug) {
> +    if (cpu->cfg.ext_sdtrig) {
>          riscv_trigger_realize(&cpu->env);
>      }
>  #endif
> @@ -1393,6 +1393,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
>
>  /* These are experimental so mark with 'x-' */
>  const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
> +    MULTI_EXT_CFG_BOOL("x-sdtrig", ext_sdtrig, true),

Drop the "x-" because Sdtrig is already frozen and public review has started.

>      MULTI_EXT_CFG_BOOL("x-smaia", ext_smaia, false),
>      MULTI_EXT_CFG_BOOL("x-ssaia", ext_ssaia, false),
>
> @@ -1480,8 +1481,6 @@ Property riscv_cpu_options[] = {
>  };
>
>  static Property riscv_cpu_properties[] = {
> -    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
> -
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index f4605fb190..341ebf726a 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -109,6 +109,7 @@ struct RISCVCPUConfig {
>      bool ext_zvfbfwma;
>      bool ext_zvfh;
>      bool ext_zvfhmin;
> +    bool ext_sdtrig;
>      bool ext_smaia;
>      bool ext_ssaia;
>      bool ext_sscofpmf;
> @@ -145,7 +146,6 @@ struct RISCVCPUConfig {
>      uint16_t cboz_blocksize;
>      bool mmu;
>      bool pmp;
> -    bool debug;
>      bool misa_w;
>
>      bool short_isa_string;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e7e23b34f4..3f7c2f1315 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -126,7 +126,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>               ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED;
>      }
>
> -    if (cpu->cfg.debug && !icount_enabled()) {
> +    if (cpu->cfg.ext_sdtrig && !icount_enabled()) {
>          flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
>      }
>  #endif
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c50a33397c..8dbb49aa88 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -543,7 +543,7 @@ static RISCVException have_mseccfg(CPURISCVState *env, 
> int csrno)
>
>  static RISCVException debug(CPURISCVState *env, int csrno)
>  {
> -    if (riscv_cpu_cfg(env)->debug) {
> +    if (riscv_cpu_cfg(env)->ext_sdtrig) {
>          return RISCV_EXCP_NONE;
>      }
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 72fe2374dc..8f9787a30f 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -231,7 +231,7 @@ static bool debug_needed(void *opaque)
>  {
>      RISCVCPU *cpu = opaque;
>
> -    return cpu->cfg.debug;
> +    return cpu->cfg.ext_sdtrig;
>  }
>
>  static int debug_post_load(void *opaque, int version_id)
> --
> 2.34.1
>
>

Regards,
Anup