Re: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 7/2/24 18:56, Xingtao Yao (Fujitsu) wrote:
> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> 
> one small suggestion:
> Keeping the addresses or values of fixed size in output message can improve the readability of logs.

Ok, I'll do it for every size.

> like:
>> +    case QEMU_PLUGIN_MEM_VALUE_U8:
>> +        g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
>> +        break;
> case QEMU_PLUGIN_MEM_VALUE_U8:
>      g_string_append_printf(out, "0x02%"PRIx8, value.data.u8);
>      break;
> 
> 
>> -----Original Message-----
>> From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org
>> <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of
>> Pierrick Bouvier
>> Sent: Wednesday, July 3, 2024 2:45 AM
>> To: qemu-devel@nongnu.org
>> Cc: Alex Bennée <alex.bennee@linaro.org>; Mahmoud Mandour
>> <ma.mandourr@gmail.com>; Pierrick Bouvier <pierrick.bouvier@linaro.org>;
>> Alexandre Iooss <erdnaxe@crans.org>; Philippe Mathieu-Daudé
>> <philmd@linaro.org>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson
>> <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net>
>> Subject: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
>>
>> By using "print-accesses=true" option, mem plugin will now print every
>> value accessed, with associated size, type (store vs load), symbol,
>> instruction address and phys/virt address accessed.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   tests/plugin/mem.c | 69
>> +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
>> index b650dddcce1..aecbad8e68d 100644
>> --- a/tests/plugin/mem.c
>> +++ b/tests/plugin/mem.c
>> @@ -21,10 +21,15 @@ typedef struct {
>>       uint64_t io_count;
>>   } CPUCount;
>>
>> +typedef struct {
>> +    uint64_t vaddr;
>> +    const char *sym;
>> +} InsnInfo;
>> +
>>   static struct qemu_plugin_scoreboard *counts;
>>   static qemu_plugin_u64 mem_count;
>>   static qemu_plugin_u64 io_count;
>> -static bool do_inline, do_callback;
>> +static bool do_inline, do_callback, do_print_accesses;
>>   static bool do_haddr;
>>   static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
>>
>> @@ -60,6 +65,44 @@ static void vcpu_mem(unsigned int cpu_index,
>> qemu_plugin_meminfo_t meminfo,
>>       }
>>   }
>>
>> +static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
>> meminfo,
>> +                         uint64_t vaddr, void *udata)
>> +{
>> +    InsnInfo *insn_info = udata;
>> +    unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
>> +    const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" : "load";
>> +    qemu_plugin_mem_value value = qemu_plugin_mem_get_value(meminfo);
>> +    uint64_t hwaddr =
>> +        qemu_plugin_hwaddr_phys_addr(qemu_plugin_get_hwaddr(meminfo,
>> vaddr));
>> +    g_autoptr(GString) out = g_string_new("");
>> +    g_string_printf(out,
>> +                    "0x%"PRIx64",%s,0x%"PRIx64",0x%"PRIx64",%d,%s,",
>> +                    insn_info->vaddr, insn_info->sym,
>> +                    vaddr, hwaddr, size, type);
>> +    switch (value.type) {
>> +    case QEMU_PLUGIN_MEM_VALUE_U8:
>> +        g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
>> +        break;
>> +    case QEMU_PLUGIN_MEM_VALUE_U16:
>> +        g_string_append_printf(out, "0x%"PRIx16, value.data.u16);
>> +        break;
>> +    case QEMU_PLUGIN_MEM_VALUE_U32:
>> +        g_string_append_printf(out, "0x%"PRIx32, value.data.u32);
>> +        break;
>> +    case QEMU_PLUGIN_MEM_VALUE_U64:
>> +        g_string_append_printf(out, "0x%"PRIx64, value.data.u64);
>> +        break;
>> +    case QEMU_PLUGIN_MEM_VALUE_U128:
>> +        g_string_append_printf(out, "0x%.0"PRIx64"%"PRIx64,
>> +                               value.data.u128.high, value.data.u128.low);
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +    g_string_append_printf(out, "\n");
>> +    qemu_plugin_outs(out->str);
>> +}
>> +
>>   static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>   {
>>       size_t n = qemu_plugin_tb_n_insns(tb);
>> @@ -79,6 +122,16 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
>> qemu_plugin_tb *tb)
>>                                                QEMU_PLUGIN_CB_NO_REGS,
>>                                                rw, NULL);
>>           }
>> +        if (do_print_accesses) {
>> +            /* we leak this pointer, to avoid locking to keep track of it */
>> +            InsnInfo *insn_info = g_malloc(sizeof(InsnInfo));
>> +            const char *sym = qemu_plugin_insn_symbol(insn);
>> +            insn_info->sym = sym ? sym : "";
>> +            insn_info->vaddr = qemu_plugin_insn_vaddr(insn);
>> +            qemu_plugin_register_vcpu_mem_cb(insn, print_access,
>> +                                             QEMU_PLUGIN_CB_NO_REGS,
>> +                                             rw, (void *) insn_info);
>> +        }
>>       }
>>   }
>>
>> @@ -117,6 +170,12 @@ QEMU_PLUGIN_EXPORT int
>> qemu_plugin_install(qemu_plugin_id_t id,
>>                   fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
>>                   return -1;
>>               }
>> +        } else if (g_strcmp0(tokens[0], "print-accesses") == 0) {
>> +            if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
>> +                                        &do_print_accesses)) {
>> +                fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
>> +                return -1;
>> +            }
>>           } else {
>>               fprintf(stderr, "option parsing failed: %s\n", opt);
>>               return -1;
>> @@ -129,6 +188,14 @@ QEMU_PLUGIN_EXPORT int
>> qemu_plugin_install(qemu_plugin_id_t id,
>>           return -1;
>>       }
>>
>> +    if (do_print_accesses) {
>> +        g_autoptr(GString) out = g_string_new("");
>> +        g_string_printf(out,
>> +                "insn_vaddr,insn_symbol,mem_vaddr,mem_hwaddr,"
>> +                "access_size,access_type,mem_value\n");
>> +        qemu_plugin_outs(out->str);
>> +    }
>> +
>>       counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
>>       mem_count = qemu_plugin_scoreboard_u64_in_struct(
>>           counts, CPUCount, mem_count);
>> --
>> 2.39.2
>>
>