Re: [PATCH v14 08/10] Adding info [tb-list|tb] commands to HMP (WIP)

[Wu, Fei]

On 6/1/2023 10:40 AM, Richard Henderson wrote:
> On 5/30/23 01:35, Fei Wu wrote:
>> +static void do_dump_tbs_info(int total, int sort_by)
>> +{
>> +    id = 1;
>> +    GList *i;
>> +    int count = total;
>> +
>> +    g_list_free(last_search);
>> +    last_search = NULL;
>> +
>> +    qht_iter(&tb_ctx.tb_stats, collect_tb_stats, NULL);
>> +
>> +    last_search = g_list_sort_with_data(last_search, inverse_sort_tbs,
>> +                                        &sort_by);
>> +
> 
> Why are you sorting on a list and not an array?
> Intuitively, sorting a list of 1 million elements seems like the wrong
> choice.
> 
> Why did you put all the comparisons in one inverse_sort_tbs function,
> and require examining sort_by?  Better would be N sorting functions
> where sort_by is evaluated once before starting the sort.
> 
> 
>> +++ b/disas/disas.c
>> @@ -8,6 +8,8 @@
>>  #include "hw/core/cpu.h"
>>  #include "exec/memory.h"
>>  
>> +#include "qemu/log-for-trace.h"
>> +
>>  /* Filled in by elfload.c.  Simplistic, but will do for now. */
>>  struct syminfo *syminfos = NULL;
>>  
>> @@ -199,6 +201,24 @@ static void initialize_debug_host(CPUDebug *s)
>>  #endif
>>  }
>>  
>> +static int
>> +__attribute__((format(printf, 2, 3)))
>> +fprintf_log(FILE *a, const char *b, ...)
>> +{
>> +    va_list ap;
>> +    va_start(ap, b);
>> +
>> +    if (!to_string) {
>> +        vfprintf(a, b, ap);
>> +    } else {
>> +        qemu_vlog(b, ap);
>> +    }
>> +
>> +    va_end(ap);
>> +
>> +    return 1;
>> +}
>> +
> 
> Not need on this either.  Global variable being checked on each
> callback, instead of selecting the proper callback earlier -- preferably
> without the global variable.
> 
> Did you really need something different than monitor_disas?  You almost
> certainly want to read physical memory and not virtual anyway.
> 
This makes me think the necessity of 'info tb', the primary extra info
it adds is guest instruction, which can be gotten from 'x/i' w/o calling
tb_gen_code.

(qemu) info tb 1
------------------------------

TB id:1 | phys:0x79bc86 virt:0xffffffff8059bc86 flags:0x01024001 0 inv/1
        | exec:56962444/0 guest inst cov:0.61%
        | trans:1 ints: g:8 op:27 op_opt:22 spills:0
        | h/g (host bytes / guest insts): 26.000000
        | time to gen at 2.4GHz => code:747.08(ns) IR:477.92(ns)

----------------
IN:
Priv: 1; Virt: 0

0xffffffff8059bc86:  00000013          nop
0xffffffff8059bc8a:  00000013          nop
0xffffffff8059bc8e:  00000013          nop
0xffffffff8059bc92:  00000013          nop
0xffffffff8059bc96:  1141              addi                    sp,sp,-16
0xffffffff8059bc98:  e422              sd                      s0,8(sp)
0xffffffff8059bc9a:  0800              addi                    s0,sp,16
0xffffffff8059bc9c:  c0102573          rdtime                  a0
------------------------------

(qemu) x/8i 0xffffffff8059bc86
x/8i 0xffffffff8059bc86
0xffffffff8059bc86:  00000013          nop
0xffffffff8059bc8a:  00000013          nop
0xffffffff8059bc8e:  00000013          nop
0xffffffff8059bc92:  00000013          nop
0xffffffff8059bc96:  1141              addi                    sp,sp,-16
0xffffffff8059bc98:  e422              sd                      s0,8(sp)
0xffffffff8059bc9a:  0800              addi                    s0,sp,16
0xffffffff8059bc9c:  c0102573          rdtime                  a0


Thanks,
Fei.

>> +void qemu_log_to_monitor(bool enable)
>> +{
>> +    to_monitor = enable;
>> +}
>> +
>> +void qemu_log_to_string(bool enable, GString *s)
>> +{
>> +    to_string = enable;
>> +    string = s;
>> +}
> 
> What are these for, and why do you need them?
> Why would to_string ever be anything other than (string != NULL)?
> Why are you using such very generic names for global variables?
> 
> 
> r~