[PATCH for-8.2 0/2] target/riscv: add zicntr and zihpm flags

[PATCH for-8.2 0/2] target/riscv: add zicntr and zihpm flags

[Daniel Henrique Barboza]

Hi,

I decided to include flags for both timer/counter extensions to make it
easier for us later on when dealing with the RVA22 profile (which
includes both). 

The features were already implemented by Atish Patra some time ago, but
back then these 2 extensions weren't introduced yet. This means that,
aside from extra stuff in riscv,isa FDT no other functional changes were
made.

Both are defaulted to 'true' since QEMU already implements both
features, but the flag can be disabled if Zicsr isn't present or, in
the case of zihpm, if pmu_num = 0.

Daniel Henrique Barboza (2):
  target/riscv/cpu.c: add zicntr extension flag
  target/riscv/cpu.c: add zihpm extension flag

 target/riscv/cpu.c     | 29 +++++++++++++++++++++++------
 target/riscv/cpu_cfg.h |  2 ++
 2 files changed, 25 insertions(+), 6 deletions(-)

-- 
2.41.0

[PATCH for-8.2 1/2] 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     | 11 +++++++++++
 target/riscv/cpu_cfg.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9339c0241d..7ec88659be 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -85,6 +85,7 @@ static const struct isa_ext_data 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(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
@@ -1291,6 +1292,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.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.
@@ -1778,6 +1783,12 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
     DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
 
+    /*
+     * Always default true - we'll disable it during
+     * realize() if needed.
+     */
+    DEFINE_PROP_BOOL("zicntr", RISCVCPU, cfg.ext_icntr, true),
+
     DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
     DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
     DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 2bd9510ba3..d36dc12b92 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;
-- 
2.41.0

Re: [PATCH for-8.2 1/2] target/riscv/cpu.c: add zicntr extension flag

[Alistair Francis]

On Tue, Jul 18, 2023 at 7:55 AM 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.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c     | 11 +++++++++++
>  target/riscv/cpu_cfg.h |  1 +
>  2 files changed, 12 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9339c0241d..7ec88659be 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -85,6 +85,7 @@ static const struct isa_ext_data 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(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
> @@ -1291,6 +1292,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          cpu->cfg.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.
> @@ -1778,6 +1783,12 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
>      DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
>
> +    /*
> +     * Always default true - we'll disable it during
> +     * realize() if needed.
> +     */
> +    DEFINE_PROP_BOOL("zicntr", RISCVCPU, cfg.ext_icntr, true),
> +
>      DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
>      DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
>      DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 2bd9510ba3..d36dc12b92 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;
> --
> 2.41.0
>
>

[PATCH for-8.2 2/2] 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).

There's a small tweak needed in riscv_cpu_realize_tcg() made:
riscv_cpu_validate_set_extensions() must be executed after the block
that executes riscv_pmu_init(). The reason is that riscv_pmu_init() will
do "cpu->cfg.pmu_num = 0" if PMU support cannot be enabled. We want to
get the latest, definite value of cfg.pmu_num during the validation() to
ensure we do the right thing.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7ec88659be..5836640d5c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -89,6 +89,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
     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(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
+    ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_ihpm),
     ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
     ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
     ISA_EXT_DATA_ENTRY(zfbfmin, PRIV_VERSION_1_12_0, ext_zfbfmin),
@@ -1296,6 +1297,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.
@@ -1426,12 +1431,6 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
         return;
     }
 
-    riscv_cpu_validate_set_extensions(cpu, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
 #ifndef CONFIG_USER_ONLY
     CPU(dev)->tcg_cflags |= CF_PCREL;
 
@@ -1446,6 +1445,12 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
         }
      }
 #endif
+
+    riscv_cpu_validate_set_extensions(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 static void riscv_cpu_realize(DeviceState *dev, Error **errp)
@@ -1784,10 +1789,11 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
 
     /*
-     * Always default true - we'll disable it during
+     * Always default true - we'll disable them during
      * realize() if needed.
      */
     DEFINE_PROP_BOOL("zicntr", RISCVCPU, cfg.ext_icntr, true),
+    DEFINE_PROP_BOOL("zihpm", RISCVCPU, cfg.ext_ihpm, true),
 
     DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
     DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index d36dc12b92..85c7a71853 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_zihintpause;
     bool ext_smstateen;
-- 
2.41.0

Re: [PATCH for-8.2 2/2] target/riscv/cpu.c: add zihpm extension flag

[Alistair Francis]

On Tue, Jul 18, 2023 at 7:55 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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).
>
> There's a small tweak needed in riscv_cpu_realize_tcg() made:
> riscv_cpu_validate_set_extensions() must be executed after the block
> that executes riscv_pmu_init(). The reason is that riscv_pmu_init() will
> do "cpu->cfg.pmu_num = 0" if PMU support cannot be enabled. We want to
> get the latest, definite value of cfg.pmu_num during the validation() to
> ensure we do the right thing.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c     | 20 +++++++++++++-------
>  target/riscv/cpu_cfg.h |  1 +
>  2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7ec88659be..5836640d5c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -89,6 +89,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
>      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(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
> +    ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_ihpm),
>      ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
>      ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
>      ISA_EXT_DATA_ENTRY(zfbfmin, PRIV_VERSION_1_12_0, ext_zfbfmin),
> @@ -1296,6 +1297,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.
> @@ -1426,12 +1431,6 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
>          return;
>      }
>
> -    riscv_cpu_validate_set_extensions(cpu, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
>  #ifndef CONFIG_USER_ONLY
>      CPU(dev)->tcg_cflags |= CF_PCREL;
>
> @@ -1446,6 +1445,12 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
>          }
>       }
>  #endif
> +
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>
>  static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> @@ -1784,10 +1789,11 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
>
>      /*
> -     * Always default true - we'll disable it during
> +     * Always default true - we'll disable them during
>       * realize() if needed.
>       */
>      DEFINE_PROP_BOOL("zicntr", RISCVCPU, cfg.ext_icntr, true),
> +    DEFINE_PROP_BOOL("zihpm", RISCVCPU, cfg.ext_ihpm, true),
>
>      DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
>      DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index d36dc12b92..85c7a71853 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_zihintpause;
>      bool ext_smstateen;
> --
> 2.41.0
>
>

Re: [PATCH for-8.2 0/2] target/riscv: add zicntr and zihpm flags

[Conor Dooley]

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

Hey,

On Mon, Jul 17, 2023 at 06:54:17PM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> I decided to include flags for both timer/counter extensions to make it
> easier for us later on when dealing with the RVA22 profile (which
> includes both). 
> 
> The features were already implemented by Atish Patra some time ago, but
> back then these 2 extensions weren't introduced yet. This means that,
> aside from extra stuff in riscv,isa FDT no other functional changes were
> made.
> 
> Both are defaulted to 'true' since QEMU already implements both
> features, but the flag can be disabled if Zicsr isn't present or, in
> the case of zihpm, if pmu_num = 0.

Out of curiosity, since you are allowing them to be disabled, how do you
intend to communicate to a guest that zicsr or zihpm are not present?

> This means that,
> aside from extra stuff in riscv,isa FDT no other functional changes were
> made.

This is barely a "functional" change either, as the presence of these
extensions has to be assumed, whether they appear in riscv,isa or not :/

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

Re: [PATCH for-8.2 0/2] target/riscv: add zicntr and zihpm flags

[Daniel Henrique Barboza]

On 7/17/23 19:33, Conor Dooley wrote:
> Hey,
> 
> On Mon, Jul 17, 2023 at 06:54:17PM -0300, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> I decided to include flags for both timer/counter extensions to make it
>> easier for us later on when dealing with the RVA22 profile (which
>> includes both).
>>
>> The features were already implemented by Atish Patra some time ago, but
>> back then these 2 extensions weren't introduced yet. This means that,
>> aside from extra stuff in riscv,isa FDT no other functional changes were
>> made.
>>
>> Both are defaulted to 'true' since QEMU already implements both
>> features, but the flag can be disabled if Zicsr isn't present or, in
>> the case of zihpm, if pmu_num = 0.
> 
> Out of curiosity, since you are allowing them to be disabled, how do you
> intend to communicate to a guest that zicsr or zihpm are not present?

At this point I'd say that existing guests are using other ways of checking
if these timers and counters are available. After this patches OSes can confirm
if these timers are available via riscv,isa, but they can't assume that
they are not available if riscv,isa doesn't display them.

There's a chance that guests will continue ignoring these 2 extensions regardless
of whether the platform exposes them or not.

> 
>> This means that,
>> aside from extra stuff in riscv,isa FDT no other functional changes were
>> made.
> 
> This is barely a "functional" change either, as the presence of these
> extensions has to be assumed, whether they appear in riscv,isa or not :/

It's more of an organizational change for the sake of QEMU internals because the
RVA22 profile happens to include zicntr and zihpm as mandatory extensions. It's
easier to add the flags than to document why we're claiming RVA22 support but
aren't displaying these 2 in riscv,isa.



Thanks,


Daniel

Re: [PATCH for-8.2 0/2] target/riscv: add zicntr and zihpm flags

[Conor Dooley]

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

On Mon, Jul 17, 2023 at 08:11:09PM -0300, Daniel Henrique Barboza wrote:
> On 7/17/23 19:33, Conor Dooley wrote:

> > On Mon, Jul 17, 2023 at 06:54:17PM -0300, Daniel Henrique Barboza wrote:
> > > Hi,
> > > 
> > > I decided to include flags for both timer/counter extensions to make it
> > > easier for us later on when dealing with the RVA22 profile (which
> > > includes both).
> > > 
> > > The features were already implemented by Atish Patra some time ago, but
> > > back then these 2 extensions weren't introduced yet. This means that,
> > > aside from extra stuff in riscv,isa FDT no other functional changes were
> > > made.
> > > 
> > > Both are defaulted to 'true' since QEMU already implements both
> > > features, but the flag can be disabled if Zicsr isn't present or, in
> > > the case of zihpm, if pmu_num = 0.
> > 
> > Out of curiosity, since you are allowing them to be disabled, how do you
> > intend to communicate to a guest that zicsr or zihpm are not present?
> 
> At this point I'd say that existing guests are using other ways of checking
> if these timers and counters are available.

Or they just assume they're there as part of their baseline requirements
;)

> After this patches OSes can confirm
> if these timers are available via riscv,isa, but they can't assume that
> they are not available if riscv,isa doesn't display them.
> 
> There's a chance that guests will continue ignoring these 2 extensions regardless
> of whether the platform exposes them or not.
> 
> > 
> > > This means that,
> > > aside from extra stuff in riscv,isa FDT no other functional changes were
> > > made.
> > 
> > This is barely a "functional" change either, as the presence of these
> > extensions has to be assumed, whether they appear in riscv,isa or not :/
> 
> It's more of an organizational change for the sake of QEMU internals because the
> RVA22 profile happens to include zicntr and zihpm as mandatory extensions. It's
> easier to add the flags than to document why we're claiming RVA22 support but
> aren't displaying these 2 in riscv,isa.

Possibly you should call out ACPI here too, since that does not suffer
from the same issues as riscv,isa in DT, and putting zicntr/zihpm et al
in the ISA string there is needed.

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