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

[Richard Henderson]

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~