Re: [PATCH 2/4] target/riscv: Add CSR name in the CSR function table
On Fri, Jan 15, 2021 at 5:35 AM Alexander Richardson
wrote:
>
> On Tue, 12 Jan 2021 at 05:02, Bin Meng wrote:
> >
> > From: Bin Meng
> >
> > In preparation to generate the CSR register list for GDB stub
> > dynamically, let's add the CSR name in the CSR function table.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > target/riscv/cpu.h | 1 +
> > target/riscv/csr.c | 332
> > +++--
> > 2 files changed, 249 insertions(+), 84 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 6f9e1cc..6684316 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -476,6 +476,7 @@ typedef int (*riscv_csr_op_fn)(CPURISCVState *env, int
> > csrno,
> > target_ulong *ret_value, target_ulong new_value, target_ulong
> > write_mask);
> >
> > typedef struct {
> > +const char *name;
> > riscv_csr_predicate_fn predicate;
> > riscv_csr_read_fn read;
> > riscv_csr_write_fn write;
>
> In our CHERI fork, we also added the name to this table for better
> instruction logging output:
>
> We used some macros to avoid repeating the same string multiple times:
> in that patch we use e.g. [CSR_FCSR] = CSR_OP_RW(fs, fcsr)," instead
> of
> "[CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr },"
> Would it make sense to upstream these helper macros? This would
> significantly reduce merge conflicts on our side in the future.
If you send the patches then I'll merge it :)
In general it would be great if you upstream as much as possible. That
will save you work in the long run and help improve the code base.
Alistair
>
> Thanks,
> Alex
>
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 507e8ee..fd2e636 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1372,112 +1372,276 @@ int riscv_csrrw_debug(CPURISCVState *env, int
> > csrno, target_ulong *ret_value,
> > /* Control and Status Register function table */
> > riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> > /* User Floating-Point CSRs */
> > -[CSR_FFLAGS] = { fs, read_fflags, write_fflags
> > },
> > -[CSR_FRM] = { fs, read_frm, write_frm
> > },
> > -[CSR_FCSR] ={ fs, read_fcsr,write_fcsr
> > },
> > +[CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
> > +[CSR_FRM] = { "frm", fs, read_frm, write_frm},
> > +[CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr },
> > /* Vector CSRs */
> > -[CSR_VSTART] = { vs, read_vstart, write_vstart
> > },
> > -[CSR_VXSAT] = { vs, read_vxsat, write_vxsat
> > },
> > -[CSR_VXRM] ={ vs, read_vxrm,write_vxrm
> > },
> > -[CSR_VL] = { vs, read_vl
> > },
> > -[CSR_VTYPE] = { vs, read_vtype
> > },
> > +[CSR_VSTART] = { "vstart", vs, read_vstart, write_vstart },
> > +[CSR_VXSAT]= { "vxsat",vs, read_vxsat, write_vxsat },
> > +[CSR_VXRM] = { "vxrm", vs, read_vxrm,write_vxrm },
> > +[CSR_VL] = { "vl", vs, read_vl},
> > +[CSR_VTYPE]= { "vtype",vs, read_vtype },
> > /* User Timers and Counters */
> > -[CSR_CYCLE] = { ctr, read_instret
> > },
> > -[CSR_INSTRET] = { ctr, read_instret
> > },
> > -[CSR_CYCLEH] = { ctr32, read_instreth
> > },
> > -[CSR_INSTRETH] ={ ctr32, read_instreth
> > },
> > -
> > -/* In privileged mode, the monitor will have to emulate TIME CSRs only
> > if
> > - * rdtime callback is not provided by machine/platform emulation */
> > -[CSR_TIME] ={ ctr, read_time
> > },
> > -[CSR_TIMEH] = { ctr32, read_timeh
> > },
> > +[CSR_CYCLE]= { "cycle",ctr,read_instret },
> > +[CSR_INSTRET] = { "instret", ctr,read_instret },
> > +[CSR_CYCLEH] = { "cycleh", ctr32, read_instreth },
> > +[CSR_INSTRETH] = { "instreth", ctr32, read_instreth },
> > +
> > +/*
> > + * In privileged mode, the monitor will have to emulate TIME CSRs only
> > if
> > + * rdtime callback is not provided by machine/platform emulation.
> > + */
> > +[CSR_TIME] = { "time", ctr, read_time },
> > +[CSR_TIMEH] = { "timeh", ctr32, read_timeh },
> >
> > #if !defined(CONFIG_USER_ONLY)
> > /* Machine Timers and Counters */
> >
Re: [PATCH 2/4] target/riscv: Add CSR name in the CSR function table
On Mon, Jan 11, 2021 at 9:05 PM Bin Meng wrote:
>
> From: Bin Meng
>
> In preparation to generate the CSR register list for GDB stub
> dynamically, let's add the CSR name in the CSR function table.
>
> Signed-off-by: Bin Meng
Reviewed-by: Alistair Francis
Alistair
> ---
>
> target/riscv/cpu.h | 1 +
> target/riscv/csr.c | 332
> +++--
> 2 files changed, 249 insertions(+), 84 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6f9e1cc..6684316 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -476,6 +476,7 @@ typedef int (*riscv_csr_op_fn)(CPURISCVState *env, int
> csrno,
> target_ulong *ret_value, target_ulong new_value, target_ulong
> write_mask);
>
> typedef struct {
> +const char *name;
> riscv_csr_predicate_fn predicate;
> riscv_csr_read_fn read;
> riscv_csr_write_fn write;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 507e8ee..fd2e636 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1372,112 +1372,276 @@ int riscv_csrrw_debug(CPURISCVState *env, int
> csrno, target_ulong *ret_value,
> /* Control and Status Register function table */
> riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> /* User Floating-Point CSRs */
> -[CSR_FFLAGS] = { fs, read_fflags, write_fflags
> },
> -[CSR_FRM] = { fs, read_frm, write_frm
> },
> -[CSR_FCSR] ={ fs, read_fcsr,write_fcsr
> },
> +[CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
> +[CSR_FRM] = { "frm", fs, read_frm, write_frm},
> +[CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr },
> /* Vector CSRs */
> -[CSR_VSTART] = { vs, read_vstart, write_vstart
> },
> -[CSR_VXSAT] = { vs, read_vxsat, write_vxsat
> },
> -[CSR_VXRM] ={ vs, read_vxrm,write_vxrm
> },
> -[CSR_VL] = { vs, read_vl
> },
> -[CSR_VTYPE] = { vs, read_vtype
> },
> +[CSR_VSTART] = { "vstart", vs, read_vstart, write_vstart },
> +[CSR_VXSAT]= { "vxsat",vs, read_vxsat, write_vxsat },
> +[CSR_VXRM] = { "vxrm", vs, read_vxrm,write_vxrm },
> +[CSR_VL] = { "vl", vs, read_vl},
> +[CSR_VTYPE]= { "vtype",vs, read_vtype },
> /* User Timers and Counters */
> -[CSR_CYCLE] = { ctr, read_instret
> },
> -[CSR_INSTRET] = { ctr, read_instret
> },
> -[CSR_CYCLEH] = { ctr32, read_instreth
> },
> -[CSR_INSTRETH] ={ ctr32, read_instreth
> },
> -
> -/* In privileged mode, the monitor will have to emulate TIME CSRs only if
> - * rdtime callback is not provided by machine/platform emulation */
> -[CSR_TIME] ={ ctr, read_time
> },
> -[CSR_TIMEH] = { ctr32, read_timeh
> },
> +[CSR_CYCLE]= { "cycle",ctr,read_instret },
> +[CSR_INSTRET] = { "instret", ctr,read_instret },
> +[CSR_CYCLEH] = { "cycleh", ctr32, read_instreth },
> +[CSR_INSTRETH] = { "instreth", ctr32, read_instreth },
> +
> +/*
> + * In privileged mode, the monitor will have to emulate TIME CSRs only if
> + * rdtime callback is not provided by machine/platform emulation.
> + */
> +[CSR_TIME] = { "time", ctr, read_time },
> +[CSR_TIMEH] = { "timeh", ctr32, read_timeh },
>
> #if !defined(CONFIG_USER_ONLY)
> /* Machine Timers and Counters */
> -[CSR_MCYCLE] = { any, read_instret
> },
> -[CSR_MINSTRET] ={ any, read_instret
> },
> -[CSR_MCYCLEH] = { any32, read_instreth
> },
> -[CSR_MINSTRETH] = { any32, read_instreth
> },
> +[CSR_MCYCLE]= { "mcycle",any, read_instret },
> +[CSR_MINSTRET] = { "minstret", any, read_instret },
> +[CSR_MCYCLEH] = { "mcycleh", any32, read_instreth },
> +[CSR_MINSTRETH] = { "minstreth", any32, read_instreth },
>
> /* Machine Information Registers */
> -[CSR_MVENDORID] = { any, read_zero
> },
> -[CSR_MARCHID] = { any, read_zero
> },
> -[CSR_MIMPID] = { any, read_zero
> },
> -[CSR_MHARTID] = { any, read_mhartid
> },
> +[CSR_MVENDORID] = { "mvendorid", any, read_zero},
> +
Re: [PATCH 2/4] target/riscv: Add CSR name in the CSR function table
Hi Alex,
On Fri, Jan 15, 2021 at 9:14 PM Alexander Richardson
wrote:
>
> On Tue, 12 Jan 2021 at 05:02, Bin Meng wrote:
> >
> > From: Bin Meng
> >
> > In preparation to generate the CSR register list for GDB stub
> > dynamically, let's add the CSR name in the CSR function table.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > target/riscv/cpu.h | 1 +
> > target/riscv/csr.c | 332
> > +++--
> > 2 files changed, 249 insertions(+), 84 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 6f9e1cc..6684316 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -476,6 +476,7 @@ typedef int (*riscv_csr_op_fn)(CPURISCVState *env, int
> > csrno,
> > target_ulong *ret_value, target_ulong new_value, target_ulong
> > write_mask);
> >
> > typedef struct {
> > +const char *name;
> > riscv_csr_predicate_fn predicate;
> > riscv_csr_read_fn read;
> > riscv_csr_write_fn write;
>
> In our CHERI fork, we also added the name to this table for better
> instruction logging output:
>
Thanks for the info.
> We used some macros to avoid repeating the same string multiple times:
> in that patch we use e.g. [CSR_FCSR] = CSR_OP_RW(fs, fcsr)," instead
> of
> "[CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr },"
> Would it make sense to upstream these helper macros? This would
> significantly reduce merge conflicts on our side in the future.
>
Anyway there will be a merge conflict. So the question is: do you guys
want to upstream the CSR logging changes you mentioned in your fork?
If not, I am not sure if it brings enough value to just upstream the
macros but maybe others have a different view.
If the answer is yes, then whoever upstreams the macro changes has to
deal with that unfortunately :(
Regards,
Bin
Re: [PATCH 2/4] target/riscv: Add CSR name in the CSR function table
On Tue, 12 Jan 2021 at 05:02, Bin Meng wrote:
>
> From: Bin Meng
>
> In preparation to generate the CSR register list for GDB stub
> dynamically, let's add the CSR name in the CSR function table.
>
> Signed-off-by: Bin Meng
> ---
>
> target/riscv/cpu.h | 1 +
> target/riscv/csr.c | 332
> +++--
> 2 files changed, 249 insertions(+), 84 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6f9e1cc..6684316 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -476,6 +476,7 @@ typedef int (*riscv_csr_op_fn)(CPURISCVState *env, int
> csrno,
> target_ulong *ret_value, target_ulong new_value, target_ulong
> write_mask);
>
> typedef struct {
> +const char *name;
> riscv_csr_predicate_fn predicate;
> riscv_csr_read_fn read;
> riscv_csr_write_fn write;
In our CHERI fork, we also added the name to this table for better
instruction logging output:
We used some macros to avoid repeating the same string multiple times:
in that patch we use e.g. [CSR_FCSR] = CSR_OP_RW(fs, fcsr)," instead
of
"[CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr },"
Would it make sense to upstream these helper macros? This would
significantly reduce merge conflicts on our side in the future.
Thanks,
Alex
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 507e8ee..fd2e636 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1372,112 +1372,276 @@ int riscv_csrrw_debug(CPURISCVState *env, int
> csrno, target_ulong *ret_value,
> /* Control and Status Register function table */
> riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> /* User Floating-Point CSRs */
> -[CSR_FFLAGS] = { fs, read_fflags, write_fflags
> },
> -[CSR_FRM] = { fs, read_frm, write_frm
> },
> -[CSR_FCSR] ={ fs, read_fcsr,write_fcsr
> },
> +[CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
> +[CSR_FRM] = { "frm", fs, read_frm, write_frm},
> +[CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr },
> /* Vector CSRs */
> -[CSR_VSTART] = { vs, read_vstart, write_vstart
> },
> -[CSR_VXSAT] = { vs, read_vxsat, write_vxsat
> },
> -[CSR_VXRM] ={ vs, read_vxrm,write_vxrm
> },
> -[CSR_VL] = { vs, read_vl
> },
> -[CSR_VTYPE] = { vs, read_vtype
> },
> +[CSR_VSTART] = { "vstart", vs, read_vstart, write_vstart },
> +[CSR_VXSAT]= { "vxsat",vs, read_vxsat, write_vxsat },
> +[CSR_VXRM] = { "vxrm", vs, read_vxrm,write_vxrm },
> +[CSR_VL] = { "vl", vs, read_vl},
> +[CSR_VTYPE]= { "vtype",vs, read_vtype },
> /* User Timers and Counters */
> -[CSR_CYCLE] = { ctr, read_instret
> },
> -[CSR_INSTRET] = { ctr, read_instret
> },
> -[CSR_CYCLEH] = { ctr32, read_instreth
> },
> -[CSR_INSTRETH] ={ ctr32, read_instreth
> },
> -
> -/* In privileged mode, the monitor will have to emulate TIME CSRs only if
> - * rdtime callback is not provided by machine/platform emulation */
> -[CSR_TIME] ={ ctr, read_time
> },
> -[CSR_TIMEH] = { ctr32, read_timeh
> },
> +[CSR_CYCLE]= { "cycle",ctr,read_instret },
> +[CSR_INSTRET] = { "instret", ctr,read_instret },
> +[CSR_CYCLEH] = { "cycleh", ctr32, read_instreth },
> +[CSR_INSTRETH] = { "instreth", ctr32, read_instreth },
> +
> +/*
> + * In privileged mode, the monitor will have to emulate TIME CSRs only if
> + * rdtime callback is not provided by machine/platform emulation.
> + */
> +[CSR_TIME] = { "time", ctr, read_time },
> +[CSR_TIMEH] = { "timeh", ctr32, read_timeh },
>
> #if !defined(CONFIG_USER_ONLY)
> /* Machine Timers and Counters */
> -[CSR_MCYCLE] = { any, read_instret
> },
> -[CSR_MINSTRET] ={ any, read_instret
> },
> -[CSR_MCYCLEH] = { any32, read_instreth
> },
> -[CSR_MINSTRETH] = { any32, read_instreth
> },
> +[CSR_MCYCLE]= { "mcycle",any, read_instret },
> +[CSR_MINSTRET] = { "minstret", any, read_instret },
> +
[PATCH 2/4] target/riscv: Add CSR name in the CSR function table
From: Bin Meng
In preparation to generate the CSR register list for GDB stub
dynamically, let's add the CSR name in the CSR function table.
Signed-off-by: Bin Meng
---
target/riscv/cpu.h | 1 +
target/riscv/csr.c | 332 +++--
2 files changed, 249 insertions(+), 84 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 6f9e1cc..6684316 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -476,6 +476,7 @@ typedef int (*riscv_csr_op_fn)(CPURISCVState *env, int
csrno,
target_ulong *ret_value, target_ulong new_value, target_ulong write_mask);
typedef struct {
+const char *name;
riscv_csr_predicate_fn predicate;
riscv_csr_read_fn read;
riscv_csr_write_fn write;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 507e8ee..fd2e636 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1372,112 +1372,276 @@ int riscv_csrrw_debug(CPURISCVState *env, int csrno,
target_ulong *ret_value,
/* Control and Status Register function table */
riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
/* User Floating-Point CSRs */
-[CSR_FFLAGS] = { fs, read_fflags, write_fflags },
-[CSR_FRM] = { fs, read_frm, write_frm },
-[CSR_FCSR] ={ fs, read_fcsr,write_fcsr},
+[CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
+[CSR_FRM] = { "frm", fs, read_frm, write_frm},
+[CSR_FCSR] = { "fcsr", fs, read_fcsr,write_fcsr },
/* Vector CSRs */
-[CSR_VSTART] = { vs, read_vstart, write_vstart },
-[CSR_VXSAT] = { vs, read_vxsat, write_vxsat },
-[CSR_VXRM] ={ vs, read_vxrm,write_vxrm},
-[CSR_VL] = { vs, read_vl },
-[CSR_VTYPE] = { vs, read_vtype },
+[CSR_VSTART] = { "vstart", vs, read_vstart, write_vstart },
+[CSR_VXSAT]= { "vxsat",vs, read_vxsat, write_vxsat },
+[CSR_VXRM] = { "vxrm", vs, read_vxrm,write_vxrm },
+[CSR_VL] = { "vl", vs, read_vl},
+[CSR_VTYPE]= { "vtype",vs, read_vtype },
/* User Timers and Counters */
-[CSR_CYCLE] = { ctr, read_instret},
-[CSR_INSTRET] = { ctr, read_instret},
-[CSR_CYCLEH] = { ctr32, read_instreth },
-[CSR_INSTRETH] ={ ctr32, read_instreth },
-
-/* In privileged mode, the monitor will have to emulate TIME CSRs only if
- * rdtime callback is not provided by machine/platform emulation */
-[CSR_TIME] ={ ctr, read_time },
-[CSR_TIMEH] = { ctr32, read_timeh},
+[CSR_CYCLE]= { "cycle",ctr,read_instret },
+[CSR_INSTRET] = { "instret", ctr,read_instret },
+[CSR_CYCLEH] = { "cycleh", ctr32, read_instreth },
+[CSR_INSTRETH] = { "instreth", ctr32, read_instreth },
+
+/*
+ * In privileged mode, the monitor will have to emulate TIME CSRs only if
+ * rdtime callback is not provided by machine/platform emulation.
+ */
+[CSR_TIME] = { "time", ctr, read_time },
+[CSR_TIMEH] = { "timeh", ctr32, read_timeh },
#if !defined(CONFIG_USER_ONLY)
/* Machine Timers and Counters */
-[CSR_MCYCLE] = { any, read_instret},
-[CSR_MINSTRET] ={ any, read_instret},
-[CSR_MCYCLEH] = { any32, read_instreth },
-[CSR_MINSTRETH] = { any32, read_instreth },
+[CSR_MCYCLE]= { "mcycle",any, read_instret },
+[CSR_MINSTRET] = { "minstret", any, read_instret },
+[CSR_MCYCLEH] = { "mcycleh", any32, read_instreth },
+[CSR_MINSTRETH] = { "minstreth", any32, read_instreth },
/* Machine Information Registers */
-[CSR_MVENDORID] = { any, read_zero },
-[CSR_MARCHID] = { any, read_zero },
-[CSR_MIMPID] = { any, read_zero },
-[CSR_MHARTID] = { any, read_mhartid},
+[CSR_MVENDORID] = { "mvendorid", any, read_zero},
+[CSR_MARCHID] = { "marchid", any, read_zero},
+[CSR_MIMPID]= { "mimpid",any, read_zero},
+[CSR_MHARTID] = { "mhartid", any, read_mhartid },
/* Machine Trap Setup */
-[CSR_MSTATUS] = { any, read_mstatus, write_mstatus },
-[CSR_MISA] ={ any, read_misa,
