qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V2 1/2] Adding premliminary support for custom CSR handling m


From: Alistair Francis
Subject: Re: [PATCH V2 1/2] Adding premliminary support for custom CSR handling mechanism
Date: Wed, 12 May 2021 16:16:56 +1000

On Tue, May 11, 2021 at 8:07 PM Ruinland Chuan-Tzu Tsai
<ruinland@andestech.com> wrote:
>
> Introduce ax25 and custom CSR handling mechanism to RISC-V platform.
> This is just a POC in which we add Andes custom CSR table directly
> into the generic code which is undresiable and requires overhaul.
>
> Signed-off-by: Dylan Jhong <dylan@andestech.com>

Thanks for the patch.

This seems like a good start. I have left some comments inline.

Why do we need a hash table? Couldn't we just have a second
riscv_csr_operations array?

> ---
>  target/riscv/cpu.c      |  28 ++++++++++
>  target/riscv/cpu.h      |  12 ++++-
>  target/riscv/cpu_bits.h | 115 ++++++++++++++++++++++++++++++++++++++++
>  target/riscv/csr.c      | 107 +++++++++++++++++++++++++++++++++++--
>  4 files changed, 256 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7401325..6dbe9d9 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,8 @@
>
>  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
>
> +GHashTable * custom_csr_map;

This should be part of CPURISCVState instead of a global

> +
>  const char * const riscv_int_regnames[] = {
>    "x0/zero", "x1/ra",  "x2/sp",  "x3/gp",  "x4/tp",  "x5/t0",   "x6/t1",
>    "x7/t2",   "x8/s0",  "x9/s1",  "x10/a0", "x11/a1", "x12/a2",  "x13/a3",
> @@ -159,6 +161,31 @@ static void rv64_base_cpu_init(Object *obj)
>      set_misa(env, RV64);
>  }
>
> +static void ax25_cpu_init(Object *obj)
> +{
> +    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> +    set_priv_version(env, PRIV_VERSION_1_10_0);
> +
> +    /* setup custom csr handler hash table */
> +    setup_custom_csr();
> +
> +}
> +
> +void setup_custom_csr(void) {
> +    custom_csr_map = g_hash_table_new_full(g_direct_hash, g_direct_equal, 
> NULL, NULL);
> +    int i;
> +    for (i = 0; i < MAX_CUSTOM_CSR_NUM; i++) {
> +        if (andes_custom_csr_table[i].csrno != 0) {
> +            g_hash_table_insert(custom_csr_map,
> +                GINT_TO_POINTER(andes_custom_csr_table[i].csrno),
> +                &andes_custom_csr_table[i].csr_opset);
> +        } else {
> +            break;
> +        }
> +    }
> +}
> +
>  static void rv64_sifive_u_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> @@ -705,6 +732,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
>      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32_sifive_u_cpu_init),
>  #elif defined(TARGET_RISCV64)
>      DEFINE_CPU(TYPE_RISCV_CPU_BASE64,           rv64_base_cpu_init),
> +    DEFINE_CPU(TYPE_RISCV_CPU_AX25,             ax25_cpu_init),

Let's add the CPU in a separate patch.

>      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51,       rv64_sifive_e_cpu_init),
>      DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U54,       rv64_sifive_u_cpu_init),
>  #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0edb282..a2f656c 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -37,6 +37,7 @@
>  #define TYPE_RISCV_CPU_ANY              RISCV_CPU_TYPE_NAME("any")
>  #define TYPE_RISCV_CPU_BASE32           RISCV_CPU_TYPE_NAME("rv32")
>  #define TYPE_RISCV_CPU_BASE64           RISCV_CPU_TYPE_NAME("rv64")
> +#define TYPE_RISCV_CPU_AX25             RISCV_CPU_TYPE_NAME("andes-ax25")
>  #define TYPE_RISCV_CPU_IBEX             RISCV_CPU_TYPE_NAME("lowrisc-ibex")
>  #define TYPE_RISCV_CPU_SIFIVE_E31       RISCV_CPU_TYPE_NAME("sifive-e31")
>  #define TYPE_RISCV_CPU_SIFIVE_E34       RISCV_CPU_TYPE_NAME("sifive-e34")
> @@ -485,16 +486,25 @@ typedef struct {
>      riscv_csr_op_fn op;
>  } riscv_csr_operations;
>
> +typedef struct {
> +    int csrno;
> +    riscv_csr_operations csr_opset;
> +    } riscv_custom_csr_operations;
> +
>  /* CSR function table constants */
>  enum {
> -    CSR_TABLE_SIZE = 0x1000
> +    CSR_TABLE_SIZE = 0x1000,
> +    MAX_CUSTOM_CSR_NUM = 100
>  };
>
>  /* CSR function table */
> +extern riscv_custom_csr_operations 
> andes_custom_csr_table[MAX_CUSTOM_CSR_NUM];
>  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> +extern GHashTable *custom_csr_map;
>
>  void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops);
>  void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
> +void setup_custom_csr(void);
>
>  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index caf4599..639bc0a 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -259,6 +259,7 @@
>  #define CSR_TDATA1          0x7a1
>  #define CSR_TDATA2          0x7a2
>  #define CSR_TDATA3          0x7a3
> +#define CSR_TINFO           0x7a4

Why add this?

>
>  /* Debug Mode Registers */
>  #define CSR_DCSR            0x7b0
> @@ -593,3 +594,117 @@
>  #define MIE_SSIE                           (1 << IRQ_S_SOFT)
>  #define MIE_USIE                           (1 << IRQ_U_SOFT)
>  #endif
> +
> +/* ========= AndeStar V5 machine mode CSRs ========= */
> +/* Configuration Registers */
> +#define CSR_MICM_CFG            0xfc0
> +#define CSR_MDCM_CFG            0xfc1
> +#define CSR_MMSC_CFG            0xfc2
> +#define CSR_MMSC_CFG2           0xfc3
> +#define CSR_MVEC_CFG            0xfc7
> +
> +/* Crash Debug CSRs */
> +#define CSR_MCRASH_STATESAVE    0xfc8
> +#define CSR_MSTATUS_CRASHSAVE   0xfc9
> +
> +/* Memory CSRs */
> +#define CSR_MILMB               0x7c0
> +#define CSR_MDLMB               0x7c1
> +#define CSR_MECC_CODE           0x7C2
> +#define CSR_MNVEC               0x7c3
> +#define CSR_MCACHE_CTL          0x7ca
> +#define CSR_MCCTLBEGINADDR      0x7cb
> +#define CSR_MCCTLCOMMAND        0x7cc
> +#define CSR_MCCTLDATA           0x7cd
> +#define CSR_MPPIB               0x7f0
> +#define CSR_MFIOB               0x7f1
> +
> +/* Hardware Stack Protection & Recording */
> +#define CSR_MHSP_CTL            0x7c6
> +#define CSR_MSP_BOUND           0x7c7
> +#define CSR_MSP_BASE            0x7c8
> +#define CSR_MXSTATUS            0x7c4
> +#define CSR_MDCAUSE             0x7c9
> +#define CSR_MSLIDELEG           0x7d5
> +#define CSR_MSAVESTATUS         0x7d6
> +#define CSR_MSAVEEPC1           0x7d7
> +#define CSR_MSAVECAUSE1         0x7d8
> +#define CSR_MSAVEEPC2           0x7d9
> +#define CSR_MSAVECAUSE2         0x7da
> +#define CSR_MSAVEDCAUSE1        0x7db
> +#define CSR_MSAVEDCAUSE2        0x7dc
> +
> +/* Control CSRs */
> +#define CSR_MPFT_CTL            0x7c5
> +#define CSR_MMISC_CTL           0x7d0
> +#define CSR_MCLK_CTL            0x7df
> +
> +/* Counter related CSRs */
> +#define CSR_MCOUNTERWEN         0x7ce
> +#define CSR_MCOUNTERINTEN       0x7cf
> +#define CSR_MCOUNTERMASK_M      0x7d1
> +#define CSR_MCOUNTERMASK_S      0x7d2
> +#define CSR_MCOUNTERMASK_U      0x7d3
> +#define CSR_MCOUNTEROVF         0x7d4
> +
> +/* Enhanced CLIC CSRs */
> +#define CSR_MIRQ_ENTRY          0x7ec
> +#define CSR_MINTSEL_JAL         0x7ed
> +#define CSR_PUSHMCAUSE          0x7ee
> +#define CSR_PUSHMEPC            0x7ef
> +#define CSR_PUSHMXSTATUS        0x7eb
> +
> +/* Andes Physical Memory Attribute(PMA) CSRs */
> +#define CSR_PMACFG0             0xbc0
> +#define CSR_PMACFG1             0xbc1
> +#define CSR_PMACFG2             0xbc2
> +#define CSR_PMACFG3             0xbc3
> +#define CSR_PMAADDR0            0xbd0
> +#define CSR_PMAADDR1            0xbd1
> +#define CSR_PMAADDR2            0xbd2
> +#define CSR_PMAADDR3            0xbd2
> +#define CSR_PMAADDR4            0xbd4
> +#define CSR_PMAADDR5            0xbd5
> +#define CSR_PMAADDR6            0xbd6
> +#define CSR_PMAADDR7            0xbd7
> +#define CSR_PMAADDR8            0xbd8
> +#define CSR_PMAADDR9            0xbd9
> +#define CSR_PMAADDR10           0xbda
> +#define CSR_PMAADDR11           0xbdb
> +#define CSR_PMAADDR12           0xbdc
> +#define CSR_PMAADDR13           0xbdd
> +#define CSR_PMAADDR14           0xbde
> +#define CSR_PMAADDR15           0xbdf
> +
> +/* ========= AndeStar V5 supervisor mode CSRs ========= */
> +/* Supervisor trap registers */
> +#define CSR_SLIE                0x9c4
> +#define CSR_SLIP                0x9c5
> +#define CSR_SDCAUSE             0x9c9
> +
> +/* Supervisor counter registers */
> +#define CSR_SCOUNTERINTEN       0x9cf
> +#define CSR_SCOUNTERMASK_M      0x9d1
> +#define CSR_SCOUNTERMASK_S      0x9d2
> +#define CSR_SCOUNTERMASK_U      0x9d3
> +#define CSR_SCOUNTEROVF         0x9d4
> +#define CSR_SCOUNTINHIBIT       0x9e0
> +#define CSR_SHPMEVENT3          0x9e3
> +#define CSR_SHPMEVENT4          0x9e4
> +#define CSR_SHPMEVENT5          0x9e5
> +#define CSR_SHPMEVENT6          0x9e6
> +
> +/* Supervisor control registers */
> +#define CSR_SCCTLDATA           0x9cd
> +#define CSR_SMISC_CTL           0x9d0
> +
> +/* ========= AndeStar V5 user mode CSRs ========= */
> +/* User mode control registers */
> +#define CSR_UITB                0x800
> +#define CSR_UCODE               0x801
> +#define CSR_UDCAUSE             0x809
> +#define CSR_UCCTLBEGINADDR      0x80b
> +#define CSR_UCCTLCOMMAND        0x80c
> +#define CSR_WFE                 0x810
> +#define CSR_SLEEPVALUE          0x811
> +#define CSR_TXEVT               0x812

Ideally this should be a seperate file

> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fd2e636..b81efcf 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -523,6 +523,14 @@ static int read_misa(CPURISCVState *env, int csrno, 
> target_ulong *val)
>      return 0;
>  }
>
> +
> +// XXX: This is just a write stub for developing custom CSR handler,
> +// if the behavior of writting such CSR is not presentable in QEMU and 
> doesn't
> +// affect the functionality, just stub it.
> +static int write_stub(CPURISCVState *env, int csrno, target_ulong val) {
> +    return 0;
> +}

Is this needed?

> +
>  static int write_misa(CPURISCVState *env, int csrno, target_ulong val)
>  {
>      if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
> @@ -1264,6 +1272,76 @@ static int write_pmpaddr(CPURISCVState *env, int 
> csrno, target_ulong val)
>
>  #endif
>
> +
> +/* Custom CSR related routines and data structures */
> +
> +gpointer is_custom_csr(int);

Just make this function static instead of having a declaration in the C file.

> +
> +gpointer is_custom_csr(int csrno) {
> +    gpointer ret;
> +    ret = g_hash_table_lookup(custom_csr_map, GINT_TO_POINTER(csrno));
> +    return ret;
> +    }
> +
> +int try_handle_custom_csr(CPURISCVState *env, int csrno,
> +    target_ulong *ret_value, target_ulong new_value, target_ulong write_mask,
> +    riscv_csr_operations *opset);
> +
> +// XXX: This part is mainly duplicate from riscv_csrrw, we need to redo the 
> logic
> +int try_handle_custom_csr(CPURISCVState *env, int csrno,
> +    target_ulong *ret_value, target_ulong new_value, target_ulong write_mask,
> +    riscv_csr_operations *opset) {
> +
> +    int ret = 0;
> +    target_ulong old_value;
> +
> +    /* check predicate */
> +    if (!opset->predicate) {
> +        return -RISCV_EXCP_ILLEGAL_INST;
> +    }
> +    ret = opset->predicate(env, csrno);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* execute combined read/write operation if it exists */
> +    if (opset->op) {
> +        return opset->op(env, csrno, ret_value, new_value, write_mask);
> +    }
> +
> +    /* if no accessor exists then return failure */
> +    if (!opset->read) {
> +        return -RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    /* read old value */
> +    ret = opset->read(env, csrno, &old_value);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* write value if writable and write mask set, otherwise drop writes */
> +    if (write_mask) {
> +        new_value = (old_value & ~write_mask) | (new_value & write_mask);
> +        if (opset->write) {
> +            ret = opset->write(env, csrno, new_value);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +    }
> +
> +    /* return old value */
> +    if (ret_value) {
> +        *ret_value = old_value;
> +    }
> +
> +    return 0;
> +
> +
> +
> +    }

It would be nice if we could reuse the existing CSR access function.
Could we make the current one more generic and just call it?

> +
>  /*
>   * riscv_csrrw - read and/or update control and status register
>   *
> @@ -1283,7 +1361,7 @@ int riscv_csrrw(CPURISCVState *env, int csrno, 
> target_ulong *ret_value,
>      /* check privileges and return -1 if check fails */
>  #if !defined(CONFIG_USER_ONLY)
>      int effective_priv = env->priv;
> -    int read_only = get_field(csrno, 0xC00) == 3;
> +    /* int read_only = get_field(csrno, 0xC00) == 3; */

Don't comment this out.

>
>      if (riscv_has_ext(env, RVH) &&
>          env->priv == PRV_S &&
> @@ -1296,10 +1374,12 @@ int riscv_csrrw(CPURISCVState *env, int csrno, 
> target_ulong *ret_value,
>          effective_priv++;
>      }
>
> -    if ((write_mask && read_only) ||
> -        (!env->debugger && (effective_priv < get_field(csrno, 0x300)))) {
> -        return -RISCV_EXCP_ILLEGAL_INST;
> -    }
> +    /*
> +     * if ((write_mask && read_only) ||
> +     *   (!env->debugger && (effective_priv < get_field(csrno, 0x300)))) {
> +     *   return -RISCV_EXCP_ILLEGAL_INST;
> +     * }
> +     */
>  #endif
>
>      /* ensure the CSR extension is enabled. */
> @@ -1307,6 +1387,12 @@ int riscv_csrrw(CPURISCVState *env, int csrno, 
> target_ulong *ret_value,
>          return -RISCV_EXCP_ILLEGAL_INST;
>      }
>
> +    /* try handle_custom_csr */
> +    riscv_csr_operations *custom_csr_opset = (riscv_csr_operations *) 
> is_custom_csr(csrno);
> +    if(NULL != custom_csr_opset) {
> +        return try_handle_custom_csr(env, csrno, ret_value, new_value, 
> write_mask, custom_csr_opset);
> +    }
> +
>      /* check predicate */
>      if (!csr_ops[csrno].predicate) {
>          return -RISCV_EXCP_ILLEGAL_INST;
> @@ -1351,6 +1437,14 @@ int riscv_csrrw(CPURISCVState *env, int csrno, 
> target_ulong *ret_value,
>      return 0;
>  }
>
> +/* Andes Custom Registers */
> +static int read_mmsc_cfg(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    /* enable pma probe */
> +    *val = 0x40000000;
> +    return 0;
> +}
> +
>  /*
>   * Debugger support.  If not in user mode, set env->debugger before the
>   * riscv_csrrw call and clear it after the call.
> @@ -1369,6 +1463,8 @@ int riscv_csrrw_debug(CPURISCVState *env, int csrno, 
> target_ulong *ret_value,
>      return ret;
>  }
>
> +#include "csr_andes.inc.c"

This doesn't seem to be used.

> +
>  /* Control and Status Register function table */
>  riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>      /* User Floating-Point CSRs */
> @@ -1645,3 +1741,4 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>      [CSR_MHPMCOUNTER31H] = { "mhpmcounter31h", any32,  read_zero },
>  #endif /* !CONFIG_USER_ONLY */
>  };
> +
> --
> 2.17.1
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]