[v7,08/12] target/riscv: Add sscofpmf extension support

Message ID	20220331000127.2107823-9-atishp@rivosinc.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org> From: Atish Patra <atishp@rivosinc.com> To: qemu-devel@nongnu.org Subject: [PATCH v7 08/12] target/riscv: Add sscofpmf extension support Date: Wed, 30 Mar 2022 17:01:22 -0700 Message-Id: <20220331000127.2107823-9-atishp@rivosinc.com> In-Reply-To: <20220331000127.2107823-1-atishp@rivosinc.com> References: <20220331000127.2107823-1-atishp@rivosinc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=2607:f8b0:4864:20::529; envelope-from=atishp@rivosinc.com; helo=mail-pg1-x529.google.com X-Spam_score_int: -4 X-Spam_score: -0.5 X-Spam_bar: / X-Spam_report: (-0.5 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, PDS_HP_HELO_NORDNS=0.659, RCVD_IN_DNSWL_NONE=-0.0001, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no autolearn_force=no X-Spam_action: no action Precedence: list Cc: Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Atish Patra <atishp@rivosinc.com>, Palmer Dabbelt <palmer@dabbelt.com>, qemu-riscv@nongnu.org Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org>
Series	Improve PMU support \| expand [v7,00/12] Improve PMU support [v7,01/12] target/riscv: Fix PMU CSR predicate function [v7,02/12] target/riscv: Implement PMU CSR predicate function for S-mode [v7,03/12] target/riscv: pmu: Rename the counters extension to pmu [v7,04/12] target/riscv: pmu: Make number of counters configurable [v7,05/12] target/riscv: Implement mcountinhibit CSR [v7,06/12] target/riscv: Add support for hpmcounters/hpmevents [v7,07/12] target/riscv: Support mcycle/minstret write operation [v7,08/12] target/riscv: Add sscofpmf extension support [v7,09/12] target/riscv: Simplify counter predicate function [v7,10/12] target/riscv: Add few cache related PMU events [v7,11/12] hw/riscv: virt: Add PMU DT node to the device tree [v7,12/12] target/riscv: Update the privilege field for sscofpmf CSRs

Atish Kumar Patra March 31, 2022, 12:01 a.m. UTC

The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
extension allows the perf to handle overflow interrupts and filtering
support. This patch provides a framework for programmable
counters to leverage the extension. As the extension doesn't have any
provision for the overflow bit for fixed counters, the fixed events
can also be monitoring using programmable counters. The underlying
counters for cycle and instruction counters are always running. Thus,
a separate timer device is programmed to handle the overflow.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.c      |  11 ++
 target/riscv/cpu.h      |  25 +++
 target/riscv/cpu_bits.h |  55 +++++++
 target/riscv/csr.c      | 156 ++++++++++++++++--
 target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
 target/riscv/pmu.h      |   7 +
 6 files changed, 590 insertions(+), 11 deletions(-)

Alistair Francis April 13, 2022, 7:08 a.m. UTC | #1

On Thu, Mar 31, 2022 at 10:19 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
> and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
> extension allows the perf to handle overflow interrupts and filtering
> support. This patch provides a framework for programmable
> counters to leverage the extension. As the extension doesn't have any
> provision for the overflow bit for fixed counters, the fixed events
> can also be monitoring using programmable counters. The underlying
> counters for cycle and instruction counters are always running. Thus,
> a separate timer device is programmed to handle the overflow.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  target/riscv/cpu.c      |  11 ++
>  target/riscv/cpu.h      |  25 +++
>  target/riscv/cpu_bits.h |  55 +++++++
>  target/riscv/csr.c      | 156 ++++++++++++++++--
>  target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
>  target/riscv/pmu.h      |   7 +
>  6 files changed, 590 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f63602828680..9715eed2fc4e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -22,6 +22,7 @@
>  #include "qemu/ctype.h"
>  #include "qemu/log.h"
>  #include "cpu.h"
> +#include "pmu.h"
>  #include "internals.h"
>  #include "exec/exec-all.h"
>  #include "qapi/error.h"
> @@ -696,6 +697,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>          set_misa(env, env->misa_mxl, ext);
>      }
>
> +#ifndef CONFIG_USER_ONLY
> +    if (cpu->cfg.pmu_num) {
> +        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                          riscv_pmu_timer_cb, cpu);
> +        }
> +     }
> +#endif
> +
>      riscv_cpu_register_gdb_regs_for_features(cs);
>
>      qemu_init_vcpu(cs);
> @@ -795,6 +805,7 @@ static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
>      DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
>      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> +    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
>      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
>      DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0fa15595fb37..a0e2279ea5e6 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -131,6 +131,8 @@ typedef struct PMUCTRState {
>      /* Snapshort value of a counter in RV32 */
>      target_ulong mhpmcounterh_prev;
>      bool started;
> +    /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> +    target_ulong irq_overflow_left;
>  } PMUCTRState;
>
>  struct CPUArchState {
> @@ -291,6 +293,9 @@ struct CPUArchState {
>      /* PMU event selector configured values. First three are unused*/
>      target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
>
> +    /* PMU event selector configured values for RV32*/
> +    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> +
>      target_ulong sscratch;
>      target_ulong mscratch;
>
> @@ -413,6 +418,7 @@ struct RISCVCPUConfig {
>      bool ext_zhinxmin;
>      bool ext_zve32f;
>      bool ext_zve64f;
> +    bool ext_sscofpmf;
>
>      /* Vendor-specific custom extensions */
>      bool ext_XVentanaCondOps;
> @@ -452,6 +458,12 @@ struct ArchCPU {
>
>      /* Configuration Settings */
>      RISCVCPUConfig cfg;
> +
> +    QEMUTimer *pmu_timer;
> +    /* A bitmask of Available programmable counters */
> +    uint32_t pmu_avail_ctrs;
> +    /* Mapping of events to counters */
> +    GHashTable *pmu_event_ctr_map;
>  };
>
>  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> @@ -709,6 +721,19 @@ enum {
>      CSR_TABLE_SIZE = 0x1000
>  };
>
> +/**
> + * The event id are encoded based on the encoding specified in the
> + * SBI specification v0.3
> + */
> +
> +enum riscv_pmu_event_idx {
> +    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> +    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> +    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> +    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> +    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> +};
> +
>  /* CSR function table */
>  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 48b39e6d52a7..d0b53e5ea072 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -400,6 +400,37 @@
>  #define CSR_MHPMEVENT29     0x33d
>  #define CSR_MHPMEVENT30     0x33e
>  #define CSR_MHPMEVENT31     0x33f
> +
> +#define CSR_MHPMEVENT3H     0x723
> +#define CSR_MHPMEVENT4H     0x724
> +#define CSR_MHPMEVENT5H     0x725
> +#define CSR_MHPMEVENT6H     0x726
> +#define CSR_MHPMEVENT7H     0x727
> +#define CSR_MHPMEVENT8H     0x728
> +#define CSR_MHPMEVENT9H     0x729
> +#define CSR_MHPMEVENT10H    0x72a
> +#define CSR_MHPMEVENT11H    0x72b
> +#define CSR_MHPMEVENT12H    0x72c
> +#define CSR_MHPMEVENT13H    0x72d
> +#define CSR_MHPMEVENT14H    0x72e
> +#define CSR_MHPMEVENT15H    0x72f
> +#define CSR_MHPMEVENT16H    0x730
> +#define CSR_MHPMEVENT17H    0x731
> +#define CSR_MHPMEVENT18H    0x732
> +#define CSR_MHPMEVENT19H    0x733
> +#define CSR_MHPMEVENT20H    0x734
> +#define CSR_MHPMEVENT21H    0x735
> +#define CSR_MHPMEVENT22H    0x736
> +#define CSR_MHPMEVENT23H    0x737
> +#define CSR_MHPMEVENT24H    0x738
> +#define CSR_MHPMEVENT25H    0x739
> +#define CSR_MHPMEVENT26H    0x73a
> +#define CSR_MHPMEVENT27H    0x73b
> +#define CSR_MHPMEVENT28H    0x73c
> +#define CSR_MHPMEVENT29H    0x73d
> +#define CSR_MHPMEVENT30H    0x73e
> +#define CSR_MHPMEVENT31H    0x73f
> +
>  #define CSR_MHPMCOUNTER3H   0xb83
>  #define CSR_MHPMCOUNTER4H   0xb84
>  #define CSR_MHPMCOUNTER5H   0xb85
> @@ -461,6 +492,7 @@
>  #define CSR_VSMTE           0x2c0
>  #define CSR_VSPMMASK        0x2c1
>  #define CSR_VSPMBASE        0x2c2
> +#define CSR_SCOUNTOVF       0xda0
>
>  /* mstatus CSR bits */
>  #define MSTATUS_UIE         0x00000001
> @@ -635,6 +667,7 @@ typedef enum RISCVException {
>  #define IRQ_VS_EXT                         10
>  #define IRQ_M_EXT                          11
>  #define IRQ_S_GEXT                         12
> +#define IRQ_PMU_OVF                        13
>  #define IRQ_LOCAL_MAX                      16
>  #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
>
> @@ -652,11 +685,13 @@ typedef enum RISCVException {
>  #define MIP_VSEIP                          (1 << IRQ_VS_EXT)
>  #define MIP_MEIP                           (1 << IRQ_M_EXT)
>  #define MIP_SGEIP                          (1 << IRQ_S_GEXT)
> +#define MIP_LCOFIP                         (1 << IRQ_PMU_OVF)
>
>  /* sip masks */
>  #define SIP_SSIP                           MIP_SSIP
>  #define SIP_STIP                           MIP_STIP
>  #define SIP_SEIP                           MIP_SEIP
> +#define SIP_LCOFIP                         MIP_LCOFIP
>
>  /* MIE masks */
>  #define MIE_SEIE                           (1 << IRQ_S_EXT)
> @@ -804,4 +839,24 @@ typedef enum RISCVException {
>  #define HVICTL_VALID_MASK                  \
>      (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
>
> +/* PMU related bits */
> +#define MIE_LCOFIE                         (1 << IRQ_PMU_OVF)
> +
> +#define MHPMEVENT_BIT_OF                   BIT_ULL(63)
> +#define MHPMEVENTH_BIT_OF                  BIT(31)
> +#define MHPMEVENT_BIT_MINH                 BIT_ULL(62)
> +#define MHPMEVENTH_BIT_MINH                BIT(30)
> +#define MHPMEVENT_BIT_SINH                 BIT_ULL(61)
> +#define MHPMEVENTH_BIT_SINH                BIT(29)
> +#define MHPMEVENT_BIT_UINH                 BIT_ULL(60)
> +#define MHPMEVENTH_BIT_UINH                BIT(28)
> +#define MHPMEVENT_BIT_VSINH                BIT_ULL(59)
> +#define MHPMEVENTH_BIT_VSINH               BIT(27)
> +#define MHPMEVENT_BIT_VUINH                BIT_ULL(58)
> +#define MHPMEVENTH_BIT_VUINH               BIT(26)
> +
> +#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
> +#define MHPMEVENT_IDX_MASK                 0xFFFFF
> +#define MHPMEVENT_SSCOF_RESVD              16
> +
>  #endif
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 04796b99d0fe..519d6377fd9f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -72,7 +72,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>      CPUState *cs = env_cpu(env);
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      int ctr_index;
> -    int base_csrno = CSR_HPMCOUNTER3;
> +    int base_csrno = CSR_CYCLE;
>      bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
>
>      if (rv32 && csrno >= CSR_CYCLEH) {
> @@ -81,11 +81,18 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>      }
>      ctr_index = csrno - base_csrno;
>
> -    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> +    if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) ||
> +        (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) {
> +        goto skip_ext_pmu_check;
> +    }
> +
> +    if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) {
>          /* No counter is enabled in PMU or the counter is out of range */
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>
> +skip_ext_pmu_check:
> +
>      if (env->priv == PRV_S) {
>          switch (csrno) {
>          case CSR_CYCLE:
> @@ -104,7 +111,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>              }
>              break;
>          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> -            ctr_index = csrno - CSR_CYCLE;
>              if (!get_field(env->mcounteren, 1 << ctr_index)) {
>                  return RISCV_EXCP_ILLEGAL_INST;
>              }
> @@ -128,7 +134,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>                  }
>                  break;
>              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> -                ctr_index = csrno - CSR_CYCLEH;
>                  if (!get_field(env->mcounteren, 1 << ctr_index)) {
>                      return RISCV_EXCP_ILLEGAL_INST;
>                  }
> @@ -158,7 +163,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>              }
>              break;
>          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> -            ctr_index = csrno - CSR_CYCLE;
>              if (!get_field(env->hcounteren, 1 << ctr_index) &&
>                   get_field(env->mcounteren, 1 << ctr_index)) {
>                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> @@ -186,7 +190,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>                  }
>                  break;
>              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> -                ctr_index = csrno - CSR_CYCLEH;
>                  if (!get_field(env->hcounteren, 1 << ctr_index) &&
>                       get_field(env->mcounteren, 1 << ctr_index)) {
>                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> @@ -238,6 +241,18 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
>      return mctr(env, csrno);
>  }
>
> +static RISCVException sscofpmf(CPURISCVState *env, int csrno)
> +{
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +
> +    if (!cpu->cfg.ext_sscofpmf) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
>  static RISCVException any(CPURISCVState *env, int csrno)
>  {
>      return RISCV_EXCP_NONE;
> @@ -622,11 +637,36 @@ static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong val)
>  {
>      int evt_index = csrno - CSR_MCOUNTINHIBIT;
>
> +    if (riscv_cpu_mxl(env) != MXL_RV32) {

Is this right? What if the guest writes the high bytes first?

Alistair

Atish Kumar Patra April 15, 2022, 11:54 p.m. UTC | #2

On Wed, Apr 13, 2022 at 12:08 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Mar 31, 2022 at 10:19 AM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
> > and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
> > extension allows the perf to handle overflow interrupts and filtering
> > support. This patch provides a framework for programmable
> > counters to leverage the extension. As the extension doesn't have any
> > provision for the overflow bit for fixed counters, the fixed events
> > can also be monitoring using programmable counters. The underlying
> > counters for cycle and instruction counters are always running. Thus,
> > a separate timer device is programmed to handle the overflow.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  target/riscv/cpu.c      |  11 ++
> >  target/riscv/cpu.h      |  25 +++
> >  target/riscv/cpu_bits.h |  55 +++++++
> >  target/riscv/csr.c      | 156 ++++++++++++++++--
> >  target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
> >  target/riscv/pmu.h      |   7 +
> >  6 files changed, 590 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index f63602828680..9715eed2fc4e 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -22,6 +22,7 @@
> >  #include "qemu/ctype.h"
> >  #include "qemu/log.h"
> >  #include "cpu.h"
> > +#include "pmu.h"
> >  #include "internals.h"
> >  #include "exec/exec-all.h"
> >  #include "qapi/error.h"
> > @@ -696,6 +697,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >          set_misa(env, env->misa_mxl, ext);
> >      }
> >
> > +#ifndef CONFIG_USER_ONLY
> > +    if (cpu->cfg.pmu_num) {
> > +        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> > +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > +                                          riscv_pmu_timer_cb, cpu);
> > +        }
> > +     }
> > +#endif
> > +
> >      riscv_cpu_register_gdb_regs_for_features(cs);
> >
> >      qemu_init_vcpu(cs);
> > @@ -795,6 +805,7 @@ static Property riscv_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> >      DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > +    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
> >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> >      DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0fa15595fb37..a0e2279ea5e6 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -131,6 +131,8 @@ typedef struct PMUCTRState {
> >      /* Snapshort value of a counter in RV32 */
> >      target_ulong mhpmcounterh_prev;
> >      bool started;
> > +    /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > +    target_ulong irq_overflow_left;
> >  } PMUCTRState;
> >
> >  struct CPUArchState {
> > @@ -291,6 +293,9 @@ struct CPUArchState {
> >      /* PMU event selector configured values. First three are unused*/
> >      target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
> >
> > +    /* PMU event selector configured values for RV32*/
> > +    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> > +
> >      target_ulong sscratch;
> >      target_ulong mscratch;
> >
> > @@ -413,6 +418,7 @@ struct RISCVCPUConfig {
> >      bool ext_zhinxmin;
> >      bool ext_zve32f;
> >      bool ext_zve64f;
> > +    bool ext_sscofpmf;
> >
> >      /* Vendor-specific custom extensions */
> >      bool ext_XVentanaCondOps;
> > @@ -452,6 +458,12 @@ struct ArchCPU {
> >
> >      /* Configuration Settings */
> >      RISCVCPUConfig cfg;
> > +
> > +    QEMUTimer *pmu_timer;
> > +    /* A bitmask of Available programmable counters */
> > +    uint32_t pmu_avail_ctrs;
> > +    /* Mapping of events to counters */
> > +    GHashTable *pmu_event_ctr_map;
> >  };
> >
> >  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> > @@ -709,6 +721,19 @@ enum {
> >      CSR_TABLE_SIZE = 0x1000
> >  };
> >
> > +/**
> > + * The event id are encoded based on the encoding specified in the
> > + * SBI specification v0.3
> > + */
> > +
> > +enum riscv_pmu_event_idx {
> > +    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > +    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > +    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > +    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > +    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > +};
> > +
> >  /* CSR function table */
> >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 48b39e6d52a7..d0b53e5ea072 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -400,6 +400,37 @@
> >  #define CSR_MHPMEVENT29     0x33d
> >  #define CSR_MHPMEVENT30     0x33e
> >  #define CSR_MHPMEVENT31     0x33f
> > +
> > +#define CSR_MHPMEVENT3H     0x723
> > +#define CSR_MHPMEVENT4H     0x724
> > +#define CSR_MHPMEVENT5H     0x725
> > +#define CSR_MHPMEVENT6H     0x726
> > +#define CSR_MHPMEVENT7H     0x727
> > +#define CSR_MHPMEVENT8H     0x728
> > +#define CSR_MHPMEVENT9H     0x729
> > +#define CSR_MHPMEVENT10H    0x72a
> > +#define CSR_MHPMEVENT11H    0x72b
> > +#define CSR_MHPMEVENT12H    0x72c
> > +#define CSR_MHPMEVENT13H    0x72d
> > +#define CSR_MHPMEVENT14H    0x72e
> > +#define CSR_MHPMEVENT15H    0x72f
> > +#define CSR_MHPMEVENT16H    0x730
> > +#define CSR_MHPMEVENT17H    0x731
> > +#define CSR_MHPMEVENT18H    0x732
> > +#define CSR_MHPMEVENT19H    0x733
> > +#define CSR_MHPMEVENT20H    0x734
> > +#define CSR_MHPMEVENT21H    0x735
> > +#define CSR_MHPMEVENT22H    0x736
> > +#define CSR_MHPMEVENT23H    0x737
> > +#define CSR_MHPMEVENT24H    0x738
> > +#define CSR_MHPMEVENT25H    0x739
> > +#define CSR_MHPMEVENT26H    0x73a
> > +#define CSR_MHPMEVENT27H    0x73b
> > +#define CSR_MHPMEVENT28H    0x73c
> > +#define CSR_MHPMEVENT29H    0x73d
> > +#define CSR_MHPMEVENT30H    0x73e
> > +#define CSR_MHPMEVENT31H    0x73f
> > +
> >  #define CSR_MHPMCOUNTER3H   0xb83
> >  #define CSR_MHPMCOUNTER4H   0xb84
> >  #define CSR_MHPMCOUNTER5H   0xb85
> > @@ -461,6 +492,7 @@
> >  #define CSR_VSMTE           0x2c0
> >  #define CSR_VSPMMASK        0x2c1
> >  #define CSR_VSPMBASE        0x2c2
> > +#define CSR_SCOUNTOVF       0xda0
> >
> >  /* mstatus CSR bits */
> >  #define MSTATUS_UIE         0x00000001
> > @@ -635,6 +667,7 @@ typedef enum RISCVException {
> >  #define IRQ_VS_EXT                         10
> >  #define IRQ_M_EXT                          11
> >  #define IRQ_S_GEXT                         12
> > +#define IRQ_PMU_OVF                        13
> >  #define IRQ_LOCAL_MAX                      16
> >  #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
> >
> > @@ -652,11 +685,13 @@ typedef enum RISCVException {
> >  #define MIP_VSEIP                          (1 << IRQ_VS_EXT)
> >  #define MIP_MEIP                           (1 << IRQ_M_EXT)
> >  #define MIP_SGEIP                          (1 << IRQ_S_GEXT)
> > +#define MIP_LCOFIP                         (1 << IRQ_PMU_OVF)
> >
> >  /* sip masks */
> >  #define SIP_SSIP                           MIP_SSIP
> >  #define SIP_STIP                           MIP_STIP
> >  #define SIP_SEIP                           MIP_SEIP
> > +#define SIP_LCOFIP                         MIP_LCOFIP
> >
> >  /* MIE masks */
> >  #define MIE_SEIE                           (1 << IRQ_S_EXT)
> > @@ -804,4 +839,24 @@ typedef enum RISCVException {
> >  #define HVICTL_VALID_MASK                  \
> >      (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
> >
> > +/* PMU related bits */
> > +#define MIE_LCOFIE                         (1 << IRQ_PMU_OVF)
> > +
> > +#define MHPMEVENT_BIT_OF                   BIT_ULL(63)
> > +#define MHPMEVENTH_BIT_OF                  BIT(31)
> > +#define MHPMEVENT_BIT_MINH                 BIT_ULL(62)
> > +#define MHPMEVENTH_BIT_MINH                BIT(30)
> > +#define MHPMEVENT_BIT_SINH                 BIT_ULL(61)
> > +#define MHPMEVENTH_BIT_SINH                BIT(29)
> > +#define MHPMEVENT_BIT_UINH                 BIT_ULL(60)
> > +#define MHPMEVENTH_BIT_UINH                BIT(28)
> > +#define MHPMEVENT_BIT_VSINH                BIT_ULL(59)
> > +#define MHPMEVENTH_BIT_VSINH               BIT(27)
> > +#define MHPMEVENT_BIT_VUINH                BIT_ULL(58)
> > +#define MHPMEVENTH_BIT_VUINH               BIT(26)
> > +
> > +#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
> > +#define MHPMEVENT_IDX_MASK                 0xFFFFF
> > +#define MHPMEVENT_SSCOF_RESVD              16
> > +
> >  #endif
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 04796b99d0fe..519d6377fd9f 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -72,7 +72,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >      CPUState *cs = env_cpu(env);
> >      RISCVCPU *cpu = RISCV_CPU(cs);
> >      int ctr_index;
> > -    int base_csrno = CSR_HPMCOUNTER3;
> > +    int base_csrno = CSR_CYCLE;
> >      bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> >
> >      if (rv32 && csrno >= CSR_CYCLEH) {
> > @@ -81,11 +81,18 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >      }
> >      ctr_index = csrno - base_csrno;
> >
> > -    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> > +    if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) ||
> > +        (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) {
> > +        goto skip_ext_pmu_check;
> > +    }
> > +
> > +    if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) {
> >          /* No counter is enabled in PMU or the counter is out of range */
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> >
> > +skip_ext_pmu_check:
> > +
> >      if (env->priv == PRV_S) {
> >          switch (csrno) {
> >          case CSR_CYCLE:
> > @@ -104,7 +111,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >              }
> >              break;
> >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > -            ctr_index = csrno - CSR_CYCLE;
> >              if (!get_field(env->mcounteren, 1 << ctr_index)) {
> >                  return RISCV_EXCP_ILLEGAL_INST;
> >              }
> > @@ -128,7 +134,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >                  }
> >                  break;
> >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > -                ctr_index = csrno - CSR_CYCLEH;
> >                  if (!get_field(env->mcounteren, 1 << ctr_index)) {
> >                      return RISCV_EXCP_ILLEGAL_INST;
> >                  }
> > @@ -158,7 +163,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >              }
> >              break;
> >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > -            ctr_index = csrno - CSR_CYCLE;
> >              if (!get_field(env->hcounteren, 1 << ctr_index) &&
> >                   get_field(env->mcounteren, 1 << ctr_index)) {
> >                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > @@ -186,7 +190,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >                  }
> >                  break;
> >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > -                ctr_index = csrno - CSR_CYCLEH;
> >                  if (!get_field(env->hcounteren, 1 << ctr_index) &&
> >                       get_field(env->mcounteren, 1 << ctr_index)) {
> >                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > @@ -238,6 +241,18 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
> >      return mctr(env, csrno);
> >  }
> >
> > +static RISCVException sscofpmf(CPURISCVState *env, int csrno)
> > +{
> > +    CPUState *cs = env_cpu(env);
> > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > +
> > +    if (!cpu->cfg.ext_sscofpmf) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >  static RISCVException any(CPURISCVState *env, int csrno)
> >  {
> >      return RISCV_EXCP_NONE;
> > @@ -622,11 +637,36 @@ static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong val)
> >  {
> >      int evt_index = csrno - CSR_MCOUNTINHIBIT;
> >
> > +    if (riscv_cpu_mxl(env) != MXL_RV32) {
>
> Is this right? What if the guest writes the high bytes first?
>

Most of the current software is implemented with lower bytes first.
But I understand your concern
that it doesn't provide any guarantee. We probably can have another
field that can track the order of the updates.
riscv_pmu_update_event_map will be only called when both lower/upper
half is complete.


> Alistair

Alistair Francis April 18, 2022, 10:45 p.m. UTC | #3

On Sat, Apr 16, 2022 at 9:54 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Wed, Apr 13, 2022 at 12:08 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Mar 31, 2022 at 10:19 AM Atish Patra <atishp@rivosinc.com> wrote:
> > >
> > > The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
> > > and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
> > > extension allows the perf to handle overflow interrupts and filtering
> > > support. This patch provides a framework for programmable
> > > counters to leverage the extension. As the extension doesn't have any
> > > provision for the overflow bit for fixed counters, the fixed events
> > > can also be monitoring using programmable counters. The underlying
> > > counters for cycle and instruction counters are always running. Thus,
> > > a separate timer device is programmed to handle the overflow.
> > >
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > >  target/riscv/cpu.c      |  11 ++
> > >  target/riscv/cpu.h      |  25 +++
> > >  target/riscv/cpu_bits.h |  55 +++++++
> > >  target/riscv/csr.c      | 156 ++++++++++++++++--
> > >  target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
> > >  target/riscv/pmu.h      |   7 +
> > >  6 files changed, 590 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index f63602828680..9715eed2fc4e 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -22,6 +22,7 @@
> > >  #include "qemu/ctype.h"
> > >  #include "qemu/log.h"
> > >  #include "cpu.h"
> > > +#include "pmu.h"
> > >  #include "internals.h"
> > >  #include "exec/exec-all.h"
> > >  #include "qapi/error.h"
> > > @@ -696,6 +697,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > >          set_misa(env, env->misa_mxl, ext);
> > >      }
> > >
> > > +#ifndef CONFIG_USER_ONLY
> > > +    if (cpu->cfg.pmu_num) {
> > > +        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> > > +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > +                                          riscv_pmu_timer_cb, cpu);
> > > +        }
> > > +     }
> > > +#endif
> > > +
> > >      riscv_cpu_register_gdb_regs_for_features(cs);
> > >
> > >      qemu_init_vcpu(cs);
> > > @@ -795,6 +805,7 @@ static Property riscv_cpu_properties[] = {
> > >      DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> > >      DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > +    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
> > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > >      DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 0fa15595fb37..a0e2279ea5e6 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -131,6 +131,8 @@ typedef struct PMUCTRState {
> > >      /* Snapshort value of a counter in RV32 */
> > >      target_ulong mhpmcounterh_prev;
> > >      bool started;
> > > +    /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > > +    target_ulong irq_overflow_left;
> > >  } PMUCTRState;
> > >
> > >  struct CPUArchState {
> > > @@ -291,6 +293,9 @@ struct CPUArchState {
> > >      /* PMU event selector configured values. First three are unused*/
> > >      target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
> > >
> > > +    /* PMU event selector configured values for RV32*/
> > > +    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> > > +
> > >      target_ulong sscratch;
> > >      target_ulong mscratch;
> > >
> > > @@ -413,6 +418,7 @@ struct RISCVCPUConfig {
> > >      bool ext_zhinxmin;
> > >      bool ext_zve32f;
> > >      bool ext_zve64f;
> > > +    bool ext_sscofpmf;
> > >
> > >      /* Vendor-specific custom extensions */
> > >      bool ext_XVentanaCondOps;
> > > @@ -452,6 +458,12 @@ struct ArchCPU {
> > >
> > >      /* Configuration Settings */
> > >      RISCVCPUConfig cfg;
> > > +
> > > +    QEMUTimer *pmu_timer;
> > > +    /* A bitmask of Available programmable counters */
> > > +    uint32_t pmu_avail_ctrs;
> > > +    /* Mapping of events to counters */
> > > +    GHashTable *pmu_event_ctr_map;
> > >  };
> > >
> > >  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> > > @@ -709,6 +721,19 @@ enum {
> > >      CSR_TABLE_SIZE = 0x1000
> > >  };
> > >
> > > +/**
> > > + * The event id are encoded based on the encoding specified in the
> > > + * SBI specification v0.3
> > > + */
> > > +
> > > +enum riscv_pmu_event_idx {
> > > +    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > > +    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > > +    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > > +    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > > +    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > > +};
> > > +
> > >  /* CSR function table */
> > >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> > >
> > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > index 48b39e6d52a7..d0b53e5ea072 100644
> > > --- a/target/riscv/cpu_bits.h
> > > +++ b/target/riscv/cpu_bits.h
> > > @@ -400,6 +400,37 @@
> > >  #define CSR_MHPMEVENT29     0x33d
> > >  #define CSR_MHPMEVENT30     0x33e
> > >  #define CSR_MHPMEVENT31     0x33f
> > > +
> > > +#define CSR_MHPMEVENT3H     0x723
> > > +#define CSR_MHPMEVENT4H     0x724
> > > +#define CSR_MHPMEVENT5H     0x725
> > > +#define CSR_MHPMEVENT6H     0x726
> > > +#define CSR_MHPMEVENT7H     0x727
> > > +#define CSR_MHPMEVENT8H     0x728
> > > +#define CSR_MHPMEVENT9H     0x729
> > > +#define CSR_MHPMEVENT10H    0x72a
> > > +#define CSR_MHPMEVENT11H    0x72b
> > > +#define CSR_MHPMEVENT12H    0x72c
> > > +#define CSR_MHPMEVENT13H    0x72d
> > > +#define CSR_MHPMEVENT14H    0x72e
> > > +#define CSR_MHPMEVENT15H    0x72f
> > > +#define CSR_MHPMEVENT16H    0x730
> > > +#define CSR_MHPMEVENT17H    0x731
> > > +#define CSR_MHPMEVENT18H    0x732
> > > +#define CSR_MHPMEVENT19H    0x733
> > > +#define CSR_MHPMEVENT20H    0x734
> > > +#define CSR_MHPMEVENT21H    0x735
> > > +#define CSR_MHPMEVENT22H    0x736
> > > +#define CSR_MHPMEVENT23H    0x737
> > > +#define CSR_MHPMEVENT24H    0x738
> > > +#define CSR_MHPMEVENT25H    0x739
> > > +#define CSR_MHPMEVENT26H    0x73a
> > > +#define CSR_MHPMEVENT27H    0x73b
> > > +#define CSR_MHPMEVENT28H    0x73c
> > > +#define CSR_MHPMEVENT29H    0x73d
> > > +#define CSR_MHPMEVENT30H    0x73e
> > > +#define CSR_MHPMEVENT31H    0x73f
> > > +
> > >  #define CSR_MHPMCOUNTER3H   0xb83
> > >  #define CSR_MHPMCOUNTER4H   0xb84
> > >  #define CSR_MHPMCOUNTER5H   0xb85
> > > @@ -461,6 +492,7 @@
> > >  #define CSR_VSMTE           0x2c0
> > >  #define CSR_VSPMMASK        0x2c1
> > >  #define CSR_VSPMBASE        0x2c2
> > > +#define CSR_SCOUNTOVF       0xda0
> > >
> > >  /* mstatus CSR bits */
> > >  #define MSTATUS_UIE         0x00000001
> > > @@ -635,6 +667,7 @@ typedef enum RISCVException {
> > >  #define IRQ_VS_EXT                         10
> > >  #define IRQ_M_EXT                          11
> > >  #define IRQ_S_GEXT                         12
> > > +#define IRQ_PMU_OVF                        13
> > >  #define IRQ_LOCAL_MAX                      16
> > >  #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
> > >
> > > @@ -652,11 +685,13 @@ typedef enum RISCVException {
> > >  #define MIP_VSEIP                          (1 << IRQ_VS_EXT)
> > >  #define MIP_MEIP                           (1 << IRQ_M_EXT)
> > >  #define MIP_SGEIP                          (1 << IRQ_S_GEXT)
> > > +#define MIP_LCOFIP                         (1 << IRQ_PMU_OVF)
> > >
> > >  /* sip masks */
> > >  #define SIP_SSIP                           MIP_SSIP
> > >  #define SIP_STIP                           MIP_STIP
> > >  #define SIP_SEIP                           MIP_SEIP
> > > +#define SIP_LCOFIP                         MIP_LCOFIP
> > >
> > >  /* MIE masks */
> > >  #define MIE_SEIE                           (1 << IRQ_S_EXT)
> > > @@ -804,4 +839,24 @@ typedef enum RISCVException {
> > >  #define HVICTL_VALID_MASK                  \
> > >      (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
> > >
> > > +/* PMU related bits */
> > > +#define MIE_LCOFIE                         (1 << IRQ_PMU_OVF)
> > > +
> > > +#define MHPMEVENT_BIT_OF                   BIT_ULL(63)
> > > +#define MHPMEVENTH_BIT_OF                  BIT(31)
> > > +#define MHPMEVENT_BIT_MINH                 BIT_ULL(62)
> > > +#define MHPMEVENTH_BIT_MINH                BIT(30)
> > > +#define MHPMEVENT_BIT_SINH                 BIT_ULL(61)
> > > +#define MHPMEVENTH_BIT_SINH                BIT(29)
> > > +#define MHPMEVENT_BIT_UINH                 BIT_ULL(60)
> > > +#define MHPMEVENTH_BIT_UINH                BIT(28)
> > > +#define MHPMEVENT_BIT_VSINH                BIT_ULL(59)
> > > +#define MHPMEVENTH_BIT_VSINH               BIT(27)
> > > +#define MHPMEVENT_BIT_VUINH                BIT_ULL(58)
> > > +#define MHPMEVENTH_BIT_VUINH               BIT(26)
> > > +
> > > +#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
> > > +#define MHPMEVENT_IDX_MASK                 0xFFFFF
> > > +#define MHPMEVENT_SSCOF_RESVD              16
> > > +
> > >  #endif
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index 04796b99d0fe..519d6377fd9f 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -72,7 +72,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > >      CPUState *cs = env_cpu(env);
> > >      RISCVCPU *cpu = RISCV_CPU(cs);
> > >      int ctr_index;
> > > -    int base_csrno = CSR_HPMCOUNTER3;
> > > +    int base_csrno = CSR_CYCLE;
> > >      bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> > >
> > >      if (rv32 && csrno >= CSR_CYCLEH) {
> > > @@ -81,11 +81,18 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > >      }
> > >      ctr_index = csrno - base_csrno;
> > >
> > > -    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> > > +    if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) ||
> > > +        (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) {
> > > +        goto skip_ext_pmu_check;
> > > +    }
> > > +
> > > +    if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) {
> > >          /* No counter is enabled in PMU or the counter is out of range */
> > >          return RISCV_EXCP_ILLEGAL_INST;
> > >      }
> > >
> > > +skip_ext_pmu_check:
> > > +
> > >      if (env->priv == PRV_S) {
> > >          switch (csrno) {
> > >          case CSR_CYCLE:
> > > @@ -104,7 +111,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > >              }
> > >              break;
> > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > -            ctr_index = csrno - CSR_CYCLE;
> > >              if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > >                  return RISCV_EXCP_ILLEGAL_INST;
> > >              }
> > > @@ -128,7 +134,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > >                  }
> > >                  break;
> > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > -                ctr_index = csrno - CSR_CYCLEH;
> > >                  if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > >                      return RISCV_EXCP_ILLEGAL_INST;
> > >                  }
> > > @@ -158,7 +163,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > >              }
> > >              break;
> > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > -            ctr_index = csrno - CSR_CYCLE;
> > >              if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > >                   get_field(env->mcounteren, 1 << ctr_index)) {
> > >                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > @@ -186,7 +190,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > >                  }
> > >                  break;
> > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > -                ctr_index = csrno - CSR_CYCLEH;
> > >                  if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > >                       get_field(env->mcounteren, 1 << ctr_index)) {
> > >                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > @@ -238,6 +241,18 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
> > >      return mctr(env, csrno);
> > >  }
> > >
> > > +static RISCVException sscofpmf(CPURISCVState *env, int csrno)
> > > +{
> > > +    CPUState *cs = env_cpu(env);
> > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > +
> > > +    if (!cpu->cfg.ext_sscofpmf) {
> > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > +    }
> > > +
> > > +    return RISCV_EXCP_NONE;
> > > +}
> > > +
> > >  static RISCVException any(CPURISCVState *env, int csrno)
> > >  {
> > >      return RISCV_EXCP_NONE;
> > > @@ -622,11 +637,36 @@ static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong val)
> > >  {
> > >      int evt_index = csrno - CSR_MCOUNTINHIBIT;
> > >
> > > +    if (riscv_cpu_mxl(env) != MXL_RV32) {
> >
> > Is this right? What if the guest writes the high bytes first?
> >
>
> Most of the current software is implemented with lower bytes first.
> But I understand your concern
> that it doesn't provide any guarantee. We probably can have another
> field that can track the order of the updates.
> riscv_pmu_update_event_map will be only called when both lower/upper
> half is complete.

Why can't you just update it on every write? Guest software should be
setting the high bits to 0xFFFF_FFFF first to avoid any issues

Alistair

>
>
> > Alistair

Atish Patra April 26, 2022, 9:33 p.m. UTC | #4

On Mon, Apr 18, 2022 at 3:46 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Apr 16, 2022 at 9:54 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >
> > On Wed, Apr 13, 2022 at 12:08 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Thu, Mar 31, 2022 at 10:19 AM Atish Patra <atishp@rivosinc.com> wrote:
> > > >
> > > > The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
> > > > and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
> > > > extension allows the perf to handle overflow interrupts and filtering
> > > > support. This patch provides a framework for programmable
> > > > counters to leverage the extension. As the extension doesn't have any
> > > > provision for the overflow bit for fixed counters, the fixed events
> > > > can also be monitoring using programmable counters. The underlying
> > > > counters for cycle and instruction counters are always running. Thus,
> > > > a separate timer device is programmed to handle the overflow.
> > > >
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > ---
> > > >  target/riscv/cpu.c      |  11 ++
> > > >  target/riscv/cpu.h      |  25 +++
> > > >  target/riscv/cpu_bits.h |  55 +++++++
> > > >  target/riscv/csr.c      | 156 ++++++++++++++++--
> > > >  target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
> > > >  target/riscv/pmu.h      |   7 +
> > > >  6 files changed, 590 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index f63602828680..9715eed2fc4e 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -22,6 +22,7 @@
> > > >  #include "qemu/ctype.h"
> > > >  #include "qemu/log.h"
> > > >  #include "cpu.h"
> > > > +#include "pmu.h"
> > > >  #include "internals.h"
> > > >  #include "exec/exec-all.h"
> > > >  #include "qapi/error.h"
> > > > @@ -696,6 +697,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > >          set_misa(env, env->misa_mxl, ext);
> > > >      }
> > > >
> > > > +#ifndef CONFIG_USER_ONLY
> > > > +    if (cpu->cfg.pmu_num) {
> > > > +        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> > > > +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > > +                                          riscv_pmu_timer_cb, cpu);
> > > > +        }
> > > > +     }
> > > > +#endif
> > > > +
> > > >      riscv_cpu_register_gdb_regs_for_features(cs);
> > > >
> > > >      qemu_init_vcpu(cs);
> > > > @@ -795,6 +805,7 @@ static Property riscv_cpu_properties[] = {
> > > >      DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> > > >      DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > +    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
> > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > >      DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > index 0fa15595fb37..a0e2279ea5e6 100644
> > > > --- a/target/riscv/cpu.h
> > > > +++ b/target/riscv/cpu.h
> > > > @@ -131,6 +131,8 @@ typedef struct PMUCTRState {
> > > >      /* Snapshort value of a counter in RV32 */
> > > >      target_ulong mhpmcounterh_prev;
> > > >      bool started;
> > > > +    /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > > > +    target_ulong irq_overflow_left;
> > > >  } PMUCTRState;
> > > >
> > > >  struct CPUArchState {
> > > > @@ -291,6 +293,9 @@ struct CPUArchState {
> > > >      /* PMU event selector configured values. First three are unused*/
> > > >      target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
> > > >
> > > > +    /* PMU event selector configured values for RV32*/
> > > > +    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> > > > +
> > > >      target_ulong sscratch;
> > > >      target_ulong mscratch;
> > > >
> > > > @@ -413,6 +418,7 @@ struct RISCVCPUConfig {
> > > >      bool ext_zhinxmin;
> > > >      bool ext_zve32f;
> > > >      bool ext_zve64f;
> > > > +    bool ext_sscofpmf;
> > > >
> > > >      /* Vendor-specific custom extensions */
> > > >      bool ext_XVentanaCondOps;
> > > > @@ -452,6 +458,12 @@ struct ArchCPU {
> > > >
> > > >      /* Configuration Settings */
> > > >      RISCVCPUConfig cfg;
> > > > +
> > > > +    QEMUTimer *pmu_timer;
> > > > +    /* A bitmask of Available programmable counters */
> > > > +    uint32_t pmu_avail_ctrs;
> > > > +    /* Mapping of events to counters */
> > > > +    GHashTable *pmu_event_ctr_map;
> > > >  };
> > > >
> > > >  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> > > > @@ -709,6 +721,19 @@ enum {
> > > >      CSR_TABLE_SIZE = 0x1000
> > > >  };
> > > >
> > > > +/**
> > > > + * The event id are encoded based on the encoding specified in the
> > > > + * SBI specification v0.3
> > > > + */
> > > > +
> > > > +enum riscv_pmu_event_idx {
> > > > +    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > > > +    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > > > +    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > > > +    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > > > +    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > > > +};
> > > > +
> > > >  /* CSR function table */
> > > >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> > > >
> > > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > > index 48b39e6d52a7..d0b53e5ea072 100644
> > > > --- a/target/riscv/cpu_bits.h
> > > > +++ b/target/riscv/cpu_bits.h
> > > > @@ -400,6 +400,37 @@
> > > >  #define CSR_MHPMEVENT29     0x33d
> > > >  #define CSR_MHPMEVENT30     0x33e
> > > >  #define CSR_MHPMEVENT31     0x33f
> > > > +
> > > > +#define CSR_MHPMEVENT3H     0x723
> > > > +#define CSR_MHPMEVENT4H     0x724
> > > > +#define CSR_MHPMEVENT5H     0x725
> > > > +#define CSR_MHPMEVENT6H     0x726
> > > > +#define CSR_MHPMEVENT7H     0x727
> > > > +#define CSR_MHPMEVENT8H     0x728
> > > > +#define CSR_MHPMEVENT9H     0x729
> > > > +#define CSR_MHPMEVENT10H    0x72a
> > > > +#define CSR_MHPMEVENT11H    0x72b
> > > > +#define CSR_MHPMEVENT12H    0x72c
> > > > +#define CSR_MHPMEVENT13H    0x72d
> > > > +#define CSR_MHPMEVENT14H    0x72e
> > > > +#define CSR_MHPMEVENT15H    0x72f
> > > > +#define CSR_MHPMEVENT16H    0x730
> > > > +#define CSR_MHPMEVENT17H    0x731
> > > > +#define CSR_MHPMEVENT18H    0x732
> > > > +#define CSR_MHPMEVENT19H    0x733
> > > > +#define CSR_MHPMEVENT20H    0x734
> > > > +#define CSR_MHPMEVENT21H    0x735
> > > > +#define CSR_MHPMEVENT22H    0x736
> > > > +#define CSR_MHPMEVENT23H    0x737
> > > > +#define CSR_MHPMEVENT24H    0x738
> > > > +#define CSR_MHPMEVENT25H    0x739
> > > > +#define CSR_MHPMEVENT26H    0x73a
> > > > +#define CSR_MHPMEVENT27H    0x73b
> > > > +#define CSR_MHPMEVENT28H    0x73c
> > > > +#define CSR_MHPMEVENT29H    0x73d
> > > > +#define CSR_MHPMEVENT30H    0x73e
> > > > +#define CSR_MHPMEVENT31H    0x73f
> > > > +
> > > >  #define CSR_MHPMCOUNTER3H   0xb83
> > > >  #define CSR_MHPMCOUNTER4H   0xb84
> > > >  #define CSR_MHPMCOUNTER5H   0xb85
> > > > @@ -461,6 +492,7 @@
> > > >  #define CSR_VSMTE           0x2c0
> > > >  #define CSR_VSPMMASK        0x2c1
> > > >  #define CSR_VSPMBASE        0x2c2
> > > > +#define CSR_SCOUNTOVF       0xda0
> > > >
> > > >  /* mstatus CSR bits */
> > > >  #define MSTATUS_UIE         0x00000001
> > > > @@ -635,6 +667,7 @@ typedef enum RISCVException {
> > > >  #define IRQ_VS_EXT                         10
> > > >  #define IRQ_M_EXT                          11
> > > >  #define IRQ_S_GEXT                         12
> > > > +#define IRQ_PMU_OVF                        13
> > > >  #define IRQ_LOCAL_MAX                      16
> > > >  #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
> > > >
> > > > @@ -652,11 +685,13 @@ typedef enum RISCVException {
> > > >  #define MIP_VSEIP                          (1 << IRQ_VS_EXT)
> > > >  #define MIP_MEIP                           (1 << IRQ_M_EXT)
> > > >  #define MIP_SGEIP                          (1 << IRQ_S_GEXT)
> > > > +#define MIP_LCOFIP                         (1 << IRQ_PMU_OVF)
> > > >
> > > >  /* sip masks */
> > > >  #define SIP_SSIP                           MIP_SSIP
> > > >  #define SIP_STIP                           MIP_STIP
> > > >  #define SIP_SEIP                           MIP_SEIP
> > > > +#define SIP_LCOFIP                         MIP_LCOFIP
> > > >
> > > >  /* MIE masks */
> > > >  #define MIE_SEIE                           (1 << IRQ_S_EXT)
> > > > @@ -804,4 +839,24 @@ typedef enum RISCVException {
> > > >  #define HVICTL_VALID_MASK                  \
> > > >      (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
> > > >
> > > > +/* PMU related bits */
> > > > +#define MIE_LCOFIE                         (1 << IRQ_PMU_OVF)
> > > > +
> > > > +#define MHPMEVENT_BIT_OF                   BIT_ULL(63)
> > > > +#define MHPMEVENTH_BIT_OF                  BIT(31)
> > > > +#define MHPMEVENT_BIT_MINH                 BIT_ULL(62)
> > > > +#define MHPMEVENTH_BIT_MINH                BIT(30)
> > > > +#define MHPMEVENT_BIT_SINH                 BIT_ULL(61)
> > > > +#define MHPMEVENTH_BIT_SINH                BIT(29)
> > > > +#define MHPMEVENT_BIT_UINH                 BIT_ULL(60)
> > > > +#define MHPMEVENTH_BIT_UINH                BIT(28)
> > > > +#define MHPMEVENT_BIT_VSINH                BIT_ULL(59)
> > > > +#define MHPMEVENTH_BIT_VSINH               BIT(27)
> > > > +#define MHPMEVENT_BIT_VUINH                BIT_ULL(58)
> > > > +#define MHPMEVENTH_BIT_VUINH               BIT(26)
> > > > +
> > > > +#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
> > > > +#define MHPMEVENT_IDX_MASK                 0xFFFFF
> > > > +#define MHPMEVENT_SSCOF_RESVD              16
> > > > +
> > > >  #endif
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > index 04796b99d0fe..519d6377fd9f 100644
> > > > --- a/target/riscv/csr.c
> > > > +++ b/target/riscv/csr.c
> > > > @@ -72,7 +72,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > >      CPUState *cs = env_cpu(env);
> > > >      RISCVCPU *cpu = RISCV_CPU(cs);
> > > >      int ctr_index;
> > > > -    int base_csrno = CSR_HPMCOUNTER3;
> > > > +    int base_csrno = CSR_CYCLE;
> > > >      bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> > > >
> > > >      if (rv32 && csrno >= CSR_CYCLEH) {
> > > > @@ -81,11 +81,18 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > >      }
> > > >      ctr_index = csrno - base_csrno;
> > > >
> > > > -    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> > > > +    if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) ||
> > > > +        (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) {
> > > > +        goto skip_ext_pmu_check;
> > > > +    }
> > > > +
> > > > +    if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) {
> > > >          /* No counter is enabled in PMU or the counter is out of range */
> > > >          return RISCV_EXCP_ILLEGAL_INST;
> > > >      }
> > > >
> > > > +skip_ext_pmu_check:
> > > > +
> > > >      if (env->priv == PRV_S) {
> > > >          switch (csrno) {
> > > >          case CSR_CYCLE:
> > > > @@ -104,7 +111,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > >              }
> > > >              break;
> > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > -            ctr_index = csrno - CSR_CYCLE;
> > > >              if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > >                  return RISCV_EXCP_ILLEGAL_INST;
> > > >              }
> > > > @@ -128,7 +134,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > >                  }
> > > >                  break;
> > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > >                  if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > >                      return RISCV_EXCP_ILLEGAL_INST;
> > > >                  }
> > > > @@ -158,7 +163,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > >              }
> > > >              break;
> > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > -            ctr_index = csrno - CSR_CYCLE;
> > > >              if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > >                   get_field(env->mcounteren, 1 << ctr_index)) {
> > > >                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > @@ -186,7 +190,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > >                  }
> > > >                  break;
> > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > >                  if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > >                       get_field(env->mcounteren, 1 << ctr_index)) {
> > > >                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > @@ -238,6 +241,18 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
> > > >      return mctr(env, csrno);
> > > >  }
> > > >
> > > > +static RISCVException sscofpmf(CPURISCVState *env, int csrno)
> > > > +{
> > > > +    CPUState *cs = env_cpu(env);
> > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > +
> > > > +    if (!cpu->cfg.ext_sscofpmf) {
> > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > +    }
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > >  static RISCVException any(CPURISCVState *env, int csrno)
> > > >  {
> > > >      return RISCV_EXCP_NONE;
> > > > @@ -622,11 +637,36 @@ static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong val)
> > > >  {
> > > >      int evt_index = csrno - CSR_MCOUNTINHIBIT;
> > > >
> > > > +    if (riscv_cpu_mxl(env) != MXL_RV32) {
> > >
> > > Is this right? What if the guest writes the high bytes first?
> > >
> >
> > Most of the current software is implemented with lower bytes first.
> > But I understand your concern
> > that it doesn't provide any guarantee. We probably can have another
> > field that can track the order of the updates.
> > riscv_pmu_update_event_map will be only called when both lower/upper
> > half is complete.
>
> Why can't you just update it on every write? Guest software should be

riscv_pmu_update_event_map maintains a hashmap between counter and event ID.
Updating at every write is unnecessary as it will do the hashmap
lookup and return fail
for high bytes write.

The events encoded as the SBI PMU spec will always have the event id
in lower 20 bits.
Technically, it is okay to just call riscv_pmu_update_event_map in
write_mhpmevent not in
write_mhpmeventh for rv32 as well.

However, I want to keep riscv/pmu.c as generic as possible to allow
future implementations
to have different event ID value > UINT32_MAX.

Let me know if you like to keep it simple and just update
riscv_pmu_update_event_map in lower bits
update right now.

> setting the high bits to 0xFFFF_FFFF first to avoid any issues
>
> Alistair
>
> >
> >
> > > Alistair
>

Alistair Francis May 4, 2022, 10:03 a.m. UTC | #5

On Wed, Apr 27, 2022 at 7:33 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Mon, Apr 18, 2022 at 3:46 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Sat, Apr 16, 2022 at 9:54 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> > >
> > > On Wed, Apr 13, 2022 at 12:08 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Thu, Mar 31, 2022 at 10:19 AM Atish Patra <atishp@rivosinc.com> wrote:
> > > > >
> > > > > The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
> > > > > and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
> > > > > extension allows the perf to handle overflow interrupts and filtering
> > > > > support. This patch provides a framework for programmable
> > > > > counters to leverage the extension. As the extension doesn't have any
> > > > > provision for the overflow bit for fixed counters, the fixed events
> > > > > can also be monitoring using programmable counters. The underlying
> > > > > counters for cycle and instruction counters are always running. Thus,
> > > > > a separate timer device is programmed to handle the overflow.
> > > > >
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > ---
> > > > >  target/riscv/cpu.c      |  11 ++
> > > > >  target/riscv/cpu.h      |  25 +++
> > > > >  target/riscv/cpu_bits.h |  55 +++++++
> > > > >  target/riscv/csr.c      | 156 ++++++++++++++++--
> > > > >  target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
> > > > >  target/riscv/pmu.h      |   7 +
> > > > >  6 files changed, 590 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > index f63602828680..9715eed2fc4e 100644
> > > > > --- a/target/riscv/cpu.c
> > > > > +++ b/target/riscv/cpu.c
> > > > > @@ -22,6 +22,7 @@
> > > > >  #include "qemu/ctype.h"
> > > > >  #include "qemu/log.h"
> > > > >  #include "cpu.h"
> > > > > +#include "pmu.h"
> > > > >  #include "internals.h"
> > > > >  #include "exec/exec-all.h"
> > > > >  #include "qapi/error.h"
> > > > > @@ -696,6 +697,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > >          set_misa(env, env->misa_mxl, ext);
> > > > >      }
> > > > >
> > > > > +#ifndef CONFIG_USER_ONLY
> > > > > +    if (cpu->cfg.pmu_num) {
> > > > > +        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> > > > > +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > > > +                                          riscv_pmu_timer_cb, cpu);
> > > > > +        }
> > > > > +     }
> > > > > +#endif
> > > > > +
> > > > >      riscv_cpu_register_gdb_regs_for_features(cs);
> > > > >
> > > > >      qemu_init_vcpu(cs);
> > > > > @@ -795,6 +805,7 @@ static Property riscv_cpu_properties[] = {
> > > > >      DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> > > > >      DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> > > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > > +    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
> > > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > > >      DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > > index 0fa15595fb37..a0e2279ea5e6 100644
> > > > > --- a/target/riscv/cpu.h
> > > > > +++ b/target/riscv/cpu.h
> > > > > @@ -131,6 +131,8 @@ typedef struct PMUCTRState {
> > > > >      /* Snapshort value of a counter in RV32 */
> > > > >      target_ulong mhpmcounterh_prev;
> > > > >      bool started;
> > > > > +    /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > > > > +    target_ulong irq_overflow_left;
> > > > >  } PMUCTRState;
> > > > >
> > > > >  struct CPUArchState {
> > > > > @@ -291,6 +293,9 @@ struct CPUArchState {
> > > > >      /* PMU event selector configured values. First three are unused*/
> > > > >      target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
> > > > >
> > > > > +    /* PMU event selector configured values for RV32*/
> > > > > +    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> > > > > +
> > > > >      target_ulong sscratch;
> > > > >      target_ulong mscratch;
> > > > >
> > > > > @@ -413,6 +418,7 @@ struct RISCVCPUConfig {
> > > > >      bool ext_zhinxmin;
> > > > >      bool ext_zve32f;
> > > > >      bool ext_zve64f;
> > > > > +    bool ext_sscofpmf;
> > > > >
> > > > >      /* Vendor-specific custom extensions */
> > > > >      bool ext_XVentanaCondOps;
> > > > > @@ -452,6 +458,12 @@ struct ArchCPU {
> > > > >
> > > > >      /* Configuration Settings */
> > > > >      RISCVCPUConfig cfg;
> > > > > +
> > > > > +    QEMUTimer *pmu_timer;
> > > > > +    /* A bitmask of Available programmable counters */
> > > > > +    uint32_t pmu_avail_ctrs;
> > > > > +    /* Mapping of events to counters */
> > > > > +    GHashTable *pmu_event_ctr_map;
> > > > >  };
> > > > >
> > > > >  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> > > > > @@ -709,6 +721,19 @@ enum {
> > > > >      CSR_TABLE_SIZE = 0x1000
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * The event id are encoded based on the encoding specified in the
> > > > > + * SBI specification v0.3
> > > > > + */
> > > > > +
> > > > > +enum riscv_pmu_event_idx {
> > > > > +    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > > > > +    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > > > > +    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > > > > +    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > > > > +    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > > > > +};
> > > > > +
> > > > >  /* CSR function table */
> > > > >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> > > > >
> > > > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > > > index 48b39e6d52a7..d0b53e5ea072 100644
> > > > > --- a/target/riscv/cpu_bits.h
> > > > > +++ b/target/riscv/cpu_bits.h
> > > > > @@ -400,6 +400,37 @@
> > > > >  #define CSR_MHPMEVENT29     0x33d
> > > > >  #define CSR_MHPMEVENT30     0x33e
> > > > >  #define CSR_MHPMEVENT31     0x33f
> > > > > +
> > > > > +#define CSR_MHPMEVENT3H     0x723
> > > > > +#define CSR_MHPMEVENT4H     0x724
> > > > > +#define CSR_MHPMEVENT5H     0x725
> > > > > +#define CSR_MHPMEVENT6H     0x726
> > > > > +#define CSR_MHPMEVENT7H     0x727
> > > > > +#define CSR_MHPMEVENT8H     0x728
> > > > > +#define CSR_MHPMEVENT9H     0x729
> > > > > +#define CSR_MHPMEVENT10H    0x72a
> > > > > +#define CSR_MHPMEVENT11H    0x72b
> > > > > +#define CSR_MHPMEVENT12H    0x72c
> > > > > +#define CSR_MHPMEVENT13H    0x72d
> > > > > +#define CSR_MHPMEVENT14H    0x72e
> > > > > +#define CSR_MHPMEVENT15H    0x72f
> > > > > +#define CSR_MHPMEVENT16H    0x730
> > > > > +#define CSR_MHPMEVENT17H    0x731
> > > > > +#define CSR_MHPMEVENT18H    0x732
> > > > > +#define CSR_MHPMEVENT19H    0x733
> > > > > +#define CSR_MHPMEVENT20H    0x734
> > > > > +#define CSR_MHPMEVENT21H    0x735
> > > > > +#define CSR_MHPMEVENT22H    0x736
> > > > > +#define CSR_MHPMEVENT23H    0x737
> > > > > +#define CSR_MHPMEVENT24H    0x738
> > > > > +#define CSR_MHPMEVENT25H    0x739
> > > > > +#define CSR_MHPMEVENT26H    0x73a
> > > > > +#define CSR_MHPMEVENT27H    0x73b
> > > > > +#define CSR_MHPMEVENT28H    0x73c
> > > > > +#define CSR_MHPMEVENT29H    0x73d
> > > > > +#define CSR_MHPMEVENT30H    0x73e
> > > > > +#define CSR_MHPMEVENT31H    0x73f
> > > > > +
> > > > >  #define CSR_MHPMCOUNTER3H   0xb83
> > > > >  #define CSR_MHPMCOUNTER4H   0xb84
> > > > >  #define CSR_MHPMCOUNTER5H   0xb85
> > > > > @@ -461,6 +492,7 @@
> > > > >  #define CSR_VSMTE           0x2c0
> > > > >  #define CSR_VSPMMASK        0x2c1
> > > > >  #define CSR_VSPMBASE        0x2c2
> > > > > +#define CSR_SCOUNTOVF       0xda0
> > > > >
> > > > >  /* mstatus CSR bits */
> > > > >  #define MSTATUS_UIE         0x00000001
> > > > > @@ -635,6 +667,7 @@ typedef enum RISCVException {
> > > > >  #define IRQ_VS_EXT                         10
> > > > >  #define IRQ_M_EXT                          11
> > > > >  #define IRQ_S_GEXT                         12
> > > > > +#define IRQ_PMU_OVF                        13
> > > > >  #define IRQ_LOCAL_MAX                      16
> > > > >  #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
> > > > >
> > > > > @@ -652,11 +685,13 @@ typedef enum RISCVException {
> > > > >  #define MIP_VSEIP                          (1 << IRQ_VS_EXT)
> > > > >  #define MIP_MEIP                           (1 << IRQ_M_EXT)
> > > > >  #define MIP_SGEIP                          (1 << IRQ_S_GEXT)
> > > > > +#define MIP_LCOFIP                         (1 << IRQ_PMU_OVF)
> > > > >
> > > > >  /* sip masks */
> > > > >  #define SIP_SSIP                           MIP_SSIP
> > > > >  #define SIP_STIP                           MIP_STIP
> > > > >  #define SIP_SEIP                           MIP_SEIP
> > > > > +#define SIP_LCOFIP                         MIP_LCOFIP
> > > > >
> > > > >  /* MIE masks */
> > > > >  #define MIE_SEIE                           (1 << IRQ_S_EXT)
> > > > > @@ -804,4 +839,24 @@ typedef enum RISCVException {
> > > > >  #define HVICTL_VALID_MASK                  \
> > > > >      (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
> > > > >
> > > > > +/* PMU related bits */
> > > > > +#define MIE_LCOFIE                         (1 << IRQ_PMU_OVF)
> > > > > +
> > > > > +#define MHPMEVENT_BIT_OF                   BIT_ULL(63)
> > > > > +#define MHPMEVENTH_BIT_OF                  BIT(31)
> > > > > +#define MHPMEVENT_BIT_MINH                 BIT_ULL(62)
> > > > > +#define MHPMEVENTH_BIT_MINH                BIT(30)
> > > > > +#define MHPMEVENT_BIT_SINH                 BIT_ULL(61)
> > > > > +#define MHPMEVENTH_BIT_SINH                BIT(29)
> > > > > +#define MHPMEVENT_BIT_UINH                 BIT_ULL(60)
> > > > > +#define MHPMEVENTH_BIT_UINH                BIT(28)
> > > > > +#define MHPMEVENT_BIT_VSINH                BIT_ULL(59)
> > > > > +#define MHPMEVENTH_BIT_VSINH               BIT(27)
> > > > > +#define MHPMEVENT_BIT_VUINH                BIT_ULL(58)
> > > > > +#define MHPMEVENTH_BIT_VUINH               BIT(26)
> > > > > +
> > > > > +#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
> > > > > +#define MHPMEVENT_IDX_MASK                 0xFFFFF
> > > > > +#define MHPMEVENT_SSCOF_RESVD              16
> > > > > +
> > > > >  #endif
> > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > > index 04796b99d0fe..519d6377fd9f 100644
> > > > > --- a/target/riscv/csr.c
> > > > > +++ b/target/riscv/csr.c
> > > > > @@ -72,7 +72,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > >      CPUState *cs = env_cpu(env);
> > > > >      RISCVCPU *cpu = RISCV_CPU(cs);
> > > > >      int ctr_index;
> > > > > -    int base_csrno = CSR_HPMCOUNTER3;
> > > > > +    int base_csrno = CSR_CYCLE;
> > > > >      bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> > > > >
> > > > >      if (rv32 && csrno >= CSR_CYCLEH) {
> > > > > @@ -81,11 +81,18 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > >      }
> > > > >      ctr_index = csrno - base_csrno;
> > > > >
> > > > > -    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> > > > > +    if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) ||
> > > > > +        (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) {
> > > > > +        goto skip_ext_pmu_check;
> > > > > +    }
> > > > > +
> > > > > +    if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) {
> > > > >          /* No counter is enabled in PMU or the counter is out of range */
> > > > >          return RISCV_EXCP_ILLEGAL_INST;
> > > > >      }
> > > > >
> > > > > +skip_ext_pmu_check:
> > > > > +
> > > > >      if (env->priv == PRV_S) {
> > > > >          switch (csrno) {
> > > > >          case CSR_CYCLE:
> > > > > @@ -104,7 +111,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > >              }
> > > > >              break;
> > > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > > -            ctr_index = csrno - CSR_CYCLE;
> > > > >              if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > > >                  return RISCV_EXCP_ILLEGAL_INST;
> > > > >              }
> > > > > @@ -128,7 +134,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > >                  }
> > > > >                  break;
> > > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > > >                  if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > > >                      return RISCV_EXCP_ILLEGAL_INST;
> > > > >                  }
> > > > > @@ -158,7 +163,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > >              }
> > > > >              break;
> > > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > > -            ctr_index = csrno - CSR_CYCLE;
> > > > >              if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > > >                   get_field(env->mcounteren, 1 << ctr_index)) {
> > > > >                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > > @@ -186,7 +190,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > >                  }
> > > > >                  break;
> > > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > > >                  if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > > >                       get_field(env->mcounteren, 1 << ctr_index)) {
> > > > >                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > > @@ -238,6 +241,18 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
> > > > >      return mctr(env, csrno);
> > > > >  }
> > > > >
> > > > > +static RISCVException sscofpmf(CPURISCVState *env, int csrno)
> > > > > +{
> > > > > +    CPUState *cs = env_cpu(env);
> > > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > > +
> > > > > +    if (!cpu->cfg.ext_sscofpmf) {
> > > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > > +    }
> > > > > +
> > > > > +    return RISCV_EXCP_NONE;
> > > > > +}
> > > > > +
> > > > >  static RISCVException any(CPURISCVState *env, int csrno)
> > > > >  {
> > > > >      return RISCV_EXCP_NONE;
> > > > > @@ -622,11 +637,36 @@ static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong val)
> > > > >  {
> > > > >      int evt_index = csrno - CSR_MCOUNTINHIBIT;
> > > > >
> > > > > +    if (riscv_cpu_mxl(env) != MXL_RV32) {
> > > >
> > > > Is this right? What if the guest writes the high bytes first?
> > > >
> > >
> > > Most of the current software is implemented with lower bytes first.
> > > But I understand your concern
> > > that it doesn't provide any guarantee. We probably can have another
> > > field that can track the order of the updates.
> > > riscv_pmu_update_event_map will be only called when both lower/upper
> > > half is complete.
> >
> > Why can't you just update it on every write? Guest software should be
>
> riscv_pmu_update_event_map maintains a hashmap between counter and event ID.
> Updating at every write is unnecessary as it will do the hashmap
> lookup and return fail
> for high bytes write.
>
> The events encoded as the SBI PMU spec will always have the event id
> in lower 20 bits.
> Technically, it is okay to just call riscv_pmu_update_event_map in
> write_mhpmevent not in
> write_mhpmeventh for rv32 as well.
>
> However, I want to keep riscv/pmu.c as generic as possible to allow
> future implementations
> to have different event ID value > UINT32_MAX.

I agree. If there is a high CSR for 32-bit we need to support 64-bit values.

>
> Let me know if you like to keep it simple and just update
> riscv_pmu_update_event_map in lower bits
> update right now.

We need to handle both writer ordres. Unless the spec states that
writes must occur in a certain order we can't assume that they will.
Otherwise this is just a bug waiting to be discovered

Alitair

>
> > setting the high bits to 0xFFFF_FFFF first to avoid any issues
> >
> > Alistair
> >
> > >
> > >
> > > > Alistair
> >
>
>
> --
> Regards,
> Atish

Alistair Francis May 4, 2022, 10:06 a.m. UTC | #6

On Wed, May 4, 2022 at 8:03 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Apr 27, 2022 at 7:33 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Mon, Apr 18, 2022 at 3:46 PM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Sat, Apr 16, 2022 at 9:54 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> > > >
> > > > On Wed, Apr 13, 2022 at 12:08 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > > >
> > > > > On Thu, Mar 31, 2022 at 10:19 AM Atish Patra <atishp@rivosinc.com> wrote:
> > > > > >
> > > > > > The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
> > > > > > and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
> > > > > > extension allows the perf to handle overflow interrupts and filtering
> > > > > > support. This patch provides a framework for programmable
> > > > > > counters to leverage the extension. As the extension doesn't have any
> > > > > > provision for the overflow bit for fixed counters, the fixed events
> > > > > > can also be monitoring using programmable counters. The underlying
> > > > > > counters for cycle and instruction counters are always running. Thus,
> > > > > > a separate timer device is programmed to handle the overflow.
> > > > > >
> > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > ---
> > > > > >  target/riscv/cpu.c      |  11 ++
> > > > > >  target/riscv/cpu.h      |  25 +++
> > > > > >  target/riscv/cpu_bits.h |  55 +++++++
> > > > > >  target/riscv/csr.c      | 156 ++++++++++++++++--
> > > > > >  target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
> > > > > >  target/riscv/pmu.h      |   7 +
> > > > > >  6 files changed, 590 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > > index f63602828680..9715eed2fc4e 100644
> > > > > > --- a/target/riscv/cpu.c
> > > > > > +++ b/target/riscv/cpu.c
> > > > > > @@ -22,6 +22,7 @@
> > > > > >  #include "qemu/ctype.h"
> > > > > >  #include "qemu/log.h"
> > > > > >  #include "cpu.h"
> > > > > > +#include "pmu.h"
> > > > > >  #include "internals.h"
> > > > > >  #include "exec/exec-all.h"
> > > > > >  #include "qapi/error.h"
> > > > > > @@ -696,6 +697,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > > >          set_misa(env, env->misa_mxl, ext);
> > > > > >      }
> > > > > >
> > > > > > +#ifndef CONFIG_USER_ONLY
> > > > > > +    if (cpu->cfg.pmu_num) {
> > > > > > +        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> > > > > > +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > > > > +                                          riscv_pmu_timer_cb, cpu);
> > > > > > +        }
> > > > > > +     }
> > > > > > +#endif
> > > > > > +
> > > > > >      riscv_cpu_register_gdb_regs_for_features(cs);
> > > > > >
> > > > > >      qemu_init_vcpu(cs);
> > > > > > @@ -795,6 +805,7 @@ static Property riscv_cpu_properties[] = {
> > > > > >      DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> > > > > >      DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> > > > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > > > +    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
> > > > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > > > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > > > >      DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> > > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > > > index 0fa15595fb37..a0e2279ea5e6 100644
> > > > > > --- a/target/riscv/cpu.h
> > > > > > +++ b/target/riscv/cpu.h
> > > > > > @@ -131,6 +131,8 @@ typedef struct PMUCTRState {
> > > > > >      /* Snapshort value of a counter in RV32 */
> > > > > >      target_ulong mhpmcounterh_prev;
> > > > > >      bool started;
> > > > > > +    /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > > > > > +    target_ulong irq_overflow_left;
> > > > > >  } PMUCTRState;
> > > > > >
> > > > > >  struct CPUArchState {
> > > > > > @@ -291,6 +293,9 @@ struct CPUArchState {
> > > > > >      /* PMU event selector configured values. First three are unused*/
> > > > > >      target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
> > > > > >
> > > > > > +    /* PMU event selector configured values for RV32*/
> > > > > > +    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> > > > > > +
> > > > > >      target_ulong sscratch;
> > > > > >      target_ulong mscratch;
> > > > > >
> > > > > > @@ -413,6 +418,7 @@ struct RISCVCPUConfig {
> > > > > >      bool ext_zhinxmin;
> > > > > >      bool ext_zve32f;
> > > > > >      bool ext_zve64f;
> > > > > > +    bool ext_sscofpmf;
> > > > > >
> > > > > >      /* Vendor-specific custom extensions */
> > > > > >      bool ext_XVentanaCondOps;
> > > > > > @@ -452,6 +458,12 @@ struct ArchCPU {
> > > > > >
> > > > > >      /* Configuration Settings */
> > > > > >      RISCVCPUConfig cfg;
> > > > > > +
> > > > > > +    QEMUTimer *pmu_timer;
> > > > > > +    /* A bitmask of Available programmable counters */
> > > > > > +    uint32_t pmu_avail_ctrs;
> > > > > > +    /* Mapping of events to counters */
> > > > > > +    GHashTable *pmu_event_ctr_map;
> > > > > >  };
> > > > > >
> > > > > >  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> > > > > > @@ -709,6 +721,19 @@ enum {
> > > > > >      CSR_TABLE_SIZE = 0x1000
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * The event id are encoded based on the encoding specified in the
> > > > > > + * SBI specification v0.3
> > > > > > + */
> > > > > > +
> > > > > > +enum riscv_pmu_event_idx {
> > > > > > +    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > > > > > +    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > > > > > +    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > > > > > +    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > > > > > +    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > > > > > +};
> > > > > > +
> > > > > >  /* CSR function table */
> > > > > >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> > > > > >
> > > > > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > > > > index 48b39e6d52a7..d0b53e5ea072 100644
> > > > > > --- a/target/riscv/cpu_bits.h
> > > > > > +++ b/target/riscv/cpu_bits.h
> > > > > > @@ -400,6 +400,37 @@
> > > > > >  #define CSR_MHPMEVENT29     0x33d
> > > > > >  #define CSR_MHPMEVENT30     0x33e
> > > > > >  #define CSR_MHPMEVENT31     0x33f
> > > > > > +
> > > > > > +#define CSR_MHPMEVENT3H     0x723
> > > > > > +#define CSR_MHPMEVENT4H     0x724
> > > > > > +#define CSR_MHPMEVENT5H     0x725
> > > > > > +#define CSR_MHPMEVENT6H     0x726
> > > > > > +#define CSR_MHPMEVENT7H     0x727
> > > > > > +#define CSR_MHPMEVENT8H     0x728
> > > > > > +#define CSR_MHPMEVENT9H     0x729
> > > > > > +#define CSR_MHPMEVENT10H    0x72a
> > > > > > +#define CSR_MHPMEVENT11H    0x72b
> > > > > > +#define CSR_MHPMEVENT12H    0x72c
> > > > > > +#define CSR_MHPMEVENT13H    0x72d
> > > > > > +#define CSR_MHPMEVENT14H    0x72e
> > > > > > +#define CSR_MHPMEVENT15H    0x72f
> > > > > > +#define CSR_MHPMEVENT16H    0x730
> > > > > > +#define CSR_MHPMEVENT17H    0x731
> > > > > > +#define CSR_MHPMEVENT18H    0x732
> > > > > > +#define CSR_MHPMEVENT19H    0x733
> > > > > > +#define CSR_MHPMEVENT20H    0x734
> > > > > > +#define CSR_MHPMEVENT21H    0x735
> > > > > > +#define CSR_MHPMEVENT22H    0x736
> > > > > > +#define CSR_MHPMEVENT23H    0x737
> > > > > > +#define CSR_MHPMEVENT24H    0x738
> > > > > > +#define CSR_MHPMEVENT25H    0x739
> > > > > > +#define CSR_MHPMEVENT26H    0x73a
> > > > > > +#define CSR_MHPMEVENT27H    0x73b
> > > > > > +#define CSR_MHPMEVENT28H    0x73c
> > > > > > +#define CSR_MHPMEVENT29H    0x73d
> > > > > > +#define CSR_MHPMEVENT30H    0x73e
> > > > > > +#define CSR_MHPMEVENT31H    0x73f
> > > > > > +
> > > > > >  #define CSR_MHPMCOUNTER3H   0xb83
> > > > > >  #define CSR_MHPMCOUNTER4H   0xb84
> > > > > >  #define CSR_MHPMCOUNTER5H   0xb85
> > > > > > @@ -461,6 +492,7 @@
> > > > > >  #define CSR_VSMTE           0x2c0
> > > > > >  #define CSR_VSPMMASK        0x2c1
> > > > > >  #define CSR_VSPMBASE        0x2c2
> > > > > > +#define CSR_SCOUNTOVF       0xda0
> > > > > >
> > > > > >  /* mstatus CSR bits */
> > > > > >  #define MSTATUS_UIE         0x00000001
> > > > > > @@ -635,6 +667,7 @@ typedef enum RISCVException {
> > > > > >  #define IRQ_VS_EXT                         10
> > > > > >  #define IRQ_M_EXT                          11
> > > > > >  #define IRQ_S_GEXT                         12
> > > > > > +#define IRQ_PMU_OVF                        13
> > > > > >  #define IRQ_LOCAL_MAX                      16
> > > > > >  #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
> > > > > >
> > > > > > @@ -652,11 +685,13 @@ typedef enum RISCVException {
> > > > > >  #define MIP_VSEIP                          (1 << IRQ_VS_EXT)
> > > > > >  #define MIP_MEIP                           (1 << IRQ_M_EXT)
> > > > > >  #define MIP_SGEIP                          (1 << IRQ_S_GEXT)
> > > > > > +#define MIP_LCOFIP                         (1 << IRQ_PMU_OVF)
> > > > > >
> > > > > >  /* sip masks */
> > > > > >  #define SIP_SSIP                           MIP_SSIP
> > > > > >  #define SIP_STIP                           MIP_STIP
> > > > > >  #define SIP_SEIP                           MIP_SEIP
> > > > > > +#define SIP_LCOFIP                         MIP_LCOFIP
> > > > > >
> > > > > >  /* MIE masks */
> > > > > >  #define MIE_SEIE                           (1 << IRQ_S_EXT)
> > > > > > @@ -804,4 +839,24 @@ typedef enum RISCVException {
> > > > > >  #define HVICTL_VALID_MASK                  \
> > > > > >      (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
> > > > > >
> > > > > > +/* PMU related bits */
> > > > > > +#define MIE_LCOFIE                         (1 << IRQ_PMU_OVF)
> > > > > > +
> > > > > > +#define MHPMEVENT_BIT_OF                   BIT_ULL(63)
> > > > > > +#define MHPMEVENTH_BIT_OF                  BIT(31)
> > > > > > +#define MHPMEVENT_BIT_MINH                 BIT_ULL(62)
> > > > > > +#define MHPMEVENTH_BIT_MINH                BIT(30)
> > > > > > +#define MHPMEVENT_BIT_SINH                 BIT_ULL(61)
> > > > > > +#define MHPMEVENTH_BIT_SINH                BIT(29)
> > > > > > +#define MHPMEVENT_BIT_UINH                 BIT_ULL(60)
> > > > > > +#define MHPMEVENTH_BIT_UINH                BIT(28)
> > > > > > +#define MHPMEVENT_BIT_VSINH                BIT_ULL(59)
> > > > > > +#define MHPMEVENTH_BIT_VSINH               BIT(27)
> > > > > > +#define MHPMEVENT_BIT_VUINH                BIT_ULL(58)
> > > > > > +#define MHPMEVENTH_BIT_VUINH               BIT(26)
> > > > > > +
> > > > > > +#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
> > > > > > +#define MHPMEVENT_IDX_MASK                 0xFFFFF
> > > > > > +#define MHPMEVENT_SSCOF_RESVD              16
> > > > > > +
> > > > > >  #endif
> > > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > > > index 04796b99d0fe..519d6377fd9f 100644
> > > > > > --- a/target/riscv/csr.c
> > > > > > +++ b/target/riscv/csr.c
> > > > > > @@ -72,7 +72,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > >      CPUState *cs = env_cpu(env);
> > > > > >      RISCVCPU *cpu = RISCV_CPU(cs);
> > > > > >      int ctr_index;
> > > > > > -    int base_csrno = CSR_HPMCOUNTER3;
> > > > > > +    int base_csrno = CSR_CYCLE;
> > > > > >      bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> > > > > >
> > > > > >      if (rv32 && csrno >= CSR_CYCLEH) {
> > > > > > @@ -81,11 +81,18 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > >      }
> > > > > >      ctr_index = csrno - base_csrno;
> > > > > >
> > > > > > -    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> > > > > > +    if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) ||
> > > > > > +        (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) {
> > > > > > +        goto skip_ext_pmu_check;
> > > > > > +    }
> > > > > > +
> > > > > > +    if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) {
> > > > > >          /* No counter is enabled in PMU or the counter is out of range */
> > > > > >          return RISCV_EXCP_ILLEGAL_INST;
> > > > > >      }
> > > > > >
> > > > > > +skip_ext_pmu_check:
> > > > > > +
> > > > > >      if (env->priv == PRV_S) {
> > > > > >          switch (csrno) {
> > > > > >          case CSR_CYCLE:
> > > > > > @@ -104,7 +111,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > >              }
> > > > > >              break;
> > > > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > > > -            ctr_index = csrno - CSR_CYCLE;
> > > > > >              if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > >                  return RISCV_EXCP_ILLEGAL_INST;
> > > > > >              }
> > > > > > @@ -128,7 +134,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > >                  }
> > > > > >                  break;
> > > > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > > > >                  if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > >                      return RISCV_EXCP_ILLEGAL_INST;
> > > > > >                  }
> > > > > > @@ -158,7 +163,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > >              }
> > > > > >              break;
> > > > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > > > -            ctr_index = csrno - CSR_CYCLE;
> > > > > >              if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > > > >                   get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > >                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > > > @@ -186,7 +190,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > >                  }
> > > > > >                  break;
> > > > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > > > >                  if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > > > >                       get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > >                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > > > @@ -238,6 +241,18 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
> > > > > >      return mctr(env, csrno);
> > > > > >  }
> > > > > >
> > > > > > +static RISCVException sscofpmf(CPURISCVState *env, int csrno)
> > > > > > +{
> > > > > > +    CPUState *cs = env_cpu(env);
> > > > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > > > +
> > > > > > +    if (!cpu->cfg.ext_sscofpmf) {
> > > > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > > > +    }
> > > > > > +
> > > > > > +    return RISCV_EXCP_NONE;
> > > > > > +}
> > > > > > +
> > > > > >  static RISCVException any(CPURISCVState *env, int csrno)
> > > > > >  {
> > > > > >      return RISCV_EXCP_NONE;
> > > > > > @@ -622,11 +637,36 @@ static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong val)
> > > > > >  {
> > > > > >      int evt_index = csrno - CSR_MCOUNTINHIBIT;
> > > > > >
> > > > > > +    if (riscv_cpu_mxl(env) != MXL_RV32) {
> > > > >
> > > > > Is this right? What if the guest writes the high bytes first?
> > > > >
> > > >
> > > > Most of the current software is implemented with lower bytes first.
> > > > But I understand your concern
> > > > that it doesn't provide any guarantee. We probably can have another
> > > > field that can track the order of the updates.
> > > > riscv_pmu_update_event_map will be only called when both lower/upper
> > > > half is complete.
> > >
> > > Why can't you just update it on every write? Guest software should be
> >
> > riscv_pmu_update_event_map maintains a hashmap between counter and event ID.
> > Updating at every write is unnecessary as it will do the hashmap
> > lookup and return fail
> > for high bytes write.

You don't handle the error so does this really matter?

Also, as you don't handle the error you probably don't need to return
it. Maybe just log relevant errors instead?

Alistair

> >
> > The events encoded as the SBI PMU spec will always have the event id
> > in lower 20 bits.
> > Technically, it is okay to just call riscv_pmu_update_event_map in
> > write_mhpmevent not in
> > write_mhpmeventh for rv32 as well.
> >
> > However, I want to keep riscv/pmu.c as generic as possible to allow
> > future implementations
> > to have different event ID value > UINT32_MAX.
>
> I agree. If there is a high CSR for 32-bit we need to support 64-bit values.
>
> >
> > Let me know if you like to keep it simple and just update
> > riscv_pmu_update_event_map in lower bits
> > update right now.
>
> We need to handle both writer ordres. Unless the spec states that
> writes must occur in a certain order we can't assume that they will.
> Otherwise this is just a bug waiting to be discovered
>
> Alitair
>
> >
> > > setting the high bits to 0xFFFF_FFFF first to avoid any issues
> > >
> > > Alistair
> > >
> > > >
> > > >
> > > > > Alistair
> > >
> >
> >
> > --
> > Regards,
> > Atish

Atish Patra May 5, 2022, 6:58 a.m. UTC | #7

On Wed, May 4, 2022 at 3:06 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, May 4, 2022 at 8:03 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Apr 27, 2022 at 7:33 AM Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Mon, Apr 18, 2022 at 3:46 PM Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Sat, Apr 16, 2022 at 9:54 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> > > > >
> > > > > On Wed, Apr 13, 2022 at 12:08 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Mar 31, 2022 at 10:19 AM Atish Patra <atishp@rivosinc.com> wrote:
> > > > > > >
> > > > > > > The Sscofpmf ('Ss' for Privileged arch and Supervisor-level extensions,
> > > > > > > and 'cofpmf' for Count OverFlow and Privilege Mode Filtering)
> > > > > > > extension allows the perf to handle overflow interrupts and filtering
> > > > > > > support. This patch provides a framework for programmable
> > > > > > > counters to leverage the extension. As the extension doesn't have any
> > > > > > > provision for the overflow bit for fixed counters, the fixed events
> > > > > > > can also be monitoring using programmable counters. The underlying
> > > > > > > counters for cycle and instruction counters are always running. Thus,
> > > > > > > a separate timer device is programmed to handle the overflow.
> > > > > > >
> > > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > > ---
> > > > > > >  target/riscv/cpu.c      |  11 ++
> > > > > > >  target/riscv/cpu.h      |  25 +++
> > > > > > >  target/riscv/cpu_bits.h |  55 +++++++
> > > > > > >  target/riscv/csr.c      | 156 ++++++++++++++++--
> > > > > > >  target/riscv/pmu.c      | 347 +++++++++++++++++++++++++++++++++++++++-
> > > > > > >  target/riscv/pmu.h      |   7 +
> > > > > > >  6 files changed, 590 insertions(+), 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > > > index f63602828680..9715eed2fc4e 100644
> > > > > > > --- a/target/riscv/cpu.c
> > > > > > > +++ b/target/riscv/cpu.c
> > > > > > > @@ -22,6 +22,7 @@
> > > > > > >  #include "qemu/ctype.h"
> > > > > > >  #include "qemu/log.h"
> > > > > > >  #include "cpu.h"
> > > > > > > +#include "pmu.h"
> > > > > > >  #include "internals.h"
> > > > > > >  #include "exec/exec-all.h"
> > > > > > >  #include "qapi/error.h"
> > > > > > > @@ -696,6 +697,15 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > > > >          set_misa(env, env->misa_mxl, ext);
> > > > > > >      }
> > > > > > >
> > > > > > > +#ifndef CONFIG_USER_ONLY
> > > > > > > +    if (cpu->cfg.pmu_num) {
> > > > > > > +        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> > > > > > > +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > > > > > +                                          riscv_pmu_timer_cb, cpu);
> > > > > > > +        }
> > > > > > > +     }
> > > > > > > +#endif
> > > > > > > +
> > > > > > >      riscv_cpu_register_gdb_regs_for_features(cs);
> > > > > > >
> > > > > > >      qemu_init_vcpu(cs);
> > > > > > > @@ -795,6 +805,7 @@ static Property riscv_cpu_properties[] = {
> > > > > > >      DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
> > > > > > >      DEFINE_PROP_BOOL("h", RISCVCPU, cfg.ext_h, true),
> > > > > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > > > > +    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
> > > > > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > > > > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > > > > >      DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> > > > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > > > > index 0fa15595fb37..a0e2279ea5e6 100644
> > > > > > > --- a/target/riscv/cpu.h
> > > > > > > +++ b/target/riscv/cpu.h
> > > > > > > @@ -131,6 +131,8 @@ typedef struct PMUCTRState {
> > > > > > >      /* Snapshort value of a counter in RV32 */
> > > > > > >      target_ulong mhpmcounterh_prev;
> > > > > > >      bool started;
> > > > > > > +    /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt trigger */
> > > > > > > +    target_ulong irq_overflow_left;
> > > > > > >  } PMUCTRState;
> > > > > > >
> > > > > > >  struct CPUArchState {
> > > > > > > @@ -291,6 +293,9 @@ struct CPUArchState {
> > > > > > >      /* PMU event selector configured values. First three are unused*/
> > > > > > >      target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
> > > > > > >
> > > > > > > +    /* PMU event selector configured values for RV32*/
> > > > > > > +    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> > > > > > > +
> > > > > > >      target_ulong sscratch;
> > > > > > >      target_ulong mscratch;
> > > > > > >
> > > > > > > @@ -413,6 +418,7 @@ struct RISCVCPUConfig {
> > > > > > >      bool ext_zhinxmin;
> > > > > > >      bool ext_zve32f;
> > > > > > >      bool ext_zve64f;
> > > > > > > +    bool ext_sscofpmf;
> > > > > > >
> > > > > > >      /* Vendor-specific custom extensions */
> > > > > > >      bool ext_XVentanaCondOps;
> > > > > > > @@ -452,6 +458,12 @@ struct ArchCPU {
> > > > > > >
> > > > > > >      /* Configuration Settings */
> > > > > > >      RISCVCPUConfig cfg;
> > > > > > > +
> > > > > > > +    QEMUTimer *pmu_timer;
> > > > > > > +    /* A bitmask of Available programmable counters */
> > > > > > > +    uint32_t pmu_avail_ctrs;
> > > > > > > +    /* Mapping of events to counters */
> > > > > > > +    GHashTable *pmu_event_ctr_map;
> > > > > > >  };
> > > > > > >
> > > > > > >  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> > > > > > > @@ -709,6 +721,19 @@ enum {
> > > > > > >      CSR_TABLE_SIZE = 0x1000
> > > > > > >  };
> > > > > > >
> > > > > > > +/**
> > > > > > > + * The event id are encoded based on the encoding specified in the
> > > > > > > + * SBI specification v0.3
> > > > > > > + */
> > > > > > > +
> > > > > > > +enum riscv_pmu_event_idx {
> > > > > > > +    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > > > > > > +    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > > > > > > +    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > > > > > > +    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > > > > > > +    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > > > > > > +};
> > > > > > > +
> > > > > > >  /* CSR function table */
> > > > > > >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> > > > > > >
> > > > > > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > > > > > index 48b39e6d52a7..d0b53e5ea072 100644
> > > > > > > --- a/target/riscv/cpu_bits.h
> > > > > > > +++ b/target/riscv/cpu_bits.h
> > > > > > > @@ -400,6 +400,37 @@
> > > > > > >  #define CSR_MHPMEVENT29     0x33d
> > > > > > >  #define CSR_MHPMEVENT30     0x33e
> > > > > > >  #define CSR_MHPMEVENT31     0x33f
> > > > > > > +
> > > > > > > +#define CSR_MHPMEVENT3H     0x723
> > > > > > > +#define CSR_MHPMEVENT4H     0x724
> > > > > > > +#define CSR_MHPMEVENT5H     0x725
> > > > > > > +#define CSR_MHPMEVENT6H     0x726
> > > > > > > +#define CSR_MHPMEVENT7H     0x727
> > > > > > > +#define CSR_MHPMEVENT8H     0x728
> > > > > > > +#define CSR_MHPMEVENT9H     0x729
> > > > > > > +#define CSR_MHPMEVENT10H    0x72a
> > > > > > > +#define CSR_MHPMEVENT11H    0x72b
> > > > > > > +#define CSR_MHPMEVENT12H    0x72c
> > > > > > > +#define CSR_MHPMEVENT13H    0x72d
> > > > > > > +#define CSR_MHPMEVENT14H    0x72e
> > > > > > > +#define CSR_MHPMEVENT15H    0x72f
> > > > > > > +#define CSR_MHPMEVENT16H    0x730
> > > > > > > +#define CSR_MHPMEVENT17H    0x731
> > > > > > > +#define CSR_MHPMEVENT18H    0x732
> > > > > > > +#define CSR_MHPMEVENT19H    0x733
> > > > > > > +#define CSR_MHPMEVENT20H    0x734
> > > > > > > +#define CSR_MHPMEVENT21H    0x735
> > > > > > > +#define CSR_MHPMEVENT22H    0x736
> > > > > > > +#define CSR_MHPMEVENT23H    0x737
> > > > > > > +#define CSR_MHPMEVENT24H    0x738
> > > > > > > +#define CSR_MHPMEVENT25H    0x739
> > > > > > > +#define CSR_MHPMEVENT26H    0x73a
> > > > > > > +#define CSR_MHPMEVENT27H    0x73b
> > > > > > > +#define CSR_MHPMEVENT28H    0x73c
> > > > > > > +#define CSR_MHPMEVENT29H    0x73d
> > > > > > > +#define CSR_MHPMEVENT30H    0x73e
> > > > > > > +#define CSR_MHPMEVENT31H    0x73f
> > > > > > > +
> > > > > > >  #define CSR_MHPMCOUNTER3H   0xb83
> > > > > > >  #define CSR_MHPMCOUNTER4H   0xb84
> > > > > > >  #define CSR_MHPMCOUNTER5H   0xb85
> > > > > > > @@ -461,6 +492,7 @@
> > > > > > >  #define CSR_VSMTE           0x2c0
> > > > > > >  #define CSR_VSPMMASK        0x2c1
> > > > > > >  #define CSR_VSPMBASE        0x2c2
> > > > > > > +#define CSR_SCOUNTOVF       0xda0
> > > > > > >
> > > > > > >  /* mstatus CSR bits */
> > > > > > >  #define MSTATUS_UIE         0x00000001
> > > > > > > @@ -635,6 +667,7 @@ typedef enum RISCVException {
> > > > > > >  #define IRQ_VS_EXT                         10
> > > > > > >  #define IRQ_M_EXT                          11
> > > > > > >  #define IRQ_S_GEXT                         12
> > > > > > > +#define IRQ_PMU_OVF                        13
> > > > > > >  #define IRQ_LOCAL_MAX                      16
> > > > > > >  #define IRQ_LOCAL_GUEST_MAX                (TARGET_LONG_BITS - 1)
> > > > > > >
> > > > > > > @@ -652,11 +685,13 @@ typedef enum RISCVException {
> > > > > > >  #define MIP_VSEIP                          (1 << IRQ_VS_EXT)
> > > > > > >  #define MIP_MEIP                           (1 << IRQ_M_EXT)
> > > > > > >  #define MIP_SGEIP                          (1 << IRQ_S_GEXT)
> > > > > > > +#define MIP_LCOFIP                         (1 << IRQ_PMU_OVF)
> > > > > > >
> > > > > > >  /* sip masks */
> > > > > > >  #define SIP_SSIP                           MIP_SSIP
> > > > > > >  #define SIP_STIP                           MIP_STIP
> > > > > > >  #define SIP_SEIP                           MIP_SEIP
> > > > > > > +#define SIP_LCOFIP                         MIP_LCOFIP
> > > > > > >
> > > > > > >  /* MIE masks */
> > > > > > >  #define MIE_SEIE                           (1 << IRQ_S_EXT)
> > > > > > > @@ -804,4 +839,24 @@ typedef enum RISCVException {
> > > > > > >  #define HVICTL_VALID_MASK                  \
> > > > > > >      (HVICTL_VTI | HVICTL_IID | HVICTL_IPRIOM | HVICTL_IPRIO)
> > > > > > >
> > > > > > > +/* PMU related bits */
> > > > > > > +#define MIE_LCOFIE                         (1 << IRQ_PMU_OVF)
> > > > > > > +
> > > > > > > +#define MHPMEVENT_BIT_OF                   BIT_ULL(63)
> > > > > > > +#define MHPMEVENTH_BIT_OF                  BIT(31)
> > > > > > > +#define MHPMEVENT_BIT_MINH                 BIT_ULL(62)
> > > > > > > +#define MHPMEVENTH_BIT_MINH                BIT(30)
> > > > > > > +#define MHPMEVENT_BIT_SINH                 BIT_ULL(61)
> > > > > > > +#define MHPMEVENTH_BIT_SINH                BIT(29)
> > > > > > > +#define MHPMEVENT_BIT_UINH                 BIT_ULL(60)
> > > > > > > +#define MHPMEVENTH_BIT_UINH                BIT(28)
> > > > > > > +#define MHPMEVENT_BIT_VSINH                BIT_ULL(59)
> > > > > > > +#define MHPMEVENTH_BIT_VSINH               BIT(27)
> > > > > > > +#define MHPMEVENT_BIT_VUINH                BIT_ULL(58)
> > > > > > > +#define MHPMEVENTH_BIT_VUINH               BIT(26)
> > > > > > > +
> > > > > > > +#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
> > > > > > > +#define MHPMEVENT_IDX_MASK                 0xFFFFF
> > > > > > > +#define MHPMEVENT_SSCOF_RESVD              16
> > > > > > > +
> > > > > > >  #endif
> > > > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > > > > index 04796b99d0fe..519d6377fd9f 100644
> > > > > > > --- a/target/riscv/csr.c
> > > > > > > +++ b/target/riscv/csr.c
> > > > > > > @@ -72,7 +72,7 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > > >      CPUState *cs = env_cpu(env);
> > > > > > >      RISCVCPU *cpu = RISCV_CPU(cs);
> > > > > > >      int ctr_index;
> > > > > > > -    int base_csrno = CSR_HPMCOUNTER3;
> > > > > > > +    int base_csrno = CSR_CYCLE;
> > > > > > >      bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> > > > > > >
> > > > > > >      if (rv32 && csrno >= CSR_CYCLEH) {
> > > > > > > @@ -81,11 +81,18 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > > >      }
> > > > > > >      ctr_index = csrno - base_csrno;
> > > > > > >
> > > > > > > -    if (!cpu->cfg.pmu_num || ctr_index >= (cpu->cfg.pmu_num)) {
> > > > > > > +    if ((csrno >= CSR_CYCLE && csrno <= CSR_INSTRET) ||
> > > > > > > +        (csrno >= CSR_CYCLEH && csrno <= CSR_INSTRETH)) {
> > > > > > > +        goto skip_ext_pmu_check;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    if ((!cpu->cfg.pmu_num || !(cpu->pmu_avail_ctrs & BIT(ctr_index)))) {
> > > > > > >          /* No counter is enabled in PMU or the counter is out of range */
> > > > > > >          return RISCV_EXCP_ILLEGAL_INST;
> > > > > > >      }
> > > > > > >
> > > > > > > +skip_ext_pmu_check:
> > > > > > > +
> > > > > > >      if (env->priv == PRV_S) {
> > > > > > >          switch (csrno) {
> > > > > > >          case CSR_CYCLE:
> > > > > > > @@ -104,7 +111,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > > >              }
> > > > > > >              break;
> > > > > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > > > > -            ctr_index = csrno - CSR_CYCLE;
> > > > > > >              if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > > >                  return RISCV_EXCP_ILLEGAL_INST;
> > > > > > >              }
> > > > > > > @@ -128,7 +134,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > > >                  }
> > > > > > >                  break;
> > > > > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > > > > >                  if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > > >                      return RISCV_EXCP_ILLEGAL_INST;
> > > > > > >                  }
> > > > > > > @@ -158,7 +163,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > > >              }
> > > > > > >              break;
> > > > > > >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > > > > > > -            ctr_index = csrno - CSR_CYCLE;
> > > > > > >              if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > > > > >                   get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > > >                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > > > > @@ -186,7 +190,6 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> > > > > > >                  }
> > > > > > >                  break;
> > > > > > >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > > > > > > -                ctr_index = csrno - CSR_CYCLEH;
> > > > > > >                  if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > > > > > >                       get_field(env->mcounteren, 1 << ctr_index)) {
> > > > > > >                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > > > > @@ -238,6 +241,18 @@ static RISCVException mctr32(CPURISCVState *env, int csrno)
> > > > > > >      return mctr(env, csrno);
> > > > > > >  }
> > > > > > >
> > > > > > > +static RISCVException sscofpmf(CPURISCVState *env, int csrno)
> > > > > > > +{
> > > > > > > +    CPUState *cs = env_cpu(env);
> > > > > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > > > > +
> > > > > > > +    if (!cpu->cfg.ext_sscofpmf) {
> > > > > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    return RISCV_EXCP_NONE;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static RISCVException any(CPURISCVState *env, int csrno)
> > > > > > >  {
> > > > > > >      return RISCV_EXCP_NONE;
> > > > > > > @@ -622,11 +637,36 @@ static int write_mhpmevent(CPURISCVState *env, int csrno, target_ulong val)
> > > > > > >  {
> > > > > > >      int evt_index = csrno - CSR_MCOUNTINHIBIT;
> > > > > > >
> > > > > > > +    if (riscv_cpu_mxl(env) != MXL_RV32) {
> > > > > >
> > > > > > Is this right? What if the guest writes the high bytes first?
> > > > > >
> > > > >
> > > > > Most of the current software is implemented with lower bytes first.
> > > > > But I understand your concern
> > > > > that it doesn't provide any guarantee. We probably can have another
> > > > > field that can track the order of the updates.
> > > > > riscv_pmu_update_event_map will be only called when both lower/upper
> > > > > half is complete.
> > > >
> > > > Why can't you just update it on every write? Guest software should be
> > >
> > > riscv_pmu_update_event_map maintains a hashmap between counter and event ID.
> > > Updating at every write is unnecessary as it will do the hashmap
> > > lookup and return fail
> > > for high bytes write.
>
> You don't handle the error so does this really matter?
>
> Also, as you don't handle the error you probably don't need to return
> it. Maybe just log relevant errors instead?
>

it's just unnecessary execution cycles and a bit odd as well. I will
maintain a flag
to track the order. We need similar flags for a few others as well
(stimecmp, vstimecmp)
anyways. That would make it closer to how actual hardware would implement also.

I will send the patch.


> Alistair
>
> > >
> > > The events encoded as the SBI PMU spec will always have the event id
> > > in lower 20 bits.
> > > Technically, it is okay to just call riscv_pmu_update_event_map in
> > > write_mhpmevent not in
> > > write_mhpmeventh for rv32 as well.
> > >
> > > However, I want to keep riscv/pmu.c as generic as possible to allow
> > > future implementations
> > > to have different event ID value > UINT32_MAX.
> >
> > I agree. If there is a high CSR for 32-bit we need to support 64-bit values.
> >
> > >
> > > Let me know if you like to keep it simple and just update
> > > riscv_pmu_update_event_map in lower bits
> > > update right now.
> >
> > We need to handle both writer ordres. Unless the spec states that
> > writes must occur in a certain order we can't assume that they will.
> > Otherwise this is just a bug waiting to be discovered
> >

Fair enough.

> > Alitair
> >
> > >
> > > > setting the high bits to 0xFFFF_FFFF first to avoid any issues
> > > >
> > > > Alistair
> > > >
> > > > >
> > > > >
> > > > > > Alistair
> > > >
> > >
> > >
> > > --
> > > Regards,
> > > Atish

[v7,08/12] target/riscv: Add sscofpmf extension support

Commit Message

Comments

Patch