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 <ruinl...@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~
