Re: [PATCH v6 11/12] target/riscv/tcg: honor user choice for G MISA bits

[Alistair Francis]

On Sat, Oct 28, 2023 at 6:56 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> RVG behaves like a profile: a single flag enables a set of bits. Right
> now we're considering user choice when handling RVG and zicsr/zifencei
> and ignoring user choice on MISA bits.
>
> We'll add user warnings for profiles when the user disables its
> mandatory extensions in the next patch. We'll do the same thing with RVG
> now to keep consistency between RVG and profile handling.
>
> First and foremost, create a new RVG only helper to avoid clogging
> riscv_cpu_validate_set_extensions(). We do not want to annoy users with
> RVG warnings like we did in the past (see 9b9741c38f), thus we'll only
> warn if RVG was user set and the user disabled a RVG extension in the
> command line.
>
> For every RVG MISA bit (IMAFD), zicsr and zifencei, the logic then
> becomes:
>
> - if enabled, do nothing;
> - if disabled and not user set, enable it;
> - if disabled and user set, throw a warning that it's a RVG mandatory
>   extension.
>
> This same logic will be used for profiles in the next patch.
>
> Note that this is a behavior change, where we would error out if the
> user disabled either zicsr or zifencei. As long as users are explicitly
> disabling things in the command line we'll let them have a go at it, at
> least in this step. We'll error out later in the validation if needed.
>
> Other notable changes from the previous RVG code:
>
> - use riscv_cpu_write_misa_bit() instead of manually updating both
>   env->misa_ext and env->misa_ext_mask;
>
> - set zicsr and zifencei directly. We're already checking if they
>   were user set and priv version will never fail for these
>   extensions, making cpu_cfg_ext_auto_update() redundant.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 73 +++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 25 deletions(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 3a96b1f476..953e8432d6 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -42,6 +42,12 @@ static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
>                                   GUINT_TO_POINTER(ext_offset));
>  }
>
> +static bool cpu_misa_ext_is_user_set(uint32_t misa_bit)
> +{
> +    return g_hash_table_contains(misa_ext_user_opts,
> +                                 GUINT_TO_POINTER(misa_bit));
> +}
> +
>  static void cpu_cfg_ext_add_user_opt(uint32_t ext_offset, bool value)
>  {
>      g_hash_table_insert(multi_ext_user_opts, GUINT_TO_POINTER(ext_offset),
> @@ -303,6 +309,46 @@ static void riscv_cpu_validate_named_features(RISCVCPU 
> *cpu)
>      riscv_cpu_validate_zic64b(cpu);
>  }
>
> +static void riscv_cpu_validate_g(RISCVCPU *cpu)
> +{
> +    const char *warn_msg = "RVG mandates disabled extension %s";
> +    uint32_t g_misa_bits[] = {RVI, RVM, RVA, RVF, RVD};
> +    bool send_warn = cpu_misa_ext_is_user_set(RVG);
> +
> +    for (int i = 0; i < ARRAY_SIZE(g_misa_bits); i++) {
> +        uint32_t bit = g_misa_bits[i];
> +
> +        if (riscv_has_ext(&cpu->env, bit)) {
> +            continue;
> +        }
> +
> +        if (!cpu_misa_ext_is_user_set(bit)) {
> +            riscv_cpu_write_misa_bit(cpu, bit, true);
> +            continue;
> +        }
> +
> +        if (send_warn) {
> +            warn_report(warn_msg, riscv_get_misa_ext_name(bit));
> +        }
> +    }
> +
> +    if (!cpu->cfg.ext_zicsr) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zicsr))) {
> +            cpu->cfg.ext_zicsr = true;
> +        } else if (send_warn) {
> +            warn_report(warn_msg, "zicsr");
> +        }
> +    }
> +
> +    if (!cpu->cfg.ext_zifencei) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zifencei))) {
> +            cpu->cfg.ext_zifencei = true;
> +        } else if (send_warn) {
> +            warn_report(warn_msg, "zifencei");
> +        }
> +    }
> +}
> +
>  /*
>   * Check consistency between chosen extensions while setting
>   * cpu->cfg accordingly.
> @@ -312,31 +358,8 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>      CPURISCVState *env = &cpu->env;
>      Error *local_err = NULL;
>
> -    /* Do some ISA extension error checking */
> -    if (riscv_has_ext(env, RVG) &&
> -        !(riscv_has_ext(env, RVI) && riscv_has_ext(env, RVM) &&
> -          riscv_has_ext(env, RVA) && riscv_has_ext(env, RVF) &&
> -          riscv_has_ext(env, RVD) &&
> -          cpu->cfg.ext_zicsr && cpu->cfg.ext_zifencei)) {
> -
> -        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zicsr)) &&
> -            !cpu->cfg.ext_zicsr) {
> -            error_setg(errp, "RVG requires Zicsr but user set Zicsr to 
> false");
> -            return;
> -        }
> -
> -        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zifencei)) &&
> -            !cpu->cfg.ext_zifencei) {
> -            error_setg(errp, "RVG requires Zifencei but user set "
> -                       "Zifencei to false");
> -            return;
> -        }
> -
> -        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zicsr), true);
> -        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zifencei), true);
> -
> -        env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> -        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> +    if (riscv_has_ext(env, RVG)) {
> +        riscv_cpu_validate_g(cpu);
>      }
>
>      if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> --
> 2.41.0
>
>