| Message ID | 20230606220200.7EBCC74635C@zero.eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: Implement gathering irq statistics | expand |
On 7/6/23 00:02, BALATON Zoltan wrote: > Count exceptions which can be queried with info irq monitor command. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > target/ppc/cpu.h | 1 + > target/ppc/cpu_init.c | 18 ++++++++++++++++++ > target/ppc/excp_helper.c | 1 + > 3 files changed, 20 insertions(+) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 6/6/23 19:02, BALATON Zoltan wrote: > Count exceptions which can be queried with info irq monitor command. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks, Daniel > target/ppc/cpu.h | 1 + > target/ppc/cpu_init.c | 18 ++++++++++++++++++ > target/ppc/excp_helper.c | 1 + > 3 files changed, 20 insertions(+) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index c7c2a5534c..d3a9197e02 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1194,6 +1194,7 @@ struct CPUArchState { > int error_code; > uint32_t pending_interrupts; > #if !defined(CONFIG_USER_ONLY) > + uint64_t excp_stats[POWERPC_EXCP_NB]; > /* > * This is the IRQ controller, which is implementation dependent and only > * relevant when emulating a complete machine. Note that this isn't used > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index 05bf73296b..716f2b5d64 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -48,6 +48,7 @@ > > #ifndef CONFIG_USER_ONLY > #include "hw/boards.h" > +#include "hw/intc/intc.h" > #endif > > /* #define PPC_DEBUG_SPR */ > @@ -7123,6 +7124,16 @@ static bool ppc_cpu_is_big_endian(CPUState *cs) > return !FIELD_EX64(env->msr, MSR, LE); > } > > +static bool ppc_get_irq_stats(InterruptStatsProvider *obj, > + uint64_t **irq_counts, unsigned int *nb_irqs) > +{ > + CPUPPCState *env = &POWERPC_CPU(obj)->env; > + > + *irq_counts = env->excp_stats; > + *nb_irqs = ARRAY_SIZE(env->excp_stats); > + return true; > +} > + > #ifdef CONFIG_TCG > static void ppc_cpu_exec_enter(CPUState *cs) > { > @@ -7286,6 +7297,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > cc->gdb_write_register = ppc_cpu_gdb_write_register; > #ifndef CONFIG_USER_ONLY > cc->sysemu_ops = &ppc_sysemu_ops; > + INTERRUPT_STATS_PROVIDER_CLASS(oc)->get_statistics = ppc_get_irq_stats; > #endif > > cc->gdb_num_core_regs = 71; > @@ -7323,6 +7335,12 @@ static const TypeInfo ppc_cpu_type_info = { > .abstract = true, > .class_size = sizeof(PowerPCCPUClass), > .class_init = ppc_cpu_class_init, > +#ifndef CONFIG_USER_ONLY > + .interfaces = (InterfaceInfo[]) { > + { TYPE_INTERRUPT_STATS_PROVIDER }, > + { } > + }, > +#endif > }; > > #ifndef CONFIG_USER_ONLY > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index fea9221501..5480d9d2c7 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -1652,6 +1652,7 @@ static void powerpc_excp(PowerPCCPU *cpu, int excp) > qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx > " => %s (%d) error=%02x\n", env->nip, powerpc_excp_name(excp), > excp, env->error_code); > + env->excp_stats[excp]++; > > switch (env->excp_model) { > case POWERPC_EXCP_40x:
On 6/7/23 00:02, BALATON Zoltan wrote: > Count exceptions which can be queried with info irq monitor command. I don't think the TYPE_INTERRUPT_STATS_PROVIDER interface was designed for CPUs. It is more suitable for interrupt controllers. C. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > target/ppc/cpu.h | 1 + > target/ppc/cpu_init.c | 18 ++++++++++++++++++ > target/ppc/excp_helper.c | 1 + > 3 files changed, 20 insertions(+) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index c7c2a5534c..d3a9197e02 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1194,6 +1194,7 @@ struct CPUArchState { > int error_code; > uint32_t pending_interrupts; > #if !defined(CONFIG_USER_ONLY) > + uint64_t excp_stats[POWERPC_EXCP_NB]; > /* > * This is the IRQ controller, which is implementation dependent and only > * relevant when emulating a complete machine. Note that this isn't used > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index 05bf73296b..716f2b5d64 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -48,6 +48,7 @@ > > #ifndef CONFIG_USER_ONLY > #include "hw/boards.h" > +#include "hw/intc/intc.h" > #endif > > /* #define PPC_DEBUG_SPR */ > @@ -7123,6 +7124,16 @@ static bool ppc_cpu_is_big_endian(CPUState *cs) > return !FIELD_EX64(env->msr, MSR, LE); > } > > +static bool ppc_get_irq_stats(InterruptStatsProvider *obj, > + uint64_t **irq_counts, unsigned int *nb_irqs) > +{ > + CPUPPCState *env = &POWERPC_CPU(obj)->env; > + > + *irq_counts = env->excp_stats; > + *nb_irqs = ARRAY_SIZE(env->excp_stats); > + return true; > +} > + > #ifdef CONFIG_TCG > static void ppc_cpu_exec_enter(CPUState *cs) > { > @@ -7286,6 +7297,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > cc->gdb_write_register = ppc_cpu_gdb_write_register; > #ifndef CONFIG_USER_ONLY > cc->sysemu_ops = &ppc_sysemu_ops; > + INTERRUPT_STATS_PROVIDER_CLASS(oc)->get_statistics = ppc_get_irq_stats; > #endif > > cc->gdb_num_core_regs = 71; > @@ -7323,6 +7335,12 @@ static const TypeInfo ppc_cpu_type_info = { > .abstract = true, > .class_size = sizeof(PowerPCCPUClass), > .class_init = ppc_cpu_class_init, > +#ifndef CONFIG_USER_ONLY > + .interfaces = (InterfaceInfo[]) { > + { TYPE_INTERRUPT_STATS_PROVIDER }, > + { } > + }, > +#endif > }; > > #ifndef CONFIG_USER_ONLY > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index fea9221501..5480d9d2c7 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -1652,6 +1652,7 @@ static void powerpc_excp(PowerPCCPU *cpu, int excp) > qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx > " => %s (%d) error=%02x\n", env->nip, powerpc_excp_name(excp), > excp, env->error_code); > + env->excp_stats[excp]++; > > switch (env->excp_model) { > case POWERPC_EXCP_40x:
On Thu, 8 Jun 2023, Cédric Le Goater wrote: > On 6/7/23 00:02, BALATON Zoltan wrote: >> Count exceptions which can be queried with info irq monitor command. > > I don't think the TYPE_INTERRUPT_STATS_PROVIDER interface was designed > for CPUs. It is more suitable for interrupt controllers. True but: - It works and provides useful statistics - At least older PPC CPUs have embedded interrupt controller as the comment in cpu.h shows so is this just a comment, question or you want something changed in this patch? Regards, BALATON Zoltan > C. > >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> target/ppc/cpu.h | 1 + >> target/ppc/cpu_init.c | 18 ++++++++++++++++++ >> target/ppc/excp_helper.c | 1 + >> 3 files changed, 20 insertions(+) >> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h >> index c7c2a5534c..d3a9197e02 100644 >> --- a/target/ppc/cpu.h >> +++ b/target/ppc/cpu.h >> @@ -1194,6 +1194,7 @@ struct CPUArchState { >> int error_code; >> uint32_t pending_interrupts; >> #if !defined(CONFIG_USER_ONLY) >> + uint64_t excp_stats[POWERPC_EXCP_NB]; >> /* >> * This is the IRQ controller, which is implementation dependent and >> only >> * relevant when emulating a complete machine. Note that this isn't >> used >> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c >> index 05bf73296b..716f2b5d64 100644 >> --- a/target/ppc/cpu_init.c >> +++ b/target/ppc/cpu_init.c >> @@ -48,6 +48,7 @@ >> #ifndef CONFIG_USER_ONLY >> #include "hw/boards.h" >> +#include "hw/intc/intc.h" >> #endif >> /* #define PPC_DEBUG_SPR */ >> @@ -7123,6 +7124,16 @@ static bool ppc_cpu_is_big_endian(CPUState *cs) >> return !FIELD_EX64(env->msr, MSR, LE); >> } >> +static bool ppc_get_irq_stats(InterruptStatsProvider *obj, >> + uint64_t **irq_counts, unsigned int >> *nb_irqs) >> +{ >> + CPUPPCState *env = &POWERPC_CPU(obj)->env; >> + >> + *irq_counts = env->excp_stats; >> + *nb_irqs = ARRAY_SIZE(env->excp_stats); >> + return true; >> +} >> + >> #ifdef CONFIG_TCG >> static void ppc_cpu_exec_enter(CPUState *cs) >> { >> @@ -7286,6 +7297,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void >> *data) >> cc->gdb_write_register = ppc_cpu_gdb_write_register; >> #ifndef CONFIG_USER_ONLY >> cc->sysemu_ops = &ppc_sysemu_ops; >> + INTERRUPT_STATS_PROVIDER_CLASS(oc)->get_statistics = >> ppc_get_irq_stats; >> #endif >> cc->gdb_num_core_regs = 71; >> @@ -7323,6 +7335,12 @@ static const TypeInfo ppc_cpu_type_info = { >> .abstract = true, >> .class_size = sizeof(PowerPCCPUClass), >> .class_init = ppc_cpu_class_init, >> +#ifndef CONFIG_USER_ONLY >> + .interfaces = (InterfaceInfo[]) { >> + { TYPE_INTERRUPT_STATS_PROVIDER }, >> + { } >> + }, >> +#endif >> }; >> #ifndef CONFIG_USER_ONLY >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index fea9221501..5480d9d2c7 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -1652,6 +1652,7 @@ static void powerpc_excp(PowerPCCPU *cpu, int excp) >> qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx >> " => %s (%d) error=%02x\n", env->nip, >> powerpc_excp_name(excp), >> excp, env->error_code); >> + env->excp_stats[excp]++; >> switch (env->excp_model) { >> case POWERPC_EXCP_40x: > > >
On 6/8/23 11:34, BALATON Zoltan wrote: > On Thu, 8 Jun 2023, Cédric Le Goater wrote: >> On 6/7/23 00:02, BALATON Zoltan wrote: >>> Count exceptions which can be queried with info irq monitor command. >> >> I don't think the TYPE_INTERRUPT_STATS_PROVIDER interface was designed >> for CPUs. It is more suitable for interrupt controllers. > > True but: > - It works and provides useful statistics > - At least older PPC CPUs have embedded interrupt controller as the comment in cpu.h shows > > so is this just a comment, question or you want something changed in this patch? It was more a general comment. It is also adding a very large array in an even larger structure. I wonder this has some impact on cache misses. C. > > Regards, > BALATON Zoltan > >> C. >> >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> target/ppc/cpu.h | 1 + >>> target/ppc/cpu_init.c | 18 ++++++++++++++++++ >>> target/ppc/excp_helper.c | 1 + >>> 3 files changed, 20 insertions(+) >>> >>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h >>> index c7c2a5534c..d3a9197e02 100644 >>> --- a/target/ppc/cpu.h >>> +++ b/target/ppc/cpu.h >>> @@ -1194,6 +1194,7 @@ struct CPUArchState { >>> int error_code; >>> uint32_t pending_interrupts; >>> #if !defined(CONFIG_USER_ONLY) >>> + uint64_t excp_stats[POWERPC_EXCP_NB]; >>> /* >>> * This is the IRQ controller, which is implementation dependent and only >>> * relevant when emulating a complete machine. Note that this isn't used >>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c >>> index 05bf73296b..716f2b5d64 100644 >>> --- a/target/ppc/cpu_init.c >>> +++ b/target/ppc/cpu_init.c >>> @@ -48,6 +48,7 @@ >>> #ifndef CONFIG_USER_ONLY >>> #include "hw/boards.h" >>> +#include "hw/intc/intc.h" >>> #endif >>> /* #define PPC_DEBUG_SPR */ >>> @@ -7123,6 +7124,16 @@ static bool ppc_cpu_is_big_endian(CPUState *cs) >>> return !FIELD_EX64(env->msr, MSR, LE); >>> } >>> +static bool ppc_get_irq_stats(InterruptStatsProvider *obj, >>> + uint64_t **irq_counts, unsigned int *nb_irqs) >>> +{ >>> + CPUPPCState *env = &POWERPC_CPU(obj)->env; >>> + >>> + *irq_counts = env->excp_stats; >>> + *nb_irqs = ARRAY_SIZE(env->excp_stats); >>> + return true; >>> +} >>> + >>> #ifdef CONFIG_TCG >>> static void ppc_cpu_exec_enter(CPUState *cs) >>> { >>> @@ -7286,6 +7297,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) >>> cc->gdb_write_register = ppc_cpu_gdb_write_register; >>> #ifndef CONFIG_USER_ONLY >>> cc->sysemu_ops = &ppc_sysemu_ops; >>> + INTERRUPT_STATS_PROVIDER_CLASS(oc)->get_statistics = ppc_get_irq_stats; >>> #endif >>> cc->gdb_num_core_regs = 71; >>> @@ -7323,6 +7335,12 @@ static const TypeInfo ppc_cpu_type_info = { >>> .abstract = true, >>> .class_size = sizeof(PowerPCCPUClass), >>> .class_init = ppc_cpu_class_init, >>> +#ifndef CONFIG_USER_ONLY >>> + .interfaces = (InterfaceInfo[]) { >>> + { TYPE_INTERRUPT_STATS_PROVIDER }, >>> + { } >>> + }, >>> +#endif >>> }; >>> #ifndef CONFIG_USER_ONLY >>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>> index fea9221501..5480d9d2c7 100644 >>> --- a/target/ppc/excp_helper.c >>> +++ b/target/ppc/excp_helper.c >>> @@ -1652,6 +1652,7 @@ static void powerpc_excp(PowerPCCPU *cpu, int excp) >>> qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx >>> " => %s (%d) error=%02x\n", env->nip, powerpc_excp_name(excp), >>> excp, env->error_code); >>> + env->excp_stats[excp]++; >>> switch (env->excp_model) { >>> case POWERPC_EXCP_40x: >> >> >>
On Wed, 14 Jun 2023, Cédric Le Goater wrote: > On 6/8/23 11:34, BALATON Zoltan wrote: >> On Thu, 8 Jun 2023, Cédric Le Goater wrote: >>> On 6/7/23 00:02, BALATON Zoltan wrote: >>>> Count exceptions which can be queried with info irq monitor command. >>> >>> I don't think the TYPE_INTERRUPT_STATS_PROVIDER interface was designed >>> for CPUs. It is more suitable for interrupt controllers. >> >> True but: >> - It works and provides useful statistics >> - At least older PPC CPUs have embedded interrupt controller as the comment >> in cpu.h shows >> >> so is this just a comment, question or you want something changed in this >> patch? > > It was more a general comment. It is also adding a very large array > in an even larger structure. I wonder this has some impact on cache > misses. I could not measure a performance decrease at least with the tests I did so it likely has no major impact but that could also be host arch specific. I've found exceptions may be one source of slowness because I see a lot of external interrupts raised with MorphOS on pegasos2 while much less with AmigaOS on the same machine and the latter seems to run faster while same MoprhOS on mac99 does not have these interrupts and runs faster than AmigaOS on pegasos2. I've added these irq counts to be able to get some info on this. This series started as an attempt to optimise this part a bit but could not make it much faster so only the clean ups remained at the end. I have more changes that in theory could help but tests showed it made things slower so I've dropped those. These were changing cs parameter to env in other functions such as powerpc_reset_excp_state(), powerpc_set_excp_state(), ppc_interrupts_little_endian(), powerpc_excp() and all its model specific variants to get rid of local cs variables but this made it slower. Or marking error checks with unlikely that also made it slower despite I heven't seen any of those errors firing so it's hard to tell which change would have a performance impact. I've tested that these in this series don't change things much, I still get about the same speed as before. Regards, BALATON Zoltan
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index c7c2a5534c..d3a9197e02 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1194,6 +1194,7 @@ struct CPUArchState { int error_code; uint32_t pending_interrupts; #if !defined(CONFIG_USER_ONLY) + uint64_t excp_stats[POWERPC_EXCP_NB]; /* * This is the IRQ controller, which is implementation dependent and only * relevant when emulating a complete machine. Note that this isn't used diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index 05bf73296b..716f2b5d64 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -48,6 +48,7 @@ #ifndef CONFIG_USER_ONLY #include "hw/boards.h" +#include "hw/intc/intc.h" #endif /* #define PPC_DEBUG_SPR */ @@ -7123,6 +7124,16 @@ static bool ppc_cpu_is_big_endian(CPUState *cs) return !FIELD_EX64(env->msr, MSR, LE); } +static bool ppc_get_irq_stats(InterruptStatsProvider *obj, + uint64_t **irq_counts, unsigned int *nb_irqs) +{ + CPUPPCState *env = &POWERPC_CPU(obj)->env; + + *irq_counts = env->excp_stats; + *nb_irqs = ARRAY_SIZE(env->excp_stats); + return true; +} + #ifdef CONFIG_TCG static void ppc_cpu_exec_enter(CPUState *cs) { @@ -7286,6 +7297,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_write_register = ppc_cpu_gdb_write_register; #ifndef CONFIG_USER_ONLY cc->sysemu_ops = &ppc_sysemu_ops; + INTERRUPT_STATS_PROVIDER_CLASS(oc)->get_statistics = ppc_get_irq_stats; #endif cc->gdb_num_core_regs = 71; @@ -7323,6 +7335,12 @@ static const TypeInfo ppc_cpu_type_info = { .abstract = true, .class_size = sizeof(PowerPCCPUClass), .class_init = ppc_cpu_class_init, +#ifndef CONFIG_USER_ONLY + .interfaces = (InterfaceInfo[]) { + { TYPE_INTERRUPT_STATS_PROVIDER }, + { } + }, +#endif }; #ifndef CONFIG_USER_ONLY diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index fea9221501..5480d9d2c7 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -1652,6 +1652,7 @@ static void powerpc_excp(PowerPCCPU *cpu, int excp) qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx " => %s (%d) error=%02x\n", env->nip, powerpc_excp_name(excp), excp, env->error_code); + env->excp_stats[excp]++; switch (env->excp_model) { case POWERPC_EXCP_40x:
Count exceptions which can be queried with info irq monitor command. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- target/ppc/cpu.h | 1 + target/ppc/cpu_init.c | 18 ++++++++++++++++++ target/ppc/excp_helper.c | 1 + 3 files changed, 20 insertions(+)