On Wed, Aug 17, 2022 at 9:24 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> From: Atish Patra <atish.patra@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.francis@wdc.com>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Signed-off-by: Atish Patra <atishp@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
>
>