From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: Atish Patra <atishp@rivosinc.com>,
qemu-riscv@nongnu.org, qemu-devel@nongnu.org
Cc: kaiwenxue1@gmail.com, palmer@dabbelt.com, liwei1518@gmail.com,
zhiwei_liu@linux.alibaba.com, bin.meng@windriver.com,
alistair.francis@wdc.com, Kaiwen Xue <kaiwenx@rivosinc.com>
Subject: Re: [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support
Date: Thu, 28 Nov 2024 09:53:52 -0300 [thread overview]
Message-ID: <ac472499-b9af-4e40-8796-fdea9ef2b69c@ventanamicro.com> (raw)
In-Reply-To: <20241117-counter_delegation-v3-8-476d6f36e3c8@rivosinc.com>
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 },
>
next prev parent reply other threads:[~2024-11-28 12:54 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
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-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
2024-11-18 1:15 ` [PATCH v3 03/11] target/riscv: Enable S*stateen bits for AIA Atish Patra
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
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
2024-11-18 1:15 ` [PATCH v3 06/11] target/riscv: Add counter delegation definitions Atish Patra
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 ` [PATCH v3 08/11] target/riscv: Add counter delegation/configuration support Atish Patra
2024-11-28 12:53 ` Daniel Henrique Barboza [this message]
2024-12-02 21:15 ` Atish Kumar Patra
2024-12-02 21:49 ` Daniel Henrique Barboza
2024-12-02 23:47 ` Atish Kumar Patra
2024-11-18 1:15 ` [PATCH v3 09/11] target/riscv: Invoke pmu init after feature enable Atish Patra
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
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ac472499-b9af-4e40-8796-fdea9ef2b69c@ventanamicro.com \
--to=dbarboza@ventanamicro.com \
--cc=alistair.francis@wdc.com \
--cc=atishp@rivosinc.com \
--cc=bin.meng@windriver.com \
--cc=kaiwenx@rivosinc.com \
--cc=kaiwenxue1@gmail.com \
--cc=liwei1518@gmail.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.