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

[蔡傳資]

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 ?

>>  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.

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.

Best regards,

Ruinland

CONFIDENTIALITY NOTICE:

This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation.

Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.