Re: [PATCH V2 0/2] Proposing custom CSR handling logic

Re: [PATCH V2 0/2] Proposing custom CSR handling logic

[Alistair Francis]

On Tue, May 11, 2021 at 8:07 PM Ruinland Chuan-Tzu Tsai
<ruinland@andestech.com> wrote:
>
> Hi all,
>
> My sincere apology that I missed the patch to include our own CSR table
> into the patch series and there were plenty of typos.
> Thus I'm sending out V2 of these tiny patches.
>
> I agree with Alistair's comment on not introducing intrusive code which
> will interfere the generic code structure. Yet since there are
> possibilities that some custom CSRs/instructions could be once drafted/
> proposed by vendors at first, and made themselves into the standard
> as the implementation become widely adopted.
>
> So in this patch set, we humbly utilzed a glib hash table for inserting
> the `struct riscv_custom_csr_operations`, check if the CSR is a non
> standard one, and then proceed the desired behavior.
>
> Once the non-standard CSRs make themselves into the specification,
> people could easily plug-and-use the code into CSR operation table
> inside `csr.c`.
>
> Ones may have concerns regarding the check code would introduce
> further overhead. For those considerations, I guess it could be solved
> by introducing a build option such as '--enable-riscv-vendor-features'
> to toggle the code.
>
> Cordially yours,
> Ruinland ChuanTzu Tsai
>
> Ruinland Chuan-Tzu Tsai (2):
>   Adding premliminary support for custom CSR handling mechanism
>   Adding custom Andes CSR table.

Thanks for the patches.

Can you please include:
 wangjunqiang@iscas.ac.cn
 qemu-devel@nongnu.org
 bin.meng@windriver.com

on future patches so everyone is included.

Alistair

>
>  target/riscv/cpu.c           |  28 ++++++++
>  target/riscv/cpu.h           |  12 +++-
>  target/riscv/cpu_bits.h      | 115 ++++++++++++++++++++++++++++++++
>  target/riscv/csr.c           | 107 ++++++++++++++++++++++++++++--
>  target/riscv/csr_andes.inc.c | 125 +++++++++++++++++++++++++++++++++++
>  5 files changed, 381 insertions(+), 6 deletions(-)
>  create mode 100644 target/riscv/csr_andes.inc.c
>
> --
> 2.17.1
>

Re: [PATCH V2 0/2] Proposing custom CSR handling logic

[Bin Meng]

On Wed, May 12, 2021 at 2:03 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 8:07 PM Ruinland Chuan-Tzu Tsai
> <ruinland@andestech.com> wrote:
> >
> > Hi all,
> >
> > My sincere apology that I missed the patch to include our own CSR table
> > into the patch series and there were plenty of typos.
> > Thus I'm sending out V2 of these tiny patches.
> >
> > I agree with Alistair's comment on not introducing intrusive code which
> > will interfere the generic code structure. Yet since there are
> > possibilities that some custom CSRs/instructions could be once drafted/
> > proposed by vendors at first, and made themselves into the standard
> > as the implementation become widely adopted.
> >
> > So in this patch set, we humbly utilzed a glib hash table for inserting
> > the `struct riscv_custom_csr_operations`, check if the CSR is a non
> > standard one, and then proceed the desired behavior.
> >
> > Once the non-standard CSRs make themselves into the specification,
> > people could easily plug-and-use the code into CSR operation table
> > inside `csr.c`.
> >
> > Ones may have concerns regarding the check code would introduce
> > further overhead. For those considerations, I guess it could be solved
> > by introducing a build option such as '--enable-riscv-vendor-features'
> > to toggle the code.
> >
> > Cordially yours,
> > Ruinland ChuanTzu Tsai
> >
> > Ruinland Chuan-Tzu Tsai (2):
> >   Adding premliminary support for custom CSR handling mechanism
> >   Adding custom Andes CSR table.
>
> Thanks for the patches.
>
> Can you please include:
>  wangjunqiang@iscas.ac.cn
>  qemu-devel@nongnu.org
>  bin.meng@windriver.com
>
> on future patches so everyone is included.

Thanks Alistair!

>
> >
> >  target/riscv/cpu.c           |  28 ++++++++
> >  target/riscv/cpu.h           |  12 +++-
> >  target/riscv/cpu_bits.h      | 115 ++++++++++++++++++++++++++++++++
> >  target/riscv/csr.c           | 107 ++++++++++++++++++++++++++++--
> >  target/riscv/csr_andes.inc.c | 125 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 381 insertions(+), 6 deletions(-)

I didn't see the original patch set in the ML, nor does it show up on
patchwork. I wonder is it posted on the ML?

Regards,
Bin

Re: [PATCH V2 0/2] Proposing custom CSR handling logic

[Alistair Francis]

On Thu, May 13, 2021 at 12:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, May 12, 2021 at 2:03 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, May 11, 2021 at 8:07 PM Ruinland Chuan-Tzu Tsai
> > <ruinland@andestech.com> wrote:
> > >
> > > Hi all,
> > >
> > > My sincere apology that I missed the patch to include our own CSR table
> > > into the patch series and there were plenty of typos.
> > > Thus I'm sending out V2 of these tiny patches.
> > >
> > > I agree with Alistair's comment on not introducing intrusive code which
> > > will interfere the generic code structure. Yet since there are
> > > possibilities that some custom CSRs/instructions could be once drafted/
> > > proposed by vendors at first, and made themselves into the standard
> > > as the implementation become widely adopted.
> > >
> > > So in this patch set, we humbly utilzed a glib hash table for inserting
> > > the `struct riscv_custom_csr_operations`, check if the CSR is a non
> > > standard one, and then proceed the desired behavior.
> > >
> > > Once the non-standard CSRs make themselves into the specification,
> > > people could easily plug-and-use the code into CSR operation table
> > > inside `csr.c`.
> > >
> > > Ones may have concerns regarding the check code would introduce
> > > further overhead. For those considerations, I guess it could be solved
> > > by introducing a build option such as '--enable-riscv-vendor-features'
> > > to toggle the code.
> > >
> > > Cordially yours,
> > > Ruinland ChuanTzu Tsai
> > >
> > > Ruinland Chuan-Tzu Tsai (2):
> > >   Adding premliminary support for custom CSR handling mechanism
> > >   Adding custom Andes CSR table.
> >
> > Thanks for the patches.
> >
> > Can you please include:
> >  wangjunqiang@iscas.ac.cn
> >  qemu-devel@nongnu.org
> >  bin.meng@windriver.com
> >
> > on future patches so everyone is included.
>
> Thanks Alistair!
>
> >
> > >
> > >  target/riscv/cpu.c           |  28 ++++++++
> > >  target/riscv/cpu.h           |  12 +++-
> > >  target/riscv/cpu_bits.h      | 115 ++++++++++++++++++++++++++++++++
> > >  target/riscv/csr.c           | 107 ++++++++++++++++++++++++++++--
> > >  target/riscv/csr_andes.inc.c | 125 +++++++++++++++++++++++++++++++++++
> > >  5 files changed, 381 insertions(+), 6 deletions(-)
>
> I didn't see the original patch set in the ML, nor does it show up on
> patchwork. I wonder is it posted on the ML?

It might not have. It is an early version and needs some more work,
but it does seem to be on the right track. I left a few comments but
hopefully the next version will make it to the mailing list.

I will CC you on the response so you can see.

Alistair

>
> Regards,
> Bin

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

[Alistair Francis]

On Wed, May 12, 2021 at 4:16 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> 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>

+ Others so everyone knows what is going on.

Alistair

>
> 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
> >