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

[Wu, Fei]

On 6/1/2023 8:12 PM, Wu, Fei wrote:
> 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.
>>
'info tb' has extra flag to specify dump out_asm, in_asm & op, so there
looks more user cases than monitor_disas, I think it's nice to have, but
not strongly required, usually we have a general idea if we get the IN
ops, plus the other counters such as g/op/... in the output.

Right now I am inclined to switch to monitor_disas, I found it causes
guest kernel panic with the selective virt addr, it happens to the
original v10 too.

------------------------------

TB id:1998 | phys:0x9c5f5ce2 virt:0x00ffffff8ff8ece2 flags:0x00024018 0
inv/1
        | exec:68/0 guest inst cov:0.00%
        | trans:1 ints: g:1 op:17 op_opt:16 spills:0
        | h/g (host bytes / guest insts): 129.000000
        | time to gen at 2.4GHz => code:1960.42(ns) IR:915.83(ns)


could not gen code for this TB (no longer mapped?)
------------------------------
[   98.213363] Unable to handle kernel paging request at virtual address
00ffffff8ff8ece2
[   98.219153] Oops [#1]
[   98.219299] Modules linked in: binfmt_misc nls_ascii nls_cp437 vfat
fat cfg80211 rfkill drm fuse drm_panel_orientation_quirks configfs
i2c_core ip_tables x_tables autofs4 ext4 crc32c_generic crc16 mbcache
jbd2 virtio_net net_failover virtio_blk failover virtio_mmio virtio
virtio_ring
[   98.220832] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.2.0+ #1
[   98.221122] Hardware name: riscv-virtio,qemu (DT)
[   98.221330] epc : arch_cpu_idle+0x1e/0x28
[   98.221985]  ra : default_idle_call+0x3c/0xb8
[   98.222150] epc : ffffffff80004398 ra : ffffffff808a0bc2 sp :
ff2000000068bf40
[   98.222339]  gp : ffffffff815688e8 tp : ff6000007ffcc240 t0 :
ff2000000064bad8
[   98.222562]  t1 : 0000000000000001 t2 : ffffffff80e02ed0 s0 :
ff2000000068bf50
[   98.222780]  s1 : 0000000000000001 a0 : 00000000000154ac a1 :
ff6000027d0f3000
[   98.223001]  a2 : ff6000007ffcc240 a3 : 0000000000000001 a4 :
ff6000027d0f3000
[   98.223217]  a5 : ff600001fdd5d500 a6 : 4000000000000000 a7 :
0000000000000000
[   98.223427]  s2 : ffffffff8156a220 s3 : ffffffff8156a418 s4 :
ffffffff80c6ba18
[   98.223636]  s5 : ffffffff815a9588 s6 : 0000000000000000 s7 :
0000000000000000
[   98.223853]  s8 : fffffffffffffffe s9 : 000000008003d6a8 s10:
0000000000000000
[   98.224065]  s11: 0000000000000000 t3 : ffffffff8156a438 t4 :
0000000000000000
[   98.224272]  t5 : 0000000000000000 t6 : 0000000000000000
[   98.224436] status: 0000000200000100 badaddr: 00ffffff8ff8ece2 cause:
000000000000000c
[   98.224778] [<ffffffff80004398>] arch_cpu_idle+0x1e/0x28
[   98.226393] ---[ end trace 0000000000000000 ]---
[   98.226770] Kernel panic - not syncing: Attempted to kill the idle task!
[   98.227188] SMP: stopping secondary CPUs
[   98.227980] ---[ end Kernel panic - not syncing: Attempted to kill
the idle task! ]---

Thanks,
Fei.

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