diff mbox series

[v13,3/6] target/riscv: Add few cache related PMU events

Message ID 20220816232321.558250-4-atishp@rivosinc.com
State New
Headers show
Series Improve PMU support | expand

Commit Message

Atish Kumar Patra Aug. 16, 2022, 11:23 p.m. UTC 
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>
---
 target/riscv/cpu_helper.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Alistair Francis Aug. 23, 2022, 12:38 a.m. UTC | #1 
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

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
>
>
Atish Kumar Patra Aug. 23, 2022, 7:19 p.m. UTC | #2 
On Mon, Aug 22, 2022 at 5:38 PM Alistair Francis <alistair23@gmail.com>
wrote:

> 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
> >
> >
>
Alistair Francis Aug. 23, 2022, 9 p.m. UTC | #3 
On Wed, Aug 24, 2022 at 5:19 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
>
>
> On Mon, Aug 22, 2022 at 5:38 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> 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 ?

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
>> >
>> >
diff mbox series

Patch

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);