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

[Wu, Fei]

On 6/7/2023 8:49 PM, Wu, Fei wrote:
> On 6/1/2023 10:40 AM, Richard Henderson wrote:
>>> +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.
>>
> Yes, it's usually okay for kernel address, but cannot re-gen the code
> for userspace virtual address (guest kernel panic on riscv guest). I
> tried monitor_disas() but it failed to disas too:
>     monitor_disas(mon, mon_get_cpu(mon), tbs->phys_pc, num_inst, true);
> 
> How to use this function correctly?
> 
'phys_pc' in tbs is returned by get_page_addr_code_hostp(), which is not
guest phys address actually, but ram_addr_t instead, so it's always
wrong for monitor_disas. After some dirty changes, tbs can record the
guest pa. Now we can disas both kernel and user space code. But still,
no code is regenerated, disas in 'info tb' is just a convenient way to 'xp'.

Is there any existing function to convert ram_addr_t to guest pa?


(qemu) info tb-list 2
TB id:0 | phys:0x11b97762e virt:0x00aaaaaab76c262e flags:0x00024010 0 inv/1
        | exec:4447539979/0 guest inst cov:69.06%
        | trans:1 ints: g:8 op:28 op_opt:23 spills:0
        | h/g (host bytes / guest insts): 37.000000

TB id:1 | phys:0x8063474e virt:0xffffffff8043474e flags:0x01024001 0 inv/1
        | exec:131719290/0 guest inst cov:2.38%
        | trans:1 ints: g:9 op:37 op_opt:35 spills:0
        | h/g (host bytes / guest insts): 51.555557

(qemu) info tb 0
------------------------------

TB id:0 | phys:0x11b97762e virt:0x00aaaaaab76c262e flags:0x00024010 0 inv/1
        | exec:5841751800/0 guest inst cov:69.06%
        | trans:1 ints: g:8 op:28 op_opt:23 spills:0
        | h/g (host bytes / guest insts): 37.000000

0x11b97762e:  00002797          auipc                   a5,8192
       # 0x11b97962e
0x11b977632:  a2278793          addi                    a5,a5,-1502
0x11b977636:  639c              ld                      a5,0(a5)
0x11b977638:  00178713          addi                    a4,a5,1
0x11b97763c:  00002797          auipc                   a5,8192
       # 0x11b97963c
0x11b977640:  a1478793          addi                    a5,a5,-1516
0x11b977644:  e398              sd                      a4,0(a5)
0x11b977646:  b7e5              j                       -24
       # 0x11b97762e

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

(qemu) xp/8i 0x11b97762e
0x11b97762e:  00002797          auipc                   a5,8192
       # 0x11b97962e
0x11b977632:  a2278793          addi                    a5,a5,-1502
0x11b977636:  639c              ld                      a5,0(a5)
0x11b977638:  00178713          addi                    a4,a5,1
0x11b97763c:  00002797          auipc                   a5,8192
       # 0x11b97963c
0x11b977640:  a1478793          addi                    a5,a5,-1516
0x11b977644:  e398              sd                      a4,0(a5)
0x11b977646:  b7e5              j                       -24
       # 0x11b97762e
(qemu)

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

TB id:1 | phys:0x8063474e virt:0xffffffff8043474e flags:0x01024001 0 inv/1
        | exec:131719290/0 guest inst cov:2.38%
        | trans:1 ints: g:9 op:37 op_opt:35 spills:0
        | h/g (host bytes / guest insts): 51.555557

0x8063474e:  00194a83          lbu                     s5,1(s2)
0x80634752:  00094803          lbu                     a6,0(s2)
0x80634756:  0b09              addi                    s6,s6,2
0x80634758:  008a9a9b          slliw                   s5,s5,8
0x8063475c:  01586833          or                      a6,a6,s5
0x80634760:  ff0b1f23          sh                      a6,-2(s6)
0x80634764:  1c7d              addi                    s8,s8,-1
0x80634766:  0909              addi                    s2,s2,2
0x80634768:  fe0c13e3          bnez                    s8,-26
      # 0x8063474e

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

(qemu) xp/9i 0x8063474e
0x8063474e:  00194a83          lbu                     s5,1(s2)
0x80634752:  00094803          lbu                     a6,0(s2)
0x80634756:  0b09              addi                    s6,s6,2
0x80634758:  008a9a9b          slliw                   s5,s5,8
0x8063475c:  01586833          or                      a6,a6,s5
0x80634760:  ff0b1f23          sh                      a6,-2(s6)
0x80634764:  1c7d              addi                    s8,s8,-1
0x80634766:  0909              addi                    s2,s2,2
0x80634768:  fe0c13e3          bnez                    s8,-26
      # 0x8063474e

Thanks,
Fei.

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