[PATCH 0/5] stop collection of instruction usage statistics

[PATCH 0/5] stop collection of instruction usage statistics

[Bruno Larsen (billionai)]

Based-on: <20210525115355.8254-1-bruno.larsen@eldorado.org.br>

The functionality of counting how many instructions were being executed and
being able to show it through the monitor, although neat, was only
supported by ppc, and now that it is migrating to use decodetree (at
least partially), those statistics won't be used anymore. Therefore,
this patch removes that functinality completely.

This series was suggested by Richard Henderson

Bruno Larsen (billionai) (5):
  target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set
  target/ppc: remove ppc_cpu_dump_statistics
  target/ppc: removed mentions to DO_PPC_STATISTICS
  monitor: removed cpustats command
  hw/core/cpu: removed cpu_dump_statistics function

 hmp-commands-info.hx   | 13 --------
 hw/core/cpu.c          |  9 ------
 include/hw/core/cpu.h  | 12 --------
 monitor/misc.c         | 11 -------
 target/ppc/cpu.h       |  1 -
 target/ppc/cpu_init.c  |  3 --
 target/ppc/translate.c | 69 +++---------------------------------------
 7 files changed, 5 insertions(+), 113 deletions(-)

-- 
2.17.1

[PATCH 1/5] target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set

[Bruno Larsen (billionai)]

Before this patch, when PPC_DUMP_CPU is set, oname is added to
opc_handler_t, but GEN_OPCODE* wouldn't set it unless DO_PPC_STATISTICS
was set as well.

This patch changes it so those changes would happen when PPC_DUMP_CPU is
set, but not statistics, because the latter is being removed.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index ea200f9637..6c0f424d81 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1345,7 +1345,7 @@ typedef struct opcode_t {
 /*****************************************************************************/
 /* PowerPC instructions table                                                */
 
-#if defined(DO_PPC_STATISTICS)
+#if defined(PPC_DUMP_CPU)
 #define GEN_OPCODE(name, op1, op2, op3, invl, _typ, _typ2)                    \
 {                                                                             \
     .opc1 = op1,                                                              \
-- 
2.17.1

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

[Bruno Larsen (billionai)]

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

[PATCH 3/5] target/ppc: removed mentions to DO_PPC_STATISTICS

[Bruno Larsen (billionai)]

Removed the commented out definition and all ifdefs relating to
PPC_DUMP_STATISTICS, as it's hardly ever used.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/translate.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index fc9fd790ca..0525e1939f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -47,7 +47,6 @@
 
 /* Include definitions for instructions classes and implementations flags */
 /* #define PPC_DEBUG_DISAS */
-/* #define DO_PPC_STATISTICS */
 
 #ifdef PPC_DEBUG_DISAS
 #  define LOG_DISAS(...) qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__)
@@ -217,12 +216,9 @@ struct opc_handler_t {
     uint64_t type2;
     /* handler */
     void (*handler)(DisasContext *ctx);
-#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
+#if defined(PPC_DUMP_CPU)
     const char *oname;
 #endif
-#if defined(DO_PPC_STATISTICS)
-    uint64_t count;
-#endif
 };
 
 /* SPR load/store helpers */
@@ -8546,7 +8542,7 @@ static int register_direct_insn(opc_handler_t **ppc_opcodes,
     if (insert_in_table(ppc_opcodes, idx, handler) < 0) {
         printf("*** ERROR: opcode %02x already assigned in main "
                "opcode table\n", idx);
-#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
+#if defined(PPC_DUMP_CPU)
         printf("           Registered handler '%s' - new handler '%s'\n",
                ppc_opcodes[idx]->oname, handler->oname);
 #endif
@@ -8570,7 +8566,7 @@ static int register_ind_in_table(opc_handler_t **table,
         if (!is_indirect_opcode(table[idx1])) {
             printf("*** ERROR: idx %02x already assigned to a direct "
                    "opcode\n", idx1);
-#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
+#if defined(PPC_DUMP_CPU)
             printf("           Registered handler '%s' - new handler '%s'\n",
                    ind_table(table[idx1])[idx2]->oname, handler->oname);
 #endif
@@ -8581,7 +8577,7 @@ static int register_ind_in_table(opc_handler_t **table,
         insert_in_table(ind_table(table[idx1]), idx2, handler) < 0) {
         printf("*** ERROR: opcode %02x already assigned in "
                "opcode table %02x\n", idx2, idx1);
-#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
+#if defined(PPC_DUMP_CPU)
         printf("           Registered handler '%s' - new handler '%s'\n",
                ind_table(table[idx1])[idx2]->oname, handler->oname);
 #endif
@@ -9036,10 +9032,6 @@ static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
         gen_invalid(ctx);
     }
 
-#if defined(DO_PPC_STATISTICS)
-    handler->count++;
-#endif
-
     translator_loop_temp_check(&ctx->base);
 }
 
-- 
2.17.1

[PATCH 4/5] monitor: removed cpustats command

[Bruno Larsen (billionai)]

Since ppc was the last architecture to collect these statistics and
it is currently phasing this collection out, the command that would query
this information is being removed.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 hmp-commands-info.hx | 13 -------------
 monitor/misc.c       | 11 -----------
 2 files changed, 24 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ab0c7aa5ee..b2347a6aea 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -500,19 +500,6 @@ SRST
     Show the current VM UUID.
 ERST
 
-    {
-        .name       = "cpustats",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show CPU statistics",
-        .cmd        = hmp_info_cpustats,
-    },
-
-SRST
-  ``info cpustats``
-    Show CPU statistics.
-ERST
-
 #if defined(CONFIG_SLIRP)
     {
         .name       = "usernet",
diff --git a/monitor/misc.c b/monitor/misc.c
index f3a393ea59..1539e18557 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
     }
 }
 
-static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
-{
-    CPUState *cs = mon_get_cpu(mon);
-
-    if (!cs) {
-        monitor_printf(mon, "No CPU available\n");
-        return;
-    }
-    cpu_dump_statistics(cs, 0);
-}
-
 static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
 {
     const char *name = qdict_get_try_str(qdict, "name");
-- 
2.17.1

[PATCH 5/5] hw/core/cpu: removed cpu_dump_statistics function

[Bruno Larsen (billionai)]

No more architectures set the pointer to dump_statistics, so there's no
point in keeping it, or the related cpu_dump_statistics function.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 hw/core/cpu.c         |  9 ---------
 include/hw/core/cpu.h | 12 ------------
 2 files changed, 21 deletions(-)

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 00330ba07d..b700d884ad 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -218,15 +218,6 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
     }
 }
 
-void cpu_dump_statistics(CPUState *cpu, int flags)
-{
-    CPUClass *cc = CPU_GET_CLASS(cpu);
-
-    if (cc->dump_statistics) {
-        cc->dump_statistics(cpu, flags);
-    }
-}
-
 void cpu_reset(CPUState *cpu)
 {
     device_cold_reset(DEVICE(cpu));
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index d45f78290e..6d14923206 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -93,7 +93,6 @@ struct AccelCPUClass;
  * not be used by any callers other than the pre-1.0 virtio devices.
  * @memory_rw_debug: Callback for GDB memory access.
  * @dump_state: Callback for dumping state.
- * @dump_statistics: Callback for dumping statistics.
  * @get_arch_id: Callback for getting architecture-dependent CPU ID.
  * @get_paging_enabled: Callback for inquiring whether paging is enabled.
  * @get_memory_mapping: Callback for obtaining the memory mappings.
@@ -155,7 +154,6 @@ struct CPUClass {
                            uint8_t *buf, int len, bool is_write);
     void (*dump_state)(CPUState *cpu, FILE *, int flags);
     GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
-    void (*dump_statistics)(CPUState *cpu, int flags);
     int64_t (*get_arch_id)(CPUState *cpu);
     bool (*get_paging_enabled)(const CPUState *cpu);
     void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list,
@@ -562,16 +560,6 @@ enum CPUDumpFlags {
  */
 void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 
-/**
- * cpu_dump_statistics:
- * @cpu: The CPU whose state is to be dumped.
- * @flags: Flags what to dump.
- *
- * Dump CPU statistics to the current monitor if we have one, else to
- * stdout.
- */
-void cpu_dump_statistics(CPUState *cpu, int flags);
-
 #ifndef CONFIG_USER_ONLY
 /**
  * cpu_get_phys_page_attrs_debug:
-- 
2.17.1

RE: [PATCH 1/5] target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set

[Luis Fernando Fujita Pires]

From: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> Before this patch, when PPC_DUMP_CPU is set, oname is added to
> opc_handler_t, but GEN_OPCODE* wouldn't set it unless DO_PPC_STATISTICS
> was set as well.
> 
> This patch changes it so those changes would happen when PPC_DUMP_CPU is
> set, but not statistics, because the latter is being removed.
> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I suggest removing dump_ppc_insns() altogether and 'oname' along with it.

Now that we're moving to decodetree, dump_ppc_insns() wouldn't show all the available opcodes anyway. And the only other locations where 'oname' is being used are when registering more than one handler for the same opcode by mistake, which won't happen anymore, as any new instructions should use decodetree.

Luis

Re: [PATCH 1/5] target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set

[Richard Henderson]

On 5/26/21 2:13 PM, Luis Fernando Fujita Pires wrote:
> From: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
>> Before this patch, when PPC_DUMP_CPU is set, oname is added to
>> opc_handler_t, but GEN_OPCODE* wouldn't set it unless DO_PPC_STATISTICS
>> was set as well.
>>
>> This patch changes it so those changes would happen when PPC_DUMP_CPU is
>> set, but not statistics, because the latter is being removed.
>>
>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
>> ---
>>   target/ppc/translate.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I suggest removing dump_ppc_insns() altogether and 'oname' along with it.
> 
> Now that we're moving to decodetree, dump_ppc_insns() wouldn't show all the available opcodes anyway. And the only other locations where 'oname' is being used are when registering more than one handler for the same opcode by mistake, which won't happen anymore, as any new instructions should use decodetree.

Agreed.

r~

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

[Richard Henderson]

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 3/5] target/ppc: removed mentions to DO_PPC_STATISTICS

[Richard Henderson]

On 5/26/21 1:21 PM, Bruno Larsen (billionai) wrote:
> Removed the commented out definition and all ifdefs relating to
> PPC_DUMP_STATISTICS, as it's hardly ever used.
> 
> Suggested-by: Richard Henderson<richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   target/ppc/translate.c | 16 ++++------------
>   1 file changed, 4 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Luis Fernando Fujita Pires]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

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>

[-- Attachment #2: Type: text/html, Size: 5006 bytes --]

Re: [PATCH 4/5] monitor: removed cpustats command

[Richard Henderson]

On 5/26/21 1:21 PM, Bruno Larsen (billionai) wrote:
> Since ppc was the last architecture to collect these statistics and
> it is currently phasing this collection out, the command that would query
> this information is being removed.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>   hmp-commands-info.hx | 13 -------------
>   monitor/misc.c       | 11 -----------
>   2 files changed, 24 deletions(-)

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ab0c7aa5ee..b2347a6aea 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -500,19 +500,6 @@ SRST
>       Show the current VM UUID.
>   ERST
>   
> -    {
> -        .name       = "cpustats",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show CPU statistics",
> -        .cmd        = hmp_info_cpustats,
> -    },
> -
> -SRST
> -  ``info cpustats``
> -    Show CPU statistics.
> -ERST
> -
>   #if defined(CONFIG_SLIRP)
>       {
>           .name       = "usernet",
> diff --git a/monitor/misc.c b/monitor/misc.c
> index f3a393ea59..1539e18557 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
>       }
>   }
>   
> -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> -{
> -    CPUState *cs = mon_get_cpu(mon);
> -
> -    if (!cs) {
> -        monitor_printf(mon, "No CPU available\n");
> -        return;
> -    }
> -    cpu_dump_statistics(cs, 0);
> -}
> -
>   static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>   {
>       const char *name = qdict_get_try_str(qdict, "name");
> 

Re: [PATCH 5/5] hw/core/cpu: removed cpu_dump_statistics function

[Richard Henderson]

On 5/26/21 1:21 PM, Bruno Larsen (billionai) wrote:
> No more architectures set the pointer to dump_statistics, so there's no
> point in keeping it, or the related cpu_dump_statistics function.
> 
> Suggested-by: Richard Henderson<richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
>   hw/core/cpu.c         |  9 ---------
>   include/hw/core/cpu.h | 12 ------------
>   2 files changed, 21 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

RE: [PATCH 3/5] target/ppc: removed mentions to DO_PPC_STATISTICS

[Luis Fernando Fujita Pires]

From: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> Removed the commented out definition and all ifdefs relating to
> PPC_DUMP_STATISTICS, as it's hardly ever used.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/translate.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)

The change looks good. Just a minor note: part of the commit msg is wrong, where you mention "PPC_DUMP_STATISTICS". The #define is DO_PPC_STATISTICS, and the function was ppc_cpu_dump_statistics(). You're probably referring to the former.

Reviewed-by: Luis Pires <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 4/5] monitor: removed cpustats command

[Luis Fernando Fujita Pires]

From: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> Since ppc was the last architecture to collect these statistics and it is currently
> phasing this collection out, the command that would query this information is
> being removed.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  hmp-commands-info.hx | 13 -------------
>  monitor/misc.c       | 11 -----------
>  2 files changed, 24 deletions(-)

Reviewed-by: Luis Pires <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 5/5] hw/core/cpu: removed cpu_dump_statistics function

[Luis Fernando Fujita Pires]

From: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> No more architectures set the pointer to dump_statistics, so there's no point in
> keeping it, or the related cpu_dump_statistics function.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  hw/core/cpu.c         |  9 ---------
>  include/hw/core/cpu.h | 12 ------------
>  2 files changed, 21 deletions(-)

Reviewed-by: Luis Pires <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 1/5] target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set

[david]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

On Wed, May 26, 2021 at 02:24:51PM -0700, Richard Henderson wrote:
> On 5/26/21 2:13 PM, Luis Fernando Fujita Pires wrote:
> > From: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > > Before this patch, when PPC_DUMP_CPU is set, oname is added to
> > > opc_handler_t, but GEN_OPCODE* wouldn't set it unless DO_PPC_STATISTICS
> > > was set as well.
> > > 
> > > This patch changes it so those changes would happen when PPC_DUMP_CPU is
> > > set, but not statistics, because the latter is being removed.
> > > 
> > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > > ---
> > >   target/ppc/translate.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > I suggest removing dump_ppc_insns() altogether and 'oname' along with it.
> > 
> > Now that we're moving to decodetree, dump_ppc_insns() wouldn't show all the available opcodes anyway. And the only other locations where 'oname' is being used are when registering more than one handler for the same opcode by mistake, which won't happen anymore, as any new instructions should use decodetree.
> 
> Agreed.

I'll wait for a follow up doing this then.

> 
> r~
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4924 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] hw/core/cpu: removed cpu_dump_statistics function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

On Wed, May 26, 2021 at 05:21:04PM -0300, Bruno Larsen (billionai) wrote:
> No more architectures set the pointer to dump_statistics, so there's no
> point in keeping it, or the related cpu_dump_statistics function.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai)
> <bruno.larsen@eldorado.org.br>

I'm happy enough to stage this through my tree, but an ack from
Eduardo or Marcel would be good to have.

> ---
>  hw/core/cpu.c         |  9 ---------
>  include/hw/core/cpu.h | 12 ------------
>  2 files changed, 21 deletions(-)
> 
> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
> index 00330ba07d..b700d884ad 100644
> --- a/hw/core/cpu.c
> +++ b/hw/core/cpu.c
> @@ -218,15 +218,6 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>      }
>  }
>  
> -void cpu_dump_statistics(CPUState *cpu, int flags)
> -{
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -
> -    if (cc->dump_statistics) {
> -        cc->dump_statistics(cpu, flags);
> -    }
> -}
> -
>  void cpu_reset(CPUState *cpu)
>  {
>      device_cold_reset(DEVICE(cpu));
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index d45f78290e..6d14923206 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -93,7 +93,6 @@ struct AccelCPUClass;
>   * not be used by any callers other than the pre-1.0 virtio devices.
>   * @memory_rw_debug: Callback for GDB memory access.
>   * @dump_state: Callback for dumping state.
> - * @dump_statistics: Callback for dumping statistics.
>   * @get_arch_id: Callback for getting architecture-dependent CPU ID.
>   * @get_paging_enabled: Callback for inquiring whether paging is enabled.
>   * @get_memory_mapping: Callback for obtaining the memory mappings.
> @@ -155,7 +154,6 @@ struct CPUClass {
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
> -    void (*dump_statistics)(CPUState *cpu, int flags);
>      int64_t (*get_arch_id)(CPUState *cpu);
>      bool (*get_paging_enabled)(const CPUState *cpu);
>      void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list,
> @@ -562,16 +560,6 @@ enum CPUDumpFlags {
>   */
>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>  
> -/**
> - * cpu_dump_statistics:
> - * @cpu: The CPU whose state is to be dumped.
> - * @flags: Flags what to dump.
> - *
> - * Dump CPU statistics to the current monitor if we have one, else to
> - * stdout.
> - */
> -void cpu_dump_statistics(CPUState *cpu, int flags);
> -
>  #ifndef CONFIG_USER_ONLY
>  /**
>   * cpu_get_phys_page_attrs_debug:

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5362 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/5] target/ppc: removed mentions to DO_PPC_STATISTICS

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]

On Wed, May 26, 2021 at 05:21:02PM -0300, Bruno Larsen (billionai) wrote:
> Removed the commented out definition and all ifdefs relating to
> PPC_DUMP_STATISTICS, as it's hardly ever used.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai)
> <bruno.larsen@eldorado.org.br>

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/translate.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index fc9fd790ca..0525e1939f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -47,7 +47,6 @@
>  
>  /* Include definitions for instructions classes and implementations flags */
>  /* #define PPC_DEBUG_DISAS */
> -/* #define DO_PPC_STATISTICS */
>  
>  #ifdef PPC_DEBUG_DISAS
>  #  define LOG_DISAS(...) qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__)
> @@ -217,12 +216,9 @@ struct opc_handler_t {
>      uint64_t type2;
>      /* handler */
>      void (*handler)(DisasContext *ctx);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>      const char *oname;
>  #endif
> -#if defined(DO_PPC_STATISTICS)
> -    uint64_t count;
> -#endif
>  };
>  
>  /* SPR load/store helpers */
> @@ -8546,7 +8542,7 @@ static int register_direct_insn(opc_handler_t **ppc_opcodes,
>      if (insert_in_table(ppc_opcodes, idx, handler) < 0) {
>          printf("*** ERROR: opcode %02x already assigned in main "
>                 "opcode table\n", idx);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>          printf("           Registered handler '%s' - new handler '%s'\n",
>                 ppc_opcodes[idx]->oname, handler->oname);
>  #endif
> @@ -8570,7 +8566,7 @@ static int register_ind_in_table(opc_handler_t **table,
>          if (!is_indirect_opcode(table[idx1])) {
>              printf("*** ERROR: idx %02x already assigned to a direct "
>                     "opcode\n", idx1);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>              printf("           Registered handler '%s' - new handler '%s'\n",
>                     ind_table(table[idx1])[idx2]->oname, handler->oname);
>  #endif
> @@ -8581,7 +8577,7 @@ static int register_ind_in_table(opc_handler_t **table,
>          insert_in_table(ind_table(table[idx1]), idx2, handler) < 0) {
>          printf("*** ERROR: opcode %02x already assigned in "
>                 "opcode table %02x\n", idx2, idx1);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>          printf("           Registered handler '%s' - new handler '%s'\n",
>                 ind_table(table[idx1])[idx2]->oname, handler->oname);
>  #endif
> @@ -9036,10 +9032,6 @@ static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
>          gen_invalid(ctx);
>      }
>  
> -#if defined(DO_PPC_STATISTICS)
> -    handler->count++;
> -#endif
> -
>      translator_loop_temp_check(&ctx->base);
>  }
>  

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/5] monitor: removed cpustats command

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Wed, May 26, 2021 at 02:28:48PM -0700, Richard Henderson wrote:
> On 5/26/21 1:21 PM, Bruno Larsen (billionai) wrote:
> > Since ppc was the last architecture to collect these statistics and
> > it is currently phasing this collection out, the command that would query
> > this information is being removed.
> > 
> > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > ---
> >   hmp-commands-info.hx | 13 -------------
> >   monitor/misc.c       | 11 -----------
> >   2 files changed, 24 deletions(-)
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.1.  I'm staging this in my tree, but an Ack from
Dave Gilbert would be appreciated.

> 
> 
> r~
> 
> > 
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index ab0c7aa5ee..b2347a6aea 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -500,19 +500,6 @@ SRST
> >       Show the current VM UUID.
> >   ERST
> > -    {
> > -        .name       = "cpustats",
> > -        .args_type  = "",
> > -        .params     = "",
> > -        .help       = "show CPU statistics",
> > -        .cmd        = hmp_info_cpustats,
> > -    },
> > -
> > -SRST
> > -  ``info cpustats``
> > -    Show CPU statistics.
> > -ERST
> > -
> >   #if defined(CONFIG_SLIRP)
> >       {
> >           .name       = "usernet",
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index f3a393ea59..1539e18557 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
> >       }
> >   }
> > -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> > -{
> > -    CPUState *cs = mon_get_cpu(mon);
> > -
> > -    if (!cs) {
> > -        monitor_printf(mon, "No CPU available\n");
> > -        return;
> > -    }
> > -    cpu_dump_statistics(cs, 0);
> > -}
> > -
> >   static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> >   {
> >       const char *name = qdict_get_try_str(qdict, "name");
> > 
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Greg Kurz]

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 4/5] monitor: removed cpustats command

[Greg Kurz]

On Wed, 26 May 2021 17:21:03 -0300
"Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:

> Since ppc was the last architecture to collect these statistics and
> it is currently phasing this collection out, the command that would query
> this information is being removed.
> 

So this is removing an obviously user visible feature. This should be
mentioned in docs/system/removed-features.rst... but, wait, I don't
see anything for it in docs/system/deprecated.rst. This is dropping
a feature without following the usual deprecation policy, i.e.
marking the feature as deprecated and only remove it 2 QEMU versions
later. Any justification for that ?

David,

Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
but the commit title appears to be broken:

'65;6401;1cmonitor: removed cpustats command

https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69

> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  hmp-commands-info.hx | 13 -------------
>  monitor/misc.c       | 11 -----------
>  2 files changed, 24 deletions(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ab0c7aa5ee..b2347a6aea 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -500,19 +500,6 @@ SRST
>      Show the current VM UUID.
>  ERST
>  
> -    {
> -        .name       = "cpustats",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show CPU statistics",
> -        .cmd        = hmp_info_cpustats,
> -    },
> -
> -SRST
> -  ``info cpustats``
> -    Show CPU statistics.
> -ERST
> -
>  #if defined(CONFIG_SLIRP)
>      {
>          .name       = "usernet",
> diff --git a/monitor/misc.c b/monitor/misc.c
> index f3a393ea59..1539e18557 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> -{
> -    CPUState *cs = mon_get_cpu(mon);
> -
> -    if (!cs) {
> -        monitor_printf(mon, "No CPU available\n");
> -        return;
> -    }
> -    cpu_dump_statistics(cs, 0);
> -}
> -
>  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>  {
>      const char *name = qdict_get_try_str(qdict, "name");

Re: [PATCH 4/5] monitor: removed cpustats command

[Dr. David Alan Gilbert]

* Bruno Larsen (billionai) (bruno.larsen@eldorado.org.br) wrote:
> Since ppc was the last architecture to collect these statistics and
> it is currently phasing this collection out, the command that would query
> this information is being removed.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp-commands-info.hx | 13 -------------
>  monitor/misc.c       | 11 -----------
>  2 files changed, 24 deletions(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ab0c7aa5ee..b2347a6aea 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -500,19 +500,6 @@ SRST
>      Show the current VM UUID.
>  ERST
>  
> -    {
> -        .name       = "cpustats",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show CPU statistics",
> -        .cmd        = hmp_info_cpustats,
> -    },
> -
> -SRST
> -  ``info cpustats``
> -    Show CPU statistics.
> -ERST
> -
>  #if defined(CONFIG_SLIRP)
>      {
>          .name       = "usernet",
> diff --git a/monitor/misc.c b/monitor/misc.c
> index f3a393ea59..1539e18557 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> -{
> -    CPUState *cs = mon_get_cpu(mon);
> -
> -    if (!cs) {
> -        monitor_printf(mon, "No CPU available\n");
> -        return;
> -    }
> -    cpu_dump_statistics(cs, 0);
> -}
> -
>  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>  {
>      const char *name = qdict_get_try_str(qdict, "name");
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 4/5] monitor: removed cpustats command

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Wed, 26 May 2021 17:21:03 -0300
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
> 
> > Since ppc was the last architecture to collect these statistics and
> > it is currently phasing this collection out, the command that would query
> > this information is being removed.
> > 
> 
> So this is removing an obviously user visible feature. This should be
> mentioned in docs/system/removed-features.rst... but, wait, I don't
> see anything for it in docs/system/deprecated.rst. This is dropping
> a feature without following the usual deprecation policy, i.e.
> marking the feature as deprecated and only remove it 2 QEMU versions
> later. Any justification for that ?

As long as the command really isn't useful any more, I wouldn't object
to that from an HMP point of view.

Dave

> David,
> 
> Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
> but the commit title appears to be broken:
> 
> '65;6401;1cmonitor: removed cpustats command
> 
> https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69
> 
> > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > ---
> >  hmp-commands-info.hx | 13 -------------
> >  monitor/misc.c       | 11 -----------
> >  2 files changed, 24 deletions(-)
> > 
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index ab0c7aa5ee..b2347a6aea 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -500,19 +500,6 @@ SRST
> >      Show the current VM UUID.
> >  ERST
> >  
> > -    {
> > -        .name       = "cpustats",
> > -        .args_type  = "",
> > -        .params     = "",
> > -        .help       = "show CPU statistics",
> > -        .cmd        = hmp_info_cpustats,
> > -    },
> > -
> > -SRST
> > -  ``info cpustats``
> > -    Show CPU statistics.
> > -ERST
> > -
> >  #if defined(CONFIG_SLIRP)
> >      {
> >          .name       = "usernet",
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index f3a393ea59..1539e18557 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
> >      }
> >  }
> >  
> > -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> > -{
> > -    CPUState *cs = mon_get_cpu(mon);
> > -
> > -    if (!cs) {
> > -        monitor_printf(mon, "No CPU available\n");
> > -        return;
> > -    }
> > -    cpu_dump_statistics(cs, 0);
> > -}
> > -
> >  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> >  {
> >      const char *name = qdict_get_try_str(qdict, "name");
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 4/5] monitor: removed cpustats command

[Greg Kurz]

On Thu, 27 May 2021 09:09:55 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Greg Kurz (groug@kaod.org) wrote:
> > On Wed, 26 May 2021 17:21:03 -0300
> > "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
> > 
> > > Since ppc was the last architecture to collect these statistics and
> > > it is currently phasing this collection out, the command that would query
> > > this information is being removed.
> > > 
> > 
> > So this is removing an obviously user visible feature. This should be
> > mentioned in docs/system/removed-features.rst... but, wait, I don't
> > see anything for it in docs/system/deprecated.rst. This is dropping
> > a feature without following the usual deprecation policy, i.e.
> > marking the feature as deprecated and only remove it 2 QEMU versions
> > later. Any justification for that ?
> 
> As long as the command really isn't useful any more, I wouldn't object
> to that from an HMP point of view.
> 

Ok then this should be documented in docs/system/removed-features.rst at
least.

> Dave
> 
> > David,
> > 
> > Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
> > but the commit title appears to be broken:
> > 
> > '65;6401;1cmonitor: removed cpustats command
> > 
> > https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69
> > 
> > > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > > ---
> > >  hmp-commands-info.hx | 13 -------------
> > >  monitor/misc.c       | 11 -----------
> > >  2 files changed, 24 deletions(-)
> > > 
> > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > index ab0c7aa5ee..b2347a6aea 100644
> > > --- a/hmp-commands-info.hx
> > > +++ b/hmp-commands-info.hx
> > > @@ -500,19 +500,6 @@ SRST
> > >      Show the current VM UUID.
> > >  ERST
> > >  
> > > -    {
> > > -        .name       = "cpustats",
> > > -        .args_type  = "",
> > > -        .params     = "",
> > > -        .help       = "show CPU statistics",
> > > -        .cmd        = hmp_info_cpustats,
> > > -    },
> > > -
> > > -SRST
> > > -  ``info cpustats``
> > > -    Show CPU statistics.
> > > -ERST
> > > -
> > >  #if defined(CONFIG_SLIRP)
> > >      {
> > >          .name       = "usernet",
> > > diff --git a/monitor/misc.c b/monitor/misc.c
> > > index f3a393ea59..1539e18557 100644
> > > --- a/monitor/misc.c
> > > +++ b/monitor/misc.c
> > > @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
> > >      }
> > >  }
> > >  
> > > -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> > > -{
> > > -    CPUState *cs = mon_get_cpu(mon);
> > > -
> > > -    if (!cs) {
> > > -        monitor_printf(mon, "No CPU available\n");
> > > -        return;
> > > -    }
> > > -    cpu_dump_statistics(cs, 0);
> > > -}
> > > -
> > >  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> > >  {
> > >      const char *name = qdict_get_try_str(qdict, "name");
> > 

Re: [PATCH 4/5] monitor: removed cpustats command

[Bruno Piazera Larsen]

[-- Attachment #1: Type: text/plain, Size: 3856 bytes --]


On 27/05/2021 05:30, Greg Kurz wrote:
> On Thu, 27 May 2021 09:09:55 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>
>> * Greg Kurz (groug@kaod.org) wrote:
>>> On Wed, 26 May 2021 17:21:03 -0300
>>> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
>>>
>>>> Since ppc was the last architecture to collect these statistics and
>>>> it is currently phasing this collection out, the command that would query
>>>> this information is being removed.
>>>>
>>> So this is removing an obviously user visible feature. This should be
>>> mentioned in docs/system/removed-features.rst... but, wait, I don't
>>> see anything for it in docs/system/deprecated.rst. This is dropping
>>> a feature without following the usual deprecation policy, i.e.
>>> marking the feature as deprecated and only remove it 2 QEMU versions
>>> later. Any justification for that ?

The command called a function that ultimately called an empty callback 
unless you changed target/ppc/translate.c and removed the comments 
around #define DO_PPC_STATISTICS. And like I mentioned in the cover 
letter (which may not have been CC'ed to you, sorry) ppc was the last 
architecture to support this feature while they were using the legacy 
decode system, but as they move to decodetree, this data wouldn't even 
be collected.

That's the justification, basically.

>> As long as the command really isn't useful any more, I wouldn't object
>> to that from an HMP point of view.
>>
> Ok then this should be documented in docs/system/removed-features.rst at
> least.
Sure, will send a patch later today with the update
>
>> Dave
>>
>>> David,
>>>
>>> Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
>>> but the commit title appears to be broken:
>>>
>>> '65;6401;1cmonitor: removed cpustats command
>>>
>>> https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69
>>>
>>>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>>>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
>>>> ---
>>>>   hmp-commands-info.hx | 13 -------------
>>>>   monitor/misc.c       | 11 -----------
>>>>   2 files changed, 24 deletions(-)
>>>>
>>>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>>>> index ab0c7aa5ee..b2347a6aea 100644
>>>> --- a/hmp-commands-info.hx
>>>> +++ b/hmp-commands-info.hx
>>>> @@ -500,19 +500,6 @@ SRST
>>>>       Show the current VM UUID.
>>>>   ERST
>>>>   
>>>> -    {
>>>> -        .name       = "cpustats",
>>>> -        .args_type  = "",
>>>> -        .params     = "",
>>>> -        .help       = "show CPU statistics",
>>>> -        .cmd        = hmp_info_cpustats,
>>>> -    },
>>>> -
>>>> -SRST
>>>> -  ``info cpustats``
>>>> -    Show CPU statistics.
>>>> -ERST
>>>> -
>>>>   #if defined(CONFIG_SLIRP)
>>>>       {
>>>>           .name       = "usernet",
>>>> diff --git a/monitor/misc.c b/monitor/misc.c
>>>> index f3a393ea59..1539e18557 100644
>>>> --- a/monitor/misc.c
>>>> +++ b/monitor/misc.c
>>>> @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
>>>>       }
>>>>   }
>>>>   
>>>> -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
>>>> -{
>>>> -    CPUState *cs = mon_get_cpu(mon);
>>>> -
>>>> -    if (!cs) {
>>>> -        monitor_printf(mon, "No CPU available\n");
>>>> -        return;
>>>> -    }
>>>> -    cpu_dump_statistics(cs, 0);
>>>> -}
>>>> -
>>>>   static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>>>>   {
>>>>       const char *name = qdict_get_try_str(qdict, "name");
-- 
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>

[-- Attachment #2: Type: text/html, Size: 5720 bytes --]

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

[Bruno Piazera Larsen]

[-- Attachment #1: Type: text/plain, Size: 2874 bytes --]


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>

[-- Attachment #2: Type: text/html, Size: 3670 bytes --]

Re: [PATCH 0/5] stop collection of instruction usage statistics

[Alex Bennée]

"Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:

> Based-on: <20210525115355.8254-1-bruno.larsen@eldorado.org.br>
>
> The functionality of counting how many instructions were being executed and
> being able to show it through the monitor, although neat, was only
> supported by ppc, and now that it is migrating to use decodetree (at
> least partially), those statistics won't be used anymore. Therefore,
> this patch removes that functinality completely.

I have no particular comment to make about the PPC stuff but with the
common translator loop we have hooks across all converted front ends to
identify the start of each instruction. It's needed for the TCG plugin
instrumentation and we could in theory use it for more integrated stats
across the board.

Out of interest what was the main aim of this code - a view of total
executed instructions or something more detailed like a breakdown of
types and ops?

>
> This series was suggested by Richard Henderson
>
> Bruno Larsen (billionai) (5):
>   target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set
>   target/ppc: remove ppc_cpu_dump_statistics
>   target/ppc: removed mentions to DO_PPC_STATISTICS
>   monitor: removed cpustats command
>   hw/core/cpu: removed cpu_dump_statistics function
>
>  hmp-commands-info.hx   | 13 --------
>  hw/core/cpu.c          |  9 ------
>  include/hw/core/cpu.h  | 12 --------
>  monitor/misc.c         | 11 -------
>  target/ppc/cpu.h       |  1 -
>  target/ppc/cpu_init.c  |  3 --
>  target/ppc/translate.c | 69 +++---------------------------------------
>  7 files changed, 5 insertions(+), 113 deletions(-)


-- 
Alex Bennée

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

[Greg Kurz]

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 0/5] stop collection of instruction usage statistics

[Bruno Piazera Larsen]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]


On 27/05/2021 10:57, Alex Bennée wrote:
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
>
>> Based-on: <20210525115355.8254-1-bruno.larsen@eldorado.org.br>
>>
>> The functionality of counting how many instructions were being executed and
>> being able to show it through the monitor, although neat, was only
>> supported by ppc, and now that it is migrating to use decodetree (at
>> least partially), those statistics won't be used anymore. Therefore,
>> this patch removes that functinality completely.
> I have no particular comment to make about the PPC stuff but with the
> common translator loop we have hooks across all converted front ends to
> identify the start of each instruction. It's needed for the TCG plugin
> instrumentation and we could in theory use it for more integrated stats
> across the board.
>
> Out of interest what was the main aim of this code - a view of total
> executed instructions or something more detailed like a breakdown of
> types and ops?

I'm pretty new to qemu, so I'm not sure what the original intent was, 
but what it did was count how many times the handler of the instruction 
was called, so you knew how many times each individual opcode was used. 
At least, that's what I think it should do, since the code doesn't even 
compile anymore for me to check

>
>> This series was suggested by Richard Henderson
>>
>> Bruno Larsen (billionai) (5):
>>    target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set
>>    target/ppc: remove ppc_cpu_dump_statistics
>>    target/ppc: removed mentions to DO_PPC_STATISTICS
>>    monitor: removed cpustats command
>>    hw/core/cpu: removed cpu_dump_statistics function
>>
>>   hmp-commands-info.hx   | 13 --------
>>   hw/core/cpu.c          |  9 ------
>>   include/hw/core/cpu.h  | 12 --------
>>   monitor/misc.c         | 11 -------
>>   target/ppc/cpu.h       |  1 -
>>   target/ppc/cpu_init.c  |  3 --
>>   target/ppc/translate.c | 69 +++---------------------------------------
>>   7 files changed, 5 insertions(+), 113 deletions(-)
>
-- 
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>

[-- Attachment #2: Type: text/html, Size: 3365 bytes --]

RE: [PATCH 0/5] stop collection of instruction usage statistics

[Luis Fernando Fujita Pires]

From: Alex Bennée <alex.bennee@linaro.org>
> I have no particular comment to make about the PPC stuff but with the common
> translator loop we have hooks across all converted front ends to identify the
> start of each instruction. It's needed for the TCG plugin instrumentation and we
> could in theory use it for more integrated stats across the board.
> 
> Out of interest what was the main aim of this code - a view of total executed
> instructions or something more detailed like a breakdown of types and ops?

The legacy instruction decoding logic in the PPC implementation uses a table that maps opcode patterns (based on different parts of the instructions) to handlers that actually implement each instruction. 
The code that is being removed would list how many times each specific handler was invoked, so it had more information than just the total count of executed instructions.

That being said, the code probably wasn't being used for a while now, as it didn't even compile.

--
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 0/5] stop collection of instruction usage statistics

[Alex Bennée]

Luis Fernando Fujita Pires <luis.pires@eldorado.org.br> writes:

> From: Alex Bennée <alex.bennee@linaro.org>
>> I have no particular comment to make about the PPC stuff but with the common
>> translator loop we have hooks across all converted front ends to identify the
>> start of each instruction. It's needed for the TCG plugin instrumentation and we
>> could in theory use it for more integrated stats across the board.
>> 
>> Out of interest what was the main aim of this code - a view of total executed
>> instructions or something more detailed like a breakdown of types and ops?
>
> The legacy instruction decoding logic in the PPC implementation uses a
> table that maps opcode patterns (based on different parts of the
> instructions) to handlers that actually implement each instruction.
> The code that is being removed would list how many times each specific handler was invoked, so it had more information than just the total count of executed instructions.
>
> That being said, the code probably wasn't being used for a while now,
> as it didn't even compile.

Ahh OK. If you wanted to you could probably re-create that information
using the howvec plugin (see contrib/plugins/howvec) if the decode
tables where added for PPC.

-- 
Alex Bennée

Re: [PATCH 4/5] monitor: removed cpustats command

[Greg Kurz]

On Thu, 27 May 2021 08:24:40 -0300
Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> wrote:

> 
> On 27/05/2021 05:30, Greg Kurz wrote:
> > On Thu, 27 May 2021 09:09:55 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >
> >> * Greg Kurz (groug@kaod.org) wrote:
> >>> On Wed, 26 May 2021 17:21:03 -0300
> >>> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
> >>>
> >>>> Since ppc was the last architecture to collect these statistics and
> >>>> it is currently phasing this collection out, the command that would query
> >>>> this information is being removed.
> >>>>
> >>> So this is removing an obviously user visible feature. This should be
> >>> mentioned in docs/system/removed-features.rst... but, wait, I don't
> >>> see anything for it in docs/system/deprecated.rst. This is dropping
> >>> a feature without following the usual deprecation policy, i.e.
> >>> marking the feature as deprecated and only remove it 2 QEMU versions
> >>> later. Any justification for that ?
> 
> The command called a function that ultimately called an empty callback 
> unless you changed target/ppc/translate.c and removed the comments 
> around #define DO_PPC_STATISTICS. And like I mentioned in the cover 
> letter (which may not have been CC'ed to you, sorry) ppc was the last 
> architecture to support this feature while they were using the legacy 
> decode system, but as they move to decodetree, this data wouldn't even 
> be collected.
> 
> That's the justification, basically.
> 

So to sum up, this HMP command doesn't produce any output with upstream
QEMU. Add a section in docs/system/removed-features.rst and just mention
that. Not worth reposting the full series just for that. This can be done
in a followup patch.

> >> As long as the command really isn't useful any more, I wouldn't object
> >> to that from an HMP point of view.
> >>
> > Ok then this should be documented in docs/system/removed-features.rst at
> > least.
> Sure, will send a patch later today with the update
> >
> >> Dave
> >>
> >>> David,
> >>>
> >>> Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
> >>> but the commit title appears to be broken:
> >>>
> >>> '65;6401;1cmonitor: removed cpustats command
> >>>
> >>> https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69
> >>>
> >>>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> >>>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> >>>> ---
> >>>>   hmp-commands-info.hx | 13 -------------
> >>>>   monitor/misc.c       | 11 -----------
> >>>>   2 files changed, 24 deletions(-)
> >>>>
> >>>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> >>>> index ab0c7aa5ee..b2347a6aea 100644
> >>>> --- a/hmp-commands-info.hx
> >>>> +++ b/hmp-commands-info.hx
> >>>> @@ -500,19 +500,6 @@ SRST
> >>>>       Show the current VM UUID.
> >>>>   ERST
> >>>>   
> >>>> -    {
> >>>> -        .name       = "cpustats",
> >>>> -        .args_type  = "",
> >>>> -        .params     = "",
> >>>> -        .help       = "show CPU statistics",
> >>>> -        .cmd        = hmp_info_cpustats,
> >>>> -    },
> >>>> -
> >>>> -SRST
> >>>> -  ``info cpustats``
> >>>> -    Show CPU statistics.
> >>>> -ERST
> >>>> -
> >>>>   #if defined(CONFIG_SLIRP)
> >>>>       {
> >>>>           .name       = "usernet",
> >>>> diff --git a/monitor/misc.c b/monitor/misc.c
> >>>> index f3a393ea59..1539e18557 100644
> >>>> --- a/monitor/misc.c
> >>>> +++ b/monitor/misc.c
> >>>> @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
> >>>>       }
> >>>>   }
> >>>>   
> >>>> -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> >>>> -{
> >>>> -    CPUState *cs = mon_get_cpu(mon);
> >>>> -
> >>>> -    if (!cs) {
> >>>> -        monitor_printf(mon, "No CPU available\n");
> >>>> -        return;
> >>>> -    }
> >>>> -    cpu_dump_statistics(cs, 0);
> >>>> -}
> >>>> -
> >>>>   static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> >>>>   {
> >>>>       const char *name = qdict_get_try_str(qdict, "name");

RE: [PATCH 0/5] stop collection of instruction usage statistics

[Luis Fernando Fujita Pires]

From: Alex Bennée <alex.bennee@linaro.org>
> Ahh OK. If you wanted to you could probably re-create that information using
> the howvec plugin (see contrib/plugins/howvec) if the decode tables where
> added for PPC.

Interesting. I hadn't looked at the plugin mechanism before. In this hypothetical case, if we wanted to get the execution count for each instruction, we could also automatically generate the instruction decoding code based on decodetree. Then it would work for any architecture that uses decodetree.

I just realized that the ppc code wasn't counting instruction executions, but translations. A slightly modified version of howvec would also take care of that by counting the instructions directly in vcpu_tb_trans(), without even registering execution callbacks. Again, all hypothetical - but neat. :)

--
Luis Pires
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br>
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH 5/5] hw/core/cpu: removed cpu_dump_statistics function

[Eduardo Habkost]

On Wed, May 26, 2021 at 05:21:04PM -0300, Bruno Larsen (billionai) wrote:
> No more architectures set the pointer to dump_statistics, so there's no
> point in keeping it, or the related cpu_dump_statistics function.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3400 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5218 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bruno Piazera Larsen]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]


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>

[-- Attachment #2: Type: text/html, Size: 2182 bytes --]

Re: [PATCH 4/5] monitor: removed cpustats command

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Wed, 26 May 2021 17:21:03 -0300
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
>
>> Since ppc was the last architecture to collect these statistics and
>> it is currently phasing this collection out, the command that would query
>> this information is being removed.
>> 
>
> So this is removing an obviously user visible feature. This should be
> mentioned in docs/system/removed-features.rst... but, wait, I don't
> see anything for it in docs/system/deprecated.rst. This is dropping
> a feature without following the usual deprecation policy, i.e.
> marking the feature as deprecated and only remove it 2 QEMU versions
> later. Any justification for that ?

Our deprecation policy applies to stable interfaces, which HMP is not.

We don't break things there just because.  But dropping a command right
away when it is no longer useful is just fine.  No need to deprecate and
wait for the grace period.

[...]

Re: [PATCH 4/5] monitor: removed cpustats command

[Greg Kurz]

On Tue, 08 Jun 2021 17:10:32 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > On Wed, 26 May 2021 17:21:03 -0300
> > "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
> >
> >> Since ppc was the last architecture to collect these statistics and
> >> it is currently phasing this collection out, the command that would query
> >> this information is being removed.
> >> 
> >
> > So this is removing an obviously user visible feature. This should be
> > mentioned in docs/system/removed-features.rst... but, wait, I don't
> > see anything for it in docs/system/deprecated.rst. This is dropping
> > a feature without following the usual deprecation policy, i.e.
> > marking the feature as deprecated and only remove it 2 QEMU versions
> > later. Any justification for that ?
> 
> Our deprecation policy applies to stable interfaces, which HMP is not.
> 
> We don't break things there just because.  But dropping a command right
> away when it is no longer useful is just fine.  No need to deprecate and
> wait for the grace period.
> 
> [...]
> 

Ah, good to know.

Thanks!

--
Greg