Re: [RFC PATCH v4 2/4] Adding basic custom/vendor CSR handling mechanism

[Bin Meng]

On Fri, Aug 6, 2021 at 2:08 PM Ruinland Chuan-Tzu Tsa(蔡傳資)
<ruinland@andestech.com> wrote:
>
>
> Hi Bin and Alistair,
>
>
> >> +#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.
>
> Will do
>
> >> +
> >> +    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);
>
> I can try.
>
> >> +
> >> +
> >> +    int i;
>
> >nits: please move this to the beginning of this function.
>
> Will do.
>
> >> +    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.
>
> Please refer to Patch 4.
> The table is null-ended.
> Thus it's a break here.
>
>
> >> +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.
>
> Will do.
>
> > +
> > +/*
> > + * 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
>
> Sounds reasonable.
>
> >>  /* 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.
> Could you elaborate a little bit more ?

These 2 should belong to patch 4 where Andes custom CSR is added.

>
> >>  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.
> Sorry for the newline.
>
> >> -#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
> Thanks for the tips.
> Will do.
>
> >> +/*
> >> + * XXX: This is just a write stub for developing custom CSR handler,
>
> >Remove XXX
> Will do.
>
> >> + * if the behavior of writting such CSR is not presentable in QEMU and 
> >> doesn't
>
> >typo: writing.
>
> >Is that present, or presentable?
>
> It's not a writing typo. I mean "presentable" with its literal meaning.
> If we cannot demonstrate a hardware feature inside QEMU, we can just stub it.
>
>
> >> +#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)
>
> Thanks for the advice, will modify it for V5.
>
>
> >> +
> >> +
>
> >Unnecessary changes
> Sorry for the newline.
>
> >>  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
> Will do.
>
> >> +    riscv_csr_operations *csrop = &csr_ops[csrno];
>
> >nits: name this variable to csr_ops
>
> It will collide with original csr_ops.

Maybe csr_op ?

> I'll change to another name.
>
>
> >>
> >> +#if defined(CONFIG_RISCV_CUSTOM)
> >> +/* Include the custom CSR table here. */
>
> >nits: remove the ending .
> Will do.
> Sorry for all these style format issues.
> I must I cherry-picked a wrong before I ran checkpatch.pl.
>
> >> +/* This file is intentionally left blank at this commit. */
>
> >nits: remove the ending .
>
> >%s/at/in
>
> Will do.
>
> >Regards,
> >Bin
>
> Thanks for the quick reply and advice.
> I'll cook the v5 ASAP.

Note: one more place you need to modify in this patch, is the
riscv_gen_dynamic_csr_xml() in target/riscv/gdbstub.c

Regards,
Bin