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

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

[Daniel Henrique Barboza]

Hi,

In this new version we changed patch 3 as suggested by Alistair in v1
[1]. Instead of creating individual always-true bool for each named
feature, create a bool flag will be always 'true' to be used as config
offset for these named extensions.

Patches based on riscv-to-apply.next.

Patches missing acks: patch 3.

Changes from v2:
- patch 3:
  - 'ext_always_enabled' bool added
  - individual always-enabled named features bools removed
- v2 link: https://lore.kernel.org/qemu-riscv/20240126133101.61344-8-ajones@ventanamicro.com/


[1] https://lore.kernel.org/qemu-riscv/20240125195319.329181-1-dbarboza@ventanamicro.com/

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         | 70 +++++++++++++++++++++++++++-----------
 target/riscv/cpu_cfg.h     | 12 +++++--
 target/riscv/cpu_helper.c  | 19 ++++++++---
 target/riscv/csr.c         |  2 +-
 target/riscv/tcg/tcg-cpu.c | 34 +++++++++---------
 5 files changed, 94 insertions(+), 43 deletions(-)

-- 
2.43.0

[PATCH v3 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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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 v3 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>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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 v3 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[].

Instead of adding one bool for each named feature that we'll always
implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
named features will point to it. This also means that KVM won't see
these features as always enable, which is our intention.

If any accelerator adds support to disable one of these features, we'll
have to promote them to regular extensions and allow users to disable it
via command line.

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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 28d3cfa8ce..94843c4f6e 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_always_enabled),
+    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled),
+    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled),
+    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled),
     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_always_enabled),
     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_always_enabled),
     ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
+    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled),
     ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
+    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled),
+    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled),
     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),
@@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+#define ALWAYS_ENABLED_FEATURE(_name) \
+    {.name = _name, \
+     .offset = CPU_CFG_OFFSET(ext_always_enabled), \
+     .enabled = true}
+
 /*
  * 'Named features' is the name we give to extensions that we
  * don't want to expose to users. They are either immutable
@@ -1523,6 +1537,23 @@ 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
+     * in TCG since QEMU RISC-V does not have a cache
+     * model.
+     */
+    ALWAYS_ENABLED_FEATURE("za64rs"),
+    ALWAYS_ENABLED_FEATURE("ziccif"),
+    ALWAYS_ENABLED_FEATURE("ziccrse"),
+    ALWAYS_ENABLED_FEATURE("ziccamoa"),
+    ALWAYS_ENABLED_FEATURE("zicclsm"),
+    ALWAYS_ENABLED_FEATURE("ssccptr"),
+
+    /* Other named features that TCG always implements */
+    ALWAYS_ENABLED_FEATURE("sstvecd"),
+    ALWAYS_ENABLED_FEATURE("sstvala"),
+    ALWAYS_ENABLED_FEATURE("sscounterenw"),
+
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2116,13 +2147,10 @@ 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 'named features' that are cache
+ * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
+ * and Zicclsm. They are always implemented in TCG and
+ * doesn't need to be manually enabled by the profile.
  */
 static RISCVCPUProfile RVA22U64 = {
     .parent = NULL,
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 698f926ab1..c5049ec664 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -126,6 +126,12 @@ struct RISCVCPUConfig {
     bool ext_svade;
     bool ext_zic64b;
 
+    /*
+     * Always 'true' boolean for named features
+     * TCG always implement/can't be disabled.
+     */
+    bool ext_always_enabled;
+
     /* 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 90861cc065..673097c6e4 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
     RISCVCPU *cpu = RISCV_CPU(cs);
     Object *obj = OBJECT(cpu);
 
+    cpu->cfg.ext_always_enabled = true;
+
     misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
     multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
     riscv_cpu_add_user_properties(obj);
-- 
2.43.0

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

[Andrew Jones]

On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
> 
> Instead of adding one bool for each named feature that we'll always
> implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> named features will point to it. This also means that KVM won't see
> these features as always enable, which is our intention.
> 
> If any accelerator adds support to disable one of these features, we'll
> have to promote them to regular extensions and allow users to disable it
> via command line.
> 
> 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         | 42 +++++++++++++++++++++++++++++++-------
>  target/riscv/cpu_cfg.h     |  6 ++++++
>  target/riscv/tcg/tcg-cpu.c |  2 ++
>  3 files changed, 43 insertions(+), 7 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

[Alistair Francis]

On Sat, Feb 3, 2024 at 1:24 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[].
>
> Instead of adding one bool for each named feature that we'll always
> implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> named features will point to it. This also means that KVM won't see
> these features as always enable, which is our intention.
>
> If any accelerator adds support to disable one of these features, we'll
> have to promote them to regular extensions and allow users to disable it
> via command line.
>
> 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 42 +++++++++++++++++++++++++++++++-------
>  target/riscv/cpu_cfg.h     |  6 ++++++
>  target/riscv/tcg/tcg-cpu.c |  2 ++
>  3 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 28d3cfa8ce..94843c4f6e 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_always_enabled),
> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled),
> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled),
> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled),
>      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_always_enabled),
>      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_always_enabled),
>      ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled),
>      ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled),
> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled),
>      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),
> @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +#define ALWAYS_ENABLED_FEATURE(_name) \
> +    {.name = _name, \
> +     .offset = CPU_CFG_OFFSET(ext_always_enabled), \
> +     .enabled = true}
> +
>  /*
>   * 'Named features' is the name we give to extensions that we
>   * don't want to expose to users. They are either immutable
> @@ -1523,6 +1537,23 @@ 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
> +     * in TCG since QEMU RISC-V does not have a cache
> +     * model.
> +     */
> +    ALWAYS_ENABLED_FEATURE("za64rs"),
> +    ALWAYS_ENABLED_FEATURE("ziccif"),
> +    ALWAYS_ENABLED_FEATURE("ziccrse"),
> +    ALWAYS_ENABLED_FEATURE("ziccamoa"),
> +    ALWAYS_ENABLED_FEATURE("zicclsm"),
> +    ALWAYS_ENABLED_FEATURE("ssccptr"),
> +
> +    /* Other named features that TCG always implements */
> +    ALWAYS_ENABLED_FEATURE("sstvecd"),
> +    ALWAYS_ENABLED_FEATURE("sstvala"),
> +    ALWAYS_ENABLED_FEATURE("sscounterenw"),
> +
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -2116,13 +2147,10 @@ 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 'named features' that are cache
> + * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> + * and Zicclsm. They are always implemented in TCG and
> + * doesn't need to be manually enabled by the profile.
>   */
>  static RISCVCPUProfile RVA22U64 = {
>      .parent = NULL,
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 698f926ab1..c5049ec664 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -126,6 +126,12 @@ struct RISCVCPUConfig {
>      bool ext_svade;
>      bool ext_zic64b;
>
> +    /*
> +     * Always 'true' boolean for named features
> +     * TCG always implement/can't be disabled.
> +     */
> +    bool ext_always_enabled;
> +
>      /* 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 90861cc065..673097c6e4 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      Object *obj = OBJECT(cpu);
>
> +    cpu->cfg.ext_always_enabled = true;
> +
>      misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>      multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>      riscv_cpu_add_user_properties(obj);
> --
> 2.43.0
>
>

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 8080 bytes --]

On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
> 
> Instead of adding one bool for each named feature that we'll always
> implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> named features will point to it. This also means that KVM won't see
> these features as always enable, which is our intention.
> 
> If any accelerator adds support to disable one of these features, we'll
> have to promote them to regular extensions and allow users to disable it
> via command line.
> 
> After this patch, here's the riscv,isa from a buildroot using the
> 'rva22s64' CPU:

Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
present in "u" profiles?

>  # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#

I want to raise my frustration with the crock we've been given here by
RVI. Any "named feature" that just creates a name for something that
already is assumed is completely useless, and DT property that is used
to communicate it's presence cannot be used - instead the property needs
to be inverted - indicating the absence of that named feature.

Without the inversion, software that parses "riscv,isa" cannot make any
determination based on the absence of the property - it could be parsing
an old DT that does not have the property or it could be parsing the DT
of a system that does not support the extension.

This is part of why I deprecated `riscv,isa`. It's the same problem as
with "zifencei" et al - does a system with `riscv,isa = "rv64imac"`
support fence.i?

Cheers,
Conor.

> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         | 42 +++++++++++++++++++++++++++++++-------
>  target/riscv/cpu_cfg.h     |  6 ++++++
>  target/riscv/tcg/tcg-cpu.c |  2 ++
>  3 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 28d3cfa8ce..94843c4f6e 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_always_enabled),
> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled),
> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled),
> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled),
>      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_always_enabled),
>      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_always_enabled),
>      ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled),
>      ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled),
> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled),
>      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),
> @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +#define ALWAYS_ENABLED_FEATURE(_name) \
> +    {.name = _name, \
> +     .offset = CPU_CFG_OFFSET(ext_always_enabled), \
> +     .enabled = true}
> +
>  /*
>   * 'Named features' is the name we give to extensions that we
>   * don't want to expose to users. They are either immutable
> @@ -1523,6 +1537,23 @@ 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
> +     * in TCG since QEMU RISC-V does not have a cache
> +     * model.
> +     */
> +    ALWAYS_ENABLED_FEATURE("za64rs"),
> +    ALWAYS_ENABLED_FEATURE("ziccif"),
> +    ALWAYS_ENABLED_FEATURE("ziccrse"),
> +    ALWAYS_ENABLED_FEATURE("ziccamoa"),
> +    ALWAYS_ENABLED_FEATURE("zicclsm"),
> +    ALWAYS_ENABLED_FEATURE("ssccptr"),
> +
> +    /* Other named features that TCG always implements */
> +    ALWAYS_ENABLED_FEATURE("sstvecd"),
> +    ALWAYS_ENABLED_FEATURE("sstvala"),
> +    ALWAYS_ENABLED_FEATURE("sscounterenw"),
> +
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2116,13 +2147,10 @@ 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 'named features' that are cache
> + * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> + * and Zicclsm. They are always implemented in TCG and
> + * doesn't need to be manually enabled by the profile.
>   */
>  static RISCVCPUProfile RVA22U64 = {
>      .parent = NULL,
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 698f926ab1..c5049ec664 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -126,6 +126,12 @@ struct RISCVCPUConfig {
>      bool ext_svade;
>      bool ext_zic64b;
>  
> +    /*
> +     * Always 'true' boolean for named features
> +     * TCG always implement/can't be disabled.
> +     */
> +    bool ext_always_enabled;
> +
>      /* 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 90861cc065..673097c6e4 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      Object *obj = OBJECT(cpu);
>  
> +    cpu->cfg.ext_always_enabled = true;
> +
>      misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>      multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>      riscv_cpu_add_user_properties(obj);
> -- 
> 2.43.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Daniel Henrique Barboza]

On 2/15/24 10:33, Conor Dooley wrote:
> On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
>>
>> Instead of adding one bool for each named feature that we'll always
>> implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
>> cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
>> named features will point to it. This also means that KVM won't see
>> these features as always enable, which is our intention.
>>
>> If any accelerator adds support to disable one of these features, we'll
>> have to promote them to regular extensions and allow users to disable it
>> via command line.
>>
>> After this patch, here's the riscv,isa from a buildroot using the
>> 'rva22s64' CPU:
> 
> Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
> present in "u" profiles?

According to the specs I've read  it seems  the S profiles includes all extensions
from U profiles. For RVA22:

"The RVA22S64 mandatory unprivileged extensions include all the mandatory
unprivileged extensions in RVA22U64."

So rva22s64 will have zicclsm and all other extensions from its U profile too.


> 
>>   # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> 
> I want to raise my frustration with the crock we've been given here by
> RVI. Any "named feature" that just creates a name for something that
> already is assumed is completely useless, and DT property that is used
> to communicate it's presence cannot be used - instead the property needs
> to be inverted - indicating the absence of that named feature.

Let's say that I'm not the biggest fan of how these profile extensions are being
dealt with in the spec :) the text is vague w.r.t whether zicclsm and others
are actual extensions, or a 'named feature'( like we're calling here in QEMU)
that is just a glorified way of saying, for example, "zic64b" instead of "all
cache blocks have 64 bytes".


Thanks,

Daniel

> 
> Without the inversion, software that parses "riscv,isa" cannot make any
> determination based on the absence of the property - it could be parsing
> an old DT that does not have the property or it could be parsing the DT
> of a system that does not support the extension.
> 
> This is part of why I deprecated `riscv,isa`. It's the same problem as
> with "zifencei" et al - does a system with `riscv,isa = "rv64imac"`
> support fence.i?
> 
> Cheers,
> Conor.
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c         | 42 +++++++++++++++++++++++++++++++-------
>>   target/riscv/cpu_cfg.h     |  6 ++++++
>>   target/riscv/tcg/tcg-cpu.c |  2 ++
>>   3 files changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 28d3cfa8ce..94843c4f6e 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_always_enabled),
>> +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled),
>> +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled),
>> +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled),
>>       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_always_enabled),
>>       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_always_enabled),
>>       ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>> +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled),
>>       ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
>> +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled),
>> +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled),
>>       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),
>> @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> +#define ALWAYS_ENABLED_FEATURE(_name) \
>> +    {.name = _name, \
>> +     .offset = CPU_CFG_OFFSET(ext_always_enabled), \
>> +     .enabled = true}
>> +
>>   /*
>>    * 'Named features' is the name we give to extensions that we
>>    * don't want to expose to users. They are either immutable
>> @@ -1523,6 +1537,23 @@ 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
>> +     * in TCG since QEMU RISC-V does not have a cache
>> +     * model.
>> +     */
>> +    ALWAYS_ENABLED_FEATURE("za64rs"),
>> +    ALWAYS_ENABLED_FEATURE("ziccif"),
>> +    ALWAYS_ENABLED_FEATURE("ziccrse"),
>> +    ALWAYS_ENABLED_FEATURE("ziccamoa"),
>> +    ALWAYS_ENABLED_FEATURE("zicclsm"),
>> +    ALWAYS_ENABLED_FEATURE("ssccptr"),
>> +
>> +    /* Other named features that TCG always implements */
>> +    ALWAYS_ENABLED_FEATURE("sstvecd"),
>> +    ALWAYS_ENABLED_FEATURE("sstvala"),
>> +    ALWAYS_ENABLED_FEATURE("sscounterenw"),
>> +
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> @@ -2116,13 +2147,10 @@ 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 'named features' that are cache
>> + * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
>> + * and Zicclsm. They are always implemented in TCG and
>> + * doesn't need to be manually enabled by the profile.
>>    */
>>   static RISCVCPUProfile RVA22U64 = {
>>       .parent = NULL,
>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>> index 698f926ab1..c5049ec664 100644
>> --- a/target/riscv/cpu_cfg.h
>> +++ b/target/riscv/cpu_cfg.h
>> @@ -126,6 +126,12 @@ struct RISCVCPUConfig {
>>       bool ext_svade;
>>       bool ext_zic64b;
>>   
>> +    /*
>> +     * Always 'true' boolean for named features
>> +     * TCG always implement/can't be disabled.
>> +     */
>> +    bool ext_always_enabled;
>> +
>>       /* 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 90861cc065..673097c6e4 100644
>> --- a/target/riscv/tcg/tcg-cpu.c
>> +++ b/target/riscv/tcg/tcg-cpu.c
>> @@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
>>       RISCVCPU *cpu = RISCV_CPU(cs);
>>       Object *obj = OBJECT(cpu);
>>   
>> +    cpu->cfg.ext_always_enabled = true;
>> +
>>       misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>>       multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>>       riscv_cpu_add_user_properties(obj);
>> -- 
>> 2.43.0
>>
>>

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

[Andrew Jones]

On Thu, Feb 15, 2024 at 11:13:51AM -0300, Daniel Henrique Barboza wrote:
...
> > I want to raise my frustration with the crock we've been given here by
> > RVI. Any "named feature" that just creates a name for something that
> > already is assumed is completely useless, and DT property that is used
> > to communicate it's presence cannot be used - instead the property needs
> > to be inverted - indicating the absence of that named feature.
> 
> Let's say that I'm not the biggest fan of how these profile extensions are being
> dealt with in the spec :) the text is vague w.r.t whether zicclsm and others
> are actual extensions, or a 'named feature'( like we're calling here in QEMU)
>

The text is vague, I certainly didn't get it at first, but it's been
clarified that these "named features" are considered extensions with
the given names and those extensions are ratified at the time the profile
in which they first appear is ratified. As I said in my other reply, I
hope the need to name old features is behind us now that the first
profiles are done.

> that is just a glorified way of saying, for example, "zic64b" instead of "all
> cache blocks have 64 bytes".

The note that accompanies "Zic64b" also states that the cache blocks may
be larger or smaller than 64 bytes. So, when a platform includes this
"Zic64b" extension in its DT it doesn't mean all blocks are 64 bytes, it
means they're all compatible with 64 bytes by either using 64-byte sub-
blocks (when they're bigger) or by sequencing cache ops across multiple
blocks (when they're smaller). So, while we can derive 'zic64b' from a
platform which does have all blocks of size 64, some platforms will need
to explicitly add it to the ISA string when they know they're compatible,
since they'll be putting other block sizes in the block size descriptions.

Thanks,
drew

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

[Andrew Jones]

On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote:
> On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
> > 
> > Instead of adding one bool for each named feature that we'll always
> > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> > named features will point to it. This also means that KVM won't see
> > these features as always enable, which is our intention.
> > 
> > If any accelerator adds support to disable one of these features, we'll
> > have to promote them to regular extensions and allow users to disable it
> > via command line.
> > 
> > After this patch, here's the riscv,isa from a buildroot using the
> > 'rva22s64' CPU:
> 
> Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
> present in "u" profiles?

"s" profiles mandate all the "u" profile mandatory extensions. For example
6.2.2 says

"""
The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged
extensions in RVA22U64.
"""

> 
> >  # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> 
> I want to raise my frustration with the crock we've been given here by
> RVI. Any "named feature" that just creates a name for something that
> already is assumed is completely useless, and DT property that is used
> to communicate it's presence cannot be used - instead the property needs
> to be inverted - indicating the absence of that named feature.
> 
> Without the inversion, software that parses "riscv,isa" cannot make any
> determination based on the absence of the property - it could be parsing
> an old DT that does not have the property or it could be parsing the DT
> of a system that does not support the extension.

I'm guessing any platform which wants to advertise that it's compliant
with a profile will update its hardware descriptions to ensure all the
profile's mandatory extensions are presented. But, I think I understand
your concern. If somebody is parsing the ISA string as way to determine
if the platform is compliant with a profile, then they may get a false
negative due to the ISA string missing a newly named feature. I'm not
sure how much of a problem that will be in practice, though, since testing
for profile compliance, just for the sake of it, doesn't seem very useful.
Software really only needs to know which extensions are available and if
it's an old feature that got newly named, then software likely already
has another way of detecting it.

> 
> This is part of why I deprecated `riscv,isa`. It's the same problem as
> with "zifencei" et al - does a system with `riscv,isa = "rv64imac"`
> support fence.i?

Yes, there's a handful of these messy things and the first profiles
expose them since they're trying to define them. Fingers crossed that
the next profiles won't have to name old features. FWIW, I at least
don't see any "This is a new extension name for this feature" notes in
the RVA23 profile.

Thanks,
drew

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4895 bytes --]

On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote:
> On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote:
> > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
> > > 
> > > Instead of adding one bool for each named feature that we'll always
> > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> > > named features will point to it. This also means that KVM won't see
> > > these features as always enable, which is our intention.
> > > 
> > > If any accelerator adds support to disable one of these features, we'll
> > > have to promote them to regular extensions and allow users to disable it
> > > via command line.
> > > 
> > > After this patch, here's the riscv,isa from a buildroot using the
> > > 'rva22s64' CPU:
> > 
> > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
> > present in "u" profiles?
> 
> "s" profiles mandate all the "u" profile mandatory extensions. For example
> 6.2.2 says
> 
> """
> The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged
> extensions in RVA22U64.
> """

Doesn't that rule out emulating misaligned access in s-mode if you want
to be profile compliant?

> > >  # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> > 
> > I want to raise my frustration with the crock we've been given here by
> > RVI. Any "named feature" that just creates a name for something that
> > already is assumed is completely useless, and DT property that is used
> > to communicate it's presence cannot be used - instead the property needs
> > to be inverted - indicating the absence of that named feature.
> > 
> > Without the inversion, software that parses "riscv,isa" cannot make any
> > determination based on the absence of the property - it could be parsing
> > an old DT that does not have the property or it could be parsing the DT
> > of a system that does not support the extension.
> 
> I'm guessing any platform which wants to advertise that it's compliant
> with a profile will update its hardware descriptions to ensure all the
> profile's mandatory extensions are presented. But, I think I understand
> your concern. If somebody is parsing the ISA string as way to determine
> if the platform is compliant with a profile, then they may get a false
> negative due to the ISA string missing a newly named feature.

Nah, you misunderstand me. I don't care at all about profiles or
checking for compliance with one. I'm only interested in how my software
can check that some feature is (or is not) supported. This creating a name
for something implicit business is not a problem in and of itself, but
putting then into "riscv,isa" is a pointless activity as it communicates
nothing.

> I'm not
> sure how much of a problem that will be in practice, though, since testing
> for profile compliance, just for the sake of it, doesn't seem very useful.
> Software really only needs to know which extensions are available and if
> it's an old feature that got newly named, then software likely already
> has another way of detecting it.

Right. That part is fine, but creating extensions for these things we
previously assumed present gives me the impression that creating systems
that do not support these features is valid. IFF that does happen,
removing the string from "riscv,isa" isn't going to be able to
communicate that the feature is unsupported. The commit message here
says:
> > > If any accelerator adds support to disable one of these features, we'll
> > > have to promote them to regular extensions and allow users to disable it
> > > via command line.

Which is part of what prompted me here, since they cannot be handled in
the same way that "regular extensions" are.

> > This is part of why I deprecated `riscv,isa`. It's the same problem as
> > with "zifencei" et al - does a system with `riscv,isa = "rv64imac"`
> > support fence.i?
> 
> Yes, there's a handful of these messy things and the first profiles
> expose them since they're trying to define them. Fingers crossed that
> the next profiles won't have to name old features. FWIW, I at least
> don't see any "This is a new extension name for this feature" notes in
> the RVA23 profile.
> 
> Thanks,
> drew

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Andrew Jones]

On Thu, Feb 15, 2024 at 04:34:32PM +0000, Conor Dooley wrote:
> On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote:
> > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote:
> > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
> > > > 
> > > > Instead of adding one bool for each named feature that we'll always
> > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> > > > named features will point to it. This also means that KVM won't see
> > > > these features as always enable, which is our intention.
> > > > 
> > > > If any accelerator adds support to disable one of these features, we'll
> > > > have to promote them to regular extensions and allow users to disable it
> > > > via command line.
> > > > 
> > > > After this patch, here's the riscv,isa from a buildroot using the
> > > > 'rva22s64' CPU:
> > > 
> > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
> > > present in "u" profiles?
> > 
> > "s" profiles mandate all the "u" profile mandatory extensions. For example
> > 6.2.2 says
> > 
> > """
> > The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged
> > extensions in RVA22U64.
> > """
> 
> Doesn't that rule out emulating misaligned access in s-mode if you want
> to be profile compliant?

That's how I interpret it, but I'll defer to a profile spec author, or
at least to somebody more confident of their spec interpretation skills.

> 
> > > >  # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> > > 
> > > I want to raise my frustration with the crock we've been given here by
> > > RVI. Any "named feature" that just creates a name for something that
> > > already is assumed is completely useless, and DT property that is used
> > > to communicate it's presence cannot be used - instead the property needs
> > > to be inverted - indicating the absence of that named feature.
> > > 
> > > Without the inversion, software that parses "riscv,isa" cannot make any
> > > determination based on the absence of the property - it could be parsing
> > > an old DT that does not have the property or it could be parsing the DT
> > > of a system that does not support the extension.
> > 
> > I'm guessing any platform which wants to advertise that it's compliant
> > with a profile will update its hardware descriptions to ensure all the
> > profile's mandatory extensions are presented. But, I think I understand
> > your concern. If somebody is parsing the ISA string as way to determine
> > if the platform is compliant with a profile, then they may get a false
> > negative due to the ISA string missing a newly named feature.
> 
> Nah, you misunderstand me. I don't care at all about profiles or
> checking for compliance with one. I'm only interested in how my software
> can check that some feature is (or is not) supported. This creating a name
> for something implicit business is not a problem in and of itself, but
> putting then into "riscv,isa" is a pointless activity as it communicates
> nothing.
> 
> > I'm not
> > sure how much of a problem that will be in practice, though, since testing
> > for profile compliance, just for the sake of it, doesn't seem very useful.
> > Software really only needs to know which extensions are available and if
> > it's an old feature that got newly named, then software likely already
> > has another way of detecting it.
> 
> Right. That part is fine, but creating extensions for these things we
> previously assumed present gives me the impression that creating systems
> that do not support these features is valid. IFF that does happen,
> removing the string from "riscv,isa" isn't going to be able to
> communicate that the feature is unsupported.

Ah, now I think I understand the concern. The new names might as well be
ignored because the absence of the names in the hardware descriptions is
ambiguous. I guess I'd encourage software that has a role in advertising
features to use the new names when it has detected the feature or assumes
the feature is present (and presumably wouldn't be running if its
assumption was wrong). If, for example, Linux puts a new name in
/proc/cpuinfo after detecting or assuming the feature's presence, then it
no longer matters that the hardware description had it or not from the
perspective of the /proc/cpuinfo consumer (assuming they're aware of which
kernel version they need). With these types of fixups and enough time,
then hopefully most of the software ecosystem will able to live in
ignorant bliss.

> The commit message here
> says:
> > > > If any accelerator adds support to disable one of these features, we'll
> > > > have to promote them to regular extensions and allow users to disable it
> > > > via command line.
> 
> Which is part of what prompted me here, since they cannot be handled in
> the same way that "regular extensions" are.
>

From QEMU's perspective, they can. Linux or whatever software consuming
the hardware descriptions may want to distrust the absence of newly
named feature extensions and do their own checks, but that's not QEMU's
concern. Actually, being able to disable these newly named features allows
Linux and other software to test how they behave when the feature goes
away. Do they crash fast and clean or do they go off in the weeds? Does
their ad hoc detection actually work?

Thanks,
drew

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 7381 bytes --]

On Thu, Feb 15, 2024 at 08:11:45PM +0100, Andrew Jones wrote:
> On Thu, Feb 15, 2024 at 04:34:32PM +0000, Conor Dooley wrote:
> > On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote:
> > > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote:
> > > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
> > > > > 
> > > > > Instead of adding one bool for each named feature that we'll always
> > > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> > > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> > > > > named features will point to it. This also means that KVM won't see
> > > > > these features as always enable, which is our intention.
> > > > > 
> > > > > If any accelerator adds support to disable one of these features, we'll
> > > > > have to promote them to regular extensions and allow users to disable it
> > > > > via command line.
> > > > > 
> > > > > After this patch, here's the riscv,isa from a buildroot using the
> > > > > 'rva22s64' CPU:
> > > > 
> > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
> > > > present in "u" profiles?
> > > 
> > > "s" profiles mandate all the "u" profile mandatory extensions. For example
> > > 6.2.2 says
> > > 
> > > """
> > > The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged
> > > extensions in RVA22U64.
> > > """
> > 
> > Doesn't that rule out emulating misaligned access in s-mode if you want
> > to be profile compliant?
> 
> That's how I interpret it, but I'll defer to a profile spec author, or
> at least to somebody more confident of their spec interpretation skills.

Hmm, actually it doesn't. Your firmware just needs to _also_ implement
it. So your OS kernel could test whether or not the misaligned access
performance is beans and then choose to emulate misaligned access
itself. Not ideal, but better than I thought.

> > > > >  # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> > > > 
> > > > I want to raise my frustration with the crock we've been given here by
> > > > RVI. Any "named feature" that just creates a name for something that
> > > > already is assumed is completely useless, and DT property that is used
> > > > to communicate it's presence cannot be used - instead the property needs
> > > > to be inverted - indicating the absence of that named feature.
> > > > 
> > > > Without the inversion, software that parses "riscv,isa" cannot make any
> > > > determination based on the absence of the property - it could be parsing
> > > > an old DT that does not have the property or it could be parsing the DT
> > > > of a system that does not support the extension.
> > > 
> > > I'm guessing any platform which wants to advertise that it's compliant
> > > with a profile will update its hardware descriptions to ensure all the
> > > profile's mandatory extensions are presented. But, I think I understand
> > > your concern. If somebody is parsing the ISA string as way to determine
> > > if the platform is compliant with a profile, then they may get a false
> > > negative due to the ISA string missing a newly named feature.
> > 
> > Nah, you misunderstand me. I don't care at all about profiles or
> > checking for compliance with one. I'm only interested in how my software
> > can check that some feature is (or is not) supported. This creating a name
> > for something implicit business is not a problem in and of itself, but
> > putting then into "riscv,isa" is a pointless activity as it communicates
> > nothing.
> > 
> > > I'm not
> > > sure how much of a problem that will be in practice, though, since testing
> > > for profile compliance, just for the sake of it, doesn't seem very useful.
> > > Software really only needs to know which extensions are available and if
> > > it's an old feature that got newly named, then software likely already
> > > has another way of detecting it.
> > 
> > Right. That part is fine, but creating extensions for these things we
> > previously assumed present gives me the impression that creating systems
> > that do not support these features is valid. IFF that does happen,
> > removing the string from "riscv,isa" isn't going to be able to
> > communicate that the feature is unsupported.
> 
> Ah, now I think I understand the concern. The new names might as well be
> ignored because the absence of the names in the hardware descriptions is
> ambiguous.

Correct.

> I guess I'd encourage software that has a role in advertising
> features to use the new names when it has detected the feature or assumes
> the feature is present (and presumably wouldn't be running if its
> assumption was wrong). If, for example, Linux puts a new name in
> /proc/cpuinfo after detecting or assuming the feature's presence, then it
> no longer matters that the hardware description had it or not from the
> perspective of the /proc/cpuinfo consumer (assuming they're aware of which
> kernel version they need). With these types of fixups and enough time,
> then hopefully most of the software ecosystem will able to live in
> ignorant bliss.

Yeah, that's effectively what we have to do. I started doing that for
zifencei/zicsr in Linux and it should be done for anything else like
this going forwards.

> > The commit message here
> > says:
> > > > > If any accelerator adds support to disable one of these features, we'll
> > > > > have to promote them to regular extensions and allow users to disable it
> > > > > via command line.
> > 
> > Which is part of what prompted me here, since they cannot be handled in
> > the same way that "regular extensions" are.
> >
> 
> From QEMU's perspective, they can.

No they can't. For a "regular extension" you populate the DT with the
extension. For these extensions it has to put negated properties in the
DT, otherwise it is incorrectly describing the hardware it is emulating.
That is handling them differently in my book! If QEMU generates an
incorrect DT representation of the hardware it is emulating, that's a
QEMU bug.

> Linux or whatever software consuming
> the hardware descriptions may want to distrust the absence of newly
> named feature extensions and do their own checks, but that's not QEMU's
> concern.

Software should be able to trust that the DT describes the system
correctly. I can only speak for Linux here, but validating the DT is not
the job of the running kernel - it should be a correct description.

> Actually, being able to disable these newly named features allows
> Linux and other software to test how they behave when the feature goes
> away.

That's helpful sure, but it doesn't absolve QEMU of having to correctly
generate a DT.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Alistair Francis]

On Fri, Feb 16, 2024 at 6:00 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Feb 15, 2024 at 08:11:45PM +0100, Andrew Jones wrote:
> > On Thu, Feb 15, 2024 at 04:34:32PM +0000, Conor Dooley wrote:
> > > On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote:
> > > > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote:
> > > > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].
> > > > > >
> > > > > > Instead of adding one bool for each named feature that we'll always
> > > > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> > > > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> > > > > > named features will point to it. This also means that KVM won't see
> > > > > > these features as always enable, which is our intention.
> > > > > >
> > > > > > If any accelerator adds support to disable one of these features, we'll
> > > > > > have to promote them to regular extensions and allow users to disable it
> > > > > > via command line.
> > > > > >
> > > > > > After this patch, here's the riscv,isa from a buildroot using the
> > > > > > 'rva22s64' CPU:
> > > > >
> > > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
> > > > > present in "u" profiles?
> > > >
> > > > "s" profiles mandate all the "u" profile mandatory extensions. For example
> > > > 6.2.2 says
> > > >
> > > > """
> > > > The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged
> > > > extensions in RVA22U64.
> > > > """
> > >
> > > Doesn't that rule out emulating misaligned access in s-mode if you want
> > > to be profile compliant?
> >
> > That's how I interpret it, but I'll defer to a profile spec author, or
> > at least to somebody more confident of their spec interpretation skills.
>
> Hmm, actually it doesn't. Your firmware just needs to _also_ implement
> it. So your OS kernel could test whether or not the misaligned access
> performance is beans and then choose to emulate misaligned access
> itself. Not ideal, but better than I thought.
>
> > > > > >  # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> > > > >
> > > > > I want to raise my frustration with the crock we've been given here by
> > > > > RVI. Any "named feature" that just creates a name for something that
> > > > > already is assumed is completely useless, and DT property that is used
> > > > > to communicate it's presence cannot be used - instead the property needs
> > > > > to be inverted - indicating the absence of that named feature.
> > > > >
> > > > > Without the inversion, software that parses "riscv,isa" cannot make any
> > > > > determination based on the absence of the property - it could be parsing
> > > > > an old DT that does not have the property or it could be parsing the DT
> > > > > of a system that does not support the extension.
> > > >
> > > > I'm guessing any platform which wants to advertise that it's compliant
> > > > with a profile will update its hardware descriptions to ensure all the
> > > > profile's mandatory extensions are presented. But, I think I understand
> > > > your concern. If somebody is parsing the ISA string as way to determine
> > > > if the platform is compliant with a profile, then they may get a false
> > > > negative due to the ISA string missing a newly named feature.
> > >
> > > Nah, you misunderstand me. I don't care at all about profiles or
> > > checking for compliance with one. I'm only interested in how my software
> > > can check that some feature is (or is not) supported. This creating a name
> > > for something implicit business is not a problem in and of itself, but
> > > putting then into "riscv,isa" is a pointless activity as it communicates
> > > nothing.
> > >
> > > > I'm not
> > > > sure how much of a problem that will be in practice, though, since testing
> > > > for profile compliance, just for the sake of it, doesn't seem very useful.
> > > > Software really only needs to know which extensions are available and if
> > > > it's an old feature that got newly named, then software likely already
> > > > has another way of detecting it.
> > >
> > > Right. That part is fine, but creating extensions for these things we
> > > previously assumed present gives me the impression that creating systems
> > > that do not support these features is valid. IFF that does happen,
> > > removing the string from "riscv,isa" isn't going to be able to
> > > communicate that the feature is unsupported.
> >
> > Ah, now I think I understand the concern. The new names might as well be
> > ignored because the absence of the names in the hardware descriptions is
> > ambiguous.
>
> Correct.
>
> > I guess I'd encourage software that has a role in advertising
> > features to use the new names when it has detected the feature or assumes
> > the feature is present (and presumably wouldn't be running if its
> > assumption was wrong). If, for example, Linux puts a new name in
> > /proc/cpuinfo after detecting or assuming the feature's presence, then it
> > no longer matters that the hardware description had it or not from the
> > perspective of the /proc/cpuinfo consumer (assuming they're aware of which
> > kernel version they need). With these types of fixups and enough time,
> > then hopefully most of the software ecosystem will able to live in
> > ignorant bliss.
>
> Yeah, that's effectively what we have to do. I started doing that for
> zifencei/zicsr in Linux and it should be done for anything else like
> this going forwards.
>
> > > The commit message here
> > > says:
> > > > > > If any accelerator adds support to disable one of these features, we'll
> > > > > > have to promote them to regular extensions and allow users to disable it
> > > > > > via command line.
> > >
> > > Which is part of what prompted me here, since they cannot be handled in
> > > the same way that "regular extensions" are.
> > >
> >
> > From QEMU's perspective, they can.
>
> No they can't. For a "regular extension" you populate the DT with the
> extension. For these extensions it has to put negated properties in the
> DT, otherwise it is incorrectly describing the hardware it is emulating.
> That is handling them differently in my book! If QEMU generates an
> incorrect DT representation of the hardware it is emulating, that's a
> QEMU bug.

QEMU listing the extensions that it supports seems to me to be the
correct approach.

It's clunky that the list of "extensions" are constantly changing.
There isn't much we can do about that from a QEMU perspective though.

Listing the hardware and what it supports is the job of the DT.

I see your concern about what happens if the "extensions" are disabled
though. Realislity they probably never will be.

>
> > Linux or whatever software consuming
> > the hardware descriptions may want to distrust the absence of newly
> > named feature extensions and do their own checks, but that's not QEMU's
> > concern.
>
> Software should be able to trust that the DT describes the system
> correctly. I can only speak for Linux here, but validating the DT is not
> the job of the running kernel - it should be a correct description.

AFAIK the DT is correct. We are describing the hardware within the
scope of the DT spec.

If a new node exists that describes what the hardware does not support
we can update to support that as well.

>
> > Actually, being able to disable these newly named features allows
> > Linux and other software to test how they behave when the feature goes
> > away.
>
> That's helpful sure, but it doesn't absolve QEMU of having to correctly
> generate a DT.

I'm pretty sure there isn't anything for us to do differently here
right? It's just a bad situation that we are trying to support.

Alistair

>
> Thanks,
> Conor.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3132 bytes --]

> > No they can't. For a "regular extension" you populate the DT with the
> > extension. For these extensions it has to put negated properties in the
> > DT, otherwise it is incorrectly describing the hardware it is emulating.
> > That is handling them differently in my book! If QEMU generates an
> > incorrect DT representation of the hardware it is emulating, that's a
> > QEMU bug.
> 
> QEMU listing the extensions that it supports seems to me to be the
> correct approach.
> 
> It's clunky that the list of "extensions" are constantly changing.
> There isn't much we can do about that from a QEMU perspective though.
> 
> Listing the hardware and what it supports is the job of the DT.
> 
> I see your concern about what happens if the "extensions" are disabled
> though. Realislity they probably never will be.

Yeah, it's something we can sweep under the rug unless/until someone
wants to disable these things.

> > > Linux or whatever software consuming
> > > the hardware descriptions may want to distrust the absence of newly
> > > named feature extensions and do their own checks, but that's not QEMU's
> > > concern.
> >
> > Software should be able to trust that the DT describes the system
> > correctly. I can only speak for Linux here, but validating the DT is not
> > the job of the running kernel - it should be a correct description.
> 
> AFAIK the DT is correct. We are describing the hardware within the
> scope of the DT spec.
> 
> If a new node exists that describes what the hardware does not support
> we can update to support that as well.

It won't be a new node property, it'll just be negated properties - eg
riscv,isa-extensions = ..., "no-zicclsm";
That's what I mean when I say that these will not be able to be treated
in the same way as any other extension, but it only applies iff someone
wants to disable them. This isn't just a QEMU problem, but QEMU is the
bleeding edge of "hardware" support, so it's cropping up here first (or
maybe only :))

> > > Actually, being able to disable these newly named features allows
> > > Linux and other software to test how they behave when the feature goes
> > > away.
> >
> > That's helpful sure, but it doesn't absolve QEMU of having to correctly
> > generate a DT.
> 
> I'm pretty sure there isn't anything for us to do differently here
> right? It's just a bad situation that we are trying to support.

Until someone wants to turn them off, you can avoid doing anything
differently, just like this amazing ascii art I found:

                _,-\/-,_
                \      /
                 \_.._/
               _,/    \,_
              / \      / \
             ,\  )    (  /,
             (__/ .''. \__)
                \,_||   /
                |  ||\ |
                | /|| \|
                () || ()
                // || ||
               //  || ||
              //   || ||
             //    || /\
 -- '' -'-' ^^'    )( '^^-- '' -'-'   miK
                  (==)
                   `~`

Hopefully posting ostriches on the QEMU list isn't grounds for a ban,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 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")
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Signed-off-by: Andrew Jones <ajones@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 94843c4f6e..9045f87481 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

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

[Alistair Francis]

On Sat, Feb 3, 2024 at 1:23 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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")
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

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

Alistair

> ---
>  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 94843c4f6e..9045f87481 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 v3 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]
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c         |  3 ++-
 target/riscv/cpu_helper.c  | 19 +++++++++++++++----
 target/riscv/tcg/tcg-cpu.c | 15 +++++----------
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9045f87481..50ac7845a8 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -960,7 +960,8 @@ 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..3a440833f8 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,18 @@ 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 673097c6e4..43c32b4a15 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -196,17 +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:
-        g_assert_not_reached();
     }
 }
 
@@ -348,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

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

[Alistair Francis]

On Sat, Feb 3, 2024 at 1:22 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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]
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         |  3 ++-
>  target/riscv/cpu_helper.c  | 19 +++++++++++++++----
>  target/riscv/tcg/tcg-cpu.c | 15 +++++----------
>  3 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9045f87481..50ac7845a8 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -960,7 +960,8 @@ 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..3a440833f8 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,18 @@ 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 673097c6e4..43c32b4a15 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -196,17 +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:
> -        g_assert_not_reached();
>      }
>  }
>
> @@ -348,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 v3 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, even for the max
cpu type, 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).

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c         | 9 ++-------
 target/riscv/tcg/tcg-cpu.c | 6 ++++++
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 50ac7845a8..f036b153a1 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1422,6 +1422,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),
@@ -1534,7 +1535,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),
 
     /*
@@ -2182,8 +2182,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 = {
@@ -2195,10 +2193,7 @@ static RISCVCPUProfile RVA22S64 = {
     .ext_offsets = {
         /* rva22s64 exts */
         CPU_CFG_OFFSET(ext_zifencei), CPU_CFG_OFFSET(ext_svpbmt),
-        CPU_CFG_OFFSET(ext_svinval),
-
-        /* rva22s64 named features */
-        CPU_CFG_OFFSET(ext_svade),
+        CPU_CFG_OFFSET(ext_svinval), CPU_CFG_OFFSET(ext_svade),
 
         RISCV_PROFILE_EXT_LIST_END
     }
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 43c32b4a15..9fc64979f1 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1314,6 +1314,12 @@ static void riscv_init_max_cpu_extensions(Object *obj)
         isa_ext_update_enabled(cpu, prop->offset, true);
     }
 
+    /*
+     * Some extensions can't be added without backward compatibilty concerns.
+     * Disable those, the user can still opt in to them on the command line.
+     */
+    cpu->cfg.ext_svade = false;
+
     /* set vector version */
     env->vext_ver = VEXT_VERSION_1_00_0;
 
-- 
2.43.0

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

[Alistair Francis]

On Sat, Feb 3, 2024 at 1:24 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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, even for the max
> cpu type, 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).
>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 9 ++-------
>  target/riscv/tcg/tcg-cpu.c | 6 ++++++
>  2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 50ac7845a8..f036b153a1 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1422,6 +1422,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),
> @@ -1534,7 +1535,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),
>
>      /*
> @@ -2182,8 +2182,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 = {
> @@ -2195,10 +2193,7 @@ static RISCVCPUProfile RVA22S64 = {
>      .ext_offsets = {
>          /* rva22s64 exts */
>          CPU_CFG_OFFSET(ext_zifencei), CPU_CFG_OFFSET(ext_svpbmt),
> -        CPU_CFG_OFFSET(ext_svinval),
> -
> -        /* rva22s64 named features */
> -        CPU_CFG_OFFSET(ext_svade),
> +        CPU_CFG_OFFSET(ext_svinval), CPU_CFG_OFFSET(ext_svade),
>
>          RISCV_PROFILE_EXT_LIST_END
>      }
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 43c32b4a15..9fc64979f1 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1314,6 +1314,12 @@ static void riscv_init_max_cpu_extensions(Object *obj)
>          isa_ext_update_enabled(cpu, prop->offset, true);
>      }
>
> +    /*
> +     * Some extensions can't be added without backward compatibilty concerns.
> +     * Disable those, the user can still opt in to them on the command line.
> +     */
> +    cpu->cfg.ext_svade = false;
> +
>      /* set vector version */
>      env->vext_ver = VEXT_VERSION_1_00_0;
>
> --
> 2.43.0
>
>

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

[Alistair Francis]

On Sat, Feb 3, 2024 at 1:23 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> In this new version we changed patch 3 as suggested by Alistair in v1
> [1]. Instead of creating individual always-true bool for each named
> feature, create a bool flag will be always 'true' to be used as config
> offset for these named extensions.
>
> Patches based on riscv-to-apply.next.
>
> Patches missing acks: patch 3.
>
> Changes from v2:
> - patch 3:
>   - 'ext_always_enabled' bool added
>   - individual always-enabled named features bools removed
> - v2 link: https://lore.kernel.org/qemu-riscv/20240126133101.61344-8-ajones@ventanamicro.com/
>
>
> [1] https://lore.kernel.org/qemu-riscv/20240125195319.329181-1-dbarboza@ventanamicro.com/
>
> 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

Do you mind rebasing? I feel bad always asking, but I think it's your
patches that cause the conflicts :P

Alistair

>
>  target/riscv/cpu.c         | 70 +++++++++++++++++++++++++++-----------
>  target/riscv/cpu_cfg.h     | 12 +++++--
>  target/riscv/cpu_helper.c  | 19 ++++++++---
>  target/riscv/csr.c         |  2 +-
>  target/riscv/tcg/tcg-cpu.c | 34 +++++++++---------
>  5 files changed, 94 insertions(+), 43 deletions(-)
>
> --
> 2.43.0
>
>

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

[Daniel Henrique Barboza]

On 2/15/24 06:52, Alistair Francis wrote:
> On Sat, Feb 3, 2024 at 1:23 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> Hi,
>>
>> In this new version we changed patch 3 as suggested by Alistair in v1
>> [1]. Instead of creating individual always-true bool for each named
>> feature, create a bool flag will be always 'true' to be used as config
>> offset for these named extensions.
>>
>> Patches based on riscv-to-apply.next.
>>
>> Patches missing acks: patch 3.
>>
>> Changes from v2:
>> - patch 3:
>>    - 'ext_always_enabled' bool added
>>    - individual always-enabled named features bools removed
>> - v2 link: https://lore.kernel.org/qemu-riscv/20240126133101.61344-8-ajones@ventanamicro.com/
>>
>>
>> [1] https://lore.kernel.org/qemu-riscv/20240125195319.329181-1-dbarboza@ventanamicro.com/
>>
>> 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
> 
> Do you mind rebasing? I feel bad always asking, but I think it's your
> patches that cause the conflicts :P

:)

I'll re-send based on current riscv-to-apply.next. Thanks,


Daniel

> 
> Alistair
> 
>>
>>   target/riscv/cpu.c         | 70 +++++++++++++++++++++++++++-----------
>>   target/riscv/cpu_cfg.h     | 12 +++++--
>>   target/riscv/cpu_helper.c  | 19 ++++++++---
>>   target/riscv/csr.c         |  2 +-
>>   target/riscv/tcg/tcg-cpu.c | 34 +++++++++---------
>>   5 files changed, 94 insertions(+), 43 deletions(-)
>>
>> --
>> 2.43.0
>>
>>