[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v4 2/4] Adding basic custom/vendor CSR handling mechanism
|
From: |
Bin Meng |
|
Subject: |
Re: [RFC PATCH v4 2/4] Adding basic custom/vendor CSR handling mechanism |
|
Date: |
Fri, 6 Aug 2021 11:35:36 +0800 |
+Alistair
On Fri, Aug 6, 2021 at 1:58 AM Ruinland Chuan-Tzu Tsai
<ruinland@andestech.com> wrote:
>
> From: Ruinalnd ChuanTzu Tsai <ruinland@andestech.com>
>
> For now we add a custom CSR handling mechanism to handle non-standard CSR read
> or write.
>
> The write_stub() and read_zero() are provided for quick placeholder usage if
> such CSRs' behavior are expected to fail-over in its user code.
>
> Signed-off-by: Dylan Jhong <dylan@andestech.com>
> ---
> target/riscv/cpu.c | 23 ++++++++++
> target/riscv/cpu.h | 31 ++++++++++++-
> target/riscv/cpu_bits.h | 4 ++
> target/riscv/csr.c | 83 ++++++++++++++++++++++++++++------
> target/riscv/custom_cpu_bits.h | 8 ++++
> 5 files changed, 134 insertions(+), 15 deletions(-)
> create mode 100644 target/riscv/custom_cpu_bits.h
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7401325..3a638b5 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -144,6 +144,29 @@ static void set_resetvec(CPURISCVState *env,
> target_ulong resetvec)
> #endif
> }
>
> +#if defined(CONFIG_RISCV_CUSTOM)
> +static void setup_custom_csr(CPURISCVState *env,
> + riscv_custom_csr_operations csr_map_struct[]
> + ) {
{ should be put to the next line, per QEMU coding convention. Please
fix this globally in this series.
> +
> + env->custom_csr_map = g_hash_table_new_full(g_direct_hash, \
> + g_direct_equal, \
> + NULL, NULL);
Is it possible to juse use g_hash_table_new() directly, with both 2
parameters being NULL, given you don't provide the destroy hooks?
like:
env->custom_csr_map = g_hash_table_new(NULL, NULL);
> +
> +
> + int i;
nits: please move this to the beginning of this function.
> + 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;
break? I think we should continue the loop.
> + }
> + }
> +}
> +#endif
> +
> 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 0edb282..52df9bb 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -239,6 +239,16 @@ struct CPURISCVState {
>
> /* Fields from here on are preserved across CPU reset. */
> QEMUTimer *timer; /* Internal timer */
> +
> + /*
> + * The reason why we have an opset map for custom CSRs and a seperated
> + * storage map is that we might have heterogeneous architecture, in which
> + * different harts have different custom CSRs.
> + * Custom CSR opset map
> + */
> + GHashTable *custom_csr_map;
> + /* Custom CSR val holder */
> + void *custom_csr_val;
> };
>
> OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
> @@ -485,17 +495,36 @@ typedef struct {
> riscv_csr_op_fn op;
> } riscv_csr_operations;
>
> +typedef struct {
> + int csrno;
> + riscv_csr_operations csr_opset;
> + } riscv_custom_csr_operations;
} should be put in the beginning without space. Please fix this
globally in this series.
> +
> +/*
> + * The reason why we have an abstraction here is that : We could have CSR
> + * number M on hart A is an alias of CSR number N on hart B. So we make a
> + * CSR number to value address map.
> + */
> +typedef struct {
> + int csrno;
> + target_ulong val;
> + } riscv_custom_csr_vals;
> +
It looks this struct is not used by any patch in this series?
> /* CSR function table constants */
> enum {
> - CSR_TABLE_SIZE = 0x1000
> + CSR_TABLE_SIZE = 0x1000,
> + MAX_CUSTOM_CSR_NUM = 100
To be consistent, name this to be: CUSTOM_CSR_TABLE_SIZE
> };
>
> /* CSR function table */
> +extern int andes_custom_csr_size;
> +extern riscv_custom_csr_operations
> andes_custom_csr_table[MAX_CUSTOM_CSR_NUM];
The above 2 should not be in this patch.
> extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
>
> void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops);
> void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
>
> +
This is unnecessary.
> void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
>
> #endif /* RISCV_CPU_H */
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index caf4599..de77242 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -593,3 +593,7 @@
> #define MIE_SSIE (1 << IRQ_S_SOFT)
> #define MIE_USIE (1 << IRQ_U_SOFT)
> #endif
> +
> +#if defined(CONFIG_RISCV_CUSTOM)
> +#include "custom_cpu_bits.h"
> +#endif
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fd2e636..1c4dc94 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -137,7 +137,8 @@ static int ctr32(CPURISCVState *env, int csrno)
> return ctr(env, csrno);
> }
>
> -#if !defined(CONFIG_USER_ONLY)
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wunused-function"
Use __attribute__((__unused__)), so we don't need to use GCC push/pop
> static int any(CPURISCVState *env, int csrno)
> {
> return 0;
> @@ -152,6 +153,25 @@ static int any32(CPURISCVState *env, int csrno)
> return any(env, csrno);
>
> }
> +#pragma GCC diagnostic pop
> +
> +/* Machine Information Registers */
> +static int read_zero(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> + return *val = 0;
> +}
> +
> +/*
> + * XXX: This is just a write stub for developing custom CSR handler,
Remove XXX
> + * if the behavior of writting such CSR is not presentable in QEMU and
> doesn't
typo: writing.
Is that present, or presentable?
> + * affect the functionality, just stub it.
> + */
> +static int write_stub(CPURISCVState *env, int csrno, target_ulong val)
> +{
> + return 0;
> +}
> +
> +#if !defined(CONFIG_USER_ONLY)
>
> static int smode(CPURISCVState *env, int csrno)
> {
> @@ -435,11 +455,6 @@ static const char valid_vm_1_10_64[16] = {
> [VM_1_10_SV57] = 1
> };
>
> -/* Machine Information Registers */
> -static int read_zero(CPURISCVState *env, int csrno, target_ulong *val)
> -{
> - return *val = 0;
> -}
>
> static int read_mhartid(CPURISCVState *env, int csrno, target_ulong *val)
> {
> @@ -1264,6 +1279,18 @@ static int write_pmpaddr(CPURISCVState *env, int
> csrno, target_ulong val)
>
> #endif
>
> +
> +#if defined(CONFIG_RISCV_CUSTOM)
> +/* Custom CSR related routines and data structures */
> +
> +static gpointer is_custom_csr(CPURISCVState *env, int csrno)
The function name suggests that the return value should be of bool
type. Suggest we do:
static bool is_custom_csr(CPURISCVState *env, int csrno,
riscv_custom_csr_operations *custom_csr_ops)
> +{
> + gpointer ret;
> + ret = g_hash_table_lookup(env->custom_csr_map, GINT_TO_POINTER(csrno));
> + return ret;
> +}
> +#endif
> +
> /*
> * riscv_csrrw - read and/or update control and status register
> *
> @@ -1273,12 +1300,19 @@ static int write_pmpaddr(CPURISCVState *env, int
> csrno, target_ulong val)
> * csrrc <-> riscv_csrrw(env, csrno, ret_value, 0, value);
> */
>
> +
> +
Unnecessary changes
> int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
> target_ulong new_value, target_ulong write_mask)
> {
> int ret;
> target_ulong old_value;
> RISCVCPU *cpu = env_archcpu(env);
> + #if !defined(CONFIG_RISCV_CUSTOM)
Please make sure #if starts from the beginning of this line, without space ahead
> + riscv_csr_operations *csrop = &csr_ops[csrno];
nits: name this variable to csr_ops
> + #else
> + riscv_csr_operations *csrop;
> + #endif
>
> /* check privileges and return -1 if check fails */
> #if !defined(CONFIG_USER_ONLY)
> @@ -1300,6 +1334,7 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> (!env->debugger && (effective_priv < get_field(csrno, 0x300)))) {
> return -RISCV_EXCP_ILLEGAL_INST;
> }
> +
> #endif
>
> /* ensure the CSR extension is enabled. */
> @@ -1307,27 +1342,43 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> return -RISCV_EXCP_ILLEGAL_INST;
> }
>
> + /* try handle_custom_csr */
> +
> + #if defined(CONFIG_RISCV_CUSTOM)
> + if (unlikely(env->custom_csr_map != NULL)) {
> + riscv_csr_operations *custom_csr_opset = (riscv_csr_operations *)
> + is_custom_csr(env, csrno);
> + if (NULL != custom_csr_opset) {
> + csrop = custom_csr_opset;
> + } else {
> + csrop = &csr_ops[csrno];
> + }
> + } else {
> + csrop = &csr_ops[csrno];
> + }
> + #endif
> +
> /* check predicate */
> - if (!csr_ops[csrno].predicate) {
> + if (!csrop->predicate) {
> return -RISCV_EXCP_ILLEGAL_INST;
> }
> - ret = csr_ops[csrno].predicate(env, csrno);
> + ret = csrop->predicate(env, csrno);
> if (ret < 0) {
> 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 (csrop->op) {
> + return csrop->op(env, csrno, ret_value, new_value, write_mask);
> }
>
> /* if no accessor exists then return failure */
> - if (!csr_ops[csrno].read) {
> + if (!csrop->read) {
> return -RISCV_EXCP_ILLEGAL_INST;
> }
>
> /* read old value */
> - ret = csr_ops[csrno].read(env, csrno, &old_value);
> + ret = csrop->read(env, csrno, &old_value);
> if (ret < 0) {
> return ret;
> }
> @@ -1335,8 +1386,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> /* 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 (csrop->write) {
> + ret = csrop->write(env, csrno, new_value);
> if (ret < 0) {
> return ret;
> }
> @@ -1369,6 +1420,10 @@ int riscv_csrrw_debug(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> return ret;
> }
>
> +#if defined(CONFIG_RISCV_CUSTOM)
> +/* Include the custom CSR table here. */
nits: remove the ending .
> +#endif
> +
> /* Control and Status Register function table */
> riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> /* User Floating-Point CSRs */
> diff --git a/target/riscv/custom_cpu_bits.h b/target/riscv/custom_cpu_bits.h
> new file mode 100644
> index 0000000..5df31f8
> --- /dev/null
> +++ b/target/riscv/custom_cpu_bits.h
> @@ -0,0 +1,8 @@
> +/*
> + * RISC-V cpu bits for custom CSR logic.
> + *
> + * Copyright (c) 2021 Andes Technology Corp.
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/* This file is intentionally left blank at this commit. */
nits: remove the ending .
%s/at/in
Regards,
Bin