Re: [PATCH v3 19/21] target/riscv: actual functions to realize crs 128-bit insns

[Richard Henderson <richard.henderson@linaro.org>]

On 10/19/21 2:48 AM, Frédéric Pétrot wrote:
> The csrs are accessed through function pointers: we set-up the table
> for the 128-bit accesses, make the stub a function that does what it
> should, and implement basic accesses on read-only csrs.
> 
> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> Co-authored-by: Fabien Portas <fabien.portas@grenoble-inp.org>
> ---
>   target/riscv/cpu.h |  16 +++++
>   target/riscv/csr.c | 152 ++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 165 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index eb4f63fcbf..253e87cd92 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -474,6 +474,15 @@ RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno,
>                                   Int128 *ret_value,
>                                   Int128 new_value, Int128 write_mask);
>   
> +typedef RISCVException (*riscv_csr_read128_fn)(CPURISCVState *env, int csrno,
> +                                               Int128 *ret_value);
> +typedef RISCVException (*riscv_csr_write128_fn)(CPURISCVState *env, int csrno,
> +                                             Int128 new_value);
> +typedef RISCVException (*riscv_csr_op128_fn)(CPURISCVState *env, int csrno,
> +                                             Int128 *ret_value,
> +                                             Int128 new_value,
> +                                             Int128 write_mask);

Do we really want all 3, or just the single rmw function?
Although I guess it's clearest to match the existing code...

> +
>   typedef struct {
>       const char *name;
>       riscv_csr_predicate_fn predicate;
> @@ -482,6 +491,12 @@ typedef struct {
>       riscv_csr_op_fn op;
>   } riscv_csr_operations;
>   
> +typedef struct {
> +    riscv_csr_read128_fn read128;
> +    riscv_csr_write128_fn write128;
> +    riscv_csr_op128_fn op128;
> +} riscv_csr_operations128;

Eh.  I think better to extend the one riscv_csr_operations structure.

> +static inline RISCVException riscv_csrrw_check_i128(CPURISCVState *env,
> +                                                    int csrno,
> +                                                    Int128 write_mask,
> +                                                    RISCVCPU *cpu)

Change "Int128 write_mask" to "bool write" and you can share this entire function with 
riscv_csrrw.

Indeed, you could split them like so:

riscv_csrrw(...)
{
     ret = csrrw_check(...);
     if (ret != RISCV_EXCP_NONE) {
         return ret;
     }
     return csrrw_do64(...);
}

riscv_csrrw_128(...)
{
     ret = csrrw_check(...);
     if (ret != RISCV_EXCP_NONE) {
         return ret;
     }
     if (csr128) {
         return csrrw_do128(...);
     }
     ret = csrrw_do64(..., old64, ...);
     if (ret == RISCV_EXCP_NONE) {
         *old_val = int128_make64(old64);
     }
     return ret;
}

> +    RISCVException ret = csr_ops[csrno].predicate(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
> +    return RISCV_EXCP_NONE;

BTW, just

     return csr_ops[csrno].predicate(env, csrno);


r~