[PATCH 0/6] riscv: named features riscv,isa, 'svade' rework

[PATCH 0/6] riscv: named features riscv,isa, 'svade' rework

[Daniel Henrique Barboza]

Hi,

This is a bundle of fixes based on discoveries that were made in the
last week or so:

- what we call "named features" are actually real extensions, which are
  considered to be ratified by the profile spec that defines them. This
  means that we need to add riscv,isa strings for them. More info can be
  found on the commit msg of patch 2;

- the design behind 'svade' and 'svadu' is wrong. 'svade' does not mean
  'we do not have svadu'. In fact they can coexist. Patch 5 gives more
  details about it.


After this series, 'svade' is promoted to a regular extension and all
the named features QEMU implements are now being displayed in riscv,isa.


Andrew Jones (3):
  target/riscv: Reset henvcfg to zero
  target/riscv: Gate hardware A/D PTE bit updating
  target/riscv: Promote svade to a normal extension

Daniel Henrique Barboza (3):
  target/riscv/tcg: set 'mmu' with 'satp' in cpu_set_profile()
  target/riscv: add riscv,isa to named features
  target/riscv: add remaining named features

 target/riscv/cpu.c         | 63 ++++++++++++++++++++++++++++----------
 target/riscv/cpu_cfg.h     | 15 +++++++--
 target/riscv/cpu_helper.c  | 18 ++++++++---
 target/riscv/csr.c         |  2 +-
 target/riscv/tcg/tcg-cpu.c | 42 +++++++++++++++----------
 5 files changed, 99 insertions(+), 41 deletions(-)

-- 
2.43.0

[PATCH 1/6] target/riscv/tcg: set 'mmu' with 'satp' in cpu_set_profile()

[Daniel Henrique Barboza]

Recent changes in options handling removed the 'mmu' default the bare
CPUs had, meaning that we must enable 'mmu' by hand when using the
rva22s64 profile CPU.

Given that this profile is setting a satp mode, it already implies that
we need a 'mmu'. Enable the 'mmu' in this case.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/tcg/tcg-cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index da437975b4..88f92d1c7d 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1107,6 +1107,7 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
 
 #ifndef CONFIG_USER_ONLY
     if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
+        object_property_set_bool(obj, "mmu", true, NULL);
         const char *satp_prop = satp_mode_str(profile->satp_mode,
                                               riscv_cpu_is_32bit(cpu));
         object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
-- 
2.43.0

[PATCH 2/6] target/riscv: add riscv,isa to named features

[Daniel Henrique Barboza]

Further discussions after the introduction of rva22 support in QEMU
revealed that what we've been calling 'named features' are actually
regular extensions, with their respective riscv,isa DTs. This is
clarified in [1]. [2] is a bug tracker asking for the profile spec to be
less cryptic about it.

As far as QEMU goes we understand extensions as something that the user
can enable/disable in the command line. This isn't the case for named
features, so we'll have to reach a middle ground.

We'll keep our existing nomenclature 'named features' to refer to any
extension that the user can't control in the command line. We'll also do
the following:

- 'svade' and 'zic64b' flags are renamed to 'ext_svade' and
  'ext_zic64b'. 'ext_svade' and 'ext_zic64b' now have riscv,isa strings and
  priv_spec versions;

- skip name feature check in cpu_bump_multi_ext_priv_ver(). Now that
  named features have a riscv,isa and an entry in isa_edata_arr[] we
  don't need to gate the call to cpu_cfg_ext_get_min_version() anymore.

[1] https://github.com/riscv/riscv-profiles/issues/121
[2] https://github.com/riscv/riscv-profiles/issues/142

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 17 +++++++++++++----
 target/riscv/cpu_cfg.h     |  6 ++++--
 target/riscv/tcg/tcg-cpu.c | 16 ++++++----------
 3 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 88e8cc8681..28d3cfa8ce 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -97,6 +97,7 @@ bool riscv_cpu_option_set(const char *optname)
  * instead.
  */
 const RISCVIsaExtData isa_edata_arr[] = {
+    ISA_EXT_DATA_ENTRY(zic64b, PRIV_VERSION_1_12_0, ext_zic64b),
     ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
     ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
     ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
@@ -171,6 +172,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
     ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
     ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
+    ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
     ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
     ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
     ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
@@ -1510,9 +1512,16 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/*
+ * 'Named features' is the name we give to extensions that we
+ * don't want to expose to users. They are either immutable
+ * (always enabled/disable) or they'll vary depending on
+ * the resulting CPU state. They have riscv,isa strings
+ * and priv_ver like regular extensions.
+ */
 const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
-    MULTI_EXT_CFG_BOOL("svade", svade, true),
-    MULTI_EXT_CFG_BOOL("zic64b", zic64b, true),
+    MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
+    MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
 
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -2130,7 +2139,7 @@ static RISCVCPUProfile RVA22U64 = {
         CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
 
         /* mandatory named features for this profile */
-        CPU_CFG_OFFSET(zic64b),
+        CPU_CFG_OFFSET(ext_zic64b),
 
         RISCV_PROFILE_EXT_LIST_END
     }
@@ -2161,7 +2170,7 @@ static RISCVCPUProfile RVA22S64 = {
         CPU_CFG_OFFSET(ext_svinval),
 
         /* rva22s64 named features */
-        CPU_CFG_OFFSET(svade),
+        CPU_CFG_OFFSET(ext_svade),
 
         RISCV_PROFILE_EXT_LIST_END
     }
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index e241922f89..698f926ab1 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -117,13 +117,15 @@ struct RISCVCPUConfig {
     bool ext_smepmp;
     bool rvv_ta_all_1s;
     bool rvv_ma_all_1s;
-    bool svade;
-    bool zic64b;
 
     uint32_t mvendorid;
     uint64_t marchid;
     uint64_t mimpid;
 
+    /* Named features  */
+    bool ext_svade;
+    bool ext_zic64b;
+
     /* Vendor-specific custom extensions */
     bool ext_xtheadba;
     bool ext_xtheadbb;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 88f92d1c7d..90861cc065 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -197,12 +197,12 @@ static bool cpu_cfg_offset_is_named_feat(uint32_t ext_offset)
 static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
 {
     switch (feat_offset) {
-    case CPU_CFG_OFFSET(zic64b):
+    case CPU_CFG_OFFSET(ext_zic64b):
         cpu->cfg.cbom_blocksize = 64;
         cpu->cfg.cbop_blocksize = 64;
         cpu->cfg.cboz_blocksize = 64;
         break;
-    case CPU_CFG_OFFSET(svade):
+    case CPU_CFG_OFFSET(ext_svade):
         cpu->cfg.ext_svadu = false;
         break;
     default:
@@ -219,10 +219,6 @@ static void cpu_bump_multi_ext_priv_ver(CPURISCVState *env,
         return;
     }
 
-    if (cpu_cfg_offset_is_named_feat(ext_offset)) {
-        return;
-    }
-
     ext_priv_ver = cpu_cfg_ext_get_min_version(ext_offset);
 
     if (env->priv_ver < ext_priv_ver) {
@@ -349,11 +345,11 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
 
 static void riscv_cpu_update_named_features(RISCVCPU *cpu)
 {
-    cpu->cfg.zic64b = cpu->cfg.cbom_blocksize == 64 &&
-                      cpu->cfg.cbop_blocksize == 64 &&
-                      cpu->cfg.cboz_blocksize == 64;
+    cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 &&
+                          cpu->cfg.cbop_blocksize == 64 &&
+                          cpu->cfg.cboz_blocksize == 64;
 
-    cpu->cfg.svade = !cpu->cfg.ext_svadu;
+    cpu->cfg.ext_svade = !cpu->cfg.ext_svadu;
 }
 
 static void riscv_cpu_validate_g(RISCVCPU *cpu)
-- 
2.43.0

[PATCH 3/6] target/riscv: add remaining named features

[Daniel Henrique Barboza]

The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
until now, we were implying that they were available.

We can't do this anymore since named features also has a riscv,isa
entry.  Let's add them to riscv_cpu_named_features[].

They will also need to be explicitly enabled in both profile
descriptions. TCG will enable the named features it already implements,
other accelerators are free to handle it as they like.

After this patch, here's the riscv,isa from a buildroot using the
'rva22s64' CPU:

 # cat /proc/device-tree/cpus/cpu@0/riscv,isa
rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
zbs_zkt_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 41 +++++++++++++++++++++++++++++---------
 target/riscv/cpu_cfg.h     |  9 +++++++++
 target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++-
 3 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 28d3cfa8ce..1ecd8a57ed 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
     ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
     ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
+    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa),
+    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif),
+    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm),
+    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse),
     ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
     ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
     ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
@@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
     ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
     ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
+    ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs),
     ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
     ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
     ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
@@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
     ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
     ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
+    ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr),
     ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
+    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw),
     ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
+    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala),
+    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd),
     ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
     ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
     ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
@@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
     MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
     MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
 
+    /*
+     * cache-related extensions that are always enabled
+     * since QEMU RISC-V does not have a cache model.
+     */
+    MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true),
+    MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true),
+    MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true),
+    MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true),
+    MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true),
+    MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true),
+
+    /* Other named features that QEMU TCG always implements */
+    MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true),
+    MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true),
+    MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true),
+
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = {
 };
 
 /*
- * RVA22U64 defines some 'named features' or 'synthetic extensions'
- * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
- * and Zicclsm. We do not implement caching in QEMU so we'll consider
- * all these named features as always enabled.
- *
- * There's no riscv,isa update for them (nor for zic64b, despite it
- * having a cfg offset) at this moment.
+ * RVA22U64 defines some cache related extensions: Za64rs,
+ * Ziccif, Ziccrse, Ziccamoa and Zicclsm.
  */
 static RISCVCPUProfile RVA22U64 = {
     .parent = NULL,
@@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = {
         CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
 
         /* mandatory named features for this profile */
-        CPU_CFG_OFFSET(ext_zic64b),
+        CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b),
+        CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse),
+        CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm),
 
         RISCV_PROFILE_EXT_LIST_END
     }
@@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = {
         CPU_CFG_OFFSET(ext_svinval),
 
         /* rva22s64 named features */
-        CPU_CFG_OFFSET(ext_svade),
+        CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
+        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
 
         RISCV_PROFILE_EXT_LIST_END
     }
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 698f926ab1..f79fc3dfd1 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -125,6 +125,15 @@ struct RISCVCPUConfig {
     /* Named features  */
     bool ext_svade;
     bool ext_zic64b;
+    bool ext_za64rs;
+    bool ext_ziccif;
+    bool ext_ziccrse;
+    bool ext_ziccamoa;
+    bool ext_zicclsm;
+    bool ext_ssccptr;
+    bool ext_sstvecd;
+    bool ext_sstvala;
+    bool ext_sscounterenw;
 
     /* Vendor-specific custom extensions */
     bool ext_xtheadba;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 90861cc065..6d5028cf84 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
         cpu->cfg.ext_svadu = false;
         break;
     default:
-        g_assert_not_reached();
+        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
+        return;
     }
 }
 
@@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
     return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
 }
 
+/* Named features that TCG always implements */
+static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu)
+{
+    cpu->cfg.ext_za64rs = true;
+    cpu->cfg.ext_ziccif = true;
+    cpu->cfg.ext_ziccrse = true;
+    cpu->cfg.ext_ziccamoa = true;
+    cpu->cfg.ext_zicclsm = true;
+    cpu->cfg.ext_ssccptr = true;
+    cpu->cfg.ext_sstvecd = true;
+    cpu->cfg.ext_sstvala = true;
+    cpu->cfg.ext_sscounterenw = true;
+}
+
 static void riscv_tcg_cpu_instance_init(CPUState *cs)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
@@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
     if (riscv_cpu_has_max_extensions(obj)) {
         riscv_init_max_cpu_extensions(obj);
     }
+
+    riscv_tcg_cpu_enable_named_feats(cpu);
 }
 
 static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
-- 
2.43.0

[PATCH 4/6] target/riscv: Reset henvcfg to zero

[Daniel Henrique Barboza]

From: Andrew Jones <ajones@ventanamicro.com>

The hypervisor should decide what it wants to enable. Zero all
configuration enable bits on reset.

Also, commit ed67d63798f2 ("target/riscv: Update CSR bits name for
svadu extension") missed one reference to 'hade'. Change it now.

Fixes: 0af3f115e68e ("target/riscv: Add *envcfg.HADE related check in address translation")
Fixes: ed67d63798f2 ("target/riscv: Update CSR bits name for svadu extension")
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 3 +--
 target/riscv/csr.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1ecd8a57ed..7fd433daee 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -961,8 +961,7 @@ static void riscv_cpu_reset_hold(Object *obj)
 
     env->menvcfg = (cpu->cfg.ext_svpbmt ? MENVCFG_PBMTE : 0) |
                    (cpu->cfg.ext_svadu ? MENVCFG_ADUE : 0);
-    env->henvcfg = (cpu->cfg.ext_svpbmt ? HENVCFG_PBMTE : 0) |
-                   (cpu->cfg.ext_svadu ? HENVCFG_ADUE : 0);
+    env->henvcfg = 0;
 
     /* Initialized default priorities of local interrupts. */
     for (i = 0; i < ARRAY_SIZE(env->miprio); i++) {
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d9a010387f..93f7bc2cb4 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2115,7 +2115,7 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
     /*
      * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
      * henvcfg.stce is read_only 0 when menvcfg.stce = 0
-     * henvcfg.hade is read_only 0 when menvcfg.hade = 0
+     * henvcfg.adue is read_only 0 when menvcfg.adue = 0
      */
     *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) |
                            env->menvcfg);
-- 
2.43.0

[PATCH 5/6] target/riscv: Gate hardware A/D PTE bit updating

[Daniel Henrique Barboza]

From: Andrew Jones <ajones@ventanamicro.com>

Gate hardware A/D PTE bit updating on {m,h}envcfg.ADUE and only
enable menvcfg.ADUE on reset if svade has not been selected. Now
that we also consider svade, we have four possible configurations:

 1) !svade && !svadu
    use hardware updating and there's no way to disable it
    (the default, which maintains past behavior. Maintaining
     the default, even with !svadu is a change that fixes [1])

 2) !svade && svadu
    use hardware updating, but also provide {m,h}envcfg.ADUE,
    allowing software to switch to exception mode
    (being able to switch is a change which fixes [1])

 3) svade && !svadu
    use exception mode and there's no way to switch to hardware
    updating
    (this behavior change fixes [2])

 4) svade && svadu
    use exception mode, but also provide {m,h}envcfg.ADUE,
    allowing software to switch to hardware updating
    (this behavior change fixes [2])

Fixes: 0af3f115e68e ("target/riscv: Add *envcfg.HADE related check in address translation") [1]
Fixes: 48531f5adb2a ("target/riscv: implement svade") [2]
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         |  2 +-
 target/riscv/cpu_helper.c  | 18 ++++++++++++++----
 target/riscv/tcg/tcg-cpu.c | 16 +++++-----------
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7fd433daee..a56c2ff91d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -960,7 +960,7 @@ static void riscv_cpu_reset_hold(Object *obj)
     env->two_stage_lookup = false;
 
     env->menvcfg = (cpu->cfg.ext_svpbmt ? MENVCFG_PBMTE : 0) |
-                   (cpu->cfg.ext_svadu ? MENVCFG_ADUE : 0);
+                   (!cpu->cfg.ext_svade && cpu->cfg.ext_svadu ? MENVCFG_ADUE : 0);
     env->henvcfg = 0;
 
     /* Initialized default priorities of local interrupts. */
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 8da9104da4..9da9758cb4 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -907,7 +907,9 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
     }
 
     bool pbmte = env->menvcfg & MENVCFG_PBMTE;
-    bool adue = env->menvcfg & MENVCFG_ADUE;
+    bool svade = riscv_cpu_cfg(env)->ext_svade;
+    bool svadu = riscv_cpu_cfg(env)->ext_svadu;
+    bool adue = svadu ? env->menvcfg & MENVCFG_ADUE : !svade;
 
     if (first_stage && two_stage && env->virt_enabled) {
         pbmte = pbmte && (env->henvcfg & HENVCFG_PBMTE);
@@ -1082,9 +1084,17 @@ restart:
         return TRANSLATE_FAIL;
     }
 
-    /* If necessary, set accessed and dirty bits. */
-    target_ulong updated_pte = pte | PTE_A |
-                (access_type == MMU_DATA_STORE ? PTE_D : 0);
+    target_ulong updated_pte = pte;
+
+    /*
+     * If ADUE is enabled, set accessed and dirty bits.
+     * Otherwise raise an exception if necessary.
+     */
+    if (adue) {
+        updated_pte |= PTE_A | (access_type == MMU_DATA_STORE ? PTE_D : 0);
+    } else if (!(pte & PTE_A) || (access_type == MMU_DATA_STORE && !(pte & PTE_D))) {
+        return TRANSLATE_FAIL;
+    }
 
     /* Page table updates need to be atomic with MTTCG enabled */
     if (updated_pte != pte && !is_debug) {
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 6d5028cf84..bc3c45b117 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -196,18 +196,14 @@ static bool cpu_cfg_offset_is_named_feat(uint32_t ext_offset)
 
 static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
 {
-    switch (feat_offset) {
-    case CPU_CFG_OFFSET(ext_zic64b):
+     /*
+      * All other named features are already enabled
+      * in riscv_tcg_cpu_instance_init().
+      */
+    if (feat_offset == CPU_CFG_OFFSET(ext_zic64b)) {
         cpu->cfg.cbom_blocksize = 64;
         cpu->cfg.cbop_blocksize = 64;
         cpu->cfg.cboz_blocksize = 64;
-        break;
-    case CPU_CFG_OFFSET(ext_svade):
-        cpu->cfg.ext_svadu = false;
-        break;
-    default:
-        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
-        return;
     }
 }
 
@@ -349,8 +345,6 @@ static void riscv_cpu_update_named_features(RISCVCPU *cpu)
     cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 &&
                           cpu->cfg.cbop_blocksize == 64 &&
                           cpu->cfg.cboz_blocksize == 64;
-
-    cpu->cfg.ext_svade = !cpu->cfg.ext_svadu;
 }
 
 static void riscv_cpu_validate_g(RISCVCPU *cpu)
-- 
2.43.0

[PATCH 6/6] target/riscv: Promote svade to a normal extension

[Daniel Henrique Barboza]

From: Andrew Jones <ajones@ventanamicro.com>

Named features are extensions which don't make sense for users to
control and are therefore not exposed on the command line. However,
svade is an extension which makes sense for users to control, so treat
it like a "normal" extension. The default is false, since QEMU has
always implemented hardware A/D PTE bit updating, so users must opt into
svade (or get it from a CPU type which enables it by default).

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index a56c2ff91d..4ddde25412 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1421,6 +1421,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
 
     MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
     MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
+    MULTI_EXT_CFG_BOOL("svade", ext_svade, false),
     MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
     MULTI_EXT_CFG_BOOL("svinval", ext_svinval, false),
     MULTI_EXT_CFG_BOOL("svnapot", ext_svnapot, false),
@@ -1528,7 +1529,6 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
  * and priv_ver like regular extensions.
  */
 const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
-    MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
     MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
 
     /*
@@ -2175,8 +2175,6 @@ static RISCVCPUProfile RVA22U64 = {
  * Other named features that we already implement: Sstvecd, Sstvala,
  * Sscounterenw
  *
- * Named features that we need to enable: svade
- *
  * The remaining features/extensions comes from RVA22U64.
  */
 static RISCVCPUProfile RVA22S64 = {
@@ -2188,11 +2186,11 @@ static RISCVCPUProfile RVA22S64 = {
     .ext_offsets = {
         /* rva22s64 exts */
         CPU_CFG_OFFSET(ext_zifencei), CPU_CFG_OFFSET(ext_svpbmt),
-        CPU_CFG_OFFSET(ext_svinval),
+        CPU_CFG_OFFSET(ext_svinval), CPU_CFG_OFFSET(ext_svade),
 
         /* rva22s64 named features */
         CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
-        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
+        CPU_CFG_OFFSET(ext_sscounterenw),
 
         RISCV_PROFILE_EXT_LIST_END
     }
-- 
2.43.0

Re: [PATCH 6/6] target/riscv: Promote svade to a normal extension

[Andrew Jones]

On Thu, Jan 25, 2024 at 04:53:19PM -0300, Daniel Henrique Barboza wrote:
> From: Andrew Jones <ajones@ventanamicro.com>
> 
> Named features are extensions which don't make sense for users to
> control and are therefore not exposed on the command line. However,
> svade is an extension which makes sense for users to control, so treat
> it like a "normal" extension. The default is false, since QEMU has
> always implemented hardware A/D PTE bit updating, so users must opt into
> svade (or get it from a CPU type which enables it by default).
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index a56c2ff91d..4ddde25412 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1421,6 +1421,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>  
>      MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
>      MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> +    MULTI_EXT_CFG_BOOL("svade", ext_svade, false),

I forgot that the 'max' cpu type will ignore this off by default setting
and enable svade by default. I'll send a v2 of this series where I ensure
svade for 'max' also defaults false.

Thanks,
drew

>      MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
>      MULTI_EXT_CFG_BOOL("svinval", ext_svinval, false),
>      MULTI_EXT_CFG_BOOL("svnapot", ext_svnapot, false),
> @@ -1528,7 +1529,6 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
>   * and priv_ver like regular extensions.
>   */
>  const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
> -    MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
>      MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
>  
>      /*
> @@ -2175,8 +2175,6 @@ static RISCVCPUProfile RVA22U64 = {
>   * Other named features that we already implement: Sstvecd, Sstvala,
>   * Sscounterenw
>   *
> - * Named features that we need to enable: svade
> - *
>   * The remaining features/extensions comes from RVA22U64.
>   */
>  static RISCVCPUProfile RVA22S64 = {
> @@ -2188,11 +2186,11 @@ static RISCVCPUProfile RVA22S64 = {
>      .ext_offsets = {
>          /* rva22s64 exts */
>          CPU_CFG_OFFSET(ext_zifencei), CPU_CFG_OFFSET(ext_svpbmt),
> -        CPU_CFG_OFFSET(ext_svinval),
> +        CPU_CFG_OFFSET(ext_svinval), CPU_CFG_OFFSET(ext_svade),
>  
>          /* rva22s64 named features */
>          CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
> -        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
> +        CPU_CFG_OFFSET(ext_sscounterenw),
>  
>          RISCV_PROFILE_EXT_LIST_END
>      }
> -- 
> 2.43.0
> 

Re: [PATCH 1/6] target/riscv/tcg: set 'mmu' with 'satp' in cpu_set_profile()

[Alistair Francis]

On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Recent changes in options handling removed the 'mmu' default the bare
> CPUs had, meaning that we must enable 'mmu' by hand when using the
> rva22s64 profile CPU.
>
> Given that this profile is setting a satp mode, it already implies that
> we need a 'mmu'. Enable the 'mmu' in this case.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index da437975b4..88f92d1c7d 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1107,6 +1107,7 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>
>  #ifndef CONFIG_USER_ONLY
>      if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> +        object_property_set_bool(obj, "mmu", true, NULL);
>          const char *satp_prop = satp_mode_str(profile->satp_mode,
>                                                riscv_cpu_is_32bit(cpu));
>          object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
> --
> 2.43.0
>
>

Re: [PATCH 2/6] target/riscv: add riscv,isa to named features

[Alistair Francis]

On Fri, Jan 26, 2024 at 6:55 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Further discussions after the introduction of rva22 support in QEMU
> revealed that what we've been calling 'named features' are actually
> regular extensions, with their respective riscv,isa DTs. This is
> clarified in [1]. [2] is a bug tracker asking for the profile spec to be
> less cryptic about it.
>
> As far as QEMU goes we understand extensions as something that the user
> can enable/disable in the command line. This isn't the case for named
> features, so we'll have to reach a middle ground.
>
> We'll keep our existing nomenclature 'named features' to refer to any
> extension that the user can't control in the command line. We'll also do
> the following:
>
> - 'svade' and 'zic64b' flags are renamed to 'ext_svade' and
>   'ext_zic64b'. 'ext_svade' and 'ext_zic64b' now have riscv,isa strings and
>   priv_spec versions;
>
> - skip name feature check in cpu_bump_multi_ext_priv_ver(). Now that
>   named features have a riscv,isa and an entry in isa_edata_arr[] we
>   don't need to gate the call to cpu_cfg_ext_get_min_version() anymore.
>
> [1] https://github.com/riscv/riscv-profiles/issues/121
> [2] https://github.com/riscv/riscv-profiles/issues/142
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 17 +++++++++++++----
>  target/riscv/cpu_cfg.h     |  6 ++++--
>  target/riscv/tcg/tcg-cpu.c | 16 ++++++----------
>  3 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 88e8cc8681..28d3cfa8ce 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -97,6 +97,7 @@ bool riscv_cpu_option_set(const char *optname)
>   * instead.
>   */
>  const RISCVIsaExtData isa_edata_arr[] = {
> +    ISA_EXT_DATA_ENTRY(zic64b, PRIV_VERSION_1_12_0, ext_zic64b),
>      ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
>      ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
>      ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> @@ -171,6 +172,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
>      ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>      ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> +    ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
>      ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
>      ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
>      ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
> @@ -1510,9 +1512,16 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +/*
> + * 'Named features' is the name we give to extensions that we
> + * don't want to expose to users. They are either immutable
> + * (always enabled/disable) or they'll vary depending on
> + * the resulting CPU state. They have riscv,isa strings
> + * and priv_ver like regular extensions.
> + */
>  const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
> -    MULTI_EXT_CFG_BOOL("svade", svade, true),
> -    MULTI_EXT_CFG_BOOL("zic64b", zic64b, true),
> +    MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
> +    MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
>
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -2130,7 +2139,7 @@ static RISCVCPUProfile RVA22U64 = {
>          CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
>
>          /* mandatory named features for this profile */
> -        CPU_CFG_OFFSET(zic64b),
> +        CPU_CFG_OFFSET(ext_zic64b),
>
>          RISCV_PROFILE_EXT_LIST_END
>      }
> @@ -2161,7 +2170,7 @@ static RISCVCPUProfile RVA22S64 = {
>          CPU_CFG_OFFSET(ext_svinval),
>
>          /* rva22s64 named features */
> -        CPU_CFG_OFFSET(svade),
> +        CPU_CFG_OFFSET(ext_svade),
>
>          RISCV_PROFILE_EXT_LIST_END
>      }
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index e241922f89..698f926ab1 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -117,13 +117,15 @@ struct RISCVCPUConfig {
>      bool ext_smepmp;
>      bool rvv_ta_all_1s;
>      bool rvv_ma_all_1s;
> -    bool svade;
> -    bool zic64b;
>
>      uint32_t mvendorid;
>      uint64_t marchid;
>      uint64_t mimpid;
>
> +    /* Named features  */
> +    bool ext_svade;
> +    bool ext_zic64b;
> +
>      /* Vendor-specific custom extensions */
>      bool ext_xtheadba;
>      bool ext_xtheadbb;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 88f92d1c7d..90861cc065 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -197,12 +197,12 @@ static bool cpu_cfg_offset_is_named_feat(uint32_t ext_offset)
>  static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
>  {
>      switch (feat_offset) {
> -    case CPU_CFG_OFFSET(zic64b):
> +    case CPU_CFG_OFFSET(ext_zic64b):
>          cpu->cfg.cbom_blocksize = 64;
>          cpu->cfg.cbop_blocksize = 64;
>          cpu->cfg.cboz_blocksize = 64;
>          break;
> -    case CPU_CFG_OFFSET(svade):
> +    case CPU_CFG_OFFSET(ext_svade):
>          cpu->cfg.ext_svadu = false;
>          break;
>      default:
> @@ -219,10 +219,6 @@ static void cpu_bump_multi_ext_priv_ver(CPURISCVState *env,
>          return;
>      }
>
> -    if (cpu_cfg_offset_is_named_feat(ext_offset)) {
> -        return;
> -    }
> -
>      ext_priv_ver = cpu_cfg_ext_get_min_version(ext_offset);
>
>      if (env->priv_ver < ext_priv_ver) {
> @@ -349,11 +345,11 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>
>  static void riscv_cpu_update_named_features(RISCVCPU *cpu)
>  {
> -    cpu->cfg.zic64b = cpu->cfg.cbom_blocksize == 64 &&
> -                      cpu->cfg.cbop_blocksize == 64 &&
> -                      cpu->cfg.cboz_blocksize == 64;
> +    cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 &&
> +                          cpu->cfg.cbop_blocksize == 64 &&
> +                          cpu->cfg.cboz_blocksize == 64;
>
> -    cpu->cfg.svade = !cpu->cfg.ext_svadu;
> +    cpu->cfg.ext_svade = !cpu->cfg.ext_svadu;
>  }
>
>  static void riscv_cpu_validate_g(RISCVCPU *cpu)
> --
> 2.43.0
>
>

Re: [PATCH 3/6] target/riscv: add remaining named features

[Alistair Francis]

On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
> until now, we were implying that they were available.
>
> We can't do this anymore since named features also has a riscv,isa
> entry.  Let's add them to riscv_cpu_named_features[].
>
> They will also need to be explicitly enabled in both profile
> descriptions. TCG will enable the named features it already implements,
> other accelerators are free to handle it as they like.
>
> After this patch, here's the riscv,isa from a buildroot using the
> 'rva22s64' CPU:
>
>  # cat /proc/device-tree/cpus/cpu@0/riscv,isa
> rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
> zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
> zbs_zkt_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         | 41 +++++++++++++++++++++++++++++---------
>  target/riscv/cpu_cfg.h     |  9 +++++++++
>  target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++-
>  3 files changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 28d3cfa8ce..1ecd8a57ed 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
>      ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
>      ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa),
> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif),
> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm),
> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse),
>      ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
>      ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
>      ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
> @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
>      ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
>      ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
> +    ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs),
>      ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
>      ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
>      ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
> @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
>      ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
>      ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> +    ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr),
>      ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw),
>      ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala),
> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd),
>      ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
>      ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
>      ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
> @@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
>      MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
>      MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
>
> +    /*
> +     * cache-related extensions that are always enabled
> +     * since QEMU RISC-V does not have a cache model.
> +     */
> +    MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true),
> +    MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true),
> +    MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true),
> +    MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true),
> +    MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true),
> +    MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true),
> +
> +    /* Other named features that QEMU TCG always implements */
> +    MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true),
> +    MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true),
> +    MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true),
> +
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = {
>  };
>
>  /*
> - * RVA22U64 defines some 'named features' or 'synthetic extensions'
> - * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> - * and Zicclsm. We do not implement caching in QEMU so we'll consider
> - * all these named features as always enabled.
> - *
> - * There's no riscv,isa update for them (nor for zic64b, despite it
> - * having a cfg offset) at this moment.
> + * RVA22U64 defines some cache related extensions: Za64rs,
> + * Ziccif, Ziccrse, Ziccamoa and Zicclsm.
>   */
>  static RISCVCPUProfile RVA22U64 = {
>      .parent = NULL,
> @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = {
>          CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
>
>          /* mandatory named features for this profile */
> -        CPU_CFG_OFFSET(ext_zic64b),
> +        CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b),
> +        CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse),
> +        CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm),
>
>          RISCV_PROFILE_EXT_LIST_END
>      }
> @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = {
>          CPU_CFG_OFFSET(ext_svinval),
>
>          /* rva22s64 named features */
> -        CPU_CFG_OFFSET(ext_svade),
> +        CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
> +        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
>
>          RISCV_PROFILE_EXT_LIST_END
>      }
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 698f926ab1..f79fc3dfd1 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -125,6 +125,15 @@ struct RISCVCPUConfig {
>      /* Named features  */
>      bool ext_svade;
>      bool ext_zic64b;
> +    bool ext_za64rs;
> +    bool ext_ziccif;
> +    bool ext_ziccrse;
> +    bool ext_ziccamoa;
> +    bool ext_zicclsm;
> +    bool ext_ssccptr;
> +    bool ext_sstvecd;
> +    bool ext_sstvala;
> +    bool ext_sscounterenw;

Overall this and the previous patch look fine.

One thing though, why store this information? I feel it could be
confusing having these variables. If a developer sets them to false
nothing actually happens, which is a little misleading

Alistair

>
>      /* Vendor-specific custom extensions */
>      bool ext_xtheadba;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 90861cc065..6d5028cf84 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
>          cpu->cfg.ext_svadu = false;
>          break;
>      default:
> -        g_assert_not_reached();
> +        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
> +        return;
>      }
>  }
>
> @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
>      return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
>  }
>
> +/* Named features that TCG always implements */
> +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu)
> +{
> +    cpu->cfg.ext_za64rs = true;
> +    cpu->cfg.ext_ziccif = true;
> +    cpu->cfg.ext_ziccrse = true;
> +    cpu->cfg.ext_ziccamoa = true;
> +    cpu->cfg.ext_zicclsm = true;
> +    cpu->cfg.ext_ssccptr = true;
> +    cpu->cfg.ext_sstvecd = true;
> +    cpu->cfg.ext_sstvala = true;
> +    cpu->cfg.ext_sscounterenw = true;
> +}
> +
>  static void riscv_tcg_cpu_instance_init(CPUState *cs)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
> @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
>      if (riscv_cpu_has_max_extensions(obj)) {
>          riscv_init_max_cpu_extensions(obj);
>      }
> +
> +    riscv_tcg_cpu_enable_named_feats(cpu);
>  }
>
>  static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> --
> 2.43.0
>
>

Re: [PATCH 3/6] target/riscv: add remaining named features

[Daniel Henrique Barboza]

On 1/29/24 22:10, Alistair Francis wrote:
> On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
>> until now, we were implying that they were available.
>>
>> We can't do this anymore since named features also has a riscv,isa
>> entry.  Let's add them to riscv_cpu_named_features[].
>>
>> They will also need to be explicitly enabled in both profile
>> descriptions. TCG will enable the named features it already implements,
>> other accelerators are free to handle it as they like.
>>
>> After this patch, here's the riscv,isa from a buildroot using the
>> 'rva22s64' CPU:
>>
>>   # cat /proc/device-tree/cpus/cpu@0/riscv,isa
>> rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
>> zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
>> zbs_zkt_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c         | 41 +++++++++++++++++++++++++++++---------
>>   target/riscv/cpu_cfg.h     |  9 +++++++++
>>   target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++-
>>   3 files changed, 59 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 28d3cfa8ce..1ecd8a57ed 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>       ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
>>       ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
>>       ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
>> +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa),
>> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif),
>> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm),
>> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse),
>>       ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
>>       ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
>>       ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
>> @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>       ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
>>       ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
>>       ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
>> +    ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs),
>>       ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
>>       ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
>>       ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
>> @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>       ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
>>       ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
>>       ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
>> +    ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr),
>>       ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw),
>>       ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
>> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala),
>> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd),
>>       ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
>>       ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
>>       ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
>> @@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
>>       MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
>>       MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
>>
>> +    /*
>> +     * cache-related extensions that are always enabled
>> +     * since QEMU RISC-V does not have a cache model.
>> +     */
>> +    MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true),
>> +    MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true),
>> +    MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true),
>> +    MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true),
>> +    MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true),
>> +    MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true),
>> +
>> +    /* Other named features that QEMU TCG always implements */
>> +    MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true),
>> +    MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true),
>> +    MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true),
>> +
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
>> @@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = {
>>   };
>>
>>   /*
>> - * RVA22U64 defines some 'named features' or 'synthetic extensions'
>> - * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
>> - * and Zicclsm. We do not implement caching in QEMU so we'll consider
>> - * all these named features as always enabled.
>> - *
>> - * There's no riscv,isa update for them (nor for zic64b, despite it
>> - * having a cfg offset) at this moment.
>> + * RVA22U64 defines some cache related extensions: Za64rs,
>> + * Ziccif, Ziccrse, Ziccamoa and Zicclsm.
>>    */
>>   static RISCVCPUProfile RVA22U64 = {
>>       .parent = NULL,
>> @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = {
>>           CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
>>
>>           /* mandatory named features for this profile */
>> -        CPU_CFG_OFFSET(ext_zic64b),
>> +        CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b),
>> +        CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse),
>> +        CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm),
>>
>>           RISCV_PROFILE_EXT_LIST_END
>>       }
>> @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = {
>>           CPU_CFG_OFFSET(ext_svinval),
>>
>>           /* rva22s64 named features */
>> -        CPU_CFG_OFFSET(ext_svade),
>> +        CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
>> +        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
>>
>>           RISCV_PROFILE_EXT_LIST_END
>>       }
>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>> index 698f926ab1..f79fc3dfd1 100644
>> --- a/target/riscv/cpu_cfg.h
>> +++ b/target/riscv/cpu_cfg.h
>> @@ -125,6 +125,15 @@ struct RISCVCPUConfig {
>>       /* Named features  */
>>       bool ext_svade;
>>       bool ext_zic64b;
>> +    bool ext_za64rs;
>> +    bool ext_ziccif;
>> +    bool ext_ziccrse;
>> +    bool ext_ziccamoa;
>> +    bool ext_zicclsm;
>> +    bool ext_ssccptr;
>> +    bool ext_sstvecd;
>> +    bool ext_sstvala;
>> +    bool ext_sscounterenw;
> 
> Overall this and the previous patch look fine.
> 
> One thing though, why store this information? I feel it could be
> confusing having these variables. If a developer sets them to false
> nothing actually happens, which is a little misleading

These extensions aren't being exposed to users. riscv_cpu_named_features[] isn't
being used to create any CPU user properties. I should've mentioned that in
patch 2 ...

As for the extra booleans that we'll be setting to 'true', as it is now
isa_edata_arr[] stores a string name, priv_ver and a cpu->cfg offset, so
everyone that adds a riscv,isa str must also have a valid bool offset in
RISCVCPUConfig. Having a bool also allow us to treat them as regular extensions
because we can re-use existing code to blindly enable them in profiles like
any other profile extension.

And, in case we need to promote them as regular user extensions, having the
booleans in place make it easier to do so. Patch 6 is doing that with 'svade'.


Thanks,

Daniel


We could create a single boolean that is always true in cpu->cfg and use it
for these entries. Another idea would be to change the riscv,isa functions to
handle these extensions separately, then we can add them in the array without
a valid cpu->cfg offfset.

> 
> Alistair
> 
>>
>>       /* Vendor-specific custom extensions */
>>       bool ext_xtheadba;
>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>> index 90861cc065..6d5028cf84 100644
>> --- a/target/riscv/tcg/tcg-cpu.c
>> +++ b/target/riscv/tcg/tcg-cpu.c
>> @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
>>           cpu->cfg.ext_svadu = false;
>>           break;
>>       default:
>> -        g_assert_not_reached();
>> +        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
>> +        return;
>>       }
>>   }
>>
>> @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
>>       return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
>>   }
>>
>> +/* Named features that TCG always implements */
>> +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu)
>> +{
>> +    cpu->cfg.ext_za64rs = true;
>> +    cpu->cfg.ext_ziccif = true;
>> +    cpu->cfg.ext_ziccrse = true;
>> +    cpu->cfg.ext_ziccamoa = true;
>> +    cpu->cfg.ext_zicclsm = true;
>> +    cpu->cfg.ext_ssccptr = true;
>> +    cpu->cfg.ext_sstvecd = true;
>> +    cpu->cfg.ext_sstvala = true;
>> +    cpu->cfg.ext_sscounterenw = true;
>> +}
>> +
>>   static void riscv_tcg_cpu_instance_init(CPUState *cs)
>>   {
>>       RISCVCPU *cpu = RISCV_CPU(cs);
>> @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
>>       if (riscv_cpu_has_max_extensions(obj)) {
>>           riscv_init_max_cpu_extensions(obj);
>>       }
>> +
>> +    riscv_tcg_cpu_enable_named_feats(cpu);
>>   }
>>
>>   static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
>> --
>> 2.43.0
>>
>>

Re: [PATCH 3/6] target/riscv: add remaining named features

[Alistair Francis]

On Thu, Feb 1, 2024 at 5:15 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/29/24 22:10, Alistair Francis wrote:
> > On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
> >> until now, we were implying that they were available.
> >>
> >> We can't do this anymore since named features also has a riscv,isa
> >> entry.  Let's add them to riscv_cpu_named_features[].
> >>
> >> They will also need to be explicitly enabled in both profile
> >> descriptions. TCG will enable the named features it already implements,
> >> other accelerators are free to handle it as they like.
> >>
> >> After this patch, here's the riscv,isa from a buildroot using the
> >> 'rva22s64' CPU:
> >>
> >>   # cat /proc/device-tree/cpus/cpu@0/riscv,isa
> >> rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
> >> zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
> >> zbs_zkt_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >>   target/riscv/cpu.c         | 41 +++++++++++++++++++++++++++++---------
> >>   target/riscv/cpu_cfg.h     |  9 +++++++++
> >>   target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++-
> >>   3 files changed, 59 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 28d3cfa8ce..1ecd8a57ed 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >>       ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
> >>       ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
> >>       ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> >> +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa),
> >> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif),
> >> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm),
> >> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse),
> >>       ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
> >>       ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
> >>       ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
> >> @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >>       ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
> >>       ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
> >>       ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
> >> +    ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs),
> >>       ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
> >>       ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
> >>       ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
> >> @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >>       ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> >>       ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> >>       ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> >> +    ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr),
> >>       ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> >> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw),
> >>       ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> >> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala),
> >> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd),
> >>       ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
> >>       ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
> >>       ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
> >> @@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
> >>       MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
> >>       MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
> >>
> >> +    /*
> >> +     * cache-related extensions that are always enabled
> >> +     * since QEMU RISC-V does not have a cache model.
> >> +     */
> >> +    MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true),
> >> +    MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true),
> >> +    MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true),
> >> +    MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true),
> >> +    MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true),
> >> +    MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true),
> >> +
> >> +    /* Other named features that QEMU TCG always implements */
> >> +    MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true),
> >> +    MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true),
> >> +    MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true),
> >> +
> >>       DEFINE_PROP_END_OF_LIST(),
> >>   };
> >>
> >> @@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = {
> >>   };
> >>
> >>   /*
> >> - * RVA22U64 defines some 'named features' or 'synthetic extensions'
> >> - * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> >> - * and Zicclsm. We do not implement caching in QEMU so we'll consider
> >> - * all these named features as always enabled.
> >> - *
> >> - * There's no riscv,isa update for them (nor for zic64b, despite it
> >> - * having a cfg offset) at this moment.
> >> + * RVA22U64 defines some cache related extensions: Za64rs,
> >> + * Ziccif, Ziccrse, Ziccamoa and Zicclsm.
> >>    */
> >>   static RISCVCPUProfile RVA22U64 = {
> >>       .parent = NULL,
> >> @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = {
> >>           CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
> >>
> >>           /* mandatory named features for this profile */
> >> -        CPU_CFG_OFFSET(ext_zic64b),
> >> +        CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b),
> >> +        CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse),
> >> +        CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm),
> >>
> >>           RISCV_PROFILE_EXT_LIST_END
> >>       }
> >> @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = {
> >>           CPU_CFG_OFFSET(ext_svinval),
> >>
> >>           /* rva22s64 named features */
> >> -        CPU_CFG_OFFSET(ext_svade),
> >> +        CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
> >> +        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
> >>
> >>           RISCV_PROFILE_EXT_LIST_END
> >>       }
> >> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> >> index 698f926ab1..f79fc3dfd1 100644
> >> --- a/target/riscv/cpu_cfg.h
> >> +++ b/target/riscv/cpu_cfg.h
> >> @@ -125,6 +125,15 @@ struct RISCVCPUConfig {
> >>       /* Named features  */
> >>       bool ext_svade;
> >>       bool ext_zic64b;
> >> +    bool ext_za64rs;
> >> +    bool ext_ziccif;
> >> +    bool ext_ziccrse;
> >> +    bool ext_ziccamoa;
> >> +    bool ext_zicclsm;
> >> +    bool ext_ssccptr;
> >> +    bool ext_sstvecd;
> >> +    bool ext_sstvala;
> >> +    bool ext_sscounterenw;
> >
> > Overall this and the previous patch look fine.
> >
> > One thing though, why store this information? I feel it could be
> > confusing having these variables. If a developer sets them to false
> > nothing actually happens, which is a little misleading
>
> These extensions aren't being exposed to users. riscv_cpu_named_features[] isn't

Yep, we should not expose them to users.

I meant developers, as in people reading the C code and compiling QEMU.

> being used to create any CPU user properties. I should've mentioned that in
> patch 2 ...
>
> As for the extra booleans that we'll be setting to 'true', as it is now
> isa_edata_arr[] stores a string name, priv_ver and a cpu->cfg offset, so
> everyone that adds a riscv,isa str must also have a valid bool offset in
> RISCVCPUConfig. Having a bool also allow us to treat them as regular extensions
> because we can re-use existing code to blindly enable them in profiles like
> any other profile extension.

Yep, and I think that makes sense. The odd part is that we don't
actually use these bools. Which I feel is confusing when looking at
the QEMU code base. Why have a variable that we don't use?

>
> And, in case we need to promote them as regular user extensions, having the
> booleans in place make it easier to do so. Patch 6 is doing that with 'svade'.

If we plan to do that, I think that also makes sense. But I suspect no
one has any plant to convert some of these to real configuration
options.

>
>
> Thanks,
>
> Daniel
>
>
> We could create a single boolean that is always true in cpu->cfg and use it
> for these entries. Another idea would be to change the riscv,isa functions to
> handle these extensions separately, then we can add them in the array without
> a valid cpu->cfg offfset.

I think these are better ideas.

Alistair

>
> >
> > Alistair
> >
> >>
> >>       /* Vendor-specific custom extensions */
> >>       bool ext_xtheadba;
> >> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> >> index 90861cc065..6d5028cf84 100644
> >> --- a/target/riscv/tcg/tcg-cpu.c
> >> +++ b/target/riscv/tcg/tcg-cpu.c
> >> @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
> >>           cpu->cfg.ext_svadu = false;
> >>           break;
> >>       default:
> >> -        g_assert_not_reached();
> >> +        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
> >> +        return;
> >>       }
> >>   }
> >>
> >> @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
> >>       return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
> >>   }
> >>
> >> +/* Named features that TCG always implements */
> >> +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu)
> >> +{
> >> +    cpu->cfg.ext_za64rs = true;
> >> +    cpu->cfg.ext_ziccif = true;
> >> +    cpu->cfg.ext_ziccrse = true;
> >> +    cpu->cfg.ext_ziccamoa = true;
> >> +    cpu->cfg.ext_zicclsm = true;
> >> +    cpu->cfg.ext_ssccptr = true;
> >> +    cpu->cfg.ext_sstvecd = true;
> >> +    cpu->cfg.ext_sstvala = true;
> >> +    cpu->cfg.ext_sscounterenw = true;
> >> +}
> >> +
> >>   static void riscv_tcg_cpu_instance_init(CPUState *cs)
> >>   {
> >>       RISCVCPU *cpu = RISCV_CPU(cs);
> >> @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
> >>       if (riscv_cpu_has_max_extensions(obj)) {
> >>           riscv_init_max_cpu_extensions(obj);
> >>       }
> >> +
> >> +    riscv_tcg_cpu_enable_named_feats(cpu);
> >>   }
> >>
> >>   static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> >> --
> >> 2.43.0
> >>
> >>

Re: [PATCH 3/6] target/riscv: add remaining named features

[Daniel Henrique Barboza]

On 2/1/24 23:14, Alistair Francis wrote:
> On Thu, Feb 1, 2024 at 5:15 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 1/29/24 22:10, Alistair Francis wrote:
>>> On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>> The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
>>>> until now, we were implying that they were available.
>>>>
>>>> We can't do this anymore since named features also has a riscv,isa
>>>> entry.  Let's add them to riscv_cpu_named_features[].
>>>>
>>>> They will also need to be explicitly enabled in both profile
>>>> descriptions. TCG will enable the named features it already implements,
>>>> other accelerators are free to handle it as they like.
>>>>
>>>> After this patch, here's the riscv,isa from a buildroot using the
>>>> 'rva22s64' CPU:
>>>>
>>>>    # cat /proc/device-tree/cpus/cpu@0/riscv,isa
>>>> rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
>>>> zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
>>>> zbs_zkt_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> ---
>>>>    target/riscv/cpu.c         | 41 +++++++++++++++++++++++++++++---------
>>>>    target/riscv/cpu_cfg.h     |  9 +++++++++
>>>>    target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++-
>>>>    3 files changed, 59 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>> index 28d3cfa8ce..1ecd8a57ed 100644
>>>> --- a/target/riscv/cpu.c
>>>> +++ b/target/riscv/cpu.c
>>>> @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>>>        ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
>>>>        ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
>>>>        ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
>>>> +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa),
>>>> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif),
>>>> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm),
>>>> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse),
>>>>        ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
>>>>        ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
>>>>        ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
>>>> @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>>>        ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
>>>>        ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
>>>>        ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
>>>> +    ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs),
>>>>        ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
>>>>        ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
>>>>        ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
>>>> @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>>>        ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
>>>>        ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
>>>>        ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
>>>> +    ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr),
>>>>        ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>>>> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw),
>>>>        ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
>>>> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala),
>>>> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd),
>>>>        ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
>>>>        ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
>>>>        ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
>>>> @@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
>>>>        MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
>>>>        MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
>>>>
>>>> +    /*
>>>> +     * cache-related extensions that are always enabled
>>>> +     * since QEMU RISC-V does not have a cache model.
>>>> +     */
>>>> +    MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true),
>>>> +    MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true),
>>>> +    MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true),
>>>> +    MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true),
>>>> +    MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true),
>>>> +    MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true),
>>>> +
>>>> +    /* Other named features that QEMU TCG always implements */
>>>> +    MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true),
>>>> +    MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true),
>>>> +    MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true),
>>>> +
>>>>        DEFINE_PROP_END_OF_LIST(),
>>>>    };
>>>>
>>>> @@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = {
>>>>    };
>>>>
>>>>    /*
>>>> - * RVA22U64 defines some 'named features' or 'synthetic extensions'
>>>> - * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
>>>> - * and Zicclsm. We do not implement caching in QEMU so we'll consider
>>>> - * all these named features as always enabled.
>>>> - *
>>>> - * There's no riscv,isa update for them (nor for zic64b, despite it
>>>> - * having a cfg offset) at this moment.
>>>> + * RVA22U64 defines some cache related extensions: Za64rs,
>>>> + * Ziccif, Ziccrse, Ziccamoa and Zicclsm.
>>>>     */
>>>>    static RISCVCPUProfile RVA22U64 = {
>>>>        .parent = NULL,
>>>> @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = {
>>>>            CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
>>>>
>>>>            /* mandatory named features for this profile */
>>>> -        CPU_CFG_OFFSET(ext_zic64b),
>>>> +        CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b),
>>>> +        CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse),
>>>> +        CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm),
>>>>
>>>>            RISCV_PROFILE_EXT_LIST_END
>>>>        }
>>>> @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = {
>>>>            CPU_CFG_OFFSET(ext_svinval),
>>>>
>>>>            /* rva22s64 named features */
>>>> -        CPU_CFG_OFFSET(ext_svade),
>>>> +        CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
>>>> +        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
>>>>
>>>>            RISCV_PROFILE_EXT_LIST_END
>>>>        }
>>>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>>>> index 698f926ab1..f79fc3dfd1 100644
>>>> --- a/target/riscv/cpu_cfg.h
>>>> +++ b/target/riscv/cpu_cfg.h
>>>> @@ -125,6 +125,15 @@ struct RISCVCPUConfig {
>>>>        /* Named features  */
>>>>        bool ext_svade;
>>>>        bool ext_zic64b;
>>>> +    bool ext_za64rs;
>>>> +    bool ext_ziccif;
>>>> +    bool ext_ziccrse;
>>>> +    bool ext_ziccamoa;
>>>> +    bool ext_zicclsm;
>>>> +    bool ext_ssccptr;
>>>> +    bool ext_sstvecd;
>>>> +    bool ext_sstvala;
>>>> +    bool ext_sscounterenw;
>>>
>>> Overall this and the previous patch look fine.
>>>
>>> One thing though, why store this information? I feel it could be
>>> confusing having these variables. If a developer sets them to false
>>> nothing actually happens, which is a little misleading
>>
>> These extensions aren't being exposed to users. riscv_cpu_named_features[] isn't
> 
> Yep, we should not expose them to users.
> 
> I meant developers, as in people reading the C code and compiling QEMU.
> 
>> being used to create any CPU user properties. I should've mentioned that in
>> patch 2 ...
>>
>> As for the extra booleans that we'll be setting to 'true', as it is now
>> isa_edata_arr[] stores a string name, priv_ver and a cpu->cfg offset, so
>> everyone that adds a riscv,isa str must also have a valid bool offset in
>> RISCVCPUConfig. Having a bool also allow us to treat them as regular extensions
>> because we can re-use existing code to blindly enable them in profiles like
>> any other profile extension.
> 
> Yep, and I think that makes sense. The odd part is that we don't
> actually use these bools. Which I feel is confusing when looking at
> the QEMU code base. Why have a variable that we don't use?

Let's say that we create an "always_enabled" boolean in cpu->cfg, always set to
'true' during riscv_cpu_init(). Then we change riscv_cpu_named_features[] to
work like this:

       MULTI_EXT_CFG_BOOL("svade", always_enabled, true),
       MULTI_EXT_CFG_BOOL("zic64b", always_enabled, true),
       MULTI_EXT_CFG_BOOL("za64rs", always_enabled, true),
       MULTI_EXT_CFG_BOOL("ziccif", always_enabled, true),
       (...)

(I would create another macro for these named features but that's beside the point)

If we go this route then we'll need changes in how profiles declare these internal
extensions, because now we're using CPU_CFG_OFFSET() with a specific boolean for
each. I would remove them from the profile description, otherwise the code would
attempt to do stuff with an 'always_enabled' bool, and instead just cite in a
comment that these other profile mandatory extensions are always enabled.


Another change on top of my head would be in isa_edata_arr[]. In this case we
would do:


>>>> +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, always_enabled),
>>>> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, always_enabled),
>>>> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, always_enabled),
>>>> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, always_enabled),


There might be other changes needed but on top of my head I think this would work.
Is that what you're referring to?


Thanks,

Daniel

> 
>>
>> And, in case we need to promote them as regular user extensions, having the
>> booleans in place make it easier to do so. Patch 6 is doing that with 'svade'.
> 
> If we plan to do that, I think that also makes sense. But I suspect no
> one has any plant to convert some of these to real configuration
> options.
> 
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>
>> We could create a single boolean that is always true in cpu->cfg and use it
>> for these entries. Another idea would be to change the riscv,isa functions to
>> handle these extensions separately, then we can add them in the array without
>> a valid cpu->cfg offfset.
> 
> I think these are better ideas.
> 
> Alistair
> 
>>
>>>
>>> Alistair
>>>
>>>>
>>>>        /* Vendor-specific custom extensions */
>>>>        bool ext_xtheadba;
>>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>>> index 90861cc065..6d5028cf84 100644
>>>> --- a/target/riscv/tcg/tcg-cpu.c
>>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>>> @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
>>>>            cpu->cfg.ext_svadu = false;
>>>>            break;
>>>>        default:
>>>> -        g_assert_not_reached();
>>>> +        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
>>>> +        return;
>>>>        }
>>>>    }
>>>>
>>>> @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
>>>>        return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
>>>>    }
>>>>
>>>> +/* Named features that TCG always implements */
>>>> +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu)
>>>> +{
>>>> +    cpu->cfg.ext_za64rs = true;
>>>> +    cpu->cfg.ext_ziccif = true;
>>>> +    cpu->cfg.ext_ziccrse = true;
>>>> +    cpu->cfg.ext_ziccamoa = true;
>>>> +    cpu->cfg.ext_zicclsm = true;
>>>> +    cpu->cfg.ext_ssccptr = true;
>>>> +    cpu->cfg.ext_sstvecd = true;
>>>> +    cpu->cfg.ext_sstvala = true;
>>>> +    cpu->cfg.ext_sscounterenw = true;
>>>> +}
>>>> +
>>>>    static void riscv_tcg_cpu_instance_init(CPUState *cs)
>>>>    {
>>>>        RISCVCPU *cpu = RISCV_CPU(cs);
>>>> @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
>>>>        if (riscv_cpu_has_max_extensions(obj)) {
>>>>            riscv_init_max_cpu_extensions(obj);
>>>>        }
>>>> +
>>>> +    riscv_tcg_cpu_enable_named_feats(cpu);
>>>>    }
>>>>
>>>>    static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
>>>> --
>>>> 2.43.0
>>>>
>>>>

Re: [PATCH 3/6] target/riscv: add remaining named features

[Alistair Francis]

On Fri, Feb 2, 2024 at 7:44 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 2/1/24 23:14, Alistair Francis wrote:
> > On Thu, Feb 1, 2024 at 5:15 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >>
> >>
> >> On 1/29/24 22:10, Alistair Francis wrote:
> >>> On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza
> >>> <dbarboza@ventanamicro.com> wrote:
> >>>>
> >>>> The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
> >>>> until now, we were implying that they were available.
> >>>>
> >>>> We can't do this anymore since named features also has a riscv,isa
> >>>> entry.  Let's add them to riscv_cpu_named_features[].
> >>>>
> >>>> They will also need to be explicitly enabled in both profile
> >>>> descriptions. TCG will enable the named features it already implements,
> >>>> other accelerators are free to handle it as they like.
> >>>>
> >>>> After this patch, here's the riscv,isa from a buildroot using the
> >>>> 'rva22s64' CPU:
> >>>>
> >>>>    # cat /proc/device-tree/cpus/cpu@0/riscv,isa
> >>>> rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
> >>>> zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
> >>>> zbs_zkt_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> >>>>
> >>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>>> ---
> >>>>    target/riscv/cpu.c         | 41 +++++++++++++++++++++++++++++---------
> >>>>    target/riscv/cpu_cfg.h     |  9 +++++++++
> >>>>    target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++-
> >>>>    3 files changed, 59 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >>>> index 28d3cfa8ce..1ecd8a57ed 100644
> >>>> --- a/target/riscv/cpu.c
> >>>> +++ b/target/riscv/cpu.c
> >>>> @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >>>>        ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
> >>>>        ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
> >>>>        ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> >>>> +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa),
> >>>> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif),
> >>>> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm),
> >>>> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse),
> >>>>        ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
> >>>>        ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
> >>>>        ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
> >>>> @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >>>>        ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
> >>>>        ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
> >>>>        ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
> >>>> +    ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs),
> >>>>        ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
> >>>>        ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
> >>>>        ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
> >>>> @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >>>>        ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> >>>>        ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> >>>>        ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> >>>> +    ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr),
> >>>>        ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> >>>> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw),
> >>>>        ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> >>>> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala),
> >>>> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd),
> >>>>        ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
> >>>>        ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
> >>>>        ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
> >>>> @@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
> >>>>        MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
> >>>>        MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
> >>>>
> >>>> +    /*
> >>>> +     * cache-related extensions that are always enabled
> >>>> +     * since QEMU RISC-V does not have a cache model.
> >>>> +     */
> >>>> +    MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true),
> >>>> +    MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true),
> >>>> +    MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true),
> >>>> +    MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true),
> >>>> +    MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true),
> >>>> +    MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true),
> >>>> +
> >>>> +    /* Other named features that QEMU TCG always implements */
> >>>> +    MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true),
> >>>> +    MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true),
> >>>> +    MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true),
> >>>> +
> >>>>        DEFINE_PROP_END_OF_LIST(),
> >>>>    };
> >>>>
> >>>> @@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = {
> >>>>    };
> >>>>
> >>>>    /*
> >>>> - * RVA22U64 defines some 'named features' or 'synthetic extensions'
> >>>> - * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> >>>> - * and Zicclsm. We do not implement caching in QEMU so we'll consider
> >>>> - * all these named features as always enabled.
> >>>> - *
> >>>> - * There's no riscv,isa update for them (nor for zic64b, despite it
> >>>> - * having a cfg offset) at this moment.
> >>>> + * RVA22U64 defines some cache related extensions: Za64rs,
> >>>> + * Ziccif, Ziccrse, Ziccamoa and Zicclsm.
> >>>>     */
> >>>>    static RISCVCPUProfile RVA22U64 = {
> >>>>        .parent = NULL,
> >>>> @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = {
> >>>>            CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
> >>>>
> >>>>            /* mandatory named features for this profile */
> >>>> -        CPU_CFG_OFFSET(ext_zic64b),
> >>>> +        CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b),
> >>>> +        CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse),
> >>>> +        CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm),
> >>>>
> >>>>            RISCV_PROFILE_EXT_LIST_END
> >>>>        }
> >>>> @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = {
> >>>>            CPU_CFG_OFFSET(ext_svinval),
> >>>>
> >>>>            /* rva22s64 named features */
> >>>> -        CPU_CFG_OFFSET(ext_svade),
> >>>> +        CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
> >>>> +        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
> >>>>
> >>>>            RISCV_PROFILE_EXT_LIST_END
> >>>>        }
> >>>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> >>>> index 698f926ab1..f79fc3dfd1 100644
> >>>> --- a/target/riscv/cpu_cfg.h
> >>>> +++ b/target/riscv/cpu_cfg.h
> >>>> @@ -125,6 +125,15 @@ struct RISCVCPUConfig {
> >>>>        /* Named features  */
> >>>>        bool ext_svade;
> >>>>        bool ext_zic64b;
> >>>> +    bool ext_za64rs;
> >>>> +    bool ext_ziccif;
> >>>> +    bool ext_ziccrse;
> >>>> +    bool ext_ziccamoa;
> >>>> +    bool ext_zicclsm;
> >>>> +    bool ext_ssccptr;
> >>>> +    bool ext_sstvecd;
> >>>> +    bool ext_sstvala;
> >>>> +    bool ext_sscounterenw;
> >>>
> >>> Overall this and the previous patch look fine.
> >>>
> >>> One thing though, why store this information? I feel it could be
> >>> confusing having these variables. If a developer sets them to false
> >>> nothing actually happens, which is a little misleading
> >>
> >> These extensions aren't being exposed to users. riscv_cpu_named_features[] isn't
> >
> > Yep, we should not expose them to users.
> >
> > I meant developers, as in people reading the C code and compiling QEMU.
> >
> >> being used to create any CPU user properties. I should've mentioned that in
> >> patch 2 ...
> >>
> >> As for the extra booleans that we'll be setting to 'true', as it is now
> >> isa_edata_arr[] stores a string name, priv_ver and a cpu->cfg offset, so
> >> everyone that adds a riscv,isa str must also have a valid bool offset in
> >> RISCVCPUConfig. Having a bool also allow us to treat them as regular extensions
> >> because we can re-use existing code to blindly enable them in profiles like
> >> any other profile extension.
> >
> > Yep, and I think that makes sense. The odd part is that we don't
> > actually use these bools. Which I feel is confusing when looking at
> > the QEMU code base. Why have a variable that we don't use?
>
> Let's say that we create an "always_enabled" boolean in cpu->cfg, always set to
> 'true' during riscv_cpu_init(). Then we change riscv_cpu_named_features[] to
> work like this:
>
>        MULTI_EXT_CFG_BOOL("svade", always_enabled, true),
>        MULTI_EXT_CFG_BOOL("zic64b", always_enabled, true),
>        MULTI_EXT_CFG_BOOL("za64rs", always_enabled, true),
>        MULTI_EXT_CFG_BOOL("ziccif", always_enabled, true),
>        (...)
>
> (I would create another macro for these named features but that's beside the point)
>
> If we go this route then we'll need changes in how profiles declare these internal
> extensions, because now we're using CPU_CFG_OFFSET() with a specific boolean for
> each. I would remove them from the profile description, otherwise the code would
> attempt to do stuff with an 'always_enabled' bool, and instead just cite in a
> comment that these other profile mandatory extensions are always enabled.
>
>
> Another change on top of my head would be in isa_edata_arr[]. In this case we
> would do:
>
>
> >>>> +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, always_enabled),
> >>>> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, always_enabled),
> >>>> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, always_enabled),
> >>>> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, always_enabled),
>
>
> There might be other changes needed but on top of my head I think this would work.
> Is that what you're referring to?

I would prefer your other offer:

> Another idea would be to change the riscv,isa functions to
> handle these extensions separately, then we can add them in the array without
> a valid cpu->cfg offfset.

but this also works.

Alistair

>
>
> Thanks,
>
> Daniel
>
> >
> >>
> >> And, in case we need to promote them as regular user extensions, having the
> >> booleans in place make it easier to do so. Patch 6 is doing that with 'svade'.
> >
> > If we plan to do that, I think that also makes sense. But I suspect no
> > one has any plant to convert some of these to real configuration
> > options.
> >
> >>
> >>
> >> Thanks,
> >>
> >> Daniel
> >>
> >>
> >> We could create a single boolean that is always true in cpu->cfg and use it
> >> for these entries. Another idea would be to change the riscv,isa functions to
> >> handle these extensions separately, then we can add them in the array without
> >> a valid cpu->cfg offfset.
> >
> > I think these are better ideas.
> >
> > Alistair
> >
> >>
> >>>
> >>> Alistair
> >>>
> >>>>
> >>>>        /* Vendor-specific custom extensions */
> >>>>        bool ext_xtheadba;
> >>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> >>>> index 90861cc065..6d5028cf84 100644
> >>>> --- a/target/riscv/tcg/tcg-cpu.c
> >>>> +++ b/target/riscv/tcg/tcg-cpu.c
> >>>> @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
> >>>>            cpu->cfg.ext_svadu = false;
> >>>>            break;
> >>>>        default:
> >>>> -        g_assert_not_reached();
> >>>> +        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
> >>>> +        return;
> >>>>        }
> >>>>    }
> >>>>
> >>>> @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
> >>>>        return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
> >>>>    }
> >>>>
> >>>> +/* Named features that TCG always implements */
> >>>> +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu)
> >>>> +{
> >>>> +    cpu->cfg.ext_za64rs = true;
> >>>> +    cpu->cfg.ext_ziccif = true;
> >>>> +    cpu->cfg.ext_ziccrse = true;
> >>>> +    cpu->cfg.ext_ziccamoa = true;
> >>>> +    cpu->cfg.ext_zicclsm = true;
> >>>> +    cpu->cfg.ext_ssccptr = true;
> >>>> +    cpu->cfg.ext_sstvecd = true;
> >>>> +    cpu->cfg.ext_sstvala = true;
> >>>> +    cpu->cfg.ext_sscounterenw = true;
> >>>> +}
> >>>> +
> >>>>    static void riscv_tcg_cpu_instance_init(CPUState *cs)
> >>>>    {
> >>>>        RISCVCPU *cpu = RISCV_CPU(cs);
> >>>> @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
> >>>>        if (riscv_cpu_has_max_extensions(obj)) {
> >>>>            riscv_init_max_cpu_extensions(obj);
> >>>>        }
> >>>> +
> >>>> +    riscv_tcg_cpu_enable_named_feats(cpu);
> >>>>    }
> >>>>
> >>>>    static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> >>>> --
> >>>> 2.43.0
> >>>>
> >>>>