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

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

[Daniel Henrique Barboza]

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

Hi,

The most notable change in this new version is that we're back to
enabling/disabling profile extensions during the property set()
callback, instead of doing an extra step during realize(), to give more
predictability on how profiles can interact with each other.

In the previous version profiles would be committed based on a fixed
internal order. This time they'll be evaluated via the common
left-to-right ordering we're used to in QEMU. This means that these
two configurations are different:

-cpu rv64,profileA=true,profileB=false
"enable all mandatory extensions of profile A, then disable all
mandatory extensions of profile B"

-cpu rv64,profileB=false,profileA=true
"disable all mandatory extensions of profile B, then enable all
mandatory extensions of profile A"

We're also adding an user warning if a profile is disabled. Between
implementing profile disablement (disable all its mandatory extensions)
versus not implementing it and having a flag that does nothing if the
user set it to 'off', we're choosing the least of two ills with the
former. However, given that it is a very niche feature that is hard to
use it right and very easy to do something silly, an user warning is
appropriate.

Series is based on top of the zicntr/zihpm implementation series since
they're both part of the rva22u64 profile.

Patches missing acks: 3, 6

Changes from v2:
- patches 1 and 2 from v2: moved to a separated series
- patches 6, 9 and 10 from v2: dropped
- patch 3:
  - use the set() callback to enable/disable mandatory profile
    extensions
- patch 6:
   - handle profile MISA bits in the profile set() callback
- v2 link: https://lore.kernel.org/qemu-riscv/20231006132134.1135297-1-dbarboza@ventanamicro.com/


Daniel Henrique Barboza (6):
  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/cpu.c         |  20 +++++++
 target/riscv/cpu.h         |  12 ++++
 target/riscv/kvm/kvm-cpu.c |   7 ++-
 target/riscv/tcg/tcg-cpu.c | 118 ++++++++++++++++++++++++++++++++-----
 4 files changed, 142 insertions(+), 15 deletions(-)

-- 
2.41.0

[PATCH v3 1/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>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 20 ++++++++++++++++++++
 target/riscv/cpu.h | 12 ++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c64cd726f4..1b75b506c4 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1397,6 +1397,26 @@ Property riscv_cpu_options[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/* Optional extensions left out: RVV, zfh, zkn, zks */
+static RISCVCPUProfile RVA22U64 = {
+    .name = "rva22u64",
+    .misa_ext = 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_zicboz),
+
+        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 7f61e17202..53c1970e0a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -66,6 +66,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 v3 2/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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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 5246fc2bdc..dc14c54ce4 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -360,7 +360,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);
     }
 }
@@ -440,6 +440,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 v3 3/6] 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 disabling all its mandatory
extensions. Disabling a profile is discouraged for regular use and will
issue an user warning. User choices for individual extensions will take
precedence, i.e. enabling a profile will not enable extensions that the
user set to 'false', and vice-versa. This will make us independent of
left-to-right ordering in the QEMU command line, i.e. the following QEMU
command lines:

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

They mean the same thing: "enable all mandatory extensions of the
rva22u64 profile while keeping zicbom and Zifencei disabled".

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>
---
 target/riscv/tcg/tcg-cpu.c | 59 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 7a4400e2ba..3dd4783191 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -757,6 +757,63 @@ 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 (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!value) {
+        warn_report("Disabling the '%s' profile is a debug/development "
+                    "tool, not recommended for regular use",
+                    profile->name);
+    }
+
+    profile->enabled = value;
+
+    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
+        ext_offset = profile->ext_offsets[i];
+
+        if (cpu_cfg_ext_is_user_set(ext_offset)) {
+            continue;
+        }
+
+        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]);
@@ -880,6 +937,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 v3 4/6] 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>
---
 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 3dd4783191..59b75a14ac 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)
 {
@@ -669,6 +670,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) {
@@ -732,6 +737,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;
 
@@ -752,7 +758,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;
+            }
         }
     }
 }
@@ -989,6 +1001,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 v3 5/6] 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>
---
 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 59b75a14ac..ba11d0566d 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)
 {
@@ -680,20 +694,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,
@@ -737,7 +745,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;
 
@@ -758,13 +765,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 v3 6/6] 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 user set the bit, doesn't
matter if to 'true' or 'false', ignore it. If the profile doesn't
declare the bit as mandatory, ignore it. Otherwise, set or clear the bit
in env->misa_ext and env->misa_ext_mask depending on whether the profile
was set to 'true' or 'false'.

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index ba11d0566d..73c7453af6 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 riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
                                      bool enabled)
 {
@@ -797,6 +803,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
 
     profile->enabled = value;
 
+    for (i = 0; misa_bits[i] != 0; i++) {
+        uint32_t bit = misa_bits[i];
+
+        if (cpu_misa_ext_is_user_set(bit) || !(profile->misa_ext & bit)) {
+            continue;
+        }
+
+        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 v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Fri, Oct 20, 2023 at 07:39:48PM -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 disabling all its mandatory
> extensions. Disabling a profile is discouraged for regular use and will
> issue an user warning. User choices for individual extensions will take
> precedence, i.e. enabling a profile will not enable extensions that the
> user set to 'false', and vice-versa. This will make us independent of
> left-to-right ordering in the QEMU command line, i.e. the following QEMU
> command lines:
> 
> -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
> -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
> 
> They mean the same thing: "enable all mandatory extensions of the
> rva22u64 profile while keeping zicbom and Zifencei disabled".

Hmm, I'm not sure I agree with special-casing profiles like this. I think
the left-to-right processing should be consistent for all. I'm also not
sure we should always warn when disabling a profile. For example, if a
user does

 -cpu rv64,rva22u64=true,rva22u64=false

then they'll get a warning, even though all they're doing is restoring the
cpu model. While that looks like an odd thing to do, a script may be
adding the rva22u64=true and the rva22u64=false is the user input which
undoes what the script did.

As far as warnings go, it'd be nice to warn when mandatory profile
extensions are disabled from an enabled profile. Doing that might be
useful for debug, but users which do it without being aware they're
"breaking" the profile may learn from that warning. Note, the warning
should only come when the profile is actually enabled and when the
extension would actually be disabled, i.e.

 -cpu rv64,rva22u64=true,c=off

should warn

 -cpu rv64,c=off,rva22u64=true

should not warn (rva22u64 overrides c=off since it's to the right)

 -cpu rv64,rva22u64=true,rva22u64=false,c=off

should not warn (rva22u64 is not enabled)

And,

 -cpu rv64,rva22u64=true,rva24u64=false

should warn for each extension which is mandatory in both profiles.

Thanks,
drew

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/23/23 05:16, Andrew Jones wrote:
> On Fri, Oct 20, 2023 at 07:39:48PM -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 disabling all its mandatory
>> extensions. Disabling a profile is discouraged for regular use and will
>> issue an user warning. User choices for individual extensions will take
>> precedence, i.e. enabling a profile will not enable extensions that the
>> user set to 'false', and vice-versa. This will make us independent of
>> left-to-right ordering in the QEMU command line, i.e. the following QEMU
>> command lines:
>>
>> -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
>> -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
>> -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
>>
>> They mean the same thing: "enable all mandatory extensions of the
>> rva22u64 profile while keeping zicbom and Zifencei disabled".
> 
> Hmm, I'm not sure I agree with special-casing profiles like this. I think
> the left-to-right processing should be consistent for all. I'm also not
> sure we should always warn when disabling a profile. For example, if a
> user does
> 
>   -cpu rv64,rva22u64=true,rva22u64=false
> 
> then they'll get a warning, even though all they're doing is restoring the
> cpu model. While that looks like an odd thing to do, a script may be
> adding the rva22u64=true and the rva22u64=false is the user input which
> undoes what the script did.

QEMU options do not work with a "the user enabled then disabled the same option,
thus it'll count as nothing happened" logic. The last instance of the option will
overwrite all previous instances. In the example you mentioned above the user would
disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
same profile was enabled beforehand.

Sure, the can put code in place to make this happen, but then this would make
profiles act different than regular extensions. "-cpu rv64,zicbom=true -cpu rv64,zicbom=false"
will disable zicbom, it will not preserve the original 'zicbom' rv64 default. If
we're going to keep left-to-right ordering consistent, this behavior should also
be consistent as well.


As for warnings, I agree that we'll throw warnings even when nothing of notice happened.
For example:

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

This will throw a warning even though the user ended up enabling the extension
in the end.


We can fix it by postponing warnings to realize().


> 
> As far as warnings go, it'd be nice to warn when mandatory profile
> extensions are disabled from an enabled profile. Doing that might be
> useful for debug, but users which do it without being aware they're
> "breaking" the profile may learn from that warning. Note, the warning
> should only come when the profile is actually enabled and when the
> extension would actually be disabled, i.e.
> 
>   -cpu rv64,rva22u64=true,c=off
> 
> should warn
> 
>   -cpu rv64,c=off,rva22u64=true
> 
> should not warn (rva22u64 overrides c=off since it's to the right)
> 
>   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> 
> should not warn (rva22u64 is not enabled)

Ack for all the above.

> 
> And,
> 
>   -cpu rv64,rva22u64=true,rva24u64=false
> 
> should warn for each extension which is mandatory in both profiles.

The way I'm imagining this happening is to cycle through all profiles during realize(),
see which ones are enabled, and then warn if the user disabled their mandatory
extensions. In this example we would warn for all rva22 mandatory extensions
that were disabled because we disabled rva24, but we won't emit any warnings for
rva24 mandatory extensions given that the profile is marked as disabled.



Thanks,

Daniel


> 
> Thanks,
> drew

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 10/23/23 05:16, Andrew Jones wrote:
> > On Fri, Oct 20, 2023 at 07:39:48PM -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 disabling all its mandatory
> > > extensions. Disabling a profile is discouraged for regular use and will
> > > issue an user warning. User choices for individual extensions will take
> > > precedence, i.e. enabling a profile will not enable extensions that the
> > > user set to 'false', and vice-versa. This will make us independent of
> > > left-to-right ordering in the QEMU command line, i.e. the following QEMU
> > > command lines:
> > > 
> > > -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> > > -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
> > > -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
> > > 
> > > They mean the same thing: "enable all mandatory extensions of the
> > > rva22u64 profile while keeping zicbom and Zifencei disabled".
> > 
> > Hmm, I'm not sure I agree with special-casing profiles like this. I think
> > the left-to-right processing should be consistent for all. I'm also not
> > sure we should always warn when disabling a profile. For example, if a
> > user does
> > 
> >   -cpu rv64,rva22u64=true,rva22u64=false
> > 
> > then they'll get a warning, even though all they're doing is restoring the
> > cpu model. While that looks like an odd thing to do, a script may be
> > adding the rva22u64=true and the rva22u64=false is the user input which
> > undoes what the script did.
> 
> QEMU options do not work with a "the user enabled then disabled the same option,
> thus it'll count as nothing happened" logic. The last instance of the option will
> overwrite all previous instances. In the example you mentioned above the user would
> disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
> same profile was enabled beforehand.

Yup, I'm aware, but I keep thinking that we'll only be using profiles with
a base cpu type. If you start with nothing (a base) and then add a profile
and take the same one away, you shouldn't be taking away anything else. I
agree that if you use a profile on some cpu type that already enabled a
bunch of stuff itself, then disabling a profile would potentially remove
some of those too, but mixing cpu types that have their own extensions and
profiles seems like a great way to confuse oneself as to what extensions
will be present.  IOW, we should be adding a base cpu type at the same
time we're adding these profiles.

> 
> Sure, the can put code in place to make this happen, but then this would make
> profiles act different than regular extensions. "-cpu rv64,zicbom=true -cpu rv64,zicbom=false"
> will disable zicbom, it will not preserve the original 'zicbom' rv64 default. If
> we're going to keep left-to-right ordering consistent, this behavior should also
> be consistent as well.

It will be consistent if we always override whatever was on the left with
what's on the right, which means with

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

zicbom will be disabled, but with

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

it will be enabled. The same goes if the properties are given to the same
-cpu parameter.

> 
> 
> As for warnings, I agree that we'll throw warnings even when nothing of notice happened.
> For example:
> 
> -cpu rv64,rva22u64=false -cpu rv64,rva22u64=true
> 
> This will throw a warning even though the user ended up enabling the extension
> in the end.
> 
> 
> We can fix it by postponing warnings to realize().
> 
> 
> > 
> > As far as warnings go, it'd be nice to warn when mandatory profile
> > extensions are disabled from an enabled profile. Doing that might be
> > useful for debug, but users which do it without being aware they're
> > "breaking" the profile may learn from that warning. Note, the warning
> > should only come when the profile is actually enabled and when the
> > extension would actually be disabled, i.e.
> > 
> >   -cpu rv64,rva22u64=true,c=off
> > 
> > should warn
> > 
> >   -cpu rv64,c=off,rva22u64=true
> > 
> > should not warn (rva22u64 overrides c=off since it's to the right)
> > 
> >   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> > 
> > should not warn (rva22u64 is not enabled)
> 
> Ack for all the above.
> 
> > 
> > And,
> > 
> >   -cpu rv64,rva22u64=true,rva24u64=false
> > 
> > should warn for each extension which is mandatory in both profiles.
> 
> The way I'm imagining this happening is to cycle through all profiles during realize(),
> see which ones are enabled, and then warn if the user disabled their mandatory
> extensions. In this example we would warn for all rva22 mandatory extensions
> that were disabled because we disabled rva24, but we won't emit any warnings for
> rva24 mandatory extensions given that the profile is marked as disabled.

Yup, sounds good.

Thanks,
drew

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/23/23 14:35, Andrew Jones wrote:
> On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 10/23/23 05:16, Andrew Jones wrote:
>>> On Fri, Oct 20, 2023 at 07:39:48PM -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 disabling all its mandatory
>>>> extensions. Disabling a profile is discouraged for regular use and will
>>>> issue an user warning. User choices for individual extensions will take
>>>> precedence, i.e. enabling a profile will not enable extensions that the
>>>> user set to 'false', and vice-versa. This will make us independent of
>>>> left-to-right ordering in the QEMU command line, i.e. the following QEMU
>>>> command lines:
>>>>
>>>> -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
>>>> -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
>>>> -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
>>>>
>>>> They mean the same thing: "enable all mandatory extensions of the
>>>> rva22u64 profile while keeping zicbom and Zifencei disabled".
>>>
>>> Hmm, I'm not sure I agree with special-casing profiles like this. I think
>>> the left-to-right processing should be consistent for all. I'm also not
>>> sure we should always warn when disabling a profile. For example, if a
>>> user does
>>>
>>>    -cpu rv64,rva22u64=true,rva22u64=false
>>>
>>> then they'll get a warning, even though all they're doing is restoring the
>>> cpu model. While that looks like an odd thing to do, a script may be
>>> adding the rva22u64=true and the rva22u64=false is the user input which
>>> undoes what the script did.
>>
>> QEMU options do not work with a "the user enabled then disabled the same option,
>> thus it'll count as nothing happened" logic. The last instance of the option will
>> overwrite all previous instances. In the example you mentioned above the user would
>> disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
>> same profile was enabled beforehand.
> 
> Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> a base cpu type. If you start with nothing (a base) and then add a profile
> and take the same one away, you shouldn't be taking away anything else. I
> agree that if you use a profile on some cpu type that already enabled a
> bunch of stuff itself, then disabling a profile would potentially remove
> some of those too, but mixing cpu types that have their own extensions and
> profiles seems like a great way to confuse oneself as to what extensions
> will be present.  IOW, we should be adding a base cpu type at the same
> time we're adding these profiles.

After this profile work is merged I suggest:

1a - make rv64 a base cpu type, no extensions enabled by default. Change the default
CPU to 'max' like linux-user mode is already doing to avoid breaking users

OR

1b - create a new rv64base|rv64bare to be used exclusively with profiles or when users
really want to know what they're loading in the CPU. No extensions enabled by default.
'rv64' remains untouched as default CPU


I can see a case for both. 1b is less work to make it happen. Thanks,


Daniel


> 
>>
>> Sure, the can put code in place to make this happen, but then this would make
>> profiles act different than regular extensions. "-cpu rv64,zicbom=true -cpu rv64,zicbom=false"
>> will disable zicbom, it will not preserve the original 'zicbom' rv64 default. If
>> we're going to keep left-to-right ordering consistent, this behavior should also
>> be consistent as well.
> 
> It will be consistent if we always override whatever was on the left with
> what's on the right, which means with
> 
>   -cpu rv64,rva22u64=true -cpu rv64,zicbom=false
> 
> zicbom will be disabled, but with
> 
>   -cpu rv64,zicbom=false -cpu rv64,rva22u64=true
> 
> it will be enabled. The same goes if the properties are given to the same
> -cpu parameter.
> 
>>
>>
>> As for warnings, I agree that we'll throw warnings even when nothing of notice happened.
>> For example:
>>
>> -cpu rv64,rva22u64=false -cpu rv64,rva22u64=true
>>
>> This will throw a warning even though the user ended up enabling the extension
>> in the end.
>>
>>
>> We can fix it by postponing warnings to realize().
>>
>>
>>>
>>> As far as warnings go, it'd be nice to warn when mandatory profile
>>> extensions are disabled from an enabled profile. Doing that might be
>>> useful for debug, but users which do it without being aware they're
>>> "breaking" the profile may learn from that warning. Note, the warning
>>> should only come when the profile is actually enabled and when the
>>> extension would actually be disabled, i.e.
>>>
>>>    -cpu rv64,rva22u64=true,c=off
>>>
>>> should warn
>>>
>>>    -cpu rv64,c=off,rva22u64=true
>>>
>>> should not warn (rva22u64 overrides c=off since it's to the right)
>>>
>>>    -cpu rv64,rva22u64=true,rva22u64=false,c=off
>>>
>>> should not warn (rva22u64 is not enabled)
>>
>> Ack for all the above.
>>
>>>
>>> And,
>>>
>>>    -cpu rv64,rva22u64=true,rva24u64=false
>>>
>>> should warn for each extension which is mandatory in both profiles.
>>
>> The way I'm imagining this happening is to cycle through all profiles during realize(),
>> see which ones are enabled, and then warn if the user disabled their mandatory
>> extensions. In this example we would warn for all rva22 mandatory extensions
>> that were disabled because we disabled rva24, but we won't emit any warnings for
>> rva24 mandatory extensions given that the profile is marked as disabled.
> 
> Yup, sounds good.
> 
> Thanks,
> drew

Re: [PATCH v3 1/6] target/riscv: add rva22u64 profile definition

[LIU Zhiwei]

On 2023/10/21 6:39, Daniel Henrique Barboza wrote:
> 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>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/cpu.c | 20 ++++++++++++++++++++
>   target/riscv/cpu.h | 12 ++++++++++++
>   2 files changed, 32 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index c64cd726f4..1b75b506c4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1397,6 +1397,26 @@ Property riscv_cpu_options[] = {
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> +/* Optional extensions left out: RVV, zfh, zkn, zks */
> +static RISCVCPUProfile RVA22U64 = {
> +    .name = "rva22u64",
> +    .misa_ext = RVM | RVA | RVF | RVD | RVC,

Why not include RVI?

Zhiwei

> +    .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_zicboz),
> +
> +        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 7f61e17202..53c1970e0a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -66,6 +66,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,

Re: [PATCH v3 2/6] target/riscv/kvm: add 'rva22u64' flag as unavailable

[LIU Zhiwei]

On 2023/10/21 6:39, Daniel Henrique Barboza wrote:
> 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>
> ---
>   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 5246fc2bdc..dc14c54ce4 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -360,7 +360,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);
>       }
>   }
> @@ -440,6 +440,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);
> +    }

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   }
>   
>   static int kvm_riscv_get_regs_core(CPUState *cs)

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[LIU Zhiwei]

On 2023/10/21 6:39, 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 disabling all its mandatory
> extensions. Disabling a profile is discouraged for regular use and will
> issue an user warning. User choices for individual extensions will take
> precedence, i.e. enabling a profile will not enable extensions that the
> user set to 'false', and vice-versa. This will make us independent of
> left-to-right ordering in the QEMU command line, i.e. the following QEMU
> command lines:
>
> -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
> -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
>
> They mean the same thing: "enable all mandatory extensions of the
> rva22u64 profile while keeping zicbom and Zifencei disabled".
>
> 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>
> ---
>   target/riscv/tcg/tcg-cpu.c | 59 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 7a4400e2ba..3dd4783191 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -757,6 +757,63 @@ 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 (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (!value) {
> +        warn_report("Disabling the '%s' profile is a debug/development "
> +                    "tool, not recommended for regular use",
> +                    profile->name);
> +    }
> +
> +    profile->enabled = value;
> +
> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
> +        ext_offset = profile->ext_offsets[i];
> +
> +        if (cpu_cfg_ext_is_user_set(ext_offset)) {
> +            continue;
> +        }
> +
> +        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]);
> @@ -880,6 +937,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);
> +

Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
>           qdev_property_add_static(DEVICE(obj), prop);
>       }

Re: [PATCH v3 4/6] target/riscv/tcg: add MISA user options hash

[LIU Zhiwei]

On 2023/10/21 6:39, Daniel Henrique Barboza wrote:
> 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>
> ---
>   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 3dd4783191..59b75a14ac 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)
>   {
> @@ -669,6 +670,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) {
> @@ -732,6 +737,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;
>   
> @@ -752,7 +758,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;
> +            }
>           }
>       }
>   }
> @@ -989,6 +1001,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);

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>       riscv_cpu_add_user_properties(obj);
>   

Re: [PATCH v3 5/6] target/riscv/tcg: add riscv_cpu_write_misa_bit()

[LIU Zhiwei]

On 2023/10/21 6:39, Daniel Henrique Barboza wrote:
> 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>
> ---
>   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 59b75a14ac..ba11d0566d 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)
>   {
> @@ -680,20 +694,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,
> @@ -737,7 +745,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;
>   
> @@ -758,13 +765,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);

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           }
>       }
>   }

Re: [PATCH v3 6/6] target/riscv/tcg: handle profile MISA bits

[LIU Zhiwei]

On 2023/10/21 6:39, Daniel Henrique Barboza 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 user set the bit, doesn't
> matter if to 'true' or 'false', ignore it. If the profile doesn't
> declare the bit as mandatory, ignore it. Otherwise, set or clear the bit
> in env->misa_ext and env->misa_ext_mask depending on whether the profile
> was set to 'true' or 'false'.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/tcg/tcg-cpu.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index ba11d0566d..73c7453af6 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 riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
>                                        bool enabled)
>   {
> @@ -797,6 +803,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>   
>       profile->enabled = value;
>   
> +    for (i = 0; misa_bits[i] != 0; i++) {
> +        uint32_t bit = misa_bits[i];
> +
> +        if (cpu_misa_ext_is_user_set(bit) || !(profile->misa_ext & bit)) {
> +            continue;
> +        }
> +
> +        riscv_cpu_write_misa_bit(cpu, bit, profile->enabled);
> +    }
> +

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
>           ext_offset = profile->ext_offsets[i];
>   

Re: [PATCH v3 1/6] target/riscv: add rva22u64 profile definition

[Daniel Henrique Barboza]

On 10/25/23 03:22, LIU Zhiwei wrote:
> 
> On 2023/10/21 6:39, Daniel Henrique Barboza wrote:
>> 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>
>> Acked-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>   target/riscv/cpu.c | 20 ++++++++++++++++++++
>>   target/riscv/cpu.h | 12 ++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index c64cd726f4..1b75b506c4 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1397,6 +1397,26 @@ Property riscv_cpu_options[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>> +/* Optional extensions left out: RVV, zfh, zkn, zks */
>> +static RISCVCPUProfile RVA22U64 = {
>> +    .name = "rva22u64",
>> +    .misa_ext = RVM | RVA | RVF | RVD | RVC,
> 
> Why not include RVI?

Because I forgot :D

I'll fix it in v4. Thanks,

Daniel

> 
> Zhiwei
> 
>> +    .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_zicboz),
>> +
>> +        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 7f61e17202..53c1970e0a 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -66,6 +66,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,

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrea Bolognani]

On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > On 10/23/23 05:16, Andrew Jones wrote:
> > > Hmm, I'm not sure I agree with special-casing profiles like this. I think
> > > the left-to-right processing should be consistent for all. I'm also not
> > > sure we should always warn when disabling a profile. For example, if a
> > > user does
> > >
> > >   -cpu rv64,rva22u64=true,rva22u64=false
> > >
> > > then they'll get a warning, even though all they're doing is restoring the
> > > cpu model. While that looks like an odd thing to do, a script may be
> > > adding the rva22u64=true and the rva22u64=false is the user input which
> > > undoes what the script did.
> >
> > QEMU options do not work with a "the user enabled then disabled the same option,
> > thus it'll count as nothing happened" logic. The last instance of the option will
> > overwrite all previous instances. In the example you mentioned above the user would
> > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
> > same profile was enabled beforehand.
>
> Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> a base cpu type. If you start with nothing (a base) and then add a profile
> and take the same one away, you shouldn't be taking away anything else. I
> agree that if you use a profile on some cpu type that already enabled a
> bunch of stuff itself, then disabling a profile would potentially remove
> some of those too, but mixing cpu types that have their own extensions and
> profiles seems like a great way to confuse oneself as to what extensions
> will be present.  IOW, we should be adding a base cpu type at the same
> time we're adding these profiles.

The question that keep bouncing around my head is: why would we even
allow disabling profiles?

It seems to me that it only makes things more complicated, and I
really can't see the use case for it.

Enabling additional features on top of a profile? There's obvious
value in that, so that you can model hardware that implements
optional and proprietary extensions. Enabling multiple profiles?
You've convinced me that it's useful. But disabling profiles, I just
don't see it. I believe Alistair was similarly unconvinced.

> > > As far as warnings go, it'd be nice to warn when mandatory profile
> > > extensions are disabled from an enabled profile. Doing that might be
> > > useful for debug, but users which do it without being aware they're
> > > "breaking" the profile may learn from that warning. Note, the warning
> > > should only come when the profile is actually enabled and when the
> > > extension would actually be disabled, i.e.
> > >
> > >   -cpu rv64,rva22u64=true,c=off
> > >
> > > should warn
> > >
> > >   -cpu rv64,c=off,rva22u64=true
> > >
> > > should not warn (rva22u64 overrides c=off since it's to the right)
> > >
> > >   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> > >
> > > should not warn (rva22u64 is not enabled)

I think these should be hard errors, not warnings.

If you're enabling a profile and then disabling an extension that's
mandatory for that profile, you've invalidated the profile. You've
asked for a configuration that doesn't make any sense: you can't have
a CPU that both implements a profile and lacks one of its mandatory
extensions.

QEMU users could easily miss the warning. libvirt users won't see it
at all. It's a user error and it needs to be treated as such IMO.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
> On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > > On 10/23/23 05:16, Andrew Jones wrote:
> > > > Hmm, I'm not sure I agree with special-casing profiles like this. I think
> > > > the left-to-right processing should be consistent for all. I'm also not
> > > > sure we should always warn when disabling a profile. For example, if a
> > > > user does
> > > >
> > > >   -cpu rv64,rva22u64=true,rva22u64=false
> > > >
> > > > then they'll get a warning, even though all they're doing is restoring the
> > > > cpu model. While that looks like an odd thing to do, a script may be
> > > > adding the rva22u64=true and the rva22u64=false is the user input which
> > > > undoes what the script did.
> > >
> > > QEMU options do not work with a "the user enabled then disabled the same option,
> > > thus it'll count as nothing happened" logic. The last instance of the option will
> > > overwrite all previous instances. In the example you mentioned above the user would
> > > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
> > > same profile was enabled beforehand.
> >
> > Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> > a base cpu type. If you start with nothing (a base) and then add a profile
> > and take the same one away, you shouldn't be taking away anything else. I
> > agree that if you use a profile on some cpu type that already enabled a
> > bunch of stuff itself, then disabling a profile would potentially remove
> > some of those too, but mixing cpu types that have their own extensions and
> > profiles seems like a great way to confuse oneself as to what extensions
> > will be present.  IOW, we should be adding a base cpu type at the same
> > time we're adding these profiles.
> 
> The question that keep bouncing around my head is: why would we even
> allow disabling profiles?
> 
> It seems to me that it only makes things more complicated, and I
> really can't see the use case for it.
> 
> Enabling additional features on top of a profile? There's obvious
> value in that, so that you can model hardware that implements
> optional and proprietary extensions. Enabling multiple profiles?
> You've convinced me that it's useful. But disabling profiles, I just
> don't see it. I believe Alistair was similarly unconvinced.

The only value I see in allowing a profile to be disabled is to undo the
enabling of the profile by specifying the profile as 'off' to the right of
it being specified as 'on'. That may seem pointless, but scripts take
advantage of being able to do that. Besides that one possible use case,
there isn't much use in disabling profiles, but treating profile
properties like every other boolean property makes the UI consistent and
should actually simplify the code.

> 
> > > > As far as warnings go, it'd be nice to warn when mandatory profile
> > > > extensions are disabled from an enabled profile. Doing that might be
> > > > useful for debug, but users which do it without being aware they're
> > > > "breaking" the profile may learn from that warning. Note, the warning
> > > > should only come when the profile is actually enabled and when the
> > > > extension would actually be disabled, i.e.
> > > >
> > > >   -cpu rv64,rva22u64=true,c=off
> > > >
> > > > should warn
> > > >
> > > >   -cpu rv64,c=off,rva22u64=true
> > > >
> > > > should not warn (rva22u64 overrides c=off since it's to the right)
> > > >
> > > >   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> > > >
> > > > should not warn (rva22u64 is not enabled)
> 
> I think these should be hard errors, not warnings.
> 
> If you're enabling a profile and then disabling an extension that's
> mandatory for that profile, you've invalidated the profile. You've
> asked for a configuration that doesn't make any sense: you can't have
> a CPU that both implements a profile and lacks one of its mandatory
> extensions.
> 

Given a platform which implements a profile which mandates extension E and
a need to debug E or test behavior where E is [incorrectly] absent, you'll
need to expand the profile first, listing each of the other extensions
manually. It'd be much faster to specify the profile, take away the
extension, and ignore the warning.

> QEMU users could easily miss the warning. libvirt users won't see it
> at all. It's a user error and it needs to be treated as such IMO.

I do agree with the concern that warnings will be missed/ignored. Maybe
QEMU needs something like -Werror for stuff like this, i.e.

 -cpu rv64,error-on-extension-warnings=on,profile-A=on,extension-of-A=off

would error out, but, without the special property, just warn. Or, flip
the default behavior around with

 -cpu rv64,ignore-extension-errors=on,profile-A=on,extension-of-A=off

which would either silently proceed or just warn, but, without the
special property, error out. libvirt would default to the error out
case, whichever that one is, but also provide an element to turn off
erroring-out.

Thanks,
drew

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrea Bolognani]

On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote:
> On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
> > On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> > > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > > > On 10/23/23 05:16, Andrew Jones wrote:
> > > > > Hmm, I'm not sure I agree with special-casing profiles like this. I think
> > > > > the left-to-right processing should be consistent for all. I'm also not
> > > > > sure we should always warn when disabling a profile. For example, if a
> > > > > user does
> > > > >
> > > > >   -cpu rv64,rva22u64=true,rva22u64=false
> > > > >
> > > > > then they'll get a warning, even though all they're doing is restoring the
> > > > > cpu model. While that looks like an odd thing to do, a script may be
> > > > > adding the rva22u64=true and the rva22u64=false is the user input which
> > > > > undoes what the script did.
> > > >
> > > > QEMU options do not work with a "the user enabled then disabled the same option,
> > > > thus it'll count as nothing happened" logic. The last instance of the option will
> > > > overwrite all previous instances. In the example you mentioned above the user would
> > > > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
> > > > same profile was enabled beforehand.
> > >
> > > Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> > > a base cpu type. If you start with nothing (a base) and then add a profile
> > > and take the same one away, you shouldn't be taking away anything else. I
> > > agree that if you use a profile on some cpu type that already enabled a
> > > bunch of stuff itself, then disabling a profile would potentially remove
> > > some of those too, but mixing cpu types that have their own extensions and
> > > profiles seems like a great way to confuse oneself as to what extensions
> > > will be present.  IOW, we should be adding a base cpu type at the same
> > > time we're adding these profiles.
> >
> > The question that keep bouncing around my head is: why would we even
> > allow disabling profiles?
> >
> > It seems to me that it only makes things more complicated, and I
> > really can't see the use case for it.
> >
> > Enabling additional features on top of a profile? There's obvious
> > value in that, so that you can model hardware that implements
> > optional and proprietary extensions. Enabling multiple profiles?
> > You've convinced me that it's useful. But disabling profiles, I just
> > don't see it. I believe Alistair was similarly unconvinced.
>
> The only value I see in allowing a profile to be disabled is to undo the
> enabling of the profile by specifying the profile as 'off' to the right of
> it being specified as 'on'. That may seem pointless, but scripts take
> advantage of being able to do that. Besides that one possible use case,
> there isn't much use in disabling profiles, but treating profile
> properties like every other boolean property makes the UI consistent and
> should actually simplify the code.

The code might be simpler, but the result is an additional burden on
the user, as the interactions between the various flags become much
more nuanced and less intuitive. I'm not convinced the trade-off is a
worthwhile one.

For the script override scenario, fair enough, but once again I feel
that we're making things much worse in the general case in order to
cater to a much narrower one. Script authors will naturally learn to
avoid hardcoding profile enablement once users have reported enough
failures resulting from that.

> > > > > As far as warnings go, it'd be nice to warn when mandatory profile
> > > > > extensions are disabled from an enabled profile. Doing that might be
> > > > > useful for debug, but users which do it without being aware they're
> > > > > "breaking" the profile may learn from that warning. Note, the warning
> > > > > should only come when the profile is actually enabled and when the
> > > > > extension would actually be disabled, i.e.
> > > > >
> > > > >   -cpu rv64,rva22u64=true,c=off
> > > > >
> > > > > should warn
> > > > >
> > > > >   -cpu rv64,c=off,rva22u64=true
> > > > >
> > > > > should not warn (rva22u64 overrides c=off since it's to the right)
> > > > >
> > > > >   -cpu rv64,rva22u64=true,rva22u64=false,c=off
> > > > >
> > > > > should not warn (rva22u64 is not enabled)
> >
> > I think these should be hard errors, not warnings.
> >
> > If you're enabling a profile and then disabling an extension that's
> > mandatory for that profile, you've invalidated the profile. You've
> > asked for a configuration that doesn't make any sense: you can't have
> > a CPU that both implements a profile and lacks one of its mandatory
> > extensions.
>
> Given a platform which implements a profile which mandates extension E and
> a need to debug E or test behavior where E is [incorrectly] absent, you'll
> need to expand the profile first, listing each of the other extensions
> manually. It'd be much faster to specify the profile, take away the
> extension, and ignore the warning.

I understand the appeal, I just think that regular users should be
prevented from stumbling into this kind of expert-level,
intentionally-broken configuration by mistake.

> > QEMU users could easily miss the warning. libvirt users won't see it
> > at all. It's a user error and it needs to be treated as such IMO.
>
> I do agree with the concern that warnings will be missed/ignored. Maybe
> QEMU needs something like -Werror for stuff like this, i.e.
>
>  -cpu rv64,error-on-extension-warnings=on,profile-A=on,extension-of-A=off
>
> would error out, but, without the special property, just warn. Or, flip
> the default behavior around with
>
>  -cpu rv64,ignore-extension-errors=on,profile-A=on,extension-of-A=off
>
> which would either silently proceed or just warn, but, without the
> special property, error out. libvirt would default to the error out
> case, whichever that one is, but also provide an element to turn off
> erroring-out.

I would be okay with something along these lines.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/26/23 14:36, Andrea Bolognani wrote:
> On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote:
>> On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
>>> On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
>>>> On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
>>>>> On 10/23/23 05:16, Andrew Jones wrote:
>>>>>> Hmm, I'm not sure I agree with special-casing profiles like this. I think
>>>>>> the left-to-right processing should be consistent for all. I'm also not
>>>>>> sure we should always warn when disabling a profile. For example, if a
>>>>>> user does
>>>>>>
>>>>>>    -cpu rv64,rva22u64=true,rva22u64=false
>>>>>>
>>>>>> then they'll get a warning, even though all they're doing is restoring the
>>>>>> cpu model. While that looks like an odd thing to do, a script may be
>>>>>> adding the rva22u64=true and the rva22u64=false is the user input which
>>>>>> undoes what the script did.
>>>>>
>>>>> QEMU options do not work with a "the user enabled then disabled the same option,
>>>>> thus it'll count as nothing happened" logic. The last instance of the option will
>>>>> overwrite all previous instances. In the example you mentioned above the user would
>>>>> disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
>>>>> same profile was enabled beforehand.
>>>>
>>>> Yup, I'm aware, but I keep thinking that we'll only be using profiles with
>>>> a base cpu type. If you start with nothing (a base) and then add a profile
>>>> and take the same one away, you shouldn't be taking away anything else. I
>>>> agree that if you use a profile on some cpu type that already enabled a
>>>> bunch of stuff itself, then disabling a profile would potentially remove
>>>> some of those too, but mixing cpu types that have their own extensions and
>>>> profiles seems like a great way to confuse oneself as to what extensions
>>>> will be present.  IOW, we should be adding a base cpu type at the same
>>>> time we're adding these profiles.
>>>
>>> The question that keep bouncing around my head is: why would we even
>>> allow disabling profiles?
>>>
>>> It seems to me that it only makes things more complicated, and I
>>> really can't see the use case for it.
>>>
>>> Enabling additional features on top of a profile? There's obvious
>>> value in that, so that you can model hardware that implements
>>> optional and proprietary extensions. Enabling multiple profiles?
>>> You've convinced me that it's useful. But disabling profiles, I just
>>> don't see it. I believe Alistair was similarly unconvinced.
>>
>> The only value I see in allowing a profile to be disabled is to undo the
>> enabling of the profile by specifying the profile as 'off' to the right of
>> it being specified as 'on'. That may seem pointless, but scripts take
>> advantage of being able to do that. Besides that one possible use case,
>> there isn't much use in disabling profiles, but treating profile
>> properties like every other boolean property makes the UI consistent and
>> should actually simplify the code.
> 
> The code might be simpler, but the result is an additional burden on
> the user, as the interactions between the various flags become much
> more nuanced and less intuitive. I'm not convinced the trade-off is a
> worthwhile one.
> 
> For the script override scenario, fair enough, but once again I feel
> that we're making things much worse in the general case in order to
> cater to a much narrower one. Script authors will naturally learn to
> avoid hardcoding profile enablement once users have reported enough
> failures resulting from that.

I'm not thrilled about how we're able to disable profiles either. I'm
coping with it because (1) it was a feedback from the first version of
this work [1] and no one had strong opinions against it back then and
(2) I believe that users won't find much use in doing "-cpu rv64,profileA=false"
in a real world/common scenario, so we can get away with this kind of
weird functionality.

The profile flag is set to 'false' by default for all current CPUs. If
the user manually sets it to 'false', well, it doesn't change the internal
state of the CPU, does it? But then I need to be creative and interpret it
as 'it's not a default false, it's an user-set false, so I need to disable
extensions'. I can't think of many qemu options that behave like that, if
any.

We also have the example of RVG, a bit that is default set to 'false' that,
when enabled, causes IMAFD_zicsr_zifencei to be enabled. Today, if the user
set RVG to 'false', nothing happens - we're not disabling IMAFD_zicsr_zifencei.
In the latest version of this work there's a deliberate effort to make RVG
behave like a profile [2], but perhaps I should make profiles behave like RVG.

Last but not the least, I'm planning to add a couple of bare-bones CPUs (rv64i
and rv64e). Disabling profiles in these CPUs is a total waste of cycles since
the CPUs are already bare.

After writing all this stuff, and realizing that profile disablement creates a
lot of confusion and has no vocal fans, I had a change of heart. Profiles will
behave like RVG -> if set, mandatory extensions will be enabled (respecting user
choice on disabled extensions, of course). If disabled, nothing happens. Fans
of the current design are welcome to weight in the discussion, of course.

If we decide in the future that stripping extensions from a CPU model is desirable
we can come up with a 'bare' option, e.g. "-cpu rv64,bare=true" will strip all
extensions from rv64. This is a much cleaner way of doing what profile disablement
is currently doing.


Thanks,


Daniel


[1] https://lore.kernel.org/qemu-riscv/ZRarBuEeBi7WkS6K@redhat.com/
[2] https://lore.kernel.org/qemu-riscv/20231025234459.581697-10-dbarboza@ventanamicro.com/






> 
>>>>>> As far as warnings go, it'd be nice to warn when mandatory profile
>>>>>> extensions are disabled from an enabled profile. Doing that might be
>>>>>> useful for debug, but users which do it without being aware they're
>>>>>> "breaking" the profile may learn from that warning. Note, the warning
>>>>>> should only come when the profile is actually enabled and when the
>>>>>> extension would actually be disabled, i.e.
>>>>>>
>>>>>>    -cpu rv64,rva22u64=true,c=off
>>>>>>
>>>>>> should warn
>>>>>>
>>>>>>    -cpu rv64,c=off,rva22u64=true
>>>>>>
>>>>>> should not warn (rva22u64 overrides c=off since it's to the right)
>>>>>>
>>>>>>    -cpu rv64,rva22u64=true,rva22u64=false,c=off
>>>>>>
>>>>>> should not warn (rva22u64 is not enabled)
>>>
>>> I think these should be hard errors, not warnings.
>>>
>>> If you're enabling a profile and then disabling an extension that's
>>> mandatory for that profile, you've invalidated the profile. You've
>>> asked for a configuration that doesn't make any sense: you can't have
>>> a CPU that both implements a profile and lacks one of its mandatory
>>> extensions.
>>
>> Given a platform which implements a profile which mandates extension E and
>> a need to debug E or test behavior where E is [incorrectly] absent, you'll
>> need to expand the profile first, listing each of the other extensions
>> manually. It'd be much faster to specify the profile, take away the
>> extension, and ignore the warning.
> 
> I understand the appeal, I just think that regular users should be
> prevented from stumbling into this kind of expert-level,
> intentionally-broken configuration by mistake.
> 
>>> QEMU users could easily miss the warning. libvirt users won't see it
>>> at all. It's a user error and it needs to be treated as such IMO.
>>
>> I do agree with the concern that warnings will be missed/ignored. Maybe
>> QEMU needs something like -Werror for stuff like this, i.e.
>>
>>   -cpu rv64,error-on-extension-warnings=on,profile-A=on,extension-of-A=off
>>
>> would error out, but, without the special property, just warn. Or, flip
>> the default behavior around with
>>
>>   -cpu rv64,ignore-extension-errors=on,profile-A=on,extension-of-A=off
>>
>> which would either silently proceed or just warn, but, without the
>> special property, error out. libvirt would default to the error out
>> case, whichever that one is, but also provide an element to turn off
>> erroring-out.
> 
> I would be okay with something along these lines.
> 

Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Andrew Jones]

On Fri, Oct 27, 2023 at 02:52:38PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 10/26/23 14:36, Andrea Bolognani wrote:
> > On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote:
> > > On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
> > > > On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> > > > > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > > > > > On 10/23/23 05:16, Andrew Jones wrote:
> > > > > > > Hmm, I'm not sure I agree with special-casing profiles like this. I think
> > > > > > > the left-to-right processing should be consistent for all. I'm also not
> > > > > > > sure we should always warn when disabling a profile. For example, if a
> > > > > > > user does
> > > > > > > 
> > > > > > >    -cpu rv64,rva22u64=true,rva22u64=false
> > > > > > > 
> > > > > > > then they'll get a warning, even though all they're doing is restoring the
> > > > > > > cpu model. While that looks like an odd thing to do, a script may be
> > > > > > > adding the rva22u64=true and the rva22u64=false is the user input which
> > > > > > > undoes what the script did.
> > > > > > 
> > > > > > QEMU options do not work with a "the user enabled then disabled the same option,
> > > > > > thus it'll count as nothing happened" logic. The last instance of the option will
> > > > > > overwrite all previous instances. In the example you mentioned above the user would
> > > > > > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
> > > > > > same profile was enabled beforehand.
> > > > > 
> > > > > Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> > > > > a base cpu type. If you start with nothing (a base) and then add a profile
> > > > > and take the same one away, you shouldn't be taking away anything else. I
> > > > > agree that if you use a profile on some cpu type that already enabled a
> > > > > bunch of stuff itself, then disabling a profile would potentially remove
> > > > > some of those too, but mixing cpu types that have their own extensions and
> > > > > profiles seems like a great way to confuse oneself as to what extensions
> > > > > will be present.  IOW, we should be adding a base cpu type at the same
> > > > > time we're adding these profiles.
> > > > 
> > > > The question that keep bouncing around my head is: why would we even
> > > > allow disabling profiles?
> > > > 
> > > > It seems to me that it only makes things more complicated, and I
> > > > really can't see the use case for it.
> > > > 
> > > > Enabling additional features on top of a profile? There's obvious
> > > > value in that, so that you can model hardware that implements
> > > > optional and proprietary extensions. Enabling multiple profiles?
> > > > You've convinced me that it's useful. But disabling profiles, I just
> > > > don't see it. I believe Alistair was similarly unconvinced.
> > > 
> > > The only value I see in allowing a profile to be disabled is to undo the
> > > enabling of the profile by specifying the profile as 'off' to the right of
> > > it being specified as 'on'. That may seem pointless, but scripts take
> > > advantage of being able to do that. Besides that one possible use case,
> > > there isn't much use in disabling profiles, but treating profile
> > > properties like every other boolean property makes the UI consistent and
> > > should actually simplify the code.
> > 
> > The code might be simpler, but the result is an additional burden on
> > the user, as the interactions between the various flags become much
> > more nuanced and less intuitive. I'm not convinced the trade-off is a
> > worthwhile one.
> > 
> > For the script override scenario, fair enough, but once again I feel
> > that we're making things much worse in the general case in order to
> > cater to a much narrower one. Script authors will naturally learn to
> > avoid hardcoding profile enablement once users have reported enough
> > failures resulting from that.
> 
> I'm not thrilled about how we're able to disable profiles either. I'm
> coping with it because (1) it was a feedback from the first version of
> this work [1] and no one had strong opinions against it back then and
> (2) I believe that users won't find much use in doing "-cpu rv64,profileA=false"
> in a real world/common scenario, so we can get away with this kind of
> weird functionality.
> 
> The profile flag is set to 'false' by default for all current CPUs. If
> the user manually sets it to 'false', well, it doesn't change the internal
> state of the CPU, does it? But then I need to be creative and interpret it
> as 'it's not a default false, it's an user-set false, so I need to disable
> extensions'. I can't think of many qemu options that behave like that, if
> any.

If we provided a mechanism to query the state of profiles, then they
should be 'true' when all their mandatory extensions are present. That
means that, despite defaulting to 'false', they should become true at
realize time after detecting all their mandatory extensions are present,
and then, if a user set the profile false, the user would be disabling
all the profile's extensions along with the profile.

Without a mechanism to query profiles, then it doesn't matter if they
remain false after realize, even if they should be true, but that's just
a coding decision (which probably deserves a code comment).

> 
> We also have the example of RVG, a bit that is default set to 'false' that,
> when enabled, causes IMAFD_zicsr_zifencei to be enabled. Today, if the user
> set RVG to 'false', nothing happens - we're not disabling IMAFD_zicsr_zifencei.

I would disable the extensions that G represents when G is explicitly
disabled, just as I suggest we do for profiles.

> In the latest version of this work there's a deliberate effort to make RVG
> behave like a profile [2], but perhaps I should make profiles behave like RVG.

More extension shorthands are coming[1] and, IMO, they should be addressed
in the same way as G (whatever the final decision on how to address G is).
Since G collects such basic extensions, then it might be hard to reason as
to why it should be possible to disable it, but if, for example, you have
a CPU which supports the Zvkn extensions and you want to test/debug with
all of those extensions disabled, then it'd be nice to do zvkn=off and
expect all of them to be disabled.

[1] https://lore.kernel.org/all/20231026151828.754279-1-max.chou@sifive.com/

> 
> Last but not the least, I'm planning to add a couple of bare-bones CPUs (rv64i
> and rv64e). Disabling profiles in these CPUs is a total waste of cycles since
> the CPUs are already bare.

I've already pointed out a couple times that disabling a profile may be
useful for scripts, particularly for bare-bones CPUs, since you know that
disabling something previously enabled is implementing a clean 'undo'.

> 
> After writing all this stuff, and realizing that profile disablement creates a
> lot of confusion

I'm having trouble imaging this hypothetical user that knows how to
construction QEMU command lines, knows what profiles are meant to enable,
knows how boolean properties work, but would be surprised that
'profile=off' disabled the profile's mandatory extensions and not
surprised when 'profile=off' does nothing. So, for me, 'profile=off' doing
nothing looks like it'll introduce confusion, not the other way around.

> and has no vocal fans, I had a change of heart. Profiles will
> behave like RVG -> if set, mandatory extensions will be enabled (respecting user
> choice on disabled extensions, of course).

I presume the user disabled extensions would need to come to the right of
the profiles on the command line to be respected.

> If disabled, nothing happens. Fans
> of the current design are welcome to weight in the discussion, of course.
> 
> If we decide in the future that stripping extensions from a CPU model is desirable
> we can come up with a 'bare' option, e.g. "-cpu rv64,bare=true" will strip all
> extensions from rv64. This is a much cleaner way of doing what profile disablement
> is currently doing.

Doesn't this just kick the can down the road? I suspect we'd end up
discussing what 'bare=false' means at that time.

Thanks,
drew