[PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[Weiwei Li]

The patch tries to separate the multi-letter extensions that may implicitly-enabled by misa.EXT from the explicitly-enabled cases, so that the misa.EXT can truely disabled by write_misa().
With this separation, the implicitly-enabled zve64d/f and zve32f extensions will no work if we clear misa.V. And clear misa.V will have no effect on the explicitly-enalbed zve64d/f and zve32f extensions.

Weiwei Li (2):
  target/riscv: Add set_implicit_extensions_from_ext() function
  target/riscv: Add ext_z*_enabled for implicitly enabled extensions

 target/riscv/cpu.c                      | 73 +++++++++++++++----------
 target/riscv/cpu.h                      |  8 +++
 target/riscv/cpu_helper.c               |  2 +-
 target/riscv/csr.c                      |  2 +-
 target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
 target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
 target/riscv/insn_trans/trans_rvi.c.inc |  5 +-
 target/riscv/insn_trans/trans_rvv.c.inc | 16 +++---
 target/riscv/translate.c                |  4 +-
 9 files changed, 68 insertions(+), 46 deletions(-)

-- 
2.25.1

[PATCH 1/2] target/riscv: Add set_implicit_extensions_from_ext() function

[Weiwei Li]

Move multi-letter extensions that may implicitly enabled from misa.EXT
alone to prepare for following separation of implicitly enabled and
explicitly enabled extensions.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/cpu.c | 56 +++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index cb68916fce..abb65d41b1 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -809,6 +809,35 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
     }
 }
 
+static void set_implicit_extensions_from_ext(RISCVCPU *cpu)
+{
+
+    /* The V vector extension depends on the Zve64d extension */
+    if (cpu->cfg.ext_v) {
+        cpu->cfg.ext_zve64d = true;
+    }
+
+    /* The Zve64d extension depends on the Zve64f extension */
+    if (cpu->cfg.ext_zve64d) {
+        cpu->cfg.ext_zve64f = true;
+    }
+
+    /* The Zve64f extension depends on the Zve32f extension */
+    if (cpu->cfg.ext_zve64f) {
+        cpu->cfg.ext_zve32f = true;
+    }
+
+    if (cpu->cfg.ext_c) {
+        cpu->cfg.ext_zca = true;
+        if (cpu->cfg.ext_f && cpu->env.misa_mxl_max == MXL_RV32) {
+            cpu->cfg.ext_zcf = true;
+        }
+        if (cpu->cfg.ext_d) {
+            cpu->cfg.ext_zcd = true;
+        }
+    }
+}
+
 /*
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly, doing a set_misa() in the end.
@@ -833,6 +862,8 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_ifencei = true;
     }
 
+    set_implicit_extensions_from_ext(cpu);
+
     if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
         error_setg(errp,
                    "I and E extensions are incompatible");
@@ -886,21 +917,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         return;
     }
 
-    /* The V vector extension depends on the Zve64d extension */
-    if (cpu->cfg.ext_v) {
-        cpu->cfg.ext_zve64d = true;
-    }
-
-    /* The Zve64d extension depends on the Zve64f extension */
-    if (cpu->cfg.ext_zve64d) {
-        cpu->cfg.ext_zve64f = true;
-    }
-
-    /* The Zve64f extension depends on the Zve32f extension */
-    if (cpu->cfg.ext_zve64f) {
-        cpu->cfg.ext_zve32f = true;
-    }
-
     if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) {
         error_setg(errp, "Zve64d/V extensions require D extension");
         return;
@@ -956,16 +972,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         }
     }
 
-    if (cpu->cfg.ext_c) {
-        cpu->cfg.ext_zca = true;
-        if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
-            cpu->cfg.ext_zcf = true;
-        }
-        if (cpu->cfg.ext_d) {
-            cpu->cfg.ext_zcd = true;
-        }
-    }
-
     if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
         error_setg(errp, "Zcf extension is only relevant to RV32");
         return;
-- 
2.25.1

[PATCH 2/2] target/riscv: Add ext_z*_enabled for implicitly enabled extensions

[Weiwei Li]

For extensions that may implicitly enabled from misa.EXT:
- ext_* is true if the extension is explicitly-enabled.
- ext_*_enabled is true no matter the extension is implicitly-enabled or
explicitly-enabled. And we change the check on the extension to it.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/cpu.c                      | 29 +++++++++++++++----------
 target/riscv/cpu.h                      |  8 +++++++
 target/riscv/cpu_helper.c               |  2 +-
 target/riscv/csr.c                      |  2 +-
 target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
 target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
 target/riscv/insn_trans/trans_rvi.c.inc |  5 +++--
 target/riscv/insn_trans/trans_rvv.c.inc | 16 +++++++-------
 target/riscv/translate.c                |  4 ++--
 9 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index abb65d41b1..815dd19f36 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -811,29 +811,35 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 
 static void set_implicit_extensions_from_ext(RISCVCPU *cpu)
 {
+    cpu->cfg.ext_zve64d_enabled = cpu->cfg.ext_zve64d;
+    cpu->cfg.ext_zve64f_enabled = cpu->cfg.ext_zve64f;
+    cpu->cfg.ext_zve32f_enabled = cpu->cfg.ext_zve32f;
+    cpu->cfg.ext_zca_enabled = cpu->cfg.ext_zca;
+    cpu->cfg.ext_zcf_enabled = cpu->cfg.ext_zcf;
+    cpu->cfg.ext_zcd_enabled = cpu->cfg.ext_zcd;
 
     /* The V vector extension depends on the Zve64d extension */
     if (cpu->cfg.ext_v) {
-        cpu->cfg.ext_zve64d = true;
+        cpu->cfg.ext_zve64d_enabled = true;
     }
 
     /* The Zve64d extension depends on the Zve64f extension */
     if (cpu->cfg.ext_zve64d) {
-        cpu->cfg.ext_zve64f = true;
+        cpu->cfg.ext_zve64f_enabled = true;
     }
 
     /* The Zve64f extension depends on the Zve32f extension */
     if (cpu->cfg.ext_zve64f) {
-        cpu->cfg.ext_zve32f = true;
+        cpu->cfg.ext_zve32f_enabled = true;
     }
 
     if (cpu->cfg.ext_c) {
-        cpu->cfg.ext_zca = true;
+        cpu->cfg.ext_zca_enabled = true;
         if (cpu->cfg.ext_f && cpu->env.misa_mxl_max == MXL_RV32) {
-            cpu->cfg.ext_zcf = true;
+            cpu->cfg.ext_zcf_enabled = true;
         }
         if (cpu->cfg.ext_d) {
-            cpu->cfg.ext_zcd = true;
+            cpu->cfg.ext_zcd_enabled = true;
         }
     }
 }
@@ -917,12 +923,12 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         return;
     }
 
-    if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) {
+    if (cpu->cfg.ext_zve64d_enabled && !cpu->cfg.ext_d) {
         error_setg(errp, "Zve64d/V extensions require D extension");
         return;
     }
 
-    if (cpu->cfg.ext_zve32f && !cpu->cfg.ext_f) {
+    if (cpu->cfg.ext_zve32f_enabled && !cpu->cfg.ext_f) {
         error_setg(errp, "Zve32f/Zve64f extensions require F extension");
         return;
     }
@@ -931,7 +937,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_zvfhmin = true;
     }
 
-    if (cpu->cfg.ext_zvfhmin && !cpu->cfg.ext_zve32f) {
+    if (cpu->cfg.ext_zvfhmin && !cpu->cfg.ext_zve32f_enabled) {
         error_setg(errp, "Zvfh/Zvfhmin extensions require Zve32f extension");
         return;
     }
@@ -988,13 +994,14 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     }
 
     if ((cpu->cfg.ext_zcf || cpu->cfg.ext_zcd || cpu->cfg.ext_zcb ||
-         cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) && !cpu->cfg.ext_zca) {
+         cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt) &&
+        !cpu->cfg.ext_zca_enabled) {
         error_setg(errp, "Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca "
                          "extension");
         return;
     }
 
-    if (cpu->cfg.ext_zcd && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
+    if (cpu->cfg.ext_zcd_enabled && (cpu->cfg.ext_zcmp || cpu->cfg.ext_zcmt)) {
         error_setg(errp, "Zcmp/Zcmt extensions are incompatible with "
                          "Zcd extension");
         return;
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 4af8ebc558..c59f099dd3 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -523,6 +523,14 @@ struct RISCVCPUConfig {
     bool debug;
     bool misa_w;
 
+    /* May implicitly enabled extensions by misa.EXT */
+    bool ext_zca_enabled;
+    bool ext_zcf_enabled;
+    bool ext_zcd_enabled;
+    bool ext_zve32f_enabled;
+    bool ext_zve64f_enabled;
+    bool ext_zve64d_enabled;
+
     bool short_isa_string;
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 433ea529b0..c9638ae905 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -51,7 +51,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
     *pc = env->xl == MXL_RV32 ? env->pc & UINT32_MAX : env->pc;
     *cs_base = 0;
 
-    if (cpu->cfg.ext_zve32f) {
+    if (cpu->cfg.ext_zve32f_enabled) {
         /*
          * If env->vl equals to VLMAX, we can use generic vector operation
          * expanders (GVEC) to accerlate the vector operations.
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index f4d2dcfdc8..1639a1bdd6 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -88,7 +88,7 @@ static RISCVException fs(CPURISCVState *env, int csrno)
 
 static RISCVException vs(CPURISCVState *env, int csrno)
 {
-    if (riscv_cpu_cfg(env)->ext_zve32f) {
+    if (riscv_cpu_cfg(env)->ext_zve32f_enabled) {
 #if !defined(CONFIG_USER_ONLY)
         if (!env->debugger && !riscv_cpu_vector_enabled(env)) {
             return RISCV_EXCP_ILLEGAL_INST;
diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
index 2c51e01c40..b0900a2274 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -32,7 +32,7 @@
 } while (0)
 
 #define REQUIRE_ZCD(ctx) do { \
-    if (!ctx->cfg_ptr->ext_zcd) {  \
+    if (!ctx->cfg_ptr->ext_zcd_enabled) {  \
         return false;     \
     } \
 } while (0)
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index 9e9fa2087a..6e810bab3c 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -31,7 +31,7 @@
 } while (0)
 
 #define REQUIRE_ZCF(ctx) do {                  \
-    if (!ctx->cfg_ptr->ext_zcf) {              \
+    if (!ctx->cfg_ptr->ext_zcf_enabled) {      \
         return false;                          \
     }                                          \
 } while (0)
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index c70c495fc5..8947176718 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -56,7 +56,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
     tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2);
 
     gen_set_pc(ctx, cpu_pc);
-    if (!ctx->cfg_ptr->ext_zca) {
+    if (!ctx->cfg_ptr->ext_zca_enabled) {
         TCGv t0 = tcg_temp_new();
 
         misaligned = gen_new_label();
@@ -169,7 +169,8 @@ static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond cond)
 
     gen_set_label(l); /* branch taken */
 
-    if (!ctx->cfg_ptr->ext_zca && ((ctx->base.pc_next + a->imm) & 0x3)) {
+    if (!ctx->cfg_ptr->ext_zca_enabled &&
+        ((ctx->base.pc_next + a->imm) & 0x3)) {
         /* misaligned */
         gen_exception_inst_addr_mis(ctx);
     } else {
diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
index ca3c4c1a3d..fbd5b9b626 100644
--- a/target/riscv/insn_trans/trans_rvv.c.inc
+++ b/target/riscv/insn_trans/trans_rvv.c.inc
@@ -42,9 +42,9 @@ static bool require_rvf(DisasContext *s)
     case MO_16:
         return s->cfg_ptr->ext_zvfh;
     case MO_32:
-        return s->cfg_ptr->ext_zve32f;
+        return s->cfg_ptr->ext_zve32f_enabled;
     case MO_64:
-        return s->cfg_ptr->ext_zve64d;
+        return s->cfg_ptr->ext_zve64d_enabled;
     default:
         return false;
     }
@@ -60,9 +60,9 @@ static bool require_scale_rvf(DisasContext *s)
     case MO_8:
         return s->cfg_ptr->ext_zvfh;
     case MO_16:
-        return s->cfg_ptr->ext_zve32f;
+        return s->cfg_ptr->ext_zve32f_enabled;
     case MO_32:
-        return s->cfg_ptr->ext_zve64d;
+        return s->cfg_ptr->ext_zve64d_enabled;
     default:
         return false;
     }
@@ -78,9 +78,9 @@ static bool require_scale_rvfmin(DisasContext *s)
     case MO_8:
         return s->cfg_ptr->ext_zvfhmin;
     case MO_16:
-        return s->cfg_ptr->ext_zve32f;
+        return s->cfg_ptr->ext_zve32f_enabled;
     case MO_32:
-        return s->cfg_ptr->ext_zve64d;
+        return s->cfg_ptr->ext_zve64d_enabled;
     default:
         return false;
     }
@@ -149,7 +149,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1, TCGv s2)
 {
     TCGv s1, dst;
 
-    if (!require_rvv(s) || !s->cfg_ptr->ext_zve32f) {
+    if (!require_rvv(s) || !s->cfg_ptr->ext_zve32f_enabled) {
         return false;
     }
 
@@ -179,7 +179,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv s1, TCGv s2)
 {
     TCGv dst;
 
-    if (!require_rvv(s) || !s->cfg_ptr->ext_zve32f) {
+    if (!require_rvv(s) || !s->cfg_ptr->ext_zve32f_enabled) {
         return false;
     }
 
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index d0094922b6..3c0c5e58eb 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -551,7 +551,7 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm)
 
     /* check misaligned: */
     next_pc = ctx->base.pc_next + imm;
-    if (!ctx->cfg_ptr->ext_zca) {
+    if (!ctx->cfg_ptr->ext_zca_enabled) {
         if ((next_pc & 0x3) != 0) {
             gen_exception_inst_addr_mis(ctx);
             return;
@@ -1137,7 +1137,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
          * The Zca extension is added as way to refer to instructions in the C
          * extension that do not include the floating-point loads and stores
          */
-        if (ctx->cfg_ptr->ext_zca && decode_insn16(ctx, opcode)) {
+        if (ctx->cfg_ptr->ext_zca_enabled && decode_insn16(ctx, opcode)) {
             return;
         }
     } else {
-- 
2.25.1

Re: [PATCH 1/2] target/riscv: Add set_implicit_extensions_from_ext() function

[Daniel Henrique Barboza]

On 4/10/23 00:35, Weiwei Li wrote:
> Move multi-letter extensions that may implicitly enabled from misa.EXT
> alone to prepare for following separation of implicitly enabled and
> explicitly enabled extensions.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---

Regardless of what we end up doing with patch 2 I believe this is a good
separation to have.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/riscv/cpu.c | 56 +++++++++++++++++++++++++---------------------
>   1 file changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cb68916fce..abb65d41b1 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -809,6 +809,35 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>       }
>   }
>   
> +static void set_implicit_extensions_from_ext(RISCVCPU *cpu)
> +{
> +
> +    /* The V vector extension depends on the Zve64d extension */
> +    if (cpu->cfg.ext_v) {
> +        cpu->cfg.ext_zve64d = true;
> +    }
> +
> +    /* The Zve64d extension depends on the Zve64f extension */
> +    if (cpu->cfg.ext_zve64d) {
> +        cpu->cfg.ext_zve64f = true;
> +    }
> +
> +    /* The Zve64f extension depends on the Zve32f extension */
> +    if (cpu->cfg.ext_zve64f) {
> +        cpu->cfg.ext_zve32f = true;
> +    }
> +
> +    if (cpu->cfg.ext_c) {
> +        cpu->cfg.ext_zca = true;
> +        if (cpu->cfg.ext_f && cpu->env.misa_mxl_max == MXL_RV32) {
> +            cpu->cfg.ext_zcf = true;
> +        }
> +        if (cpu->cfg.ext_d) {
> +            cpu->cfg.ext_zcd = true;
> +        }
> +    }
> +}
> +
>   /*
>    * Check consistency between chosen extensions while setting
>    * cpu->cfg accordingly, doing a set_misa() in the end.
> @@ -833,6 +862,8 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           cpu->cfg.ext_ifencei = true;
>       }
>   
> +    set_implicit_extensions_from_ext(cpu);
> +
>       if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
>           error_setg(errp,
>                      "I and E extensions are incompatible");
> @@ -886,21 +917,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           return;
>       }
>   
> -    /* The V vector extension depends on the Zve64d extension */
> -    if (cpu->cfg.ext_v) {
> -        cpu->cfg.ext_zve64d = true;
> -    }
> -
> -    /* The Zve64d extension depends on the Zve64f extension */
> -    if (cpu->cfg.ext_zve64d) {
> -        cpu->cfg.ext_zve64f = true;
> -    }
> -
> -    /* The Zve64f extension depends on the Zve32f extension */
> -    if (cpu->cfg.ext_zve64f) {
> -        cpu->cfg.ext_zve32f = true;
> -    }
> -
>       if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) {
>           error_setg(errp, "Zve64d/V extensions require D extension");
>           return;
> @@ -956,16 +972,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           }
>       }
>   
> -    if (cpu->cfg.ext_c) {
> -        cpu->cfg.ext_zca = true;
> -        if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
> -            cpu->cfg.ext_zcf = true;
> -        }
> -        if (cpu->cfg.ext_d) {
> -            cpu->cfg.ext_zcd = true;
> -        }
> -    }
> -
>       if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
>           error_setg(errp, "Zcf extension is only relevant to RV32");
>           return;

Re: [PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[Daniel Henrique Barboza]

Hi,

On 4/10/23 00:35, Weiwei Li wrote:
> The patch tries to separate the multi-letter extensions that may implicitly-enabled by misa.EXT from the explicitly-enabled cases, so that the misa.EXT can truely disabled by write_misa().
> With this separation, the implicitly-enabled zve64d/f and zve32f extensions will no work if we clear misa.V. And clear misa.V will have no effect on the explicitly-enalbed zve64d/f and zve32f extensions.

For this particular case of write_misa() I'm not sure if we need all that. If we want
to grant user choice on write_misa(), let's say that the user wants to enable/disable
RVV, we can enable/disable all RVV related Z-extensions by hand. It's just a matter
of writing enable/disable code that write_misa() would use.

In the end, write_misa() is also an user choice. If write_misa() wants to disable RVV,
this means that the user wants to disable RVV, so it doesn't matter whether the user
enabled zve32f on the command line or not - we disable zve32f as well. Same thing for
RVC and its related Z-extensions.

The reason why I didn't do this particular code for RVC and RVV is because we have
pending work in the ML that I would like to get it merged first. And there's a few
caveats we need to decide what to do, e.g. what if the user disables F but V is
enabled? Do we refuse write_misa()? Do we disable RVV?


All this said, patch 1 is still a good addition to make it easier to distinguish
the Z-extensions we're enabling due to MISA bits. I believe we should use
set_implicit_extensions_from_ext() in the future for all similar situations.



Thanks,

Daniel



> 
> Weiwei Li (2):
>    target/riscv: Add set_implicit_extensions_from_ext() function
>    target/riscv: Add ext_z*_enabled for implicitly enabled extensions
> 
>   target/riscv/cpu.c                      | 73 +++++++++++++++----------
>   target/riscv/cpu.h                      |  8 +++
>   target/riscv/cpu_helper.c               |  2 +-
>   target/riscv/csr.c                      |  2 +-
>   target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
>   target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
>   target/riscv/insn_trans/trans_rvi.c.inc |  5 +-
>   target/riscv/insn_trans/trans_rvv.c.inc | 16 +++---
>   target/riscv/translate.c                |  4 +-
>   9 files changed, 68 insertions(+), 46 deletions(-)
> 

Re: [PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[liweiwei]

On 2023/4/10 21:48, Daniel Henrique Barboza wrote:
> Hi,
>
> On 4/10/23 00:35, Weiwei Li wrote:
>> The patch tries to separate the multi-letter extensions that may 
>> implicitly-enabled by misa.EXT from the explicitly-enabled cases, so 
>> that the misa.EXT can truely disabled by write_misa().
>> With this separation, the implicitly-enabled zve64d/f and zve32f 
>> extensions will no work if we clear misa.V. And clear misa.V will 
>> have no effect on the explicitly-enalbed zve64d/f and zve32f extensions.
>
> For this particular case of write_misa() I'm not sure if we need all 
> that. If we want
> to grant user choice on write_misa(), let's say that the user wants to 
> enable/disable
> RVV, we can enable/disable all RVV related Z-extensions by hand. It's 
> just a matter
> of writing enable/disable code that write_misa() would use.
>
> In the end, write_misa() is also an user choice. If write_misa() wants 
> to disable RVV,
> this means that the user wants to disable RVV, so it doesn't matter 
> whether the user
> enabled zve32f on the command line or not - we disable zve32f as well. 
> Same thing for
> RVC and its related Z-extensions.
>
Yeah. It's also a choice. It's a question whether we take C_Zca and C  
as the same user choice.

If we consider them as different, then this patch works. And this patch 
can bypass the priv version problem.

> The reason why I didn't do this particular code for RVC and RVV is 
> because we have
> pending work in the ML that I would like to get it merged first. And 
> there's a few
> caveats we need to decide what to do, e.g. what if the user disables F 
> but V is
> enabled? Do we refuse write_misa()? Do we disable RVV?
>
In section 3.1.1 of privilege spec:

"If an ISA feature x depends on an ISA feature y, then attempting to 
enable feature x but disable

feature y results in both features being disabled."

Even though there is also another description in the following content 
of the same section:

"An implementation may impose additional constraints on the collective 
setting of two or more misa
fields, in which case they function collectively as a single WARL field. 
An attempt to write an
unsupported combination causes those bits to be set to some supported 
combination."

I think the former description is more explicit.

Regards,

Weiwei Li

>
> All this said, patch 1 is still a good addition to make it easier to 
> distinguish
> the Z-extensions we're enabling due to MISA bits. I believe we should use
> set_implicit_extensions_from_ext() in the future for all similar 
> situations.
>
>
>
> Thanks,
>
> Daniel
>
>
>
>>
>> Weiwei Li (2):
>>    target/riscv: Add set_implicit_extensions_from_ext() function
>>    target/riscv: Add ext_z*_enabled for implicitly enabled extensions
>>
>>   target/riscv/cpu.c                      | 73 +++++++++++++++----------
>>   target/riscv/cpu.h                      |  8 +++
>>   target/riscv/cpu_helper.c               |  2 +-
>>   target/riscv/csr.c                      |  2 +-
>>   target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
>>   target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
>>   target/riscv/insn_trans/trans_rvi.c.inc |  5 +-
>>   target/riscv/insn_trans/trans_rvv.c.inc | 16 +++---
>>   target/riscv/translate.c                |  4 +-
>>   9 files changed, 68 insertions(+), 46 deletions(-)
>>

Re: [PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[Daniel Henrique Barboza]

On 4/10/23 11:20, liweiwei wrote:
> 
> On 2023/4/10 21:48, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> On 4/10/23 00:35, Weiwei Li wrote:
>>> The patch tries to separate the multi-letter extensions that may implicitly-enabled by misa.EXT from the explicitly-enabled cases, so that the misa.EXT can truely disabled by write_misa().
>>> With this separation, the implicitly-enabled zve64d/f and zve32f extensions will no work if we clear misa.V. And clear misa.V will have no effect on the explicitly-enalbed zve64d/f and zve32f extensions.
>>
>> For this particular case of write_misa() I'm not sure if we need all that. If we want
>> to grant user choice on write_misa(), let's say that the user wants to enable/disable
>> RVV, we can enable/disable all RVV related Z-extensions by hand. It's just a matter
>> of writing enable/disable code that write_misa() would use.
>>
>> In the end, write_misa() is also an user choice. If write_misa() wants to disable RVV,
>> this means that the user wants to disable RVV, so it doesn't matter whether the user
>> enabled zve32f on the command line or not - we disable zve32f as well. Same thing for
>> RVC and its related Z-extensions.
>>
> Yeah. It's also a choice. It's a question whether we take C_Zca and C as the same user choice.
> 
> If we consider them as different, then this patch works. And this patch can bypass the priv version problem.
> 
>> The reason why I didn't do this particular code for RVC and RVV is because we have
>> pending work in the ML that I would like to get it merged first. And there's a few
>> caveats we need to decide what to do, e.g. what if the user disables F but V is
>> enabled? Do we refuse write_misa()? Do we disable RVV?
>>
> In section 3.1.1 of privilege spec:
> 
> "If an ISA feature x depends on an ISA feature y, then attempting to enable feature x but disable
> 
> feature y results in both features being disabled."
> 
> Even though there is also another description in the following content of the same section:
> 
> "An implementation may impose additional constraints on the collective setting of two or more misa
> fields, in which case they function collectively as a single WARL field. An attempt to write an
> unsupported combination causes those bits to be set to some supported combination."
> 
> I think the former description is more explicit.

Yeah. Disabling a MISA bit should disable all dependencies of the bit and there's
not much to discuss about it.

As far as the current write_misa() impl in the mailing list goes, we're refusing to
disable the bit (e.g. the validation would fail if RVF is disabled while keeping all
its dependencies, write_misa() becomes a no-op).

If we decide to give users this kind of control I believe we should disregard all user
choice during boot and enable/disable extensions as required.



Thanks,

Daniel

> 
> Regards,
> 
> Weiwei Li
> 
>>
>> All this said, patch 1 is still a good addition to make it easier to distinguish
>> the Z-extensions we're enabling due to MISA bits. I believe we should use
>> set_implicit_extensions_from_ext() in the future for all similar situations.
>>
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>
>>
>>>
>>> Weiwei Li (2):
>>>    target/riscv: Add set_implicit_extensions_from_ext() function
>>>    target/riscv: Add ext_z*_enabled for implicitly enabled extensions
>>>
>>>   target/riscv/cpu.c                      | 73 +++++++++++++++----------
>>>   target/riscv/cpu.h                      |  8 +++
>>>   target/riscv/cpu_helper.c               |  2 +-
>>>   target/riscv/csr.c                      |  2 +-
>>>   target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
>>>   target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
>>>   target/riscv/insn_trans/trans_rvi.c.inc |  5 +-
>>>   target/riscv/insn_trans/trans_rvv.c.inc | 16 +++---
>>>   target/riscv/translate.c                |  4 +-
>>>   9 files changed, 68 insertions(+), 46 deletions(-)
>>>
> 

Re: [PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[liweiwei]

On 2023/4/10 21:48, Daniel Henrique Barboza wrote:
> Hi,
>
> On 4/10/23 00:35, Weiwei Li wrote:
>> The patch tries to separate the multi-letter extensions that may 
>> implicitly-enabled by misa.EXT from the explicitly-enabled cases, so 
>> that the misa.EXT can truely disabled by write_misa().
>> With this separation, the implicitly-enabled zve64d/f and zve32f 
>> extensions will no work if we clear misa.V. And clear misa.V will 
>> have no effect on the explicitly-enalbed zve64d/f and zve32f extensions.
>
> For this particular case of write_misa() I'm not sure if we need all 
> that. If we want
> to grant user choice on write_misa(), let's say that the user wants to 
> enable/disable
> RVV, we can enable/disable all RVV related Z-extensions by hand. It's 
> just a matter
> of writing enable/disable code that write_misa() would use.
>
> In the end, write_misa() is also an user choice. If write_misa() wants 
> to disable RVV,
> this means that the user wants to disable RVV, so it doesn't matter 
> whether the user
> enabled zve32f on the command line or not - we disable zve32f as well. 
> Same thing for
> RVC and its related Z-extensions.

I just find we should also separate the cases here. Zcmp/Zcmt require Zca.

If we disable Zca when misa.C is cleared, whether we need disable 
Zcmp/Zcmt?

If yes, then we need another new parameter(to indicate they are diabled by

clearing misa.C ) to decide whether we should re-enable them when misa.C 
is set.

If not, we should make clearing misa.C failed in this case.

Regards,

Weiwei Li

>
> The reason why I didn't do this particular code for RVC and RVV is 
> because we have
> pending work in the ML that I would like to get it merged first. And 
> there's a few
> caveats we need to decide what to do, e.g. what if the user disables F 
> but V is
> enabled? Do we refuse write_misa()? Do we disable RVV?
>
>
> All this said, patch 1 is still a good addition to make it easier to 
> distinguish
> the Z-extensions we're enabling due to MISA bits. I believe we should use
> set_implicit_extensions_from_ext() in the future for all similar 
> situations.
>
>
>
> Thanks,
>
> Daniel
>
>
>
>>
>> Weiwei Li (2):
>>    target/riscv: Add set_implicit_extensions_from_ext() function
>>    target/riscv: Add ext_z*_enabled for implicitly enabled extensions
>>
>>   target/riscv/cpu.c                      | 73 +++++++++++++++----------
>>   target/riscv/cpu.h                      |  8 +++
>>   target/riscv/cpu_helper.c               |  2 +-
>>   target/riscv/csr.c                      |  2 +-
>>   target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
>>   target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
>>   target/riscv/insn_trans/trans_rvi.c.inc |  5 +-
>>   target/riscv/insn_trans/trans_rvv.c.inc | 16 +++---
>>   target/riscv/translate.c                |  4 +-
>>   9 files changed, 68 insertions(+), 46 deletions(-)
>>

Re: [PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[liweiwei]

On 2023/4/11 02:35, Daniel Henrique Barboza wrote:
>
>
> On 4/10/23 11:20, liweiwei wrote:
>>
>> On 2023/4/10 21:48, Daniel Henrique Barboza wrote:
>>> Hi,
>>>
>>> On 4/10/23 00:35, Weiwei Li wrote:
>>>> The patch tries to separate the multi-letter extensions that may 
>>>> implicitly-enabled by misa.EXT from the explicitly-enabled cases, 
>>>> so that the misa.EXT can truely disabled by write_misa().
>>>> With this separation, the implicitly-enabled zve64d/f and zve32f 
>>>> extensions will no work if we clear misa.V. And clear misa.V will 
>>>> have no effect on the explicitly-enalbed zve64d/f and zve32f 
>>>> extensions.
>>>
>>> For this particular case of write_misa() I'm not sure if we need all 
>>> that. If we want
>>> to grant user choice on write_misa(), let's say that the user wants 
>>> to enable/disable
>>> RVV, we can enable/disable all RVV related Z-extensions by hand. 
>>> It's just a matter
>>> of writing enable/disable code that write_misa() would use.
>>>
>>> In the end, write_misa() is also an user choice. If write_misa() 
>>> wants to disable RVV,
>>> this means that the user wants to disable RVV, so it doesn't matter 
>>> whether the user
>>> enabled zve32f on the command line or not - we disable zve32f as 
>>> well. Same thing for
>>> RVC and its related Z-extensions.
>>>
>> Yeah. It's also a choice. It's a question whether we take C_Zca and C 
>> as the same user choice.
>>
>> If we consider them as different, then this patch works. And this 
>> patch can bypass the priv version problem.
>>
>>> The reason why I didn't do this particular code for RVC and RVV is 
>>> because we have
>>> pending work in the ML that I would like to get it merged first. And 
>>> there's a few
>>> caveats we need to decide what to do, e.g. what if the user disables 
>>> F but V is
>>> enabled? Do we refuse write_misa()? Do we disable RVV?
>>>
>> In section 3.1.1 of privilege spec:
>>
>> "If an ISA feature x depends on an ISA feature y, then attempting to 
>> enable feature x but disable
>>
>> feature y results in both features being disabled."
>>
>> Even though there is also another description in the following 
>> content of the same section:
>>
>> "An implementation may impose additional constraints on the 
>> collective setting of two or more misa
>> fields, in which case they function collectively as a single WARL 
>> field. An attempt to write an
>> unsupported combination causes those bits to be set to some supported 
>> combination."
>>
>> I think the former description is more explicit.
>
> Yeah. Disabling a MISA bit should disable all dependencies of the bit 
> and there's
> not much to discuss about it.
>
> As far as the current write_misa() impl in the mailing list goes, 
> we're refusing to
> disable the bit (e.g. the validation would fail if RVF is disabled 
> while keeping all
> its dependencies, write_misa() becomes a no-op).
>
> If we decide to give users this kind of control I believe we should 
> disregard all user
> choice during boot and enable/disable extensions as required.

Sorry, I don't get your idea here. Why should we disregard all user 
choice during boot?

Regards,

Weiwei Li

>
>
>
> Thanks,
>
> Daniel
>
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>> All this said, patch 1 is still a good addition to make it easier to 
>>> distinguish
>>> the Z-extensions we're enabling due to MISA bits. I believe we 
>>> should use
>>> set_implicit_extensions_from_ext() in the future for all similar 
>>> situations.
>>>
>>>
>>>
>>> Thanks,
>>>
>>> Daniel
>>>
>>>
>>>
>>>>
>>>> Weiwei Li (2):
>>>>    target/riscv: Add set_implicit_extensions_from_ext() function
>>>>    target/riscv: Add ext_z*_enabled for implicitly enabled extensions
>>>>
>>>>   target/riscv/cpu.c                      | 73 
>>>> +++++++++++++++----------
>>>>   target/riscv/cpu.h                      |  8 +++
>>>>   target/riscv/cpu_helper.c               |  2 +-
>>>>   target/riscv/csr.c                      |  2 +-
>>>>   target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
>>>>   target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
>>>>   target/riscv/insn_trans/trans_rvi.c.inc |  5 +-
>>>>   target/riscv/insn_trans/trans_rvv.c.inc | 16 +++---
>>>>   target/riscv/translate.c                |  4 +-
>>>>   9 files changed, 68 insertions(+), 46 deletions(-)
>>>>
>>

Re: [PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[Daniel Henrique Barboza]

On 4/10/23 21:15, liweiwei wrote:
> 
> On 2023/4/10 21:48, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> On 4/10/23 00:35, Weiwei Li wrote:
>>> The patch tries to separate the multi-letter extensions that may implicitly-enabled by misa.EXT from the explicitly-enabled cases, so that the misa.EXT can truely disabled by write_misa().
>>> With this separation, the implicitly-enabled zve64d/f and zve32f extensions will no work if we clear misa.V. And clear misa.V will have no effect on the explicitly-enalbed zve64d/f and zve32f extensions.
>>
>> For this particular case of write_misa() I'm not sure if we need all that. If we want
>> to grant user choice on write_misa(), let's say that the user wants to enable/disable
>> RVV, we can enable/disable all RVV related Z-extensions by hand. It's just a matter
>> of writing enable/disable code that write_misa() would use.
>>
>> In the end, write_misa() is also an user choice. If write_misa() wants to disable RVV,
>> this means that the user wants to disable RVV, so it doesn't matter whether the user
>> enabled zve32f on the command line or not - we disable zve32f as well. Same thing for
>> RVC and its related Z-extensions.
> 
> I just find we should also separate the cases here. Zcmp/Zcmt require Zca.
> 
> If we disable Zca when misa.C is cleared, whether we need disable Zcmp/Zcmt?

IMO write_misa() shouldn't be able to disable Z-extensions that it can't turn it back
on. E.g. RVV disabling zve64d/f is ok because, if we re-enable RVV, they'll be re-enabled
back.


> 
> If yes, then we need another new parameter(to indicate they are diabled by
> 
> clearing misa.C ) to decide whether we should re-enable them when misa.C is set.
> 
> If not, we should make clearing misa.C failed in this case.

I'd rather fail the operation. write_misa() should always make reversible operations. If
a certain CSR write would put us in a state that we can't go back on, we should fail.

For now I think I'll go back to that cleanup I made, rebase it, get one of Weiwei patches
that fixes the sifive breakage I talked about in the other thread, and see if we can
get that more simple version of write_misa() merged. We can continue these discussions
on top of that code.


Thanks,


Daniel


> 
> Regards,
> 
> Weiwei Li
> 
>>
>> The reason why I didn't do this particular code for RVC and RVV is because we have
>> pending work in the ML that I would like to get it merged first. And there's a few
>> caveats we need to decide what to do, e.g. what if the user disables F but V is
>> enabled? Do we refuse write_misa()? Do we disable RVV?
>>
>>
>> All this said, patch 1 is still a good addition to make it easier to distinguish
>> the Z-extensions we're enabling due to MISA bits. I believe we should use
>> set_implicit_extensions_from_ext() in the future for all similar situations.
>>
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>
>>
>>>
>>> Weiwei Li (2):
>>>    target/riscv: Add set_implicit_extensions_from_ext() function
>>>    target/riscv: Add ext_z*_enabled for implicitly enabled extensions
>>>
>>>   target/riscv/cpu.c                      | 73 +++++++++++++++----------
>>>   target/riscv/cpu.h                      |  8 +++
>>>   target/riscv/cpu_helper.c               |  2 +-
>>>   target/riscv/csr.c                      |  2 +-
>>>   target/riscv/insn_trans/trans_rvd.c.inc |  2 +-
>>>   target/riscv/insn_trans/trans_rvf.c.inc |  2 +-
>>>   target/riscv/insn_trans/trans_rvi.c.inc |  5 +-
>>>   target/riscv/insn_trans/trans_rvv.c.inc | 16 +++---
>>>   target/riscv/translate.c                |  4 +-
>>>   9 files changed, 68 insertions(+), 46 deletions(-)
>>>
> 

Re: [PATCH 1/2] target/riscv: Add set_implicit_extensions_from_ext() function

[Alistair Francis]

On Mon, Apr 10, 2023 at 1:36 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Move multi-letter extensions that may implicitly enabled from misa.EXT
> alone to prepare for following separation of implicitly enabled and
> explicitly enabled extensions.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

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

Alistair

> ---
>  target/riscv/cpu.c | 56 +++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cb68916fce..abb65d41b1 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -809,6 +809,35 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>      }
>  }
>
> +static void set_implicit_extensions_from_ext(RISCVCPU *cpu)
> +{
> +
> +    /* The V vector extension depends on the Zve64d extension */
> +    if (cpu->cfg.ext_v) {
> +        cpu->cfg.ext_zve64d = true;
> +    }
> +
> +    /* The Zve64d extension depends on the Zve64f extension */
> +    if (cpu->cfg.ext_zve64d) {
> +        cpu->cfg.ext_zve64f = true;
> +    }
> +
> +    /* The Zve64f extension depends on the Zve32f extension */
> +    if (cpu->cfg.ext_zve64f) {
> +        cpu->cfg.ext_zve32f = true;
> +    }
> +
> +    if (cpu->cfg.ext_c) {
> +        cpu->cfg.ext_zca = true;
> +        if (cpu->cfg.ext_f && cpu->env.misa_mxl_max == MXL_RV32) {
> +            cpu->cfg.ext_zcf = true;
> +        }
> +        if (cpu->cfg.ext_d) {
> +            cpu->cfg.ext_zcd = true;
> +        }
> +    }
> +}
> +
>  /*
>   * Check consistency between chosen extensions while setting
>   * cpu->cfg accordingly, doing a set_misa() in the end.
> @@ -833,6 +862,8 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          cpu->cfg.ext_ifencei = true;
>      }
>
> +    set_implicit_extensions_from_ext(cpu);
> +
>      if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
>          error_setg(errp,
>                     "I and E extensions are incompatible");
> @@ -886,21 +917,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          return;
>      }
>
> -    /* The V vector extension depends on the Zve64d extension */
> -    if (cpu->cfg.ext_v) {
> -        cpu->cfg.ext_zve64d = true;
> -    }
> -
> -    /* The Zve64d extension depends on the Zve64f extension */
> -    if (cpu->cfg.ext_zve64d) {
> -        cpu->cfg.ext_zve64f = true;
> -    }
> -
> -    /* The Zve64f extension depends on the Zve32f extension */
> -    if (cpu->cfg.ext_zve64f) {
> -        cpu->cfg.ext_zve32f = true;
> -    }
> -
>      if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) {
>          error_setg(errp, "Zve64d/V extensions require D extension");
>          return;
> @@ -956,16 +972,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          }
>      }
>
> -    if (cpu->cfg.ext_c) {
> -        cpu->cfg.ext_zca = true;
> -        if (cpu->cfg.ext_f && env->misa_mxl_max == MXL_RV32) {
> -            cpu->cfg.ext_zcf = true;
> -        }
> -        if (cpu->cfg.ext_d) {
> -            cpu->cfg.ext_zcd = true;
> -        }
> -    }
> -
>      if (env->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
>          error_setg(errp, "Zcf extension is only relevant to RV32");
>          return;
> --
> 2.25.1
>
>