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

[Richard Henderson]

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~