* [PATCH v3 01/11] target/riscv: Add properties for Indirect CSR Access extension
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-28 11:39 ` Daniel Henrique Barboza
2024-11-18 1:15 ` [PATCH v3 02/11] target/riscv: Decouple AIA processing from xiselect and xireg Atish Patra
` (9 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis, Kaiwen Xue
From: Kaiwen Xue <kaiwenx@rivosinc.com>
This adds the properties for sxcsrind. Definitions of new registers and
implementations will come with future patches.
Signed-off-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
---
target/riscv/cpu.c | 2 ++
target/riscv/cpu_cfg.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f219f0c3b527..963f1f3af9ae 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -185,12 +185,14 @@ const RISCVIsaExtData isa_edata_arr[] = {
ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
+ ISA_EXT_DATA_ENTRY(smcsrind, PRIV_VERSION_1_13_0, ext_smcsrind),
ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
+ ISA_EXT_DATA_ENTRY(sscsrind, PRIV_VERSION_1_12_0, ext_sscsrind),
ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 59d6fc445d18..8b974255f6fb 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -79,6 +79,8 @@ struct RISCVCPUConfig {
bool ext_smstateen;
bool ext_sstc;
bool ext_smcntrpmf;
+ bool ext_smcsrind;
+ bool ext_sscsrind;
bool ext_svadu;
bool ext_svinval;
bool ext_svnapot;
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 01/11] target/riscv: Add properties for Indirect CSR Access extension
2024-11-18 1:15 ` [PATCH v3 01/11] target/riscv: Add properties for Indirect CSR Access extension Atish Patra
@ 2024-11-28 11:39 ` Daniel Henrique Barboza
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Henrique Barboza @ 2024-11-28 11:39 UTC (permalink / raw)
To: Atish Patra, qemu-riscv, qemu-devel
Cc: kaiwenxue1, palmer, liwei1518, zhiwei_liu, bin.meng,
alistair.francis, Kaiwen Xue
On 11/17/24 10:15 PM, Atish Patra wrote:
> From: Kaiwen Xue <kaiwenx@rivosinc.com>
>
> This adds the properties for sxcsrind. Definitions of new registers and
> implementations will come with future patches.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/cpu.c | 2 ++
> target/riscv/cpu_cfg.h | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f219f0c3b527..963f1f3af9ae 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -185,12 +185,14 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
> ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
> ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
> + ISA_EXT_DATA_ENTRY(smcsrind, PRIV_VERSION_1_13_0, ext_smcsrind),
> ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
> ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
> + ISA_EXT_DATA_ENTRY(sscsrind, PRIV_VERSION_1_12_0, ext_sscsrind),
> ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
> ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 59d6fc445d18..8b974255f6fb 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -79,6 +79,8 @@ struct RISCVCPUConfig {
> bool ext_smstateen;
> bool ext_sstc;
> bool ext_smcntrpmf;
> + bool ext_smcsrind;
> + bool ext_sscsrind;
> bool ext_svadu;
> bool ext_svinval;
> bool ext_svnapot;
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 02/11] target/riscv: Decouple AIA processing from xiselect and xireg
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
2024-11-18 1:15 ` [PATCH v3 01/11] target/riscv: Add properties for Indirect CSR Access extension Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-18 1:15 ` [PATCH v3 03/11] target/riscv: Enable S*stateen bits for AIA Atish Patra
` (8 subsequent siblings)
10 siblings, 0 replies; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis, Kaiwen Xue
From: Kaiwen Xue <kaiwenx@rivosinc.com>
Since xiselect and xireg also will be of use in sxcsrind, AIA should
have its own separated interface when those CSRs are accessed.
Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/csr.c | 165 ++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 139 insertions(+), 26 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9846770820f4..52e0139fc99c 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -29,6 +29,7 @@
#include "sysemu/cpu-timers.h"
#include "qemu/guest-random.h"
#include "qapi/error.h"
+#include <stdbool.h>
/* CSR function table public API */
void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops)
@@ -305,6 +306,15 @@ static RISCVException aia_any32(CPURISCVState *env, int csrno)
return any32(env, csrno);
}
+static RISCVException csrind_or_aia_any(CPURISCVState *env, int csrno)
+{
+ if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ return any(env, csrno);
+}
+
static RISCVException smode(CPURISCVState *env, int csrno)
{
if (riscv_has_ext(env, RVS)) {
@@ -341,6 +351,30 @@ static RISCVException aia_smode32(CPURISCVState *env, int csrno)
return smode32(env, csrno);
}
+static bool csrind_extensions_present(CPURISCVState *env)
+{
+ return riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind;
+}
+
+static bool aia_extensions_present(CPURISCVState *env)
+{
+ return riscv_cpu_cfg(env)->ext_smaia || riscv_cpu_cfg(env)->ext_ssaia;
+}
+
+static bool csrind_or_aia_extensions_present(CPURISCVState *env)
+{
+ return csrind_extensions_present(env) || aia_extensions_present(env);
+}
+
+static RISCVException csrind_or_aia_smode(CPURISCVState *env, int csrno)
+{
+ if (!csrind_or_aia_extensions_present(env)) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ return smode(env, csrno);
+}
+
static RISCVException hmode(CPURISCVState *env, int csrno)
{
if (riscv_has_ext(env, RVH)) {
@@ -360,6 +394,15 @@ static RISCVException hmode32(CPURISCVState *env, int csrno)
}
+static RISCVException csrind_or_aia_hmode(CPURISCVState *env, int csrno)
+{
+ if (!csrind_or_aia_extensions_present(env)) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ return hmode(env, csrno);
+}
+
static RISCVException umode(CPURISCVState *env, int csrno)
{
if (riscv_has_ext(env, RVU)) {
@@ -1966,6 +2009,22 @@ static int aia_xlate_vs_csrno(CPURISCVState *env, int csrno)
};
}
+static int csrind_xlate_vs_csrno(CPURISCVState *env, int csrno)
+{
+ if (!env->virt_enabled) {
+ return csrno;
+ }
+
+ switch (csrno) {
+ case CSR_SISELECT:
+ return CSR_VSISELECT;
+ case CSR_SIREG:
+ return CSR_VSIREG;
+ default:
+ return csrno;
+ };
+}
+
static RISCVException rmw_xiselect(CPURISCVState *env, int csrno,
target_ulong *val, target_ulong new_val,
target_ulong wr_mask)
@@ -1973,7 +2032,7 @@ static RISCVException rmw_xiselect(CPURISCVState *env, int csrno,
target_ulong *iselect;
/* Translate CSR number for VS-mode */
- csrno = aia_xlate_vs_csrno(env, csrno);
+ csrno = csrind_xlate_vs_csrno(env, csrno);
/* Find the iselect CSR based on CSR number */
switch (csrno) {
@@ -2002,6 +2061,12 @@ static RISCVException rmw_xiselect(CPURISCVState *env, int csrno,
return RISCV_EXCP_NONE;
}
+static bool xiselect_aia_range(target_ulong isel)
+{
+ return (ISELECT_IPRIO0 <= isel && isel <= ISELECT_IPRIO15) ||
+ (ISELECT_IMSIC_FIRST <= isel && isel <= ISELECT_IMSIC_LAST);
+}
+
static int rmw_iprio(target_ulong xlen,
target_ulong iselect, uint8_t *iprio,
target_ulong *val, target_ulong new_val,
@@ -2047,45 +2112,44 @@ static int rmw_iprio(target_ulong xlen,
return 0;
}
-static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
- target_ulong *val, target_ulong new_val,
- target_ulong wr_mask)
+static RISCVException rmw_xireg_aia(CPURISCVState *env, int csrno,
+ target_ulong isel, target_ulong *val,
+ target_ulong new_val, target_ulong wr_mask)
{
- bool virt, isel_reserved;
- uint8_t *iprio;
+ bool virt = false, isel_reserved = false;
int ret = -EINVAL;
- target_ulong priv, isel, vgein;
-
- /* Translate CSR number for VS-mode */
- csrno = aia_xlate_vs_csrno(env, csrno);
+ uint8_t *iprio;
+ target_ulong priv, vgein;
- /* Decode register details from CSR number */
- virt = false;
- isel_reserved = false;
+ /* VS-mode CSR number passed in has already been translated */
switch (csrno) {
case CSR_MIREG:
+ if (!riscv_cpu_cfg(env)->ext_smaia) {
+ goto done;
+ }
iprio = env->miprio;
- isel = env->miselect;
priv = PRV_M;
break;
case CSR_SIREG:
- if (env->priv == PRV_S && env->mvien & MIP_SEIP &&
+ if (!riscv_cpu_cfg(env)->ext_ssaia ||
+ (env->priv == PRV_S && env->mvien & MIP_SEIP &&
env->siselect >= ISELECT_IMSIC_EIDELIVERY &&
- env->siselect <= ISELECT_IMSIC_EIE63) {
+ env->siselect <= ISELECT_IMSIC_EIE63)) {
goto done;
}
iprio = env->siprio;
- isel = env->siselect;
priv = PRV_S;
break;
case CSR_VSIREG:
+ if (!riscv_cpu_cfg(env)->ext_ssaia) {
+ goto done;
+ }
iprio = env->hviprio;
- isel = env->vsiselect;
priv = PRV_S;
virt = true;
break;
default:
- goto done;
+ goto done;
};
/* Find the selected guest interrupt file */
@@ -2116,10 +2180,54 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
}
done:
+ /*
+ * If AIA is not enabled, illegal instruction exception is always
+ * returned regardless of whether we are in VS-mode or not
+ */
if (ret) {
return (env->virt_enabled && virt && !isel_reserved) ?
RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
}
+
+ return RISCV_EXCP_NONE;
+}
+
+static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
+ target_ulong *val, target_ulong new_val,
+ target_ulong wr_mask)
+{
+ bool virt = false;
+ int ret = -EINVAL;
+ target_ulong isel;
+
+ /* Translate CSR number for VS-mode */
+ csrno = csrind_xlate_vs_csrno(env, csrno);
+
+ /* Decode register details from CSR number */
+ switch (csrno) {
+ case CSR_MIREG:
+ isel = env->miselect;
+ break;
+ case CSR_SIREG:
+ isel = env->siselect;
+ break;
+ case CSR_VSIREG:
+ isel = env->vsiselect;
+ virt = true;
+ break;
+ default:
+ goto done;
+ };
+
+ if (xiselect_aia_range(isel)) {
+ return rmw_xireg_aia(env, csrno, isel, val, new_val, wr_mask);
+ }
+
+done:
+ if (ret) {
+ return (env->virt_enabled && virt) ?
+ RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
+ }
return RISCV_EXCP_NONE;
}
@@ -5065,8 +5173,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
[CSR_MIP] = { "mip", any, NULL, NULL, rmw_mip },
/* Machine-Level Window to Indirectly Accessed Registers (AIA) */
- [CSR_MISELECT] = { "miselect", aia_any, NULL, NULL, rmw_xiselect },
- [CSR_MIREG] = { "mireg", aia_any, NULL, NULL, rmw_xireg },
+ [CSR_MISELECT] = { "miselect", csrind_or_aia_any, NULL, NULL,
+ rmw_xiselect },
+ [CSR_MIREG] = { "mireg", csrind_or_aia_any, NULL, NULL,
+ rmw_xireg },
/* Machine-Level Interrupts (AIA) */
[CSR_MTOPEI] = { "mtopei", aia_any, NULL, NULL, rmw_xtopei },
@@ -5184,8 +5294,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
[CSR_SATP] = { "satp", satp, read_satp, write_satp },
/* Supervisor-Level Window to Indirectly Accessed Registers (AIA) */
- [CSR_SISELECT] = { "siselect", aia_smode, NULL, NULL, rmw_xiselect },
- [CSR_SIREG] = { "sireg", aia_smode, NULL, NULL, rmw_xireg },
+ [CSR_SISELECT] = { "siselect", csrind_or_aia_smode, NULL, NULL,
+ rmw_xiselect },
+ [CSR_SIREG] = { "sireg", csrind_or_aia_smode, NULL, NULL,
+ rmw_xireg },
/* Supervisor-Level Interrupts (AIA) */
[CSR_STOPEI] = { "stopei", aia_smode, NULL, NULL, rmw_xtopei },
@@ -5264,9 +5376,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
/*
* VS-Level Window to Indirectly Accessed Registers (H-extension with AIA)
*/
- [CSR_VSISELECT] = { "vsiselect", aia_hmode, NULL, NULL,
- rmw_xiselect },
- [CSR_VSIREG] = { "vsireg", aia_hmode, NULL, NULL, rmw_xireg },
+ [CSR_VSISELECT] = { "vsiselect", csrind_or_aia_hmode, NULL, NULL,
+ rmw_xiselect },
+ [CSR_VSIREG] = { "vsireg", csrind_or_aia_hmode, NULL, NULL,
+ rmw_xireg },
/* VS-Level Interrupts (H-extension with AIA) */
[CSR_VSTOPEI] = { "vstopei", aia_hmode, NULL, NULL, rmw_xtopei },
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 03/11] target/riscv: Enable S*stateen bits for AIA
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
2024-11-18 1:15 ` [PATCH v3 01/11] target/riscv: Add properties for Indirect CSR Access extension Atish Patra
2024-11-18 1:15 ` [PATCH v3 02/11] target/riscv: Decouple AIA processing from xiselect and xireg Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-18 1:15 ` [PATCH v3 04/11] target/riscv: Support generic CSR indirect access Atish Patra
` (7 subsequent siblings)
10 siblings, 0 replies; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis
As per the ratified AIA spec v1.0, three stateen bits control AIA CSR
access.
Bit 60 controls the indirect CSRs
Bit 59 controls the most AIA CSR state
Bit 58 controls the IMSIC state such as stopei and vstopei
Enable the corresponding bits in [m|h]stateen and enable corresponding
checks in the CSR accessor functions.
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/csr.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 84 insertions(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 52e0139fc99c..c91a26a52ef6 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -335,19 +335,42 @@ static RISCVException smode32(CPURISCVState *env, int csrno)
static RISCVException aia_smode(CPURISCVState *env, int csrno)
{
+ int ret;
+
if (!riscv_cpu_cfg(env)->ext_ssaia) {
return RISCV_EXCP_ILLEGAL_INST;
}
+ if (csrno == CSR_STOPEI) {
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
+ } else {
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
+ }
+
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
+
return smode(env, csrno);
}
static RISCVException aia_smode32(CPURISCVState *env, int csrno)
{
+ int ret;
+
if (!riscv_cpu_cfg(env)->ext_ssaia) {
return RISCV_EXCP_ILLEGAL_INST;
}
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
+
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
+
return smode32(env, csrno);
}
@@ -586,15 +609,38 @@ static RISCVException pointer_masking(CPURISCVState *env, int csrno)
static RISCVException aia_hmode(CPURISCVState *env, int csrno)
{
+ int ret;
+
if (!riscv_cpu_cfg(env)->ext_ssaia) {
return RISCV_EXCP_ILLEGAL_INST;
}
- return hmode(env, csrno);
+ if (csrno == CSR_VSTOPEI) {
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
+ } else {
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
+ }
+
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
+
+ return hmode(env, csrno);
}
static RISCVException aia_hmode32(CPURISCVState *env, int csrno)
{
+ int ret;
+
+ if (!riscv_cpu_cfg(env)->ext_ssaia) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_AIA);
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
+
if (!riscv_cpu_cfg(env)->ext_ssaia) {
return RISCV_EXCP_ILLEGAL_INST;
}
@@ -2030,6 +2076,12 @@ static RISCVException rmw_xiselect(CPURISCVState *env, int csrno,
target_ulong wr_mask)
{
target_ulong *iselect;
+ int ret;
+
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SVSLCT);
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
/* Translate CSR number for VS-mode */
csrno = csrind_xlate_vs_csrno(env, csrno);
@@ -2200,6 +2252,11 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
int ret = -EINVAL;
target_ulong isel;
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SVSLCT);
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
+
/* Translate CSR number for VS-mode */
csrno = csrind_xlate_vs_csrno(env, csrno);
@@ -2678,6 +2735,19 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
wr_mask |= SMSTATEEN0_P1P13;
}
+ if (riscv_cpu_cfg(env)->ext_smaia) {
+ wr_mask |= SMSTATEEN0_SVSLCT;
+ }
+
+ /*
+ * As per the AIA specification, SMSTATEEN0_IMSIC is valid only if IMSIC is
+ * implemented. However, that information is with MachineState and we can't
+ * figure that out in csr.c. Just enable if Smaia is available.
+ */
+ if (riscv_cpu_cfg(env)->ext_smaia) {
+ wr_mask |= (SMSTATEEN0_AIA | SMSTATEEN0_IMSIC);
+ }
+
return write_mstateen(env, csrno, wr_mask, new_val);
}
@@ -2758,6 +2828,19 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
wr_mask |= SMSTATEEN0_FCSR;
}
+ if (riscv_cpu_cfg(env)->ext_ssaia) {
+ wr_mask |= SMSTATEEN0_SVSLCT;
+ }
+
+ /*
+ * As per the AIA specification, SMSTATEEN0_IMSIC is valid only if IMSIC is
+ * implemented. However, that information is with MachineState and we can't
+ * figure that out in csr.c. Just enable if Ssaia is available.
+ */
+ if (riscv_cpu_cfg(env)->ext_ssaia) {
+ wr_mask |= (SMSTATEEN0_AIA | SMSTATEEN0_IMSIC);
+ }
+
return write_hstateen(env, csrno, wr_mask, new_val);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 04/11] target/riscv: Support generic CSR indirect access
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (2 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 03/11] target/riscv: Enable S*stateen bits for AIA Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-28 11:52 ` Daniel Henrique Barboza
2024-11-18 1:15 ` [PATCH v3 05/11] target/riscv: Add properties for counter delegation ISA extensions Atish Patra
` (6 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis, Kaiwen Xue
From: Kaiwen Xue <kaiwenx@rivosinc.com>
This adds the indirect access registers required by sscsrind/smcsrind
and the operations on them. Note that xiselect and xireg are used for
both AIA and sxcsrind, and the behavior of accessing them depends on
whether each extension is enabled and the value stored in xiselect.
Co-developed-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
---
target/riscv/cpu_bits.h | 28 ++++++++-
target/riscv/csr.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 171 insertions(+), 6 deletions(-)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 385a2c67c24b..e13c5420a251 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -173,6 +173,13 @@
#define CSR_MISELECT 0x350
#define CSR_MIREG 0x351
+/* Machine Indirect Register Alias */
+#define CSR_MIREG2 0x352
+#define CSR_MIREG3 0x353
+#define CSR_MIREG4 0x355
+#define CSR_MIREG5 0x356
+#define CSR_MIREG6 0x357
+
/* Machine-Level Interrupts (AIA) */
#define CSR_MTOPEI 0x35c
#define CSR_MTOPI 0xfb0
@@ -222,6 +229,13 @@
#define CSR_SISELECT 0x150
#define CSR_SIREG 0x151
+/* Supervisor Indirect Register Alias */
+#define CSR_SIREG2 0x152
+#define CSR_SIREG3 0x153
+#define CSR_SIREG4 0x155
+#define CSR_SIREG5 0x156
+#define CSR_SIREG6 0x157
+
/* Supervisor-Level Interrupts (AIA) */
#define CSR_STOPEI 0x15c
#define CSR_STOPI 0xdb0
@@ -288,6 +302,13 @@
#define CSR_VSISELECT 0x250
#define CSR_VSIREG 0x251
+/* Virtual Supervisor Indirect Alias */
+#define CSR_VSIREG2 0x252
+#define CSR_VSIREG3 0x253
+#define CSR_VSIREG4 0x255
+#define CSR_VSIREG5 0x256
+#define CSR_VSIREG6 0x257
+
/* VS-Level Interrupts (H-extension with AIA) */
#define CSR_VSTOPEI 0x25c
#define CSR_VSTOPI 0xeb0
@@ -863,10 +884,13 @@ typedef enum RISCVException {
#define ISELECT_IMSIC_EIE63 0xff
#define ISELECT_IMSIC_FIRST ISELECT_IMSIC_EIDELIVERY
#define ISELECT_IMSIC_LAST ISELECT_IMSIC_EIE63
-#define ISELECT_MASK 0x1ff
+#define ISELECT_MASK_AIA 0x1ff
+
+/* MISELECT, SISELECT, and VSISELECT bits (AIA) */
+#define ISELECT_MASK_SXCSRIND 0xfff
/* Dummy [M|S|VS]ISELECT value for emulating [M|S|VS]TOPEI CSRs */
-#define ISELECT_IMSIC_TOPEI (ISELECT_MASK + 1)
+#define ISELECT_IMSIC_TOPEI (ISELECT_MASK_AIA + 1)
/* IMSIC bits (AIA) */
#define IMSIC_TOPEI_IID_SHIFT 16
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index c91a26a52ef6..57e9c9e25f02 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -306,6 +306,17 @@ static RISCVException aia_any32(CPURISCVState *env, int csrno)
return any32(env, csrno);
}
+static RISCVException csrind_any(CPURISCVState *env, int csrno)
+{
+ RISCVCPU *cpu = env_archcpu(env);
+
+ if (!cpu->cfg.ext_smcsrind) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ return RISCV_EXCP_NONE;
+}
+
static RISCVException csrind_or_aia_any(CPURISCVState *env, int csrno)
{
if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) {
@@ -389,6 +400,15 @@ static bool csrind_or_aia_extensions_present(CPURISCVState *env)
return csrind_extensions_present(env) || aia_extensions_present(env);
}
+static RISCVException csrind_smode(CPURISCVState *env, int csrno)
+{
+ if (!csrind_extensions_present(env)) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ return smode(env, csrno);
+}
+
static RISCVException csrind_or_aia_smode(CPURISCVState *env, int csrno)
{
if (!csrind_or_aia_extensions_present(env)) {
@@ -417,6 +437,15 @@ static RISCVException hmode32(CPURISCVState *env, int csrno)
}
+static RISCVException csrind_hmode(CPURISCVState *env, int csrno)
+{
+ if (!csrind_extensions_present(env)) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ return hmode(env, csrno);
+}
+
static RISCVException csrind_or_aia_hmode(CPURISCVState *env, int csrno)
{
if (!csrind_or_aia_extensions_present(env)) {
@@ -2065,7 +2094,12 @@ static int csrind_xlate_vs_csrno(CPURISCVState *env, int csrno)
case CSR_SISELECT:
return CSR_VSISELECT;
case CSR_SIREG:
- return CSR_VSIREG;
+ case CSR_SIREG2:
+ case CSR_SIREG3:
+ case CSR_SIREG4:
+ case CSR_SIREG5:
+ case CSR_SIREG6:
+ return CSR_VSIREG + (csrno - CSR_SIREG);
default:
return csrno;
};
@@ -2105,7 +2139,12 @@ static RISCVException rmw_xiselect(CPURISCVState *env, int csrno,
*val = *iselect;
}
- wr_mask &= ISELECT_MASK;
+ if (riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind) {
+ wr_mask &= ISELECT_MASK_SXCSRIND;
+ } else {
+ wr_mask &= ISELECT_MASK_AIA;
+ }
+
if (wr_mask) {
*iselect = (*iselect & ~wr_mask) | (new_val & wr_mask);
}
@@ -2244,6 +2283,59 @@ done:
return RISCV_EXCP_NONE;
}
+/*
+ * rmw_xireg_csrind: Perform indirect access to xireg and xireg2-xireg6
+ *
+ * Perform indirect access to xireg and xireg2-xireg6.
+ * This is a generic interface for all xireg CSRs. Apart from AIA, all other
+ * extension using csrind should be implemented here.
+ */
+static int rmw_xireg_csrind(CPURISCVState *env, int csrno,
+ target_ulong isel, target_ulong *val,
+ target_ulong new_val, target_ulong wr_mask)
+{
+ return -EINVAL;
+}
+
+static int rmw_xiregi(CPURISCVState *env, int csrno, target_ulong *val,
+ target_ulong new_val, target_ulong wr_mask)
+{
+ bool virt = false;
+ int ret = -EINVAL;
+ target_ulong isel;
+
+ ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SVSLCT);
+ if (ret != RISCV_EXCP_NONE) {
+ return ret;
+ }
+
+ /* Translate CSR number for VS-mode */
+ csrno = csrind_xlate_vs_csrno(env, csrno);
+
+ if (CSR_MIREG <= csrno && csrno <= CSR_MIREG6 &&
+ csrno != CSR_MIREG4 - 1) {
+ isel = env->miselect;
+ } else if (CSR_SIREG <= csrno && csrno <= CSR_SIREG6 &&
+ csrno != CSR_SIREG4 - 1) {
+ isel = env->siselect;
+ } else if (CSR_VSIREG <= csrno && csrno <= CSR_VSIREG6 &&
+ csrno != CSR_VSIREG4 - 1) {
+ isel = env->vsiselect;
+ virt = true;
+ } else {
+ goto done;
+ }
+
+ return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
+
+done:
+ if (ret) {
+ return (env->virt_enabled && virt) ?
+ RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
+ }
+ return RISCV_EXCP_NONE;
+}
+
static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
target_ulong *val, target_ulong new_val,
target_ulong wr_mask)
@@ -2276,8 +2368,21 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
goto done;
};
+ /*
+ * Use the xiselect range to determine actual op on xireg.
+ *
+ * Since we only checked the existence of AIA or Indirect Access in the
+ * predicate, we should check the existence of the exact extension when
+ * we get to a specific range and return illegal instruction exception even
+ * in VS-mode.
+ */
if (xiselect_aia_range(isel)) {
return rmw_xireg_aia(env, csrno, isel, val, new_val, wr_mask);
+ } else if (riscv_cpu_cfg(env)->ext_smcsrind ||
+ riscv_cpu_cfg(env)->ext_sscsrind) {
+ return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
+ } else {
+ return RISCV_EXCP_ILLEGAL_INST;
}
done:
@@ -2735,7 +2840,7 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
wr_mask |= SMSTATEEN0_P1P13;
}
- if (riscv_cpu_cfg(env)->ext_smaia) {
+ if (riscv_cpu_cfg(env)->ext_smaia || riscv_cpu_cfg(env)->ext_smcsrind) {
wr_mask |= SMSTATEEN0_SVSLCT;
}
@@ -2828,7 +2933,7 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
wr_mask |= SMSTATEEN0_FCSR;
}
- if (riscv_cpu_cfg(env)->ext_ssaia) {
+ if (riscv_cpu_cfg(env)->ext_ssaia || riscv_cpu_cfg(env)->ext_sscsrind) {
wr_mask |= SMSTATEEN0_SVSLCT;
}
@@ -5261,6 +5366,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
[CSR_MIREG] = { "mireg", csrind_or_aia_any, NULL, NULL,
rmw_xireg },
+ /* Machine Indirect Register Alias */
+ [CSR_MIREG2] = { "mireg2", csrind_any, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_MIREG3] = { "mireg3", csrind_any, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_MIREG4] = { "mireg4", csrind_any, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_MIREG5] = { "mireg5", csrind_any, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_MIREG6] = { "mireg6", csrind_any, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+
/* Machine-Level Interrupts (AIA) */
[CSR_MTOPEI] = { "mtopei", aia_any, NULL, NULL, rmw_xtopei },
[CSR_MTOPI] = { "mtopi", aia_any, read_mtopi },
@@ -5382,6 +5499,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
[CSR_SIREG] = { "sireg", csrind_or_aia_smode, NULL, NULL,
rmw_xireg },
+ /* Supervisor Indirect Register Alias */
+ [CSR_SIREG2] = { "sireg2", csrind_smode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_SIREG3] = { "sireg3", csrind_smode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_SIREG4] = { "sireg4", csrind_smode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_SIREG5] = { "sireg5", csrind_smode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_SIREG6] = { "sireg6", csrind_smode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+
/* Supervisor-Level Interrupts (AIA) */
[CSR_STOPEI] = { "stopei", aia_smode, NULL, NULL, rmw_xtopei },
[CSR_STOPI] = { "stopi", aia_smode, read_stopi },
@@ -5464,6 +5593,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
[CSR_VSIREG] = { "vsireg", csrind_or_aia_hmode, NULL, NULL,
rmw_xireg },
+ /* Virtual Supervisor Indirect Alias */
+ [CSR_VSIREG2] = { "vsireg2", csrind_hmode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_VSIREG3] = { "vsireg3", csrind_hmode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_VSIREG4] = { "vsireg4", csrind_hmode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_VSIREG5] = { "vsireg5", csrind_hmode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+ [CSR_VSIREG6] = { "vsireg6", csrind_hmode, NULL, NULL, rmw_xiregi,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+
/* VS-Level Interrupts (H-extension with AIA) */
[CSR_VSTOPEI] = { "vstopei", aia_hmode, NULL, NULL, rmw_xtopei },
[CSR_VSTOPI] = { "vstopi", aia_hmode, read_vstopi },
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 04/11] target/riscv: Support generic CSR indirect access
2024-11-18 1:15 ` [PATCH v3 04/11] target/riscv: Support generic CSR indirect access Atish Patra
@ 2024-11-28 11:52 ` Daniel Henrique Barboza
2024-11-28 12:52 ` Richard Henderson
2024-12-02 21:16 ` Atish Kumar Patra
0 siblings, 2 replies; 23+ messages in thread
From: Daniel Henrique Barboza @ 2024-11-28 11:52 UTC (permalink / raw)
To: Atish Patra, qemu-riscv, qemu-devel
Cc: kaiwenxue1, palmer, liwei1518, zhiwei_liu, bin.meng,
alistair.francis, Kaiwen Xue
On 11/17/24 10:15 PM, Atish Patra wrote:
> From: Kaiwen Xue <kaiwenx@rivosinc.com>
>
> This adds the indirect access registers required by sscsrind/smcsrind
> and the operations on them. Note that xiselect and xireg are used for
> both AIA and sxcsrind, and the behavior of accessing them depends on
> whether each extension is enabled and the value stored in xiselect.
>
> Co-developed-by: Atish Patra <atishp@rivosinc.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> ---
> target/riscv/cpu_bits.h | 28 ++++++++-
> target/riscv/csr.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 171 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 385a2c67c24b..e13c5420a251 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -173,6 +173,13 @@
> #define CSR_MISELECT 0x350
> #define CSR_MIREG 0x351
>
> +/* Machine Indirect Register Alias */
> +#define CSR_MIREG2 0x352
> +#define CSR_MIREG3 0x353
> +#define CSR_MIREG4 0x355
> +#define CSR_MIREG5 0x356
> +#define CSR_MIREG6 0x357
> +
> /* Machine-Level Interrupts (AIA) */
> #define CSR_MTOPEI 0x35c
> #define CSR_MTOPI 0xfb0
> @@ -222,6 +229,13 @@
> #define CSR_SISELECT 0x150
> #define CSR_SIREG 0x151
>
> +/* Supervisor Indirect Register Alias */
> +#define CSR_SIREG2 0x152
> +#define CSR_SIREG3 0x153
> +#define CSR_SIREG4 0x155
> +#define CSR_SIREG5 0x156
> +#define CSR_SIREG6 0x157
> +
> /* Supervisor-Level Interrupts (AIA) */
> #define CSR_STOPEI 0x15c
> #define CSR_STOPI 0xdb0
> @@ -288,6 +302,13 @@
> #define CSR_VSISELECT 0x250
> #define CSR_VSIREG 0x251
>
> +/* Virtual Supervisor Indirect Alias */
> +#define CSR_VSIREG2 0x252
> +#define CSR_VSIREG3 0x253
> +#define CSR_VSIREG4 0x255
> +#define CSR_VSIREG5 0x256
> +#define CSR_VSIREG6 0x257
> +
> /* VS-Level Interrupts (H-extension with AIA) */
> #define CSR_VSTOPEI 0x25c
> #define CSR_VSTOPI 0xeb0
> @@ -863,10 +884,13 @@ typedef enum RISCVException {
> #define ISELECT_IMSIC_EIE63 0xff
> #define ISELECT_IMSIC_FIRST ISELECT_IMSIC_EIDELIVERY
> #define ISELECT_IMSIC_LAST ISELECT_IMSIC_EIE63
> -#define ISELECT_MASK 0x1ff
> +#define ISELECT_MASK_AIA 0x1ff
> +
> +/* MISELECT, SISELECT, and VSISELECT bits (AIA) */
> +#define ISELECT_MASK_SXCSRIND 0xfff
>
> /* Dummy [M|S|VS]ISELECT value for emulating [M|S|VS]TOPEI CSRs */
> -#define ISELECT_IMSIC_TOPEI (ISELECT_MASK + 1)
> +#define ISELECT_IMSIC_TOPEI (ISELECT_MASK_AIA + 1)
>
> /* IMSIC bits (AIA) */
> #define IMSIC_TOPEI_IID_SHIFT 16
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c91a26a52ef6..57e9c9e25f02 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -306,6 +306,17 @@ static RISCVException aia_any32(CPURISCVState *env, int csrno)
> return any32(env, csrno);
> }
>
> +static RISCVException csrind_any(CPURISCVState *env, int csrno)
> +{
> + RISCVCPU *cpu = env_archcpu(env);
It is desirable to avoid env_archcpu() calls because it's a bit costly. If you're
doing something else with the RISCVCPU pointer then it is what it is, but if you
want it just to access cpu_cfg you can use riscv_cpu_cfg(env). This is being done
in patch 2:
+ if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
In fact this is also being done in rmw_xiselect down below.
> +
> + if (!cpu->cfg.ext_smcsrind) {
> + return RISCV_EXCP_ILLEGAL_INST;
> + }
> +
> + return RISCV_EXCP_NONE;
> +}
> +
> static RISCVException csrind_or_aia_any(CPURISCVState *env, int csrno)
> {
> if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) {
> @@ -389,6 +400,15 @@ static bool csrind_or_aia_extensions_present(CPURISCVState *env)
> return csrind_extensions_present(env) || aia_extensions_present(env);
> }
>
> +static RISCVException csrind_smode(CPURISCVState *env, int csrno)
> +{
> + if (!csrind_extensions_present(env)) {
> + return RISCV_EXCP_ILLEGAL_INST;
> + }
> +
> + return smode(env, csrno);
> +}
> +
> static RISCVException csrind_or_aia_smode(CPURISCVState *env, int csrno)
> {
> if (!csrind_or_aia_extensions_present(env)) {
> @@ -417,6 +437,15 @@ static RISCVException hmode32(CPURISCVState *env, int csrno)
>
> }
>
> +static RISCVException csrind_hmode(CPURISCVState *env, int csrno)
> +{
> + if (!csrind_extensions_present(env)) {
> + return RISCV_EXCP_ILLEGAL_INST;
> + }
> +
> + return hmode(env, csrno);
> +}
> +
> static RISCVException csrind_or_aia_hmode(CPURISCVState *env, int csrno)
> {
> if (!csrind_or_aia_extensions_present(env)) {
> @@ -2065,7 +2094,12 @@ static int csrind_xlate_vs_csrno(CPURISCVState *env, int csrno)
> case CSR_SISELECT:
> return CSR_VSISELECT;
> case CSR_SIREG:
> - return CSR_VSIREG;
> + case CSR_SIREG2:
> + case CSR_SIREG3:
> + case CSR_SIREG4:
> + case CSR_SIREG5:
> + case CSR_SIREG6:
> + return CSR_VSIREG + (csrno - CSR_SIREG);
> default:
> return csrno;
> };
> @@ -2105,7 +2139,12 @@ static RISCVException rmw_xiselect(CPURISCVState *env, int csrno,
> *val = *iselect;
> }
>
> - wr_mask &= ISELECT_MASK;
> + if (riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind) {
> + wr_mask &= ISELECT_MASK_SXCSRIND;
> + } else {
> + wr_mask &= ISELECT_MASK_AIA;
> + }
> +
> if (wr_mask) {
> *iselect = (*iselect & ~wr_mask) | (new_val & wr_mask);
> }
> @@ -2244,6 +2283,59 @@ done:
> return RISCV_EXCP_NONE;
> }
>
> +/*
> + * rmw_xireg_csrind: Perform indirect access to xireg and xireg2-xireg6
> + *
> + * Perform indirect access to xireg and xireg2-xireg6.
> + * This is a generic interface for all xireg CSRs. Apart from AIA, all other
> + * extension using csrind should be implemented here.
> + */
> +static int rmw_xireg_csrind(CPURISCVState *env, int csrno,
> + target_ulong isel, target_ulong *val,
> + target_ulong new_val, target_ulong wr_mask)
> +{
> + return -EINVAL;
> +}
> +
> +static int rmw_xiregi(CPURISCVState *env, int csrno, target_ulong *val,
> + target_ulong new_val, target_ulong wr_mask)
> +{
> + bool virt = false;
> + int ret = -EINVAL;
> + target_ulong isel;
> +
> + ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SVSLCT);
> + if (ret != RISCV_EXCP_NONE) {
> + return ret;
> + }
> +
> + /* Translate CSR number for VS-mode */
> + csrno = csrind_xlate_vs_csrno(env, csrno);
> +
> + if (CSR_MIREG <= csrno && csrno <= CSR_MIREG6 &&
> + csrno != CSR_MIREG4 - 1) {
> + isel = env->miselect;
> + } else if (CSR_SIREG <= csrno && csrno <= CSR_SIREG6 &&
> + csrno != CSR_SIREG4 - 1) {
> + isel = env->siselect;
> + } else if (CSR_VSIREG <= csrno && csrno <= CSR_VSIREG6 &&
> + csrno != CSR_VSIREG4 - 1) {
> + isel = env->vsiselect;
> + virt = true;
> + } else {
> + goto done;
> + }
> +
> + return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
> +
> +done:
> + if (ret) {
Do you need this check? Up there you're doing:
> + if (ret != RISCV_EXCP_NONE) {
> + return ret;
> + }
And 'ret' is not changed afterwards, meaning it's guaranteed to be RISCV_EXCP_NONE and
"if (ret)" will always be true.
Thanks,
Daniel
> + return (env->virt_enabled && virt) ?
> + RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
> + }
> + return RISCV_EXCP_NONE;
> +}
> +
> static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
> target_ulong *val, target_ulong new_val,
> target_ulong wr_mask)
> @@ -2276,8 +2368,21 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
> goto done;
> };
>
> + /*
> + * Use the xiselect range to determine actual op on xireg.
> + *
> + * Since we only checked the existence of AIA or Indirect Access in the
> + * predicate, we should check the existence of the exact extension when
> + * we get to a specific range and return illegal instruction exception even
> + * in VS-mode.
> + */
> if (xiselect_aia_range(isel)) {
> return rmw_xireg_aia(env, csrno, isel, val, new_val, wr_mask);
> + } else if (riscv_cpu_cfg(env)->ext_smcsrind ||
> + riscv_cpu_cfg(env)->ext_sscsrind) {
> + return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
> + } else {
> + return RISCV_EXCP_ILLEGAL_INST;
> }
>
> done:
> @@ -2735,7 +2840,7 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
> wr_mask |= SMSTATEEN0_P1P13;
> }
>
> - if (riscv_cpu_cfg(env)->ext_smaia) {
> + if (riscv_cpu_cfg(env)->ext_smaia || riscv_cpu_cfg(env)->ext_smcsrind) {
> wr_mask |= SMSTATEEN0_SVSLCT;
> }
>
> @@ -2828,7 +2933,7 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
> wr_mask |= SMSTATEEN0_FCSR;
> }
>
> - if (riscv_cpu_cfg(env)->ext_ssaia) {
> + if (riscv_cpu_cfg(env)->ext_ssaia || riscv_cpu_cfg(env)->ext_sscsrind) {
> wr_mask |= SMSTATEEN0_SVSLCT;
> }
>
> @@ -5261,6 +5366,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> [CSR_MIREG] = { "mireg", csrind_or_aia_any, NULL, NULL,
> rmw_xireg },
>
> + /* Machine Indirect Register Alias */
> + [CSR_MIREG2] = { "mireg2", csrind_any, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_MIREG3] = { "mireg3", csrind_any, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_MIREG4] = { "mireg4", csrind_any, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_MIREG5] = { "mireg5", csrind_any, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_MIREG6] = { "mireg6", csrind_any, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> +
> /* Machine-Level Interrupts (AIA) */
> [CSR_MTOPEI] = { "mtopei", aia_any, NULL, NULL, rmw_xtopei },
> [CSR_MTOPI] = { "mtopi", aia_any, read_mtopi },
> @@ -5382,6 +5499,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> [CSR_SIREG] = { "sireg", csrind_or_aia_smode, NULL, NULL,
> rmw_xireg },
>
> + /* Supervisor Indirect Register Alias */
> + [CSR_SIREG2] = { "sireg2", csrind_smode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_SIREG3] = { "sireg3", csrind_smode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_SIREG4] = { "sireg4", csrind_smode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_SIREG5] = { "sireg5", csrind_smode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_SIREG6] = { "sireg6", csrind_smode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> +
> /* Supervisor-Level Interrupts (AIA) */
> [CSR_STOPEI] = { "stopei", aia_smode, NULL, NULL, rmw_xtopei },
> [CSR_STOPI] = { "stopi", aia_smode, read_stopi },
> @@ -5464,6 +5593,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> [CSR_VSIREG] = { "vsireg", csrind_or_aia_hmode, NULL, NULL,
> rmw_xireg },
>
> + /* Virtual Supervisor Indirect Alias */
> + [CSR_VSIREG2] = { "vsireg2", csrind_hmode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_VSIREG3] = { "vsireg3", csrind_hmode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_VSIREG4] = { "vsireg4", csrind_hmode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_VSIREG5] = { "vsireg5", csrind_hmode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> + [CSR_VSIREG6] = { "vsireg6", csrind_hmode, NULL, NULL, rmw_xiregi,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> +
> /* VS-Level Interrupts (H-extension with AIA) */
> [CSR_VSTOPEI] = { "vstopei", aia_hmode, NULL, NULL, rmw_xtopei },
> [CSR_VSTOPI] = { "vstopi", aia_hmode, read_vstopi },
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 04/11] target/riscv: Support generic CSR indirect access
2024-11-28 11:52 ` Daniel Henrique Barboza
@ 2024-11-28 12:52 ` Richard Henderson
2024-12-02 21:16 ` Atish Kumar Patra
1 sibling, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2024-11-28 12:52 UTC (permalink / raw)
To: qemu-devel
On 11/28/24 05:52, Daniel Henrique Barboza wrote:
>
>
> On 11/17/24 10:15 PM, Atish Patra wrote:
>> From: Kaiwen Xue <kaiwenx@rivosinc.com>
>>
>> This adds the indirect access registers required by sscsrind/smcsrind
>> and the operations on them. Note that xiselect and xireg are used for
>> both AIA and sxcsrind, and the behavior of accessing them depends on
>> whether each extension is enabled and the value stored in xiselect.
>>
>> Co-developed-by: Atish Patra <atishp@rivosinc.com>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
>> ---
>> target/riscv/cpu_bits.h | 28 ++++++++-
>> target/riscv/csr.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 171 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index 385a2c67c24b..e13c5420a251 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -173,6 +173,13 @@
>> #define CSR_MISELECT 0x350
>> #define CSR_MIREG 0x351
>> +/* Machine Indirect Register Alias */
>> +#define CSR_MIREG2 0x352
>> +#define CSR_MIREG3 0x353
>> +#define CSR_MIREG4 0x355
>> +#define CSR_MIREG5 0x356
>> +#define CSR_MIREG6 0x357
>> +
>> /* Machine-Level Interrupts (AIA) */
>> #define CSR_MTOPEI 0x35c
>> #define CSR_MTOPI 0xfb0
>> @@ -222,6 +229,13 @@
>> #define CSR_SISELECT 0x150
>> #define CSR_SIREG 0x151
>> +/* Supervisor Indirect Register Alias */
>> +#define CSR_SIREG2 0x152
>> +#define CSR_SIREG3 0x153
>> +#define CSR_SIREG4 0x155
>> +#define CSR_SIREG5 0x156
>> +#define CSR_SIREG6 0x157
>> +
>> /* Supervisor-Level Interrupts (AIA) */
>> #define CSR_STOPEI 0x15c
>> #define CSR_STOPI 0xdb0
>> @@ -288,6 +302,13 @@
>> #define CSR_VSISELECT 0x250
>> #define CSR_VSIREG 0x251
>> +/* Virtual Supervisor Indirect Alias */
>> +#define CSR_VSIREG2 0x252
>> +#define CSR_VSIREG3 0x253
>> +#define CSR_VSIREG4 0x255
>> +#define CSR_VSIREG5 0x256
>> +#define CSR_VSIREG6 0x257
>> +
>> /* VS-Level Interrupts (H-extension with AIA) */
>> #define CSR_VSTOPEI 0x25c
>> #define CSR_VSTOPI 0xeb0
>> @@ -863,10 +884,13 @@ typedef enum RISCVException {
>> #define ISELECT_IMSIC_EIE63 0xff
>> #define ISELECT_IMSIC_FIRST ISELECT_IMSIC_EIDELIVERY
>> #define ISELECT_IMSIC_LAST ISELECT_IMSIC_EIE63
>> -#define ISELECT_MASK 0x1ff
>> +#define ISELECT_MASK_AIA 0x1ff
>> +
>> +/* MISELECT, SISELECT, and VSISELECT bits (AIA) */
>> +#define ISELECT_MASK_SXCSRIND 0xfff
>> /* Dummy [M|S|VS]ISELECT value for emulating [M|S|VS]TOPEI CSRs */
>> -#define ISELECT_IMSIC_TOPEI (ISELECT_MASK + 1)
>> +#define ISELECT_IMSIC_TOPEI (ISELECT_MASK_AIA + 1)
>> /* IMSIC bits (AIA) */
>> #define IMSIC_TOPEI_IID_SHIFT 16
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index c91a26a52ef6..57e9c9e25f02 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -306,6 +306,17 @@ static RISCVException aia_any32(CPURISCVState *env, int csrno)
>> return any32(env, csrno);
>> }
>> +static RISCVException csrind_any(CPURISCVState *env, int csrno)
>> +{
>> + RISCVCPU *cpu = env_archcpu(env);
>
> It is desirable to avoid env_archcpu() calls because it's a bit costly.
No, it's not. You're thinking of RISCV_CPU, with the qom dynamic cast assert.
env_archcpu is one subtraction.
> If you're
> doing something else with the RISCVCPU pointer then it is what it is, but if you
> want it just to access cpu_cfg you can use riscv_cpu_cfg(env).
... but using riscv_cpu_cfg is still a good idea here. :-)
r~
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 04/11] target/riscv: Support generic CSR indirect access
2024-11-28 11:52 ` Daniel Henrique Barboza
2024-11-28 12:52 ` Richard Henderson
@ 2024-12-02 21:16 ` Atish Kumar Patra
1 sibling, 0 replies; 23+ messages in thread
From: Atish Kumar Patra @ 2024-12-02 21:16 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-riscv, qemu-devel, kaiwenxue1, palmer, liwei1518, zhiwei_liu,
bin.meng, alistair.francis, Kaiwen Xue
On Thu, Nov 28, 2024 at 3:52 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 11/17/24 10:15 PM, Atish Patra wrote:
> > From: Kaiwen Xue <kaiwenx@rivosinc.com>
> >
> > This adds the indirect access registers required by sscsrind/smcsrind
> > and the operations on them. Note that xiselect and xireg are used for
> > both AIA and sxcsrind, and the behavior of accessing them depends on
> > whether each extension is enabled and the value stored in xiselect.
> >
> > Co-developed-by: Atish Patra <atishp@rivosinc.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> > ---
> > target/riscv/cpu_bits.h | 28 ++++++++-
> > target/riscv/csr.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 171 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 385a2c67c24b..e13c5420a251 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -173,6 +173,13 @@
> > #define CSR_MISELECT 0x350
> > #define CSR_MIREG 0x351
> >
> > +/* Machine Indirect Register Alias */
> > +#define CSR_MIREG2 0x352
> > +#define CSR_MIREG3 0x353
> > +#define CSR_MIREG4 0x355
> > +#define CSR_MIREG5 0x356
> > +#define CSR_MIREG6 0x357
> > +
> > /* Machine-Level Interrupts (AIA) */
> > #define CSR_MTOPEI 0x35c
> > #define CSR_MTOPI 0xfb0
> > @@ -222,6 +229,13 @@
> > #define CSR_SISELECT 0x150
> > #define CSR_SIREG 0x151
> >
> > +/* Supervisor Indirect Register Alias */
> > +#define CSR_SIREG2 0x152
> > +#define CSR_SIREG3 0x153
> > +#define CSR_SIREG4 0x155
> > +#define CSR_SIREG5 0x156
> > +#define CSR_SIREG6 0x157
> > +
> > /* Supervisor-Level Interrupts (AIA) */
> > #define CSR_STOPEI 0x15c
> > #define CSR_STOPI 0xdb0
> > @@ -288,6 +302,13 @@
> > #define CSR_VSISELECT 0x250
> > #define CSR_VSIREG 0x251
> >
> > +/* Virtual Supervisor Indirect Alias */
> > +#define CSR_VSIREG2 0x252
> > +#define CSR_VSIREG3 0x253
> > +#define CSR_VSIREG4 0x255
> > +#define CSR_VSIREG5 0x256
> > +#define CSR_VSIREG6 0x257
> > +
> > /* VS-Level Interrupts (H-extension with AIA) */
> > #define CSR_VSTOPEI 0x25c
> > #define CSR_VSTOPI 0xeb0
> > @@ -863,10 +884,13 @@ typedef enum RISCVException {
> > #define ISELECT_IMSIC_EIE63 0xff
> > #define ISELECT_IMSIC_FIRST ISELECT_IMSIC_EIDELIVERY
> > #define ISELECT_IMSIC_LAST ISELECT_IMSIC_EIE63
> > -#define ISELECT_MASK 0x1ff
> > +#define ISELECT_MASK_AIA 0x1ff
> > +
> > +/* MISELECT, SISELECT, and VSISELECT bits (AIA) */
> > +#define ISELECT_MASK_SXCSRIND 0xfff
> >
> > /* Dummy [M|S|VS]ISELECT value for emulating [M|S|VS]TOPEI CSRs */
> > -#define ISELECT_IMSIC_TOPEI (ISELECT_MASK + 1)
> > +#define ISELECT_IMSIC_TOPEI (ISELECT_MASK_AIA + 1)
> >
> > /* IMSIC bits (AIA) */
> > #define IMSIC_TOPEI_IID_SHIFT 16
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index c91a26a52ef6..57e9c9e25f02 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -306,6 +306,17 @@ static RISCVException aia_any32(CPURISCVState *env, int csrno)
> > return any32(env, csrno);
> > }
> >
> > +static RISCVException csrind_any(CPURISCVState *env, int csrno)
> > +{
> > + RISCVCPU *cpu = env_archcpu(env);
>
> It is desirable to avoid env_archcpu() calls because it's a bit costly. If you're
> doing something else with the RISCVCPU pointer then it is what it is, but if you
> want it just to access cpu_cfg you can use riscv_cpu_cfg(env). This is being done
> in patch 2:
>
Yeah. This one slipped from the original patch. Thanks for noticing it. Fixed.
>
> + if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) {
> + return RISCV_EXCP_ILLEGAL_INST;
> + }
>
>
> In fact this is also being done in rmw_xiselect down below.
>
> > +
> > + if (!cpu->cfg.ext_smcsrind) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > +
> > + return RISCV_EXCP_NONE;
> > +}
> > +
> > static RISCVException csrind_or_aia_any(CPURISCVState *env, int csrno)
> > {
> > if (!riscv_cpu_cfg(env)->ext_smaia && !riscv_cpu_cfg(env)->ext_smcsrind) {
> > @@ -389,6 +400,15 @@ static bool csrind_or_aia_extensions_present(CPURISCVState *env)
> > return csrind_extensions_present(env) || aia_extensions_present(env);
> > }
> >
> > +static RISCVException csrind_smode(CPURISCVState *env, int csrno)
> > +{
> > + if (!csrind_extensions_present(env)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > +
> > + return smode(env, csrno);
> > +}
> > +
> > static RISCVException csrind_or_aia_smode(CPURISCVState *env, int csrno)
> > {
> > if (!csrind_or_aia_extensions_present(env)) {
> > @@ -417,6 +437,15 @@ static RISCVException hmode32(CPURISCVState *env, int csrno)
> >
> > }
> >
> > +static RISCVException csrind_hmode(CPURISCVState *env, int csrno)
> > +{
> > + if (!csrind_extensions_present(env)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > +
> > + return hmode(env, csrno);
> > +}
> > +
> > static RISCVException csrind_or_aia_hmode(CPURISCVState *env, int csrno)
> > {
> > if (!csrind_or_aia_extensions_present(env)) {
> > @@ -2065,7 +2094,12 @@ static int csrind_xlate_vs_csrno(CPURISCVState *env, int csrno)
> > case CSR_SISELECT:
> > return CSR_VSISELECT;
> > case CSR_SIREG:
> > - return CSR_VSIREG;
> > + case CSR_SIREG2:
> > + case CSR_SIREG3:
> > + case CSR_SIREG4:
> > + case CSR_SIREG5:
> > + case CSR_SIREG6:
> > + return CSR_VSIREG + (csrno - CSR_SIREG);
> > default:
> > return csrno;
> > };
> > @@ -2105,7 +2139,12 @@ static RISCVException rmw_xiselect(CPURISCVState *env, int csrno,
> > *val = *iselect;
> > }
> >
> > - wr_mask &= ISELECT_MASK;
> > + if (riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind) {
> > + wr_mask &= ISELECT_MASK_SXCSRIND;
> > + } else {
> > + wr_mask &= ISELECT_MASK_AIA;
> > + }
> > +
> > if (wr_mask) {
> > *iselect = (*iselect & ~wr_mask) | (new_val & wr_mask);
> > }
> > @@ -2244,6 +2283,59 @@ done:
> > return RISCV_EXCP_NONE;
> > }
> >
> > +/*
> > + * rmw_xireg_csrind: Perform indirect access to xireg and xireg2-xireg6
> > + *
> > + * Perform indirect access to xireg and xireg2-xireg6.
> > + * This is a generic interface for all xireg CSRs. Apart from AIA, all other
> > + * extension using csrind should be implemented here.
> > + */
> > +static int rmw_xireg_csrind(CPURISCVState *env, int csrno,
> > + target_ulong isel, target_ulong *val,
> > + target_ulong new_val, target_ulong wr_mask)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +static int rmw_xiregi(CPURISCVState *env, int csrno, target_ulong *val,
> > + target_ulong new_val, target_ulong wr_mask)
> > +{
> > + bool virt = false;
> > + int ret = -EINVAL;
> > + target_ulong isel;
> > +
> > + ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SVSLCT);
> > + if (ret != RISCV_EXCP_NONE) {
> > + return ret;
> > + }
> > +
> > + /* Translate CSR number for VS-mode */
> > + csrno = csrind_xlate_vs_csrno(env, csrno);
> > +
> > + if (CSR_MIREG <= csrno && csrno <= CSR_MIREG6 &&
> > + csrno != CSR_MIREG4 - 1) {
> > + isel = env->miselect;
> > + } else if (CSR_SIREG <= csrno && csrno <= CSR_SIREG6 &&
> > + csrno != CSR_SIREG4 - 1) {
> > + isel = env->siselect;
> > + } else if (CSR_VSIREG <= csrno && csrno <= CSR_VSIREG6 &&
> > + csrno != CSR_VSIREG4 - 1) {
> > + isel = env->vsiselect;
> > + virt = true;
> > + } else {
> > + goto done;
> > + }
> > +
> > + return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
> > +
> > +done:
> > + if (ret) {
>
> Do you need this check? Up there you're doing:
>
> > + if (ret != RISCV_EXCP_NONE) {
> > + return ret;
> > + }
>
>
> And 'ret' is not changed afterwards, meaning it's guaranteed to be RISCV_EXCP_NONE and
> "if (ret)" will always be true.
>
>
Yeah. I have removed it. Thanks!
> Thanks,
>
> Daniel
>
>
> > + return (env->virt_enabled && virt) ?
> > + RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
> > + }
> > + return RISCV_EXCP_NONE;
> > +}
> > +
> > static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
> > target_ulong *val, target_ulong new_val,
> > target_ulong wr_mask)
> > @@ -2276,8 +2368,21 @@ static RISCVException rmw_xireg(CPURISCVState *env, int csrno,
> > goto done;
> > };
> >
> > + /*
> > + * Use the xiselect range to determine actual op on xireg.
> > + *
> > + * Since we only checked the existence of AIA or Indirect Access in the
> > + * predicate, we should check the existence of the exact extension when
> > + * we get to a specific range and return illegal instruction exception even
> > + * in VS-mode.
> > + */
> > if (xiselect_aia_range(isel)) {
> > return rmw_xireg_aia(env, csrno, isel, val, new_val, wr_mask);
> > + } else if (riscv_cpu_cfg(env)->ext_smcsrind ||
> > + riscv_cpu_cfg(env)->ext_sscsrind) {
> > + return rmw_xireg_csrind(env, csrno, isel, val, new_val, wr_mask);
> > + } else {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > }
> >
> > done:
> > @@ -2735,7 +2840,7 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
> > wr_mask |= SMSTATEEN0_P1P13;
> > }
> >
> > - if (riscv_cpu_cfg(env)->ext_smaia) {
> > + if (riscv_cpu_cfg(env)->ext_smaia || riscv_cpu_cfg(env)->ext_smcsrind) {
> > wr_mask |= SMSTATEEN0_SVSLCT;
> > }
> >
> > @@ -2828,7 +2933,7 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
> > wr_mask |= SMSTATEEN0_FCSR;
> > }
> >
> > - if (riscv_cpu_cfg(env)->ext_ssaia) {
> > + if (riscv_cpu_cfg(env)->ext_ssaia || riscv_cpu_cfg(env)->ext_sscsrind) {
> > wr_mask |= SMSTATEEN0_SVSLCT;
> > }
> >
> > @@ -5261,6 +5366,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> > [CSR_MIREG] = { "mireg", csrind_or_aia_any, NULL, NULL,
> > rmw_xireg },
> >
> > + /* Machine Indirect Register Alias */
> > + [CSR_MIREG2] = { "mireg2", csrind_any, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_MIREG3] = { "mireg3", csrind_any, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_MIREG4] = { "mireg4", csrind_any, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_MIREG5] = { "mireg5", csrind_any, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_MIREG6] = { "mireg6", csrind_any, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +
> > /* Machine-Level Interrupts (AIA) */
> > [CSR_MTOPEI] = { "mtopei", aia_any, NULL, NULL, rmw_xtopei },
> > [CSR_MTOPI] = { "mtopi", aia_any, read_mtopi },
> > @@ -5382,6 +5499,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> > [CSR_SIREG] = { "sireg", csrind_or_aia_smode, NULL, NULL,
> > rmw_xireg },
> >
> > + /* Supervisor Indirect Register Alias */
> > + [CSR_SIREG2] = { "sireg2", csrind_smode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_SIREG3] = { "sireg3", csrind_smode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_SIREG4] = { "sireg4", csrind_smode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_SIREG5] = { "sireg5", csrind_smode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_SIREG6] = { "sireg6", csrind_smode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +
> > /* Supervisor-Level Interrupts (AIA) */
> > [CSR_STOPEI] = { "stopei", aia_smode, NULL, NULL, rmw_xtopei },
> > [CSR_STOPI] = { "stopi", aia_smode, read_stopi },
> > @@ -5464,6 +5593,18 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> > [CSR_VSIREG] = { "vsireg", csrind_or_aia_hmode, NULL, NULL,
> > rmw_xireg },
> >
> > + /* Virtual Supervisor Indirect Alias */
> > + [CSR_VSIREG2] = { "vsireg2", csrind_hmode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_VSIREG3] = { "vsireg3", csrind_hmode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_VSIREG4] = { "vsireg4", csrind_hmode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_VSIREG5] = { "vsireg5", csrind_hmode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > + [CSR_VSIREG6] = { "vsireg6", csrind_hmode, NULL, NULL, rmw_xiregi,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +
> > /* VS-Level Interrupts (H-extension with AIA) */
> > [CSR_VSTOPEI] = { "vstopei", aia_hmode, NULL, NULL, rmw_xtopei },
> > [CSR_VSTOPI] = { "vstopi", aia_hmode, read_vstopi },
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 05/11] target/riscv: Add properties for counter delegation ISA extensions
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (3 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 04/11] target/riscv: Support generic CSR indirect access Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-28 11:55 ` Daniel Henrique Barboza
2024-11-18 1:15 ` [PATCH v3 06/11] target/riscv: Add counter delegation definitions Atish Patra
` (5 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis
This adds the properties for counter delegation ISA extensions
(Smcdeleg/Ssccfg). Definitions of new registers and and implementation
will come in the next set of patches.
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/cpu.c | 2 ++
target/riscv/cpu_cfg.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 963f1f3af9ae..82edd28e2e1d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -184,11 +184,13 @@ const RISCVIsaExtData isa_edata_arr[] = {
ISA_EXT_DATA_ENTRY(zhinx, PRIV_VERSION_1_12_0, ext_zhinx),
ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
+ ISA_EXT_DATA_ENTRY(smcdeleg, PRIV_VERSION_1_13_0, ext_smcdeleg),
ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
ISA_EXT_DATA_ENTRY(smcsrind, PRIV_VERSION_1_13_0, ext_smcsrind),
ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
+ ISA_EXT_DATA_ENTRY(ssccfg, PRIV_VERSION_1_13_0, ext_ssccfg),
ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 8b974255f6fb..ae2b019703fe 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -78,6 +78,8 @@ struct RISCVCPUConfig {
bool ext_ztso;
bool ext_smstateen;
bool ext_sstc;
+ bool ext_smcdeleg;
+ bool ext_ssccfg;
bool ext_smcntrpmf;
bool ext_smcsrind;
bool ext_sscsrind;
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 05/11] target/riscv: Add properties for counter delegation ISA extensions
2024-11-18 1:15 ` [PATCH v3 05/11] target/riscv: Add properties for counter delegation ISA extensions Atish Patra
@ 2024-11-28 11:55 ` Daniel Henrique Barboza
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Henrique Barboza @ 2024-11-28 11:55 UTC (permalink / raw)
To: Atish Patra, qemu-riscv, qemu-devel
Cc: kaiwenxue1, palmer, liwei1518, zhiwei_liu, bin.meng,
alistair.francis
On 11/17/24 10:15 PM, Atish Patra wrote:
> This adds the properties for counter delegation ISA extensions
> (Smcdeleg/Ssccfg). Definitions of new registers and and implementation
> will come in the next set of patches.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/cpu.c | 2 ++
> target/riscv/cpu_cfg.h | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 963f1f3af9ae..82edd28e2e1d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -184,11 +184,13 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(zhinx, PRIV_VERSION_1_12_0, ext_zhinx),
> ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
> ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
> + ISA_EXT_DATA_ENTRY(smcdeleg, PRIV_VERSION_1_13_0, ext_smcdeleg),
> ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
> ISA_EXT_DATA_ENTRY(smcsrind, PRIV_VERSION_1_13_0, ext_smcsrind),
> ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> + ISA_EXT_DATA_ENTRY(ssccfg, PRIV_VERSION_1_13_0, ext_ssccfg),
> ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
> ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 8b974255f6fb..ae2b019703fe 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -78,6 +78,8 @@ struct RISCVCPUConfig {
> bool ext_ztso;
> bool ext_smstateen;
> bool ext_sstc;
> + bool ext_smcdeleg;
> + bool ext_ssccfg;
> bool ext_smcntrpmf;
> bool ext_smcsrind;
> bool ext_sscsrind;
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 06/11] target/riscv: Add counter delegation definitions
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (4 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 05/11] target/riscv: Add properties for counter delegation ISA extensions Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-18 1:15 ` [PATCH v3 07/11] target/riscv: Add select value range check for counter delegation Atish Patra
` (4 subsequent siblings)
10 siblings, 0 replies; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis, Kaiwen Xue
From: Kaiwen Xue <kaiwenx@rivosinc.com>
This adds definitions for counter delegation, including the new
scountinhibit register and the mstateen.CD bit.
Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/cpu.h | 1 +
target/riscv/cpu_bits.h | 8 +++++++-
target/riscv/machine.c | 1 +
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 284b11282197..903268626374 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -385,6 +385,7 @@ struct CPUArchState {
uint32_t scounteren;
uint32_t mcounteren;
+ uint32_t scountinhibit;
uint32_t mcountinhibit;
/* PMU cycle & instret privilege mode filtering */
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index e13c5420a251..4ac065ac5e5a 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -210,6 +210,9 @@
#define CSR_SSTATEEN2 0x10E
#define CSR_SSTATEEN3 0x10F
+/* Supervisor Counter Delegation */
+#define CSR_SCOUNTINHIBIT 0x120
+
/* Supervisor Trap Handling */
#define CSR_SSCRATCH 0x140
#define CSR_SEPC 0x141
@@ -791,6 +794,7 @@ typedef enum RISCVException {
#define MENVCFG_CBIE (3UL << 4)
#define MENVCFG_CBCFE BIT(6)
#define MENVCFG_CBZE BIT(7)
+#define MENVCFG_CDE (1ULL << 60)
#define MENVCFG_ADUE (1ULL << 61)
#define MENVCFG_PBMTE (1ULL << 62)
#define MENVCFG_STCE (1ULL << 63)
@@ -886,7 +890,9 @@ typedef enum RISCVException {
#define ISELECT_IMSIC_LAST ISELECT_IMSIC_EIE63
#define ISELECT_MASK_AIA 0x1ff
-/* MISELECT, SISELECT, and VSISELECT bits (AIA) */
+/* [M|S|VS]SELCT value for Indirect CSR Access Extension */
+#define ISELECT_CD_FIRST 0x40
+#define ISELECT_CD_LAST 0x5f
#define ISELECT_MASK_SXCSRIND 0xfff
/* Dummy [M|S|VS]ISELECT value for emulating [M|S|VS]TOPEI CSRs */
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 99f0af507717..e1bdc31c7c53 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -434,6 +434,7 @@ const VMStateDescription vmstate_riscv_cpu = {
VMSTATE_UINTTL(env.siselect, RISCVCPU),
VMSTATE_UINT32(env.scounteren, RISCVCPU),
VMSTATE_UINT32(env.mcounteren, RISCVCPU),
+ VMSTATE_UINT32(env.scountinhibit, RISCVCPU),
VMSTATE_UINT32(env.mcountinhibit, RISCVCPU),
VMSTATE_STRUCT_ARRAY(env.pmu_ctrs, RISCVCPU, RV_MAX_MHPMCOUNTERS, 0,
vmstate_pmu_ctr_state, PMUCTRState),
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 07/11] target/riscv: Add select value range check for counter delegation
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (5 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 06/11] target/riscv: Add counter delegation definitions Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-18 1:15 ` [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support Atish Patra
` (3 subsequent siblings)
10 siblings, 0 replies; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis, Kaiwen Xue
From: Kaiwen Xue <kaiwenx@rivosinc.com>
This adds checks in ops performed on xireg and xireg2-xireg6 so that the
counter delegation function will receive a valid xiselect value with the
proper extensions enabled.
Co-developed-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/csr.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 57e9c9e25f02..97cc8059ad37 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2158,6 +2158,11 @@ static bool xiselect_aia_range(target_ulong isel)
(ISELECT_IMSIC_FIRST <= isel && isel <= ISELECT_IMSIC_LAST);
}
+static bool xiselect_cd_range(target_ulong isel)
+{
+ return (ISELECT_CD_FIRST <= isel && isel <= ISELECT_CD_LAST);
+}
+
static int rmw_iprio(target_ulong xlen,
target_ulong iselect, uint8_t *iprio,
target_ulong *val, target_ulong new_val,
@@ -2283,6 +2288,17 @@ done:
return RISCV_EXCP_NONE;
}
+static int rmw_xireg_cd(CPURISCVState *env, int csrno,
+ target_ulong isel, target_ulong *val,
+ target_ulong new_val, target_ulong wr_mask)
+{
+ if (!riscv_cpu_cfg(env)->ext_smcdeleg) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+ /* TODO: Implement the functionality later */
+ return RISCV_EXCP_NONE;
+}
+
/*
* rmw_xireg_csrind: Perform indirect access to xireg and xireg2-xireg6
*
@@ -2294,7 +2310,25 @@ static int rmw_xireg_csrind(CPURISCVState *env, int csrno,
target_ulong isel, target_ulong *val,
target_ulong new_val, target_ulong wr_mask)
{
- return -EINVAL;
+ int ret = -EINVAL;
+ bool virt = csrno == CSR_VSIREG ? true : false;
+
+ if (xiselect_cd_range(isel)) {
+ ret = rmw_xireg_cd(env, csrno, isel, val, new_val, wr_mask);
+ } else {
+ /*
+ * As per the specification, access to unimplented region is undefined
+ * but recommendation is to raise illegal instruction exception.
+ */
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ if (ret) {
+ return (env->virt_enabled && virt) ?
+ RISCV_EXCP_VIRT_INSTRUCTION_FAULT : RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ return RISCV_EXCP_NONE;
}
static int rmw_xiregi(CPURISCVState *env, int csrno, target_ulong *val,
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (6 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 07/11] target/riscv: Add select value range check for counter delegation Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-28 12:53 ` Daniel Henrique Barboza
2024-11-18 1:15 ` [PATCH v3 09/11] target/riscv: Invoke pmu init after feature enable Atish Patra
` (2 subsequent siblings)
10 siblings, 1 reply; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis, Kaiwen Xue
From: Kaiwen Xue <kaiwenx@rivosinc.com>
The Smcdeleg/Ssccfg adds the support for counter delegation via
S*indcsr and Ssccfg.
It also adds a new shadow CSR scountinhibit and menvcfg enable bit (CDE)
to enable this extension and scountovf virtualization.
Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
Co-developed-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/csr.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 289 insertions(+), 11 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 97cc8059ad37..31ea8b8ec20d 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -385,6 +385,21 @@ static RISCVException aia_smode32(CPURISCVState *env, int csrno)
return smode32(env, csrno);
}
+static RISCVException scountinhibit_pred(CPURISCVState *env, int csrno)
+{
+ RISCVCPU *cpu = env_archcpu(env);
+
+ if (!cpu->cfg.ext_ssccfg || !cpu->cfg.ext_smcdeleg) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ if (env->virt_enabled) {
+ return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+ }
+
+ return smode(env, csrno);
+}
+
static bool csrind_extensions_present(CPURISCVState *env)
{
return riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind;
@@ -1222,10 +1237,9 @@ done:
return result;
}
-static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
- target_ulong val)
+static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val,
+ uint32_t ctr_idx)
{
- int ctr_idx = csrno - CSR_MCYCLE;
PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
uint64_t mhpmctr_val = val;
@@ -1250,10 +1264,9 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
return RISCV_EXCP_NONE;
}
-static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
- target_ulong val)
+static RISCVException riscv_pmu_write_ctrh(CPURISCVState *env, target_ulong val,
+ uint32_t ctr_idx)
{
- int ctr_idx = csrno - CSR_MCYCLEH;
PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
uint64_t mhpmctr_val = counter->mhpmcounter_val;
uint64_t mhpmctrh_val = val;
@@ -1275,6 +1288,20 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
return RISCV_EXCP_NONE;
}
+static int write_mhpmcounter(CPURISCVState *env, int csrno, target_ulong val)
+{
+ int ctr_idx = csrno - CSR_MCYCLE;
+
+ return riscv_pmu_write_ctr(env, val, ctr_idx);
+}
+
+static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
+{
+ int ctr_idx = csrno - CSR_MCYCLEH;
+
+ return riscv_pmu_write_ctrh(env, val, ctr_idx);
+}
+
RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
bool upper_half, uint32_t ctr_idx)
{
@@ -1340,6 +1367,167 @@ static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
return riscv_pmu_read_ctr(env, val, true, ctr_index);
}
+static int rmw_cd_mhpmcounter(CPURISCVState *env, int ctr_idx,
+ target_ulong *val, target_ulong new_val,
+ target_ulong wr_mask)
+{
+ if (wr_mask != 0 && wr_mask != -1) {
+ return -EINVAL;
+ }
+
+ if (!wr_mask && val) {
+ riscv_pmu_read_ctr(env, val, false, ctr_idx);
+ } else if (wr_mask) {
+ riscv_pmu_write_ctr(env, new_val, ctr_idx);
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rmw_cd_mhpmcounterh(CPURISCVState *env, int ctr_idx,
+ target_ulong *val, target_ulong new_val,
+ target_ulong wr_mask)
+{
+ if (wr_mask != 0 && wr_mask != -1) {
+ return -EINVAL;
+ }
+
+ if (!wr_mask && val) {
+ riscv_pmu_read_ctr(env, val, true, ctr_idx);
+ } else if (wr_mask) {
+ riscv_pmu_write_ctrh(env, new_val, ctr_idx);
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rmw_cd_mhpmevent(CPURISCVState *env, int evt_index,
+ target_ulong *val, target_ulong new_val,
+ target_ulong wr_mask)
+{
+ uint64_t mhpmevt_val = new_val;
+
+ if (wr_mask != 0 && wr_mask != -1) {
+ return -EINVAL;
+ }
+
+ if (!wr_mask && val) {
+ *val = env->mhpmevent_val[evt_index];
+ if (riscv_cpu_cfg(env)->ext_sscofpmf) {
+ *val &= ~MHPMEVENT_BIT_MINH;
+ }
+ } else if (wr_mask) {
+ wr_mask &= ~MHPMEVENT_BIT_MINH;
+ mhpmevt_val = (new_val & wr_mask) |
+ (env->mhpmevent_val[evt_index] & ~wr_mask);
+ if (riscv_cpu_mxl(env) == MXL_RV32) {
+ mhpmevt_val = mhpmevt_val |
+ ((uint64_t)env->mhpmeventh_val[evt_index] << 32);
+ }
+ env->mhpmevent_val[evt_index] = mhpmevt_val;
+ riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rmw_cd_mhpmeventh(CPURISCVState *env, int evt_index,
+ target_ulong *val, target_ulong new_val,
+ target_ulong wr_mask)
+{
+ uint64_t mhpmevth_val;
+ uint64_t mhpmevt_val = env->mhpmevent_val[evt_index];
+
+ if (wr_mask != 0 && wr_mask != -1) {
+ return -EINVAL;
+ }
+
+ if (!wr_mask && val) {
+ *val = env->mhpmeventh_val[evt_index];
+ if (riscv_cpu_cfg(env)->ext_sscofpmf) {
+ *val &= ~MHPMEVENTH_BIT_MINH;
+ }
+ } else if (wr_mask) {
+ wr_mask &= ~MHPMEVENTH_BIT_MINH;
+ env->mhpmeventh_val[evt_index] =
+ (new_val & wr_mask) | (env->mhpmeventh_val[evt_index] & ~wr_mask);
+ mhpmevth_val = env->mhpmeventh_val[evt_index];
+ mhpmevt_val = mhpmevt_val | (mhpmevth_val << 32);
+ riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rmw_cd_ctr_cfg(CPURISCVState *env, int cfg_index, target_ulong *val,
+ target_ulong new_val, target_ulong wr_mask)
+{
+ switch (cfg_index) {
+ case 0: /* CYCLECFG */
+ if (wr_mask) {
+ wr_mask &= ~MCYCLECFG_BIT_MINH;
+ env->mcyclecfg = (new_val & wr_mask) | (env->mcyclecfg & ~wr_mask);
+ } else {
+ *val = env->mcyclecfg &= ~MHPMEVENTH_BIT_MINH;
+ }
+ break;
+ case 2: /* INSTRETCFG */
+ if (wr_mask) {
+ wr_mask &= ~MINSTRETCFG_BIT_MINH;
+ env->minstretcfg = (new_val & wr_mask) |
+ (env->minstretcfg & ~wr_mask);
+ } else {
+ *val = env->minstretcfg &= ~MHPMEVENTH_BIT_MINH;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int rmw_cd_ctr_cfgh(CPURISCVState *env, int cfg_index, target_ulong *val,
+ target_ulong new_val, target_ulong wr_mask)
+{
+
+ if (riscv_cpu_mxl(env) != MXL_RV32) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ switch (cfg_index) {
+ case 0: /* CYCLECFGH */
+ if (wr_mask) {
+ wr_mask &= ~MCYCLECFGH_BIT_MINH;
+ env->mcyclecfgh = (new_val & wr_mask) |
+ (env->mcyclecfgh & ~wr_mask);
+ } else {
+ *val = env->mcyclecfgh;
+ }
+ break;
+ case 2: /* INSTRETCFGH */
+ if (wr_mask) {
+ wr_mask &= ~MINSTRETCFGH_BIT_MINH;
+ env->minstretcfgh = (new_val & wr_mask) |
+ (env->minstretcfgh & ~wr_mask);
+ } else {
+ *val = env->minstretcfgh;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
+
static RISCVException read_scountovf(CPURISCVState *env, int csrno,
target_ulong *val)
{
@@ -1349,6 +1537,14 @@ static RISCVException read_scountovf(CPURISCVState *env, int csrno,
target_ulong *mhpm_evt_val;
uint64_t of_bit_mask;
+ /* Virtualize scountovf for counter delegation */
+ if (riscv_cpu_cfg(env)->ext_sscofpmf &&
+ riscv_cpu_cfg(env)->ext_ssccfg &&
+ get_field(env->menvcfg, MENVCFG_CDE) &&
+ env->virt_enabled) {
+ return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+ }
+
if (riscv_cpu_mxl(env) == MXL_RV32) {
mhpm_evt_val = env->mhpmeventh_val;
of_bit_mask = MHPMEVENTH_BIT_OF;
@@ -2292,11 +2488,70 @@ static int rmw_xireg_cd(CPURISCVState *env, int csrno,
target_ulong isel, target_ulong *val,
target_ulong new_val, target_ulong wr_mask)
{
- if (!riscv_cpu_cfg(env)->ext_smcdeleg) {
+ int ret = -EINVAL;
+ int ctr_index = isel - ISELECT_CD_FIRST;
+ int isel_hpm_start = ISELECT_CD_FIRST + 3;
+
+ if (!riscv_cpu_cfg(env)->ext_smcdeleg || !riscv_cpu_cfg(env)->ext_ssccfg) {
return RISCV_EXCP_ILLEGAL_INST;
}
- /* TODO: Implement the functionality later */
- return RISCV_EXCP_NONE;
+
+ /* Invalid siselect value for reserved */
+ if (ctr_index == 1) {
+ goto done;
+ }
+
+ /* sireg4 and sireg5 provides access RV32 only CSRs */
+ if (((csrno == CSR_SIREG5) || (csrno == CSR_SIREG4)) &&
+ (riscv_cpu_mxl(env) != MXL_RV32)) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ }
+
+ /* Check Sscofpmf dependancy */
+ if (!riscv_cpu_cfg(env)->ext_sscofpmf && csrno == CSR_SIREG5 &&
+ (isel_hpm_start <= isel && isel <= ISELECT_CD_LAST)) {
+ goto done;
+ }
+
+ /* Check smcntrpmf dependancy */
+ if (!riscv_cpu_cfg(env)->ext_smcntrpmf &&
+ (csrno == CSR_SIREG2 || csrno == CSR_SIREG5) &&
+ (ISELECT_CD_FIRST <= isel && isel < isel_hpm_start)) {
+ goto done;
+ }
+
+ if (!get_field(env->mcounteren, BIT(ctr_index)) ||
+ !get_field(env->menvcfg, MENVCFG_CDE)) {
+ goto done;
+ }
+
+ switch (csrno) {
+ case CSR_SIREG:
+ ret = rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
+ break;
+ case CSR_SIREG4:
+ ret = rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
+ break;
+ case CSR_SIREG2:
+ if (ctr_index <= 2) {
+ ret = rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
+ } else {
+ ret = rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
+ }
+ break;
+ case CSR_SIREG5:
+ if (ctr_index <= 2) {
+ ret = rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
+ } else {
+ ret = rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
+ }
+ break;
+ default:
+ goto done;
+ }
+
+done:
+ return ret;
}
/*
@@ -2578,6 +2833,21 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
return RISCV_EXCP_NONE;
}
+static RISCVException read_scountinhibit(CPURISCVState *env, int csrno,
+ target_ulong *val)
+{
+ /* S-mode can only access the bits delegated by M-mode */
+ *val = env->mcountinhibit & env->mcounteren;
+ return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_scountinhibit(CPURISCVState *env, int csrno,
+ target_ulong val)
+{
+ write_mcountinhibit(env, csrno, val & env->mcounteren);
+ return RISCV_EXCP_NONE;
+}
+
static RISCVException read_mcounteren(CPURISCVState *env, int csrno,
target_ulong *val)
{
@@ -2680,11 +2950,13 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
target_ulong val)
{
const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
- uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
+ uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE |
+ MENVCFG_CBZE | MENVCFG_CDE;
if (riscv_cpu_mxl(env) == MXL_RV64) {
mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
(cfg->ext_sstc ? MENVCFG_STCE : 0) |
+ (cfg->ext_smcdeleg ? MENVCFG_CDE : 0) |
(cfg->ext_svadu ? MENVCFG_ADUE : 0);
if (env_archcpu(env)->cfg.ext_zicfilp) {
@@ -2713,7 +2985,8 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
(cfg->ext_sstc ? MENVCFG_STCE : 0) |
- (cfg->ext_svadu ? MENVCFG_ADUE : 0);
+ (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
+ (cfg->ext_smcdeleg ? MENVCFG_CDE : 0);
uint64_t valh = (uint64_t)val << 32;
env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
@@ -5498,6 +5771,11 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
write_sstateen_1_3,
.min_priv_ver = PRIV_VERSION_1_12_0 },
+ /* Supervisor Counter Delegation */
+ [CSR_SCOUNTINHIBIT] = {"scountinhibit", scountinhibit_pred,
+ read_scountinhibit, write_scountinhibit,
+ .min_priv_ver = PRIV_VERSION_1_12_0 },
+
/* Supervisor Trap Setup */
[CSR_SSTATUS] = { "sstatus", smode, read_sstatus, write_sstatus,
NULL, read_sstatus_i128 },
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support
2024-11-18 1:15 ` [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support Atish Patra
@ 2024-11-28 12:53 ` Daniel Henrique Barboza
2024-12-02 21:15 ` Atish Kumar Patra
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Henrique Barboza @ 2024-11-28 12:53 UTC (permalink / raw)
To: Atish Patra, qemu-riscv, qemu-devel
Cc: kaiwenxue1, palmer, liwei1518, zhiwei_liu, bin.meng,
alistair.francis, Kaiwen Xue
On 11/17/24 10:15 PM, Atish Patra wrote:
> From: Kaiwen Xue <kaiwenx@rivosinc.com>
>
> The Smcdeleg/Ssccfg adds the support for counter delegation via
> S*indcsr and Ssccfg.
>
> It also adds a new shadow CSR scountinhibit and menvcfg enable bit (CDE)
> to enable this extension and scountovf virtualization.
>
> Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> Co-developed-by: Atish Patra <atishp@rivosinc.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> target/riscv/csr.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 289 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 97cc8059ad37..31ea8b8ec20d 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -385,6 +385,21 @@ static RISCVException aia_smode32(CPURISCVState *env, int csrno)
> return smode32(env, csrno);
> }
>
> +static RISCVException scountinhibit_pred(CPURISCVState *env, int csrno)
> +{
> + RISCVCPU *cpu = env_archcpu(env);
> +
> + if (!cpu->cfg.ext_ssccfg || !cpu->cfg.ext_smcdeleg) {
> + return RISCV_EXCP_ILLEGAL_INST;
> + }
> +
> + if (env->virt_enabled) {
> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> + }
> +
> + return smode(env, csrno);
> +}
> +
> static bool csrind_extensions_present(CPURISCVState *env)
> {
> return riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind;
> @@ -1222,10 +1237,9 @@ done:
> return result;
> }
>
> -static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> - target_ulong val)
> +static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val,
> + uint32_t ctr_idx)
> {
> - int ctr_idx = csrno - CSR_MCYCLE;
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = val;
>
> @@ -1250,10 +1264,9 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> return RISCV_EXCP_NONE;
> }
>
> -static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> - target_ulong val)
> +static RISCVException riscv_pmu_write_ctrh(CPURISCVState *env, target_ulong val,
> + uint32_t ctr_idx)
> {
> - int ctr_idx = csrno - CSR_MCYCLEH;
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = counter->mhpmcounter_val;
> uint64_t mhpmctrh_val = val;
> @@ -1275,6 +1288,20 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> return RISCV_EXCP_NONE;
> }
>
> +static int write_mhpmcounter(CPURISCVState *env, int csrno, target_ulong val)
> +{
> + int ctr_idx = csrno - CSR_MCYCLE;
> +
> + return riscv_pmu_write_ctr(env, val, ctr_idx);
> +}
> +
> +static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
> +{
> + int ctr_idx = csrno - CSR_MCYCLEH;
> +
> + return riscv_pmu_write_ctrh(env, val, ctr_idx);
> +}
> +
> RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> bool upper_half, uint32_t ctr_idx)
> {
> @@ -1340,6 +1367,167 @@ static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> return riscv_pmu_read_ctr(env, val, true, ctr_index);
> }
>
> +static int rmw_cd_mhpmcounter(CPURISCVState *env, int ctr_idx,
> + target_ulong *val, target_ulong new_val,
> + target_ulong wr_mask)
> +{
> + if (wr_mask != 0 && wr_mask != -1) {
> + return -EINVAL;
> + }
> +
> + if (!wr_mask && val) {
> + riscv_pmu_read_ctr(env, val, false, ctr_idx);
> + } else if (wr_mask) {
> + riscv_pmu_write_ctr(env, new_val, ctr_idx);
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rmw_cd_mhpmcounterh(CPURISCVState *env, int ctr_idx,
> + target_ulong *val, target_ulong new_val,
> + target_ulong wr_mask)
> +{
> + if (wr_mask != 0 && wr_mask != -1) {
> + return -EINVAL;
> + }
> +
> + if (!wr_mask && val) {
> + riscv_pmu_read_ctr(env, val, true, ctr_idx);
> + } else if (wr_mask) {
> + riscv_pmu_write_ctrh(env, new_val, ctr_idx);
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rmw_cd_mhpmevent(CPURISCVState *env, int evt_index,
> + target_ulong *val, target_ulong new_val,
> + target_ulong wr_mask)
> +{
> + uint64_t mhpmevt_val = new_val;
> +
> + if (wr_mask != 0 && wr_mask != -1) {
> + return -EINVAL;
> + }
> +
> + if (!wr_mask && val) {
> + *val = env->mhpmevent_val[evt_index];
> + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
> + *val &= ~MHPMEVENT_BIT_MINH;
> + }
> + } else if (wr_mask) {
> + wr_mask &= ~MHPMEVENT_BIT_MINH;
> + mhpmevt_val = (new_val & wr_mask) |
> + (env->mhpmevent_val[evt_index] & ~wr_mask);
> + if (riscv_cpu_mxl(env) == MXL_RV32) {
> + mhpmevt_val = mhpmevt_val |
> + ((uint64_t)env->mhpmeventh_val[evt_index] << 32);
> + }
> + env->mhpmevent_val[evt_index] = mhpmevt_val;
> + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rmw_cd_mhpmeventh(CPURISCVState *env, int evt_index,
> + target_ulong *val, target_ulong new_val,
> + target_ulong wr_mask)
> +{
> + uint64_t mhpmevth_val;
> + uint64_t mhpmevt_val = env->mhpmevent_val[evt_index];
> +
> + if (wr_mask != 0 && wr_mask != -1) {
> + return -EINVAL;
> + }
> +
> + if (!wr_mask && val) {
> + *val = env->mhpmeventh_val[evt_index];
> + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
> + *val &= ~MHPMEVENTH_BIT_MINH;
> + }
> + } else if (wr_mask) {
> + wr_mask &= ~MHPMEVENTH_BIT_MINH;
> + env->mhpmeventh_val[evt_index] =
> + (new_val & wr_mask) | (env->mhpmeventh_val[evt_index] & ~wr_mask);
> + mhpmevth_val = env->mhpmeventh_val[evt_index];
> + mhpmevt_val = mhpmevt_val | (mhpmevth_val << 32);
> + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rmw_cd_ctr_cfg(CPURISCVState *env, int cfg_index, target_ulong *val,
> + target_ulong new_val, target_ulong wr_mask)
> +{
> + switch (cfg_index) {
> + case 0: /* CYCLECFG */
> + if (wr_mask) {
> + wr_mask &= ~MCYCLECFG_BIT_MINH;
> + env->mcyclecfg = (new_val & wr_mask) | (env->mcyclecfg & ~wr_mask);
> + } else {
> + *val = env->mcyclecfg &= ~MHPMEVENTH_BIT_MINH;
> + }
> + break;
> + case 2: /* INSTRETCFG */
> + if (wr_mask) {
> + wr_mask &= ~MINSTRETCFG_BIT_MINH;
> + env->minstretcfg = (new_val & wr_mask) |
> + (env->minstretcfg & ~wr_mask);
> + } else {
> + *val = env->minstretcfg &= ~MHPMEVENTH_BIT_MINH;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int rmw_cd_ctr_cfgh(CPURISCVState *env, int cfg_index, target_ulong *val,
> + target_ulong new_val, target_ulong wr_mask)
> +{
> +
> + if (riscv_cpu_mxl(env) != MXL_RV32) {
> + return RISCV_EXCP_ILLEGAL_INST;
> + }
> +
> + switch (cfg_index) {
> + case 0: /* CYCLECFGH */
> + if (wr_mask) {
> + wr_mask &= ~MCYCLECFGH_BIT_MINH;
> + env->mcyclecfgh = (new_val & wr_mask) |
> + (env->mcyclecfgh & ~wr_mask);
> + } else {
> + *val = env->mcyclecfgh;
> + }
> + break;
> + case 2: /* INSTRETCFGH */
> + if (wr_mask) {
> + wr_mask &= ~MINSTRETCFGH_BIT_MINH;
> + env->minstretcfgh = (new_val & wr_mask) |
> + (env->minstretcfgh & ~wr_mask);
> + } else {
> + *val = env->minstretcfgh;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +
> static RISCVException read_scountovf(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> @@ -1349,6 +1537,14 @@ static RISCVException read_scountovf(CPURISCVState *env, int csrno,
> target_ulong *mhpm_evt_val;
> uint64_t of_bit_mask;
>
> + /* Virtualize scountovf for counter delegation */
> + if (riscv_cpu_cfg(env)->ext_sscofpmf &&
> + riscv_cpu_cfg(env)->ext_ssccfg &&
> + get_field(env->menvcfg, MENVCFG_CDE) &&
> + env->virt_enabled) {
> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> + }
> +
> if (riscv_cpu_mxl(env) == MXL_RV32) {
> mhpm_evt_val = env->mhpmeventh_val;
> of_bit_mask = MHPMEVENTH_BIT_OF;
> @@ -2292,11 +2488,70 @@ static int rmw_xireg_cd(CPURISCVState *env, int csrno,
> target_ulong isel, target_ulong *val,
> target_ulong new_val, target_ulong wr_mask)
> {
> - if (!riscv_cpu_cfg(env)->ext_smcdeleg) {
> + int ret = -EINVAL;
It seems like both 'ret' and the 'done' label are being used as shortcuts to do
'return ret', and every time 'ret' is assigned to something else can be replaced
by an early 'return' exit.
I would remove 'ret' and the 'done' label and:
> + int ctr_index = isel - ISELECT_CD_FIRST;
> + int isel_hpm_start = ISELECT_CD_FIRST + 3;
> +
> + if (!riscv_cpu_cfg(env)->ext_smcdeleg || !riscv_cpu_cfg(env)->ext_ssccfg) {
> return RISCV_EXCP_ILLEGAL_INST;
> }
> - /* TODO: Implement the functionality later */
> - return RISCV_EXCP_NONE;
> +
> + /* Invalid siselect value for reserved */
> + if (ctr_index == 1) {
> + goto done;
return -EINVAL;
> + }
> +
> + /* sireg4 and sireg5 provides access RV32 only CSRs */
> + if (((csrno == CSR_SIREG5) || (csrno == CSR_SIREG4)) &&
> + (riscv_cpu_mxl(env) != MXL_RV32)) {
> + return RISCV_EXCP_ILLEGAL_INST;
> + }
> +
> + /* Check Sscofpmf dependancy */
> + if (!riscv_cpu_cfg(env)->ext_sscofpmf && csrno == CSR_SIREG5 &&
> + (isel_hpm_start <= isel && isel <= ISELECT_CD_LAST)) {
> + goto done;
return -EINVAL;
> + }
> +
> + /* Check smcntrpmf dependancy */
> + if (!riscv_cpu_cfg(env)->ext_smcntrpmf &&
> + (csrno == CSR_SIREG2 || csrno == CSR_SIREG5) &&
> + (ISELECT_CD_FIRST <= isel && isel < isel_hpm_start)) {
> + goto done;
return -EINVAL;
> + }
> +
> + if (!get_field(env->mcounteren, BIT(ctr_index)) ||
> + !get_field(env->menvcfg, MENVCFG_CDE)) {
> + goto done;
return -EINVAL;
> + }
> +
> + switch (csrno) {
> + case CSR_SIREG:
> + ret = rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
return rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
> + break;
> + case CSR_SIREG4:
> + ret = rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
return rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
> + break;
> + case CSR_SIREG2:
> + if (ctr_index <= 2) {
> + ret = rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
return rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
> + } else {
> + ret = rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
return rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
> + }
> + break;
> + case CSR_SIREG5:
> + if (ctr_index <= 2) {
> + ret = rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
return rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
> + } else {
> + ret = rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
return rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
> + }
> + break;
> + default:
> + goto done;
return -EINVAL;
> + }
> +
> +done:
> + return ret;
And remove this last 'return' since we're doing all possible returns already.
Thanks,
Daniel
> }
>
> /*
> @@ -2578,6 +2833,21 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
> return RISCV_EXCP_NONE;
> }
>
> +static RISCVException read_scountinhibit(CPURISCVState *env, int csrno,
> + target_ulong *val)
> +{
> + /* S-mode can only access the bits delegated by M-mode */
> + *val = env->mcountinhibit & env->mcounteren;
> + return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_scountinhibit(CPURISCVState *env, int csrno,
> + target_ulong val)
> +{
> + write_mcountinhibit(env, csrno, val & env->mcounteren);
> + return RISCV_EXCP_NONE;
> +}
> +
> static RISCVException read_mcounteren(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> @@ -2680,11 +2950,13 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> target_ulong val)
> {
> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> - uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
> + uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE |
> + MENVCFG_CBZE | MENVCFG_CDE;
>
> if (riscv_cpu_mxl(env) == MXL_RV64) {
> mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0) |
> (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>
> if (env_archcpu(env)->cfg.ext_zicfilp) {
> @@ -2713,7 +2985,8 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> - (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> + (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
> + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0);
> uint64_t valh = (uint64_t)val << 32;
>
> env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> @@ -5498,6 +5771,11 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> write_sstateen_1_3,
> .min_priv_ver = PRIV_VERSION_1_12_0 },
>
> + /* Supervisor Counter Delegation */
> + [CSR_SCOUNTINHIBIT] = {"scountinhibit", scountinhibit_pred,
> + read_scountinhibit, write_scountinhibit,
> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> +
> /* Supervisor Trap Setup */
> [CSR_SSTATUS] = { "sstatus", smode, read_sstatus, write_sstatus,
> NULL, read_sstatus_i128 },
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support
2024-11-28 12:53 ` Daniel Henrique Barboza
@ 2024-12-02 21:15 ` Atish Kumar Patra
2024-12-02 21:49 ` Daniel Henrique Barboza
0 siblings, 1 reply; 23+ messages in thread
From: Atish Kumar Patra @ 2024-12-02 21:15 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-riscv, qemu-devel, kaiwenxue1, palmer, liwei1518, zhiwei_liu,
bin.meng, alistair.francis, Kaiwen Xue
On Thu, Nov 28, 2024 at 4:53 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 11/17/24 10:15 PM, Atish Patra wrote:
> > From: Kaiwen Xue <kaiwenx@rivosinc.com>
> >
> > The Smcdeleg/Ssccfg adds the support for counter delegation via
> > S*indcsr and Ssccfg.
> >
> > It also adds a new shadow CSR scountinhibit and menvcfg enable bit (CDE)
> > to enable this extension and scountovf virtualization.
> >
> > Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> > Co-developed-by: Atish Patra <atishp@rivosinc.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> > target/riscv/csr.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 289 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 97cc8059ad37..31ea8b8ec20d 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -385,6 +385,21 @@ static RISCVException aia_smode32(CPURISCVState *env, int csrno)
> > return smode32(env, csrno);
> > }
> >
> > +static RISCVException scountinhibit_pred(CPURISCVState *env, int csrno)
> > +{
> > + RISCVCPU *cpu = env_archcpu(env);
> > +
> > + if (!cpu->cfg.ext_ssccfg || !cpu->cfg.ext_smcdeleg) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > +
> > + if (env->virt_enabled) {
> > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > + }
> > +
> > + return smode(env, csrno);
> > +}
> > +
> > static bool csrind_extensions_present(CPURISCVState *env)
> > {
> > return riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind;
> > @@ -1222,10 +1237,9 @@ done:
> > return result;
> > }
> >
> > -static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> > - target_ulong val)
> > +static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val,
> > + uint32_t ctr_idx)
> > {
> > - int ctr_idx = csrno - CSR_MCYCLE;
> > PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> > uint64_t mhpmctr_val = val;
> >
> > @@ -1250,10 +1264,9 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > -static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> > - target_ulong val)
> > +static RISCVException riscv_pmu_write_ctrh(CPURISCVState *env, target_ulong val,
> > + uint32_t ctr_idx)
> > {
> > - int ctr_idx = csrno - CSR_MCYCLEH;
> > PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> > uint64_t mhpmctr_val = counter->mhpmcounter_val;
> > uint64_t mhpmctrh_val = val;
> > @@ -1275,6 +1288,20 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > +static int write_mhpmcounter(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > + int ctr_idx = csrno - CSR_MCYCLE;
> > +
> > + return riscv_pmu_write_ctr(env, val, ctr_idx);
> > +}
> > +
> > +static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > + int ctr_idx = csrno - CSR_MCYCLEH;
> > +
> > + return riscv_pmu_write_ctrh(env, val, ctr_idx);
> > +}
> > +
> > RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> > bool upper_half, uint32_t ctr_idx)
> > {
> > @@ -1340,6 +1367,167 @@ static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> > return riscv_pmu_read_ctr(env, val, true, ctr_index);
> > }
> >
> > +static int rmw_cd_mhpmcounter(CPURISCVState *env, int ctr_idx,
> > + target_ulong *val, target_ulong new_val,
> > + target_ulong wr_mask)
> > +{
> > + if (wr_mask != 0 && wr_mask != -1) {
> > + return -EINVAL;
> > + }
> > +
> > + if (!wr_mask && val) {
> > + riscv_pmu_read_ctr(env, val, false, ctr_idx);
> > + } else if (wr_mask) {
> > + riscv_pmu_write_ctr(env, new_val, ctr_idx);
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rmw_cd_mhpmcounterh(CPURISCVState *env, int ctr_idx,
> > + target_ulong *val, target_ulong new_val,
> > + target_ulong wr_mask)
> > +{
> > + if (wr_mask != 0 && wr_mask != -1) {
> > + return -EINVAL;
> > + }
> > +
> > + if (!wr_mask && val) {
> > + riscv_pmu_read_ctr(env, val, true, ctr_idx);
> > + } else if (wr_mask) {
> > + riscv_pmu_write_ctrh(env, new_val, ctr_idx);
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rmw_cd_mhpmevent(CPURISCVState *env, int evt_index,
> > + target_ulong *val, target_ulong new_val,
> > + target_ulong wr_mask)
> > +{
> > + uint64_t mhpmevt_val = new_val;
> > +
> > + if (wr_mask != 0 && wr_mask != -1) {
> > + return -EINVAL;
> > + }
> > +
> > + if (!wr_mask && val) {
> > + *val = env->mhpmevent_val[evt_index];
> > + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
> > + *val &= ~MHPMEVENT_BIT_MINH;
> > + }
> > + } else if (wr_mask) {
> > + wr_mask &= ~MHPMEVENT_BIT_MINH;
> > + mhpmevt_val = (new_val & wr_mask) |
> > + (env->mhpmevent_val[evt_index] & ~wr_mask);
> > + if (riscv_cpu_mxl(env) == MXL_RV32) {
> > + mhpmevt_val = mhpmevt_val |
> > + ((uint64_t)env->mhpmeventh_val[evt_index] << 32);
> > + }
> > + env->mhpmevent_val[evt_index] = mhpmevt_val;
> > + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rmw_cd_mhpmeventh(CPURISCVState *env, int evt_index,
> > + target_ulong *val, target_ulong new_val,
> > + target_ulong wr_mask)
> > +{
> > + uint64_t mhpmevth_val;
> > + uint64_t mhpmevt_val = env->mhpmevent_val[evt_index];
> > +
> > + if (wr_mask != 0 && wr_mask != -1) {
> > + return -EINVAL;
> > + }
> > +
> > + if (!wr_mask && val) {
> > + *val = env->mhpmeventh_val[evt_index];
> > + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
> > + *val &= ~MHPMEVENTH_BIT_MINH;
> > + }
> > + } else if (wr_mask) {
> > + wr_mask &= ~MHPMEVENTH_BIT_MINH;
> > + env->mhpmeventh_val[evt_index] =
> > + (new_val & wr_mask) | (env->mhpmeventh_val[evt_index] & ~wr_mask);
> > + mhpmevth_val = env->mhpmeventh_val[evt_index];
> > + mhpmevt_val = mhpmevt_val | (mhpmevth_val << 32);
> > + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rmw_cd_ctr_cfg(CPURISCVState *env, int cfg_index, target_ulong *val,
> > + target_ulong new_val, target_ulong wr_mask)
> > +{
> > + switch (cfg_index) {
> > + case 0: /* CYCLECFG */
> > + if (wr_mask) {
> > + wr_mask &= ~MCYCLECFG_BIT_MINH;
> > + env->mcyclecfg = (new_val & wr_mask) | (env->mcyclecfg & ~wr_mask);
> > + } else {
> > + *val = env->mcyclecfg &= ~MHPMEVENTH_BIT_MINH;
> > + }
> > + break;
> > + case 2: /* INSTRETCFG */
> > + if (wr_mask) {
> > + wr_mask &= ~MINSTRETCFG_BIT_MINH;
> > + env->minstretcfg = (new_val & wr_mask) |
> > + (env->minstretcfg & ~wr_mask);
> > + } else {
> > + *val = env->minstretcfg &= ~MHPMEVENTH_BIT_MINH;
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +static int rmw_cd_ctr_cfgh(CPURISCVState *env, int cfg_index, target_ulong *val,
> > + target_ulong new_val, target_ulong wr_mask)
> > +{
> > +
> > + if (riscv_cpu_mxl(env) != MXL_RV32) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > +
> > + switch (cfg_index) {
> > + case 0: /* CYCLECFGH */
> > + if (wr_mask) {
> > + wr_mask &= ~MCYCLECFGH_BIT_MINH;
> > + env->mcyclecfgh = (new_val & wr_mask) |
> > + (env->mcyclecfgh & ~wr_mask);
> > + } else {
> > + *val = env->mcyclecfgh;
> > + }
> > + break;
> > + case 2: /* INSTRETCFGH */
> > + if (wr_mask) {
> > + wr_mask &= ~MINSTRETCFGH_BIT_MINH;
> > + env->minstretcfgh = (new_val & wr_mask) |
> > + (env->minstretcfgh & ~wr_mask);
> > + } else {
> > + *val = env->minstretcfgh;
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > +
> > static RISCVException read_scountovf(CPURISCVState *env, int csrno,
> > target_ulong *val)
> > {
> > @@ -1349,6 +1537,14 @@ static RISCVException read_scountovf(CPURISCVState *env, int csrno,
> > target_ulong *mhpm_evt_val;
> > uint64_t of_bit_mask;
> >
> > + /* Virtualize scountovf for counter delegation */
> > + if (riscv_cpu_cfg(env)->ext_sscofpmf &&
> > + riscv_cpu_cfg(env)->ext_ssccfg &&
> > + get_field(env->menvcfg, MENVCFG_CDE) &&
> > + env->virt_enabled) {
> > + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > + }
> > +
> > if (riscv_cpu_mxl(env) == MXL_RV32) {
> > mhpm_evt_val = env->mhpmeventh_val;
> > of_bit_mask = MHPMEVENTH_BIT_OF;
> > @@ -2292,11 +2488,70 @@ static int rmw_xireg_cd(CPURISCVState *env, int csrno,
> > target_ulong isel, target_ulong *val,
> > target_ulong new_val, target_ulong wr_mask)
> > {
> > - if (!riscv_cpu_cfg(env)->ext_smcdeleg) {
> > + int ret = -EINVAL;
>
> It seems like both 'ret' and the 'done' label are being used as shortcuts to do
> 'return ret', and every time 'ret' is assigned to something else can be replaced
> by an early 'return' exit.
>
> I would remove 'ret' and the 'done' label and:
>
>
>
> > + int ctr_index = isel - ISELECT_CD_FIRST;
> > + int isel_hpm_start = ISELECT_CD_FIRST + 3;
> > +
> > + if (!riscv_cpu_cfg(env)->ext_smcdeleg || !riscv_cpu_cfg(env)->ext_ssccfg) {
> > return RISCV_EXCP_ILLEGAL_INST;
> > }
> > - /* TODO: Implement the functionality later */
> > - return RISCV_EXCP_NONE;
> > +
> > + /* Invalid siselect value for reserved */
> > + if (ctr_index == 1) {
> > + goto done;
>
> return -EINVAL;
> > + }
> > +
> > + /* sireg4 and sireg5 provides access RV32 only CSRs */
> > + if (((csrno == CSR_SIREG5) || (csrno == CSR_SIREG4)) &&
> > + (riscv_cpu_mxl(env) != MXL_RV32)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + }
> > +
> > + /* Check Sscofpmf dependancy */
> > + if (!riscv_cpu_cfg(env)->ext_sscofpmf && csrno == CSR_SIREG5 &&
> > + (isel_hpm_start <= isel && isel <= ISELECT_CD_LAST)) {
> > + goto done;
>
> return -EINVAL;
>
> > + }
> > +
> > + /* Check smcntrpmf dependancy */
> > + if (!riscv_cpu_cfg(env)->ext_smcntrpmf &&
> > + (csrno == CSR_SIREG2 || csrno == CSR_SIREG5) &&
> > + (ISELECT_CD_FIRST <= isel && isel < isel_hpm_start)) {
> > + goto done;
>
> return -EINVAL;
>
> > + }
> > +
> > + if (!get_field(env->mcounteren, BIT(ctr_index)) ||
> > + !get_field(env->menvcfg, MENVCFG_CDE)) {
> > + goto done;
>
> return -EINVAL;
>
> > + }
> > +
> > + switch (csrno) {
> > + case CSR_SIREG:
> > + ret = rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
>
> return rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
> > + break;
> > + case CSR_SIREG4:
> > + ret = rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
>
> return rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
> > + break;
> > + case CSR_SIREG2:
> > + if (ctr_index <= 2) {
> > + ret = rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
>
> return rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
> > + } else {
> > + ret = rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
>
> return rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
>
> > + }
> > + break;
> > + case CSR_SIREG5:
> > + if (ctr_index <= 2) {
> > + ret = rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
>
> return rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
>
> > + } else {
> > + ret = rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
>
> return rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
>
> > + }
> > + break;
> > + default:
> > + goto done;
>
> return -EINVAL;
>
> > + }
> > +
> > +done:
> > + return ret;
>
> And remove this last 'return' since we're doing all possible returns already.
>
Personally, I prefer a single return in a switch case block. That's
why I have the jump label.
If you feel too strongly about that, I can change as per your suggestion though.
>
> Thanks,
>
> Daniel
>
> > }
> >
> > /*
> > @@ -2578,6 +2833,21 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > +static RISCVException read_scountinhibit(CPURISCVState *env, int csrno,
> > + target_ulong *val)
> > +{
> > + /* S-mode can only access the bits delegated by M-mode */
> > + *val = env->mcountinhibit & env->mcounteren;
> > + return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_scountinhibit(CPURISCVState *env, int csrno,
> > + target_ulong val)
> > +{
> > + write_mcountinhibit(env, csrno, val & env->mcounteren);
> > + return RISCV_EXCP_NONE;
> > +}
> > +
> > static RISCVException read_mcounteren(CPURISCVState *env, int csrno,
> > target_ulong *val)
> > {
> > @@ -2680,11 +2950,13 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> > target_ulong val)
> > {
> > const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> > - uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
> > + uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE |
> > + MENVCFG_CBZE | MENVCFG_CDE;
> >
> > if (riscv_cpu_mxl(env) == MXL_RV64) {
> > mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> > (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> > + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0) |
> > (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> >
> > if (env_archcpu(env)->cfg.ext_zicfilp) {
> > @@ -2713,7 +2985,8 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> > const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> > uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> > (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> > - (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> > + (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
> > + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0);
> > uint64_t valh = (uint64_t)val << 32;
> >
> > env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> > @@ -5498,6 +5771,11 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> > write_sstateen_1_3,
> > .min_priv_ver = PRIV_VERSION_1_12_0 },
> >
> > + /* Supervisor Counter Delegation */
> > + [CSR_SCOUNTINHIBIT] = {"scountinhibit", scountinhibit_pred,
> > + read_scountinhibit, write_scountinhibit,
> > + .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +
> > /* Supervisor Trap Setup */
> > [CSR_SSTATUS] = { "sstatus", smode, read_sstatus, write_sstatus,
> > NULL, read_sstatus_i128 },
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support
2024-12-02 21:15 ` Atish Kumar Patra
@ 2024-12-02 21:49 ` Daniel Henrique Barboza
2024-12-02 23:47 ` Atish Kumar Patra
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Henrique Barboza @ 2024-12-02 21:49 UTC (permalink / raw)
To: Atish Kumar Patra
Cc: qemu-riscv, qemu-devel, kaiwenxue1, palmer, liwei1518, zhiwei_liu,
bin.meng, alistair.francis, Kaiwen Xue
On 12/2/24 6:15 PM, Atish Kumar Patra wrote:
> On Thu, Nov 28, 2024 at 4:53 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 11/17/24 10:15 PM, Atish Patra wrote:
>>> From: Kaiwen Xue <kaiwenx@rivosinc.com>
>>>
>>> The Smcdeleg/Ssccfg adds the support for counter delegation via
>>> S*indcsr and Ssccfg.
>>>
>>> It also adds a new shadow CSR scountinhibit and menvcfg enable bit (CDE)
>>> to enable this extension and scountovf virtualization.
>>>
>>> Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
>>> Co-developed-by: Atish Patra <atishp@rivosinc.com>
>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> ---
>>> target/riscv/csr.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 289 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index 97cc8059ad37..31ea8b8ec20d 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -385,6 +385,21 @@ static RISCVException aia_smode32(CPURISCVState *env, int csrno)
>>> return smode32(env, csrno);
>>> }
>>>
>>> +static RISCVException scountinhibit_pred(CPURISCVState *env, int csrno)
>>> +{
>>> + RISCVCPU *cpu = env_archcpu(env);
>>> +
>>> + if (!cpu->cfg.ext_ssccfg || !cpu->cfg.ext_smcdeleg) {
>>> + return RISCV_EXCP_ILLEGAL_INST;
>>> + }
>>> +
>>> + if (env->virt_enabled) {
>>> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>>> + }
>>> +
>>> + return smode(env, csrno);
>>> +}
>>> +
>>> static bool csrind_extensions_present(CPURISCVState *env)
>>> {
>>> return riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind;
>>> @@ -1222,10 +1237,9 @@ done:
>>> return result;
>>> }
>>>
>>> -static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
>>> - target_ulong val)
>>> +static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val,
>>> + uint32_t ctr_idx)
>>> {
>>> - int ctr_idx = csrno - CSR_MCYCLE;
>>> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
>>> uint64_t mhpmctr_val = val;
>>>
>>> @@ -1250,10 +1264,9 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
>>> return RISCV_EXCP_NONE;
>>> }
>>>
>>> -static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
>>> - target_ulong val)
>>> +static RISCVException riscv_pmu_write_ctrh(CPURISCVState *env, target_ulong val,
>>> + uint32_t ctr_idx)
>>> {
>>> - int ctr_idx = csrno - CSR_MCYCLEH;
>>> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
>>> uint64_t mhpmctr_val = counter->mhpmcounter_val;
>>> uint64_t mhpmctrh_val = val;
>>> @@ -1275,6 +1288,20 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
>>> return RISCV_EXCP_NONE;
>>> }
>>>
>>> +static int write_mhpmcounter(CPURISCVState *env, int csrno, target_ulong val)
>>> +{
>>> + int ctr_idx = csrno - CSR_MCYCLE;
>>> +
>>> + return riscv_pmu_write_ctr(env, val, ctr_idx);
>>> +}
>>> +
>>> +static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
>>> +{
>>> + int ctr_idx = csrno - CSR_MCYCLEH;
>>> +
>>> + return riscv_pmu_write_ctrh(env, val, ctr_idx);
>>> +}
>>> +
>>> RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>>> bool upper_half, uint32_t ctr_idx)
>>> {
>>> @@ -1340,6 +1367,167 @@ static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
>>> return riscv_pmu_read_ctr(env, val, true, ctr_index);
>>> }
>>>
>>> +static int rmw_cd_mhpmcounter(CPURISCVState *env, int ctr_idx,
>>> + target_ulong *val, target_ulong new_val,
>>> + target_ulong wr_mask)
>>> +{
>>> + if (wr_mask != 0 && wr_mask != -1) {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!wr_mask && val) {
>>> + riscv_pmu_read_ctr(env, val, false, ctr_idx);
>>> + } else if (wr_mask) {
>>> + riscv_pmu_write_ctr(env, new_val, ctr_idx);
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int rmw_cd_mhpmcounterh(CPURISCVState *env, int ctr_idx,
>>> + target_ulong *val, target_ulong new_val,
>>> + target_ulong wr_mask)
>>> +{
>>> + if (wr_mask != 0 && wr_mask != -1) {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!wr_mask && val) {
>>> + riscv_pmu_read_ctr(env, val, true, ctr_idx);
>>> + } else if (wr_mask) {
>>> + riscv_pmu_write_ctrh(env, new_val, ctr_idx);
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int rmw_cd_mhpmevent(CPURISCVState *env, int evt_index,
>>> + target_ulong *val, target_ulong new_val,
>>> + target_ulong wr_mask)
>>> +{
>>> + uint64_t mhpmevt_val = new_val;
>>> +
>>> + if (wr_mask != 0 && wr_mask != -1) {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!wr_mask && val) {
>>> + *val = env->mhpmevent_val[evt_index];
>>> + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
>>> + *val &= ~MHPMEVENT_BIT_MINH;
>>> + }
>>> + } else if (wr_mask) {
>>> + wr_mask &= ~MHPMEVENT_BIT_MINH;
>>> + mhpmevt_val = (new_val & wr_mask) |
>>> + (env->mhpmevent_val[evt_index] & ~wr_mask);
>>> + if (riscv_cpu_mxl(env) == MXL_RV32) {
>>> + mhpmevt_val = mhpmevt_val |
>>> + ((uint64_t)env->mhpmeventh_val[evt_index] << 32);
>>> + }
>>> + env->mhpmevent_val[evt_index] = mhpmevt_val;
>>> + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int rmw_cd_mhpmeventh(CPURISCVState *env, int evt_index,
>>> + target_ulong *val, target_ulong new_val,
>>> + target_ulong wr_mask)
>>> +{
>>> + uint64_t mhpmevth_val;
>>> + uint64_t mhpmevt_val = env->mhpmevent_val[evt_index];
>>> +
>>> + if (wr_mask != 0 && wr_mask != -1) {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!wr_mask && val) {
>>> + *val = env->mhpmeventh_val[evt_index];
>>> + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
>>> + *val &= ~MHPMEVENTH_BIT_MINH;
>>> + }
>>> + } else if (wr_mask) {
>>> + wr_mask &= ~MHPMEVENTH_BIT_MINH;
>>> + env->mhpmeventh_val[evt_index] =
>>> + (new_val & wr_mask) | (env->mhpmeventh_val[evt_index] & ~wr_mask);
>>> + mhpmevth_val = env->mhpmeventh_val[evt_index];
>>> + mhpmevt_val = mhpmevt_val | (mhpmevth_val << 32);
>>> + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int rmw_cd_ctr_cfg(CPURISCVState *env, int cfg_index, target_ulong *val,
>>> + target_ulong new_val, target_ulong wr_mask)
>>> +{
>>> + switch (cfg_index) {
>>> + case 0: /* CYCLECFG */
>>> + if (wr_mask) {
>>> + wr_mask &= ~MCYCLECFG_BIT_MINH;
>>> + env->mcyclecfg = (new_val & wr_mask) | (env->mcyclecfg & ~wr_mask);
>>> + } else {
>>> + *val = env->mcyclecfg &= ~MHPMEVENTH_BIT_MINH;
>>> + }
>>> + break;
>>> + case 2: /* INSTRETCFG */
>>> + if (wr_mask) {
>>> + wr_mask &= ~MINSTRETCFG_BIT_MINH;
>>> + env->minstretcfg = (new_val & wr_mask) |
>>> + (env->minstretcfg & ~wr_mask);
>>> + } else {
>>> + *val = env->minstretcfg &= ~MHPMEVENTH_BIT_MINH;
>>> + }
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int rmw_cd_ctr_cfgh(CPURISCVState *env, int cfg_index, target_ulong *val,
>>> + target_ulong new_val, target_ulong wr_mask)
>>> +{
>>> +
>>> + if (riscv_cpu_mxl(env) != MXL_RV32) {
>>> + return RISCV_EXCP_ILLEGAL_INST;
>>> + }
>>> +
>>> + switch (cfg_index) {
>>> + case 0: /* CYCLECFGH */
>>> + if (wr_mask) {
>>> + wr_mask &= ~MCYCLECFGH_BIT_MINH;
>>> + env->mcyclecfgh = (new_val & wr_mask) |
>>> + (env->mcyclecfgh & ~wr_mask);
>>> + } else {
>>> + *val = env->mcyclecfgh;
>>> + }
>>> + break;
>>> + case 2: /* INSTRETCFGH */
>>> + if (wr_mask) {
>>> + wr_mask &= ~MINSTRETCFGH_BIT_MINH;
>>> + env->minstretcfgh = (new_val & wr_mask) |
>>> + (env->minstretcfgh & ~wr_mask);
>>> + } else {
>>> + *val = env->minstretcfgh;
>>> + }
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +
>>> static RISCVException read_scountovf(CPURISCVState *env, int csrno,
>>> target_ulong *val)
>>> {
>>> @@ -1349,6 +1537,14 @@ static RISCVException read_scountovf(CPURISCVState *env, int csrno,
>>> target_ulong *mhpm_evt_val;
>>> uint64_t of_bit_mask;
>>>
>>> + /* Virtualize scountovf for counter delegation */
>>> + if (riscv_cpu_cfg(env)->ext_sscofpmf &&
>>> + riscv_cpu_cfg(env)->ext_ssccfg &&
>>> + get_field(env->menvcfg, MENVCFG_CDE) &&
>>> + env->virt_enabled) {
>>> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>>> + }
>>> +
>>> if (riscv_cpu_mxl(env) == MXL_RV32) {
>>> mhpm_evt_val = env->mhpmeventh_val;
>>> of_bit_mask = MHPMEVENTH_BIT_OF;
>>> @@ -2292,11 +2488,70 @@ static int rmw_xireg_cd(CPURISCVState *env, int csrno,
>>> target_ulong isel, target_ulong *val,
>>> target_ulong new_val, target_ulong wr_mask)
>>> {
>>> - if (!riscv_cpu_cfg(env)->ext_smcdeleg) {
>>> + int ret = -EINVAL;
>>
>> It seems like both 'ret' and the 'done' label are being used as shortcuts to do
>> 'return ret', and every time 'ret' is assigned to something else can be replaced
>> by an early 'return' exit.
>>
>> I would remove 'ret' and the 'done' label and:
>>
>>
>>
>>> + int ctr_index = isel - ISELECT_CD_FIRST;
>>> + int isel_hpm_start = ISELECT_CD_FIRST + 3;
>>> +
>>> + if (!riscv_cpu_cfg(env)->ext_smcdeleg || !riscv_cpu_cfg(env)->ext_ssccfg) {
>>> return RISCV_EXCP_ILLEGAL_INST;
>>> }
>>> - /* TODO: Implement the functionality later */
>>> - return RISCV_EXCP_NONE;
>>> +
>>> + /* Invalid siselect value for reserved */
>>> + if (ctr_index == 1) {
>>> + goto done;
>>
>> return -EINVAL;
>>> + }
>>> +
>>> + /* sireg4 and sireg5 provides access RV32 only CSRs */
>>> + if (((csrno == CSR_SIREG5) || (csrno == CSR_SIREG4)) &&
>>> + (riscv_cpu_mxl(env) != MXL_RV32)) {
>>> + return RISCV_EXCP_ILLEGAL_INST;
>>> + }
>>> +
>>> + /* Check Sscofpmf dependancy */
>>> + if (!riscv_cpu_cfg(env)->ext_sscofpmf && csrno == CSR_SIREG5 &&
>>> + (isel_hpm_start <= isel && isel <= ISELECT_CD_LAST)) {
>>> + goto done;
>>
>> return -EINVAL;
>>
>>> + }
>>> +
>>> + /* Check smcntrpmf dependancy */
>>> + if (!riscv_cpu_cfg(env)->ext_smcntrpmf &&
>>> + (csrno == CSR_SIREG2 || csrno == CSR_SIREG5) &&
>>> + (ISELECT_CD_FIRST <= isel && isel < isel_hpm_start)) {
>>> + goto done;
>>
>> return -EINVAL;
>>
>>> + }
>>> +
>>> + if (!get_field(env->mcounteren, BIT(ctr_index)) ||
>>> + !get_field(env->menvcfg, MENVCFG_CDE)) {
>>> + goto done;
>>
>> return -EINVAL;
>>
>>> + }
>>> +
>>> + switch (csrno) {
>>> + case CSR_SIREG:
>>> + ret = rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
>>
>> return rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
>>> + break;
>>> + case CSR_SIREG4:
>>> + ret = rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
>>
>> return rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
>>> + break;
>>> + case CSR_SIREG2:
>>> + if (ctr_index <= 2) {
>>> + ret = rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
>>
>> return rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
>>> + } else {
>>> + ret = rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
>>
>> return rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
>>
>>> + }
>>> + break;
>>> + case CSR_SIREG5:
>>> + if (ctr_index <= 2) {
>>> + ret = rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
>>
>> return rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
>>
>>> + } else {
>>> + ret = rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
>>
>> return rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
>>
>>> + }
>>> + break;
>>> + default:
>>> + goto done;
>>
>> return -EINVAL;
>>
>>> + }
>>> +
>>> +done:
>>> + return ret;
>>
>> And remove this last 'return' since we're doing all possible returns already.
>>
>
> Personally, I prefer a single return in a switch case block. That's
> why I have the jump label.
> If you feel too strongly about that, I can change as per your suggestion though.
Yeah I forgot to mention in my reply that this was more a code style suggestion than
"please change it". Feel free to keep it as is.
If you want consistency with the label + return pattern throughout the function you could
remove the instances of 'return RISCV_EXCP_ILLEGAL_INST' and do
ret = return RISCV_EXCP_ILLEGAL_INST;
goto done;
That way we don't have early 'return' exits in some places and 'goto done' in others.
And again, optional code style comments. Thanks,
Daniel
>
>>
>> Thanks,
>>
>> Daniel
>>
>>> }
>>>
>>> /*
>>> @@ -2578,6 +2833,21 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
>>> return RISCV_EXCP_NONE;
>>> }
>>>
>>> +static RISCVException read_scountinhibit(CPURISCVState *env, int csrno,
>>> + target_ulong *val)
>>> +{
>>> + /* S-mode can only access the bits delegated by M-mode */
>>> + *val = env->mcountinhibit & env->mcounteren;
>>> + return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_scountinhibit(CPURISCVState *env, int csrno,
>>> + target_ulong val)
>>> +{
>>> + write_mcountinhibit(env, csrno, val & env->mcounteren);
>>> + return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> static RISCVException read_mcounteren(CPURISCVState *env, int csrno,
>>> target_ulong *val)
>>> {
>>> @@ -2680,11 +2950,13 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>>> target_ulong val)
>>> {
>>> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>>> - uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
>>> + uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE |
>>> + MENVCFG_CBZE | MENVCFG_CDE;
>>>
>>> if (riscv_cpu_mxl(env) == MXL_RV64) {
>>> mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>> (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>>> + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0) |
>>> (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>>
>>> if (env_archcpu(env)->cfg.ext_zicfilp) {
>>> @@ -2713,7 +2985,8 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>>> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>>> uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
>>> (cfg->ext_sstc ? MENVCFG_STCE : 0) |
>>> - (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>> + (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
>>> + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0);
>>> uint64_t valh = (uint64_t)val << 32;
>>>
>>> env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
>>> @@ -5498,6 +5771,11 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>>> write_sstateen_1_3,
>>> .min_priv_ver = PRIV_VERSION_1_12_0 },
>>>
>>> + /* Supervisor Counter Delegation */
>>> + [CSR_SCOUNTINHIBIT] = {"scountinhibit", scountinhibit_pred,
>>> + read_scountinhibit, write_scountinhibit,
>>> + .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +
>>> /* Supervisor Trap Setup */
>>> [CSR_SSTATUS] = { "sstatus", smode, read_sstatus, write_sstatus,
>>> NULL, read_sstatus_i128 },
>>>
>>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support
2024-12-02 21:49 ` Daniel Henrique Barboza
@ 2024-12-02 23:47 ` Atish Kumar Patra
0 siblings, 0 replies; 23+ messages in thread
From: Atish Kumar Patra @ 2024-12-02 23:47 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-riscv, qemu-devel, kaiwenxue1, palmer, liwei1518, zhiwei_liu,
bin.meng, alistair.francis, Kaiwen Xue
On Mon, Dec 2, 2024 at 1:49 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 12/2/24 6:15 PM, Atish Kumar Patra wrote:
> > On Thu, Nov 28, 2024 at 4:53 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >>
> >>
> >> On 11/17/24 10:15 PM, Atish Patra wrote:
> >>> From: Kaiwen Xue <kaiwenx@rivosinc.com>
> >>>
> >>> The Smcdeleg/Ssccfg adds the support for counter delegation via
> >>> S*indcsr and Ssccfg.
> >>>
> >>> It also adds a new shadow CSR scountinhibit and menvcfg enable bit (CDE)
> >>> to enable this extension and scountovf virtualization.
> >>>
> >>> Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> >>> Co-developed-by: Atish Patra <atishp@rivosinc.com>
> >>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>> ---
> >>> target/riscv/csr.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> >>> 1 file changed, 289 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >>> index 97cc8059ad37..31ea8b8ec20d 100644
> >>> --- a/target/riscv/csr.c
> >>> +++ b/target/riscv/csr.c
> >>> @@ -385,6 +385,21 @@ static RISCVException aia_smode32(CPURISCVState *env, int csrno)
> >>> return smode32(env, csrno);
> >>> }
> >>>
> >>> +static RISCVException scountinhibit_pred(CPURISCVState *env, int csrno)
> >>> +{
> >>> + RISCVCPU *cpu = env_archcpu(env);
> >>> +
> >>> + if (!cpu->cfg.ext_ssccfg || !cpu->cfg.ext_smcdeleg) {
> >>> + return RISCV_EXCP_ILLEGAL_INST;
> >>> + }
> >>> +
> >>> + if (env->virt_enabled) {
> >>> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >>> + }
> >>> +
> >>> + return smode(env, csrno);
> >>> +}
> >>> +
> >>> static bool csrind_extensions_present(CPURISCVState *env)
> >>> {
> >>> return riscv_cpu_cfg(env)->ext_smcsrind || riscv_cpu_cfg(env)->ext_sscsrind;
> >>> @@ -1222,10 +1237,9 @@ done:
> >>> return result;
> >>> }
> >>>
> >>> -static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> >>> - target_ulong val)
> >>> +static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val,
> >>> + uint32_t ctr_idx)
> >>> {
> >>> - int ctr_idx = csrno - CSR_MCYCLE;
> >>> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> >>> uint64_t mhpmctr_val = val;
> >>>
> >>> @@ -1250,10 +1264,9 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
> >>> return RISCV_EXCP_NONE;
> >>> }
> >>>
> >>> -static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> >>> - target_ulong val)
> >>> +static RISCVException riscv_pmu_write_ctrh(CPURISCVState *env, target_ulong val,
> >>> + uint32_t ctr_idx)
> >>> {
> >>> - int ctr_idx = csrno - CSR_MCYCLEH;
> >>> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> >>> uint64_t mhpmctr_val = counter->mhpmcounter_val;
> >>> uint64_t mhpmctrh_val = val;
> >>> @@ -1275,6 +1288,20 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
> >>> return RISCV_EXCP_NONE;
> >>> }
> >>>
> >>> +static int write_mhpmcounter(CPURISCVState *env, int csrno, target_ulong val)
> >>> +{
> >>> + int ctr_idx = csrno - CSR_MCYCLE;
> >>> +
> >>> + return riscv_pmu_write_ctr(env, val, ctr_idx);
> >>> +}
> >>> +
> >>> +static int write_mhpmcounterh(CPURISCVState *env, int csrno, target_ulong val)
> >>> +{
> >>> + int ctr_idx = csrno - CSR_MCYCLEH;
> >>> +
> >>> + return riscv_pmu_write_ctrh(env, val, ctr_idx);
> >>> +}
> >>> +
> >>> RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> >>> bool upper_half, uint32_t ctr_idx)
> >>> {
> >>> @@ -1340,6 +1367,167 @@ static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> >>> return riscv_pmu_read_ctr(env, val, true, ctr_index);
> >>> }
> >>>
> >>> +static int rmw_cd_mhpmcounter(CPURISCVState *env, int ctr_idx,
> >>> + target_ulong *val, target_ulong new_val,
> >>> + target_ulong wr_mask)
> >>> +{
> >>> + if (wr_mask != 0 && wr_mask != -1) {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (!wr_mask && val) {
> >>> + riscv_pmu_read_ctr(env, val, false, ctr_idx);
> >>> + } else if (wr_mask) {
> >>> + riscv_pmu_write_ctr(env, new_val, ctr_idx);
> >>> + } else {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int rmw_cd_mhpmcounterh(CPURISCVState *env, int ctr_idx,
> >>> + target_ulong *val, target_ulong new_val,
> >>> + target_ulong wr_mask)
> >>> +{
> >>> + if (wr_mask != 0 && wr_mask != -1) {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (!wr_mask && val) {
> >>> + riscv_pmu_read_ctr(env, val, true, ctr_idx);
> >>> + } else if (wr_mask) {
> >>> + riscv_pmu_write_ctrh(env, new_val, ctr_idx);
> >>> + } else {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int rmw_cd_mhpmevent(CPURISCVState *env, int evt_index,
> >>> + target_ulong *val, target_ulong new_val,
> >>> + target_ulong wr_mask)
> >>> +{
> >>> + uint64_t mhpmevt_val = new_val;
> >>> +
> >>> + if (wr_mask != 0 && wr_mask != -1) {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (!wr_mask && val) {
> >>> + *val = env->mhpmevent_val[evt_index];
> >>> + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
> >>> + *val &= ~MHPMEVENT_BIT_MINH;
> >>> + }
> >>> + } else if (wr_mask) {
> >>> + wr_mask &= ~MHPMEVENT_BIT_MINH;
> >>> + mhpmevt_val = (new_val & wr_mask) |
> >>> + (env->mhpmevent_val[evt_index] & ~wr_mask);
> >>> + if (riscv_cpu_mxl(env) == MXL_RV32) {
> >>> + mhpmevt_val = mhpmevt_val |
> >>> + ((uint64_t)env->mhpmeventh_val[evt_index] << 32);
> >>> + }
> >>> + env->mhpmevent_val[evt_index] = mhpmevt_val;
> >>> + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
> >>> + } else {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int rmw_cd_mhpmeventh(CPURISCVState *env, int evt_index,
> >>> + target_ulong *val, target_ulong new_val,
> >>> + target_ulong wr_mask)
> >>> +{
> >>> + uint64_t mhpmevth_val;
> >>> + uint64_t mhpmevt_val = env->mhpmevent_val[evt_index];
> >>> +
> >>> + if (wr_mask != 0 && wr_mask != -1) {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (!wr_mask && val) {
> >>> + *val = env->mhpmeventh_val[evt_index];
> >>> + if (riscv_cpu_cfg(env)->ext_sscofpmf) {
> >>> + *val &= ~MHPMEVENTH_BIT_MINH;
> >>> + }
> >>> + } else if (wr_mask) {
> >>> + wr_mask &= ~MHPMEVENTH_BIT_MINH;
> >>> + env->mhpmeventh_val[evt_index] =
> >>> + (new_val & wr_mask) | (env->mhpmeventh_val[evt_index] & ~wr_mask);
> >>> + mhpmevth_val = env->mhpmeventh_val[evt_index];
> >>> + mhpmevt_val = mhpmevt_val | (mhpmevth_val << 32);
> >>> + riscv_pmu_update_event_map(env, mhpmevt_val, evt_index);
> >>> + } else {
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int rmw_cd_ctr_cfg(CPURISCVState *env, int cfg_index, target_ulong *val,
> >>> + target_ulong new_val, target_ulong wr_mask)
> >>> +{
> >>> + switch (cfg_index) {
> >>> + case 0: /* CYCLECFG */
> >>> + if (wr_mask) {
> >>> + wr_mask &= ~MCYCLECFG_BIT_MINH;
> >>> + env->mcyclecfg = (new_val & wr_mask) | (env->mcyclecfg & ~wr_mask);
> >>> + } else {
> >>> + *val = env->mcyclecfg &= ~MHPMEVENTH_BIT_MINH;
> >>> + }
> >>> + break;
> >>> + case 2: /* INSTRETCFG */
> >>> + if (wr_mask) {
> >>> + wr_mask &= ~MINSTRETCFG_BIT_MINH;
> >>> + env->minstretcfg = (new_val & wr_mask) |
> >>> + (env->minstretcfg & ~wr_mask);
> >>> + } else {
> >>> + *val = env->minstretcfg &= ~MHPMEVENTH_BIT_MINH;
> >>> + }
> >>> + break;
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int rmw_cd_ctr_cfgh(CPURISCVState *env, int cfg_index, target_ulong *val,
> >>> + target_ulong new_val, target_ulong wr_mask)
> >>> +{
> >>> +
> >>> + if (riscv_cpu_mxl(env) != MXL_RV32) {
> >>> + return RISCV_EXCP_ILLEGAL_INST;
> >>> + }
> >>> +
> >>> + switch (cfg_index) {
> >>> + case 0: /* CYCLECFGH */
> >>> + if (wr_mask) {
> >>> + wr_mask &= ~MCYCLECFGH_BIT_MINH;
> >>> + env->mcyclecfgh = (new_val & wr_mask) |
> >>> + (env->mcyclecfgh & ~wr_mask);
> >>> + } else {
> >>> + *val = env->mcyclecfgh;
> >>> + }
> >>> + break;
> >>> + case 2: /* INSTRETCFGH */
> >>> + if (wr_mask) {
> >>> + wr_mask &= ~MINSTRETCFGH_BIT_MINH;
> >>> + env->minstretcfgh = (new_val & wr_mask) |
> >>> + (env->minstretcfgh & ~wr_mask);
> >>> + } else {
> >>> + *val = env->minstretcfgh;
> >>> + }
> >>> + break;
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> + return 0;
> >>> +}
> >>> +
> >>> +
> >>> static RISCVException read_scountovf(CPURISCVState *env, int csrno,
> >>> target_ulong *val)
> >>> {
> >>> @@ -1349,6 +1537,14 @@ static RISCVException read_scountovf(CPURISCVState *env, int csrno,
> >>> target_ulong *mhpm_evt_val;
> >>> uint64_t of_bit_mask;
> >>>
> >>> + /* Virtualize scountovf for counter delegation */
> >>> + if (riscv_cpu_cfg(env)->ext_sscofpmf &&
> >>> + riscv_cpu_cfg(env)->ext_ssccfg &&
> >>> + get_field(env->menvcfg, MENVCFG_CDE) &&
> >>> + env->virt_enabled) {
> >>> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >>> + }
> >>> +
> >>> if (riscv_cpu_mxl(env) == MXL_RV32) {
> >>> mhpm_evt_val = env->mhpmeventh_val;
> >>> of_bit_mask = MHPMEVENTH_BIT_OF;
> >>> @@ -2292,11 +2488,70 @@ static int rmw_xireg_cd(CPURISCVState *env, int csrno,
> >>> target_ulong isel, target_ulong *val,
> >>> target_ulong new_val, target_ulong wr_mask)
> >>> {
> >>> - if (!riscv_cpu_cfg(env)->ext_smcdeleg) {
> >>> + int ret = -EINVAL;
> >>
> >> It seems like both 'ret' and the 'done' label are being used as shortcuts to do
> >> 'return ret', and every time 'ret' is assigned to something else can be replaced
> >> by an early 'return' exit.
> >>
> >> I would remove 'ret' and the 'done' label and:
> >>
> >>
> >>
> >>> + int ctr_index = isel - ISELECT_CD_FIRST;
> >>> + int isel_hpm_start = ISELECT_CD_FIRST + 3;
> >>> +
> >>> + if (!riscv_cpu_cfg(env)->ext_smcdeleg || !riscv_cpu_cfg(env)->ext_ssccfg) {
> >>> return RISCV_EXCP_ILLEGAL_INST;
> >>> }
> >>> - /* TODO: Implement the functionality later */
> >>> - return RISCV_EXCP_NONE;
> >>> +
> >>> + /* Invalid siselect value for reserved */
> >>> + if (ctr_index == 1) {
> >>> + goto done;
> >>
> >> return -EINVAL;
> >>> + }
> >>> +
> >>> + /* sireg4 and sireg5 provides access RV32 only CSRs */
> >>> + if (((csrno == CSR_SIREG5) || (csrno == CSR_SIREG4)) &&
> >>> + (riscv_cpu_mxl(env) != MXL_RV32)) {
> >>> + return RISCV_EXCP_ILLEGAL_INST;
> >>> + }
> >>> +
> >>> + /* Check Sscofpmf dependancy */
> >>> + if (!riscv_cpu_cfg(env)->ext_sscofpmf && csrno == CSR_SIREG5 &&
> >>> + (isel_hpm_start <= isel && isel <= ISELECT_CD_LAST)) {
> >>> + goto done;
> >>
> >> return -EINVAL;
> >>
> >>> + }
> >>> +
> >>> + /* Check smcntrpmf dependancy */
> >>> + if (!riscv_cpu_cfg(env)->ext_smcntrpmf &&
> >>> + (csrno == CSR_SIREG2 || csrno == CSR_SIREG5) &&
> >>> + (ISELECT_CD_FIRST <= isel && isel < isel_hpm_start)) {
> >>> + goto done;
> >>
> >> return -EINVAL;
> >>
> >>> + }
> >>> +
> >>> + if (!get_field(env->mcounteren, BIT(ctr_index)) ||
> >>> + !get_field(env->menvcfg, MENVCFG_CDE)) {
> >>> + goto done;
> >>
> >> return -EINVAL;
> >>
> >>> + }
> >>> +
> >>> + switch (csrno) {
> >>> + case CSR_SIREG:
> >>> + ret = rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
> >>
> >> return rmw_cd_mhpmcounter(env, ctr_index, val, new_val, wr_mask);
> >>> + break;
> >>> + case CSR_SIREG4:
> >>> + ret = rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
> >>
> >> return rmw_cd_mhpmcounterh(env, ctr_index, val, new_val, wr_mask);
> >>> + break;
> >>> + case CSR_SIREG2:
> >>> + if (ctr_index <= 2) {
> >>> + ret = rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
> >>
> >> return rmw_cd_ctr_cfg(env, ctr_index, val, new_val, wr_mask);
> >>> + } else {
> >>> + ret = rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
> >>
> >> return rmw_cd_mhpmevent(env, ctr_index, val, new_val, wr_mask);
> >>
> >>> + }
> >>> + break;
> >>> + case CSR_SIREG5:
> >>> + if (ctr_index <= 2) {
> >>> + ret = rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
> >>
> >> return rmw_cd_ctr_cfgh(env, ctr_index, val, new_val, wr_mask);
> >>
> >>> + } else {
> >>> + ret = rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
> >>
> >> return rmw_cd_mhpmeventh(env, ctr_index, val, new_val, wr_mask);
> >>
> >>> + }
> >>> + break;
> >>> + default:
> >>> + goto done;
> >>
> >> return -EINVAL;
> >>
> >>> + }
> >>> +
> >>> +done:
> >>> + return ret;
> >>
> >> And remove this last 'return' since we're doing all possible returns already.
> >>
> >
> > Personally, I prefer a single return in a switch case block. That's
> > why I have the jump label.
> > If you feel too strongly about that, I can change as per your suggestion though.
>
>
> Yeah I forgot to mention in my reply that this was more a code style suggestion than
> "please change it". Feel free to keep it as is.
>
> If you want consistency with the label + return pattern throughout the function you could
> remove the instances of 'return RISCV_EXCP_ILLEGAL_INST' and do
>
> ret = return RISCV_EXCP_ILLEGAL_INST;
> goto done;
>
> That way we don't have early 'return' exits in some places and 'goto done' in others.
>
Sounds good. Fixed.
> And again, optional code style comments. Thanks,
>
>
> Daniel
>
>
> >
> >>
> >> Thanks,
> >>
> >> Daniel
> >>
> >>> }
> >>>
> >>> /*
> >>> @@ -2578,6 +2833,21 @@ static RISCVException write_mcountinhibit(CPURISCVState *env, int csrno,
> >>> return RISCV_EXCP_NONE;
> >>> }
> >>>
> >>> +static RISCVException read_scountinhibit(CPURISCVState *env, int csrno,
> >>> + target_ulong *val)
> >>> +{
> >>> + /* S-mode can only access the bits delegated by M-mode */
> >>> + *val = env->mcountinhibit & env->mcounteren;
> >>> + return RISCV_EXCP_NONE;
> >>> +}
> >>> +
> >>> +static RISCVException write_scountinhibit(CPURISCVState *env, int csrno,
> >>> + target_ulong val)
> >>> +{
> >>> + write_mcountinhibit(env, csrno, val & env->mcounteren);
> >>> + return RISCV_EXCP_NONE;
> >>> +}
> >>> +
> >>> static RISCVException read_mcounteren(CPURISCVState *env, int csrno,
> >>> target_ulong *val)
> >>> {
> >>> @@ -2680,11 +2950,13 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> >>> target_ulong val)
> >>> {
> >>> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> >>> - uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
> >>> + uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE |
> >>> + MENVCFG_CBZE | MENVCFG_CDE;
> >>>
> >>> if (riscv_cpu_mxl(env) == MXL_RV64) {
> >>> mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> >>> (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> >>> + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0) |
> >>> (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> >>>
> >>> if (env_archcpu(env)->cfg.ext_zicfilp) {
> >>> @@ -2713,7 +2985,8 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> >>> const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> >>> uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> >>> (cfg->ext_sstc ? MENVCFG_STCE : 0) |
> >>> - (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> >>> + (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
> >>> + (cfg->ext_smcdeleg ? MENVCFG_CDE : 0);
> >>> uint64_t valh = (uint64_t)val << 32;
> >>>
> >>> env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> >>> @@ -5498,6 +5771,11 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >>> write_sstateen_1_3,
> >>> .min_priv_ver = PRIV_VERSION_1_12_0 },
> >>>
> >>> + /* Supervisor Counter Delegation */
> >>> + [CSR_SCOUNTINHIBIT] = {"scountinhibit", scountinhibit_pred,
> >>> + read_scountinhibit, write_scountinhibit,
> >>> + .min_priv_ver = PRIV_VERSION_1_12_0 },
> >>> +
> >>> /* Supervisor Trap Setup */
> >>> [CSR_SSTATUS] = { "sstatus", smode, read_sstatus, write_sstatus,
> >>> NULL, read_sstatus_i128 },
> >>>
> >>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 09/11] target/riscv: Invoke pmu init after feature enable
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (7 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-18 1:15 ` [PATCH v3 10/11] target/riscv: Add implied rule for counter delegation extensions Atish Patra
2024-11-18 1:15 ` [PATCH v3 11/11] target/riscv: Add configuration for S[m|s]csrind, Smcdeleg/Ssccfg Atish Patra
10 siblings, 0 replies; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis
The dependant ISA features are enabled at the end of cpu_realize
in finalize_features. Thus, PMU init should be invoked after that
only. Move the init invocation to riscv_tcg_cpu_finalize_features.
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/tcg/tcg-cpu.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index c62c2216961b..2b57aa4d1704 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -934,6 +934,20 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
error_propagate(errp, local_err);
return;
}
+#ifndef CONFIG_USER_ONLY
+ if (cpu->cfg.pmu_mask) {
+ riscv_pmu_init(cpu, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ if (cpu->cfg.ext_sscofpmf) {
+ cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ riscv_pmu_timer_cb, cpu);
+ }
+ }
+#endif
}
void riscv_tcg_cpu_finalize_dynamic_decoder(RISCVCPU *cpu)
@@ -981,7 +995,6 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
#ifndef CONFIG_USER_ONLY
CPURISCVState *env = &cpu->env;
- Error *local_err = NULL;
tcg_cflags_set(CPU(cs), CF_PCREL);
@@ -989,19 +1002,6 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
riscv_timer_init(cpu);
}
- if (cpu->cfg.pmu_mask) {
- riscv_pmu_init(cpu, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
- return false;
- }
-
- if (cpu->cfg.ext_sscofpmf) {
- cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
- riscv_pmu_timer_cb, cpu);
- }
- }
-
/* With H-Ext, VSSIP, VSTIP, VSEIP and SGEIP are hardwired to one. */
if (riscv_has_ext(env, RVH)) {
env->mideleg = MIP_VSSIP | MIP_VSTIP | MIP_VSEIP | MIP_SGEIP;
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v3 10/11] target/riscv: Add implied rule for counter delegation extensions
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (8 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 09/11] target/riscv: Invoke pmu init after feature enable Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-28 12:54 ` Daniel Henrique Barboza
2024-11-18 1:15 ` [PATCH v3 11/11] target/riscv: Add configuration for S[m|s]csrind, Smcdeleg/Ssccfg Atish Patra
10 siblings, 1 reply; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis
The counter delegation/configuration extensions depend on the following
extensions.
1. Smcdeleg - To enable counter delegation from M to S
2. S[m|s]csrind - To enable indirect access CSRs
Add an implied rule so that these extensions are enabled by default
if the sscfg extension is enabled.
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/cpu.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 82edd28e2e1d..410ca2e3a666 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -2642,6 +2642,16 @@ static RISCVCPUImpliedExtsRule ZVKSG_IMPLIED = {
},
};
+static RISCVCPUImpliedExtsRule SSCFG_IMPLIED = {
+ .ext = CPU_CFG_OFFSET(ext_ssccfg),
+ .implied_multi_exts = {
+ CPU_CFG_OFFSET(ext_smcsrind), CPU_CFG_OFFSET(ext_sscsrind),
+ CPU_CFG_OFFSET(ext_smcdeleg),
+
+ RISCV_IMPLIED_EXTS_RULE_END
+ },
+};
+
RISCVCPUImpliedExtsRule *riscv_misa_ext_implied_rules[] = {
&RVA_IMPLIED, &RVD_IMPLIED, &RVF_IMPLIED,
&RVM_IMPLIED, &RVV_IMPLIED, NULL
@@ -2659,7 +2669,7 @@ RISCVCPUImpliedExtsRule *riscv_multi_ext_implied_rules[] = {
&ZVE64X_IMPLIED, &ZVFBFMIN_IMPLIED, &ZVFBFWMA_IMPLIED,
&ZVFH_IMPLIED, &ZVFHMIN_IMPLIED, &ZVKN_IMPLIED,
&ZVKNC_IMPLIED, &ZVKNG_IMPLIED, &ZVKNHB_IMPLIED,
- &ZVKS_IMPLIED, &ZVKSC_IMPLIED, &ZVKSG_IMPLIED,
+ &ZVKS_IMPLIED, &ZVKSC_IMPLIED, &ZVKSG_IMPLIED, &SSCFG_IMPLIED,
NULL
};
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 10/11] target/riscv: Add implied rule for counter delegation extensions
2024-11-18 1:15 ` [PATCH v3 10/11] target/riscv: Add implied rule for counter delegation extensions Atish Patra
@ 2024-11-28 12:54 ` Daniel Henrique Barboza
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Henrique Barboza @ 2024-11-28 12:54 UTC (permalink / raw)
To: Atish Patra, qemu-riscv, qemu-devel
Cc: kaiwenxue1, palmer, liwei1518, zhiwei_liu, bin.meng,
alistair.francis
On 11/17/24 10:15 PM, Atish Patra wrote:
> The counter delegation/configuration extensions depend on the following
> extensions.
>
> 1. Smcdeleg - To enable counter delegation from M to S
> 2. S[m|s]csrind - To enable indirect access CSRs
>
> Add an implied rule so that these extensions are enabled by default
> if the sscfg extension is enabled.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/cpu.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 82edd28e2e1d..410ca2e3a666 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2642,6 +2642,16 @@ static RISCVCPUImpliedExtsRule ZVKSG_IMPLIED = {
> },
> };
>
> +static RISCVCPUImpliedExtsRule SSCFG_IMPLIED = {
> + .ext = CPU_CFG_OFFSET(ext_ssccfg),
> + .implied_multi_exts = {
> + CPU_CFG_OFFSET(ext_smcsrind), CPU_CFG_OFFSET(ext_sscsrind),
> + CPU_CFG_OFFSET(ext_smcdeleg),
> +
> + RISCV_IMPLIED_EXTS_RULE_END
> + },
> +};
> +
> RISCVCPUImpliedExtsRule *riscv_misa_ext_implied_rules[] = {
> &RVA_IMPLIED, &RVD_IMPLIED, &RVF_IMPLIED,
> &RVM_IMPLIED, &RVV_IMPLIED, NULL
> @@ -2659,7 +2669,7 @@ RISCVCPUImpliedExtsRule *riscv_multi_ext_implied_rules[] = {
> &ZVE64X_IMPLIED, &ZVFBFMIN_IMPLIED, &ZVFBFWMA_IMPLIED,
> &ZVFH_IMPLIED, &ZVFHMIN_IMPLIED, &ZVKN_IMPLIED,
> &ZVKNC_IMPLIED, &ZVKNG_IMPLIED, &ZVKNHB_IMPLIED,
> - &ZVKS_IMPLIED, &ZVKSC_IMPLIED, &ZVKSG_IMPLIED,
> + &ZVKS_IMPLIED, &ZVKSC_IMPLIED, &ZVKSG_IMPLIED, &SSCFG_IMPLIED,
> NULL
> };
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 11/11] target/riscv: Add configuration for S[m|s]csrind, Smcdeleg/Ssccfg
2024-11-18 1:15 [PATCH v3 00/11] Add RISC-V Counter delegation ISA extension support Atish Patra
` (9 preceding siblings ...)
2024-11-18 1:15 ` [PATCH v3 10/11] target/riscv: Add implied rule for counter delegation extensions Atish Patra
@ 2024-11-18 1:15 ` Atish Patra
2024-11-28 12:58 ` Daniel Henrique Barboza
10 siblings, 1 reply; 23+ messages in thread
From: Atish Patra @ 2024-11-18 1:15 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: kaiwenxue1, Atish Patra, palmer, liwei1518, zhiwei_liu, bin.meng,
dbarboza, alistair.francis
Add configuration options so that they can be enabled/disabld from
qemu commandline.
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/cpu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 410ca2e3a666..2a4f285a974f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1477,6 +1477,10 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
/* Defaults for standard extensions */
MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
MULTI_EXT_CFG_BOOL("smcntrpmf", ext_smcntrpmf, false),
+ MULTI_EXT_CFG_BOOL("smcsrind", ext_smcsrind, false),
+ MULTI_EXT_CFG_BOOL("smcdeleg", ext_smcdeleg, false),
+ MULTI_EXT_CFG_BOOL("sscsrind", ext_sscsrind, false),
+ MULTI_EXT_CFG_BOOL("ssccfg", ext_ssccfg, false),
MULTI_EXT_CFG_BOOL("zifencei", ext_zifencei, true),
MULTI_EXT_CFG_BOOL("zicfilp", ext_zicfilp, false),
MULTI_EXT_CFG_BOOL("zicfiss", ext_zicfiss, false),
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v3 11/11] target/riscv: Add configuration for S[m|s]csrind, Smcdeleg/Ssccfg
2024-11-18 1:15 ` [PATCH v3 11/11] target/riscv: Add configuration for S[m|s]csrind, Smcdeleg/Ssccfg Atish Patra
@ 2024-11-28 12:58 ` Daniel Henrique Barboza
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Henrique Barboza @ 2024-11-28 12:58 UTC (permalink / raw)
To: Atish Patra, qemu-riscv, qemu-devel
Cc: kaiwenxue1, palmer, liwei1518, zhiwei_liu, bin.meng,
alistair.francis
On 11/17/24 10:15 PM, Atish Patra wrote:
> Add configuration options so that they can be enabled/disabld from
> qemu commandline.
>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/cpu.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 410ca2e3a666..2a4f285a974f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1477,6 +1477,10 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
> /* Defaults for standard extensions */
> MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
> MULTI_EXT_CFG_BOOL("smcntrpmf", ext_smcntrpmf, false),
> + MULTI_EXT_CFG_BOOL("smcsrind", ext_smcsrind, false),
> + MULTI_EXT_CFG_BOOL("smcdeleg", ext_smcdeleg, false),
> + MULTI_EXT_CFG_BOOL("sscsrind", ext_sscsrind, false),
> + MULTI_EXT_CFG_BOOL("ssccfg", ext_ssccfg, false),
> MULTI_EXT_CFG_BOOL("zifencei", ext_zifencei, true),
> MULTI_EXT_CFG_BOOL("zicfilp", ext_zicfilp, false),
> MULTI_EXT_CFG_BOOL("zicfiss", ext_zicfiss, false),
>
^ permalink raw reply [flat|nested] 23+ messages in thread