Re: [PATCH 20/26] target/riscv: generalize custom CSR functionality

[Alistair Francis]

On Mon, May 12, 2025 at 7:54 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> While at it, constify it so that the RISCVCSR array in RISCVCPUDef
> can also be const.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.h    | 15 ++++++++++++---
>  target/riscv/cpu.c    | 25 ++++++++++++++++++++++++-
>  target/riscv/csr.c    |  2 +-
>  target/riscv/th_csr.c | 21 +++------------------
>  4 files changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 8a8d61f8099..f68af8cb124 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -537,6 +537,8 @@ struct ArchCPU {
>      const GPtrArray *decoders;
>  };
>
> +typedef struct RISCVCSR RISCVCSR;
> +
>  typedef struct RISCVCPUDef {
>      RISCVMXL misa_mxl_max;  /* max mxl for this cpu */
>      RISCVCPUProfile *profile;
> @@ -545,6 +547,7 @@ typedef struct RISCVCPUDef {
>      int32_t vext_spec;
>      RISCVCPUConfig cfg;
>      bool bare;
> +    const RISCVCSR *custom_csrs;
>  } RISCVCPUDef;
>
>  /**
> @@ -891,6 +894,12 @@ typedef struct {
>      uint32_t min_priv_ver;
>  } riscv_csr_operations;
>
> +struct RISCVCSR {
> +    int csrno;
> +    bool (*insertion_test)(RISCVCPU *cpu);
> +    riscv_csr_operations csr_ops;
> +};
> +
>  /* CSR function table constants */
>  enum {
>      CSR_TABLE_SIZE = 0x1000
> @@ -945,7 +954,7 @@ extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
>  extern const bool valid_vm_1_10_32[], valid_vm_1_10_64[];
>
>  void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops);
> -void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
> +void riscv_set_csr_ops(int csrno, const riscv_csr_operations *ops);
>
>  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
>
> @@ -954,8 +963,8 @@ target_ulong riscv_new_csr_seed(target_ulong new_value,
>
>  const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
>
> -/* Implemented in th_csr.c */
> -void th_register_custom_csrs(RISCVCPU *cpu);
> +/* In th_csr.c */
> +extern const RISCVCSR th_csr_list[];
>
>  const char *priv_spec_to_str(int priv_version);
>  #endif /* RISCV_CPU_H */
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 171f27fdd87..bc45815383d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -485,6 +485,19 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
>  }
>  #endif
>
> +#ifndef CONFIG_USER_ONLY
> +static void riscv_register_custom_csrs(RISCVCPU *cpu, const RISCVCSR 
> *csr_list)
> +{
> +    for (size_t i = 0; csr_list[i].csr_ops.name; i++) {
> +        int csrno = csr_list[i].csrno;
> +        const riscv_csr_operations *csr_ops = &csr_list[i].csr_ops;
> +        if (!csr_list[i].insertion_test || csr_list[i].insertion_test(cpu)) {
> +            riscv_set_csr_ops(csrno, csr_ops);
> +        }
> +    }
> +}
> +#endif
> +
>  #if defined(TARGET_RISCV64)
>  static void rv64_thead_c906_cpu_init(Object *obj)
>  {
> @@ -511,7 +524,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
>      cpu->cfg.mvendorid = THEAD_VENDOR_ID;
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(cpu, VM_1_10_SV39);
> -    th_register_custom_csrs(cpu);
> +    riscv_register_custom_csrs(cpu, th_csr_list);
>  #endif
>
>      /* inherited from parent obj via riscv_cpu_init() */
> @@ -1304,6 +1317,11 @@ static void riscv_cpu_init(Object *obj)
>      if (mcc->def->vext_spec != RISCV_PROFILE_ATTR_UNUSED) {
>          cpu->env.vext_ver = mcc->def->vext_spec;
>      }
> +#ifndef CONFIG_USER_ONLY
> +    if (mcc->def->custom_csrs) {
> +        riscv_register_custom_csrs(cpu, mcc->def->custom_csrs);
> +    }
> +#endif
>  }
>
>  typedef struct misa_ext_info {
> @@ -2906,6 +2924,11 @@ static void riscv_cpu_class_base_init(ObjectClass *c, 
> const void *data)
>          mcc->def->misa_ext |= def->misa_ext;
>
>          riscv_cpu_cfg_merge(&mcc->def->cfg, &def->cfg);
> +
> +        if (def->custom_csrs) {
> +            assert(!mcc->def->custom_csrs);
> +            mcc->def->custom_csrs = def->custom_csrs;
> +        }
>      }
>
>      if (!object_class_is_abstract(c)) {
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 74760f98cca..833d06237ae 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -38,7 +38,7 @@ void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops)
>      *ops = csr_ops[csrno & (CSR_TABLE_SIZE - 1)];
>  }
>
> -void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> +void riscv_set_csr_ops(int csrno, const riscv_csr_operations *ops)
>  {
>      csr_ops[csrno & (CSR_TABLE_SIZE - 1)] = *ops;
>  }
> diff --git a/target/riscv/th_csr.c b/target/riscv/th_csr.c
> index 969a9fe3c80..49eb7bbab5f 100644
> --- a/target/riscv/th_csr.c
> +++ b/target/riscv/th_csr.c
> @@ -27,12 +27,6 @@
>  #define TH_SXSTATUS_MAEE        BIT(21)
>  #define TH_SXSTATUS_THEADISAEE  BIT(22)
>
> -typedef struct {
> -    int csrno;
> -    bool (*insertion_test)(RISCVCPU *cpu);
> -    riscv_csr_operations csr_ops;
> -} riscv_csr;
> -
>  static RISCVException smode(CPURISCVState *env, int csrno)
>  {
>      if (riscv_has_ext(env, RVS)) {
> @@ -55,20 +49,11 @@ static RISCVException read_th_sxstatus(CPURISCVState 
> *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> -static riscv_csr th_csr_list[] = {
> +const RISCVCSR th_csr_list[] = {
>      {
>          .csrno = CSR_TH_SXSTATUS,
>          .insertion_test = test_thead_mvendorid,
>          .csr_ops = { "th.sxstatus", smode, read_th_sxstatus }
> -    }
> +    },
> +    { }
>  };
> -void th_register_custom_csrs(RISCVCPU *cpu)
> -{
> -    for (size_t i = 0; i < ARRAY_SIZE(th_csr_list); i++) {
> -        int csrno = th_csr_list[i].csrno;
> -        riscv_csr_operations *csr_ops = &th_csr_list[i].csr_ops;
> -        if (!th_csr_list[i].insertion_test || 
> th_csr_list[i].insertion_test(cpu)) {
> -            riscv_set_csr_ops(csrno, csr_ops);
> -        }
> -    }
> -}
> --
> 2.49.0
>