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 ?
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
> >
> >
>
