Re: [PATCH RESEND v5 24/26] contrib/plugins: Allow to log registers

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/09/05 0:30, Alex Bennée wrote:
> 
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2023/08/31 0:08, Alex Bennée wrote:
>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>
>>>> This demonstrates how a register can be read from a plugin.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    docs/devel/tcg-plugins.rst |  10 ++-
>>>>    contrib/plugins/execlog.c  | 140 ++++++++++++++++++++++++++++---------
>>>>    2 files changed, 117 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
>>>> index 81dcd43a61..c9f8b27590 100644
>>>> --- a/docs/devel/tcg-plugins.rst
>>>> +++ b/docs/devel/tcg-plugins.rst
>>>> @@ -497,6 +497,15 @@ arguments if required::
>>>>      $ qemu-system-arm $(QEMU_ARGS) \
>>>>        -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin
>>>>    +This plugin can also dump a specified register. The
>>>> specification of register
>>>> +follows `GDB standard target features <https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html>`__.
>>>> +
>>>> +Specify the name of the feature that contains the register and the name of the
>>>> +register with ``rfile`` and ``reg`` options, respectively::
>>>> +
>>>> +  $ qemu-system-arm $(QEMU_ARGS) \
>>>> +    -plugin ./contrib/plugins/libexeclog.so,rfile=org.gnu.gdb.arm.core,reg=sp -d plugin
>>>> +
>>>>    - contrib/plugins/cache.c
>>>>      Cache modelling plugin that measures the performance of a given
>>>> L1 cache
>>>> @@ -583,4 +592,3 @@ The following API is generated from the inline documentation in
>>>>    include the full kernel-doc annotations.
>>>>      .. kernel-doc:: include/qemu/qemu-plugin.h
>>>> -
>>>> diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
>>>> index 82dc2f584e..aa05840fd0 100644
>>>> --- a/contrib/plugins/execlog.c
>>>> +++ b/contrib/plugins/execlog.c
>>>> @@ -15,27 +15,43 @@
>>>>      #include <qemu-plugin.h>
>>>>    +typedef struct CPU {
>>>> +    /* Store last executed instruction on each vCPU as a GString */
>>>> +    GString *last_exec;
>>>> +    GByteArray *reg_history[2];
>>>> +
>>>> +    int reg;
>>>> +} CPU;
>>>> +
>>>>    QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>>>    
>>> <snip>
>>>>          /* Store new instruction in cache */
>>>>        /* vcpu_mem will add memory access information to last_exec */
>>>> -    g_string_printf(s, "%u, ", cpu_index);
>>>> -    g_string_append(s, (char *)udata);
>>>> +    g_string_printf(cpus[cpu_index].last_exec, "%u, ", cpu_index);
>>>> +    g_string_append(cpus[cpu_index].last_exec, (char *)udata);
>>>> +
>>>> +    g_rw_lock_reader_unlock(&expand_array_lock);
>>>>    }
>>>>      /**
>>>> @@ -167,8 +197,10 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>>                                                 QEMU_PLUGIN_MEM_RW, NULL);
>>>>                  /* Register callback on instruction */
>>>> -            qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec,
>>>> -                                                   QEMU_PLUGIN_CB_NO_REGS, output);
>>>> +            qemu_plugin_register_vcpu_insn_exec_cb(
>>>> +                insn, vcpu_insn_exec,
>>>> +                rfile_name ? QEMU_PLUGIN_CB_R_REGS : QEMU_PLUGIN_CB_NO_REGS,
>>>> +                output);
>>>>                  /* reset skip */
>>>>                skip = (imatches || amatches);
>>>> @@ -177,17 +209,53 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>>        }
>>>>    }
>>>>    +static void vcpu_init(qemu_plugin_id_t id, unsigned int
>>>> vcpu_index)
>>>> +{
>>>> +    int reg = 0;
>>>> +    bool found = false;
>>>> +
>>>> +    expand_cpu(vcpu_index);
>>>> +
>>>> +    if (rfile_name) {
>>>> +        int i;
>>>> +        int j;
>>>> +        int n;
>>>> +
>>>> +        qemu_plugin_register_file_t *rfiles =
>>>> +            qemu_plugin_get_register_files(vcpu_index, &n);
>>>> +
>>>> +        for (i = 0; i < n; i++) {
>>>> +            if (g_strcmp0(rfiles[i].name, rfile_name) == 0) {
>>>> +                for (j = 0; j < rfiles[i].num_regs; j++) {
>>>> +                    if (g_strcmp0(rfiles[i].regs[j], reg_name) == 0) {
>>>> +                        reg += j;
>>>> +                        found = true;
>>>> +                        break;
>>>> +                    }
>>>> +                }
>>>> +                break;
>>>> +            }
>>>> +
>>>> +            reg += rfiles[i].num_regs;
>>>> +        }
>>>> +
>>>> +        g_free(rfiles);
>>>> +    }
>>> This makes me question the value of exposing the register file
>>> directly
>>> to the plugin. I would much rather have a lookup utility function with
>>> an optional tag. Something like:
>>>     plugin_reg_t qemu_plugin_find_register(const char *name, const
>>> char *tag);
>>> And make tag optional. I think in the general case "name" should be
>>> enough.
>>
>> I have explained the reason why I introduced register file abstraction
>> instead of adding a function to look up a register for an earlier
>> version of this series:
>>> I added a function that returns all register information instead of a
>>> function that looks up a register so that a plugin can enumerate
>>> registers. Such capability is useful for a plugin that dumps all
>>> registers or a plugin that simulates processor (such a plugin may want
>>> to warn if there are unknown registers).
> 
> Fair enough. However I think a simple search interface will also be
> useful for the more common case.

I'll add one in a future version.

> 
>> How would you define name and tag? They are something we currently do
>> not have, and I'm trying to add new types of identifiers since such
>> identifiers will be needed to be defined for different architectures
>> and require documentation and extra work to avoid name conflicts and
>> ensure interface stability.
> 
> The name would be the register name which AFAICT are unique across the
> system. If you have examples of clashes I'm curious as to what they are.

Here I'm talking about creating and maintaining a set of identifiers 
independent of GDB. I'm suggesting to reuse the identifiers GDB use 
since they are already designed so that there are no clashes.

> I'm still conflicted about baking gdb-isms into this ABI because they
> aren't as stable as they could be either. Either way we do state:
> 
>    This is a new feature for QEMU and it does allow people to develop
>    out-of-tree plugins that can be dynamically linked into a running QEMU
>    process. However the project reserves the right to change or break the
>    API should it need to do so. The best way to avoid this is to submit
>    your plugin upstream so they can be updated if/when the API changes.
> 
> So I'm not overly concerned about formalising a stable ABI for now.
> 
>>
>>>
>>>> +
>>>> +    g_rw_lock_writer_lock(&expand_array_lock);
>>>> +    cpus[vcpu_index].reg = found ? reg : -1;
>>>> +    g_rw_lock_writer_unlock(&expand_array_lock);
>>>> +}
>>>> +
>>>>    /**
>>>>     * On plugin exit, print last instruction in cache
>>>>     */
>>>>    static void plugin_exit(qemu_plugin_id_t id, void *p)
>>>>    {
>>>>        guint i;
>>>> -    GString *s;
>>>> -    for (i = 0; i < last_exec->len; i++) {
>>>> -        s = g_ptr_array_index(last_exec, i);
>>>> -        if (s->str) {
>>>> -            qemu_plugin_outs(s->str);
>>>> +    for (i = 0; i < num_cpus; i++) {
>>>> +        if (cpus[i].last_exec->str) {
>>>> +            qemu_plugin_outs(cpus[i].last_exec->str);
>>>>                qemu_plugin_outs("\n");
>>>>            }
>>>>        }
>>>> @@ -224,9 +292,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>>>>         * we don't know the size before emulation.
>>>>         */
>>>>        if (info->system_emulation) {
>>>> -        last_exec = g_ptr_array_sized_new(info->system.max_vcpus);
>>>> -    } else {
>>>> -        last_exec = g_ptr_array_new();
>>>> +        cpus = g_new(CPU, info->system.max_vcpus);
>>>>        }
>>>>          for (int i = 0; i < argc; i++) {
>>>> @@ -236,13 +302,23 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>>>>                parse_insn_match(tokens[1]);
>>>>            } else if (g_strcmp0(tokens[0], "afilter") == 0) {
>>>>                parse_vaddr_match(tokens[1]);
>>>> +        } else if (g_strcmp0(tokens[0], "rfile") == 0) {
>>>> +            rfile_name = g_strdup(tokens[1]);
>>>> +        } else if (g_strcmp0(tokens[0], "reg") == 0) {
>>>> +            reg_name = g_strdup(tokens[1]);
>>> And then instead of having the rfile/reg distinction support a
>>> command
>>> line like:
>>>     qemu-aarch64 -plugin
>>> contrib/plugins/libexeclog.so,reg=sp,reg=x1,reg=sve:p1
>>> so if the user specifies a reg of the form TAG:REG we can pass that
>>> as
>>> the option tag string. It also avoids exposing all the details of gdb to
>>> plugins while still allowing the utility function to search by rname
>>> internally (even if only a substring match?),
>>
>> That implicitly assumes TAG does not contain a colon. I'm avoiding to
>> make such an implicit assumption because it is a reference for plugin
>> writers who may create out-of-tree plugins. We should retrain
>> ourselves to tell the plugin writers not to make such an assumption
>> that may not hold in the future version of QEMU.
>>
>> I consider a substring match harmful for a similar reason. There is no
>> guarantee that a future version of QEMU will not introduce a new
>> register that match with the existing substring and break interface
>> stability.
>>
>> It is not necessary that identifiers are consistent with ones GDB use.
>> What matters here is that the identifiers are documented, stable and
>> immune from conflicts.
> 
> We've a couple of cases of GDB having to issue new XML interface names
> to handle cases where the previous definition missed important bits.
> Hence my unease at exposing them to the plugin interface.

Why you had to issue new feature names when adding missing definitions? 
I expect the reasoning for changing GDB interface names should also 
apply for TCG plugins: when not changing interface names will break GDB, 
it should be also likely to break TCG plugins using the affected features.

I consider names used in GDB stable. GDB versions will break if they are 
not.

> 
> The plugin interface shouldn't (yet?) be regarded as a stable interface
> (c.f. above).
The plugin interface has been for almost 5 years. If it is not stable 
yet, when it will be? In any case, the interface stability should be 
considered as an eventual goal; otherwise the existing infrastructure 
for plugin API versioning will make no sense.