Re: [PATCH v4 17/17] target/riscv: actual functions to realize crs 128-bit insns

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

On 10/25/21 8:28 AM, Frédéric Pétrot wrote:
> +typedef RISCVException (*riscv_csr_op128_fn)(CPURISCVState *env, int csrno,
> +                                             Int128 *ret_value,
> +                                             Int128 new_value,
> +                                             Int128 write_mask);
> +
>  typedef struct {
>      const char *name;
>      riscv_csr_predicate_fn predicate;
>      riscv_csr_read_fn read;
>      riscv_csr_write_fn write;
>      riscv_csr_op_fn op;
> +    riscv_csr_read128_fn read128;
> +    riscv_csr_write128_fn write128;
> +    riscv_csr_op128_fn op128;

I think you should drop op128 until there is a need for it.

> +static RISCVException read_mstatus_i128(CPURISCVState *env, int csrno,
> +                                        Int128 *val)
> +{
> +    *val = int128_make128(env->mstatus, add_status_sd(riscv_cpu_mxl(env), 0));
> +    return RISCV_EXCP_NONE;
> +}

Well, you don't need to read mxl here, as you cannot arrive to this function without it 
being MXL_RV128.

I think you need a similar read_sstatus_i128 function.

> -    /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails */
> +    /* check privileges and return -1 if check fails */

Bad rebase change; the return type is RISCVException.

> +    /* write value if writable and write mask set, otherwise drop writes */
> +    if (int128_nz(write_mask)) {
> +        new_value = int128_or(int128_and(old_value, int128_not(write_mask)),
> +                              int128_and(new_value, write_mask));
> +        if (csr_ops[csrno].write128) {
> +            ret = csr_ops[csrno].write128(env, csrno, new_value);
> +            if (ret != RISCV_EXCP_NONE) {
> +                return ret;
> +            }
> +        }
> +    }

This is wrong; you certainly don't want to drop writes; you want to forward writes to the 
64-bit version.  At least...

> +    [CSR_MSTATUS]     = { "mstatus",    any,   read_mstatus,     write_mstatus, NULL,
> +                                               read_mstatus_i128                   },
> +    [CSR_MISA]        = { "misa",       any,   read_misa,        write_misa, NULL,
> +                                               read_misa_i128                      },

... that is certainly the case for these CSRs.  Alternately, you've got to implement these 
write functions.


r~