[RFC,v5,2/3] riscv: Introduce custom CSR hooks to riscv_csrrw()

riscv_csrrw() will be called by CSR handling helpers, which is the
most suitable place for checking wheter a custom CSR is being accessed.

If we're touching a custom CSR, invoke the registered handlers.

Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com>
---
 target/riscv/cpu.c             | 19 +++++++++++++++++
 target/riscv/cpu.h             | 13 +++++++++++-
 target/riscv/csr.c             | 38 +++++++++++++++++++++++++++-------
 target/riscv/custom_csr_defs.h |  7 +++++++
 4 files changed, 68 insertions(+), 9 deletions(-)
 create mode 100644 target/riscv/custom_csr_defs.h

On Fri, Oct 22, 2021 at 1:13 AM Ruinland Chuan-Tzu Tsai
<ruinland@andestech.com> wrote:
>
> riscv_csrrw() will be called by CSR handling helpers, which is the
> most suitable place for checking wheter a custom CSR is being accessed.
>
> If we're touching a custom CSR, invoke the registered handlers.
>
> Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com>

Awesome! This looks really good :)

> ---
>  target/riscv/cpu.c             | 19 +++++++++++++++++
>  target/riscv/cpu.h             | 13 +++++++++++-
>  target/riscv/csr.c             | 38 +++++++++++++++++++++++++++-------
>  target/riscv/custom_csr_defs.h |  7 +++++++
>  4 files changed, 68 insertions(+), 9 deletions(-)
>  create mode 100644 target/riscv/custom_csr_defs.h
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 0c93b7edd7..a72fd32f01 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,9 @@
>
>  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
>
> +GHashTable *custom_csr_map = NULL;
> +#include "custom_csr_defs.h"
> +
>  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",
> @@ -149,6 +152,22 @@ static void set_resetvec(CPURISCVState *env, target_ulong resetvec)
>  #endif
>  }
>
> +static void setup_custom_csr(CPURISCVState *env,
> +                             riscv_custom_csr_operations csr_map_struct[])
> +{
> +    int i;
> +    env->custom_csr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
> +    for (i = 0; i < MAX_CUSTOM_CSR_NUM; i++) {
> +        if (csr_map_struct[i].csrno != 0) {
> +            g_hash_table_insert(env->custom_csr_map,
> +                GINT_TO_POINTER(csr_map_struct[i].csrno),
> +                &csr_map_struct[i].csr_opset);
> +        } else {
> +            break;
> +        }
> +    }
> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 3bef0d1265..012be10d0a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -245,6 +245,11 @@ struct CPURISCVState {
>
>      /* Fields from here on are preserved across CPU reset. */
>      QEMUTimer *timer; /* Internal timer */
> +
> +    /* Custom CSR opset table per hart */
> +    GHashTable *custom_csr_map;
> +    /* Custom CSR value holder per hart */
> +    void *custom_csr_val;
>  };
>
>  OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
> @@ -496,9 +501,15 @@ 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 */
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 23fbbd3216..1048ee3b44 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1403,6 +1403,14 @@ static RISCVException write_pmpaddr(CPURISCVState *env, int csrno,
>
>  #endif
>
> +/* Custom CSR related routines */
> +static gpointer find_custom_csr(CPURISCVState *env, int csrno)
> +{
> +    gpointer ret;
> +    ret = g_hash_table_lookup(env->custom_csr_map, GINT_TO_POINTER(csrno));
> +    return ret;

You can just return directly here, so:

return g_hash_table_lookup(env->custom_csr_map, GINT_TO_POINTER(csrno));

Also, I think you need to run checkpatch.pl on this patch (you should
run it on all patches).

Alistair

> +}
> +
>  /*
>   * riscv_csrrw - read and/or update control and status register
>   *
> @@ -1419,6 +1427,7 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>      RISCVException ret;
>      target_ulong old_value;
>      RISCVCPU *cpu = env_archcpu(env);
> +    riscv_csr_operations *csr_op;
>      int read_only = get_field(csrno, 0xC00) == 3;
>
>      /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> @@ -1449,26 +1458,39 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>
> +    /* try to handle_custom_csr */
> +    if (unlikely(env->custom_csr_map != NULL)) {
> +        riscv_csr_operations *custom_csr_opset = (riscv_csr_operations *)
> +            find_custom_csr(env, csrno);
> +        if (custom_csr_opset != NULL) {
> +            csr_op = custom_csr_opset;
> +            } else {
> +            csr_op = &csr_ops[csrno];
> +            }
> +        } else {
> +        csr_op = &csr_ops[csrno];
> +        }
> +
>      /* check predicate */
> -    if (!csr_ops[csrno].predicate) {
> +    if (!csr_op->predicate) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
> -    ret = csr_ops[csrno].predicate(env, csrno);
> +    ret = csr_op->predicate(env, csrno);
>      if (ret != RISCV_EXCP_NONE) {
>          return ret;
>      }
>
>      /* execute combined read/write operation if it exists */
> -    if (csr_ops[csrno].op) {
> -        return csr_ops[csrno].op(env, csrno, ret_value, new_value, write_mask);
> +    if (csr_op->op) {
> +        return csr_op->op(env, csrno, ret_value, new_value, write_mask);
>      }
>
>      /* if no accessor exists then return failure */
> -    if (!csr_ops[csrno].read) {
> +    if (!csr_op->read) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>      /* read old value */
> -    ret = csr_ops[csrno].read(env, csrno, &old_value);
> +    ret = csr_op->read(env, csrno, &old_value);
>      if (ret != RISCV_EXCP_NONE) {
>          return ret;
>      }
> @@ -1476,8 +1498,8 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>      /* 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 (csr_ops[csrno].write) {
> -            ret = csr_ops[csrno].write(env, csrno, new_value);
> +        if (csr_op->write) {
> +            ret = csr_op->write(env, csrno, new_value);
>              if (ret != RISCV_EXCP_NONE) {
>                  return ret;
>              }
> diff --git a/target/riscv/custom_csr_defs.h b/target/riscv/custom_csr_defs.h
> new file mode 100644
> index 0000000000..4dbf8cf1fd
> --- /dev/null
> +++ b/target/riscv/custom_csr_defs.h
> @@ -0,0 +1,7 @@
> +/*
> + * Copyright (c) 2021 Andes Technology Corp.
> + * SPDX-License-Identifier: GPL-2.0+
> + * Custom CSR variables provided by <cpu_model_name>_csr.c
> + */
> +
> +/* Left blank purposely in this commit. */
> --
> 2.25.1
>

On 10/21/21 8:09 AM, Ruinland Chuan-Tzu Tsai wrote:
> riscv_csrrw() will be called by CSR handling helpers, which is the
> most suitable place for checking wheter a custom CSR is being accessed.
> 
> If we're touching a custom CSR, invoke the registered handlers.
> 
> Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com>
> ---
>   target/riscv/cpu.c             | 19 +++++++++++++++++
>   target/riscv/cpu.h             | 13 +++++++++++-
>   target/riscv/csr.c             | 38 +++++++++++++++++++++++++++-------
>   target/riscv/custom_csr_defs.h |  7 +++++++
>   4 files changed, 68 insertions(+), 9 deletions(-)
>   create mode 100644 target/riscv/custom_csr_defs.h
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 0c93b7edd7..a72fd32f01 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,9 @@
>   
>   static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
>   
> +GHashTable *custom_csr_map = NULL;

Leftover from a previous revision?

> +#include "custom_csr_defs.h"

I think that the few declarations that are required can just go in internals.h.

> +
>   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",
> @@ -149,6 +152,22 @@ static void set_resetvec(CPURISCVState *env, target_ulong resetvec)
>   #endif
>   }
>   
> +static void setup_custom_csr(CPURISCVState *env,
> +                             riscv_custom_csr_operations csr_map_struct[])

Why is this static?  Surely it needs to be called from csr_andes.c, etc?
Oh, I see that in the next patch you call this directly from ax25_cpu_init.

I think the split of declarations is less than ideal.  See below.

> +{
> +    int i;
> +    env->custom_csr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
> +    for (i = 0; i < MAX_CUSTOM_CSR_NUM; i++) {

Having a hard maximum seems a mistake.  You should pass in the array size...

> +        if (csr_map_struct[i].csrno != 0) {
> +            g_hash_table_insert(env->custom_csr_map,
> +                GINT_TO_POINTER(csr_map_struct[i].csrno),
> +                &csr_map_struct[i].csr_opset);
> +        } else {
> +            break;
> +        }

... which would also allow you to remove the terminator in the data, and the check here.

> @@ -245,6 +245,11 @@ struct CPURISCVState {
>   
>       /* Fields from here on are preserved across CPU reset. */
>       QEMUTimer *timer; /* Internal timer */
> +
> +    /* Custom CSR opset table per hart */
> +    GHashTable *custom_csr_map;

I think placing this in the CPURISCVState is incorrect.  A much better place would be on 
the RISCVCPUClass, so that there is exactly one copy of this per cpu type, i.e. all 
A25/AX25 cpus would share the same table.

You would of course need to hook class_init, which the current definition of DEFINE_CPU 
does not allow.  But that's easy to fix.

> +    /* Custom CSR value holder per hart */
> +    void *custom_csr_val;
>   };

Value singular?  Anyhow, I think that it's a mistake trying to hide the value structure in 
another file.  It complicates allocation of the CPURISCVState, and you have no mechanism 
by which to migrate the csr values.

I think you should simply place your structure here in CPURISCVState.

> +static gpointer find_custom_csr(CPURISCVState *env, int csrno)
> +{
> +    gpointer ret;
> +    ret = g_hash_table_lookup(env->custom_csr_map, GINT_TO_POINTER(csrno));
> +    return ret;
> +}

Fix the return type; the csr is not gpointer.
It would be better to put the map not null test here.

I think it would be even better to name this find_csr,
and include the normal index of csr_ops[] if the map
test fails.

> @@ -1449,26 +1458,39 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
>           return RISCV_EXCP_ILLEGAL_INST;
>       }
>   
> +    /* try to handle_custom_csr */
> +    if (unlikely(env->custom_csr_map != NULL)) {
> +        riscv_csr_operations *custom_csr_opset = (riscv_csr_operations *)
> +            find_custom_csr(env, csrno);
> +        if (custom_csr_opset != NULL) {
> +            csr_op = custom_csr_opset;
> +            } else {
> +            csr_op = &csr_ops[csrno];
> +            }
> +        } else {
> +        csr_op = &csr_ops[csrno];
> +        }

As Alistair noted, run checkpatch.pl to find all of these indentation errors.

That said, a better structure here would be

     csr_op = find_csr(env, csrno);
     if (csr_op == NULL) {
         return RISCV_EXCP_ILLEGAL_INST;
     }


r~

On Thu, Oct 21, 2021 at 05:08:09PM -0700, Richard Henderson wrote:
> On 10/21/21 8:09 AM, Ruinland Chuan-Tzu Tsai wrote:
> > riscv_csrrw() will be called by CSR handling helpers, which is the
> > most suitable place for checking wheter a custom CSR is being accessed.
> > 
> > If we're touching a custom CSR, invoke the registered handlers.
> > 
> > Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com>
> > ---
> >   target/riscv/cpu.c             | 19 +++++++++++++++++
> >   target/riscv/cpu.h             | 13 +++++++++++-
> >   target/riscv/csr.c             | 38 +++++++++++++++++++++++++++-------
> >   target/riscv/custom_csr_defs.h |  7 +++++++
> >   4 files changed, 68 insertions(+), 9 deletions(-)
> >   create mode 100644 target/riscv/custom_csr_defs.h
> > 
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 0c93b7edd7..a72fd32f01 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -34,6 +34,9 @@
> >   static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> > +GHashTable *custom_csr_map = NULL;
> 
> Leftover from a previous revision?

By default there's no custom_csr_map (pointing to NULL) and thus only the
custom CSR equipped CPU models will need to initialize that. Standard CPU will
pass the check of custom_csr_map in riscv_csrrw() by default.

> 
> > +#include "custom_csr_defs.h"
> 
> I think that the few declarations that are required can just go in internals.h.

Wilco.

> 
> > +
> >   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",
> > @@ -149,6 +152,22 @@ static void set_resetvec(CPURISCVState *env, target_ulong resetvec)
> >   #endif
> >   }
> > +static void setup_custom_csr(CPURISCVState *env,
> > +                             riscv_custom_csr_operations csr_map_struct[])
> 
> Why is this static?  Surely it needs to be called from csr_andes.c, etc?
> Oh, I see that in the next patch you call this directly from ax25_cpu_init.
> 
> I think the split of declarations is less than ideal.  See below.
> 
> > +{
> > +    int i;
> > +    env->custom_csr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
> > +    for (i = 0; i < MAX_CUSTOM_CSR_NUM; i++) {
> 
> Having a hard maximum seems a mistake.  You should pass in the array size...
> 
> > +        if (csr_map_struct[i].csrno != 0) {
> > +            g_hash_table_insert(env->custom_csr_map,
> > +                GINT_TO_POINTER(csr_map_struct[i].csrno),
> > +                &csr_map_struct[i].csr_opset);
> > +        } else {
> > +            break;
> > +        }
> 
> ... which would also allow you to remove the terminator in the data, and the check here.

Wilco.

> 
> > @@ -245,6 +245,11 @@ struct CPURISCVState {
> >       /* Fields from here on are preserved across CPU reset. */
> >       QEMUTimer *timer; /* Internal timer */
> > +
> > +    /* Custom CSR opset table per hart */
> > +    GHashTable *custom_csr_map;
> 
> I think placing this in the CPURISCVState is incorrect.  A much better place
> would be on the RISCVCPUClass, so that there is exactly one copy of this per
> cpu type, i.e. all A25/AX25 cpus would share the same table.
> 
> You would of course need to hook class_init, which the current definition of
> DEFINE_CPU does not allow.  But that's easy to fix.
> 
> > +    /* Custom CSR value holder per hart */
> > +    void *custom_csr_val;
> >   };
> 
> Value singular?  Anyhow, I think that it's a mistake trying to hide the
> value structure in another file.  It complicates allocation of the
> CPURISCVState, and you have no mechanism by which to migrate the csr values.

What I'm trying to do here is to provide a hook for CSR value storage and let
it being set during CPU initilization. We have heterogeneous harts which
have different sets of CSRs.

> 
> I think you should simply place your structure here in CPURISCVState.
> 
> > +static gpointer find_custom_csr(CPURISCVState *env, int csrno)
> > +{
> > +    gpointer ret;
> > +    ret = g_hash_table_lookup(env->custom_csr_map, GINT_TO_POINTER(csrno));
> > +    return ret;
> > +}
> 
> Fix the return type; the csr is not gpointer.
> It would be better to put the map not null test here.
> 
> I think it would be even better to name this find_csr,
> and include the normal index of csr_ops[] if the map
> test fails.

Wilco.

> 
> > @@ -1449,26 +1458,39 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> >           return RISCV_EXCP_ILLEGAL_INST;
> >       }
> > +    /* try to handle_custom_csr */
> > +    if (unlikely(env->custom_csr_map != NULL)) {
> > +        riscv_csr_operations *custom_csr_opset = (riscv_csr_operations *)
> > +            find_custom_csr(env, csrno);
> > +        if (custom_csr_opset != NULL) {
> > +            csr_op = custom_csr_opset;
> > +            } else {
> > +            csr_op = &csr_ops[csrno];
> > +            }
> > +        } else {
> > +        csr_op = &csr_ops[csrno];
> > +        }
> 
> As Alistair noted, run checkpatch.pl to find all of these indentation errors.
> 
> That said, a better structure here would be
> 
>     csr_op = find_csr(env, csrno);
>     if (csr_op == NULL) {
>         return RISCV_EXCP_ILLEGAL_INST;
>     }

Thanks for the tips. Wilco.
> 
> 
> r~

Cordially yours,
Ruinland

On Fri, Oct 22, 2021 at 08:43:08AM +1000, Alistair Francis wrote:
> On Fri, Oct 22, 2021 at 1:13 AM Ruinland Chuan-Tzu Tsai
> <ruinland@andestech.com> wrote:
> >
> > riscv_csrrw() will be called by CSR handling helpers, which is the
> > most suitable place for checking wheter a custom CSR is being accessed.
> >
> > If we're touching a custom CSR, invoke the registered handlers.
> >
> > Signed-off-by: Ruinland Chuan-Tzu Tsai <ruinland@andestech.com>
> 
> Awesome! This looks really good :)
> 
> > ---
> >  target/riscv/cpu.c             | 19 +++++++++++++++++
> >  target/riscv/cpu.h             | 13 +++++++++++-
> >  target/riscv/csr.c             | 38 +++++++++++++++++++++++++++-------
> >  target/riscv/custom_csr_defs.h |  7 +++++++
> >  4 files changed, 68 insertions(+), 9 deletions(-)
> >  create mode 100644 target/riscv/custom_csr_defs.h
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 0c93b7edd7..a72fd32f01 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -34,6 +34,9 @@
> >
> >  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> >
> > +GHashTable *custom_csr_map = NULL;
> > +#include "custom_csr_defs.h"
> > +
> >  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",
> > @@ -149,6 +152,22 @@ static void set_resetvec(CPURISCVState *env, target_ulong resetvec)
> >  #endif
> >  }
> >
> > +static void setup_custom_csr(CPURISCVState *env,
> > +                             riscv_custom_csr_operations csr_map_struct[])
> > +{
> > +    int i;
> > +    env->custom_csr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
> > +    for (i = 0; i < MAX_CUSTOM_CSR_NUM; i++) {
> > +        if (csr_map_struct[i].csrno != 0) {
> > +            g_hash_table_insert(env->custom_csr_map,
> > +                GINT_TO_POINTER(csr_map_struct[i].csrno),
> > +                &csr_map_struct[i].csr_opset);
> > +        } else {
> > +            break;
> > +        }
> > +    }
> > +}
> > +
> >  static void riscv_any_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 3bef0d1265..012be10d0a 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -245,6 +245,11 @@ struct CPURISCVState {
> >
> >      /* Fields from here on are preserved across CPU reset. */
> >      QEMUTimer *timer; /* Internal timer */
> > +
> > +    /* Custom CSR opset table per hart */
> > +    GHashTable *custom_csr_map;
> > +    /* Custom CSR value holder per hart */
> > +    void *custom_csr_val;
> >  };
> >
> >  OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
> > @@ -496,9 +501,15 @@ 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 */
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 23fbbd3216..1048ee3b44 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1403,6 +1403,14 @@ static RISCVException write_pmpaddr(CPURISCVState *env, int csrno,
> >
> >  #endif
> >
> > +/* Custom CSR related routines */
> > +static gpointer find_custom_csr(CPURISCVState *env, int csrno)
> > +{
> > +    gpointer ret;
> > +    ret = g_hash_table_lookup(env->custom_csr_map, GINT_TO_POINTER(csrno));
> > +    return ret;
> 
> You can just return directly here, so:
> 
> return g_hash_table_lookup(env->custom_csr_map, GINT_TO_POINTER(csrno));
> 
> Also, I think you need to run checkpatch.pl on this patch (you should
> run it on all patches).

Wilco.
Thanks for the tips.
And sorry for my negligence.

Cordially yours,
Ruinland

> 
> Alistair
> 
> > +}
> > +
> >  /*
> >   * riscv_csrrw - read and/or update control and status register
> >   *
> > @@ -1419,6 +1427,7 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> >      RISCVException ret;
> >      target_ulong old_value;
> >      RISCVCPU *cpu = env_archcpu(env);
> > +    riscv_csr_operations *csr_op;
> >      int read_only = get_field(csrno, 0xC00) == 3;
> >
> >      /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> > @@ -1449,26 +1458,39 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> >
> > +    /* try to handle_custom_csr */
> > +    if (unlikely(env->custom_csr_map != NULL)) {
> > +        riscv_csr_operations *custom_csr_opset = (riscv_csr_operations *)
> > +            find_custom_csr(env, csrno);
> > +        if (custom_csr_opset != NULL) {
> > +            csr_op = custom_csr_opset;
> > +            } else {
> > +            csr_op = &csr_ops[csrno];
> > +            }
> > +        } else {
> > +        csr_op = &csr_ops[csrno];
> > +        }
> > +
> >      /* check predicate */
> > -    if (!csr_ops[csrno].predicate) {
> > +    if (!csr_op->predicate) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> > -    ret = csr_ops[csrno].predicate(env, csrno);
> > +    ret = csr_op->predicate(env, csrno);
> >      if (ret != RISCV_EXCP_NONE) {
> >          return ret;
> >      }
> >
> >      /* execute combined read/write operation if it exists */
> > -    if (csr_ops[csrno].op) {
> > -        return csr_ops[csrno].op(env, csrno, ret_value, new_value, write_mask);
> > +    if (csr_op->op) {
> > +        return csr_op->op(env, csrno, ret_value, new_value, write_mask);
> >      }
> >
> >      /* if no accessor exists then return failure */
> > -    if (!csr_ops[csrno].read) {
> > +    if (!csr_op->read) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> >      /* read old value */
> > -    ret = csr_ops[csrno].read(env, csrno, &old_value);
> > +    ret = csr_op->read(env, csrno, &old_value);
> >      if (ret != RISCV_EXCP_NONE) {
> >          return ret;
> >      }
> > @@ -1476,8 +1498,8 @@ RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> >      /* 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 (csr_ops[csrno].write) {
> > -            ret = csr_ops[csrno].write(env, csrno, new_value);
> > +        if (csr_op->write) {
> > +            ret = csr_op->write(env, csrno, new_value);
> >              if (ret != RISCV_EXCP_NONE) {
> >                  return ret;
> >              }
> > diff --git a/target/riscv/custom_csr_defs.h b/target/riscv/custom_csr_defs.h
> > new file mode 100644
> > index 0000000000..4dbf8cf1fd
> > --- /dev/null
> > +++ b/target/riscv/custom_csr_defs.h
> > @@ -0,0 +1,7 @@
> > +/*
> > + * Copyright (c) 2021 Andes Technology Corp.
> > + * SPDX-License-Identifier: GPL-2.0+
> > + * Custom CSR variables provided by <cpu_model_name>_csr.c
> > + */
> > +
> > +/* Left blank purposely in this commit. */
> > --
> > 2.25.1
> >

On 10/22/21 1:34 AM, Ruinland ChuanTzu Tsai wrote:
>>> +    /* Custom CSR value holder per hart */
>>> +    void *custom_csr_val;
>>>    };
>>
>> Value singular?  Anyhow, I think that it's a mistake trying to hide the
>> value structure in another file.  It complicates allocation of the
>> CPURISCVState, and you have no mechanism by which to migrate the csr values.
> 
> What I'm trying to do here is to provide a hook for CSR value storage and let
> it being set during CPU initilization. We have heterogeneous harts which
> have different sets of CSRs.

I understand that, but the common CPURISCVState should contain the storage for all of the 
CSRs for every possible hart.

I might have made a different call if we had a more object-y language, but we have C.  The 
difference in size (here exactly one word) is not worth the complication of splitting 
structures apart.


r~