Re: [PATCH 2/4] target/riscv: Add CSR name in the CSR function table

[Bin Meng]

Hi Alex,

On Fri, Jan 15, 2021 at 9:14 PM Alexander Richardson
<Alexander.Richardson@cl.cam.ac.uk> wrote:
>
> On Tue, 12 Jan 2021 at 05:02, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > 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 <bin.meng@windriver.com>
> > ---
> >
> >  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:
> <https://github.com/CTSRD-CHERI/qemu/commit/446dbebdfd7d8f9b75041041f5e8c5ebc8f815b8#diff-ff63c6a6531e6beae8637664aed5a008f79c76b8f14fd941c370d28c670faa12R1396>

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