[PATCH v6 00/12] RVA22U64 profile support

[PATCH v6 00/12] RVA22U64 profile support

[Daniel Henrique Barboza]

Based-on: 20231023153927.435083-1-dbarboza@ventanamicro.com
("[PATCH v3 0/4] riscv: zicntr/zihpm flags and disable support")

Hi,

This version has several changes proposed by Drew in v5 and a design
change after a discussion we have in the v3 review [1].

Notable changes:

- zicbop extension flag is added (patch 1). Given that this is an actual
  extension, not a 'named feature', and we already implement is as a
  no-op, we're adding the flag to make our riscv,isa compliant with what
  the profile mandates;

- zic64b is no longer an user flag. zic64b is a named extension, a
  glorified way of telling that we're using 64 byte cache blocks. Users
  can indirectly set it to true/false by simply editing
  cbo{m,p,z}_blocksize;

- zic64b, an all future named extensions, are now exposed in
  query-cpu-model-expansion (patch 3);

- marking a profile as 'false' in the command line no longer disables
  its mandatory extensions in the CPU. Since profile flags are disabled
  by default in all current CPUs, setting 'rva22u64=false' will cause no
  change of state in the CPU, i.e. it'll do nothing. This is the same
  mechanic that RVG already uses and we'll make profiles behave the same
  way. We'll not "be creative" and interpret a 'false' from the command
  line mean something different than the existing 'false'. If we want a
  way to mass disable CPU extensions we can implement cleaner ways of
  doing it. See [1] for even more context/rant about the previous design
  and why we're changing it.

Patches based on top of:

[PATCH v3 0/4] riscv: zicntr/zihpm flags and disable support

Patches missing acks: patches 1,2,3

Changes from v5:
- patch 1 (new):
  - add zicbop extension flag
- patch 2 (patch 1 from v5):
  - zic64b no longer exposed as user flag, no longer throwing user warnings
- patch 3 (new):
  - change query-cpu-model-expansion to include named extensions like zic64b
- patch 4 (patch 2 from v5):
  - remove extra whitespace in profile description comment
  - added "(nor for zic64b, despite it having a cfg offset)" in the profile description comment
  - moved 'NULL' to its own line  in riscv_profiles[]
  - added 'zicbop' as a mandatory profile extension
- patch 6 (patch 4 from v5):
  - add 64 bit CPU restriction for profiles
  - marking a profile as 'false' no longer removes all the mandatory extensions of the CPU model,
    making it compatible with what RVG does:
      profileA=true means "enable all mandatory extensions of profileA"
      profileA=false means "do not enable all mandatory extensions of profileA"
- v5 link: https://lore.kernel.org/qemu-riscv/20231025234459.581697-1-dbarboza@ventanamicro.com/

[1] https://lore.kernel.org/qemu-riscv/e3f53179-7f7e-42a9-8a13-a81bf1beeb89@ventanamicro.com/

Daniel Henrique Barboza (12):
  target/riscv: add zicbop extension flag
  target/riscv/tcg: add 'zic64b' support
  riscv-qmp-cmds.c: expose named features in cpu_model_expansion
  target/riscv: add rva22u64 profile definition
  target/riscv/kvm: add 'rva22u64' flag as unavailable
  target/riscv/tcg: add user flag for profile support
  target/riscv/tcg: add MISA user options hash
  target/riscv/tcg: add riscv_cpu_write_misa_bit()
  target/riscv/tcg: handle profile MISA bits
  target/riscv/tcg: add hash table insert helpers
  target/riscv/tcg: honor user choice for G MISA bits
  target/riscv/tcg: warn if profile exts are disabled

 hw/riscv/virt.c               |   5 +
 target/riscv/cpu.c            |  48 +++++-
 target/riscv/cpu.h            |  15 ++
 target/riscv/cpu_cfg.h        |   3 +
 target/riscv/kvm/kvm-cpu.c    |   7 +-
 target/riscv/riscv-qmp-cmds.c |  30 +++-
 target/riscv/tcg/tcg-cpu.c    | 277 ++++++++++++++++++++++++++++------
 7 files changed, 335 insertions(+), 50 deletions(-)

-- 
2.41.0

[PATCH v6 01/12] target/riscv: add zicbop extension flag

[Daniel Henrique Barboza]

QEMU already implements zicbom (Cache Block Management Operations) and
zicboz (Cache Block Zero Operations). Commit 59cb29d6a5 ("target/riscv:
add Zicbop cbo.prefetch{i, r, m} placeholder") added placeholders for
what would be the instructions for zicbop (Cache Block Prefetch
Operations), which are now no-ops.

The RVA22U64 profile mandates zicbop, which means that applications that
run with this profile might expect zicbop to be present in the riscv,isa
DT and might behave badly if it's absent.

Adding zicbop as an extension will make our future RVA22U64
implementation more in line with what userspace expects and, if/when
cache block prefetch operations became relevant to QEMU, we already have
the extension flag to turn then on/off as needed.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c        | 5 +++++
 target/riscv/cpu.c     | 3 +++
 target/riscv/cpu_cfg.h | 2 ++
 3 files changed, 10 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 1732c42915..99c087240f 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -273,6 +273,11 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
                                   cpu_ptr->cfg.cboz_blocksize);
         }
 
+        if (cpu_ptr->cfg.ext_zicbop) {
+            qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbop-block-size",
+                                  cpu_ptr->cfg.cbop_blocksize);
+        }
+
         qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv");
         qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", "okay");
         qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg",
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f40da4c661..6c0050988f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -78,6 +78,7 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
  */
 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(zicond, PRIV_VERSION_1_12_0, ext_zicond),
     ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
@@ -1336,6 +1337,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("zhinxmin", ext_zhinxmin, false),
 
     MULTI_EXT_CFG_BOOL("zicbom", ext_zicbom, true),
+    MULTI_EXT_CFG_BOOL("zicbop", ext_zicbop, true),
     MULTI_EXT_CFG_BOOL("zicboz", ext_zicboz, true),
 
     MULTI_EXT_CFG_BOOL("zmmul", ext_zmmul, false),
@@ -1424,6 +1426,7 @@ Property riscv_cpu_options[] = {
     DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
 
     DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
+    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
     DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
 
     DEFINE_PROP_END_OF_LIST(),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 6eef4a51ea..2203b4c45b 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -65,6 +65,7 @@ struct RISCVCPUConfig {
     bool ext_zicntr;
     bool ext_zicsr;
     bool ext_zicbom;
+    bool ext_zicbop;
     bool ext_zicboz;
     bool ext_zicond;
     bool ext_zihintntl;
@@ -134,6 +135,7 @@ struct RISCVCPUConfig {
     uint16_t vlen;
     uint16_t elen;
     uint16_t cbom_blocksize;
+    uint16_t cbop_blocksize;
     uint16_t cboz_blocksize;
     bool mmu;
     bool pmp;
-- 
2.41.0

[PATCH v6 02/12] target/riscv/tcg: add 'zic64b' support

[Daniel Henrique Barboza]

zic64b is defined in the RVA22U64 profile [1] as a named feature for
"Cache blocks must be 64 bytes in size, naturally aligned in the address
space". It's a fantasy name for 64 bytes cache blocks. The RVA22U64
profile mandates this feature, meaning that applications using this
profile expects 64 bytes cache blocks.

To make the upcoming RVA22U64 implementation complete, we'll zic64b as
a 'named feature', not a regular extension. This means that:

- it won't be exposed to users;
- it won't be written in riscv,isa.

This will be extended to other named extensions in the future, so we're
creating some common boilerplate for them as well.

zic64b is default to 'true' since we're already using 64 bytes blocks.
If any cache block size (cbo{m,p,z}_blocksize) is changed to something
different than 64, zic64b is set to 'false'.

Our profile implementation will then be able to check the current state
of zic64b and take the appropriate action (e.g. throw a warning).

[1] https://github.com/riscv/riscv-profiles/releases/download/v1.0/profiles.pdf

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 15 ++++++++++++---
 target/riscv/cpu.h         |  3 +++
 target/riscv/cpu_cfg.h     |  1 +
 target/riscv/tcg/tcg-cpu.c | 14 ++++++++++++++
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 6c0050988f..316d468a19 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1396,6 +1396,12 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
+    MULTI_EXT_CFG_BOOL("zic64b", zic64b, true),
+
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 /* Deprecated entries marked for future removal */
 const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[] = {
     MULTI_EXT_CFG_BOOL("Zifencei", ext_zifencei, true),
@@ -1425,9 +1431,12 @@ Property riscv_cpu_options[] = {
     DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
     DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
 
-    DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
-    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
-    DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
+    DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU,
+                       cfg.cbom_blocksize, CB_DEF_VALUE),
+    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU,
+                       cfg.cbop_blocksize, CB_DEF_VALUE),
+    DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU,
+                       cfg.cboz_blocksize, CB_DEF_VALUE),
 
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 8efc4d83ec..ee9abe61d6 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -64,6 +64,8 @@ extern const uint32_t misa_bits[];
 const char *riscv_get_misa_ext_name(uint32_t bit);
 const char *riscv_get_misa_ext_description(uint32_t bit);
 
+#define CB_DEF_VALUE 64
+
 #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
 
 /* Privileged specification version */
@@ -745,6 +747,7 @@ typedef struct RISCVCPUMultiExtConfig {
 extern const RISCVCPUMultiExtConfig riscv_cpu_extensions[];
 extern const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[];
 extern const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[];
+extern const RISCVCPUMultiExtConfig riscv_cpu_named_features[];
 extern const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[];
 extern Property riscv_cpu_options[];
 
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 2203b4c45b..f61a8434c4 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -108,6 +108,7 @@ struct RISCVCPUConfig {
     bool ext_smepmp;
     bool rvv_ta_all_1s;
     bool rvv_ma_all_1s;
+    bool zic64b;
 
     uint32_t mvendorid;
     uint64_t marchid;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 093bda2e75..65d59bc984 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -264,6 +264,18 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
     }
 }
 
+static void riscv_cpu_validate_zic64b(RISCVCPU *cpu)
+{
+    cpu->cfg.zic64b = cpu->cfg.cbom_blocksize == CB_DEF_VALUE &&
+                      cpu->cfg.cbop_blocksize == CB_DEF_VALUE &&
+                      cpu->cfg.cboz_blocksize == CB_DEF_VALUE;
+}
+
+static void riscv_cpu_validate_named_features(RISCVCPU *cpu)
+{
+    riscv_cpu_validate_zic64b(cpu);
+}
+
 /*
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
@@ -586,6 +598,8 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
         return;
     }
 
+    riscv_cpu_validate_named_features(cpu);
+
     if (cpu->cfg.ext_smepmp && !cpu->cfg.pmp) {
         /*
          * Enhanced PMP should only be available
-- 
2.41.0

[PATCH v6 03/12] riscv-qmp-cmds.c: expose named features in cpu_model_expansion

[Daniel Henrique Barboza]

Named features (zic64b the sole example at this moment) aren't expose to
users, thus we need another way to expose them.

Go through each named feature, get its boolean value, do the needed
conversions (bool to qbool, qbool to QObject) and add it to output dict.

Another adjustment is needed: named features are evaluated during
finalize(), so riscv_cpu_finalize_features() needs to be mandatory
regardless of whether we have an input dict or not. Otherwise zic64b
will always return 'false', which is incorrect: the default values of
cache blocksizes (cbom_blocksize and cboz_blocksize) are set to 64,
satisfying the conditions for zic64b.

Here's an API usage example after this patch:

 $ ./build/qemu-system-riscv64 -S -M virt -display none
    -qmp tcp:localhost:1234,server,wait=off

 $ ./scripts/qmp/qmp-shell localhost:1234
Welcome to the QMP low-level shell!
Connected to QEMU 8.1.50

(QEMU) query-cpu-model-expansion type=full model={"name":"rv64"}
{"return": {"model":
    {"name": "rv64", "props": {... "zic64b": true, ...}}}}

zic64b is set to 'true', as expected, since all cache sizes are 64
bytes by default.

If we change one of the cache blocksizes, zic64b is returned as 'false':

(QEMU) query-cpu-model-expansion type=full model={"name":"rv64","props":{"cbom_blocksize":128}}
{"return": {"model":
    {"name": "rv64", "props": {... "zic64b": false, ...}}}}

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/riscv-qmp-cmds.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
index 2f2dbae7c8..5ada279776 100644
--- a/target/riscv/riscv-qmp-cmds.c
+++ b/target/riscv/riscv-qmp-cmds.c
@@ -26,6 +26,7 @@
 
 #include "qapi/error.h"
 #include "qapi/qapi-commands-machine-target.h"
+#include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qobject-input-visitor.h"
@@ -99,6 +100,22 @@ static void riscv_obj_add_multiext_props(Object *obj, QDict *qdict_out,
     }
 }
 
+static void riscv_obj_add_named_feats_qdict(Object *obj, QDict *qdict_out)
+{
+    const RISCVCPUMultiExtConfig *named_cfg;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    QObject *value;
+    bool flag_val;
+
+    for (int i = 0; riscv_cpu_named_features[i].name != NULL; i++) {
+        named_cfg = &riscv_cpu_named_features[i];
+        flag_val = isa_ext_is_enabled(cpu, named_cfg->offset);
+        value = QOBJECT(qbool_from_bool(flag_val));
+
+        qdict_put_obj(qdict_out, named_cfg->name, value);
+    }
+}
+
 static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
                                            const QDict *qdict_in,
                                            Error **errp)
@@ -129,11 +146,6 @@ static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
         goto err;
     }
 
-    riscv_cpu_finalize_features(RISCV_CPU(obj), &local_err);
-    if (local_err) {
-        goto err;
-    }
-
     visit_end_struct(visitor, NULL);
 
 err:
@@ -191,6 +203,13 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
         }
     }
 
+    riscv_cpu_finalize_features(RISCV_CPU(obj), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        object_unref(obj);
+        return NULL;
+    }
+
     expansion_info = g_new0(CpuModelExpansionInfo, 1);
     expansion_info->model = g_malloc0(sizeof(*expansion_info->model));
     expansion_info->model->name = g_strdup(model->name);
@@ -200,6 +219,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
     riscv_obj_add_multiext_props(obj, qdict_out, riscv_cpu_extensions);
     riscv_obj_add_multiext_props(obj, qdict_out, riscv_cpu_experimental_exts);
     riscv_obj_add_multiext_props(obj, qdict_out, riscv_cpu_vendor_exts);
+    riscv_obj_add_named_feats_qdict(obj, qdict_out);
 
     /* Add our CPU boolean options too */
     riscv_obj_add_qdict_prop(obj, qdict_out, "mmu");
-- 
2.41.0

[PATCH v6 04/12] target/riscv: add rva22u64 profile definition

[Daniel Henrique Barboza]

The rva22U64 profile, described in:

https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc#rva22-profiles

Contains a set of CPU extensions aimed for 64-bit userspace
applications. Enabling this set to be enabled via a single user flag
makes it convenient to enable a predictable set of features for the CPU,
giving users more predicability when running/testing their workloads.

QEMU implements all possible extensions of this profile. All the so called
'synthetic extensions' described in the profile that are cache related are
ignored/assumed enabled (Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa, Zicclsm)
since we do not implement a cache model.

An abstraction called RISCVCPUProfile is created to store the profile.
'ext_offsets' contains mandatory extensions that QEMU supports. Same
thing with the 'misa_ext' mask. Optional extensions must be enabled
manually in the command line if desired.

The design here is to use the common target/riscv/cpu.c file to store
the profile declaration and export it to the accelerator files. Each
accelerator is then responsible to expose it (or not) to users and how
to enable the extensions.

Next patches will implement the profile for TCG and KVM.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c | 32 ++++++++++++++++++++++++++++++++
 target/riscv/cpu.h | 12 ++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 316d468a19..8ee6d46967 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1441,6 +1441,38 @@ Property riscv_cpu_options[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/*
+ * 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.
+ */
+static RISCVCPUProfile RVA22U64 = {
+    .name = "rva22u64",
+    .misa_ext = RVI | RVM | RVA | RVF | RVD | RVC,
+    .ext_offsets = {
+        CPU_CFG_OFFSET(ext_zicsr), CPU_CFG_OFFSET(ext_zihintpause),
+        CPU_CFG_OFFSET(ext_zba), CPU_CFG_OFFSET(ext_zbb),
+        CPU_CFG_OFFSET(ext_zbs), CPU_CFG_OFFSET(ext_zfhmin),
+        CPU_CFG_OFFSET(ext_zkt), CPU_CFG_OFFSET(ext_zicntr),
+        CPU_CFG_OFFSET(ext_zihpm), CPU_CFG_OFFSET(ext_zicbom),
+        CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
+
+        /* mandatory named features for this profile */
+        CPU_CFG_OFFSET(zic64b),
+
+        RISCV_PROFILE_EXT_LIST_END
+    }
+};
+
+RISCVCPUProfile *riscv_profiles[] = {
+    &RVA22U64,
+    NULL,
+};
+
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index ee9abe61d6..d3d82c5a7a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -68,6 +68,18 @@ const char *riscv_get_misa_ext_description(uint32_t bit);
 
 #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
 
+typedef struct riscv_cpu_profile {
+    const char *name;
+    uint32_t misa_ext;
+    bool enabled;
+    bool user_set;
+    const int32_t ext_offsets[];
+} RISCVCPUProfile;
+
+#define RISCV_PROFILE_EXT_LIST_END -1
+
+extern RISCVCPUProfile *riscv_profiles[];
+
 /* Privileged specification version */
 enum {
     PRIV_VERSION_1_10_0 = 0,
-- 
2.41.0

[PATCH v6 05/12] target/riscv/kvm: add 'rva22u64' flag as unavailable

[Daniel Henrique Barboza]

KVM does not have the means to support enabling the rva22u64 profile.
The main reasons are:

- we're missing support for some mandatory rva22u64 extensions in the
  KVM module;

- we can't make promises about enabling a profile since it all depends
  on host support in the end.

We'll revisit this decision in the future if needed. For now mark the
'rva22u64' profile as unavailable when running a KVM CPU:

$ qemu-system-riscv64 -machine virt,accel=kvm -cpu rv64,rva22u64=true
qemu-system-riscv64: can't apply global rv64-riscv-cpu.rva22u64=true:
    'rva22u64' is not available with KVM

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6d1c0a7915..2243dfde64 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -393,7 +393,7 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
     }
 
     if (value) {
-        error_setg(errp, "extension %s is not available with KVM",
+        error_setg(errp, "'%s' is not available with KVM",
                    propname);
     }
 }
@@ -474,6 +474,11 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
     riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_extensions);
     riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_vendor_exts);
     riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_experimental_exts);
+
+   /* We don't have the needed KVM support for profiles */
+    for (i = 0; riscv_profiles[i] != NULL; i++) {
+        riscv_cpu_add_kvm_unavail_prop(cpu_obj, riscv_profiles[i]->name);
+    }
 }
 
 static int kvm_riscv_get_regs_core(CPUState *cs)
-- 
2.41.0

[PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

The TCG emulation implements all the extensions described in the
RVA22U64 profile, both mandatory and optional. The mandatory extensions
will be enabled via the profile flag. We'll leave the optional
extensions to be enabled by hand.

Given that this is the first profile we're implementing in TCG we'll
need some ground work first:

- all profiles declared in riscv_profiles[] will be exposed to users.
TCG is the main accelerator we're considering when adding profile
support in QEMU, so for now it's safe to assume that all profiles in
riscv_profiles[] will be relevant to TCG;

- we'll not support user profile settings for vendor CPUs. The flags
will still be exposed but users won't be able to change them. The idea
is that vendor CPUs in the future can enable profiles internally in
their cpu_init() functions, showing to the external world that the CPU
supports a certain profile. But users won't be able to enable/disable
it;

- Setting a profile to 'true' means 'enable all mandatory extensions of
this profile, setting it to 'false' means 'do not enable all mandatory
extensions for this profile'. This is the same semantics used by RVG.
Regular left-to-right option order will determine the resulting CPU
configuration, i.e. the following QEMU command line:

-cpu rv64,zicbom=false,zifencei=false,rva22u64=true

Enables all rva22u64 mandatory extensions, including 'zicbom' and
'zifencei', while this other command line:

-cpu rv64,rva22u64=true,zicbom=false,zifencei=false

Enables all mandatory rva22u64 extensions, and then disable both zicbom
and zifencei.

For now we'll handle multi-letter extensions only. MISA extensions need
additional steps that we'll take care later.

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 65d59bc984..5fdec8f04e 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -783,6 +783,67 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
     }
 }
 
+static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    RISCVCPUProfile *profile = opaque;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    bool value;
+    int i, ext_offset;
+
+    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) == NULL) {
+        error_setg(errp, "Profile %s only available for generic CPUs",
+                   profile->name);
+        return;
+    }
+
+    if (cpu->env.misa_mxl != MXL_RV64) {
+        error_setg(errp, "Profile %s only available for 64 bit CPUs",
+                   profile->name);
+        return;
+    }
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    profile->user_set = true;
+    profile->enabled = value;
+
+    if (!profile->enabled) {
+        return;
+    }
+
+    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
+        ext_offset = profile->ext_offsets[i];
+
+        g_hash_table_insert(multi_ext_user_opts,
+                            GUINT_TO_POINTER(ext_offset),
+                            (gpointer)profile->enabled);
+        isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
+    }
+}
+
+static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    RISCVCPUProfile *profile = opaque;
+    bool value = profile->enabled;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void riscv_cpu_add_profiles(Object *cpu_obj)
+{
+    for (int i = 0; riscv_profiles[i] != NULL; i++) {
+        const RISCVCPUProfile *profile = riscv_profiles[i];
+
+        object_property_add(cpu_obj, profile->name, "bool",
+                            cpu_get_profile, cpu_set_profile,
+                            NULL, (void *)profile);
+    }
+}
+
 static bool cpu_ext_is_deprecated(const char *ext_name)
 {
     return isupper(ext_name[0]);
@@ -906,6 +967,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
 
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
 
+    riscv_cpu_add_profiles(obj);
+
     for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
         qdev_property_add_static(DEVICE(obj), prop);
     }
-- 
2.41.0

[PATCH v6 07/12] target/riscv/tcg: add MISA user options hash

[Daniel Henrique Barboza]

We already track user choice for multi-letter extensions because we
needed to honor user choice when enabling/disabling extensions during
realize(). We refrained from adding the same mechanism for MISA
extensions since we didn't need it.

Profile support requires tne need to check for user choice for MISA
extensions, so let's add the corresponding hash now. It works like the
existing multi-letter hash (multi_ext_user_opts) but tracking MISA bits
options in the cpu_set_misa_ext_cfg() callback.

Note that we can't re-use the same hash from multi-letter extensions
because that hash uses cpu->cfg offsets as keys, while for MISA
extensions we're using MISA bits as keys.

After adding the user hash in cpu_set_misa_ext_cfg(), setting default
values with object_property_set_bool() in add_misa_properties() will end
up marking the user choice hash with them. Set the default value
manually to avoid it.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/tcg/tcg-cpu.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 5fdec8f04e..e98953aabf 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -34,6 +34,7 @@
 
 /* Hash that stores user set extensions */
 static GHashTable *multi_ext_user_opts;
+static GHashTable *misa_ext_user_opts;
 
 static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
 {
@@ -695,6 +696,10 @@ static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
         return;
     }
 
+    g_hash_table_insert(misa_ext_user_opts,
+                        GUINT_TO_POINTER(misa_bit),
+                        (gpointer)value);
+
     prev_val = env->misa_ext & misa_bit;
 
     if (value == prev_val) {
@@ -758,6 +763,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
  */
 static void riscv_cpu_add_misa_properties(Object *cpu_obj)
 {
+    CPURISCVState *env = &RISCV_CPU(cpu_obj)->env;
     bool use_def_vals = riscv_cpu_is_generic(cpu_obj);
     int i;
 
@@ -778,7 +784,13 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
                             NULL, (void *)misa_cfg);
         object_property_set_description(cpu_obj, name, desc);
         if (use_def_vals) {
-            object_property_set_bool(cpu_obj, name, misa_cfg->enabled, NULL);
+            if (misa_cfg->enabled) {
+                env->misa_ext |= bit;
+                env->misa_ext_mask |= bit;
+            } else {
+                env->misa_ext &= ~bit;
+                env->misa_ext_mask &= ~bit;
+            }
         }
     }
 }
@@ -1019,6 +1031,7 @@ static void tcg_cpu_instance_init(CPUState *cs)
     RISCVCPU *cpu = RISCV_CPU(cs);
     Object *obj = OBJECT(cpu);
 
+    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.41.0

[PATCH v6 08/12] target/riscv/tcg: add riscv_cpu_write_misa_bit()

[Daniel Henrique Barboza]

We have two instances of the setting/clearing a MISA bit from
env->misa_ext and env->misa_ext_mask pattern. And the next patch will
end up adding one more.

Create a helper to avoid code repetition.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/tcg/tcg-cpu.c | 44 ++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index e98953aabf..910360ce37 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -42,6 +42,20 @@ static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
                                  GUINT_TO_POINTER(ext_offset));
 }
 
+static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
+                                     bool enabled)
+{
+    CPURISCVState *env = &cpu->env;
+
+    if (enabled) {
+        env->misa_ext |= bit;
+        env->misa_ext_mask |= bit;
+    } else {
+        env->misa_ext &= ~bit;
+        env->misa_ext_mask &= ~bit;
+    }
+}
+
 static void riscv_cpu_synchronize_from_tb(CPUState *cs,
                                           const TranslationBlock *tb)
 {
@@ -706,20 +720,14 @@ static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    if (value) {
-        if (!generic_cpu) {
-            g_autofree char *cpuname = riscv_cpu_get_name(cpu);
-            error_setg(errp, "'%s' CPU does not allow enabling extensions",
-                       cpuname);
-            return;
-        }
-
-        env->misa_ext |= misa_bit;
-        env->misa_ext_mask |= misa_bit;
-    } else {
-        env->misa_ext &= ~misa_bit;
-        env->misa_ext_mask &= ~misa_bit;
+    if (value && !generic_cpu) {
+        g_autofree char *cpuname = riscv_cpu_get_name(cpu);
+        error_setg(errp, "'%s' CPU does not allow enabling extensions",
+                   cpuname);
+        return;
     }
+
+    riscv_cpu_write_misa_bit(cpu, misa_bit, value);
 }
 
 static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
@@ -763,7 +771,6 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
  */
 static void riscv_cpu_add_misa_properties(Object *cpu_obj)
 {
-    CPURISCVState *env = &RISCV_CPU(cpu_obj)->env;
     bool use_def_vals = riscv_cpu_is_generic(cpu_obj);
     int i;
 
@@ -784,13 +791,8 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
                             NULL, (void *)misa_cfg);
         object_property_set_description(cpu_obj, name, desc);
         if (use_def_vals) {
-            if (misa_cfg->enabled) {
-                env->misa_ext |= bit;
-                env->misa_ext_mask |= bit;
-            } else {
-                env->misa_ext &= ~bit;
-                env->misa_ext_mask &= ~bit;
-            }
+            riscv_cpu_write_misa_bit(RISCV_CPU(cpu_obj), bit,
+                                     misa_cfg->enabled);
         }
     }
 }
-- 
2.41.0

[PATCH v6 09/12] target/riscv/tcg: handle profile MISA bits

[Daniel Henrique Barboza]

The profile support is handling multi-letter extensions only. Let's add
support for MISA bits as well.

We'll go through every known MISA bit. If the profile doesn't declare
the bit as mandatory, ignore it. Otherwise, set the bit in env->misa_ext
and env->misa_ext_mask.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/tcg/tcg-cpu.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 910360ce37..6ba27b824b 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -828,6 +828,19 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
         return;
     }
 
+    for (i = 0; misa_bits[i] != 0; i++) {
+        uint32_t bit = misa_bits[i];
+
+        if  (!(profile->misa_ext & bit)) {
+            continue;
+        }
+
+        g_hash_table_insert(misa_ext_user_opts,
+                            GUINT_TO_POINTER(bit),
+                            (gpointer)value);
+        riscv_cpu_write_misa_bit(cpu, bit, profile->enabled);
+    }
+
     for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
         ext_offset = profile->ext_offsets[i];
 
-- 
2.41.0

[PATCH v6 10/12] target/riscv/tcg: add hash table insert helpers

[Daniel Henrique Barboza]

Latest patches added several g_hash_table_insert() patterns. Add two
helpers, one for each user hash, to make the code cleaner.

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 6ba27b824b..3a96b1f476 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -42,6 +42,18 @@ static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
                                  GUINT_TO_POINTER(ext_offset));
 }
 
+static void cpu_cfg_ext_add_user_opt(uint32_t ext_offset, bool value)
+{
+    g_hash_table_insert(multi_ext_user_opts, GUINT_TO_POINTER(ext_offset),
+                        (gpointer)value);
+}
+
+static void cpu_misa_ext_add_user_opt(uint32_t bit, bool value)
+{
+    g_hash_table_insert(misa_ext_user_opts, GUINT_TO_POINTER(bit),
+                        (gpointer)value);
+}
+
 static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
                                      bool enabled)
 {
@@ -710,9 +722,7 @@ static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    g_hash_table_insert(misa_ext_user_opts,
-                        GUINT_TO_POINTER(misa_bit),
-                        (gpointer)value);
+    cpu_misa_ext_add_user_opt(misa_bit, value);
 
     prev_val = env->misa_ext & misa_bit;
 
@@ -835,18 +845,14 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
             continue;
         }
 
-        g_hash_table_insert(misa_ext_user_opts,
-                            GUINT_TO_POINTER(bit),
-                            (gpointer)value);
+        cpu_misa_ext_add_user_opt(bit, profile->enabled);
         riscv_cpu_write_misa_bit(cpu, bit, profile->enabled);
     }
 
     for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
         ext_offset = profile->ext_offsets[i];
 
-        g_hash_table_insert(multi_ext_user_opts,
-                            GUINT_TO_POINTER(ext_offset),
-                            (gpointer)profile->enabled);
+        cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled);
         isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
     }
 }
@@ -909,9 +915,7 @@ static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
                     multi_ext_cfg->name, lower);
     }
 
-    g_hash_table_insert(multi_ext_user_opts,
-                        GUINT_TO_POINTER(multi_ext_cfg->offset),
-                        (gpointer)value);
+    cpu_cfg_ext_add_user_opt(multi_ext_cfg->offset, value);
 
     prev_val = isa_ext_is_enabled(cpu, multi_ext_cfg->offset);
 
-- 
2.41.0

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

[Daniel Henrique Barboza]

RVG behaves like a profile: a single flag enables a set of bits. Right
now we're considering user choice when handling RVG and zicsr/zifencei
and ignoring user choice on MISA bits.

We'll add user warnings for profiles when the user disables its
mandatory extensions in the next patch. We'll do the same thing with RVG
now to keep consistency between RVG and profile handling.

First and foremost, create a new RVG only helper to avoid clogging
riscv_cpu_validate_set_extensions(). We do not want to annoy users with
RVG warnings like we did in the past (see 9b9741c38f), thus we'll only
warn if RVG was user set and the user disabled a RVG extension in the
command line.

For every RVG MISA bit (IMAFD), zicsr and zifencei, the logic then
becomes:

- if enabled, do nothing;
- if disabled and not user set, enable it;
- if disabled and user set, throw a warning that it's a RVG mandatory
  extension.

This same logic will be used for profiles in the next patch.

Note that this is a behavior change, where we would error out if the
user disabled either zicsr or zifencei. As long as users are explicitly
disabling things in the command line we'll let them have a go at it, at
least in this step. We'll error out later in the validation if needed.

Other notable changes from the previous RVG code:

- use riscv_cpu_write_misa_bit() instead of manually updating both
  env->misa_ext and env->misa_ext_mask;

- set zicsr and zifencei directly. We're already checking if they
  were user set and priv version will never fail for these
  extensions, making cpu_cfg_ext_auto_update() redundant.

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

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

[PATCH v6 12/12] target/riscv/tcg: warn if profile exts are disabled

[Daniel Henrique Barboza]

Enabling a profile and then disabling some of its mandatory extensions
is a valid use. It can be useful for debugging and testing. But the
common expected use of enabling a profile is to enable all its mandatory
extensions.

Add an user warning when mandatory extensions from an enabled profile
are disabled in the command line, like we're already doing with RVG.

After this patch, this will throw warnings:

-cpu rv64,rva22u64=true,zihintpause=false,zicbom=false,zicboz=false

qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension zihintpause
qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension zicbom
qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension zicboz

Note that the following  will NOT throw warnings because the profile is
being enabled last, hence all its mandatory extensions will be enabled:

-cpu rv64,zihintpause=false,zicbom=false,zicboz=false,rva22u64=true

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 953e8432d6..5e7855b41b 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -147,6 +147,27 @@ static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
     g_assert_not_reached();
 }
 
+static const char *cpu_cfg_ext_get_name(uint32_t ext_offset)
+{
+    const RISCVCPUMultiExtConfig *feat;
+    const RISCVIsaExtData *edata;
+
+    for (edata = isa_edata_arr; edata && edata->name; edata++) {
+        if (edata->ext_enable_offset == ext_offset) {
+            return edata->name;
+        }
+    }
+
+    for (feat = riscv_cpu_named_features; feat->name != NULL; feat++) {
+        if (feat->offset == ext_offset) {
+            return feat->name;
+        }
+    }
+
+    g_assert_not_reached();
+}
+
+
 static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
                                     bool value)
 {
@@ -631,6 +652,45 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     riscv_cpu_disable_priv_spec_isa_exts(cpu);
 }
 
+static void riscv_cpu_validate_profile(RISCVCPU *cpu,
+                                       RISCVCPUProfile *profile)
+{
+    const char *warn_msg = "Profile %s mandates disabled extension %s";
+    int i;
+
+    for (i = 0; misa_bits[i] != 0; i++) {
+        uint32_t bit = misa_bits[i];
+
+        if (!(profile->misa_ext & bit)) {
+            continue;
+        }
+
+        if (!riscv_has_ext(&cpu->env, bit)) {
+            warn_report(warn_msg, profile->name, riscv_get_misa_ext_name(bit));
+        }
+    }
+
+    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
+        int ext_offset = profile->ext_offsets[i];
+
+        if (!isa_ext_is_enabled(cpu, ext_offset)) {
+            warn_report(warn_msg, profile->name,
+                        cpu_cfg_ext_get_name(ext_offset));
+        }
+    }
+}
+
+static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
+{
+    for (int i = 0; riscv_profiles[i] != NULL; i++) {
+        RISCVCPUProfile *profile = riscv_profiles[i];
+
+        if (profile->user_set && profile->enabled) {
+            riscv_cpu_validate_profile(cpu, profile);
+        }
+    }
+}
+
 void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
 {
     CPURISCVState *env = &cpu->env;
@@ -649,6 +709,7 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
     }
 
     riscv_cpu_validate_named_features(cpu);
+    riscv_cpu_validate_profiles(cpu);
 
     if (cpu->cfg.ext_smepmp && !cpu->cfg.pmp) {
         /*
-- 
2.41.0

Re: [PATCH v6 01/12] target/riscv: add zicbop extension flag

[Andrew Jones]

On Sat, Oct 28, 2023 at 05:54:16AM -0300, Daniel Henrique Barboza wrote:
> QEMU already implements zicbom (Cache Block Management Operations) and
> zicboz (Cache Block Zero Operations). Commit 59cb29d6a5 ("target/riscv:
> add Zicbop cbo.prefetch{i, r, m} placeholder") added placeholders for
> what would be the instructions for zicbop (Cache Block Prefetch
> Operations), which are now no-ops.
> 
> The RVA22U64 profile mandates zicbop, which means that applications that
> run with this profile might expect zicbop to be present in the riscv,isa
> DT and might behave badly if it's absent.
> 
> Adding zicbop as an extension will make our future RVA22U64
> implementation more in line with what userspace expects and, if/when
> cache block prefetch operations became relevant to QEMU, we already have
> the extension flag to turn then on/off as needed.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/virt.c        | 5 +++++
>  target/riscv/cpu.c     | 3 +++
>  target/riscv/cpu_cfg.h | 2 ++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 1732c42915..99c087240f 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -273,6 +273,11 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>                                    cpu_ptr->cfg.cboz_blocksize);
>          }
>  
> +        if (cpu_ptr->cfg.ext_zicbop) {
> +            qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbop-block-size",

I think we need to get this node approved by devicetree@vger.kernel.org
and merged into [1] first.

[1] Linux repo: Documentation/devicetree/bindings/riscv/cpus.yaml

> +                                  cpu_ptr->cfg.cbop_blocksize);
> +        }
> +
>          qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv");
>          qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", "okay");
>          qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg",
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f40da4c661..6c0050988f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -78,6 +78,7 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
>   */
>  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(zicond, PRIV_VERSION_1_12_0, ext_zicond),
>      ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
> @@ -1336,6 +1337,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      MULTI_EXT_CFG_BOOL("zhinxmin", ext_zhinxmin, false),
>  
>      MULTI_EXT_CFG_BOOL("zicbom", ext_zicbom, true),
> +    MULTI_EXT_CFG_BOOL("zicbop", ext_zicbop, true),
>      MULTI_EXT_CFG_BOOL("zicboz", ext_zicboz, true),
>  
>      MULTI_EXT_CFG_BOOL("zmmul", ext_zmmul, false),
> @@ -1424,6 +1426,7 @@ Property riscv_cpu_options[] = {
>      DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
>  
>      DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
> +    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
>      DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>  
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 6eef4a51ea..2203b4c45b 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -65,6 +65,7 @@ struct RISCVCPUConfig {
>      bool ext_zicntr;
>      bool ext_zicsr;
>      bool ext_zicbom;
> +    bool ext_zicbop;
>      bool ext_zicboz;
>      bool ext_zicond;
>      bool ext_zihintntl;
> @@ -134,6 +135,7 @@ struct RISCVCPUConfig {
>      uint16_t vlen;
>      uint16_t elen;
>      uint16_t cbom_blocksize;
> +    uint16_t cbop_blocksize;
>      uint16_t cboz_blocksize;
>      bool mmu;
>      bool pmp;
> -- 
> 2.41.0
>

Otherwise,

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

Thanks,
drew

Re: [PATCH v6 02/12] target/riscv/tcg: add 'zic64b' support

[Andrew Jones]

On Sat, Oct 28, 2023 at 05:54:17AM -0300, Daniel Henrique Barboza wrote:
> zic64b is defined in the RVA22U64 profile [1] as a named feature for
> "Cache blocks must be 64 bytes in size, naturally aligned in the address
> space". It's a fantasy name for 64 bytes cache blocks. The RVA22U64
> profile mandates this feature, meaning that applications using this
> profile expects 64 bytes cache blocks.
> 
> To make the upcoming RVA22U64 implementation complete, we'll zic64b as
> a 'named feature', not a regular extension. This means that:
> 
> - it won't be exposed to users;
> - it won't be written in riscv,isa.
> 
> This will be extended to other named extensions in the future, so we're
> creating some common boilerplate for them as well.
> 
> zic64b is default to 'true' since we're already using 64 bytes blocks.
> If any cache block size (cbo{m,p,z}_blocksize) is changed to something
> different than 64, zic64b is set to 'false'.
> 
> Our profile implementation will then be able to check the current state
> of zic64b and take the appropriate action (e.g. throw a warning).
> 
> [1] https://github.com/riscv/riscv-profiles/releases/download/v1.0/profiles.pdf
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         | 15 ++++++++++++---
>  target/riscv/cpu.h         |  3 +++
>  target/riscv/cpu_cfg.h     |  1 +
>  target/riscv/tcg/tcg-cpu.c | 14 ++++++++++++++
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 6c0050988f..316d468a19 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1396,6 +1396,12 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
> +    MULTI_EXT_CFG_BOOL("zic64b", zic64b, true),
> +
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  /* Deprecated entries marked for future removal */
>  const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[] = {
>      MULTI_EXT_CFG_BOOL("Zifencei", ext_zifencei, true),
> @@ -1425,9 +1431,12 @@ Property riscv_cpu_options[] = {
>      DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
>      DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
>  
> -    DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
> -    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
> -    DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
> +    DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU,
> +                       cfg.cbom_blocksize, CB_DEF_VALUE),
> +    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU,
> +                       cfg.cbop_blocksize, CB_DEF_VALUE),
> +    DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU,
> +                       cfg.cboz_blocksize, CB_DEF_VALUE),

I wouldn't introduce the CB_DEF_VALUE define. I state why below.

>  
>      DEFINE_PROP_END_OF_LIST(),
>  };
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 8efc4d83ec..ee9abe61d6 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -64,6 +64,8 @@ extern const uint32_t misa_bits[];
>  const char *riscv_get_misa_ext_name(uint32_t bit);
>  const char *riscv_get_misa_ext_description(uint32_t bit);
>  
> +#define CB_DEF_VALUE 64
> +
>  #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
>  
>  /* Privileged specification version */
> @@ -745,6 +747,7 @@ typedef struct RISCVCPUMultiExtConfig {
>  extern const RISCVCPUMultiExtConfig riscv_cpu_extensions[];
>  extern const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[];
>  extern const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[];
> +extern const RISCVCPUMultiExtConfig riscv_cpu_named_features[];
>  extern const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[];
>  extern Property riscv_cpu_options[];
>  
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 2203b4c45b..f61a8434c4 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -108,6 +108,7 @@ struct RISCVCPUConfig {
>      bool ext_smepmp;
>      bool rvv_ta_all_1s;
>      bool rvv_ma_all_1s;
> +    bool zic64b;
>  
>      uint32_t mvendorid;
>      uint64_t marchid;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 093bda2e75..65d59bc984 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -264,6 +264,18 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>      }
>  }
>  
> +static void riscv_cpu_validate_zic64b(RISCVCPU *cpu)
> +{
> +    cpu->cfg.zic64b = cpu->cfg.cbom_blocksize == CB_DEF_VALUE &&
> +                      cpu->cfg.cbop_blocksize == CB_DEF_VALUE &&
> +                      cpu->cfg.cboz_blocksize == CB_DEF_VALUE;

The zic64b name has an explicit 64 in it, so CB_DEF_VALUE must be 64,
which implies it should also be named something with an explicit 64
in it. However, there's really no point in doing

 #define NUM_64 64

so I'd just drop the define altogether.

> +}
> +
> +static void riscv_cpu_validate_named_features(RISCVCPU *cpu)
> +{
> +    riscv_cpu_validate_zic64b(cpu);
> +}
> +
>  /*
>   * Check consistency between chosen extensions while setting
>   * cpu->cfg accordingly.
> @@ -586,6 +598,8 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>          return;
>      }
>  
> +    riscv_cpu_validate_named_features(cpu);
> +
>      if (cpu->cfg.ext_smepmp && !cpu->cfg.pmp) {
>          /*
>           * Enhanced PMP should only be available
> -- 
> 2.41.0
>

Thanks,
drew

Re: [PATCH v6 03/12] riscv-qmp-cmds.c: expose named features in cpu_model_expansion

[Andrew Jones]

On Sat, Oct 28, 2023 at 05:54:18AM -0300, Daniel Henrique Barboza wrote:
> Named features (zic64b the sole example at this moment) aren't expose to
> users, thus we need another way to expose them.
> 
> Go through each named feature, get its boolean value, do the needed
> conversions (bool to qbool, qbool to QObject) and add it to output dict.
> 
> Another adjustment is needed: named features are evaluated during
> finalize(), so riscv_cpu_finalize_features() needs to be mandatory
> regardless of whether we have an input dict or not. Otherwise zic64b
> will always return 'false', which is incorrect: the default values of
> cache blocksizes (cbom_blocksize and cboz_blocksize) are set to 64,

and cbop_blocksize

> satisfying the conditions for zic64b.
> 
> Here's an API usage example after this patch:
> 
>  $ ./build/qemu-system-riscv64 -S -M virt -display none
>     -qmp tcp:localhost:1234,server,wait=off
> 
>  $ ./scripts/qmp/qmp-shell localhost:1234
> Welcome to the QMP low-level shell!
> Connected to QEMU 8.1.50
> 
> (QEMU) query-cpu-model-expansion type=full model={"name":"rv64"}
> {"return": {"model":
>     {"name": "rv64", "props": {... "zic64b": true, ...}}}}
> 
> zic64b is set to 'true', as expected, since all cache sizes are 64
> bytes by default.
> 
> If we change one of the cache blocksizes, zic64b is returned as 'false':
> 
> (QEMU) query-cpu-model-expansion type=full model={"name":"rv64","props":{"cbom_blocksize":128}}
> {"return": {"model":
>     {"name": "rv64", "props": {... "zic64b": false, ...}}}}
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/riscv-qmp-cmds.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
> index 2f2dbae7c8..5ada279776 100644
> --- a/target/riscv/riscv-qmp-cmds.c
> +++ b/target/riscv/riscv-qmp-cmds.c
> @@ -26,6 +26,7 @@
>  
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-machine-target.h"
> +#include "qapi/qmp/qbool.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qobject-input-visitor.h"
> @@ -99,6 +100,22 @@ static void riscv_obj_add_multiext_props(Object *obj, QDict *qdict_out,
>      }
>  }
>  
> +static void riscv_obj_add_named_feats_qdict(Object *obj, QDict *qdict_out)
> +{
> +    const RISCVCPUMultiExtConfig *named_cfg;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    QObject *value;
> +    bool flag_val;
> +
> +    for (int i = 0; riscv_cpu_named_features[i].name != NULL; i++) {
> +        named_cfg = &riscv_cpu_named_features[i];
> +        flag_val = isa_ext_is_enabled(cpu, named_cfg->offset);
> +        value = QOBJECT(qbool_from_bool(flag_val));
> +
> +        qdict_put_obj(qdict_out, named_cfg->name, value);
> +    }
> +}
> +
>  static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
>                                             const QDict *qdict_in,
>                                             Error **errp)
> @@ -129,11 +146,6 @@ static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
>          goto err;
>      }
>  
> -    riscv_cpu_finalize_features(RISCV_CPU(obj), &local_err);
> -    if (local_err) {
> -        goto err;
> -    }
> -
>      visit_end_struct(visitor, NULL);
>  
>  err:
> @@ -191,6 +203,13 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>          }
>      }
>  
> +    riscv_cpu_finalize_features(RISCV_CPU(obj), &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        object_unref(obj);
> +        return NULL;
> +    }
> +
>      expansion_info = g_new0(CpuModelExpansionInfo, 1);
>      expansion_info->model = g_malloc0(sizeof(*expansion_info->model));
>      expansion_info->model->name = g_strdup(model->name);
> @@ -200,6 +219,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      riscv_obj_add_multiext_props(obj, qdict_out, riscv_cpu_extensions);
>      riscv_obj_add_multiext_props(obj, qdict_out, riscv_cpu_experimental_exts);
>      riscv_obj_add_multiext_props(obj, qdict_out, riscv_cpu_vendor_exts);
> +    riscv_obj_add_named_feats_qdict(obj, qdict_out);
>  
>      /* Add our CPU boolean options too */
>      riscv_obj_add_qdict_prop(obj, qdict_out, "mmu");
> -- 
> 2.41.0
>

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

Re: [PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Sat, Oct 28, 2023 at 05:54:21AM -0300, Daniel Henrique Barboza wrote:
> The TCG emulation implements all the extensions described in the
> RVA22U64 profile, both mandatory and optional. The mandatory extensions
> will be enabled via the profile flag. We'll leave the optional
> extensions to be enabled by hand.
> 
> Given that this is the first profile we're implementing in TCG we'll
> need some ground work first:
> 
> - all profiles declared in riscv_profiles[] will be exposed to users.
> TCG is the main accelerator we're considering when adding profile
> support in QEMU, so for now it's safe to assume that all profiles in
> riscv_profiles[] will be relevant to TCG;
> 
> - we'll not support user profile settings for vendor CPUs. The flags
> will still be exposed but users won't be able to change them. The idea
> is that vendor CPUs in the future can enable profiles internally in
> their cpu_init() functions, showing to the external world that the CPU
> supports a certain profile. But users won't be able to enable/disable
> it;
> 
> - Setting a profile to 'true' means 'enable all mandatory extensions of
> this profile, setting it to 'false' means 'do not enable all mandatory
> extensions for this profile'.

While this definition of profile=off looks appealing at first, it's really
just saying 'do not do anything', which means it's limiting the potential
of the property. But, I'll stop talking about this now, as I have another
design suggestion instead:

Since profiles are like G, and other shorthand extensions (just without an
official shorthand extension name), then I believe they should behave like
shorthand extensions. Also, since shorthand extensions can be derived from
their parts, then I think shorthand extensions should behave like
synthetic extensions. For example, zic64b starts 'false', but, at realize
time, if all its conditions are present, then it turns 'true'. Shorthand
extensions should be able to do that too by detecting that each of the
extensions they represent is present.

So, I think we should automatically turn G and profiles (and future
shorthand extensions) on when all their respective extensions are present.

Thanks,
drew

Re: [PATCH v6 01/12] target/riscv: add zicbop extension flag

[Daniel Henrique Barboza]

On 10/28/23 06:49, Andrew Jones wrote:
> On Sat, Oct 28, 2023 at 05:54:16AM -0300, Daniel Henrique Barboza wrote:
>> QEMU already implements zicbom (Cache Block Management Operations) and
>> zicboz (Cache Block Zero Operations). Commit 59cb29d6a5 ("target/riscv:
>> add Zicbop cbo.prefetch{i, r, m} placeholder") added placeholders for
>> what would be the instructions for zicbop (Cache Block Prefetch
>> Operations), which are now no-ops.
>>
>> The RVA22U64 profile mandates zicbop, which means that applications that
>> run with this profile might expect zicbop to be present in the riscv,isa
>> DT and might behave badly if it's absent.
>>
>> Adding zicbop as an extension will make our future RVA22U64
>> implementation more in line with what userspace expects and, if/when
>> cache block prefetch operations became relevant to QEMU, we already have
>> the extension flag to turn then on/off as needed.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/virt.c        | 5 +++++
>>   target/riscv/cpu.c     | 3 +++
>>   target/riscv/cpu_cfg.h | 2 ++
>>   3 files changed, 10 insertions(+)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 1732c42915..99c087240f 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -273,6 +273,11 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>>                                     cpu_ptr->cfg.cboz_blocksize);
>>           }
>>   
>> +        if (cpu_ptr->cfg.ext_zicbop) {
>> +            qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbop-block-size",
> 
> I think we need to get this node approved by devicetree@vger.kernel.org
> and merged into [1] first.
> 
> [1] Linux repo: Documentation/devicetree/bindings/riscv/cpus.yaml

Ouch ... I wasn't expecting that :(

We can't add zicbop without the blocksize DT and then claim that we're
implement rva22u64. It'll be an 'almost implementation' of the profile
because Zicbop is 'almost supported'.

I'll send a DT patch for cbop-block-size and see how that goes. We would
need an ack for the DT folks that we're not adding the wrong attribute
for cbop. If we can't have an ack in time we should just postpone profile
support for next release.

I'll send a v7 to keep reviews rolling on our side but now it seems like
we have a kernel dependency to merge this. Thanks,


Daniel


> 
>> +                                  cpu_ptr->cfg.cbop_blocksize);
>> +        }
>> +
>>           qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv");
>>           qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", "okay");
>>           qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg",
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index f40da4c661..6c0050988f 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -78,6 +78,7 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
>>    */
>>   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(zicond, PRIV_VERSION_1_12_0, ext_zicond),
>>       ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
>> @@ -1336,6 +1337,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>>       MULTI_EXT_CFG_BOOL("zhinxmin", ext_zhinxmin, false),
>>   
>>       MULTI_EXT_CFG_BOOL("zicbom", ext_zicbom, true),
>> +    MULTI_EXT_CFG_BOOL("zicbop", ext_zicbop, true),
>>       MULTI_EXT_CFG_BOOL("zicboz", ext_zicboz, true),
>>   
>>       MULTI_EXT_CFG_BOOL("zmmul", ext_zmmul, false),
>> @@ -1424,6 +1426,7 @@ Property riscv_cpu_options[] = {
>>       DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
>>   
>>       DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
>> +    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
>>       DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>>   
>>       DEFINE_PROP_END_OF_LIST(),
>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>> index 6eef4a51ea..2203b4c45b 100644
>> --- a/target/riscv/cpu_cfg.h
>> +++ b/target/riscv/cpu_cfg.h
>> @@ -65,6 +65,7 @@ struct RISCVCPUConfig {
>>       bool ext_zicntr;
>>       bool ext_zicsr;
>>       bool ext_zicbom;
>> +    bool ext_zicbop;
>>       bool ext_zicboz;
>>       bool ext_zicond;
>>       bool ext_zihintntl;
>> @@ -134,6 +135,7 @@ struct RISCVCPUConfig {
>>       uint16_t vlen;
>>       uint16_t elen;
>>       uint16_t cbom_blocksize;
>> +    uint16_t cbop_blocksize;
>>       uint16_t cboz_blocksize;
>>       bool mmu;
>>       bool pmp;
>> -- 
>> 2.41.0
>>
> 
> Otherwise,
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> Thanks,
> drew

Re: [PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Alistair Francis]

On Sat, Oct 28, 2023 at 8:44 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Sat, Oct 28, 2023 at 05:54:21AM -0300, Daniel Henrique Barboza wrote:
> > The TCG emulation implements all the extensions described in the
> > RVA22U64 profile, both mandatory and optional. The mandatory extensions
> > will be enabled via the profile flag. We'll leave the optional
> > extensions to be enabled by hand.
> >
> > Given that this is the first profile we're implementing in TCG we'll
> > need some ground work first:
> >
> > - all profiles declared in riscv_profiles[] will be exposed to users.
> > TCG is the main accelerator we're considering when adding profile
> > support in QEMU, so for now it's safe to assume that all profiles in
> > riscv_profiles[] will be relevant to TCG;
> >
> > - we'll not support user profile settings for vendor CPUs. The flags
> > will still be exposed but users won't be able to change them. The idea
> > is that vendor CPUs in the future can enable profiles internally in
> > their cpu_init() functions, showing to the external world that the CPU
> > supports a certain profile. But users won't be able to enable/disable
> > it;
> >
> > - Setting a profile to 'true' means 'enable all mandatory extensions of
> > this profile, setting it to 'false' means 'do not enable all mandatory
> > extensions for this profile'.
>
> While this definition of profile=off looks appealing at first, it's really
> just saying 'do not do anything', which means it's limiting the potential
> of the property. But, I'll stop talking about this now, as I have another

This seems like the way to go to me

> design suggestion instead:
>
> Since profiles are like G, and other shorthand extensions (just without an
> official shorthand extension name), then I believe they should behave like
> shorthand extensions. Also, since shorthand extensions can be derived from
> their parts, then I think shorthand extensions should behave like
> synthetic extensions. For example, zic64b starts 'false', but, at realize
> time, if all its conditions are present, then it turns 'true'. Shorthand
> extensions should be able to do that too by detecting that each of the
> extensions they represent is present.
>
> So, I think we should automatically turn G and profiles (and future
> shorthand extensions) on when all their respective extensions are present.

I think that's a good idea and something we should support

Alistair

>
> Thanks,
> drew
>

Re: [PATCH v6 09/12] target/riscv/tcg: handle profile MISA bits

[Alistair Francis]

On Sat, Oct 28, 2023 at 7:35 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The profile support is handling multi-letter extensions only. Let's add
> support for MISA bits as well.
>
> We'll go through every known MISA bit. If the profile doesn't declare
> the bit as mandatory, ignore it. Otherwise, set the bit in env->misa_ext
> and env->misa_ext_mask.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 910360ce37..6ba27b824b 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -828,6 +828,19 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>
> +    for (i = 0; misa_bits[i] != 0; i++) {
> +        uint32_t bit = misa_bits[i];
> +
> +        if  (!(profile->misa_ext & bit)) {
> +            continue;
> +        }
> +
> +        g_hash_table_insert(misa_ext_user_opts,
> +                            GUINT_TO_POINTER(bit),
> +                            (gpointer)value);
> +        riscv_cpu_write_misa_bit(cpu, bit, profile->enabled);
> +    }
> +
>      for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
>          ext_offset = profile->ext_offsets[i];
>
> --
> 2.41.0
>
>

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

[Alistair Francis]

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

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

Alistair

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

Re: [PATCH v6 12/12] target/riscv/tcg: warn if profile exts are disabled

[Alistair Francis]

On Sat, Oct 28, 2023 at 8:07 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Enabling a profile and then disabling some of its mandatory extensions
> is a valid use. It can be useful for debugging and testing. But the
> common expected use of enabling a profile is to enable all its mandatory
> extensions.
>
> Add an user warning when mandatory extensions from an enabled profile
> are disabled in the command line, like we're already doing with RVG.
>
> After this patch, this will throw warnings:
>
> -cpu rv64,rva22u64=true,zihintpause=false,zicbom=false,zicboz=false
>
> qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension zihintpause
> qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension zicbom
> qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension zicboz
>
> Note that the following  will NOT throw warnings because the profile is
> being enabled last, hence all its mandatory extensions will be enabled:
>
> -cpu rv64,zihintpause=false,zicbom=false,zicboz=false,rva22u64=true
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 61 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 953e8432d6..5e7855b41b 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -147,6 +147,27 @@ static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
>      g_assert_not_reached();
>  }
>
> +static const char *cpu_cfg_ext_get_name(uint32_t ext_offset)
> +{
> +    const RISCVCPUMultiExtConfig *feat;
> +    const RISCVIsaExtData *edata;
> +
> +    for (edata = isa_edata_arr; edata && edata->name; edata++) {
> +        if (edata->ext_enable_offset == ext_offset) {
> +            return edata->name;
> +        }
> +    }
> +
> +    for (feat = riscv_cpu_named_features; feat->name != NULL; feat++) {
> +        if (feat->offset == ext_offset) {
> +            return feat->name;
> +        }
> +    }
> +
> +    g_assert_not_reached();
> +}
> +
> +
>  static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
>                                      bool value)
>  {
> @@ -631,6 +652,45 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>      riscv_cpu_disable_priv_spec_isa_exts(cpu);
>  }
>
> +static void riscv_cpu_validate_profile(RISCVCPU *cpu,
> +                                       RISCVCPUProfile *profile)
> +{
> +    const char *warn_msg = "Profile %s mandates disabled extension %s";
> +    int i;
> +
> +    for (i = 0; misa_bits[i] != 0; i++) {
> +        uint32_t bit = misa_bits[i];
> +
> +        if (!(profile->misa_ext & bit)) {
> +            continue;
> +        }
> +
> +        if (!riscv_has_ext(&cpu->env, bit)) {
> +            warn_report(warn_msg, profile->name, riscv_get_misa_ext_name(bit));
> +        }
> +    }
> +
> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
> +        int ext_offset = profile->ext_offsets[i];
> +
> +        if (!isa_ext_is_enabled(cpu, ext_offset)) {
> +            warn_report(warn_msg, profile->name,
> +                        cpu_cfg_ext_get_name(ext_offset));
> +        }
> +    }
> +}
> +
> +static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
> +{
> +    for (int i = 0; riscv_profiles[i] != NULL; i++) {
> +        RISCVCPUProfile *profile = riscv_profiles[i];
> +
> +        if (profile->user_set && profile->enabled) {
> +            riscv_cpu_validate_profile(cpu, profile);
> +        }
> +    }
> +}
> +
>  void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>  {
>      CPURISCVState *env = &cpu->env;
> @@ -649,6 +709,7 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>      }
>
>      riscv_cpu_validate_named_features(cpu);
> +    riscv_cpu_validate_profiles(cpu);
>
>      if (cpu->cfg.ext_smepmp && !cpu->cfg.pmp) {
>          /*
> --
> 2.41.0
>
>

Re: [PATCH v6 01/12] target/riscv: add zicbop extension flag

[Daniel Henrique Barboza]

On 10/28/23 06:49, Andrew Jones wrote:
> On Sat, Oct 28, 2023 at 05:54:16AM -0300, Daniel Henrique Barboza wrote:
>> QEMU already implements zicbom (Cache Block Management Operations) and
>> zicboz (Cache Block Zero Operations). Commit 59cb29d6a5 ("target/riscv:
>> add Zicbop cbo.prefetch{i, r, m} placeholder") added placeholders for
>> what would be the instructions for zicbop (Cache Block Prefetch
>> Operations), which are now no-ops.
>>
>> The RVA22U64 profile mandates zicbop, which means that applications that
>> run with this profile might expect zicbop to be present in the riscv,isa
>> DT and might behave badly if it's absent.
>>
>> Adding zicbop as an extension will make our future RVA22U64
>> implementation more in line with what userspace expects and, if/when
>> cache block prefetch operations became relevant to QEMU, we already have
>> the extension flag to turn then on/off as needed.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/virt.c        | 5 +++++
>>   target/riscv/cpu.c     | 3 +++
>>   target/riscv/cpu_cfg.h | 2 ++
>>   3 files changed, 10 insertions(+)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 1732c42915..99c087240f 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -273,6 +273,11 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>>                                     cpu_ptr->cfg.cboz_blocksize);
>>           }
>>   
>> +        if (cpu_ptr->cfg.ext_zicbop) {
>> +            qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbop-block-size",
> 
> I think we need to get this node approved by devicetree@vger.kernel.org
> and merged into [1] first.
> 
> [1] Linux repo: Documentation/devicetree/bindings/riscv/cpus.yaml

We got an ack from the devicetree folks to add the "riscv,cbop-block-size"
DT node:

https://lore.kernel.org/linux-devicetree/20231030-attest-unchain-ab99981a5738@spud/


I believe we don't have blockers to proceed with this change then. Thanks,


Daniel

> 
>> +                                  cpu_ptr->cfg.cbop_blocksize);
>> +        }
>> +
>>           qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv");
>>           qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", "okay");
>>           qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg",
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index f40da4c661..6c0050988f 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -78,6 +78,7 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
>>    */
>>   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(zicond, PRIV_VERSION_1_12_0, ext_zicond),
>>       ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
>> @@ -1336,6 +1337,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>>       MULTI_EXT_CFG_BOOL("zhinxmin", ext_zhinxmin, false),
>>   
>>       MULTI_EXT_CFG_BOOL("zicbom", ext_zicbom, true),
>> +    MULTI_EXT_CFG_BOOL("zicbop", ext_zicbop, true),
>>       MULTI_EXT_CFG_BOOL("zicboz", ext_zicboz, true),
>>   
>>       MULTI_EXT_CFG_BOOL("zmmul", ext_zmmul, false),
>> @@ -1424,6 +1426,7 @@ Property riscv_cpu_options[] = {
>>       DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
>>   
>>       DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
>> +    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
>>       DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>>   
>>       DEFINE_PROP_END_OF_LIST(),
>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
>> index 6eef4a51ea..2203b4c45b 100644
>> --- a/target/riscv/cpu_cfg.h
>> +++ b/target/riscv/cpu_cfg.h
>> @@ -65,6 +65,7 @@ struct RISCVCPUConfig {
>>       bool ext_zicntr;
>>       bool ext_zicsr;
>>       bool ext_zicbom;
>> +    bool ext_zicbop;
>>       bool ext_zicboz;
>>       bool ext_zicond;
>>       bool ext_zihintntl;
>> @@ -134,6 +135,7 @@ struct RISCVCPUConfig {
>>       uint16_t vlen;
>>       uint16_t elen;
>>       uint16_t cbom_blocksize;
>> +    uint16_t cbop_blocksize;
>>       uint16_t cboz_blocksize;
>>       bool mmu;
>>       bool pmp;
>> -- 
>> 2.41.0
>>
> 
> Otherwise,
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> Thanks,
> drew

Re: [PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/28/23 05:54, Daniel Henrique Barboza wrote:
> The TCG emulation implements all the extensions described in the
> RVA22U64 profile, both mandatory and optional. The mandatory extensions
> will be enabled via the profile flag. We'll leave the optional
> extensions to be enabled by hand.
> 
> Given that this is the first profile we're implementing in TCG we'll
> need some ground work first:
> 
> - all profiles declared in riscv_profiles[] will be exposed to users.
> TCG is the main accelerator we're considering when adding profile
> support in QEMU, so for now it's safe to assume that all profiles in
> riscv_profiles[] will be relevant to TCG;
> 
> - we'll not support user profile settings for vendor CPUs. The flags
> will still be exposed but users won't be able to change them. The idea
> is that vendor CPUs in the future can enable profiles internally in
> their cpu_init() functions, showing to the external world that the CPU
> supports a certain profile. But users won't be able to enable/disable
> it;
> 
> - Setting a profile to 'true' means 'enable all mandatory extensions of
> this profile, setting it to 'false' means 'do not enable all mandatory
> extensions for this profile'. This is the same semantics used by RVG.
> Regular left-to-right option order will determine the resulting CPU
> configuration, i.e. the following QEMU command line:
> 
> -cpu rv64,zicbom=false,zifencei=false,rva22u64=true
> 
> Enables all rva22u64 mandatory extensions, including 'zicbom' and
> 'zifencei', while this other command line:
> 
> -cpu rv64,rva22u64=true,zicbom=false,zifencei=false
> 
> Enables all mandatory rva22u64 extensions, and then disable both zicbom
> and zifencei.
> 
> For now we'll handle multi-letter extensions only. MISA extensions need
> additional steps that we'll take care later.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/tcg/tcg-cpu.c | 63 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 65d59bc984..5fdec8f04e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -783,6 +783,67 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>       }
>   }
>   
> +static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    RISCVCPUProfile *profile = opaque;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    bool value;
> +    int i, ext_offset;
> +
> +    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) == NULL) {
> +        error_setg(errp, "Profile %s only available for generic CPUs",
> +                   profile->name);
> +        return;
> +    }
> +
> +    if (cpu->env.misa_mxl != MXL_RV64) {
> +        error_setg(errp, "Profile %s only available for 64 bit CPUs",
> +                   profile->name);
> +        return;
> +    }
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    profile->user_set = true;
> +    profile->enabled = value;
> +
> +    if (!profile->enabled) {
> +        return;
> +    }

My idea here was to prevent the following from disabling default rv64
extensions:

-cpu rv64,rva22u64=false

And yeah, this is a niche (I'll refrain from using the word I really wanted) use
of the profile extension, but we should keep in mind that setting a default 'false'
option to 'false' shouldn't make changes in the CPU.

But now if I do this:

-cpu rv64,rva22u64=true (...) -cpu rv64,rva22u64=false

This will enable the profile in rv64 regardless of setting it to 'false' later
on. Which is also a weird mechanic. One way of solving this would be to postpone
profile enable/disable to realize()/finalize(), but then we're back to the problem
we had in v2 where, for multiple profiles, we can't tell the order in which the
user enabled/disabled them in the command line during realize().

Let me think a bit about it.


Daniel

> +
> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
> +        ext_offset = profile->ext_offsets[i];
> +
> +        g_hash_table_insert(multi_ext_user_opts,
> +                            GUINT_TO_POINTER(ext_offset),
> +                            (gpointer)profile->enabled);
> +        isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
> +    }
> +}
> +
> +static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    RISCVCPUProfile *profile = opaque;
> +    bool value = profile->enabled;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void riscv_cpu_add_profiles(Object *cpu_obj)
> +{
> +    for (int i = 0; riscv_profiles[i] != NULL; i++) {
> +        const RISCVCPUProfile *profile = riscv_profiles[i];
> +
> +        object_property_add(cpu_obj, profile->name, "bool",
> +                            cpu_get_profile, cpu_set_profile,
> +                            NULL, (void *)profile);
> +    }
> +}
> +
>   static bool cpu_ext_is_deprecated(const char *ext_name)
>   {
>       return isupper(ext_name[0]);
> @@ -906,6 +967,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
>   
>       riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
>   
> +    riscv_cpu_add_profiles(obj);
> +
>       for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
>           qdev_property_add_static(DEVICE(obj), prop);
>       }

Re: [PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/30/23 10:28, Daniel Henrique Barboza wrote:
> 
> 
> On 10/28/23 05:54, Daniel Henrique Barboza wrote:
>> The TCG emulation implements all the extensions described in the
>> RVA22U64 profile, both mandatory and optional. The mandatory extensions
>> will be enabled via the profile flag. We'll leave the optional
>> extensions to be enabled by hand.
>>
>> Given that this is the first profile we're implementing in TCG we'll
>> need some ground work first:
>>
>> - all profiles declared in riscv_profiles[] will be exposed to users.
>> TCG is the main accelerator we're considering when adding profile
>> support in QEMU, so for now it's safe to assume that all profiles in
>> riscv_profiles[] will be relevant to TCG;
>>
>> - we'll not support user profile settings for vendor CPUs. The flags
>> will still be exposed but users won't be able to change them. The idea
>> is that vendor CPUs in the future can enable profiles internally in
>> their cpu_init() functions, showing to the external world that the CPU
>> supports a certain profile. But users won't be able to enable/disable
>> it;
>>
>> - Setting a profile to 'true' means 'enable all mandatory extensions of
>> this profile, setting it to 'false' means 'do not enable all mandatory
>> extensions for this profile'. This is the same semantics used by RVG.
>> Regular left-to-right option order will determine the resulting CPU
>> configuration, i.e. the following QEMU command line:
>>
>> -cpu rv64,zicbom=false,zifencei=false,rva22u64=true
>>
>> Enables all rva22u64 mandatory extensions, including 'zicbom' and
>> 'zifencei', while this other command line:
>>
>> -cpu rv64,rva22u64=true,zicbom=false,zifencei=false
>>
>> Enables all mandatory rva22u64 extensions, and then disable both zicbom
>> and zifencei.
>>
>> For now we'll handle multi-letter extensions only. MISA extensions need
>> additional steps that we'll take care later.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> ---
>>   target/riscv/tcg/tcg-cpu.c | 63 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>
>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>> index 65d59bc984..5fdec8f04e 100644
>> --- a/target/riscv/tcg/tcg-cpu.c
>> +++ b/target/riscv/tcg/tcg-cpu.c
>> @@ -783,6 +783,67 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>       }
>>   }
>> +static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>> +                            void *opaque, Error **errp)
>> +{
>> +    RISCVCPUProfile *profile = opaque;
>> +    RISCVCPU *cpu = RISCV_CPU(obj);
>> +    bool value;
>> +    int i, ext_offset;
>> +
>> +    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) == NULL) {
>> +        error_setg(errp, "Profile %s only available for generic CPUs",
>> +                   profile->name);
>> +        return;
>> +    }
>> +
>> +    if (cpu->env.misa_mxl != MXL_RV64) {
>> +        error_setg(errp, "Profile %s only available for 64 bit CPUs",
>> +                   profile->name);
>> +        return;
>> +    }
>> +
>> +    if (!visit_type_bool(v, name, &value, errp)) {
>> +        return;
>> +    }
>> +
>> +    profile->user_set = true;
>> +    profile->enabled = value;
>> +
>> +    if (!profile->enabled) {
>> +        return;
>> +    }
> 
> My idea here was to prevent the following from disabling default rv64
> extensions:
> 
> -cpu rv64,rva22u64=false
> 
> And yeah, this is a niche (I'll refrain from using the word I really wanted) use
> of the profile extension, but we should keep in mind that setting a default 'false'
> option to 'false' shouldn't make changes in the CPU.
> 
> But now if I do this:
> 
> -cpu rv64,rva22u64=true (...) -cpu rv64,rva22u64=false
> 
> This will enable the profile in rv64 regardless of setting it to 'false' later
> on. Which is also a weird mechanic. One way of solving this would be to postpone
> profile enable/disable to realize()/finalize(), but then we're back to the problem
> we had in v2 where, for multiple profiles, we can't tell the order in which the
> user enabled/disabled them in the command line during realize().
> 
> Let me think a bit about it.

To be honest, all the debate around this feature is due to rv64 having default
extensions and users doing non-orthodox stuff with the flag that will crop
rv64 defaults, resulting in something that we don't know what this is.

And what aggravates the issue is that, ATM, we don't have any other CPU aside
from rv64 (and max) to use profiles on.

The RVA22U64 profile spec mentions: "RV64I is the mandatory base ISA for RVA22U64".
So we have a base. And we should base profiles around the base, not on a CPU that
has existing default values.

I'll add the 'rv64i' CPU in v7. This will be a bare-bones CPU that will only have
RVI enabled by default. Profile support will be based on top of this CPU, making
enable/disable profiles a trivial matter since we have a fixed minimal base. This
will give users a stable way of using profiles.

As long as we have the rv64i CPU I'll start not caring for what happens with
'-cpu rv64,rva22u64=false' - users are free to use profiles in rv64 if they want,
but the feature is designed around rv64i.

I'll also throw a bone for all CPUs and add 'rva22u64' as an internal flag for
everybody, including vendor CPUs, that will reflect whether the CPU complies with
the profile. query-cpu-model-expansion can expose this flag, allowing the tooling
to have an easier time verifying if a profile is implemented or not.


Thanks,


Daniel


> 
> 
> Daniel
> 
>> +
>> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
>> +        ext_offset = profile->ext_offsets[i];
>> +
>> +        g_hash_table_insert(multi_ext_user_opts,
>> +                            GUINT_TO_POINTER(ext_offset),
>> +                            (gpointer)profile->enabled);
>> +        isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
>> +    }
>> +}
>> +
>> +static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
>> +                            void *opaque, Error **errp)
>> +{
>> +    RISCVCPUProfile *profile = opaque;
>> +    bool value = profile->enabled;
>> +
>> +    visit_type_bool(v, name, &value, errp);
>> +}
>> +
>> +static void riscv_cpu_add_profiles(Object *cpu_obj)
>> +{
>> +    for (int i = 0; riscv_profiles[i] != NULL; i++) {
>> +        const RISCVCPUProfile *profile = riscv_profiles[i];
>> +
>> +        object_property_add(cpu_obj, profile->name, "bool",
>> +                            cpu_get_profile, cpu_set_profile,
>> +                            NULL, (void *)profile);
>> +    }
>> +}
>> +
>>   static bool cpu_ext_is_deprecated(const char *ext_name)
>>   {
>>       return isupper(ext_name[0]);
>> @@ -906,6 +967,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
>>       riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
>> +    riscv_cpu_add_profiles(obj);
>> +
>>       for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
>>           qdev_property_add_static(DEVICE(obj), prop);
>>       }

Re: [PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Alistair Francis]

On Tue, Oct 31, 2023 at 3:19 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/30/23 10:28, Daniel Henrique Barboza wrote:
> >
> >
> > On 10/28/23 05:54, Daniel Henrique Barboza wrote:
> >> The TCG emulation implements all the extensions described in the
> >> RVA22U64 profile, both mandatory and optional. The mandatory extensions
> >> will be enabled via the profile flag. We'll leave the optional
> >> extensions to be enabled by hand.
> >>
> >> Given that this is the first profile we're implementing in TCG we'll
> >> need some ground work first:
> >>
> >> - all profiles declared in riscv_profiles[] will be exposed to users.
> >> TCG is the main accelerator we're considering when adding profile
> >> support in QEMU, so for now it's safe to assume that all profiles in
> >> riscv_profiles[] will be relevant to TCG;
> >>
> >> - we'll not support user profile settings for vendor CPUs. The flags
> >> will still be exposed but users won't be able to change them. The idea
> >> is that vendor CPUs in the future can enable profiles internally in
> >> their cpu_init() functions, showing to the external world that the CPU
> >> supports a certain profile. But users won't be able to enable/disable
> >> it;
> >>
> >> - Setting a profile to 'true' means 'enable all mandatory extensions of
> >> this profile, setting it to 'false' means 'do not enable all mandatory
> >> extensions for this profile'. This is the same semantics used by RVG.
> >> Regular left-to-right option order will determine the resulting CPU
> >> configuration, i.e. the following QEMU command line:
> >>
> >> -cpu rv64,zicbom=false,zifencei=false,rva22u64=true
> >>
> >> Enables all rva22u64 mandatory extensions, including 'zicbom' and
> >> 'zifencei', while this other command line:
> >>
> >> -cpu rv64,rva22u64=true,zicbom=false,zifencei=false
> >>
> >> Enables all mandatory rva22u64 extensions, and then disable both zicbom
> >> and zifencei.
> >>
> >> For now we'll handle multi-letter extensions only. MISA extensions need
> >> additional steps that we'll take care later.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> >> ---
> >>   target/riscv/tcg/tcg-cpu.c | 63 ++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 63 insertions(+)
> >>
> >> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> >> index 65d59bc984..5fdec8f04e 100644
> >> --- a/target/riscv/tcg/tcg-cpu.c
> >> +++ b/target/riscv/tcg/tcg-cpu.c
> >> @@ -783,6 +783,67 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
> >>       }
> >>   }
> >> +static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
> >> +                            void *opaque, Error **errp)
> >> +{
> >> +    RISCVCPUProfile *profile = opaque;
> >> +    RISCVCPU *cpu = RISCV_CPU(obj);
> >> +    bool value;
> >> +    int i, ext_offset;
> >> +
> >> +    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) == NULL) {
> >> +        error_setg(errp, "Profile %s only available for generic CPUs",
> >> +                   profile->name);
> >> +        return;
> >> +    }
> >> +
> >> +    if (cpu->env.misa_mxl != MXL_RV64) {
> >> +        error_setg(errp, "Profile %s only available for 64 bit CPUs",
> >> +                   profile->name);
> >> +        return;
> >> +    }
> >> +
> >> +    if (!visit_type_bool(v, name, &value, errp)) {
> >> +        return;
> >> +    }
> >> +
> >> +    profile->user_set = true;
> >> +    profile->enabled = value;
> >> +
> >> +    if (!profile->enabled) {
> >> +        return;
> >> +    }
> >
> > My idea here was to prevent the following from disabling default rv64
> > extensions:
> >
> > -cpu rv64,rva22u64=false

I think that's the right approach

> >
> > And yeah, this is a niche (I'll refrain from using the word I really wanted) use
> > of the profile extension, but we should keep in mind that setting a default 'false'
> > option to 'false' shouldn't make changes in the CPU.
> >
> > But now if I do this:
> >
> > -cpu rv64,rva22u64=true (...) -cpu rv64,rva22u64=false
> >
> > This will enable the profile in rv64 regardless of setting it to 'false' later
> > on. Which is also a weird mechanic. One way of solving this would be to postpone

Urgh, that is odd.

> > profile enable/disable to realize()/finalize(), but then we're back to the problem
> > we had in v2 where, for multiple profiles, we can't tell the order in which the
> > user enabled/disabled them in the command line during realize().

Are the properties not just set in order automatically? So we end up
with the property being set by the last one?

> >
> > Let me think a bit about it.
>
> To be honest, all the debate around this feature is due to rv64 having default
> extensions and users doing non-orthodox stuff with the flag that will crop
> rv64 defaults, resulting in something that we don't know what this is.

Ok, just to summarise.

The idea is that having a CPU with nothing enabled makes it easy to
then enable features based on what extensions have been enabled. That
way if a profile is false, we can just ignore it. The result is the
features are disabled as that is the default state.

>
> And what aggravates the issue is that, ATM, we don't have any other CPU aside
> from rv64 (and max) to use profiles on.
>
> The RVA22U64 profile spec mentions: "RV64I is the mandatory base ISA for RVA22U64".
> So we have a base. And we should base profiles around the base, not on a CPU that
> has existing default values.

That is only a base for RVA22U64 though. Aren't there embedded
profiles that might use rv64e as a base? What about RV32 as well?

>
> I'll add the 'rv64i' CPU in v7. This will be a bare-bones CPU that will only have
> RVI enabled by default. Profile support will be based on top of this CPU, making
> enable/disable profiles a trivial matter since we have a fixed minimal base. This
> will give users a stable way of using profiles.
>
> As long as we have the rv64i CPU I'll start not caring for what happens with
> '-cpu rv64,rva22u64=false' - users are free to use profiles in rv64 if they want,

What does happen with -cpu rv64,rva22u64=false though?

I feel like this doesn't really address the problem

> but the feature is designed around rv64i.

One other thought it to create a CPU per extension. So the actual CPU
is rva22u64. That way it is easy for users to enable/disable features
on top of the extension and we don't have to worry about the complex
true/false operations for extensions

>
> I'll also throw a bone for all CPUs and add 'rva22u64' as an internal flag for
> everybody, including vendor CPUs, that will reflect whether the CPU complies with
> the profile. query-cpu-model-expansion can expose this flag, allowing the tooling
> to have an easier time verifying if a profile is implemented or not.

Great!

Alistair

>
>
> Thanks,
>
>
> Daniel
>
>
> >
> >
> > Daniel
> >
> >> +
> >> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
> >> +        ext_offset = profile->ext_offsets[i];
> >> +
> >> +        g_hash_table_insert(multi_ext_user_opts,
> >> +                            GUINT_TO_POINTER(ext_offset),
> >> +                            (gpointer)profile->enabled);
> >> +        isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
> >> +    }
> >> +}
> >> +
> >> +static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
> >> +                            void *opaque, Error **errp)
> >> +{
> >> +    RISCVCPUProfile *profile = opaque;
> >> +    bool value = profile->enabled;
> >> +
> >> +    visit_type_bool(v, name, &value, errp);
> >> +}
> >> +
> >> +static void riscv_cpu_add_profiles(Object *cpu_obj)
> >> +{
> >> +    for (int i = 0; riscv_profiles[i] != NULL; i++) {
> >> +        const RISCVCPUProfile *profile = riscv_profiles[i];
> >> +
> >> +        object_property_add(cpu_obj, profile->name, "bool",
> >> +                            cpu_get_profile, cpu_set_profile,
> >> +                            NULL, (void *)profile);
> >> +    }
> >> +}
> >> +
> >>   static bool cpu_ext_is_deprecated(const char *ext_name)
> >>   {
> >>       return isupper(ext_name[0]);
> >> @@ -906,6 +967,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
> >>       riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
> >> +    riscv_cpu_add_profiles(obj);
> >> +
> >>       for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
> >>           qdev_property_add_static(DEVICE(obj), prop);
> >>       }
>

Re: [PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 11/2/23 00:00, Alistair Francis wrote:
> On Tue, Oct 31, 2023 at 3:19 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 10/30/23 10:28, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 10/28/23 05:54, Daniel Henrique Barboza wrote:
>>>> The TCG emulation implements all the extensions described in the
>>>> RVA22U64 profile, both mandatory and optional. The mandatory extensions
>>>> will be enabled via the profile flag. We'll leave the optional
>>>> extensions to be enabled by hand.
>>>>
>>>> Given that this is the first profile we're implementing in TCG we'll
>>>> need some ground work first:
>>>>
>>>> - all profiles declared in riscv_profiles[] will be exposed to users.
>>>> TCG is the main accelerator we're considering when adding profile
>>>> support in QEMU, so for now it's safe to assume that all profiles in
>>>> riscv_profiles[] will be relevant to TCG;
>>>>
>>>> - we'll not support user profile settings for vendor CPUs. The flags
>>>> will still be exposed but users won't be able to change them. The idea
>>>> is that vendor CPUs in the future can enable profiles internally in
>>>> their cpu_init() functions, showing to the external world that the CPU
>>>> supports a certain profile. But users won't be able to enable/disable
>>>> it;
>>>>
>>>> - Setting a profile to 'true' means 'enable all mandatory extensions of
>>>> this profile, setting it to 'false' means 'do not enable all mandatory
>>>> extensions for this profile'. This is the same semantics used by RVG.
>>>> Regular left-to-right option order will determine the resulting CPU
>>>> configuration, i.e. the following QEMU command line:
>>>>
>>>> -cpu rv64,zicbom=false,zifencei=false,rva22u64=true
>>>>
>>>> Enables all rva22u64 mandatory extensions, including 'zicbom' and
>>>> 'zifencei', while this other command line:
>>>>
>>>> -cpu rv64,rva22u64=true,zicbom=false,zifencei=false
>>>>
>>>> Enables all mandatory rva22u64 extensions, and then disable both zicbom
>>>> and zifencei.
>>>>
>>>> For now we'll handle multi-letter extensions only. MISA extensions need
>>>> additional steps that we'll take care later.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>> ---
>>>>    target/riscv/tcg/tcg-cpu.c | 63 ++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 63 insertions(+)
>>>>
>>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>>> index 65d59bc984..5fdec8f04e 100644
>>>> --- a/target/riscv/tcg/tcg-cpu.c
>>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>>> @@ -783,6 +783,67 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>>>>        }
>>>>    }
>>>> +static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>>>> +                            void *opaque, Error **errp)
>>>> +{
>>>> +    RISCVCPUProfile *profile = opaque;
>>>> +    RISCVCPU *cpu = RISCV_CPU(obj);
>>>> +    bool value;
>>>> +    int i, ext_offset;
>>>> +
>>>> +    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) == NULL) {
>>>> +        error_setg(errp, "Profile %s only available for generic CPUs",
>>>> +                   profile->name);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (cpu->env.misa_mxl != MXL_RV64) {
>>>> +        error_setg(errp, "Profile %s only available for 64 bit CPUs",
>>>> +                   profile->name);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (!visit_type_bool(v, name, &value, errp)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    profile->user_set = true;
>>>> +    profile->enabled = value;
>>>> +
>>>> +    if (!profile->enabled) {
>>>> +        return;
>>>> +    }
>>>
>>> My idea here was to prevent the following from disabling default rv64
>>> extensions:
>>>
>>> -cpu rv64,rva22u64=false
> 
> I think that's the right approach
> 
>>>
>>> And yeah, this is a niche (I'll refrain from using the word I really wanted) use
>>> of the profile extension, but we should keep in mind that setting a default 'false'
>>> option to 'false' shouldn't make changes in the CPU.
>>>
>>> But now if I do this:
>>>
>>> -cpu rv64,rva22u64=true (...) -cpu rv64,rva22u64=false
>>>
>>> This will enable the profile in rv64 regardless of setting it to 'false' later
>>> on. Which is also a weird mechanic. One way of solving this would be to postpone
> 
> Urgh, that is odd.
> 
>>> profile enable/disable to realize()/finalize(), but then we're back to the problem
>>> we had in v2 where, for multiple profiles, we can't tell the order in which the
>>> user enabled/disabled them in the command line during realize().
> 
> Are the properties not just set in order automatically? So we end up
> with the property being set by the last one?
> 
>>>
>>> Let me think a bit about it.
>>
>> To be honest, all the debate around this feature is due to rv64 having default
>> extensions and users doing non-orthodox stuff with the flag that will crop
>> rv64 defaults, resulting in something that we don't know what this is.
> 
> Ok, just to summarise.
> 
> The idea is that having a CPU with nothing enabled makes it easy to
> then enable features based on what extensions have been enabled. That
> way if a profile is false, we can just ignore it. The result is the
> features are disabled as that is the default state.
> 
>>
>> And what aggravates the issue is that, ATM, we don't have any other CPU aside
>> from rv64 (and max) to use profiles on.
>>
>> The RVA22U64 profile spec mentions: "RV64I is the mandatory base ISA for RVA22U64".
>> So we have a base. And we should base profiles around the base, not on a CPU that
>> has existing default values.
> 
> That is only a base for RVA22U64 though. Aren't there embedded
> profiles that might use rv64e as a base? What about RV32 as well?

For these profiles we would block them from running into RV64I because it's not
the right base ISA. We should add a RV64E CPU for them.

Same thing with any 32 bit profiles that we might add in the future - rva22u64
isn't compatible with any 32 bits ATM.

> 
>>
>> I'll add the 'rv64i' CPU in v7. This will be a bare-bones CPU that will only have
>> RVI enabled by default. Profile support will be based on top of this CPU, making
>> enable/disable profiles a trivial matter since we have a fixed minimal base. This
>> will give users a stable way of using profiles.
>>
>> As long as we have the rv64i CPU I'll start not caring for what happens with
>> '-cpu rv64,rva22u64=false' - users are free to use profiles in rv64 if they want,
> 
> What does happen with -cpu rv64,rva22u64=false though?
> 
> I feel like this doesn't really address the problem

It doesn't, because it's an user problem. We're wasting too much time trying to
sandbag weird command lines. "-cpu rv64,rva22u64=false" is a nonsense command
line that will give a nonsense CPU.

A failsafe solution I see for it is to forbid profile support for rv64. But this
is too extreme because "-cpu rv64,rva22u64=true" is a valid use, and a way more
common use, that would be cropped out because we want to babysit a minority of
users doing nonsense.

IMO we should let users use profiles with any capable CPU and document properly
what it means to do stuff like "-cpu rv64,rva22u64=false".

And now that I thought more about it, there's no gain for the 'max' CPU to support
profiles (all extensions enabled by default) but -cpu max,rva22u64=false will still
be possible. I guess I'll remove profile support for 'max' CPU.

> 
>> but the feature is designed around rv64i.
> 
> One other thought it to create a CPU per extension. So the actual CPU
> is rva22u64. That way it is easy for users to enable/disable features
> on top of the extension and we don't have to worry about the complex
> true/false operations for extensions

We can make aliases. '-cpu rva22u64' would be an alias for '-cpu rv64i,rva22u64=true'.
Same thing with every other profile we end up adding.


Thanks,


Daniel

> 
>>
>> I'll also throw a bone for all CPUs and add 'rva22u64' as an internal flag for
>> everybody, including vendor CPUs, that will reflect whether the CPU complies with
>> the profile. query-cpu-model-expansion can expose this flag, allowing the tooling
>> to have an easier time verifying if a profile is implemented or not.
> 
> Great!
> 
> Alistair
> 
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>>
>>>
>>> Daniel
>>>
>>>> +
>>>> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
>>>> +        ext_offset = profile->ext_offsets[i];
>>>> +
>>>> +        g_hash_table_insert(multi_ext_user_opts,
>>>> +                            GUINT_TO_POINTER(ext_offset),
>>>> +                            (gpointer)profile->enabled);
>>>> +        isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
>>>> +                            void *opaque, Error **errp)
>>>> +{
>>>> +    RISCVCPUProfile *profile = opaque;
>>>> +    bool value = profile->enabled;
>>>> +
>>>> +    visit_type_bool(v, name, &value, errp);
>>>> +}
>>>> +
>>>> +static void riscv_cpu_add_profiles(Object *cpu_obj)
>>>> +{
>>>> +    for (int i = 0; riscv_profiles[i] != NULL; i++) {
>>>> +        const RISCVCPUProfile *profile = riscv_profiles[i];
>>>> +
>>>> +        object_property_add(cpu_obj, profile->name, "bool",
>>>> +                            cpu_get_profile, cpu_set_profile,
>>>> +                            NULL, (void *)profile);
>>>> +    }
>>>> +}
>>>> +
>>>>    static bool cpu_ext_is_deprecated(const char *ext_name)
>>>>    {
>>>>        return isupper(ext_name[0]);
>>>> @@ -906,6 +967,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
>>>>        riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
>>>> +    riscv_cpu_add_profiles(obj);
>>>> +
>>>>        for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
>>>>            qdev_property_add_static(DEVICE(obj), prop);
>>>>        }
>>