| Message ID | 20210526202104.127910-3-bruno.larsen@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | stop collection of instruction usage statistics | expand |
On 5/26/21 1:21 PM, Bruno Larsen (billionai) wrote: > This function requires surce code modification to be useful, which means > it probably is not used often, and the move to using decodetree means > the statistics won't even be collected anymore. > > Also removed setting dump_statistics in ppc_cpu_realize, since it was > only useful when in conjunction with ppc_cpu_dump_statistics. > > Suggested-by: Richard Henderson<richard.henderson@linaro.org> > Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br> > --- > target/ppc/cpu.h | 1 - > target/ppc/cpu_init.c | 3 --- > target/ppc/translate.c | 51 ------------------------------------------ > 3 files changed, 55 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
From: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > This function requires surce code modification to be useful, which means it > probably is not used often, and the move to using decodetree means the > statistics won't even be collected anymore. > > Also removed setting dump_statistics in ppc_cpu_realize, since it was only useful > when in conjunction with ppc_cpu_dump_statistics. > > Suggested-by: Richard Henderson<richard.henderson@linaro.org<mailto:richard.henderson@linaro.org>> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br<mailto:bruno.larsen@eldorado.org.br>> > --- > target/ppc/cpu.h | 1 - > target/ppc/cpu_init.c | 3 --- > target/ppc/translate.c | 51 ------------------------------------------ > 3 files changed, 55 deletions(-) It's not just that ppc_cpu_dump_statistics() wouldn't be useful, but it wouldn't even compile. The change looks good. Reviewed-by: Luis Pires <luis.pires@eldorado.org.br<mailto:luis.pires@eldorado.org.br>> Luis Pires Instituto de Pesquisas ELDORADO<http://www.eldorado.org.br/> Departamento de Computação Embarcada Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>
On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote: > This function requires surce code modification to be useful, which means > it probably is not used often, and the move to using decodetree means > the statistics won't even be collected anymore. > > Also removed setting dump_statistics in ppc_cpu_realize, since it was > only useful when in conjunction with ppc_cpu_dump_statistics. > > Suggested-by: Richard Henderson<richard.henderson@linaro.org> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > --- > target/ppc/cpu.h | 1 - > target/ppc/cpu_init.c | 3 --- > target/ppc/translate.c | 51 ------------------------------------------ > 3 files changed, 55 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 203f07e48e..c3d1b492e4 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, > void ppc_cpu_do_interrupt(CPUState *cpu); > bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); > void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); > -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); > hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); > int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index f5ae2f150d..bd05f53fa4 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > cc->class_by_name = ppc_cpu_class_by_name; > cc->has_work = ppc_cpu_has_work; > cc->dump_state = ppc_cpu_dump_state; > -#ifdef CONFIG_TCG > - cc->dump_statistics = ppc_cpu_dump_statistics; > -#endif This confuses me. The ifdefs you're removing aren't present in my tree, and AFAICT they never existed since your own patch created cpu_init.c. So.. please rebase and check that. > cc->set_pc = ppc_cpu_set_pc; > cc->gdb_read_register = ppc_cpu_gdb_read_register; > cc->gdb_write_register = ppc_cpu_gdb_write_register; > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 6c0f424d81..fc9fd790ca 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -8881,57 +8881,6 @@ int ppc_fixup_cpu(PowerPCCPU *cpu) > return 0; > } > > - > -void ppc_cpu_dump_statistics(CPUState *cs, int flags) > -{ > -#if defined(DO_PPC_STATISTICS) > - PowerPCCPU *cpu = POWERPC_CPU(cs); > - opc_handler_t **t1, **t2, **t3, *handler; > - int op1, op2, op3; > - > - t1 = cpu->env.opcodes; > - for (op1 = 0; op1 < 64; op1++) { > - handler = t1[op1]; > - if (is_indirect_opcode(handler)) { > - t2 = ind_table(handler); > - for (op2 = 0; op2 < 32; op2++) { > - handler = t2[op2]; > - if (is_indirect_opcode(handler)) { > - t3 = ind_table(handler); > - for (op3 = 0; op3 < 32; op3++) { > - handler = t3[op3]; > - if (handler->count == 0) { > - continue; > - } > - qemu_printf("%02x %02x %02x (%02x %04d) %16s: " > - "%016" PRIx64 " %" PRId64 "\n", > - op1, op2, op3, op1, (op3 << 5) | op2, > - handler->oname, > - handler->count, handler->count); > - } > - } else { > - if (handler->count == 0) { > - continue; > - } > - qemu_printf("%02x %02x (%02x %04d) %16s: " > - "%016" PRIx64 " %" PRId64 "\n", > - op1, op2, op1, op2, handler->oname, > - handler->count, handler->count); > - } > - } > - } else { > - if (handler->count == 0) { > - continue; > - } > - qemu_printf("%02x (%02x ) %16s: %016" PRIx64 > - " %" PRId64 "\n", > - op1, op1, handler->oname, > - handler->count, handler->count); > - } > - } > -#endif > -} > - > static bool decode_legacy(PowerPCCPU *cpu, DisasContext *ctx, uint32_t insn) > { > opc_handler_t **table, *handler;
On Thu, May 27, 2021 at 11:20:01AM +1000, David Gibson wrote: > On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote: > > This function requires surce code modification to be useful, which means > > it probably is not used often, and the move to using decodetree means > > the statistics won't even be collected anymore. > > > > Also removed setting dump_statistics in ppc_cpu_realize, since it was > > only useful when in conjunction with ppc_cpu_dump_statistics. > > > > Suggested-by: Richard Henderson<richard.henderson@linaro.org> > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > > --- > > target/ppc/cpu.h | 1 - > > target/ppc/cpu_init.c | 3 --- > > target/ppc/translate.c | 51 ------------------------------------------ > > 3 files changed, 55 deletions(-) > > > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > > index 203f07e48e..c3d1b492e4 100644 > > --- a/target/ppc/cpu.h > > +++ b/target/ppc/cpu.h > > @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, > > void ppc_cpu_do_interrupt(CPUState *cpu); > > bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); > > void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); > > -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); > > hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > > int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); > > int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > > index f5ae2f150d..bd05f53fa4 100644 > > --- a/target/ppc/cpu_init.c > > +++ b/target/ppc/cpu_init.c > > @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > > cc->class_by_name = ppc_cpu_class_by_name; > > cc->has_work = ppc_cpu_has_work; > > cc->dump_state = ppc_cpu_dump_state; > > -#ifdef CONFIG_TCG > > - cc->dump_statistics = ppc_cpu_dump_statistics; > > -#endif > > This confuses me. The ifdefs you're removing aren't present in my > tree, and AFAICT they never existed since your own patch created > cpu_init.c. > > So.. please rebase and check that. Duh, sorry, I looked at this set out of order with your latest !tcg patches. Now that I've applied those, I've applied those one as well. > > > cc->set_pc = ppc_cpu_set_pc; > > cc->gdb_read_register = ppc_cpu_gdb_read_register; > > cc->gdb_write_register = ppc_cpu_gdb_write_register; > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > > index 6c0f424d81..fc9fd790ca 100644 > > --- a/target/ppc/translate.c > > +++ b/target/ppc/translate.c > > @@ -8881,57 +8881,6 @@ int ppc_fixup_cpu(PowerPCCPU *cpu) > > return 0; > > } > > > > - > > -void ppc_cpu_dump_statistics(CPUState *cs, int flags) > > -{ > > -#if defined(DO_PPC_STATISTICS) > > - PowerPCCPU *cpu = POWERPC_CPU(cs); > > - opc_handler_t **t1, **t2, **t3, *handler; > > - int op1, op2, op3; > > - > > - t1 = cpu->env.opcodes; > > - for (op1 = 0; op1 < 64; op1++) { > > - handler = t1[op1]; > > - if (is_indirect_opcode(handler)) { > > - t2 = ind_table(handler); > > - for (op2 = 0; op2 < 32; op2++) { > > - handler = t2[op2]; > > - if (is_indirect_opcode(handler)) { > > - t3 = ind_table(handler); > > - for (op3 = 0; op3 < 32; op3++) { > > - handler = t3[op3]; > > - if (handler->count == 0) { > > - continue; > > - } > > - qemu_printf("%02x %02x %02x (%02x %04d) %16s: " > > - "%016" PRIx64 " %" PRId64 "\n", > > - op1, op2, op3, op1, (op3 << 5) | op2, > > - handler->oname, > > - handler->count, handler->count); > > - } > > - } else { > > - if (handler->count == 0) { > > - continue; > > - } > > - qemu_printf("%02x %02x (%02x %04d) %16s: " > > - "%016" PRIx64 " %" PRId64 "\n", > > - op1, op2, op1, op2, handler->oname, > > - handler->count, handler->count); > > - } > > - } > > - } else { > > - if (handler->count == 0) { > > - continue; > > - } > > - qemu_printf("%02x (%02x ) %16s: %016" PRIx64 > > - " %" PRId64 "\n", > > - op1, op1, handler->oname, > > - handler->count, handler->count); > > - } > > - } > > -#endif > > -} > > - > > static bool decode_legacy(PowerPCCPU *cpu, DisasContext *ctx, uint32_t insn) > > { > > opc_handler_t **table, *handler; >
On Wed, 26 May 2021 17:21:01 -0300 "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote: > This function requires surce code modification to be useful, which means s/surce/source > it probably is not used often, and the move to using decodetree means > the statistics won't even be collected anymore. > > Also removed setting dump_statistics in ppc_cpu_realize, since it was s/ppc_cpu_realize/ppc_cpu_class_init > only useful when in conjunction with ppc_cpu_dump_statistics. > > Suggested-by: Richard Henderson<richard.henderson@linaro.org> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > --- > target/ppc/cpu.h | 1 - > target/ppc/cpu_init.c | 3 --- > target/ppc/translate.c | 51 ------------------------------------------ > 3 files changed, 55 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 203f07e48e..c3d1b492e4 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, > void ppc_cpu_do_interrupt(CPUState *cpu); > bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); > void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); > -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); > hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); > int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index f5ae2f150d..bd05f53fa4 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > cc->class_by_name = ppc_cpu_class_by_name; > cc->has_work = ppc_cpu_has_work; > cc->dump_state = ppc_cpu_dump_state; > -#ifdef CONFIG_TCG > - cc->dump_statistics = ppc_cpu_dump_statistics; > -#endif > cc->set_pc = ppc_cpu_set_pc; > cc->gdb_read_register = ppc_cpu_gdb_read_register; > cc->gdb_write_register = ppc_cpu_gdb_write_register; > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 6c0f424d81..fc9fd790ca 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -8881,57 +8881,6 @@ int ppc_fixup_cpu(PowerPCCPU *cpu) > return 0; > } > > - > -void ppc_cpu_dump_statistics(CPUState *cs, int flags) > -{ > -#if defined(DO_PPC_STATISTICS) > - PowerPCCPU *cpu = POWERPC_CPU(cs); > - opc_handler_t **t1, **t2, **t3, *handler; > - int op1, op2, op3; > - > - t1 = cpu->env.opcodes; > - for (op1 = 0; op1 < 64; op1++) { > - handler = t1[op1]; > - if (is_indirect_opcode(handler)) { > - t2 = ind_table(handler); > - for (op2 = 0; op2 < 32; op2++) { > - handler = t2[op2]; > - if (is_indirect_opcode(handler)) { > - t3 = ind_table(handler); > - for (op3 = 0; op3 < 32; op3++) { > - handler = t3[op3]; > - if (handler->count == 0) { > - continue; > - } > - qemu_printf("%02x %02x %02x (%02x %04d) %16s: " > - "%016" PRIx64 " %" PRId64 "\n", > - op1, op2, op3, op1, (op3 << 5) | op2, > - handler->oname, > - handler->count, handler->count); > - } > - } else { > - if (handler->count == 0) { > - continue; > - } > - qemu_printf("%02x %02x (%02x %04d) %16s: " > - "%016" PRIx64 " %" PRId64 "\n", > - op1, op2, op1, op2, handler->oname, > - handler->count, handler->count); > - } > - } > - } else { > - if (handler->count == 0) { > - continue; > - } > - qemu_printf("%02x (%02x ) %16s: %016" PRIx64 > - " %" PRId64 "\n", > - op1, op1, handler->oname, > - handler->count, handler->count); > - } > - } > -#endif > -} > - > static bool decode_legacy(PowerPCCPU *cpu, DisasContext *ctx, uint32_t insn) > { > opc_handler_t **table, *handler;
On 27/05/2021 01:35, David Gibson wrote: > On Thu, May 27, 2021 at 11:20:01AM +1000, David Gibson wrote: >> On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote: >>> This function requires surce code modification to be useful, which means >>> it probably is not used often, and the move to using decodetree means >>> the statistics won't even be collected anymore. >>> >>> Also removed setting dump_statistics in ppc_cpu_realize, since it was >>> only useful when in conjunction with ppc_cpu_dump_statistics. >>> >>> Suggested-by: Richard Henderson<richard.henderson@linaro.org> >>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> >>> --- >>> target/ppc/cpu.h | 1 - >>> target/ppc/cpu_init.c | 3 --- >>> target/ppc/translate.c | 51 ------------------------------------------ >>> 3 files changed, 55 deletions(-) >>> >>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h >>> index 203f07e48e..c3d1b492e4 100644 >>> --- a/target/ppc/cpu.h >>> +++ b/target/ppc/cpu.h >>> @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, >>> void ppc_cpu_do_interrupt(CPUState *cpu); >>> bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); >>> void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); >>> -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); >>> hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); >>> int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); >>> int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); >>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c >>> index f5ae2f150d..bd05f53fa4 100644 >>> --- a/target/ppc/cpu_init.c >>> +++ b/target/ppc/cpu_init.c >>> @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) >>> cc->class_by_name = ppc_cpu_class_by_name; >>> cc->has_work = ppc_cpu_has_work; >>> cc->dump_state = ppc_cpu_dump_state; >>> -#ifdef CONFIG_TCG >>> - cc->dump_statistics = ppc_cpu_dump_statistics; >>> -#endif >> This confuses me. The ifdefs you're removing aren't present in my >> tree, and AFAICT they never existed since your own patch created >> cpu_init.c. >> >> So.. please rebase and check that. > Duh, sorry, I looked at this set out of order with your latest !tcg > patches. Now that I've applied those, I've applied those one as well. Let me just check, where do you keep your most updated tree? I'm rebasing on your github tree, but ppc-for-6.1 there seems quite outdated (still the same as main)
On Thu, 27 May 2021 10:22:50 -0300 Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> wrote: > > On 27/05/2021 01:35, David Gibson wrote: > > On Thu, May 27, 2021 at 11:20:01AM +1000, David Gibson wrote: > >> On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote: > >>> This function requires surce code modification to be useful, which means > >>> it probably is not used often, and the move to using decodetree means > >>> the statistics won't even be collected anymore. > >>> > >>> Also removed setting dump_statistics in ppc_cpu_realize, since it was > >>> only useful when in conjunction with ppc_cpu_dump_statistics. > >>> > >>> Suggested-by: Richard Henderson<richard.henderson@linaro.org> > >>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > >>> --- > >>> target/ppc/cpu.h | 1 - > >>> target/ppc/cpu_init.c | 3 --- > >>> target/ppc/translate.c | 51 ------------------------------------------ > >>> 3 files changed, 55 deletions(-) > >>> > >>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > >>> index 203f07e48e..c3d1b492e4 100644 > >>> --- a/target/ppc/cpu.h > >>> +++ b/target/ppc/cpu.h > >>> @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, > >>> void ppc_cpu_do_interrupt(CPUState *cpu); > >>> bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); > >>> void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); > >>> -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); > >>> hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > >>> int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); > >>> int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); > >>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > >>> index f5ae2f150d..bd05f53fa4 100644 > >>> --- a/target/ppc/cpu_init.c > >>> +++ b/target/ppc/cpu_init.c > >>> @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > >>> cc->class_by_name = ppc_cpu_class_by_name; > >>> cc->has_work = ppc_cpu_has_work; > >>> cc->dump_state = ppc_cpu_dump_state; > >>> -#ifdef CONFIG_TCG > >>> - cc->dump_statistics = ppc_cpu_dump_statistics; > >>> -#endif > >> This confuses me. The ifdefs you're removing aren't present in my > >> tree, and AFAICT they never existed since your own patch created > >> cpu_init.c. > >> > >> So.. please rebase and check that. > > Duh, sorry, I looked at this set out of order with your latest !tcg > > patches. Now that I've applied those, I've applied those one as well. > Let me just check, where do you keep your most updated tree? I'm > rebasing on your github tree, but ppc-for-6.1 there seems quite outdated > (still the same as main) Try here: https://gitlab.com/dgibson/qemu/-/commits/ppc-for-6.1/ Cheers, -- Greg
On Thu, May 27, 2021 at 04:01:52PM +0200, Greg Kurz wrote: > On Thu, 27 May 2021 10:22:50 -0300 > Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> wrote: > > > > > On 27/05/2021 01:35, David Gibson wrote: > > > On Thu, May 27, 2021 at 11:20:01AM +1000, David Gibson wrote: > > >> On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote: > > >>> This function requires surce code modification to be useful, which means > > >>> it probably is not used often, and the move to using decodetree means > > >>> the statistics won't even be collected anymore. > > >>> > > >>> Also removed setting dump_statistics in ppc_cpu_realize, since it was > > >>> only useful when in conjunction with ppc_cpu_dump_statistics. > > >>> > > >>> Suggested-by: Richard Henderson<richard.henderson@linaro.org> > > >>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > > >>> --- > > >>> target/ppc/cpu.h | 1 - > > >>> target/ppc/cpu_init.c | 3 --- > > >>> target/ppc/translate.c | 51 ------------------------------------------ > > >>> 3 files changed, 55 deletions(-) > > >>> > > >>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > > >>> index 203f07e48e..c3d1b492e4 100644 > > >>> --- a/target/ppc/cpu.h > > >>> +++ b/target/ppc/cpu.h > > >>> @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, > > >>> void ppc_cpu_do_interrupt(CPUState *cpu); > > >>> bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); > > >>> void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); > > >>> -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); > > >>> hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > > >>> int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); > > >>> int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); > > >>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > > >>> index f5ae2f150d..bd05f53fa4 100644 > > >>> --- a/target/ppc/cpu_init.c > > >>> +++ b/target/ppc/cpu_init.c > > >>> @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > > >>> cc->class_by_name = ppc_cpu_class_by_name; > > >>> cc->has_work = ppc_cpu_has_work; > > >>> cc->dump_state = ppc_cpu_dump_state; > > >>> -#ifdef CONFIG_TCG > > >>> - cc->dump_statistics = ppc_cpu_dump_statistics; > > >>> -#endif > > >> This confuses me. The ifdefs you're removing aren't present in my > > >> tree, and AFAICT they never existed since your own patch created > > >> cpu_init.c. > > >> > > >> So.. please rebase and check that. > > > Duh, sorry, I looked at this set out of order with your latest !tcg > > > patches. Now that I've applied those, I've applied those one as well. > > Let me just check, where do you keep your most updated tree? I'm > > rebasing on your github tree, but ppc-for-6.1 there seems quite outdated > > (still the same as main) > > Try here: > > https://gitlab.com/dgibson/qemu/-/commits/ppc-for-6.1/ Right, I moved to gitlab a while back. I sometimes push to the old github tree as well, but I don't already remember.
On Thu, May 27, 2021 at 08:01:56AM +0200, Greg Kurz wrote: > On Wed, 26 May 2021 17:21:01 -0300 > "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote: > > > This function requires surce code modification to be useful, which means > > s/surce/source > > > it probably is not used often, and the move to using decodetree means > > the statistics won't even be collected anymore. > > > > Also removed setting dump_statistics in ppc_cpu_realize, since it was > > s/ppc_cpu_realize/ppc_cpu_class_init A rebase on main has triggered a conflict with this patch, so I've removed it from my tree again. > > > only useful when in conjunction with ppc_cpu_dump_statistics. > > > > Suggested-by: Richard Henderson<richard.henderson@linaro.org> > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > > --- > > target/ppc/cpu.h | 1 - > > target/ppc/cpu_init.c | 3 --- > > target/ppc/translate.c | 51 ------------------------------------------ > > 3 files changed, 55 deletions(-) > > > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > > index 203f07e48e..c3d1b492e4 100644 > > --- a/target/ppc/cpu.h > > +++ b/target/ppc/cpu.h > > @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, > > void ppc_cpu_do_interrupt(CPUState *cpu); > > bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); > > void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); > > -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); > > hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > > int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); > > int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > > index f5ae2f150d..bd05f53fa4 100644 > > --- a/target/ppc/cpu_init.c > > +++ b/target/ppc/cpu_init.c > > @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > > cc->class_by_name = ppc_cpu_class_by_name; > > cc->has_work = ppc_cpu_has_work; > > cc->dump_state = ppc_cpu_dump_state; > > -#ifdef CONFIG_TCG > > - cc->dump_statistics = ppc_cpu_dump_statistics; > > -#endif > > cc->set_pc = ppc_cpu_set_pc; > > cc->gdb_read_register = ppc_cpu_gdb_read_register; > > cc->gdb_write_register = ppc_cpu_gdb_write_register; > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > > index 6c0f424d81..fc9fd790ca 100644 > > --- a/target/ppc/translate.c > > +++ b/target/ppc/translate.c > > @@ -8881,57 +8881,6 @@ int ppc_fixup_cpu(PowerPCCPU *cpu) > > return 0; > > } > > > > - > > -void ppc_cpu_dump_statistics(CPUState *cs, int flags) > > -{ > > -#if defined(DO_PPC_STATISTICS) > > - PowerPCCPU *cpu = POWERPC_CPU(cs); > > - opc_handler_t **t1, **t2, **t3, *handler; > > - int op1, op2, op3; > > - > > - t1 = cpu->env.opcodes; > > - for (op1 = 0; op1 < 64; op1++) { > > - handler = t1[op1]; > > - if (is_indirect_opcode(handler)) { > > - t2 = ind_table(handler); > > - for (op2 = 0; op2 < 32; op2++) { > > - handler = t2[op2]; > > - if (is_indirect_opcode(handler)) { > > - t3 = ind_table(handler); > > - for (op3 = 0; op3 < 32; op3++) { > > - handler = t3[op3]; > > - if (handler->count == 0) { > > - continue; > > - } > > - qemu_printf("%02x %02x %02x (%02x %04d) %16s: " > > - "%016" PRIx64 " %" PRId64 "\n", > > - op1, op2, op3, op1, (op3 << 5) | op2, > > - handler->oname, > > - handler->count, handler->count); > > - } > > - } else { > > - if (handler->count == 0) { > > - continue; > > - } > > - qemu_printf("%02x %02x (%02x %04d) %16s: " > > - "%016" PRIx64 " %" PRId64 "\n", > > - op1, op2, op1, op2, handler->oname, > > - handler->count, handler->count); > > - } > > - } > > - } else { > > - if (handler->count == 0) { > > - continue; > > - } > > - qemu_printf("%02x (%02x ) %16s: %016" PRIx64 > > - " %" PRId64 "\n", > > - op1, op1, handler->oname, > > - handler->count, handler->count); > > - } > > - } > > -#endif > > -} > > - > > static bool decode_legacy(PowerPCCPU *cpu, DisasContext *ctx, uint32_t insn) > > { > > opc_handler_t **table, *handler; >
On 29/05/2021 02:47, David Gibson wrote: > On Thu, May 27, 2021 at 08:01:56AM +0200, Greg Kurz wrote: >> On Wed, 26 May 2021 17:21:01 -0300 >> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote: >> >>> This function requires surce code modification to be useful, which means >> s/surce/source >> >>> it probably is not used often, and the move to using decodetree means >>> the statistics won't even be collected anymore. >>> >>> Also removed setting dump_statistics in ppc_cpu_realize, since it was >> s/ppc_cpu_realize/ppc_cpu_class_init > A rebase on main has triggered a conflict with this patch, so I've > removed it from my tree again. Did you answer to the correct patch? From what I can see, this patch is still in, but patch 5 is not. I fixed the rebase problem that that one had and will send it later, with the rest of the patch series.
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 203f07e48e..c3d1b492e4 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass, void ppc_cpu_do_interrupt(CPUState *cpu); bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req); void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags); -void ppc_cpu_dump_statistics(CPUState *cpu, int flags); hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg); int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg); diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index f5ae2f150d..bd05f53fa4 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) cc->class_by_name = ppc_cpu_class_by_name; cc->has_work = ppc_cpu_has_work; cc->dump_state = ppc_cpu_dump_state; -#ifdef CONFIG_TCG - cc->dump_statistics = ppc_cpu_dump_statistics; -#endif cc->set_pc = ppc_cpu_set_pc; cc->gdb_read_register = ppc_cpu_gdb_read_register; cc->gdb_write_register = ppc_cpu_gdb_write_register; diff --git a/target/ppc/translate.c b/target/ppc/translate.c index 6c0f424d81..fc9fd790ca 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -8881,57 +8881,6 @@ int ppc_fixup_cpu(PowerPCCPU *cpu) return 0; } - -void ppc_cpu_dump_statistics(CPUState *cs, int flags) -{ -#if defined(DO_PPC_STATISTICS) - PowerPCCPU *cpu = POWERPC_CPU(cs); - opc_handler_t **t1, **t2, **t3, *handler; - int op1, op2, op3; - - t1 = cpu->env.opcodes; - for (op1 = 0; op1 < 64; op1++) { - handler = t1[op1]; - if (is_indirect_opcode(handler)) { - t2 = ind_table(handler); - for (op2 = 0; op2 < 32; op2++) { - handler = t2[op2]; - if (is_indirect_opcode(handler)) { - t3 = ind_table(handler); - for (op3 = 0; op3 < 32; op3++) { - handler = t3[op3]; - if (handler->count == 0) { - continue; - } - qemu_printf("%02x %02x %02x (%02x %04d) %16s: " - "%016" PRIx64 " %" PRId64 "\n", - op1, op2, op3, op1, (op3 << 5) | op2, - handler->oname, - handler->count, handler->count); - } - } else { - if (handler->count == 0) { - continue; - } - qemu_printf("%02x %02x (%02x %04d) %16s: " - "%016" PRIx64 " %" PRId64 "\n", - op1, op2, op1, op2, handler->oname, - handler->count, handler->count); - } - } - } else { - if (handler->count == 0) { - continue; - } - qemu_printf("%02x (%02x ) %16s: %016" PRIx64 - " %" PRId64 "\n", - op1, op1, handler->oname, - handler->count, handler->count); - } - } -#endif -} - static bool decode_legacy(PowerPCCPU *cpu, DisasContext *ctx, uint32_t insn) { opc_handler_t **table, *handler;
This function requires surce code modification to be useful, which means it probably is not used often, and the move to using decodetree means the statistics won't even be collected anymore. Also removed setting dump_statistics in ppc_cpu_realize, since it was only useful when in conjunction with ppc_cpu_dump_statistics. Suggested-by: Richard Henderson<richard.henderson@linaro.org> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> --- target/ppc/cpu.h | 1 - target/ppc/cpu_init.c | 3 --- target/ppc/translate.c | 51 ------------------------------------------ 3 files changed, 55 deletions(-)