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