Re: [PATCH v10 11/12] hw/riscv: virt: Add PMU DT node to the device tree

[Atish Patra]

On Thu, Jul 14, 2022 at 3:27 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi Atish,
>
> Am Dienstag, 21. Juni 2022, 01:16:01 CEST schrieb Atish Patra:
> > Qemu virt machine can support few cache events and cycle/instret counters.
> > It also supports counter overflow for these events.
> >
> > Add a DT node so that OpenSBI/Linux kernel is aware of the virt machine
> > capabilities. There are some dummy nodes added for testing as well.
> >
> > Acked-by: Alistair Francis <alistair.francis@wdc.com>
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
>
> > +static void create_fdt_socket_pmu(RISCVVirtState *s,
> > +                                  int socket, uint32_t *phandle,
> > +                                  uint32_t *intc_phandles)
> > +{
> > +    int cpu;
> > +    char *pmu_name;
> > +    uint32_t *pmu_cells;
> > +    MachineState *mc = MACHINE(s);
> > +    RISCVCPU hart = s->soc[socket].harts[0];
> > +
> > +    pmu_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2);
> > +
> > +    for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) {
> > +        pmu_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> > +        pmu_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_PMU_OVF);
> > +    }
> > +
> > +    pmu_name = g_strdup_printf("/soc/pmu");
> > +    qemu_fdt_add_subnode(mc->fdt, pmu_name);
> > +    qemu_fdt_setprop_string(mc->fdt, pmu_name, "compatible", "riscv,pmu");
>
> Where is the binding document for this?
>
> As the comment below states the "riscv,event-to-mhpmcounters" property
> is opensbi-specific and gets removed in the opensbi stage, but that still
> leaves the pmu node in it and from the version I found, Rob wasn't overly
> happy with the compatible [0]. Did this get addressed?
>

This is OpenSBI specific binding.
https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md

Linux kernel doesn't use binding anymore. The earlier version patches
relied on the DT binding.
However, based on the feedback it was removed.

OpenSBI should delete the node and the interrupt-extended property
deletion is necessary at this point.

>
> Thanks
> Heiko
>
>
> [0] https://lore.kernel.org/all/YXhPqfpXh1VZN07T@robh.at.kernel.org/
>
>
>
> > +    riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name);
> > +
> > +    g_free(pmu_name);
> > +    g_free(pmu_cells);
> > +}
> > +
> >  static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry 
> > *memmap,
> >                                 bool is_32_bit, uint32_t *phandle,
> >                                 uint32_t *irq_mmio_phandle,
> > @@ -759,6 +786,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const 
> > MemMapEntry *memmap,
> >                      &intc_phandles[phandle_pos]);
> >              }
> >          }
> > +        create_fdt_socket_pmu(s, socket, phandle, intc_phandles);
> >      }
> >
> >      if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7d9e2aca12a9..69bbd9fff4e1 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1110,6 +1110,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char 
> > **isa_str, int max_str_len)
> >          ISA_EDATA_ENTRY(zve64f, ext_zve64f),
> >          ISA_EDATA_ENTRY(zhinx, ext_zhinx),
> >          ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
> > +        ISA_EDATA_ENTRY(sscofpmf, ext_sscofpmf),
> >          ISA_EDATA_ENTRY(svinval, ext_svinval),
> >          ISA_EDATA_ENTRY(svnapot, ext_svnapot),
> >          ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index 34096941c0ce..59feb3c243dd 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -20,11 +20,68 @@
> >  #include "cpu.h"
> >  #include "pmu.h"
> >  #include "sysemu/cpu-timers.h"
> > +#include "sysemu/device_tree.h"
> >
> >  #define RISCV_TIMEBASE_FREQ 1000000000 /* 1Ghz */
> >  #define MAKE_32BIT_MASK(shift, length) \
> >          (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
> >
> > +/**
> > + * To keep it simple, any event can be mapped to any programmable counters 
> > in
> > + * QEMU. The generic cycle & instruction count events can also be monitored
> > + * using programmable counters. In that case, mcycle & minstret must 
> > continue
> > + * to provide the correct value as well. Heterogeneous PMU per hart is not
> > + * supported yet. Thus, number of counters are same across all harts.
> > + */
> > +void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
> > +{
> > +    uint32_t fdt_event_ctr_map[20] = {};
> > +    uint32_t cmask;
> > +
> > +    /* All the programmable counters can map to any event */
> > +    cmask = MAKE_32BIT_MASK(3, num_ctrs);
> > +
> > +   /**
> > +    * The event encoding is specified in the SBI specification
> > +    * Event idx is a 20bits wide number encoded as follows:
> > +    * event_idx[19:16] = type
> > +    * event_idx[15:0] = code
> > +    * The code field in cache events are encoded as follows:
> > +    * event_idx.code[15:3] = cache_id
> > +    * event_idx.code[2:1] = op_id
> > +    * event_idx.code[0:0] = result_id
> > +    */
> > +
> > +   /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> > +   fdt_event_ctr_map[0] = cpu_to_be32(0x00000001);
> > +   fdt_event_ctr_map[1] = cpu_to_be32(0x00000001);
> > +   fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
> > +
> > +   /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> > +   fdt_event_ctr_map[3] = cpu_to_be32(0x00000002);
> > +   fdt_event_ctr_map[4] = cpu_to_be32(0x00000002);
> > +   fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
> > +
> > +   /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
> > +   fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
> > +   fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
> > +   fdt_event_ctr_map[8] = cpu_to_be32(cmask);
> > +
> > +   /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
> > +   fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
> > +   fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
> > +   fdt_event_ctr_map[11] = cpu_to_be32(cmask);
> > +
> > +   /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
> > +   fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
> > +   fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
> > +   fdt_event_ctr_map[14] = cpu_to_be32(cmask);
> > +
> > +   /* This a OpenSBI specific DT property documented in OpenSBI docs */
> > +   qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
> > +                    fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
> > +}
> > +
> >  static bool riscv_pmu_counter_valid(RISCVCPU *cpu, uint32_t ctr_idx)
> >  {
> >      if (ctr_idx < 3 || ctr_idx >= RV_MAX_MHPMCOUNTERS ||
> > diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> > index 036653627f78..3004ce37b636 100644
> > --- a/target/riscv/pmu.h
> > +++ b/target/riscv/pmu.h
> > @@ -31,5 +31,6 @@ int riscv_pmu_init(RISCVCPU *cpu, int num_counters);
> >  int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
> >                                 uint32_t ctr_idx);
> >  int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> > +void riscv_pmu_generate_fdt_node(void *fdt, int num_counters, char 
> > *pmu_name);
> >  int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value,
> >                            uint32_t ctr_idx);
> >
>
>
>
>
>


-- 
Regards,
Atish