[PATCH 0/6] riscv: RVA22U64 profile support

[PATCH 0/6] riscv: RVA22U64 profile support

[Daniel Henrique Barboza]

Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
("[PATCH 0/2] riscv: add extension properties for all cpus")

Hi,

These patches implements the base profile support for qemu-riscv and the
first profile, RVA22U64. 

As discussed in this thread [1] we're aiming for a flag that enables all
mandatory extensions of a profile. Optional extensions were left behind
and must be enabled by hand if desired. Since this is the first profile
we're adding, we'll need to add the base framework as well. 

The RVA22U64 profile was chosen because qemu-riscv implements all its
extensions, both mandatory and optional. That includes 'zicntr' and
'zihpm', which we support for awhile but aren't adverting to userspace.

Other design decisions made:

- disabling a profile flag does nothing, i.e. we won't mass disable
  mandatory extensions of the rva22U64 profile if the user sets
  rva22u64=false;

- profile support for vendor CPUs consists into checking if the CPU
  happens to have the mandatory extensions required for it. In case it
  doesn't we'll error out. This is done to follow the same prerogative
  we always had of not allowing extensions being enabled for vendor
  CPUs;

- the KVM driver doesn't support profiles. In theory we could apply the
  same logic as for the vendor CPUs, but KVM has a long way to go before
  that becomes a factor. We'll revisit this decision when KVM is able to
  support at least one profile.

Patch 5 ("enable profile support for vendor CPUs") needs the following
series to be applied beforehand:

"[PATCH 0/2] riscv: add extension properties for all cpus"

Otherwise we won't be able to add the profile flag to vendor CPUs.

[1] https://lore.kernel.org/qemu-riscv/35a847a1-2720-14ab-61b0-c72d77d5f43b@ventanamicro.com/

Daniel Henrique Barboza (6):
  target/riscv/cpu.c: add zicntr extension flag
  target/riscv/cpu.c: add zihpm extension flag
  target/riscv: add rva22u64 profile definition
  target/riscv/tcg: implement rva22u64 profile
  target/riscv/tcg-cpu.c: enable profile support for vendor CPUs
  target/riscv/kvm: add 'rva22u64' flag as unavailable

 target/riscv/cpu.c         | 25 ++++++++++
 target/riscv/cpu.h         | 10 ++++
 target/riscv/cpu_cfg.h     |  2 +
 target/riscv/kvm/kvm-cpu.c |  5 +-
 target/riscv/tcg/tcg-cpu.c | 98 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 139 insertions(+), 1 deletion(-)

-- 
2.41.0

[PATCH 1/6] target/riscv/cpu.c: add zicntr extension flag

[Daniel Henrique Barboza]

zicntr is the Base Counters and Timers extension described in chapter 12
of the unprivileged spec. It describes support for RDCYCLE, RDTIME and
RDINSTRET.

QEMU already implements it way before it was a discrete extension.
zicntr is part of the RVA22 profile, so let's add it to QEMU to make the
future profile implementation flag complete.

Given than it represents an already existing feature, default it to
'true'. Change the realize() time validation to disable it in case its
dependency (icsr) isn't present.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 521bb88538..8783a415b1 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -79,6 +79,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_icbom),
     ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_icboz),
     ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
+    ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_icntr),
     ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_icsr),
     ISA_EXT_DATA_ENTRY(zifencei, PRIV_VERSION_1_10_0, ext_ifencei),
     ISA_EXT_DATA_ENTRY(zihintntl, PRIV_VERSION_1_10_0, ext_zihintntl),
@@ -1265,6 +1266,12 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("svnapot", ext_svnapot, false),
     MULTI_EXT_CFG_BOOL("svpbmt", ext_svpbmt, false),
 
+    /*
+     * Always default true - we'll disable it during
+     * realize() if needed.
+     */
+    MULTI_EXT_CFG_BOOL("zicntr", ext_icntr, true),
+
     MULTI_EXT_CFG_BOOL("zba", ext_zba, true),
     MULTI_EXT_CFG_BOOL("zbb", ext_zbb, true),
     MULTI_EXT_CFG_BOOL("zbc", ext_zbc, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 0e6a0f245c..671b8c7cb8 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -62,6 +62,7 @@ struct RISCVCPUConfig {
     bool ext_zksh;
     bool ext_zkt;
     bool ext_ifencei;
+    bool ext_icntr;
     bool ext_icsr;
     bool ext_icbom;
     bool ext_icboz;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index a90ee63b06..ce0fde0f5d 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -542,6 +542,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zksh), true);
     }
 
+    if (cpu->cfg.ext_icntr && !cpu->cfg.ext_icsr) {
+        cpu->cfg.ext_icntr = false;
+    }
+
     /*
      * Disable isa extensions based on priv spec after we
      * validated and set everything we need.
-- 
2.41.0

[PATCH 2/6] target/riscv/cpu.c: add zihpm extension flag

[Daniel Henrique Barboza]

zihpm is the Hardware Performance Counters extension described in
chapter 12 of the unprivileged spec. It describes support for 29
unprivileged performance counters, hpmcounter3-hpmcounter21.

As with zicntr, QEMU already implements zihpm before it was even an
extension. zihpm is also part of the RVA22 profile, so add it to QEMU
to complement the future future profile implementation.

Default it to 'true' since it was always present in the code. Change the
realize() time validation to disable it in case 'icsr' isn't present and
if there's no hardware counters (cpu->cfg.pmu_num is zero).

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8783a415b1..b3befccf89 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -84,6 +84,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zifencei, PRIV_VERSION_1_10_0, ext_ifencei),
     ISA_EXT_DATA_ENTRY(zihintntl, PRIV_VERSION_1_10_0, ext_zihintntl),
     ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
+    ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_ihpm),
     ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
     ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
     ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
@@ -1267,10 +1268,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("svpbmt", ext_svpbmt, false),
 
     /*
-     * Always default true - we'll disable it during
+     * Always default true - we'll disable them during
      * realize() if needed.
      */
     MULTI_EXT_CFG_BOOL("zicntr", ext_icntr, true),
+    MULTI_EXT_CFG_BOOL("zihpm", ext_ihpm, true),
 
     MULTI_EXT_CFG_BOOL("zba", ext_zba, true),
     MULTI_EXT_CFG_BOOL("zbb", ext_zbb, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 671b8c7cb8..cf228546da 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -66,6 +66,7 @@ struct RISCVCPUConfig {
     bool ext_icsr;
     bool ext_icbom;
     bool ext_icboz;
+    bool ext_ihpm;
     bool ext_zicond;
     bool ext_zihintntl;
     bool ext_zihintpause;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index ce0fde0f5d..11e34782b9 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -546,6 +546,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_icntr = false;
     }
 
+    if (cpu->cfg.ext_ihpm && (!cpu->cfg.ext_icsr || cpu->cfg.pmu_num == 0)) {
+        cpu->cfg.ext_ihpm = false;
+    }
+
     /*
      * Disable isa extensions based on priv spec after we
      * validated and set everything we need.
-- 
2.41.0

[PATCH 3/6] 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. The exception
is Zicbop (Cache-Block Prefetch Operations) that is not available since
QEMU RISC-V does not implement a cache model. For this same reason all
the so called 'synthetic extensions' described in the profile that are
cache related are ignored (Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa,
Zicclsm).

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>
---
 target/riscv/cpu.c | 16 ++++++++++++++++
 target/riscv/cpu.h | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b3befccf89..c83807f179 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1376,6 +1376,22 @@ Property riscv_cpu_options[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/* Optional extensions left out: RVV, zfh, zkn, zks */
+const RISCVCPUProfile RVA22U64 = {
+    .name = "rva22u64",
+    .misa_ext = RVM | RVA | RVF | RVD | RVC,
+    .ext_offsets = {
+        CPU_CFG_OFFSET(ext_icsr), 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_icntr),
+        CPU_CFG_OFFSET(ext_ihpm), CPU_CFG_OFFSET(ext_icbom),
+        CPU_CFG_OFFSET(ext_icboz),
+
+        RISCV_PROFILE_EXT_LIST_END
+    }
+};
+
 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 3f11e69223..615946b919 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -66,6 +66,16 @@ 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;
+    const int32_t ext_offsets[];
+} RISCVCPUProfile;
+
+#define RISCV_PROFILE_EXT_LIST_END -1
+
+extern const RISCVCPUProfile RVA22U64;
+
 /* Privileged specification version */
 enum {
     PRIV_VERSION_1_10_0 = 0,
-- 
2.41.0

[PATCH 4/6] target/riscv/tcg: implement rva22u64 profile

[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 wiring first. 'cpu_set_profile', our set() callback for the
profile user flag that we'll expose, will do the heavy lifting. We'll
assign misa_ext and misa_ext_mask based on the profile .misa_ext, and
enable all extensions from .ext_offsets[].

We'll also update the user choice hash 'multi_ext_user_opts' for each
extension. The idea is to reflect that setting a profile is the same as
setting all extensions of the profile in the command line. This will
prevent us from mishandling those by accident during realize() time, in
particular in validate_set_extensions(), when we might enable/disable
extensions based on certain criterias.

After cpu_set_profile() is figured out then it's a matter of exposing
the user flag for the profile using the profile name (in this case,
'rva22u64') during riscv_cpu_add_user_properties().

We will expose the profile option for vendor CPUs in the next patch
since it requires special handling. Expose it to generic CPUs only for
now.

Here's an example with the 'rv64' CPU:

 $ qemu-system-riscv64 -M virt -cpu rv64,rva22u64=true (...)

 # cat /proc/cpuinfo
processor	: 0
hart		: 0
isa		: rv64imafdch_zicbom_zicboz_zicntr_zicsr_zifencei_zihintntl_zihintpause_zihpm_zawrs_zfa_zfhmin_zca_zcd_zba_zbb_zbc_zbs_zkt_sstc_svadu

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 11e34782b9..03435521c9 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -740,6 +740,57 @@ 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)
+{
+    const RISCVCPUProfile *profile = opaque;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
+    int i = 0;
+    bool value;
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    /* We won't disable extensions if the user disables the profile */
+    if (!value) {
+        return;
+    }
+
+    env->misa_ext |= profile->misa_ext;
+    env->misa_ext_mask |= profile->misa_ext;
+
+    for (i = 0;; i++) {
+        int ext_offset = profile->ext_offsets[i];
+
+        if (ext_offset == RISCV_PROFILE_EXT_LIST_END) {
+            break;
+        }
+
+        isa_ext_update_enabled(cpu, ext_offset, true);
+        g_hash_table_insert(multi_ext_user_opts,
+                            GUINT_TO_POINTER(ext_offset),
+                            (gpointer)true);
+    }
+}
+
+static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    bool value;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void riscv_cpu_add_profile_prop(Object *cpu_obj,
+                                       const RISCVCPUProfile *profile)
+{
+    object_property_add(cpu_obj, profile->name, "bool",
+                        cpu_get_profile, cpu_set_profile,
+                        NULL, (void *)profile);
+}
+
 static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
                                   void *opaque, Error **errp)
 {
@@ -834,6 +885,10 @@ static void riscv_cpu_add_user_properties(Object *obj)
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_vendor_exts);
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_experimental_exts);
 
+    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) != NULL) {
+        riscv_cpu_add_profile_prop(obj, &RVA22U64);
+    }
+
     for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
         qdev_property_add_static(DEVICE(obj), prop);
     }
-- 
2.41.0

[PATCH 5/6] target/riscv/tcg-cpu.c: enable profile support for vendor CPUs

[Daniel Henrique Barboza]

Vendor CPUs can implement any profile they want as long as all required
extensions are being set in their respective cpu_init().

We do not enable extensions for vendor CPUs and that will still be true
with profile support. The idea then is to enable the profile option for
vendor CPUs and  let users try to enable it. In case the vendor CPU
do not implement all mandatory extensions of the profile, error out.
Proceed as usual otherwise.

Here's an example of what happens if we try to enable the rva22u64
profile with the veyron-v1 CPU:

./qemu-system-riscv64 -M virt -cpu veyron-v1,rva22u64=true

qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.rva22u64=true:
  Setting profile 'rva22u64' failed: CPU veyron-v1 does not support extension 'Zihintpause'

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 03435521c9..c8f688292e 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -99,6 +99,31 @@ static const struct TCGCPUOps riscv_tcg_ops = {
 #endif /* !CONFIG_USER_ONLY */
 };
 
+static const char *cpu_get_multi_ext_cfg_name(uint32_t ext_offset)
+{
+    const RISCVCPUMultiExtConfig *prop;
+
+    for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
+        if (prop->offset == ext_offset) {
+            return prop->name;
+        }
+    }
+
+    for (prop = riscv_cpu_experimental_exts; prop && prop->name; prop++) {
+        if (prop->offset == ext_offset) {
+            return prop->name;
+        }
+    }
+
+    for (prop = riscv_cpu_vendor_exts; prop && prop->name; prop++) {
+        if (prop->offset == ext_offset) {
+            return prop->name;
+        }
+    }
+
+    return NULL;
+}
+
 static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
 {
     const RISCVIsaExtData *edata;
@@ -747,7 +772,7 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
     RISCVCPU *cpu = RISCV_CPU(obj);
     CPURISCVState *env = &cpu->env;
     int i = 0;
-    bool value;
+    bool value, generic_cpu;
 
     if (!visit_type_bool(v, name, &value, errp)) {
         return;
@@ -758,16 +783,28 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
         return;
     }
 
+    generic_cpu = object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
+
     env->misa_ext |= profile->misa_ext;
     env->misa_ext_mask |= profile->misa_ext;
 
     for (i = 0;; i++) {
         int ext_offset = profile->ext_offsets[i];
+        bool prev_val = isa_ext_is_enabled(cpu, ext_offset);
 
         if (ext_offset == RISCV_PROFILE_EXT_LIST_END) {
             break;
         }
 
+        if (!prev_val && !generic_cpu) {
+            const char *ext_name = cpu_get_multi_ext_cfg_name(ext_offset);
+            const char *cpu_name = riscv_cpu_get_name(cpu);
+
+            error_setg(errp, "Setting profile '%s' failed: CPU %s does not "
+                       "support extension '%s'", name, cpu_name, ext_name);
+            return;
+        }
+
         isa_ext_update_enabled(cpu, ext_offset, true);
         g_hash_table_insert(multi_ext_user_opts,
                             GUINT_TO_POINTER(ext_offset),
@@ -885,9 +922,7 @@ static void riscv_cpu_add_user_properties(Object *obj)
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_vendor_exts);
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_experimental_exts);
 
-    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) != NULL) {
-        riscv_cpu_add_profile_prop(obj, &RVA22U64);
-    }
+    riscv_cpu_add_profile_prop(obj, &RVA22U64);
 
     for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
         qdev_property_add_static(DEVICE(obj), prop);
-- 
2.41.0

[PATCH 6/6] 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>
---
 target/riscv/kvm/kvm-cpu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index c6615cb807..258e360422 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -358,7 +358,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);
     }
 }
@@ -438,6 +438,9 @@ 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 this profile */
+   riscv_cpu_add_kvm_unavail_prop(cpu_obj, RVA22U64.name);
 }
 
 static int kvm_riscv_get_regs_core(CPUState *cs)
-- 
2.41.0

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Andrea Bolognani]

On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
> ("[PATCH 0/2] riscv: add extension properties for all cpus")
>
> Hi,
>
> These patches implements the base profile support for qemu-riscv and the
> first profile, RVA22U64.
>
> As discussed in this thread [1] we're aiming for a flag that enables all
> mandatory extensions of a profile. Optional extensions were left behind
> and must be enabled by hand if desired. Since this is the first profile
> we're adding, we'll need to add the base framework as well.
>
> The RVA22U64 profile was chosen because qemu-riscv implements all its
> extensions, both mandatory and optional. That includes 'zicntr' and
> 'zihpm', which we support for awhile but aren't adverting to userspace.
>
> Other design decisions made:
>
> - disabling a profile flag does nothing, i.e. we won't mass disable
>   mandatory extensions of the rva22U64 profile if the user sets
>   rva22u64=false;

Wouldn't it make more sense to error out when this is requested?

Silently ignoring an explicit request made by the user is pretty much
never a good idea in my experience.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel P. Berrangé]

On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
> ("[PATCH 0/2] riscv: add extension properties for all cpus")
> 
> Hi,
> 
> These patches implements the base profile support for qemu-riscv and the
> first profile, RVA22U64. 
> 
> As discussed in this thread [1] we're aiming for a flag that enables all
> mandatory extensions of a profile. Optional extensions were left behind
> and must be enabled by hand if desired. Since this is the first profile
> we're adding, we'll need to add the base framework as well. 
> 
> The RVA22U64 profile was chosen because qemu-riscv implements all its
> extensions, both mandatory and optional. That includes 'zicntr' and
> 'zihpm', which we support for awhile but aren't adverting to userspace.
> 
> Other design decisions made:
> 
> - disabling a profile flag does nothing, i.e. we won't mass disable
>   mandatory extensions of the rva22U64 profile if the user sets
>   rva22u64=false;

Why shouldn't this be allowed ?

IIUC, a profile is syntactic sugar for a group of features. If
we can disable individual features explicitly, why should we
not allow use of the profile as sugar to disable them en-mass ?


BTW, I would caution that the semantics of mixing groups of
features, with individual features in -cpu is likely to be
ill defined, as you cannot rely on left-to-right processing
of the -cpu arguments.

IOW, if you say  -cpu $foo,$group=on,$feature=off

you might expect this to result in '$feature' being disabled
if it were implied by $group. This is not guaranteed as the
QDict processing of options could result in us effectively
processing   -cpu $foo,$feature=off,$group=on

This brokeness with CPU feature groups and their interaction
with CPU feature flags already impacts s390x:

  https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg00981.html

There is a possible way to fix it by declaring an ordering
such that all groups will be processed fully, before any
individual features are processed, and declaring that group
or feature names must not be repeated. This avoids a reliance
on left-to-right ordering:

  https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg01005.html

that is still likely broken, however, if multiple different
groups are listed, and there is overlap in their feature
sets.

TL;DR: feature groups are pretty error prone if more than
one is listed by the user, or they're combined with individual
features.

> 
> - profile support for vendor CPUs consists into checking if the CPU
>   happens to have the mandatory extensions required for it. In case it
>   doesn't we'll error out. This is done to follow the same prerogative
>   we always had of not allowing extensions being enabled for vendor
>   CPUs;

Why shouldn't this be allowed ?

> - the KVM driver doesn't support profiles. In theory we could apply the
>   same logic as for the vendor CPUs, but KVM has a long way to go before
>   that becomes a factor. We'll revisit this decision when KVM is able to
>   support at least one profile.
> 
> Patch 5 ("enable profile support for vendor CPUs") needs the following
> series to be applied beforehand:
> 
> "[PATCH 0/2] riscv: add extension properties for all cpus"
> 
> Otherwise we won't be able to add the profile flag to vendor CPUs.
> 
> [1] https://lore.kernel.org/qemu-riscv/35a847a1-2720-14ab-61b0-c72d77d5f43b@ventanamicro.com/
> 
> Daniel Henrique Barboza (6):
>   target/riscv/cpu.c: add zicntr extension flag
>   target/riscv/cpu.c: add zihpm extension flag
>   target/riscv: add rva22u64 profile definition
>   target/riscv/tcg: implement rva22u64 profile
>   target/riscv/tcg-cpu.c: enable profile support for vendor CPUs
>   target/riscv/kvm: add 'rva22u64' flag as unavailable
> 
>  target/riscv/cpu.c         | 25 ++++++++++
>  target/riscv/cpu.h         | 10 ++++
>  target/riscv/cpu_cfg.h     |  2 +
>  target/riscv/kvm/kvm-cpu.c |  5 +-
>  target/riscv/tcg/tcg-cpu.c | 98 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 139 insertions(+), 1 deletion(-)
> 
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel Henrique Barboza]

On 9/29/23 07:46, Daniel P. Berrangé wrote:
> On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
>> Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
>> ("[PATCH 0/2] riscv: add extension properties for all cpus")
>>
>> Hi,
>>
>> These patches implements the base profile support for qemu-riscv and the
>> first profile, RVA22U64.
>>
>> As discussed in this thread [1] we're aiming for a flag that enables all
>> mandatory extensions of a profile. Optional extensions were left behind
>> and must be enabled by hand if desired. Since this is the first profile
>> we're adding, we'll need to add the base framework as well.
>>
>> The RVA22U64 profile was chosen because qemu-riscv implements all its
>> extensions, both mandatory and optional. That includes 'zicntr' and
>> 'zihpm', which we support for awhile but aren't adverting to userspace.
>>
>> Other design decisions made:
>>
>> - disabling a profile flag does nothing, i.e. we won't mass disable
>>    mandatory extensions of the rva22U64 profile if the user sets
>>    rva22u64=false;
> 
> Why shouldn't this be allowed ?
> 
> IIUC, a profile is syntactic sugar for a group of features. If
> we can disable individual features explicitly, why should we
> not allow use of the profile as sugar to disable them en-mass ?

In theory there's no harm in allowing mass disabling of extensions but, given
it's a whole profile, we would end up disabling most/all CPU extensions and
the guest would do nothing.

There is a thread in the ML:

https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@mail.gmail.com/

Where we discussed the possibility of having a minimal CPU extension set. We didn't
reach a consensus because the definition of "minimal CPU extension set" vary between
OSes (Linux requires IMAFD, FreeBSD might require something differ).

Assuming we reach a consensus on what a minimal set is, we could allow disabling mass
extensions via probile but keeping this minimal set, for example. At very least we
shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I is
the minimum set that I would assume for now.


> 
> 
> BTW, I would caution that the semantics of mixing groups of
> features, with individual features in -cpu is likely to be
> ill defined, as you cannot rely on left-to-right processing
> of the -cpu arguments.
> 
> IOW, if you say  -cpu $foo,$group=on,$feature=off
> 
> you might expect this to result in '$feature' being disabled
> if it were implied by $group. This is not guaranteed as the
> QDict processing of options could result in us effectively
> processing   -cpu $foo,$feature=off,$group=on
> 
> This brokeness with CPU feature groups and their interaction
> with CPU feature flags already impacts s390x:
> 
>    https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg00981.html
> 
> There is a possible way to fix it by declaring an ordering
> such that all groups will be processed fully, before any
> individual features are processed, and declaring that group
> or feature names must not be repeated. This avoids a reliance
> on left-to-right ordering:
> 
>    https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg01005.html
> 
> that is still likely broken, however, if multiple different
> groups are listed, and there is overlap in their feature
> sets.

Just read the discussion. I agree with restricting the command line flexibility
to make things more consistent, but IIRC libvirt (and others) uses a lot of
command line appending and restricting these use cases now will break stuff
up. I recall pc64 domains using <qemu:commandline> to add extra cpu/machine
flags to the domain back in the day. Surely we can claim (I guess) that command
line pass-through is unstable and users shouldn't expect extended support for
it, but we all know that users will be less than pleased when their domains
start breaking because QEMU restricted the command line.

As for the current RISC-V case, one thing we can do is to postpone feature group
processing to realize() time, since in that stage we're already done with the
command line processing. We can make a sanity check between the feature group
flags and error out if there's something off. That would make things a little
less fragile that what they are, albeit I'm sure that there will be some cases
that will be left uncovered.

> 
> TL;DR: feature groups are pretty error prone if more than
> one is listed by the user, or they're combined with individual
> features.
> 
>>
>> - profile support for vendor CPUs consists into checking if the CPU
>>    happens to have the mandatory extensions required for it. In case it
>>    doesn't we'll error out. This is done to follow the same prerogative
>>    we always had of not allowing extensions being enabled for vendor
>>    CPUs;
> 
> Why shouldn't this be allowed ?

There's no technical reason to not allow it. The reason it's forbid is to be
closer to what the real hardware would do. E.g. the real hardware doesn't allow
users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
a privileged spec restriction as well, so if a CPU is running in an older spec
it can't enable extensions that were added later.

If vendor CPUs from x86 and others behave in a different way with feature enablement
I'd like to hear about it. I can say that what RISC-V is doing in this regard is
not that far from what PowerPC does.
  


Thanks,

Daniel

> 
>> - the KVM driver doesn't support profiles. In theory we could apply the
>>    same logic as for the vendor CPUs, but KVM has a long way to go before
>>    that becomes a factor. We'll revisit this decision when KVM is able to
>>    support at least one profile.
>>
>> Patch 5 ("enable profile support for vendor CPUs") needs the following
>> series to be applied beforehand:
>>
>> "[PATCH 0/2] riscv: add extension properties for all cpus"
>>
>> Otherwise we won't be able to add the profile flag to vendor CPUs.
>>
>> [1] https://lore.kernel.org/qemu-riscv/35a847a1-2720-14ab-61b0-c72d77d5f43b@ventanamicro.com/
>>
>> Daniel Henrique Barboza (6):
>>    target/riscv/cpu.c: add zicntr extension flag
>>    target/riscv/cpu.c: add zihpm extension flag
>>    target/riscv: add rva22u64 profile definition
>>    target/riscv/tcg: implement rva22u64 profile
>>    target/riscv/tcg-cpu.c: enable profile support for vendor CPUs
>>    target/riscv/kvm: add 'rva22u64' flag as unavailable
>>
>>   target/riscv/cpu.c         | 25 ++++++++++
>>   target/riscv/cpu.h         | 10 ++++
>>   target/riscv/cpu_cfg.h     |  2 +
>>   target/riscv/kvm/kvm-cpu.c |  5 +-
>>   target/riscv/tcg/tcg-cpu.c | 98 ++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 139 insertions(+), 1 deletion(-)
>>
>> -- 
>> 2.41.0
>>
>>
> 
> With regards,
> Daniel

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel P. Berrangé]

On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/29/23 07:46, Daniel P. Berrangé wrote:
> > On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> > > Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
> > > ("[PATCH 0/2] riscv: add extension properties for all cpus")
> > > 
> > > Hi,
> > > 
> > > These patches implements the base profile support for qemu-riscv and the
> > > first profile, RVA22U64.
> > > 
> > > As discussed in this thread [1] we're aiming for a flag that enables all
> > > mandatory extensions of a profile. Optional extensions were left behind
> > > and must be enabled by hand if desired. Since this is the first profile
> > > we're adding, we'll need to add the base framework as well.
> > > 
> > > The RVA22U64 profile was chosen because qemu-riscv implements all its
> > > extensions, both mandatory and optional. That includes 'zicntr' and
> > > 'zihpm', which we support for awhile but aren't adverting to userspace.
> > > 
> > > Other design decisions made:
> > > 
> > > - disabling a profile flag does nothing, i.e. we won't mass disable
> > >    mandatory extensions of the rva22U64 profile if the user sets
> > >    rva22u64=false;
> > 
> > Why shouldn't this be allowed ?
> > 
> > IIUC, a profile is syntactic sugar for a group of features. If
> > we can disable individual features explicitly, why should we
> > not allow use of the profile as sugar to disable them en-mass ?
> 
> In theory there's no harm in allowing mass disabling of extensions but, given
> it's a whole profile, we would end up disabling most/all CPU extensions and
> the guest would do nothing.

True, that is just user error though.  They could disable a profile
and then manually re-enable individual features, and thus get a
working system.

> There is a thread in the ML:
> 
> https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@mail.gmail.com/
> 
> Where we discussed the possibility of having a minimal CPU extension set. We didn't
> reach a consensus because the definition of "minimal CPU extension set" vary between
> OSes (Linux requires IMAFD, FreeBSD might require something differ).
> 
> Assuming we reach a consensus on what a minimal set is, we could allow disabling mass
> extensions via probile but keeping this minimal set, for example. At very least we
> shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I is
> the minimum set that I would assume for now.

I'd probably just call that user error too. 

> > 
> > TL;DR: feature groups are pretty error prone if more than
> > one is listed by the user, or they're combined with individual
> > features.
> > 
> > > 
> > > - profile support for vendor CPUs consists into checking if the CPU
> > >    happens to have the mandatory extensions required for it. In case it
> > >    doesn't we'll error out. This is done to follow the same prerogative
> > >    we always had of not allowing extensions being enabled for vendor
> > >    CPUs;
> > 
> > Why shouldn't this be allowed ?
> 
> There's no technical reason to not allow it. The reason it's forbid is to be
> closer to what the real hardware would do. E.g. the real hardware doesn't allow
> users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
> a privileged spec restriction as well, so if a CPU is running in an older spec
> it can't enable extensions that were added later.

Real hardware is constrained in not being able to invent arbitrary
new features on chip. Virtual machines  are not constrained, so
I don't think the inability of hardware todo this, is an especially
strong reason to limit software emulation.

What I don't like about this, is that (IIUC) the '$profile=on' option
now has different semantics depending on what CPU it is used with.

ie  using it with a vendor CPU,   $profile=on  becomes an assertion
that the vendor CPU contains all the features needed to satisfy
$profile. It won't enable/disable anything, just check it is present.

With a non-vendor CPU, using $profile=on becomes a mechanism to force
enable all the features needed to satisfy $profile, there is no
mechanism to just check for presence.

Having two different semantics for the same syntax is generally considered
bad design practice.

This points towards supporting a tri-state, not boolean. $profile=check
for validation only, and $profile=on for force enablement.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel Henrique Barboza]

On 9/29/23 08:55, Daniel P. Berrangé wrote:
> On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 9/29/23 07:46, Daniel P. Berrangé wrote:
>>> On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
>>>> Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
>>>> ("[PATCH 0/2] riscv: add extension properties for all cpus")
>>>>
>>>> Hi,
>>>>
>>>> These patches implements the base profile support for qemu-riscv and the
>>>> first profile, RVA22U64.
>>>>
>>>> As discussed in this thread [1] we're aiming for a flag that enables all
>>>> mandatory extensions of a profile. Optional extensions were left behind
>>>> and must be enabled by hand if desired. Since this is the first profile
>>>> we're adding, we'll need to add the base framework as well.
>>>>
>>>> The RVA22U64 profile was chosen because qemu-riscv implements all its
>>>> extensions, both mandatory and optional. That includes 'zicntr' and
>>>> 'zihpm', which we support for awhile but aren't adverting to userspace.
>>>>
>>>> Other design decisions made:
>>>>
>>>> - disabling a profile flag does nothing, i.e. we won't mass disable
>>>>     mandatory extensions of the rva22U64 profile if the user sets
>>>>     rva22u64=false;
>>>
>>> Why shouldn't this be allowed ?
>>>
>>> IIUC, a profile is syntactic sugar for a group of features. If
>>> we can disable individual features explicitly, why should we
>>> not allow use of the profile as sugar to disable them en-mass ?
>>
>> In theory there's no harm in allowing mass disabling of extensions but, given
>> it's a whole profile, we would end up disabling most/all CPU extensions and
>> the guest would do nothing.
> 
> True, that is just user error though.  They could disable a profile
> and then manually re-enable individual features, and thus get a
> working system.
> 
>> There is a thread in the ML:
>>
>> https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@mail.gmail.com/
>>
>> Where we discussed the possibility of having a minimal CPU extension set. We didn't
>> reach a consensus because the definition of "minimal CPU extension set" vary between
>> OSes (Linux requires IMAFD, FreeBSD might require something differ).
>>
>> Assuming we reach a consensus on what a minimal set is, we could allow disabling mass
>> extensions via probile but keeping this minimal set, for example. At very least we
>> shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I is
>> the minimum set that I would assume for now.
> 
> I'd probably just call that user error too.
> 
>>>
>>> TL;DR: feature groups are pretty error prone if more than
>>> one is listed by the user, or they're combined with individual
>>> features.
>>>
>>>>
>>>> - profile support for vendor CPUs consists into checking if the CPU
>>>>     happens to have the mandatory extensions required for it. In case it
>>>>     doesn't we'll error out. This is done to follow the same prerogative
>>>>     we always had of not allowing extensions being enabled for vendor
>>>>     CPUs;
>>>
>>> Why shouldn't this be allowed ?
>>
>> There's no technical reason to not allow it. The reason it's forbid is to be
>> closer to what the real hardware would do. E.g. the real hardware doesn't allow
>> users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
>> a privileged spec restriction as well, so if a CPU is running in an older spec
>> it can't enable extensions that were added later.
> 
> Real hardware is constrained in not being able to invent arbitrary
> new features on chip. Virtual machines  are not constrained, so
> I don't think the inability of hardware todo this, is an especially
> strong reason to limit software emulation.
> 
> What I don't like about this, is that (IIUC) the '$profile=on' option
> now has different semantics depending on what CPU it is used with.
> 
> ie  using it with a vendor CPU,   $profile=on  becomes an assertion
> that the vendor CPU contains all the features needed to satisfy
> $profile. It won't enable/disable anything, just check it is present.
> 
> With a non-vendor CPU, using $profile=on becomes a mechanism to force
> enable all the features needed to satisfy $profile, there is no
> mechanism to just check for presence.
> 
> Having two different semantics for the same syntax is generally considered
> bad design practice.
> 
> This points towards supporting a tri-state, not boolean. $profile=check
> for validation only, and $profile=on for force enablement.

This would leave us with:

- $profile=off => disable all extensions. Let users hit themselves in the foot if they
don't enable any other extensions. Note that disabling a profile and enabling extensions
on top of it is very sensitive to left-to-right ordering, so it would be good to have
a way to enforce this ordering somehow (feature groups always first);

- $profile=on => only valid for generic CPUs;

- $profile=check -> valid for all CPUs, would only check if the CPU implements the profile.


I think this is fine. Drew, care to weight in?


Thanks,

Daniel

> 
> 
> With regards,
> Daniel

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel P. Berrangé]

On Fri, Sep 29, 2023 at 09:49:47AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/29/23 08:55, Daniel P. Berrangé wrote:
> > On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 9/29/23 07:46, Daniel P. Berrangé wrote:
> > > > On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> > > > > Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
> > > > > ("[PATCH 0/2] riscv: add extension properties for all cpus")
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > These patches implements the base profile support for qemu-riscv and the
> > > > > first profile, RVA22U64.
> > > > > 
> > > > > As discussed in this thread [1] we're aiming for a flag that enables all
> > > > > mandatory extensions of a profile. Optional extensions were left behind
> > > > > and must be enabled by hand if desired. Since this is the first profile
> > > > > we're adding, we'll need to add the base framework as well.
> > > > > 
> > > > > The RVA22U64 profile was chosen because qemu-riscv implements all its
> > > > > extensions, both mandatory and optional. That includes 'zicntr' and
> > > > > 'zihpm', which we support for awhile but aren't adverting to userspace.
> > > > > 
> > > > > Other design decisions made:
> > > > > 
> > > > > - disabling a profile flag does nothing, i.e. we won't mass disable
> > > > >     mandatory extensions of the rva22U64 profile if the user sets
> > > > >     rva22u64=false;
> > > > 
> > > > Why shouldn't this be allowed ?
> > > > 
> > > > IIUC, a profile is syntactic sugar for a group of features. If
> > > > we can disable individual features explicitly, why should we
> > > > not allow use of the profile as sugar to disable them en-mass ?
> > > 
> > > In theory there's no harm in allowing mass disabling of extensions but, given
> > > it's a whole profile, we would end up disabling most/all CPU extensions and
> > > the guest would do nothing.
> > 
> > True, that is just user error though.  They could disable a profile
> > and then manually re-enable individual features, and thus get a
> > working system.
> > 
> > > There is a thread in the ML:
> > > 
> > > https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@mail.gmail.com/
> > > 
> > > Where we discussed the possibility of having a minimal CPU extension set. We didn't
> > > reach a consensus because the definition of "minimal CPU extension set" vary between
> > > OSes (Linux requires IMAFD, FreeBSD might require something differ).
> > > 
> > > Assuming we reach a consensus on what a minimal set is, we could allow disabling mass
> > > extensions via probile but keeping this minimal set, for example. At very least we
> > > shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I is
> > > the minimum set that I would assume for now.
> > 
> > I'd probably just call that user error too.
> > 
> > > > 
> > > > TL;DR: feature groups are pretty error prone if more than
> > > > one is listed by the user, or they're combined with individual
> > > > features.
> > > > 
> > > > > 
> > > > > - profile support for vendor CPUs consists into checking if the CPU
> > > > >     happens to have the mandatory extensions required for it. In case it
> > > > >     doesn't we'll error out. This is done to follow the same prerogative
> > > > >     we always had of not allowing extensions being enabled for vendor
> > > > >     CPUs;
> > > > 
> > > > Why shouldn't this be allowed ?
> > > 
> > > There's no technical reason to not allow it. The reason it's forbid is to be
> > > closer to what the real hardware would do. E.g. the real hardware doesn't allow
> > > users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
> > > a privileged spec restriction as well, so if a CPU is running in an older spec
> > > it can't enable extensions that were added later.
> > 
> > Real hardware is constrained in not being able to invent arbitrary
> > new features on chip. Virtual machines  are not constrained, so
> > I don't think the inability of hardware todo this, is an especially
> > strong reason to limit software emulation.
> > 
> > What I don't like about this, is that (IIUC) the '$profile=on' option
> > now has different semantics depending on what CPU it is used with.
> > 
> > ie  using it with a vendor CPU,   $profile=on  becomes an assertion
> > that the vendor CPU contains all the features needed to satisfy
> > $profile. It won't enable/disable anything, just check it is present.
> > 
> > With a non-vendor CPU, using $profile=on becomes a mechanism to force
> > enable all the features needed to satisfy $profile, there is no
> > mechanism to just check for presence.
> > 
> > Having two different semantics for the same syntax is generally considered
> > bad design practice.
> > 
> > This points towards supporting a tri-state, not boolean. $profile=check
> > for validation only, and $profile=on for force enablement.
> 
> This would leave us with:
> 
> - $profile=off => disable all extensions. Let users hit themselves in the foot if they
> don't enable any other extensions. Note that disabling a profile and enabling extensions
> on top of it is very sensitive to left-to-right ordering, so it would be good to have
> a way to enforce this ordering somehow (feature groups always first);

It is also order sensitive if 2 profiles have overlap in the
extensions they represent. So might also require an ordering
of profiles themselves to be defined if you permit multiple
profiles.

If we dont want to think about this immediately that, then
we should make $profile=off into a fatal error rather than
silently ignoring it

> - $profile=on => only valid for generic CPUs;
> 
> - $profile=check -> valid for all CPUs, would only check if the CPU implements the profile.
> 
> 
> I think this is fine. Drew, care to weight in?


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel Henrique Barboza]

On 9/29/23 09:52, Daniel P. Berrangé wrote:
> On Fri, Sep 29, 2023 at 09:49:47AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 9/29/23 08:55, Daniel P. Berrangé wrote:
>>> On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 9/29/23 07:46, Daniel P. Berrangé wrote:
>>>>> On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
>>>>>> Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
>>>>>> ("[PATCH 0/2] riscv: add extension properties for all cpus")
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> These patches implements the base profile support for qemu-riscv and the
>>>>>> first profile, RVA22U64.
>>>>>>
>>>>>> As discussed in this thread [1] we're aiming for a flag that enables all
>>>>>> mandatory extensions of a profile. Optional extensions were left behind
>>>>>> and must be enabled by hand if desired. Since this is the first profile
>>>>>> we're adding, we'll need to add the base framework as well.
>>>>>>
>>>>>> The RVA22U64 profile was chosen because qemu-riscv implements all its
>>>>>> extensions, both mandatory and optional. That includes 'zicntr' and
>>>>>> 'zihpm', which we support for awhile but aren't adverting to userspace.
>>>>>>
>>>>>> Other design decisions made:
>>>>>>
>>>>>> - disabling a profile flag does nothing, i.e. we won't mass disable
>>>>>>      mandatory extensions of the rva22U64 profile if the user sets
>>>>>>      rva22u64=false;
>>>>>
>>>>> Why shouldn't this be allowed ?
>>>>>
>>>>> IIUC, a profile is syntactic sugar for a group of features. If
>>>>> we can disable individual features explicitly, why should we
>>>>> not allow use of the profile as sugar to disable them en-mass ?
>>>>
>>>> In theory there's no harm in allowing mass disabling of extensions but, given
>>>> it's a whole profile, we would end up disabling most/all CPU extensions and
>>>> the guest would do nothing.
>>>
>>> True, that is just user error though.  They could disable a profile
>>> and then manually re-enable individual features, and thus get a
>>> working system.
>>>
>>>> There is a thread in the ML:
>>>>
>>>> https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@mail.gmail.com/
>>>>
>>>> Where we discussed the possibility of having a minimal CPU extension set. We didn't
>>>> reach a consensus because the definition of "minimal CPU extension set" vary between
>>>> OSes (Linux requires IMAFD, FreeBSD might require something differ).
>>>>
>>>> Assuming we reach a consensus on what a minimal set is, we could allow disabling mass
>>>> extensions via probile but keeping this minimal set, for example. At very least we
>>>> shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I is
>>>> the minimum set that I would assume for now.
>>>
>>> I'd probably just call that user error too.
>>>
>>>>>
>>>>> TL;DR: feature groups are pretty error prone if more than
>>>>> one is listed by the user, or they're combined with individual
>>>>> features.
>>>>>
>>>>>>
>>>>>> - profile support for vendor CPUs consists into checking if the CPU
>>>>>>      happens to have the mandatory extensions required for it. In case it
>>>>>>      doesn't we'll error out. This is done to follow the same prerogative
>>>>>>      we always had of not allowing extensions being enabled for vendor
>>>>>>      CPUs;
>>>>>
>>>>> Why shouldn't this be allowed ?
>>>>
>>>> There's no technical reason to not allow it. The reason it's forbid is to be
>>>> closer to what the real hardware would do. E.g. the real hardware doesn't allow
>>>> users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
>>>> a privileged spec restriction as well, so if a CPU is running in an older spec
>>>> it can't enable extensions that were added later.
>>>
>>> Real hardware is constrained in not being able to invent arbitrary
>>> new features on chip. Virtual machines  are not constrained, so
>>> I don't think the inability of hardware todo this, is an especially
>>> strong reason to limit software emulation.
>>>
>>> What I don't like about this, is that (IIUC) the '$profile=on' option
>>> now has different semantics depending on what CPU it is used with.
>>>
>>> ie  using it with a vendor CPU,   $profile=on  becomes an assertion
>>> that the vendor CPU contains all the features needed to satisfy
>>> $profile. It won't enable/disable anything, just check it is present.
>>>
>>> With a non-vendor CPU, using $profile=on becomes a mechanism to force
>>> enable all the features needed to satisfy $profile, there is no
>>> mechanism to just check for presence.
>>>
>>> Having two different semantics for the same syntax is generally considered
>>> bad design practice.
>>>
>>> This points towards supporting a tri-state, not boolean. $profile=check
>>> for validation only, and $profile=on for force enablement.
>>
>> This would leave us with:
>>
>> - $profile=off => disable all extensions. Let users hit themselves in the foot if they
>> don't enable any other extensions. Note that disabling a profile and enabling extensions
>> on top of it is very sensitive to left-to-right ordering, so it would be good to have
>> a way to enforce this ordering somehow (feature groups always first);
> 
> It is also order sensitive if 2 profiles have overlap in the
> extensions they represent. So might also require an ordering
> of profiles themselves to be defined if you permit multiple
> profiles.
> 
> If we dont want to think about this immediately that, then
> we should make $profile=off into a fatal error rather than
> silently ignoring it

I don't mind handling it right now, I just don't know how hehe

I'll re-read the thread you sent earlier and see if there's something I missed. I got the
impression that we need your qom patch first.


Thanks,

Daniel

> 
>> - $profile=on => only valid for generic CPUs;
>>
>> - $profile=check -> valid for all CPUs, would only check if the CPU implements the profile.
>>
>>
>> I think this is fine. Drew, care to weight in?
> 
> 
> With regards,
> Daniel

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel P. Berrangé]

On Fri, Sep 29, 2023 at 10:26:08AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/29/23 09:52, Daniel P. Berrangé wrote:
> > On Fri, Sep 29, 2023 at 09:49:47AM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 9/29/23 08:55, Daniel P. Berrangé wrote:
> > > > On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
> > > > > 
> > > > > 
> > > > > On 9/29/23 07:46, Daniel P. Berrangé wrote:
> > > > > > On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> > > > > > > Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
> > > > > > > ("[PATCH 0/2] riscv: add extension properties for all cpus")
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > These patches implements the base profile support for qemu-riscv and the
> > > > > > > first profile, RVA22U64.
> > > > > > > 
> > > > > > > As discussed in this thread [1] we're aiming for a flag that enables all
> > > > > > > mandatory extensions of a profile. Optional extensions were left behind
> > > > > > > and must be enabled by hand if desired. Since this is the first profile
> > > > > > > we're adding, we'll need to add the base framework as well.
> > > > > > > 
> > > > > > > The RVA22U64 profile was chosen because qemu-riscv implements all its
> > > > > > > extensions, both mandatory and optional. That includes 'zicntr' and
> > > > > > > 'zihpm', which we support for awhile but aren't adverting to userspace.
> > > > > > > 
> > > > > > > Other design decisions made:
> > > > > > > 
> > > > > > > - disabling a profile flag does nothing, i.e. we won't mass disable
> > > > > > >      mandatory extensions of the rva22U64 profile if the user sets
> > > > > > >      rva22u64=false;
> > > > > > 
> > > > > > Why shouldn't this be allowed ?
> > > > > > 
> > > > > > IIUC, a profile is syntactic sugar for a group of features. If
> > > > > > we can disable individual features explicitly, why should we
> > > > > > not allow use of the profile as sugar to disable them en-mass ?
> > > > > 
> > > > > In theory there's no harm in allowing mass disabling of extensions but, given
> > > > > it's a whole profile, we would end up disabling most/all CPU extensions and
> > > > > the guest would do nothing.
> > > > 
> > > > True, that is just user error though.  They could disable a profile
> > > > and then manually re-enable individual features, and thus get a
> > > > working system.
> > > > 
> > > > > There is a thread in the ML:
> > > > > 
> > > > > https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@mail.gmail.com/
> > > > > 
> > > > > Where we discussed the possibility of having a minimal CPU extension set. We didn't
> > > > > reach a consensus because the definition of "minimal CPU extension set" vary between
> > > > > OSes (Linux requires IMAFD, FreeBSD might require something differ).
> > > > > 
> > > > > Assuming we reach a consensus on what a minimal set is, we could allow disabling mass
> > > > > extensions via probile but keeping this minimal set, for example. At very least we
> > > > > shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I is
> > > > > the minimum set that I would assume for now.
> > > > 
> > > > I'd probably just call that user error too.
> > > > 
> > > > > > 
> > > > > > TL;DR: feature groups are pretty error prone if more than
> > > > > > one is listed by the user, or they're combined with individual
> > > > > > features.
> > > > > > 
> > > > > > > 
> > > > > > > - profile support for vendor CPUs consists into checking if the CPU
> > > > > > >      happens to have the mandatory extensions required for it. In case it
> > > > > > >      doesn't we'll error out. This is done to follow the same prerogative
> > > > > > >      we always had of not allowing extensions being enabled for vendor
> > > > > > >      CPUs;
> > > > > > 
> > > > > > Why shouldn't this be allowed ?
> > > > > 
> > > > > There's no technical reason to not allow it. The reason it's forbid is to be
> > > > > closer to what the real hardware would do. E.g. the real hardware doesn't allow
> > > > > users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
> > > > > a privileged spec restriction as well, so if a CPU is running in an older spec
> > > > > it can't enable extensions that were added later.
> > > > 
> > > > Real hardware is constrained in not being able to invent arbitrary
> > > > new features on chip. Virtual machines  are not constrained, so
> > > > I don't think the inability of hardware todo this, is an especially
> > > > strong reason to limit software emulation.
> > > > 
> > > > What I don't like about this, is that (IIUC) the '$profile=on' option
> > > > now has different semantics depending on what CPU it is used with.
> > > > 
> > > > ie  using it with a vendor CPU,   $profile=on  becomes an assertion
> > > > that the vendor CPU contains all the features needed to satisfy
> > > > $profile. It won't enable/disable anything, just check it is present.
> > > > 
> > > > With a non-vendor CPU, using $profile=on becomes a mechanism to force
> > > > enable all the features needed to satisfy $profile, there is no
> > > > mechanism to just check for presence.
> > > > 
> > > > Having two different semantics for the same syntax is generally considered
> > > > bad design practice.
> > > > 
> > > > This points towards supporting a tri-state, not boolean. $profile=check
> > > > for validation only, and $profile=on for force enablement.
> > > 
> > > This would leave us with:
> > > 
> > > - $profile=off => disable all extensions. Let users hit themselves in the foot if they
> > > don't enable any other extensions. Note that disabling a profile and enabling extensions
> > > on top of it is very sensitive to left-to-right ordering, so it would be good to have
> > > a way to enforce this ordering somehow (feature groups always first);
> > 
> > It is also order sensitive if 2 profiles have overlap in the
> > extensions they represent. So might also require an ordering
> > of profiles themselves to be defined if you permit multiple
> > profiles.
> > 
> > If we dont want to think about this immediately that, then
> > we should make $profile=off into a fatal error rather than
> > silently ignoring it
> 
> I don't mind handling it right now, I just don't know how hehe
> 
> I'll re-read the thread you sent earlier and see if there's something I missed. I got the
> impression that we need your qom patch first.

My patch is dropped as it was considered too gross. Kevin provided
a new impl for ARRAY properties which eliminates the ordering problem.

IOW, QemuOpts iteration order will remain undefined in general.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Alistair Francis]

On Fri, Sep 29, 2023 at 10:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Sep 29, 2023 at 09:49:47AM -0300, Daniel Henrique Barboza wrote:
> >
> >
> > On 9/29/23 08:55, Daniel P. Berrangé wrote:
> > > On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
> > > >
> > > >
> > > > On 9/29/23 07:46, Daniel P. Berrangé wrote:
> > > > > On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> > > > > > Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
> > > > > > ("[PATCH 0/2] riscv: add extension properties for all cpus")
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > These patches implements the base profile support for qemu-riscv and the
> > > > > > first profile, RVA22U64.
> > > > > >
> > > > > > As discussed in this thread [1] we're aiming for a flag that enables all
> > > > > > mandatory extensions of a profile. Optional extensions were left behind
> > > > > > and must be enabled by hand if desired. Since this is the first profile
> > > > > > we're adding, we'll need to add the base framework as well.
> > > > > >
> > > > > > The RVA22U64 profile was chosen because qemu-riscv implements all its
> > > > > > extensions, both mandatory and optional. That includes 'zicntr' and
> > > > > > 'zihpm', which we support for awhile but aren't adverting to userspace.
> > > > > >
> > > > > > Other design decisions made:
> > > > > >
> > > > > > - disabling a profile flag does nothing, i.e. we won't mass disable
> > > > > >     mandatory extensions of the rva22U64 profile if the user sets
> > > > > >     rva22u64=false;
> > > > >
> > > > > Why shouldn't this be allowed ?
> > > > >
> > > > > IIUC, a profile is syntactic sugar for a group of features. If
> > > > > we can disable individual features explicitly, why should we
> > > > > not allow use of the profile as sugar to disable them en-mass ?
> > > >
> > > > In theory there's no harm in allowing mass disabling of extensions but, given
> > > > it's a whole profile, we would end up disabling most/all CPU extensions and
> > > > the guest would do nothing.
> > >
> > > True, that is just user error though.  They could disable a profile
> > > and then manually re-enable individual features, and thus get a
> > > working system.
> > >
> > > > There is a thread in the ML:
> > > >
> > > > https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@mail.gmail.com/
> > > >
> > > > Where we discussed the possibility of having a minimal CPU extension set. We didn't
> > > > reach a consensus because the definition of "minimal CPU extension set" vary between
> > > > OSes (Linux requires IMAFD, FreeBSD might require something differ).
> > > >
> > > > Assuming we reach a consensus on what a minimal set is, we could allow disabling mass
> > > > extensions via probile but keeping this minimal set, for example. At very least we
> > > > shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I is
> > > > the minimum set that I would assume for now.
> > >
> > > I'd probably just call that user error too.
> > >
> > > > >
> > > > > TL;DR: feature groups are pretty error prone if more than
> > > > > one is listed by the user, or they're combined with individual
> > > > > features.
> > > > >
> > > > > >
> > > > > > - profile support for vendor CPUs consists into checking if the CPU
> > > > > >     happens to have the mandatory extensions required for it. In case it
> > > > > >     doesn't we'll error out. This is done to follow the same prerogative
> > > > > >     we always had of not allowing extensions being enabled for vendor
> > > > > >     CPUs;
> > > > >
> > > > > Why shouldn't this be allowed ?
> > > >
> > > > There's no technical reason to not allow it. The reason it's forbid is to be
> > > > closer to what the real hardware would do. E.g. the real hardware doesn't allow
> > > > users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
> > > > a privileged spec restriction as well, so if a CPU is running in an older spec
> > > > it can't enable extensions that were added later.
> > >
> > > Real hardware is constrained in not being able to invent arbitrary
> > > new features on chip. Virtual machines  are not constrained, so
> > > I don't think the inability of hardware todo this, is an especially
> > > strong reason to limit software emulation.

I think exposing flexibility in vendor CPUs just creates confusion.

As a user if I start QEMU with "-cpu company-cpu" then I am expecting
to get an emulation of company-cpu.

> > >
> > > What I don't like about this, is that (IIUC) the '$profile=on' option
> > > now has different semantics depending on what CPU it is used with.
> > >
> > > ie  using it with a vendor CPU,   $profile=on  becomes an assertion
> > > that the vendor CPU contains all the features needed to satisfy
> > > $profile. It won't enable/disable anything, just check it is present.
> > >
> > > With a non-vendor CPU, using $profile=on becomes a mechanism to force
> > > enable all the features needed to satisfy $profile, there is no
> > > mechanism to just check for presence.
> > >
> > > Having two different semantics for the same syntax is generally considered
> > > bad design practice.
> > >
> > > This points towards supporting a tri-state, not boolean. $profile=check
> > > for validation only, and $profile=on for force enablement.
> >
> > This would leave us with:
> >
> > - $profile=off => disable all extensions. Let users hit themselves in the foot if they
> > don't enable any other extensions. Note that disabling a profile and enabling extensions
> > on top of it is very sensitive to left-to-right ordering, so it would be good to have
> > a way to enforce this ordering somehow (feature groups always first);
>
> It is also order sensitive if 2 profiles have overlap in the
> extensions they represent. So might also require an ordering
> of profiles themselves to be defined if you permit multiple
> profiles.
>
> If we dont want to think about this immediately that, then
> we should make $profile=off into a fatal error rather than
> silently ignoring it

I think that makes sense.

I think we can be pretty strict on profiles options. To me it seems
reasonable to say a user can enable **one** profile. Once that profile
is enabled they get all of those extensions.

If possible/simple we can then allow them to manually enable and/or
disable extensions on top of that. I don't see any use in allowing
users to turn profiles "off" though. I'm not even clear what that
means.

Alistair

>
> > - $profile=on => only valid for generic CPUs;
> >
> > - $profile=check -> valid for all CPUs, would only check if the CPU implements the profile.
> >
> >
> > I think this is fine. Drew, care to weight in?
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>