This series adds the counter delegation extension support. The counter
delegation ISA extension(Smcdeleg/Ssccfg) actually depends on multiple ISA
extensions.

1. S[m|s]csrind : The indirect CSR extension[1] which defines additional
   5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size limitation of
   RISC-V CSR address space.
2. Smstateen: The stateen bit[60] controls the access to the registers
   indirectly via the above indirect registers.
3. Smcdeleg/Ssccfg: The counter delegation extensions[2]

The counter delegation extension allows Supervisor mode to program the
hpmevent and hpmcounters directly without needing the assistance from the
M-mode via SBI calls. This results in a faster perf profiling and very
few traps. This extension also introduces a scountinhibit CSR which allows
to stop/start any counter directly from the S-mode. As the counter
delegation extension potentially can have more than 100 CSRs, the specificaiton
leverages the indirect CSR extension to save the precious CSR address range.

Due to the dependancy of these extensions, the following extensions must be
enabled to use the counter delegation feature in S-mode.

"smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"

This makes the qemu command line quite tedious. In stead of that, I think we
can enable these features by default if there is no objection.

The first 2 patches decouple the indirect CSR usage from AIA implementation
while patch3 adds stateen bits validation for AIA.
The PATCH4 implements indirect CSR extensions while remaining patches
implement the counter delegation extensions.

The Qemu patches can be found here:
https://github.com/atishp04/qemu/tree/counter_delegation_rfc

The opensbi patch can be found here:
https://github.com/atishp04/opensbi/tree/counter_delegation_v1

The Linux kernel patches can be found here:
https://github.com/atishp04/linux/tree/counter_delegation_rfc

[1] https://github.com/riscv/riscv-indirect-csr-access
[2] https://github.com/riscv/riscv-smcdeleg-ssccfg

Atish Patra (1):
target/riscv: Enable S*stateen bits for AIA

Kaiwen Xue (7):
target/riscv: Add properties for Indirect CSR Access extension
target/riscv: Decouple AIA processing from xiselect and xireg
target/riscv: Support generic CSR indirect access
target/riscv: Add smcdeleg/ssccfg properties
target/riscv: Add counter delegation definitions
target/riscv: Add select value range check for counter delegation
target/riscv: Add counter delegation/configuration support

target/riscv/cpu.c      |   8 +
target/riscv/cpu.h      |   1 +
target/riscv/cpu_bits.h |  34 +-
target/riscv/cpu_cfg.h  |   4 +
target/riscv/csr.c      | 713 +++++++++++++++++++++++++++++++++++++---
target/riscv/machine.c  |   1 +
6 files changed, 722 insertions(+), 39 deletions(-)

--
2.34.1

Hi Atish,


I see that Rajnesh sent some patches that were built on top of this
work [1], and this series no longer applies neither to alistair's
risc-to-apply.next nor to master.

If you could send a rebased version of this series that would be great.


Thanks,


Daniel



[1] https://lore.kernel.org/qemu-riscv/20240529160950.132754-1-rkanwal@rivosinc.com/


On 2/16/24 21:01, Atish Patra wrote:
> This series adds the counter delegation extension support. The counter
> delegation ISA extension(Smcdeleg/Ssccfg) actually depends on multiple ISA
> extensions.
> 
> 1. S[m|s]csrind : The indirect CSR extension[1] which defines additional
>     5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size limitation of
>     RISC-V CSR address space.
> 2. Smstateen: The stateen bit[60] controls the access to the registers
>     indirectly via the above indirect registers.
> 3. Smcdeleg/Ssccfg: The counter delegation extensions[2]
> 
> The counter delegation extension allows Supervisor mode to program the
> hpmevent and hpmcounters directly without needing the assistance from the
> M-mode via SBI calls. This results in a faster perf profiling and very
> few traps. This extension also introduces a scountinhibit CSR which allows
> to stop/start any counter directly from the S-mode. As the counter
> delegation extension potentially can have more than 100 CSRs, the specificaiton
> leverages the indirect CSR extension to save the precious CSR address range.
> 
> Due to the dependancy of these extensions, the following extensions must be
> enabled to use the counter delegation feature in S-mode.
> 
> "smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"
> 
> This makes the qemu command line quite tedious. In stead of that, I think we
> can enable these features by default if there is no objection.
> 
> The first 2 patches decouple the indirect CSR usage from AIA implementation
> while patch3 adds stateen bits validation for AIA.
> The PATCH4 implements indirect CSR extensions while remaining patches
> implement the counter delegation extensions.
> 
> The Qemu patches can be found here:
> https://github.com/atishp04/qemu/tree/counter_delegation_rfc
> 
> The opensbi patch can be found here:
> https://github.com/atishp04/opensbi/tree/counter_delegation_v1
> 
> The Linux kernel patches can be found here:
> https://github.com/atishp04/linux/tree/counter_delegation_rfc
> 
> [1] https://github.com/riscv/riscv-indirect-csr-access
> [2] https://github.com/riscv/riscv-smcdeleg-ssccfg
> 
> Atish Patra (1):
> target/riscv: Enable S*stateen bits for AIA
> 
> Kaiwen Xue (7):
> target/riscv: Add properties for Indirect CSR Access extension
> target/riscv: Decouple AIA processing from xiselect and xireg
> target/riscv: Support generic CSR indirect access
> target/riscv: Add smcdeleg/ssccfg properties
> target/riscv: Add counter delegation definitions
> target/riscv: Add select value range check for counter delegation
> target/riscv: Add counter delegation/configuration support
> 
> target/riscv/cpu.c      |   8 +
> target/riscv/cpu.h      |   1 +
> target/riscv/cpu_bits.h |  34 +-
> target/riscv/cpu_cfg.h  |   4 +
> target/riscv/csr.c      | 713 +++++++++++++++++++++++++++++++++++++---
> target/riscv/machine.c  |   1 +
> 6 files changed, 722 insertions(+), 39 deletions(-)
> 
> --
> 2.34.1
>

Hi Daniel,

On Sat, Jun 1, 2024 at 2:52 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi Atish,
>
>
> I see that Rajnesh sent some patches that were built on top of this
> work [1], and this series no longer applies neither to alistair's
> risc-to-apply.next nor to master.
>
> If you could send a rebased version of this series that would be great.
>
>

Yes. I am planning to send revised series for counter delegation,
Smcntrpmf and a few other miscellaneous fixes
soon.

> Thanks,
>
>
> Daniel
>
>
>
> [1] https://lore.kernel.org/qemu-riscv/20240529160950.132754-1-rkanwal@rivosinc.com/
>
>
> On 2/16/24 21:01, Atish Patra wrote:
> > This series adds the counter delegation extension support. The counter
> > delegation ISA extension(Smcdeleg/Ssccfg) actually depends on multiple ISA
> > extensions.
> >
> > 1. S[m|s]csrind : The indirect CSR extension[1] which defines additional
> >     5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size limitation of
> >     RISC-V CSR address space.
> > 2. Smstateen: The stateen bit[60] controls the access to the registers
> >     indirectly via the above indirect registers.
> > 3. Smcdeleg/Ssccfg: The counter delegation extensions[2]
> >
> > The counter delegation extension allows Supervisor mode to program the
> > hpmevent and hpmcounters directly without needing the assistance from the
> > M-mode via SBI calls. This results in a faster perf profiling and very
> > few traps. This extension also introduces a scountinhibit CSR which allows
> > to stop/start any counter directly from the S-mode. As the counter
> > delegation extension potentially can have more than 100 CSRs, the specificaiton
> > leverages the indirect CSR extension to save the precious CSR address range.
> >
> > Due to the dependancy of these extensions, the following extensions must be
> > enabled to use the counter delegation feature in S-mode.
> >
> > "smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"
> >
> > This makes the qemu command line quite tedious. In stead of that, I think we
> > can enable these features by default if there is no objection.
> >
> > The first 2 patches decouple the indirect CSR usage from AIA implementation
> > while patch3 adds stateen bits validation for AIA.
> > The PATCH4 implements indirect CSR extensions while remaining patches
> > implement the counter delegation extensions.
> >
> > The Qemu patches can be found here:
> > https://github.com/atishp04/qemu/tree/counter_delegation_rfc
> >
> > The opensbi patch can be found here:
> > https://github.com/atishp04/opensbi/tree/counter_delegation_v1
> >
> > The Linux kernel patches can be found here:
> > https://github.com/atishp04/linux/tree/counter_delegation_rfc
> >
> > [1] https://github.com/riscv/riscv-indirect-csr-access
> > [2] https://github.com/riscv/riscv-smcdeleg-ssccfg
> >
> > Atish Patra (1):
> > target/riscv: Enable S*stateen bits for AIA
> >
> > Kaiwen Xue (7):
> > target/riscv: Add properties for Indirect CSR Access extension
> > target/riscv: Decouple AIA processing from xiselect and xireg
> > target/riscv: Support generic CSR indirect access
> > target/riscv: Add smcdeleg/ssccfg properties
> > target/riscv: Add counter delegation definitions
> > target/riscv: Add select value range check for counter delegation
> > target/riscv: Add counter delegation/configuration support
> >
> > target/riscv/cpu.c      |   8 +
> > target/riscv/cpu.h      |   1 +
> > target/riscv/cpu_bits.h |  34 +-
> > target/riscv/cpu_cfg.h  |   4 +
> > target/riscv/csr.c      | 713 +++++++++++++++++++++++++++++++++++++---
> > target/riscv/machine.c  |   1 +
> > 6 files changed, 722 insertions(+), 39 deletions(-)
> >
> > --
> > 2.34.1
> >

Hi Atish,

This series and its dependency, which I assume it's

"[PATCH v4 0/5] Add ISA extension smcntrpmf support"

Doesn't apply in neither master nor riscv-to-apply.next because of this patch:

"target/riscv: Use RISCVException as return type for all csr ops"

That changed some functions from 'int' to "RISCVException" type. The conflicts
from the v4 series are rather trivial but the conflicts for this RFC are annoying
to deal with. It would be better if you could re-send both series rebased with
the latest changes.

One more thing:

On 2/16/24 21:01, Atish Patra wrote:
> This series adds the counter delegation extension support. The counter
> delegation ISA extension(Smcdeleg/Ssccfg) actually depends on multiple ISA
> extensions.
> 
> 1. S[m|s]csrind : The indirect CSR extension[1] which defines additional
>     5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size limitation of
>     RISC-V CSR address space.
> 2. Smstateen: The stateen bit[60] controls the access to the registers
>     indirectly via the above indirect registers.
> 3. Smcdeleg/Ssccfg: The counter delegation extensions[2]
> 
> The counter delegation extension allows Supervisor mode to program the
> hpmevent and hpmcounters directly without needing the assistance from the
> M-mode via SBI calls. This results in a faster perf profiling and very
> few traps. This extension also introduces a scountinhibit CSR which allows
> to stop/start any counter directly from the S-mode. As the counter
> delegation extension potentially can have more than 100 CSRs, the specificaiton
> leverages the indirect CSR extension to save the precious CSR address range.
> 
> Due to the dependancy of these extensions, the following extensions must be
> enabled to use the counter delegation feature in S-mode.
> 
> "smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"
> 
> This makes the qemu command line quite tedious. In stead of that, I think we
> can enable these features by default if there is no objection.

It wasn't need so far but, if needed, we can add specialized setters for extensions
that has multiple dependencies. Instead of the usual setter we would do something
like:

cpu_set_ssccfg() {

     if (enabled) {
         smstateen=true
         sscofpmf=true
         smcdeleg=true
         smcsrind=true
         sscsrind=true
     }
}


The advantage is that this setter would also work for CPUs that doesn't inherit defaults,
like bare-cps and profile CPUs.

That doesn't mean we can't add defaults for rv64, but for this particular case I wonder if
the 'max' CPU wouldn't be better.


Thanks,


Daniel

> 
> The first 2 patches decouple the indirect CSR usage from AIA implementation
> while patch3 adds stateen bits validation for AIA.
> The PATCH4 implements indirect CSR extensions while remaining patches
> implement the counter delegation extensions.
> 
> The Qemu patches can be found here:
> https://github.com/atishp04/qemu/tree/counter_delegation_rfc
> 
> The opensbi patch can be found here:
> https://github.com/atishp04/opensbi/tree/counter_delegation_v1
> 
> The Linux kernel patches can be found here:
> https://github.com/atishp04/linux/tree/counter_delegation_rfc
> 
> [1] https://github.com/riscv/riscv-indirect-csr-access
> [2] https://github.com/riscv/riscv-smcdeleg-ssccfg
> 
> Atish Patra (1):
> target/riscv: Enable S*stateen bits for AIA
> 
> Kaiwen Xue (7):
> target/riscv: Add properties for Indirect CSR Access extension
> target/riscv: Decouple AIA processing from xiselect and xireg
> target/riscv: Support generic CSR indirect access
> target/riscv: Add smcdeleg/ssccfg properties
> target/riscv: Add counter delegation definitions
> target/riscv: Add select value range check for counter delegation
> target/riscv: Add counter delegation/configuration support
> 
> target/riscv/cpu.c      |   8 +
> target/riscv/cpu.h      |   1 +
> target/riscv/cpu_bits.h |  34 +-
> target/riscv/cpu_cfg.h  |   4 +
> target/riscv/csr.c      | 713 +++++++++++++++++++++++++++++++++++++---
> target/riscv/machine.c  |   1 +
> 6 files changed, 722 insertions(+), 39 deletions(-)
> 
> --
> 2.34.1
>

On Wed, Feb 21, 2024 at 6:58 AM Daniel Henrique Barboza <
dbarboza@ventanamicro.com> wrote:

> Hi Atish,
>
> This series and its dependency, which I assume it's
>
> "[PATCH v4 0/5] Add ISA extension smcntrpmf support"
>
> Doesn't apply in neither master nor riscv-to-apply.next because of this
> patch:
>
"target/riscv: Use RISCVException as return type for all csr ops"
>
> That changed some functions from 'int' to "RISCVException" type. The
> conflicts
> from the v4 series are rather trivial but the conflicts for this RFC are
> annoying
> to deal with. It would be better if you could re-send both series rebased
> with
> the latest changes.
>
>
I was waiting for Alistair's ACK on the smcntrpmf series as he had some
comments. It looks like he is okay
with the series now (no further questions).  Let me respin both the series.


> One more thing:
>
> On 2/16/24 21:01, Atish Patra wrote:
> > This series adds the counter delegation extension support. The counter
> > delegation ISA extension(Smcdeleg/Ssccfg) actually depends on multiple
> ISA
> > extensions.
> >
> > 1. S[m|s]csrind : The indirect CSR extension[1] which defines additional
> >     5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size limitation
> of
> >     RISC-V CSR address space.
> > 2. Smstateen: The stateen bit[60] controls the access to the registers
> >     indirectly via the above indirect registers.
> > 3. Smcdeleg/Ssccfg: The counter delegation extensions[2]
> >
> > The counter delegation extension allows Supervisor mode to program the
> > hpmevent and hpmcounters directly without needing the assistance from the
> > M-mode via SBI calls. This results in a faster perf profiling and very
> > few traps. This extension also introduces a scountinhibit CSR which
> allows
> > to stop/start any counter directly from the S-mode. As the counter
> > delegation extension potentially can have more than 100 CSRs, the
> specificaiton
> > leverages the indirect CSR extension to save the precious CSR address
> range.
> >
> > Due to the dependancy of these extensions, the following extensions must
> be
> > enabled to use the counter delegation feature in S-mode.
> >
> >
> "smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"
> >
> > This makes the qemu command line quite tedious. In stead of that, I
> think we
> > can enable these features by default if there is no objection.
>
> It wasn't need so far but, if needed, we can add specialized setters for
> extensions
> that has multiple dependencies. Instead of the usual setter we would do
> something
> like:
>
> cpu_set_ssccfg() {
>
>      if (enabled) {
>          smstateen=true
>          sscofpmf=true
>          smcdeleg=true
>          smcsrind=true
>          sscsrind=true
>      }
> }
>
>
> The advantage is that this setter would also work for CPUs that doesn't
> inherit defaults,
> like bare-cps and profile CPUs.
>
>
Your suggested approach looks good to me. But I was asking about concerns
about enabling these extensions
by default rather than the actual mechanism to implement it. Few of the
extensions listed here such as smstateen,smcsrind
sscsrind are independent ISA extensions which are used for other ISA
extensions as well.

It looks like you are okay with the use case also ?


> That doesn't mean we can't add defaults for rv64, but for this particular
> case I wonder if
> the 'max' CPU wouldn't be better.
>
>
Not sure what you mean here. What does 'max' cpu have to do with pmu
extensions ?


>
> Thanks,
>
>
> Daniel
>
> >
> > The first 2 patches decouple the indirect CSR usage from AIA
> implementation
> > while patch3 adds stateen bits validation for AIA.
> > The PATCH4 implements indirect CSR extensions while remaining patches
> > implement the counter delegation extensions.
> >
> > The Qemu patches can be found here:
> > https://github.com/atishp04/qemu/tree/counter_delegation_rfc
> >
> > The opensbi patch can be found here:
> > https://github.com/atishp04/opensbi/tree/counter_delegation_v1
> >
> > The Linux kernel patches can be found here:
> > https://github.com/atishp04/linux/tree/counter_delegation_rfc
> >
> > [1] https://github.com/riscv/riscv-indirect-csr-access
> > [2] https://github.com/riscv/riscv-smcdeleg-ssccfg
> >
> > Atish Patra (1):
> > target/riscv: Enable S*stateen bits for AIA
> >
> > Kaiwen Xue (7):
> > target/riscv: Add properties for Indirect CSR Access extension
> > target/riscv: Decouple AIA processing from xiselect and xireg
> > target/riscv: Support generic CSR indirect access
> > target/riscv: Add smcdeleg/ssccfg properties
> > target/riscv: Add counter delegation definitions
> > target/riscv: Add select value range check for counter delegation
> > target/riscv: Add counter delegation/configuration support
> >
> > target/riscv/cpu.c      |   8 +
> > target/riscv/cpu.h      |   1 +
> > target/riscv/cpu_bits.h |  34 +-
> > target/riscv/cpu_cfg.h  |   4 +
> > target/riscv/csr.c      | 713 +++++++++++++++++++++++++++++++++++++---
> > target/riscv/machine.c  |   1 +
> > 6 files changed, 722 insertions(+), 39 deletions(-)
> >
> > --
> > 2.34.1
> >
>

On 2/21/24 14:06, Atish Kumar Patra wrote:
> 
> 
> On Wed, Feb 21, 2024 at 6:58 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com <mailto:dbarboza@ventanamicro.com>> wrote:
> 
>     Hi Atish,
> 
>     This series and its dependency, which I assume it's
> 
>     "[PATCH v4 0/5] Add ISA extension smcntrpmf support"
> 
>     Doesn't apply in neither master nor riscv-to-apply.next because of this patch:
> 
>     "target/riscv: Use RISCVException as return type for all csr ops"
> 
>     That changed some functions from 'int' to "RISCVException" type. The conflicts
>     from the v4 series are rather trivial but the conflicts for this RFC are annoying
>     to deal with. It would be better if you could re-send both series rebased with
>     the latest changes.
> 
> 
> I was waiting for Alistair's ACK on the smcntrpmf series as he had some comments. It looks like he is okay
> with the series now (no further questions).  Let me respin both the series.
> 
>     One more thing:
> 
>     On 2/16/24 21:01, Atish Patra wrote:
>      > This series adds the counter delegation extension support. The counter
>      > delegation ISA extension(Smcdeleg/Ssccfg) actually depends on multiple ISA
>      > extensions.
>      >
>      > 1. S[m|s]csrind : The indirect CSR extension[1] which defines additional
>      >     5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size limitation of
>      >     RISC-V CSR address space.
>      > 2. Smstateen: The stateen bit[60] controls the access to the registers
>      >     indirectly via the above indirect registers.
>      > 3. Smcdeleg/Ssccfg: The counter delegation extensions[2]
>      >
>      > The counter delegation extension allows Supervisor mode to program the
>      > hpmevent and hpmcounters directly without needing the assistance from the
>      > M-mode via SBI calls. This results in a faster perf profiling and very
>      > few traps. This extension also introduces a scountinhibit CSR which allows
>      > to stop/start any counter directly from the S-mode. As the counter
>      > delegation extension potentially can have more than 100 CSRs, the specificaiton
>      > leverages the indirect CSR extension to save the precious CSR address range.
>      >
>      > Due to the dependancy of these extensions, the following extensions must be
>      > enabled to use the counter delegation feature in S-mode.
>      >
>      > "smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"
>      >
>      > This makes the qemu command line quite tedious. In stead of that, I think we
>      > can enable these features by default if there is no objection.
> 
>     It wasn't need so far but, if needed, we can add specialized setters for extensions
>     that has multiple dependencies. Instead of the usual setter we would do something
>     like:
> 
>     cpu_set_ssccfg() {
> 
>           if (enabled) {
>               smstateen=true
>               sscofpmf=true
>               smcdeleg=true
>               smcsrind=true
>               sscsrind=true
>           }
>     }
> 
> 
>     The advantage is that this setter would also work for CPUs that doesn't inherit defaults,
>     like bare-cps and profile CPUs.
> 
> 
> Your suggested approach looks good to me. But I was asking about concerns about enabling these extensions
> by default rather than the actual mechanism to implement it. Few of the extensions listed here such as smstateen,smcsrind
> sscsrind are independent ISA extensions which are used for other ISA extensions as well.
> 
> It looks like you are okay with the use case also ?

I don't mind setting new defaults in rv64.

> 
>     That doesn't mean we can't add defaults for rv64, but for this particular case I wonder if
>     the 'max' CPU wouldn't be better.
> 
> 
> Not sure what you mean here. What does 'max' cpu have to do with pmu extensions ?


Save a few exceptions, all the extensions declared in riscv_cpu_extensions[]
will be enabled in the 'max' CPU, regardless of their default value for the
rv64 CPU (see riscv_init_max_cpu_extensions() in tcg-cpu.c).

If we count both the v4 and this RFC, the following extensions were added in
riscv_cpu_extensions[]:

+    MULTI_EXT_CFG_BOOL("smcntrpmf", ext_smcntrpmf, false),

+    MULTI_EXT_CFG_BOOL("smcsrind", ext_smcsrind, false),
+    MULTI_EXT_CFG_BOOL("sscsrind", ext_sscsrind, false),

+    MULTI_EXT_CFG_BOOL("smcdeleg", ext_smcdeleg, false),
+    MULTI_EXT_CFG_BOOL("ssccfg", ext_ssccfg, false),


All of them will be enabled by default in the 'max' CPU.

This is what I was referring to. We can just use the 'max' CPU and don't worry about
enabling defaults in rv64.


Thanks,

Daniel

> 
> 
>     Thanks,
> 
> 
>     Daniel
> 
>      >
>      > The first 2 patches decouple the indirect CSR usage from AIA implementation
>      > while patch3 adds stateen bits validation for AIA.
>      > The PATCH4 implements indirect CSR extensions while remaining patches
>      > implement the counter delegation extensions.
>      >
>      > The Qemu patches can be found here:
>      > https://github.com/atishp04/qemu/tree/counter_delegation_rfc <https://github.com/atishp04/qemu/tree/counter_delegation_rfc>
>      >
>      > The opensbi patch can be found here:
>      > https://github.com/atishp04/opensbi/tree/counter_delegation_v1 <https://github.com/atishp04/opensbi/tree/counter_delegation_v1>
>      >
>      > The Linux kernel patches can be found here:
>      > https://github.com/atishp04/linux/tree/counter_delegation_rfc <https://github.com/atishp04/linux/tree/counter_delegation_rfc>
>      >
>      > [1] https://github.com/riscv/riscv-indirect-csr-access <https://github.com/riscv/riscv-indirect-csr-access>
>      > [2] https://github.com/riscv/riscv-smcdeleg-ssccfg <https://github.com/riscv/riscv-smcdeleg-ssccfg>
>      >
>      > Atish Patra (1):
>      > target/riscv: Enable S*stateen bits for AIA
>      >
>      > Kaiwen Xue (7):
>      > target/riscv: Add properties for Indirect CSR Access extension
>      > target/riscv: Decouple AIA processing from xiselect and xireg
>      > target/riscv: Support generic CSR indirect access
>      > target/riscv: Add smcdeleg/ssccfg properties
>      > target/riscv: Add counter delegation definitions
>      > target/riscv: Add select value range check for counter delegation
>      > target/riscv: Add counter delegation/configuration support
>      >
>      > target/riscv/cpu.c      |   8 +
>      > target/riscv/cpu.h      |   1 +
>      > target/riscv/cpu_bits.h |  34 +-
>      > target/riscv/cpu_cfg.h  |   4 +
>      > target/riscv/csr.c      | 713 +++++++++++++++++++++++++++++++++++++---
>      > target/riscv/machine.c  |   1 +
>      > 6 files changed, 722 insertions(+), 39 deletions(-)
>      >
>      > --
>      > 2.34.1
>      >
> 

On 2/21/24 10:26, Daniel Henrique Barboza wrote:
>
>
> On 2/21/24 14:06, Atish Kumar Patra wrote:
>>
>>
>> On Wed, Feb 21, 2024 at 6:58 AM Daniel Henrique Barboza 
>> <dbarboza@ventanamicro.com <mailto:dbarboza@ventanamicro.com>> wrote:
>>
>>     Hi Atish,
>>
>>     This series and its dependency, which I assume it's
>>
>>     "[PATCH v4 0/5] Add ISA extension smcntrpmf support"
>>
>>     Doesn't apply in neither master nor riscv-to-apply.next because 
>> of this patch:
>>
>>     "target/riscv: Use RISCVException as return type for all csr ops"
>>
>>     That changed some functions from 'int' to "RISCVException" type. 
>> The conflicts
>>     from the v4 series are rather trivial but the conflicts for this 
>> RFC are annoying
>>     to deal with. It would be better if you could re-send both series 
>> rebased with
>>     the latest changes.
>>
>>
>> I was waiting for Alistair's ACK on the smcntrpmf series as he had 
>> some comments. It looks like he is okay
>> with the series now (no further questions).  Let me respin both the 
>> series.
>>
>>     One more thing:
>>
>>     On 2/16/24 21:01, Atish Patra wrote:
>>      > This series adds the counter delegation extension support. The 
>> counter
>>      > delegation ISA extension(Smcdeleg/Ssccfg) actually depends on 
>> multiple ISA
>>      > extensions.
>>      >
>>      > 1. S[m|s]csrind : The indirect CSR extension[1] which defines 
>> additional
>>      >     5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size 
>> limitation of
>>      >     RISC-V CSR address space.
>>      > 2. Smstateen: The stateen bit[60] controls the access to the 
>> registers
>>      >     indirectly via the above indirect registers.
>>      > 3. Smcdeleg/Ssccfg: The counter delegation extensions[2]
>>      >
>>      > The counter delegation extension allows Supervisor mode to 
>> program the
>>      > hpmevent and hpmcounters directly without needing the 
>> assistance from the
>>      > M-mode via SBI calls. This results in a faster perf profiling 
>> and very
>>      > few traps. This extension also introduces a scountinhibit CSR 
>> which allows
>>      > to stop/start any counter directly from the S-mode. As the 
>> counter
>>      > delegation extension potentially can have more than 100 CSRs, 
>> the specificaiton
>>      > leverages the indirect CSR extension to save the precious CSR 
>> address range.
>>      >
>>      > Due to the dependancy of these extensions, the following 
>> extensions must be
>>      > enabled to use the counter delegation feature in S-mode.
>>      >
>>      > 
>> "smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"
>>      >
>>      > This makes the qemu command line quite tedious. In stead of 
>> that, I think we
>>      > can enable these features by default if there is no objection.
>>
>>     It wasn't need so far but, if needed, we can add specialized 
>> setters for extensions
>>     that has multiple dependencies. Instead of the usual setter we 
>> would do something
>>     like:
>>
>>     cpu_set_ssccfg() {
>>
>>           if (enabled) {
>>               smstateen=true
>>               sscofpmf=true
>>               smcdeleg=true
>>               smcsrind=true
>>               sscsrind=true
>>           }
>>     }
>>
>>
>>     The advantage is that this setter would also work for CPUs that 
>> doesn't inherit defaults,
>>     like bare-cps and profile CPUs.
>>
>>
>> Your suggested approach looks good to me. But I was asking about 
>> concerns about enabling these extensions
>> by default rather than the actual mechanism to implement it. Few of 
>> the extensions listed here such as smstateen,smcsrind
>> sscsrind are independent ISA extensions which are used for other ISA 
>> extensions as well.
>>
>> It looks like you are okay with the use case also ?
>
> I don't mind setting new defaults in rv64.
>
>>
>>     That doesn't mean we can't add defaults for rv64, but for this 
>> particular case I wonder if
>>     the 'max' CPU wouldn't be better.
>>
>>
>> Not sure what you mean here. What does 'max' cpu have to do with pmu 
>> extensions ?
>
>
> Save a few exceptions, all the extensions declared in 
> riscv_cpu_extensions[]
> will be enabled in the 'max' CPU, regardless of their default value 
> for the
> rv64 CPU (see riscv_init_max_cpu_extensions() in tcg-cpu.c).
>

Ahh okay. That makes sense. I got confused with maxcpus option.


> If we count both the v4 and this RFC, the following extensions were 
> added in
> riscv_cpu_extensions[]:
>
> +    MULTI_EXT_CFG_BOOL("smcntrpmf", ext_smcntrpmf, false),
>
> +    MULTI_EXT_CFG_BOOL("smcsrind", ext_smcsrind, false),
> +    MULTI_EXT_CFG_BOOL("sscsrind", ext_sscsrind, false),
>
> +    MULTI_EXT_CFG_BOOL("smcdeleg", ext_smcdeleg, false),
> +    MULTI_EXT_CFG_BOOL("ssccfg", ext_ssccfg, false),
>
>
> All of them will be enabled by default in the 'max' CPU.
>
> This is what I was referring to. We can just use the 'max' CPU and 
> don't worry about
> enabling defaults in rv64.
>
We should definitely enable them in 'max' cpu as these extensions are 
ratified.
The comment in the code says it should enable all ratified extensions. 
Is that a guiding policy ?
Qemu allows merging non frozen extensions.

But rv64 still is the default one everyone running. So I feel we should 
enable there as for user convenience.

>
> Thanks,
>
> Daniel
>
>>
>>
>>     Thanks,
>>
>>
>>     Daniel
>>
>>      >
>>      > The first 2 patches decouple the indirect CSR usage from AIA 
>> implementation
>>      > while patch3 adds stateen bits validation for AIA.
>>      > The PATCH4 implements indirect CSR extensions while remaining 
>> patches
>>      > implement the counter delegation extensions.
>>      >
>>      > The Qemu patches can be found here:
>>      > https://github.com/atishp04/qemu/tree/counter_delegation_rfc 
>> <https://github.com/atishp04/qemu/tree/counter_delegation_rfc>
>>      >
>>      > The opensbi patch can be found here:
>>      > https://github.com/atishp04/opensbi/tree/counter_delegation_v1 
>> <https://github.com/atishp04/opensbi/tree/counter_delegation_v1>
>>      >
>>      > The Linux kernel patches can be found here:
>>      > https://github.com/atishp04/linux/tree/counter_delegation_rfc 
>> <https://github.com/atishp04/linux/tree/counter_delegation_rfc>
>>      >
>>      > [1] https://github.com/riscv/riscv-indirect-csr-access 
>> <https://github.com/riscv/riscv-indirect-csr-access>
>>      > [2] https://github.com/riscv/riscv-smcdeleg-ssccfg 
>> <https://github.com/riscv/riscv-smcdeleg-ssccfg>
>>      >
>>      > Atish Patra (1):
>>      > target/riscv: Enable S*stateen bits for AIA
>>      >
>>      > Kaiwen Xue (7):
>>      > target/riscv: Add properties for Indirect CSR Access extension
>>      > target/riscv: Decouple AIA processing from xiselect and xireg
>>      > target/riscv: Support generic CSR indirect access
>>      > target/riscv: Add smcdeleg/ssccfg properties
>>      > target/riscv: Add counter delegation definitions
>>      > target/riscv: Add select value range check for counter delegation
>>      > target/riscv: Add counter delegation/configuration support
>>      >
>>      > target/riscv/cpu.c      |   8 +
>>      > target/riscv/cpu.h      |   1 +
>>      > target/riscv/cpu_bits.h |  34 +-
>>      > target/riscv/cpu_cfg.h  |   4 +
>>      > target/riscv/csr.c      | 713 
>> +++++++++++++++++++++++++++++++++++++---
>>      > target/riscv/machine.c  |   1 +
>>      > 6 files changed, 722 insertions(+), 39 deletions(-)
>>      >
>>      > --
>>      > 2.34.1
>>      >
>>

On 2/21/24 17:17, Atish Patra wrote:
> 
> On 2/21/24 10:26, Daniel Henrique Barboza wrote:
>>
>>
>> On 2/21/24 14:06, Atish Kumar Patra wrote:
>>>
>>>
>>> On Wed, Feb 21, 2024 at 6:58 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com <mailto:dbarboza@ventanamicro.com>> wrote:
>>>
>>>     Hi Atish,
>>>
>>>     This series and its dependency, which I assume it's
>>>
>>>     "[PATCH v4 0/5] Add ISA extension smcntrpmf support"
>>>
>>>     Doesn't apply in neither master nor riscv-to-apply.next because of this patch:
>>>
>>>     "target/riscv: Use RISCVException as return type for all csr ops"
>>>
>>>     That changed some functions from 'int' to "RISCVException" type. The conflicts
>>>     from the v4 series are rather trivial but the conflicts for this RFC are annoying
>>>     to deal with. It would be better if you could re-send both series rebased with
>>>     the latest changes.
>>>
>>>
>>> I was waiting for Alistair's ACK on the smcntrpmf series as he had some comments. It looks like he is okay
>>> with the series now (no further questions).  Let me respin both the series.
>>>
>>>     One more thing:
>>>
>>>     On 2/16/24 21:01, Atish Patra wrote:
>>>      > This series adds the counter delegation extension support. The counter
>>>      > delegation ISA extension(Smcdeleg/Ssccfg) actually depends on multiple ISA
>>>      > extensions.
>>>      >
>>>      > 1. S[m|s]csrind : The indirect CSR extension[1] which defines additional
>>>      >     5 ([M|S|VS]IREG2-[M|S|VS]IREG6) register to address size limitation of
>>>      >     RISC-V CSR address space.
>>>      > 2. Smstateen: The stateen bit[60] controls the access to the registers
>>>      >     indirectly via the above indirect registers.
>>>      > 3. Smcdeleg/Ssccfg: The counter delegation extensions[2]
>>>      >
>>>      > The counter delegation extension allows Supervisor mode to program the
>>>      > hpmevent and hpmcounters directly without needing the assistance from the
>>>      > M-mode via SBI calls. This results in a faster perf profiling and very
>>>      > few traps. This extension also introduces a scountinhibit CSR which allows
>>>      > to stop/start any counter directly from the S-mode. As the counter
>>>      > delegation extension potentially can have more than 100 CSRs, the specificaiton
>>>      > leverages the indirect CSR extension to save the precious CSR address range.
>>>      >
>>>      > Due to the dependancy of these extensions, the following extensions must be
>>>      > enabled to use the counter delegation feature in S-mode.
>>>      >
>>>      > "smstateen=true,sscofpmf=true,ssccfg=true,smcdeleg=true,smcsrind=true,sscsrind=true"
>>>      >
>>>      > This makes the qemu command line quite tedious. In stead of that, I think we
>>>      > can enable these features by default if there is no objection.
>>>
>>>     It wasn't need so far but, if needed, we can add specialized setters for extensions
>>>     that has multiple dependencies. Instead of the usual setter we would do something
>>>     like:
>>>
>>>     cpu_set_ssccfg() {
>>>
>>>           if (enabled) {
>>>               smstateen=true
>>>               sscofpmf=true
>>>               smcdeleg=true
>>>               smcsrind=true
>>>               sscsrind=true
>>>           }
>>>     }
>>>
>>>
>>>     The advantage is that this setter would also work for CPUs that doesn't inherit defaults,
>>>     like bare-cps and profile CPUs.
>>>
>>>
>>> Your suggested approach looks good to me. But I was asking about concerns about enabling these extensions
>>> by default rather than the actual mechanism to implement it. Few of the extensions listed here such as smstateen,smcsrind
>>> sscsrind are independent ISA extensions which are used for other ISA extensions as well.
>>>
>>> It looks like you are okay with the use case also ?
>>
>> I don't mind setting new defaults in rv64.
>>
>>>
>>>     That doesn't mean we can't add defaults for rv64, but for this particular case I wonder if
>>>     the 'max' CPU wouldn't be better.
>>>
>>>
>>> Not sure what you mean here. What does 'max' cpu have to do with pmu extensions ?
>>
>>
>> Save a few exceptions, all the extensions declared in riscv_cpu_extensions[]
>> will be enabled in the 'max' CPU, regardless of their default value for the
>> rv64 CPU (see riscv_init_max_cpu_extensions() in tcg-cpu.c).
>>
> 
> Ahh okay. That makes sense. I got confused with maxcpus option.
> 
> 
>> If we count both the v4 and this RFC, the following extensions were added in
>> riscv_cpu_extensions[]:
>>
>> +    MULTI_EXT_CFG_BOOL("smcntrpmf", ext_smcntrpmf, false),
>>
>> +    MULTI_EXT_CFG_BOOL("smcsrind", ext_smcsrind, false),
>> +    MULTI_EXT_CFG_BOOL("sscsrind", ext_sscsrind, false),
>>
>> +    MULTI_EXT_CFG_BOOL("smcdeleg", ext_smcdeleg, false),
>> +    MULTI_EXT_CFG_BOOL("ssccfg", ext_ssccfg, false),
>>
>>
>> All of them will be enabled by default in the 'max' CPU.
>>
>> This is what I was referring to. We can just use the 'max' CPU and don't worry about
>> enabling defaults in rv64.
>>
> We should definitely enable them in 'max' cpu as these extensions are ratified.
> The comment in the code says it should enable all ratified extensions. Is that a guiding policy ?
> Qemu allows merging non frozen extensions.

I guess we need to update that comment ... we're been accepting "frozen" extensions as
stable (i.e. no need for the leading "x-") recently. It doesn't have to be ratified.

> 
> But rv64 still is the default one everyone running. So I feel we should enable there as for user convenience.

I've been talking with Drew about making 'max' the default CPU for riscv64. Users
that don't bother with any particular CPU can then use the most feature-rich CPU we
can provide by default. rv64 will still be around for people that wants more control,
although for real control of what the CPU is enabling or not I advise using rv64i or
even a profile CPU.

But anyway, as I said in an earlier reply, I don't mind enabling more defaults in rv64,
especially in a case where one extension have 3+ dependencies.


Thanks,

Daniel


> 
>>
>> Thanks,
>>
>> Daniel
>>
>>>
>>>
>>>     Thanks,
>>>
>>>
>>>     Daniel
>>>
>>>      >
>>>      > The first 2 patches decouple the indirect CSR usage from AIA implementation
>>>      > while patch3 adds stateen bits validation for AIA.
>>>      > The PATCH4 implements indirect CSR extensions while remaining patches
>>>      > implement the counter delegation extensions.
>>>      >
>>>      > The Qemu patches can be found here:
>>>      > https://github.com/atishp04/qemu/tree/counter_delegation_rfc <https://github.com/atishp04/qemu/tree/counter_delegation_rfc>
>>>      >
>>>      > The opensbi patch can be found here:
>>>      > https://github.com/atishp04/opensbi/tree/counter_delegation_v1 <https://github.com/atishp04/opensbi/tree/counter_delegation_v1>
>>>      >
>>>      > The Linux kernel patches can be found here:
>>>      > https://github.com/atishp04/linux/tree/counter_delegation_rfc <https://github.com/atishp04/linux/tree/counter_delegation_rfc>
>>>      >
>>>      > [1] https://github.com/riscv/riscv-indirect-csr-access <https://github.com/riscv/riscv-indirect-csr-access>
>>>      > [2] https://github.com/riscv/riscv-smcdeleg-ssccfg <https://github.com/riscv/riscv-smcdeleg-ssccfg>
>>>      >
>>>      > Atish Patra (1):
>>>      > target/riscv: Enable S*stateen bits for AIA
>>>      >
>>>      > Kaiwen Xue (7):
>>>      > target/riscv: Add properties for Indirect CSR Access extension
>>>      > target/riscv: Decouple AIA processing from xiselect and xireg
>>>      > target/riscv: Support generic CSR indirect access
>>>      > target/riscv: Add smcdeleg/ssccfg properties
>>>      > target/riscv: Add counter delegation definitions
>>>      > target/riscv: Add select value range check for counter delegation
>>>      > target/riscv: Add counter delegation/configuration support
>>>      >
>>>      > target/riscv/cpu.c      |   8 +
>>>      > target/riscv/cpu.h      |   1 +
>>>      > target/riscv/cpu_bits.h |  34 +-
>>>      > target/riscv/cpu_cfg.h  |   4 +
>>>      > target/riscv/csr.c      | 713 +++++++++++++++++++++++++++++++++++++---
>>>      > target/riscv/machine.c  |   1 +
>>>      > 6 files changed, 722 insertions(+), 39 deletions(-)
>>>      >
>>>      > --
>>>      > 2.34.1
>>>      >
>>>