[PATCH v2 00/10] riscv: RVA22U64 profile support

[PATCH v2 00/10] riscv: RVA22U64 profile support

[Daniel Henrique Barboza]

Hi,

Several design changes were made in this version after the reviews and
feedback in the v1 [1]. The high-level summary is:

- we'll no longer allow users to set profile flags for vendor CPUs. If
  we're to adhere to the current policy of not allowing users to enable
  extensions for vendor CPUs, the profile support would become a
  glorified way of checking if the vendor CPU happens to support a
  specific profile. If a future vendor CPU supports a profile the CPU
  can declare it manually in its cpu_init() function, the flag will
  still be set, but users can't change it;

- disabling a profile will now disable all the mandatory extensions from
  the CPU;

- the profile logic was moved to realize() time in a step we're calling
  'commit profile'. This allows us to enable/disable profile extensions
  after considering user input in other individual extensions. The
  result is that we don't care about the order in which the profile flag
  was set in comparison with other extensions in the command line, i.e.
  the following lines are equal:

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

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

  and they mean 'enable the rva22u64 profile while keeping zicbom and
  Zifencei disabled'.


Other minor changes were needed as result of these design changes. E.g.
we're now having to track MISA extensions set by users (patch 7),
something that we were doing only for multi-letter extensions.

Changes from v1:
- patch 6 from v1 ("target/riscv/kvm: add 'rva22u64' flag as unavailable"):
    - moved up to patch 4
- patch 5 from v1("target/riscv/tcg-cpu.c: enable profile support for vendor CPUs"):
    - dropped
- patch 6 (new):
  - add riscv_cpu_commit_profile()
- patch 7 (new):
  - add user choice hash for MISA extensions
- patch 9 (new):
  - handle MISA bits user choice when commiting profiles
- patch 8 and 10 (new):
  - helpers to avoid code repetition
- v1 link: https://lore.kernel.org/qemu-riscv/20230926194951.183767-1-dbarboza@ventanamicro.com/


Daniel Henrique Barboza (10):
  target/riscv/cpu.c: add zicntr extension flag
  target/riscv/cpu.c: add zihpm extension flag
  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: commit profiles during realize()
  target/riscv/tcg: add MISA user options hash
  target/riscv/tcg: add riscv_cpu_write_misa_bit()
  target/riscv/tcg: handle MISA bits on profile commit
  target/riscv/tcg: add hash table insert helpers

 target/riscv/cpu.c         |  29 +++++++
 target/riscv/cpu.h         |  12 +++
 target/riscv/cpu_cfg.h     |   2 +
 target/riscv/kvm/kvm-cpu.c |   7 +-
 target/riscv/tcg/tcg-cpu.c | 165 +++++++++++++++++++++++++++++++++----
 5 files changed, 197 insertions(+), 18 deletions(-)

-- 
2.41.0

[PATCH v2 01/10] target/riscv/cpu.c: add zicntr extension flag

[Daniel Henrique Barboza]

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

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

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

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

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

Re: [PATCH v2 01/10] target/riscv/cpu.c: add zicntr extension flag

[Alistair Francis]

On Fri, Oct 6, 2023 at 11:23 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> zicntr is the Base Counters and Timers extension described in chapter 12
> of the unprivileged spec. It describes support for RDCYCLE, RDTIME and
> RDINSTRET.
>
> QEMU already implements it way before it was a discrete extension.
> zicntr is part of the RVA22 profile, so let's add it to QEMU to make the
> future profile implementation flag complete.
>
> Given than it represents an already existing feature, default it to
> 'true'. Change the realize() time validation to disable it in case its
> dependency (icsr) isn't present.

What happens if a user disables this though?

Alistair

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

Re: [PATCH v2 01/10] target/riscv/cpu.c: add zicntr extension flag

[Daniel Henrique Barboza]

On 10/10/23 23:51, Alistair Francis wrote:
> On Fri, Oct 6, 2023 at 11:23 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> zicntr is the Base Counters and Timers extension described in chapter 12
>> of the unprivileged spec. It describes support for RDCYCLE, RDTIME and
>> RDINSTRET.
>>
>> QEMU already implements it way before it was a discrete extension.
>> zicntr is part of the RVA22 profile, so let's add it to QEMU to make the
>> future profile implementation flag complete.
>>
>> Given than it represents an already existing feature, default it to
>> 'true'. Change the realize() time validation to disable it in case its
>> dependency (icsr) isn't present.
> 
> What happens if a user disables this though?


Hmmmm ... nothing hehe

I think I'll have to go back to the drawing board with this and the next patch.
Both zicntr and zihpm are regular extensions and should be treated as such, meaning
that we should be able to disable what they're doing.

To avoid breaking what we currently have we'll make them always 'true' by default,
for all current CPUs, and then users are free to disable them.



Thanks,

Daniel

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

[PATCH v2 02/10] target/riscv/cpu.c: add zihpm extension flag

[Daniel Henrique Barboza]

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

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

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

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

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

[PATCH v2 03/10] target/riscv: add rva22u64 profile definition

[Daniel Henrique Barboza]

The rva22U64 profile, described in:

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

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

QEMU implements all possible extensions of this profile. The exception
is Zicbop (Cache-Block Prefetch Operations) that is not available since
QEMU RISC-V does not implement a cache model. For this same reason all
the so called 'synthetic extensions' described in the profile that are
cache related are ignored (Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa,
Zicclsm).

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

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

Next patches will implement the profile for TCG and KVM.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 20 ++++++++++++++++++++
 target/riscv/cpu.h | 12 ++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b3befccf89..a439ff57a4 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1376,6 +1376,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_icsr), CPU_CFG_OFFSET(ext_zihintpause),
+        CPU_CFG_OFFSET(ext_zba), CPU_CFG_OFFSET(ext_zbb),
+        CPU_CFG_OFFSET(ext_zbs), CPU_CFG_OFFSET(ext_zfhmin),
+        CPU_CFG_OFFSET(ext_zkt), CPU_CFG_OFFSET(ext_icntr),
+        CPU_CFG_OFFSET(ext_ihpm), CPU_CFG_OFFSET(ext_icbom),
+        CPU_CFG_OFFSET(ext_icboz),
+
+        RISCV_PROFILE_EXT_LIST_END
+    }
+};
+
+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 3f11e69223..216bbbe7cd 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

Re: [PATCH v2 03/10] target/riscv: add rva22u64 profile definition

[Alistair Francis]

On Fri, Oct 6, 2023 at 11:23 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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>

Alistair

> ---
>  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 b3befccf89..a439ff57a4 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1376,6 +1376,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_icsr), CPU_CFG_OFFSET(ext_zihintpause),
> +        CPU_CFG_OFFSET(ext_zba), CPU_CFG_OFFSET(ext_zbb),
> +        CPU_CFG_OFFSET(ext_zbs), CPU_CFG_OFFSET(ext_zfhmin),
> +        CPU_CFG_OFFSET(ext_zkt), CPU_CFG_OFFSET(ext_icntr),
> +        CPU_CFG_OFFSET(ext_ihpm), CPU_CFG_OFFSET(ext_icbom),
> +        CPU_CFG_OFFSET(ext_icboz),
> +
> +        RISCV_PROFILE_EXT_LIST_END
> +    }
> +};
> +
> +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 3f11e69223..216bbbe7cd 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 v2 04/10] target/riscv/kvm: add 'rva22u64' flag as unavailable

[Daniel Henrique Barboza]

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

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

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

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

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

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 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 c6615cb807..5f563b83df 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -358,7 +358,7 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
     }
 
     if (value) {
-        error_setg(errp, "extension %s is not available with KVM",
+        error_setg(errp, "'%s' is not available with KVM",
                    propname);
     }
 }
@@ -438,6 +438,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

Re: [PATCH v2 04/10] target/riscv/kvm: add 'rva22u64' flag as unavailable

[Alistair Francis]

On Fri, Oct 6, 2023 at 11:24 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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>

Alistair

> ---
>  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 c6615cb807..5f563b83df 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -358,7 +358,7 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
>      }
>
>      if (value) {
> -        error_setg(errp, "extension %s is not available with KVM",
> +        error_setg(errp, "'%s' is not available with KVM",
>                     propname);
>      }
>  }
> @@ -438,6 +438,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 v2 05/10] 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;

- the set() callback for the profile user property will set the
  'user_set' flag for each profile that users enable/disable in the
  command line;

- 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.

For now we'll just expose the user flags for all profiles. Next patch
will introduce the 'commit profile' logic.

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 731192bafc..a8ea869e6e 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -740,6 +740,50 @@ 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;
+    bool value;
+
+    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;
+    }
+
+    profile->user_set = true;
+    profile->enabled = value;
+}
+
+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;; i++) {
+        const RISCVCPUProfile *profile = riscv_profiles[i];
+
+        if (!profile) {
+            break;
+        }
+
+        object_property_add(cpu_obj, profile->name, "bool",
+                            cpu_get_profile, cpu_set_profile,
+                            NULL, (void *)profile);
+    }
+}
+
 static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
                                   void *opaque, Error **errp)
 {
@@ -834,6 +878,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_vendor_exts);
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_experimental_exts);
 
+    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 v2 06/10] target/riscv/tcg: commit profiles during realize()

[Daniel Henrique Barboza]

To 'commit' a profile means enabling/disabling all its mandatory
extensions after taking into account individual user choice w.r.t MISA
and multi-letter extensions. We'll handle multi-letter extennsions now -
MISA extensions needs additional steps that we'll take care later.

riscv_cpu_manage_profiles() will scroll through all profiles available
in QEMU and call riscv_cpu_commit_profile() for any profile that the
user set, either to 'true' or 'false'.

Setting a profile to 'true' means 'enable all mandatory extensions of
this profile'. Setting it to 'false' means disabling all its mandatory
extensions. Since we're doing it during realize() time we already have
all user choices for individual extensions sorted out, and they'll take
precedence. 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".

Enabling extensions in the profile is also considered an user choice, so
all extensions enabled will be added in the multi_ext_user_opts hash.

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index a8ea869e6e..8fb77e9e35 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -264,6 +264,41 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
     }
 }
 
+static void riscv_cpu_commit_profile(RISCVCPU *cpu, RISCVCPUProfile *profile)
+{
+    int i;
+
+    for (i = 0;; i++) {
+        int ext_offset = profile->ext_offsets[i];
+
+        if (ext_offset == RISCV_PROFILE_EXT_LIST_END) {
+            break;
+        }
+
+        if (cpu_cfg_ext_is_user_set(ext_offset)) {
+            continue;
+        }
+
+        g_hash_table_insert(multi_ext_user_opts,
+                            GUINT_TO_POINTER(ext_offset),
+                            (gpointer)profile->enabled);
+        isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
+    }
+}
+
+static void riscv_cpu_manage_profiles(RISCVCPU *cpu)
+{
+    for (int i = 0; riscv_profiles[i] != NULL; i++) {
+        RISCVCPUProfile *profile = riscv_profiles[i];
+
+        if (!profile->user_set) {
+            continue;
+        }
+
+        riscv_cpu_commit_profile(cpu, profile);
+    }
+}
+
 /*
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
@@ -273,6 +308,8 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     CPURISCVState *env = &cpu->env;
     Error *local_err = NULL;
 
+    riscv_cpu_manage_profiles(cpu);
+
     /* Do some ISA extension error checking */
     if (riscv_has_ext(env, RVG) &&
         !(riscv_has_ext(env, RVI) && riscv_has_ext(env, RVM) &&
-- 
2.41.0

[PATCH v2 07/10] 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>
---
 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 8fb77e9e35..58de4428a9 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)
 {
@@ -689,6 +690,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) {
@@ -752,6 +757,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;
 
@@ -772,7 +778,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;
+            }
         }
     }
 }
@@ -967,6 +979,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

Re: [PATCH v2 07/10] target/riscv/tcg: add MISA user options hash

[Alistair Francis]

On Sat, Oct 7, 2023 at 12:25 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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>

Alistair

> ---
>  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 8fb77e9e35..58de4428a9 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)
>  {
> @@ -689,6 +690,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) {
> @@ -752,6 +757,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;
>
> @@ -772,7 +778,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;
> +            }
>          }
>      }
>  }
> @@ -967,6 +979,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 v2 08/10] 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>
---
 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 58de4428a9..b1e778913c 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)
 {
@@ -700,20 +714,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,
@@ -757,7 +765,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;
 
@@ -778,13 +785,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

Re: [PATCH v2 08/10] target/riscv/tcg: add riscv_cpu_write_misa_bit()

[Alistair Francis]

On Sat, Oct 7, 2023 at 12:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> 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>

Alistair

> ---
>  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 58de4428a9..b1e778913c 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)
>  {
> @@ -700,20 +714,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,
> @@ -757,7 +765,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;
>
> @@ -778,13 +785,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 v2 09/10] target/riscv/tcg: handle MISA bits on profile commit

[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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index b1e778913c..d7540274f4 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)
 {
@@ -283,6 +289,20 @@ static void riscv_cpu_commit_profile(RISCVCPU *cpu, RISCVCPUProfile *profile)
 {
     int i;
 
+    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;
+        }
+
+        g_hash_table_insert(misa_ext_user_opts,
+                            GUINT_TO_POINTER(bit),
+                            (gpointer)profile->enabled);
+
+        riscv_cpu_write_misa_bit(cpu, bit, profile->enabled);
+    }
+
     for (i = 0;; i++) {
         int ext_offset = profile->ext_offsets[i];
 
-- 
2.41.0

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

[Daniel Henrique Barboza]

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

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index d7540274f4..d4ad1c09b3 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -42,12 +42,24 @@ static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
                                  GUINT_TO_POINTER(ext_offset));
 }
 
+static void cpu_cfg_ext_add_user_opt(uint32_t ext_offset, bool value)
+{
+    g_hash_table_insert(multi_ext_user_opts, GUINT_TO_POINTER(ext_offset),
+                        (gpointer)value);
+}
+
 static bool cpu_misa_ext_is_user_set(uint32_t misa_bit)
 {
     return g_hash_table_contains(misa_ext_user_opts,
                                  GUINT_TO_POINTER(misa_bit));
 }
 
+static void cpu_misa_ext_add_user_opt(uint32_t bit, bool value)
+{
+    g_hash_table_insert(misa_ext_user_opts, GUINT_TO_POINTER(bit),
+                        (gpointer)value);
+}
+
 static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t bit,
                                      bool enabled)
 {
@@ -296,9 +308,7 @@ static void riscv_cpu_commit_profile(RISCVCPU *cpu, RISCVCPUProfile *profile)
             continue;
         }
 
-        g_hash_table_insert(misa_ext_user_opts,
-                            GUINT_TO_POINTER(bit),
-                            (gpointer)profile->enabled);
+        cpu_misa_ext_add_user_opt(bit, profile->enabled);
 
         riscv_cpu_write_misa_bit(cpu, bit, profile->enabled);
     }
@@ -314,9 +324,8 @@ static void riscv_cpu_commit_profile(RISCVCPU *cpu, RISCVCPUProfile *profile)
             continue;
         }
 
-        g_hash_table_insert(multi_ext_user_opts,
-                            GUINT_TO_POINTER(ext_offset),
-                            (gpointer)profile->enabled);
+        cpu_cfg_ext_add_user_opt(ext_offset, profile->enabled);
+
         isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
     }
 }
@@ -724,9 +733,7 @@ static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    g_hash_table_insert(misa_ext_user_opts,
-                        GUINT_TO_POINTER(misa_bit),
-                        (gpointer)value);
+    cpu_misa_ext_add_user_opt(misa_bit, value);
 
     prev_val = env->misa_ext & misa_bit;
 
@@ -867,9 +874,7 @@ static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    g_hash_table_insert(multi_ext_user_opts,
-                        GUINT_TO_POINTER(multi_ext_cfg->offset),
-                        (gpointer)value);
+    cpu_cfg_ext_add_user_opt(multi_ext_cfg->offset, value);
 
     prev_val = isa_ext_is_enabled(cpu, multi_ext_cfg->offset);
 
-- 
2.41.0

Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Alistair Francis]

On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> Several design changes were made in this version after the reviews and
> feedback in the v1 [1]. The high-level summary is:
>
> - we'll no longer allow users to set profile flags for vendor CPUs. If
>   we're to adhere to the current policy of not allowing users to enable
>   extensions for vendor CPUs, the profile support would become a
>   glorified way of checking if the vendor CPU happens to support a
>   specific profile. If a future vendor CPU supports a profile the CPU
>   can declare it manually in its cpu_init() function, the flag will
>   still be set, but users can't change it;
>
> - disabling a profile will now disable all the mandatory extensions from
>   the CPU;

What happens if you enable one profile and disable a different one?

Alistair

>
> - the profile logic was moved to realize() time in a step we're calling
>   'commit profile'. This allows us to enable/disable profile extensions
>   after considering user input in other individual extensions. The
>   result is that we don't care about the order in which the profile flag
>   was set in comparison with other extensions in the command line, i.e.
>   the following lines are equal:
>
>   -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
>
>   -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
>
>   and they mean 'enable the rva22u64 profile while keeping zicbom and
>   Zifencei disabled'.
>
>
> Other minor changes were needed as result of these design changes. E.g.
> we're now having to track MISA extensions set by users (patch 7),
> something that we were doing only for multi-letter extensions.
>
> Changes from v1:
> - patch 6 from v1 ("target/riscv/kvm: add 'rva22u64' flag as unavailable"):
>     - moved up to patch 4
> - patch 5 from v1("target/riscv/tcg-cpu.c: enable profile support for vendor CPUs"):
>     - dropped
> - patch 6 (new):
>   - add riscv_cpu_commit_profile()
> - patch 7 (new):
>   - add user choice hash for MISA extensions
> - patch 9 (new):
>   - handle MISA bits user choice when commiting profiles
> - patch 8 and 10 (new):
>   - helpers to avoid code repetition
> - v1 link: https://lore.kernel.org/qemu-riscv/20230926194951.183767-1-dbarboza@ventanamicro.com/
>
>
> Daniel Henrique Barboza (10):
>   target/riscv/cpu.c: add zicntr extension flag
>   target/riscv/cpu.c: add zihpm extension flag
>   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: commit profiles during realize()
>   target/riscv/tcg: add MISA user options hash
>   target/riscv/tcg: add riscv_cpu_write_misa_bit()
>   target/riscv/tcg: handle MISA bits on profile commit
>   target/riscv/tcg: add hash table insert helpers
>
>  target/riscv/cpu.c         |  29 +++++++
>  target/riscv/cpu.h         |  12 +++
>  target/riscv/cpu_cfg.h     |   2 +
>  target/riscv/kvm/kvm-cpu.c |   7 +-
>  target/riscv/tcg/tcg-cpu.c | 165 +++++++++++++++++++++++++++++++++----
>  5 files changed, 197 insertions(+), 18 deletions(-)
>
> --
> 2.41.0
>
>

Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Daniel Henrique Barboza]

On 10/11/23 00:01, Alistair Francis wrote:
> On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> Hi,
>>
>> Several design changes were made in this version after the reviews and
>> feedback in the v1 [1]. The high-level summary is:
>>
>> - we'll no longer allow users to set profile flags for vendor CPUs. If
>>    we're to adhere to the current policy of not allowing users to enable
>>    extensions for vendor CPUs, the profile support would become a
>>    glorified way of checking if the vendor CPU happens to support a
>>    specific profile. If a future vendor CPU supports a profile the CPU
>>    can declare it manually in its cpu_init() function, the flag will
>>    still be set, but users can't change it;
>>
>> - disabling a profile will now disable all the mandatory extensions from
>>    the CPU;
> 
> What happens if you enable one profile and disable a different one?

With this implementation as is the profiles will be evaluated by the order they're
declared in riscv_cpu_profiles[]. Which isn't exactly ideal since we're exchanging
a left-to-right ordering in the command line by an arbitrary order that we happened
to set in the code.

I can make some tweaks to make the profiles sensible to left-to-right order between
them, while keeping regular extension with higher priority. e.g.:


-cpu rv64,zicbom=true,profileA=false,profileB=true,zicboz=false
-cpu rv64,profileA=false,zicbom=true,zicboz=false,profileB=true
-cpu rv64,profileA=false,profileB=true,zicbom=true,zicboz=false

These would all do the same thing: "keeping zicbom=true and zicboz=false, disable profileA
and then enable profile B"

Switching the profiles order would have a different result:

-cpu rv64,profileB=true,profileA=false,zicbom=true,zicboz=false

"keeping zicbom=true and zicboz=false, enable profile B and then disable profile A"


I'm happy to hear any other alternative/ideas. We'll either deal with some left-to-right
ordering w.r.t profiles or deal with an internal profile commit ordering. TBH I think
it's sensible to demand left-to-right command line ordering for profiles only.


Thanks,


Daniel





> 
> Alistair
> 
>>
>> - the profile logic was moved to realize() time in a step we're calling
>>    'commit profile'. This allows us to enable/disable profile extensions
>>    after considering user input in other individual extensions. The
>>    result is that we don't care about the order in which the profile flag
>>    was set in comparison with other extensions in the command line, i.e.
>>    the following lines are equal:
>>
>>    -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
>>
>>    -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
>>
>>    and they mean 'enable the rva22u64 profile while keeping zicbom and
>>    Zifencei disabled'.
>>
>>
>> Other minor changes were needed as result of these design changes. E.g.
>> we're now having to track MISA extensions set by users (patch 7),
>> something that we were doing only for multi-letter extensions.
>>
>> Changes from v1:
>> - patch 6 from v1 ("target/riscv/kvm: add 'rva22u64' flag as unavailable"):
>>      - moved up to patch 4
>> - patch 5 from v1("target/riscv/tcg-cpu.c: enable profile support for vendor CPUs"):
>>      - dropped
>> - patch 6 (new):
>>    - add riscv_cpu_commit_profile()
>> - patch 7 (new):
>>    - add user choice hash for MISA extensions
>> - patch 9 (new):
>>    - handle MISA bits user choice when commiting profiles
>> - patch 8 and 10 (new):
>>    - helpers to avoid code repetition
>> - v1 link: https://lore.kernel.org/qemu-riscv/20230926194951.183767-1-dbarboza@ventanamicro.com/
>>
>>
>> Daniel Henrique Barboza (10):
>>    target/riscv/cpu.c: add zicntr extension flag
>>    target/riscv/cpu.c: add zihpm extension flag
>>    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: commit profiles during realize()
>>    target/riscv/tcg: add MISA user options hash
>>    target/riscv/tcg: add riscv_cpu_write_misa_bit()
>>    target/riscv/tcg: handle MISA bits on profile commit
>>    target/riscv/tcg: add hash table insert helpers
>>
>>   target/riscv/cpu.c         |  29 +++++++
>>   target/riscv/cpu.h         |  12 +++
>>   target/riscv/cpu_cfg.h     |   2 +
>>   target/riscv/kvm/kvm-cpu.c |   7 +-
>>   target/riscv/tcg/tcg-cpu.c | 165 +++++++++++++++++++++++++++++++++----
>>   5 files changed, 197 insertions(+), 18 deletions(-)
>>
>> --
>> 2.41.0
>>
>>

Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Alistair Francis]

On Fri, Oct 13, 2023 at 5:07 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/11/23 00:01, Alistair Francis wrote:
> > On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> Hi,
> >>
> >> Several design changes were made in this version after the reviews and
> >> feedback in the v1 [1]. The high-level summary is:
> >>
> >> - we'll no longer allow users to set profile flags for vendor CPUs. If
> >>    we're to adhere to the current policy of not allowing users to enable
> >>    extensions for vendor CPUs, the profile support would become a
> >>    glorified way of checking if the vendor CPU happens to support a
> >>    specific profile. If a future vendor CPU supports a profile the CPU
> >>    can declare it manually in its cpu_init() function, the flag will
> >>    still be set, but users can't change it;
> >>
> >> - disabling a profile will now disable all the mandatory extensions from
> >>    the CPU;
> >
> > What happens if you enable one profile and disable a different one?
>
> With this implementation as is the profiles will be evaluated by the order they're
> declared in riscv_cpu_profiles[]. Which isn't exactly ideal since we're exchanging
> a left-to-right ordering in the command line by an arbitrary order that we happened
> to set in the code.
>
> I can make some tweaks to make the profiles sensible to left-to-right order between
> them, while keeping regular extension with higher priority. e.g.:
>
>
> -cpu rv64,zicbom=true,profileA=false,profileB=true,zicboz=false
> -cpu rv64,profileA=false,zicbom=true,zicboz=false,profileB=true
> -cpu rv64,profileA=false,profileB=true,zicbom=true,zicboz=false

I think we should just not allow this.

I don't understand why a user would want this and what they mean here.
What if profileA and profileB have overlapping extensions?

Alistair

>
> These would all do the same thing: "keeping zicbom=true and zicboz=false, disable profileA
> and then enable profile B"
>
> Switching the profiles order would have a different result:
>
> -cpu rv64,profileB=true,profileA=false,zicbom=true,zicboz=false
>
> "keeping zicbom=true and zicboz=false, enable profile B and then disable profile A"
>
>
> I'm happy to hear any other alternative/ideas. We'll either deal with some left-to-right
> ordering w.r.t profiles or deal with an internal profile commit ordering. TBH I think
> it's sensible to demand left-to-right command line ordering for profiles only.
>
>
> Thanks,
>
>
> Daniel
>
>
>
>
>
> >
> > Alistair
> >
> >>
> >> - the profile logic was moved to realize() time in a step we're calling
> >>    'commit profile'. This allows us to enable/disable profile extensions
> >>    after considering user input in other individual extensions. The
> >>    result is that we don't care about the order in which the profile flag
> >>    was set in comparison with other extensions in the command line, i.e.
> >>    the following lines are equal:
> >>
> >>    -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> >>
> >>    -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
> >>
> >>    and they mean 'enable the rva22u64 profile while keeping zicbom and
> >>    Zifencei disabled'.
> >>
> >>
> >> Other minor changes were needed as result of these design changes. E.g.
> >> we're now having to track MISA extensions set by users (patch 7),
> >> something that we were doing only for multi-letter extensions.
> >>
> >> Changes from v1:
> >> - patch 6 from v1 ("target/riscv/kvm: add 'rva22u64' flag as unavailable"):
> >>      - moved up to patch 4
> >> - patch 5 from v1("target/riscv/tcg-cpu.c: enable profile support for vendor CPUs"):
> >>      - dropped
> >> - patch 6 (new):
> >>    - add riscv_cpu_commit_profile()
> >> - patch 7 (new):
> >>    - add user choice hash for MISA extensions
> >> - patch 9 (new):
> >>    - handle MISA bits user choice when commiting profiles
> >> - patch 8 and 10 (new):
> >>    - helpers to avoid code repetition
> >> - v1 link: https://lore.kernel.org/qemu-riscv/20230926194951.183767-1-dbarboza@ventanamicro.com/
> >>
> >>
> >> Daniel Henrique Barboza (10):
> >>    target/riscv/cpu.c: add zicntr extension flag
> >>    target/riscv/cpu.c: add zihpm extension flag
> >>    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: commit profiles during realize()
> >>    target/riscv/tcg: add MISA user options hash
> >>    target/riscv/tcg: add riscv_cpu_write_misa_bit()
> >>    target/riscv/tcg: handle MISA bits on profile commit
> >>    target/riscv/tcg: add hash table insert helpers
> >>
> >>   target/riscv/cpu.c         |  29 +++++++
> >>   target/riscv/cpu.h         |  12 +++
> >>   target/riscv/cpu_cfg.h     |   2 +
> >>   target/riscv/kvm/kvm-cpu.c |   7 +-
> >>   target/riscv/tcg/tcg-cpu.c | 165 +++++++++++++++++++++++++++++++++----
> >>   5 files changed, 197 insertions(+), 18 deletions(-)
> >>
> >> --
> >> 2.41.0
> >>
> >>

Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Andrew Jones]

On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 10/11/23 00:01, Alistair Francis wrote:
> > On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> > > 
> > > Hi,
> > > 
> > > Several design changes were made in this version after the reviews and
> > > feedback in the v1 [1]. The high-level summary is:
> > > 
> > > - we'll no longer allow users to set profile flags for vendor CPUs. If
> > >    we're to adhere to the current policy of not allowing users to enable
> > >    extensions for vendor CPUs, the profile support would become a
> > >    glorified way of checking if the vendor CPU happens to support a
> > >    specific profile. If a future vendor CPU supports a profile the CPU
> > >    can declare it manually in its cpu_init() function, the flag will
> > >    still be set, but users can't change it;
> > > 
> > > - disabling a profile will now disable all the mandatory extensions from
> > >    the CPU;
> > 
> > What happens if you enable one profile and disable a different one?
> 
> With this implementation as is the profiles will be evaluated by the order they're
> declared in riscv_cpu_profiles[]. Which isn't exactly ideal since we're exchanging
> a left-to-right ordering in the command line by an arbitrary order that we happened
> to set in the code.
> 
> I can make some tweaks to make the profiles sensible to left-to-right order between
> them, while keeping regular extension with higher priority. e.g.:
> 
> 
> -cpu rv64,zicbom=true,profileA=false,profileB=true,zicboz=false
> -cpu rv64,profileA=false,zicbom=true,zicboz=false,profileB=true
> -cpu rv64,profileA=false,profileB=true,zicbom=true,zicboz=false
> 
> These would all do the same thing: "keeping zicbom=true and zicboz=false, disable profileA
> and then enable profile B"
> 
> Switching the profiles order would have a different result:
> 
> -cpu rv64,profileB=true,profileA=false,zicbom=true,zicboz=false
> 
> "keeping zicbom=true and zicboz=false, enable profile B and then disable profile A"
> 
> 
> I'm happy to hear any other alternative/ideas. We'll either deal with some left-to-right
> ordering w.r.t profiles or deal with an internal profile commit ordering. TBH I think
> it's sensible to demand left-to-right command line ordering for profiles only.

left-to-right ordering is how the rest of QEMU properties work and scripts
depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS
where $MORE_PROPS can not only add more props but also override default
props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on).
left-to-right also works with multiple -cpu parameters, i.e. -cpu
$MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props
with my props.

I don't think profiles should be treated special with regard to this. They
should behave the same as any property. If one does
profileA=off,profileB=on and there are overlapping extensions then a
sanity check in cpu-finalize should catch that and error out. Otherwise,
why not. Profiles are just like big 'G' extensions and 'G' would behave
the same way.

Thanks,
drew

Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Alistair Francis]

On Mon, Oct 16, 2023 at 7:03 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza wrote:
> >
> >
> > On 10/11/23 00:01, Alistair Francis wrote:
> > > On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
> > > <dbarboza@ventanamicro.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Several design changes were made in this version after the reviews and
> > > > feedback in the v1 [1]. The high-level summary is:
> > > >
> > > > - we'll no longer allow users to set profile flags for vendor CPUs. If
> > > >    we're to adhere to the current policy of not allowing users to enable
> > > >    extensions for vendor CPUs, the profile support would become a
> > > >    glorified way of checking if the vendor CPU happens to support a
> > > >    specific profile. If a future vendor CPU supports a profile the CPU
> > > >    can declare it manually in its cpu_init() function, the flag will
> > > >    still be set, but users can't change it;
> > > >
> > > > - disabling a profile will now disable all the mandatory extensions from
> > > >    the CPU;
> > >
> > > What happens if you enable one profile and disable a different one?
> >
> > With this implementation as is the profiles will be evaluated by the order they're
> > declared in riscv_cpu_profiles[]. Which isn't exactly ideal since we're exchanging
> > a left-to-right ordering in the command line by an arbitrary order that we happened
> > to set in the code.
> >
> > I can make some tweaks to make the profiles sensible to left-to-right order between
> > them, while keeping regular extension with higher priority. e.g.:
> >
> >
> > -cpu rv64,zicbom=true,profileA=false,profileB=true,zicboz=false
> > -cpu rv64,profileA=false,zicbom=true,zicboz=false,profileB=true
> > -cpu rv64,profileA=false,profileB=true,zicbom=true,zicboz=false
> >
> > These would all do the same thing: "keeping zicbom=true and zicboz=false, disable profileA
> > and then enable profile B"
> >
> > Switching the profiles order would have a different result:
> >
> > -cpu rv64,profileB=true,profileA=false,zicbom=true,zicboz=false
> >
> > "keeping zicbom=true and zicboz=false, enable profile B and then disable profile A"
> >
> >
> > I'm happy to hear any other alternative/ideas. We'll either deal with some left-to-right
> > ordering w.r.t profiles or deal with an internal profile commit ordering. TBH I think
> > it's sensible to demand left-to-right command line ordering for profiles only.
>
> left-to-right ordering is how the rest of QEMU properties work and scripts
> depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS
> where $MORE_PROPS can not only add more props but also override default
> props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on).
> left-to-right also works with multiple -cpu parameters, i.e. -cpu
> $MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props
> with my props.

That seems like the way to go then

>
> I don't think profiles should be treated special with regard to this. They
> should behave the same as any property. If one does
> profileA=off,profileB=on and there are overlapping extensions then a

But what does this mean? What intent is the user saying here?

For example if a user says:

    RVA22U64=off,RVA24U64=on

They only want the extensions that were added in RVA24U64? What about
G and the standard extensions?

To me it just seems really strange to have more than 1 profile.
Profiles are there to help software and users have common platforms.
Why would a user want to mix-n-match them

Alistair

> sanity check in cpu-finalize should catch that and error out. Otherwise,
> why not. Profiles are just like big 'G' extensions and 'G' would behave
> the same way.
>
> Thanks,
> drew

Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Andrew Jones]

On Tue, Oct 17, 2023 at 01:55:47PM +1000, Alistair Francis wrote:
> On Mon, Oct 16, 2023 at 7:03 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza wrote:
> > >
> > >
> > > On 10/11/23 00:01, Alistair Francis wrote:
> > > > On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
> > > > <dbarboza@ventanamicro.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Several design changes were made in this version after the reviews and
> > > > > feedback in the v1 [1]. The high-level summary is:
> > > > >
> > > > > - we'll no longer allow users to set profile flags for vendor CPUs. If
> > > > >    we're to adhere to the current policy of not allowing users to enable
> > > > >    extensions for vendor CPUs, the profile support would become a
> > > > >    glorified way of checking if the vendor CPU happens to support a
> > > > >    specific profile. If a future vendor CPU supports a profile the CPU
> > > > >    can declare it manually in its cpu_init() function, the flag will
> > > > >    still be set, but users can't change it;
> > > > >
> > > > > - disabling a profile will now disable all the mandatory extensions from
> > > > >    the CPU;
> > > >
> > > > What happens if you enable one profile and disable a different one?
> > >
> > > With this implementation as is the profiles will be evaluated by the order they're
> > > declared in riscv_cpu_profiles[]. Which isn't exactly ideal since we're exchanging
> > > a left-to-right ordering in the command line by an arbitrary order that we happened
> > > to set in the code.
> > >
> > > I can make some tweaks to make the profiles sensible to left-to-right order between
> > > them, while keeping regular extension with higher priority. e.g.:
> > >
> > >
> > > -cpu rv64,zicbom=true,profileA=false,profileB=true,zicboz=false
> > > -cpu rv64,profileA=false,zicbom=true,zicboz=false,profileB=true
> > > -cpu rv64,profileA=false,profileB=true,zicbom=true,zicboz=false
> > >
> > > These would all do the same thing: "keeping zicbom=true and zicboz=false, disable profileA
> > > and then enable profile B"
> > >
> > > Switching the profiles order would have a different result:
> > >
> > > -cpu rv64,profileB=true,profileA=false,zicbom=true,zicboz=false
> > >
> > > "keeping zicbom=true and zicboz=false, enable profile B and then disable profile A"
> > >
> > >
> > > I'm happy to hear any other alternative/ideas. We'll either deal with some left-to-right
> > > ordering w.r.t profiles or deal with an internal profile commit ordering. TBH I think
> > > it's sensible to demand left-to-right command line ordering for profiles only.
> >
> > left-to-right ordering is how the rest of QEMU properties work and scripts
> > depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS
> > where $MORE_PROPS can not only add more props but also override default
> > props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on).
> > left-to-right also works with multiple -cpu parameters, i.e. -cpu
> > $MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props
> > with my props.
> 
> That seems like the way to go then
> 
> >
> > I don't think profiles should be treated special with regard to this. They
> > should behave the same as any property. If one does
> > profileA=off,profileB=on and there are overlapping extensions then a
> 
> But what does this mean? What intent is the user saying here?
> 
> For example if a user says:
> 
>     RVA22U64=off,RVA24U64=on
> 
> They only want the extensions that were added in RVA24U64? What about
> G and the standard extensions?

Disabling a profile is certainly odd, because I wouldn't expect profiles
to be used with any CPU type other than a base type, i.e. they should be
used to enable extensions on a barebones CPU model, which means setting
them off would be noops.  And, if a profile is set off for a cpu model
where extensions are set either by the model or by previous profile and
extension properties, then it also seems like an odd use, but that's at
least consistent with how other properties would work, so I'm not sure we
need to forbid it.

> 
> To me it just seems really strange to have more than 1 profile.
> Profiles are there to help software and users have common platforms.
> Why would a user want to mix-n-match them

I think it's possible users will want to describe platforms which are
compatible with more than one profile, e.g. RVA22U64=on,RVA24U64=on.

The example I gave Andrea about this was that C may get demoted from
mandatory to optional in later profiles. If a platform is built which
conforms to an older profile with C and to the later profile where C
is only optional, then enabling both profiles will ensure that C is
enabled, whereas only enabling the later profile will not, and then
C must be added manually after inspecting the older profile to see
what will be missed. OIOW, the burden of individual extension management
will still be present if only single profiles may be enabled at a time.
(And, even if the later profile was a strict superset of the older one,
then, if a user wants to explicitly describe a platform which claims
compatibility with both profiles, they probably shouldn't be punished
for it, even if the resulting extension enablement would be equivalent
to only enabling the later profile.)

Thanks,
drew

Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Daniel Henrique Barboza]

On 10/17/23 05:08, Andrew Jones wrote:
> On Tue, Oct 17, 2023 at 01:55:47PM +1000, Alistair Francis wrote:
>> On Mon, Oct 16, 2023 at 7:03 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>>>
>>> On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 10/11/23 00:01, Alistair Francis wrote:
>>>>> On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
>>>>> <dbarboza@ventanamicro.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Several design changes were made in this version after the reviews and
>>>>>> feedback in the v1 [1]. The high-level summary is:
>>>>>>
>>>>>> - we'll no longer allow users to set profile flags for vendor CPUs. If
>>>>>>     we're to adhere to the current policy of not allowing users to enable
>>>>>>     extensions for vendor CPUs, the profile support would become a
>>>>>>     glorified way of checking if the vendor CPU happens to support a
>>>>>>     specific profile. If a future vendor CPU supports a profile the CPU
>>>>>>     can declare it manually in its cpu_init() function, the flag will
>>>>>>     still be set, but users can't change it;
>>>>>>
>>>>>> - disabling a profile will now disable all the mandatory extensions from
>>>>>>     the CPU;
>>>>>
>>>>> What happens if you enable one profile and disable a different one?
>>>>
>>>> With this implementation as is the profiles will be evaluated by the order they're
>>>> declared in riscv_cpu_profiles[]. Which isn't exactly ideal since we're exchanging
>>>> a left-to-right ordering in the command line by an arbitrary order that we happened
>>>> to set in the code.
>>>>
>>>> I can make some tweaks to make the profiles sensible to left-to-right order between
>>>> them, while keeping regular extension with higher priority. e.g.:
>>>>
>>>>
>>>> -cpu rv64,zicbom=true,profileA=false,profileB=true,zicboz=false
>>>> -cpu rv64,profileA=false,zicbom=true,zicboz=false,profileB=true
>>>> -cpu rv64,profileA=false,profileB=true,zicbom=true,zicboz=false
>>>>
>>>> These would all do the same thing: "keeping zicbom=true and zicboz=false, disable profileA
>>>> and then enable profile B"
>>>>
>>>> Switching the profiles order would have a different result:
>>>>
>>>> -cpu rv64,profileB=true,profileA=false,zicbom=true,zicboz=false
>>>>
>>>> "keeping zicbom=true and zicboz=false, enable profile B and then disable profile A"
>>>>
>>>>
>>>> I'm happy to hear any other alternative/ideas. We'll either deal with some left-to-right
>>>> ordering w.r.t profiles or deal with an internal profile commit ordering. TBH I think
>>>> it's sensible to demand left-to-right command line ordering for profiles only.
>>>
>>> left-to-right ordering is how the rest of QEMU properties work and scripts
>>> depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS
>>> where $MORE_PROPS can not only add more props but also override default
>>> props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on).
>>> left-to-right also works with multiple -cpu parameters, i.e. -cpu
>>> $MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props
>>> with my props.
>>
>> That seems like the way to go then
>>
>>>
>>> I don't think profiles should be treated special with regard to this. They
>>> should behave the same as any property. If one does
>>> profileA=off,profileB=on and there are overlapping extensions then a
>>
>> But what does this mean? What intent is the user saying here?
>>
>> For example if a user says:
>>
>>      RVA22U64=off,RVA24U64=on
>>
>> They only want the extensions that were added in RVA24U64? What about
>> G and the standard extensions?
> 
> Disabling a profile is certainly odd, because I wouldn't expect profiles
> to be used with any CPU type other than a base type, i.e. they should be
> used to enable extensions on a barebones CPU model, which means setting
> them off would be noops.  And, if a profile is set off for a cpu model
> where extensions are set either by the model or by previous profile and
> extension properties, then it also seems like an odd use, but that's at
> least consistent with how other properties would work, so I'm not sure we
> need to forbid it.

It's weird to add a flag that users can set to 'off' and nothing happens.

That said, I'm considering profile disablement a debug/development option.
I am thinking about adding a warning when users disables a profile like:

"disabling a profile is recommended only for troubleshooting and is discouraged
for regular use"

And also mention something along those lines in the docs as well.

We might be compelled into implementing profile disablement because it's weird
otherwise, but we're not obligated to promote it. In fact I want to actively
discourage it.


Thanks,

Daniel


> 
>>
>> To me it just seems really strange to have more than 1 profile.
>> Profiles are there to help software and users have common platforms.
>> Why would a user want to mix-n-match them
> 
> I think it's possible users will want to describe platforms which are
> compatible with more than one profile, e.g. RVA22U64=on,RVA24U64=on.
> 
> The example I gave Andrea about this was that C may get demoted from
> mandatory to optional in later profiles. If a platform is built which
> conforms to an older profile with C and to the later profile where C
> is only optional, then enabling both profiles will ensure that C is
> enabled, whereas only enabling the later profile will not, and then
> C must be added manually after inspecting the older profile to see
> what will be missed. OIOW, the burden of individual extension management
> will still be present if only single profiles may be enabled at a time.
> (And, even if the later profile was a strict superset of the older one,
> then, if a user wants to explicitly describe a platform which claims
> compatibility with both profiles, they probably shouldn't be punished
> for it, even if the resulting extension enablement would be equivalent
> to only enabling the later profile.)
> 
> Thanks,
> drew