[PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics

Bruno Larsen (billionai) posted 5 patches 4 years, 1 month ago
[PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by Bruno Larsen (billionai) 4 years, 1 month ago
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
     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;
-- 
2.17.1


RE: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by Luis Fernando Fujita Pires 4 years, 1 month ago
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>
Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by Richard Henderson 4 years, 1 month ago
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~

Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by David Gibson 4 years, 1 month ago
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;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by David Gibson 4 years, 1 month ago
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;
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by Bruno Piazera Larsen 4 years, 1 month ago
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)
-- 
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by Greg Kurz 4 years, 1 month ago
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


Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by David Gibson 4 years, 1 month ago
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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by Greg Kurz 4 years, 1 month ago
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;


Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by David Gibson 4 years, 1 month ago
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;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics
Posted by Bruno Piazera Larsen 4 years, 1 month ago
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.

-- 

Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>