| Message ID | 20220824221701.41932-5-atishp@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve PMU support | expand |
On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: > 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. Hey Atish! I was fiddling with dumping the virt machine dtb again today to check some dt-binding changes I was making for the isa string would play nicely with the virt machine & I noticed that this patch has introduced a new validation failure: ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml I assume this is the aforementioned "dummy" node & you have no intention of creating a binding for this? Thanks, Conor. > 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> > --- > hw/riscv/virt.c | 16 +++++++++++++ > target/riscv/pmu.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++ > target/riscv/pmu.h | 1 + > 3 files changed, 74 insertions(+) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index ff8c0df5cd47..befa9d2c26ac 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -30,6 +30,7 @@ > #include "hw/char/serial.h" > #include "target/riscv/cpu.h" > #include "hw/core/sysbus-fdt.h" > +#include "target/riscv/pmu.h" > #include "hw/riscv/riscv_hart.h" > #include "hw/riscv/virt.h" > #include "hw/riscv/boot.h" > @@ -708,6 +709,20 @@ static void create_fdt_socket_aplic(RISCVVirtState *s, > aplic_phandles[socket] = aplic_s_phandle; > } > > +static void create_fdt_pmu(RISCVVirtState *s) > +{ > + char *pmu_name; > + MachineState *mc = MACHINE(s); > + RISCVCPU hart = s->soc[0].harts[0]; > + > + 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"); > + riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name); > + > + g_free(pmu_name); > +} > + > static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, > bool is_32_bit, uint32_t *phandle, > uint32_t *irq_mmio_phandle, > @@ -1036,6 +1051,7 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, > > create_fdt_flash(s, memmap); > create_fdt_fw_cfg(s, memmap); > + create_fdt_pmu(s); > > update_bootargs: > if (cmdline && *cmdline) { > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c > index a5f504e53c88..b8e56d2b7b8e 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); > -- > 2.25.1 > > >
On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: > > 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. > > Hey Atish! > > I was fiddling with dumping the virt machine dtb again today to check > some dt-binding changes I was making for the isa string would play > nicely with the virt machine & I noticed that this patch has introduced > a new validation failure: > > ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb > > dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb > /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} > From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml > > I assume this is the aforementioned "dummy" node & you have no intention > of creating a binding for this? > It is a dummy node from Linux kernel perspective. OpenSbi use this node to figure out the hpmcounter mappings. > Thanks, > Conor. > > > 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> > > --- > > hw/riscv/virt.c | 16 +++++++++++++ > > target/riscv/pmu.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++ > > target/riscv/pmu.h | 1 + > > 3 files changed, 74 insertions(+) > > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > > index ff8c0df5cd47..befa9d2c26ac 100644 > > --- a/hw/riscv/virt.c > > +++ b/hw/riscv/virt.c > > @@ -30,6 +30,7 @@ > > #include "hw/char/serial.h" > > #include "target/riscv/cpu.h" > > #include "hw/core/sysbus-fdt.h" > > +#include "target/riscv/pmu.h" > > #include "hw/riscv/riscv_hart.h" > > #include "hw/riscv/virt.h" > > #include "hw/riscv/boot.h" > > @@ -708,6 +709,20 @@ static void create_fdt_socket_aplic(RISCVVirtState *s, > > aplic_phandles[socket] = aplic_s_phandle; > > } > > > > +static void create_fdt_pmu(RISCVVirtState *s) > > +{ > > + char *pmu_name; > > + MachineState *mc = MACHINE(s); > > + RISCVCPU hart = s->soc[0].harts[0]; > > + > > + 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"); > > + riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name); > > + > > + g_free(pmu_name); > > +} > > + > > static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, > > bool is_32_bit, uint32_t *phandle, > > uint32_t *irq_mmio_phandle, > > @@ -1036,6 +1051,7 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, > > > > create_fdt_flash(s, memmap); > > create_fdt_fw_cfg(s, memmap); > > + create_fdt_pmu(s); > > > > update_bootargs: > > if (cmdline && *cmdline) { > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c > > index a5f504e53c88..b8e56d2b7b8e 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); > > -- > > 2.25.1 > > > > > >
On 28/11/2022 20:16, Atish Kumar Patra wrote: > On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: >> >> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: >>> 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. >> >> Hey Atish! >> >> I was fiddling with dumping the virt machine dtb again today to check >> some dt-binding changes I was making for the isa string would play >> nicely with the virt machine & I noticed that this patch has introduced >> a new validation failure: >> >> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb >> >> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb >> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} >> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml >> >> I assume this is the aforementioned "dummy" node & you have no intention >> of creating a binding for this? >> > > It is a dummy node from Linux kernel perspective. OpenSbi use this > node to figure out the hpmcounter mappings. Aye, but should it not have a binding anyway, since they're not meant to be linux specific? >>> 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> >>> --- >>> hw/riscv/virt.c | 16 +++++++++++++ >>> target/riscv/pmu.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++ >>> target/riscv/pmu.h | 1 + >>> 3 files changed, 74 insertions(+) >>> >>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >>> index ff8c0df5cd47..befa9d2c26ac 100644 >>> --- a/hw/riscv/virt.c >>> +++ b/hw/riscv/virt.c >>> @@ -30,6 +30,7 @@ >>> #include "hw/char/serial.h" >>> #include "target/riscv/cpu.h" >>> #include "hw/core/sysbus-fdt.h" >>> +#include "target/riscv/pmu.h" >>> #include "hw/riscv/riscv_hart.h" >>> #include "hw/riscv/virt.h" >>> #include "hw/riscv/boot.h" >>> @@ -708,6 +709,20 @@ static void create_fdt_socket_aplic(RISCVVirtState *s, >>> aplic_phandles[socket] = aplic_s_phandle; >>> } >>> >>> +static void create_fdt_pmu(RISCVVirtState *s) >>> +{ >>> + char *pmu_name; >>> + MachineState *mc = MACHINE(s); >>> + RISCVCPU hart = s->soc[0].harts[0]; >>> + >>> + 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"); >>> + riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name); >>> + >>> + g_free(pmu_name); >>> +} >>> + >>> static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, >>> bool is_32_bit, uint32_t *phandle, >>> uint32_t *irq_mmio_phandle, >>> @@ -1036,6 +1051,7 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, >>> >>> create_fdt_flash(s, memmap); >>> create_fdt_fw_cfg(s, memmap); >>> + create_fdt_pmu(s); >>> >>> update_bootargs: >>> if (cmdline && *cmdline) { >>> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c >>> index a5f504e53c88..b8e56d2b7b8e 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); >>> -- >>> 2.25.1 >>> >>> >>> >
On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: > > On 28/11/2022 20:16, Atish Kumar Patra wrote: > > On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: > >> > >> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: > >>> 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. > >> > >> Hey Atish! > >> > >> I was fiddling with dumping the virt machine dtb again today to check > >> some dt-binding changes I was making for the isa string would play > >> nicely with the virt machine & I noticed that this patch has introduced > >> a new validation failure: > >> > >> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb > >> > >> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb > >> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} > >> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml > >> > >> I assume this is the aforementioned "dummy" node & you have no intention > >> of creating a binding for this? > >> > > > > It is a dummy node from Linux kernel perspective. OpenSbi use this > > node to figure out the hpmcounter mappings. > > Aye, but should it not have a binding anyway, since they're not > meant to be linux specific? > It is documented in OpenSBI. https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md Are you suggesting that any non-Linux specific DT nodes should be part of Linux DT binding 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> > >>> --- > >>> hw/riscv/virt.c | 16 +++++++++++++ > >>> target/riscv/pmu.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++ > >>> target/riscv/pmu.h | 1 + > >>> 3 files changed, 74 insertions(+) > >>> > >>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > >>> index ff8c0df5cd47..befa9d2c26ac 100644 > >>> --- a/hw/riscv/virt.c > >>> +++ b/hw/riscv/virt.c > >>> @@ -30,6 +30,7 @@ > >>> #include "hw/char/serial.h" > >>> #include "target/riscv/cpu.h" > >>> #include "hw/core/sysbus-fdt.h" > >>> +#include "target/riscv/pmu.h" > >>> #include "hw/riscv/riscv_hart.h" > >>> #include "hw/riscv/virt.h" > >>> #include "hw/riscv/boot.h" > >>> @@ -708,6 +709,20 @@ static void create_fdt_socket_aplic(RISCVVirtState *s, > >>> aplic_phandles[socket] = aplic_s_phandle; > >>> } > >>> > >>> +static void create_fdt_pmu(RISCVVirtState *s) > >>> +{ > >>> + char *pmu_name; > >>> + MachineState *mc = MACHINE(s); > >>> + RISCVCPU hart = s->soc[0].harts[0]; > >>> + > >>> + 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"); > >>> + riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name); > >>> + > >>> + g_free(pmu_name); > >>> +} > >>> + > >>> static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, > >>> bool is_32_bit, uint32_t *phandle, > >>> uint32_t *irq_mmio_phandle, > >>> @@ -1036,6 +1051,7 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, > >>> > >>> create_fdt_flash(s, memmap); > >>> create_fdt_fw_cfg(s, memmap); > >>> + create_fdt_pmu(s); > >>> > >>> update_bootargs: > >>> if (cmdline && *cmdline) { > >>> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c > >>> index a5f504e53c88..b8e56d2b7b8e 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); > >>> -- > >>> 2.25.1 > >>> > >>> > >>> > >
On 28/11/2022 20:41, Atish Kumar Patra wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: >> >> On 28/11/2022 20:16, Atish Kumar Patra wrote: >>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: >>>> >>>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: >>>>> 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. >>>> >>>> Hey Atish! >>>> >>>> I was fiddling with dumping the virt machine dtb again today to check >>>> some dt-binding changes I was making for the isa string would play >>>> nicely with the virt machine & I noticed that this patch has introduced >>>> a new validation failure: >>>> >>>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb >>>> >>>> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb >>>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} >>>> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml >>>> >>>> I assume this is the aforementioned "dummy" node & you have no intention >>>> of creating a binding for this? >>>> >>> >>> It is a dummy node from Linux kernel perspective. OpenSbi use this >>> node to figure out the hpmcounter mappings. >> >> Aye, but should it not have a binding anyway, since they're not >> meant to be linux specific? >> > It is documented in OpenSBI. > https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md > > Are you suggesting that any non-Linux specific DT nodes should be part > of Linux DT binding as well ? I thought the point was that they were *not* meant to be linux specific, just happening to be housed there.
On Mon, Nov 28, 2022 at 09:10:03PM +0000, Conor.Dooley@microchip.com wrote: > On 28/11/2022 20:41, Atish Kumar Patra wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: > >> > >> On 28/11/2022 20:16, Atish Kumar Patra wrote: > >>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: > >>>> > >>>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: > >>>>> 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. > >>>> > >>>> Hey Atish! > >>>> > >>>> I was fiddling with dumping the virt machine dtb again today to check > >>>> some dt-binding changes I was making for the isa string would play > >>>> nicely with the virt machine & I noticed that this patch has introduced > >>>> a new validation failure: > >>>> > >>>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb > >>>> > >>>> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb > >>>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} > >>>> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml > >>>> > >>>> I assume this is the aforementioned "dummy" node & you have no intention > >>>> of creating a binding for this? > >>>> > >>> > >>> It is a dummy node from Linux kernel perspective. OpenSbi use this > >>> node to figure out the hpmcounter mappings. > >> > >> Aye, but should it not have a binding anyway, since they're not > >> meant to be linux specific? > >> > > It is documented in OpenSBI. > > https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md > > > > Are you suggesting that any non-Linux specific DT nodes should be part > > of Linux DT binding as well ? > > I thought the point was that they were *not* meant to be linux specific, > just happening to be housed there. > I'm not sure if there's an official policy on where DT nodes should be specified, but it looks like Samuel's opinion is that they should live in the Linux kernel, whether they're used there or not [1]. [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html Thanks, drew
On 29/11/2022 07:08, Andrew Jones wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Nov 28, 2022 at 09:10:03PM +0000, Conor.Dooley@microchip.com wrote: >> On 28/11/2022 20:41, Atish Kumar Patra wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: >>>> >>>> On 28/11/2022 20:16, Atish Kumar Patra wrote: >>>>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: >>>>>> >>>>>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: >>>>>>> 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. >>>>>> >>>>>> Hey Atish! >>>>>> >>>>>> I was fiddling with dumping the virt machine dtb again today to check >>>>>> some dt-binding changes I was making for the isa string would play >>>>>> nicely with the virt machine & I noticed that this patch has introduced >>>>>> a new validation failure: >>>>>> >>>>>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb >>>>>> >>>>>> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb >>>>>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} >>>>>> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml >>>>>> >>>>>> I assume this is the aforementioned "dummy" node & you have no intention >>>>>> of creating a binding for this? >>>>>> >>>>> >>>>> It is a dummy node from Linux kernel perspective. OpenSbi use this >>>>> node to figure out the hpmcounter mappings. >>>> >>>> Aye, but should it not have a binding anyway, since they're not >>>> meant to be linux specific? >>>> >>> It is documented in OpenSBI. >>> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md >>> >>> Are you suggesting that any non-Linux specific DT nodes should be part >>> of Linux DT binding as well ? >> >> I thought the point was that they were *not* meant to be linux specific, >> just happening to be housed there. >> > > I'm not sure if there's an official policy on where DT nodes should be > specified, but it looks like Samuel's opinion is that they should live > in the Linux kernel, whether they're used there or not [1]. > > [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html Yah, that was also my understanding. See also U-Boot moving to unify their custom bindings into the linux repo: https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-sjg@chromium.org/
On Mon, Nov 28, 2022 at 11:32 PM <Conor.Dooley@microchip.com> wrote: > > On 29/11/2022 07:08, Andrew Jones wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Mon, Nov 28, 2022 at 09:10:03PM +0000, Conor.Dooley@microchip.com wrote: > >> On 28/11/2022 20:41, Atish Kumar Patra wrote: > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> > >>> On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: > >>>> > >>>> On 28/11/2022 20:16, Atish Kumar Patra wrote: > >>>>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: > >>>>>> > >>>>>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: > >>>>>>> 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. > >>>>>> > >>>>>> Hey Atish! > >>>>>> > >>>>>> I was fiddling with dumping the virt machine dtb again today to check > >>>>>> some dt-binding changes I was making for the isa string would play > >>>>>> nicely with the virt machine & I noticed that this patch has introduced > >>>>>> a new validation failure: > >>>>>> > >>>>>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb > >>>>>> > >>>>>> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb > >>>>>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} > >>>>>> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml > >>>>>> > >>>>>> I assume this is the aforementioned "dummy" node & you have no intention > >>>>>> of creating a binding for this? > >>>>>> > >>>>> > >>>>> It is a dummy node from Linux kernel perspective. OpenSbi use this > >>>>> node to figure out the hpmcounter mappings. > >>>> > >>>> Aye, but should it not have a binding anyway, since they're not > >>>> meant to be linux specific? > >>>> > >>> It is documented in OpenSBI. > >>> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md > >>> > >>> Are you suggesting that any non-Linux specific DT nodes should be part > >>> of Linux DT binding as well ? > >> > >> I thought the point was that they were *not* meant to be linux specific, > >> just happening to be housed there. > >> > > > > I'm not sure if there's an official policy on where DT nodes should be > > specified, but it looks like Samuel's opinion is that they should live > > in the Linux kernel, whether they're used there or not [1]. > > > > [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html > > Yah, that was also my understanding. See also U-Boot moving to unify > their custom bindings into the linux repo: > https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-sjg@chromium.org/ > This adds the U-Boot specific DT properties to the dts schema itself, not Linux kernel DT bindings. I am not opposed to adding PMU DT bindings to Linux but there should be a clear policy on this. What about OpenSBI domain DT bindings ? If every other DT based open source project starts adding their DT binding to the Linux kernel, that may go downhill pretty soon. > > >
On 29/11/2022 09:27, Atish Kumar Patra wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Nov 28, 2022 at 11:32 PM <Conor.Dooley@microchip.com> wrote: >> >> On 29/11/2022 07:08, Andrew Jones wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Mon, Nov 28, 2022 at 09:10:03PM +0000, Conor.Dooley@microchip.com wrote: >>>> On 28/11/2022 20:41, Atish Kumar Patra wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>> >>>>> On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: >>>>>> >>>>>> On 28/11/2022 20:16, Atish Kumar Patra wrote: >>>>>>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: >>>>>>>> >>>>>>>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: >>>>>>>>> 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. >>>>>>>> >>>>>>>> Hey Atish! >>>>>>>> >>>>>>>> I was fiddling with dumping the virt machine dtb again today to check >>>>>>>> some dt-binding changes I was making for the isa string would play >>>>>>>> nicely with the virt machine & I noticed that this patch has introduced >>>>>>>> a new validation failure: >>>>>>>> >>>>>>>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb >>>>>>>> >>>>>>>> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb >>>>>>>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} >>>>>>>> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml >>>>>>>> >>>>>>>> I assume this is the aforementioned "dummy" node & you have no intention >>>>>>>> of creating a binding for this? >>>>>>>> >>>>>>> >>>>>>> It is a dummy node from Linux kernel perspective. OpenSbi use this >>>>>>> node to figure out the hpmcounter mappings. >>>>>> >>>>>> Aye, but should it not have a binding anyway, since they're not >>>>>> meant to be linux specific? >>>>>> >>>>> It is documented in OpenSBI. >>>>> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md >>>>> >>>>> Are you suggesting that any non-Linux specific DT nodes should be part >>>>> of Linux DT binding as well ? >>>> >>>> I thought the point was that they were *not* meant to be linux specific, >>>> just happening to be housed there. >>>> >>> >>> I'm not sure if there's an official policy on where DT nodes should be >>> specified, but it looks like Samuel's opinion is that they should live >>> in the Linux kernel, whether they're used there or not [1]. >>> >>> [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html >> >> Yah, that was also my understanding. See also U-Boot moving to unify >> their custom bindings into the linux repo: >> https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-sjg@chromium.org/ >> > > This adds the U-Boot specific DT properties to the dts schema itself, > not Linux kernel DT bindings. Yeah, sorry. I muddled things up a little there. My point was that they are trying to get to a stage where dt-validate and those tools work for them too. I'm not sure were I said "linux repo" rather than "dt-schema repo" when I double checked the file paths in the link before pasting it to make sure it was the dt-schema one.. I blame it being early. > I am not opposed to adding PMU DT bindings to Linux but there should > be a clear policy on this. > What about OpenSBI domain DT bindings ? > If every other DT based open source project starts adding their DT > binding to the Linux kernel, that may go downhill pretty soon. Maybe I am misunderstanding, but I had thought the goal was to get to user-independent bindings. Rob and Krzysztof certainly labour the point that the bindings should not reflect how one operating system's drivers would like to see them & u-boot or FreeBSD using a property is grounds for it not being removed from the bindings in the linux tree. I'll go and actually ask Rob.
+CC Rob, which I probably should've done earlier, so context all preserved On 29/11/2022 09:42, Conor Dooley wrote: > On 29/11/2022 09:27, Atish Kumar Patra wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On Mon, Nov 28, 2022 at 11:32 PM <Conor.Dooley@microchip.com> wrote: >>> >>> On 29/11/2022 07:08, Andrew Jones wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> >>>> On Mon, Nov 28, 2022 at 09:10:03PM +0000, Conor.Dooley@microchip.com wrote: >>>>> On 28/11/2022 20:41, Atish Kumar Patra wrote: >>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>>> >>>>>> On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: >>>>>>> >>>>>>> On 28/11/2022 20:16, Atish Kumar Patra wrote: >>>>>>>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: >>>>>>>>> >>>>>>>>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: >>>>>>>>>> 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. >>>>>>>>> >>>>>>>>> Hey Atish! >>>>>>>>> >>>>>>>>> I was fiddling with dumping the virt machine dtb again today to check >>>>>>>>> some dt-binding changes I was making for the isa string would play >>>>>>>>> nicely with the virt machine & I noticed that this patch has introduced >>>>>>>>> a new validation failure: >>>>>>>>> >>>>>>>>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb >>>>>>>>> >>>>>>>>> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb >>>>>>>>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} >>>>>>>>> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml >>>>>>>>> >>>>>>>>> I assume this is the aforementioned "dummy" node & you have no intention >>>>>>>>> of creating a binding for this? >>>>>>>>> >>>>>>>> >>>>>>>> It is a dummy node from Linux kernel perspective. OpenSbi use this >>>>>>>> node to figure out the hpmcounter mappings. >>>>>>> >>>>>>> Aye, but should it not have a binding anyway, since they're not >>>>>>> meant to be linux specific? >>>>>>> >>>>>> It is documented in OpenSBI. >>>>>> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md >>>>>> >>>>>> Are you suggesting that any non-Linux specific DT nodes should be part >>>>>> of Linux DT binding as well ? >>>>> >>>>> I thought the point was that they were *not* meant to be linux specific, >>>>> just happening to be housed there. >>>>> >>>> >>>> I'm not sure if there's an official policy on where DT nodes should be >>>> specified, but it looks like Samuel's opinion is that they should live >>>> in the Linux kernel, whether they're used there or not [1]. >>>> >>>> [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html >>> >>> Yah, that was also my understanding. See also U-Boot moving to unify >>> their custom bindings into the linux repo: >>> https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-sjg@chromium.org/ >>> >> >> This adds the U-Boot specific DT properties to the dts schema itself, >> not Linux kernel DT bindings. > > Yeah, sorry. I muddled things up a little there. My point was that they > are trying to get to a stage where dt-validate and those tools work for > them too. I'm not sure were I said "linux repo" rather than "dt-schema > repo" when I double checked the file paths in the link before pasting it > to make sure it was the dt-schema one.. I blame it being early. > >> I am not opposed to adding PMU DT bindings to Linux but there should >> be a clear policy on this. >> What about OpenSBI domain DT bindings ? >> If every other DT based open source project starts adding their DT >> binding to the Linux kernel, that may go downhill pretty soon. Rob, perhaps you can be a source of clarity here :) My early morning muddling didn't help things. > Maybe I am misunderstanding, but I had thought the goal was to get to > user-independent bindings. Rob and Krzysztof certainly labour the point > that the bindings should not reflect how one operating system's drivers > would like to see them & u-boot or FreeBSD using a property is grounds > for it not being removed from the bindings in the linux tree. > > I'll go and actually ask Rob. I did go & ask Rob, to which he said "I'll apply it even if no driver." Do you want to whip up a binding, or shall I?
On Tue, Nov 29, 2022 at 3:54 PM <Conor.Dooley@microchip.com> wrote: > > +CC Rob, which I probably should've done earlier, so > context all preserved > > On 29/11/2022 09:42, Conor Dooley wrote: > > On 29/11/2022 09:27, Atish Kumar Patra wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> > >> On Mon, Nov 28, 2022 at 11:32 PM <Conor.Dooley@microchip.com> wrote: > >>> > >>> On 29/11/2022 07:08, Andrew Jones wrote: > >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>>> > >>>> On Mon, Nov 28, 2022 at 09:10:03PM +0000, Conor.Dooley@microchip.com wrote: > >>>>> On 28/11/2022 20:41, Atish Kumar Patra wrote: > >>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>>>>> > >>>>>> On Mon, Nov 28, 2022 at 12:38 PM <Conor.Dooley@microchip.com> wrote: > >>>>>>> > >>>>>>> On 28/11/2022 20:16, Atish Kumar Patra wrote: > >>>>>>>> On Thu, Nov 24, 2022 at 5:17 AM Conor Dooley <conor.dooley@microchip.com> wrote: > >>>>>>>>> > >>>>>>>>> On Wed, Aug 24, 2022 at 03:17:00PM -0700, Atish Patra wrote: > >>>>>>>>>> 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. > >>>>>>>>> > >>>>>>>>> Hey Atish! > >>>>>>>>> > >>>>>>>>> I was fiddling with dumping the virt machine dtb again today to check > >>>>>>>>> some dt-binding changes I was making for the isa string would play > >>>>>>>>> nicely with the virt machine & I noticed that this patch has introduced > >>>>>>>>> a new validation failure: > >>>>>>>>> > >>>>>>>>> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb > >>>>>>>>> > >>>>>>>>> dt-validate -p ../linux/Documentation/devicetree/bindings/processed-schema.json qemu.dtb > >>>>>>>>> /home/conor/stuff/qemu/qemu.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281, 2, 2, 524284, 65561, 65561, 524280, 65563, 65563, 524280, 65569, 65569, 524280, 0, 0, 0, 0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'} > >>>>>>>>> From schema: /home/conor/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml > >>>>>>>>> > >>>>>>>>> I assume this is the aforementioned "dummy" node & you have no intention > >>>>>>>>> of creating a binding for this? > >>>>>>>>> > >>>>>>>> > >>>>>>>> It is a dummy node from Linux kernel perspective. OpenSbi use this > >>>>>>>> node to figure out the hpmcounter mappings. > >>>>>>> > >>>>>>> Aye, but should it not have a binding anyway, since they're not > >>>>>>> meant to be linux specific? > >>>>>>> > >>>>>> It is documented in OpenSBI. > >>>>>> https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md > >>>>>> > >>>>>> Are you suggesting that any non-Linux specific DT nodes should be part > >>>>>> of Linux DT binding as well ? > >>>>> > >>>>> I thought the point was that they were *not* meant to be linux specific, > >>>>> just happening to be housed there. > >>>>> > >>>> > >>>> I'm not sure if there's an official policy on where DT nodes should be > >>>> specified, but it looks like Samuel's opinion is that they should live > >>>> in the Linux kernel, whether they're used there or not [1]. > >>>> > >>>> [1] http://lists.infradead.org/pipermail/opensbi/2022-October/003522.html > >>> > >>> Yah, that was also my understanding. See also U-Boot moving to unify > >>> their custom bindings into the linux repo: > >>> https://lore.kernel.org/linux-devicetree/20220930001410.2802843-1-sjg@chromium.org/ > >>> > >> > >> This adds the U-Boot specific DT properties to the dts schema itself, > >> not Linux kernel DT bindings. > > > > Yeah, sorry. I muddled things up a little there. My point was that they > > are trying to get to a stage where dt-validate and those tools work for > > them too. I'm not sure were I said "linux repo" rather than "dt-schema > > repo" when I double checked the file paths in the link before pasting it > > to make sure it was the dt-schema one.. I blame it being early. > > > >> I am not opposed to adding PMU DT bindings to Linux but there should > >> be a clear policy on this. > >> What about OpenSBI domain DT bindings ? > >> If every other DT based open source project starts adding their DT > >> binding to the Linux kernel, that may go downhill pretty soon. > > Rob, perhaps you can be a source of clarity here :) My early morning > muddling didn't help things. > > > > Maybe I am misunderstanding, but I had thought the goal was to get to > > user-independent bindings. Rob and Krzysztof certainly labour the point > > that the bindings should not reflect how one operating system's drivers > > would like to see them & u-boot or FreeBSD using a property is grounds > > for it not being removed from the bindings in the linux tree. > > > > I'll go and actually ask Rob. > > I did go & ask Rob, to which he said "I'll apply it even if no driver." > In Linux DT binding or dt schema repo ? I am a bit confused now as we talked about both above :). > Do you want to whip up a binding, or shall I? >
On 30/11/2022 08:13, Atish Kumar Patra wrote: > In Linux DT binding or dt schema repo ? I am a bit confused now as we > talked about both above :). I asked about a dt-binding and not a schema change, so the former ;)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index ff8c0df5cd47..befa9d2c26ac 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -30,6 +30,7 @@ #include "hw/char/serial.h" #include "target/riscv/cpu.h" #include "hw/core/sysbus-fdt.h" +#include "target/riscv/pmu.h" #include "hw/riscv/riscv_hart.h" #include "hw/riscv/virt.h" #include "hw/riscv/boot.h" @@ -708,6 +709,20 @@ static void create_fdt_socket_aplic(RISCVVirtState *s, aplic_phandles[socket] = aplic_s_phandle; } +static void create_fdt_pmu(RISCVVirtState *s) +{ + char *pmu_name; + MachineState *mc = MACHINE(s); + RISCVCPU hart = s->soc[0].harts[0]; + + 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"); + riscv_pmu_generate_fdt_node(mc->fdt, hart.cfg.pmu_num, pmu_name); + + g_free(pmu_name); +} + static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, bool is_32_bit, uint32_t *phandle, uint32_t *irq_mmio_phandle, @@ -1036,6 +1051,7 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, create_fdt_flash(s, memmap); create_fdt_fw_cfg(s, memmap); + create_fdt_pmu(s); update_bootargs: if (cmdline && *cmdline) { diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c index a5f504e53c88..b8e56d2b7b8e 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);