On Wed, Aug 24, 2022 at 5:19 AM Atish Kumar Patra <ati...@rivosinc.com> wrote: > > > > On Mon, Aug 22, 2022 at 5:38 PM Alistair Francis <alistai...@gmail.com> wrote: >> >> On Wed, Aug 17, 2022 at 9:24 AM Atish Patra <ati...@rivosinc.com> wrote: >> > >> > From: Atish Patra <atish.pa...@wdc.com> >> > >> > Qemu can monitor the following cache related PMU events through >> > tlb_fill functions. >> > >> > 1. DTLB load/store miss >> > 3. ITLB prefetch miss >> > >> > Increment the PMU counter in tlb_fill function. >> > >> > Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> >> > Tested-by: Heiko Stuebner <he...@sntech.de> >> > Signed-off-by: Atish Patra <atish.pa...@wdc.com> >> > Signed-off-by: Atish Patra <ati...@rivosinc.com> >> >> This patch causes a number of test failures. >> >> On some boots I see lots of these errors printed: >> >> qemu-system-riscv64: GLib: g_hash_table_lookup: assertion 'hash_table >> != NULL' failed >> >> and I'm unable to boot Linux. >> >> The command line is: >> >> qemu-system-riscv64 \ >> -machine sifive_u \ >> -serial mon:stdio -serial null -nographic \ >> -append "root=/dev/ram rw highres=off console=ttySIF0 ip=dhcp >> earlycon=sbi" \ >> -smp 5 \ >> -d guest_errors \ >> -kernel ./images/qemuriscv64/Image \ >> -initrd ./images/qemuriscv64/buildroot/rootfs.cpio \ >> -bios default -m 256M >> >> I see that faiulre with the 5.19-rc7 kernel and OpenSBI 1.1. >> >> I also see the same messages on other machines when running baremetal >> code. I'm going to drop these patches from my tree >> > > Sorry for the breakage. This should fix the issue. We just need to add few > sanity checks > for the platforms that don't support these events. > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c > index ad33b81b2ea0..0a473010cadd 100644 > --- a/target/riscv/pmu.c > +++ b/target/riscv/pmu.c > @@ -187,6 +187,9 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum > riscv_pmu_event_idx event_idx) > CPURISCVState *env = &cpu->env; > gpointer value; > > + if (!cpu->cfg.pmu_num) > + return 0; > + > value = g_hash_table_lookup(cpu->pmu_event_ctr_map, > GUINT_TO_POINTER(event_idx)); > if (!value) { > @@ -221,6 +224,9 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState > *env, > } > > cpu = RISCV_CPU(env_cpu(env)); > + if (!cpu->pmu_event_ctr_map) > + return false; > + > event_idx = RISCV_PMU_EVENT_HW_INSTRUCTIONS; > ctr_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_event_ctr_map, > GUINT_TO_POINTER(event_idx))); > @@ -243,6 +249,9 @@ bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, > uint32_t target_ctr) > } > > cpu = RISCV_CPU(env_cpu(env)); > + if (!cpu->pmu_event_ctr_map) > + return false; > + > event_idx = RISCV_PMU_EVENT_HW_CPU_CYCLES; > ctr_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_event_ctr_map, > GUINT_TO_POINTER(event_idx))); > @@ -280,7 +289,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, > uint64_t value, > uint32_t event_idx; > RISCVCPU *cpu = RISCV_CPU(env_cpu(env)); > > - if (!riscv_pmu_counter_valid(cpu, ctr_idx)) { > + if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->pmu_event_ctr_map) { > return -1; > } > > Should I respin the series or send this as a fix ?
Can you wait till tomorrow, rebase on my branch and then send a new series? I'm just chasing down another issue today Alistair > >> Alistair >> >> > --- >> > target/riscv/cpu_helper.c | 25 +++++++++++++++++++++++++ >> > 1 file changed, 25 insertions(+) >> > >> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> > index 1e4faa84e839..81948b37dd9a 100644 >> > --- a/target/riscv/cpu_helper.c >> > +++ b/target/riscv/cpu_helper.c >> > @@ -21,11 +21,13 @@ >> > #include "qemu/log.h" >> > #include "qemu/main-loop.h" >> > #include "cpu.h" >> > +#include "pmu.h" >> > #include "exec/exec-all.h" >> > #include "instmap.h" >> > #include "tcg/tcg-op.h" >> > #include "trace.h" >> > #include "semihosting/common-semi.h" >> > +#include "cpu_bits.h" >> > >> > int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) >> > { >> > @@ -1188,6 +1190,28 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, >> > vaddr addr, >> > cpu_loop_exit_restore(cs, retaddr); >> > } >> > >> > + >> > +static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType >> > access_type) >> > +{ >> > + enum riscv_pmu_event_idx pmu_event_type; >> > + >> > + switch (access_type) { >> > + case MMU_INST_FETCH: >> > + pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS; >> > + break; >> > + case MMU_DATA_LOAD: >> > + pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS; >> > + break; >> > + case MMU_DATA_STORE: >> > + pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS; >> > + break; >> > + default: >> > + return; >> > + } >> > + >> > + riscv_pmu_incr_ctr(cpu, pmu_event_type); >> > +} >> > + >> > bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >> > MMUAccessType access_type, int mmu_idx, >> > bool probe, uintptr_t retaddr) >> > @@ -1286,6 +1310,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, >> > int size, >> > } >> > } >> > } else { >> > + pmu_tlb_fill_incr_ctr(cpu, access_type); >> > /* Single stage lookup */ >> > ret = get_physical_address(env, &pa, &prot, address, NULL, >> > access_type, mmu_idx, true, false, >> > false); >> > -- >> > 2.25.1 >> > >> >