[v2] plugins: add tb convenience functions

[PATCH v2 0/3] plugins: add tb convenience functions

Posted by Luke Craig 9 months, 2 weeks ago

This PR extends the plugin API with two functions which allow convenient access around tbs.

The first, qemu_plugin_tb_size, provides a mechanism for determining the total size of a translation block.

The second, qemu_plugin_tb_get_insn_by_vaddr, allows users to get a reference to an instruction by its virtual address rather than just its index.

In response to feedback from Pierrick I have updated the implementation of qemu_plugin_tb_size.

Additionally, I have added these functions to the insn.c test plugin in response to Alex's feedback.

Lastly, I'll provide a reply to Alex's feeback (repeated below):

> But the general comment is this is an example of tying the plugin API
> too deeply with the internals of the translator. Why does a plugin need
> to know what is an implementation detail?

Finding the line between implementation detail and relevant to plugins is challenging, but I submitted this change because I found myself implementing these functions in plugins. If you'd like for me to enumerate examples where knowing the tb_size is relevant to analysis I'd be happy to submit some.

Luke Craig (3):
  plugin: extend API with qemu_plugin_tb_get_insn_by_vaddr
  plugin: extend API with qemu_plugin_tb_size
  plugins: extend insn test for new convenience functions

 include/qemu/qemu-plugin.h | 21 +++++++++++++++++++++
 plugins/api.c              | 20 ++++++++++++++++++++
 tests/tcg/plugins/insn.c   | 10 ++++++++++
 3 files changed, 51 insertions(+)

-- 
2.34.1

Re: [PATCH v2 0/3] plugins: add tb convenience functions

Posted by Pierrick Bouvier 9 months, 2 weeks ago

Hi Luke,

On 1/31/25 09:57, Luke Craig wrote:
> This PR extends the plugin API with two functions which allow convenient access around tbs.
> 
> The first, qemu_plugin_tb_size, provides a mechanism for determining the total size of a translation block.
> 
> The second, qemu_plugin_tb_get_insn_by_vaddr, allows users to get a reference to an instruction by its virtual address rather than just its index.
> 
> In response to feedback from Pierrick I have updated the implementation of qemu_plugin_tb_size.
> 
> Additionally, I have added these functions to the insn.c test plugin in response to Alex's feedback.
> 
> Lastly, I'll provide a reply to Alex's feeback (repeated below):
> 
>> But the general comment is this is an example of tying the plugin API
>> too deeply with the internals of the translator. Why does a plugin need
>> to know what is an implementation detail?
> 
> Finding the line between implementation detail and relevant to plugins is challenging, but I submitted this change because I found myself implementing these functions in plugins. If you'd like for me to enumerate examples where knowing the tb_size is relevant to analysis I'd be happy to submit some.
> 
> Luke Craig (3):
>    plugin: extend API with qemu_plugin_tb_get_insn_by_vaddr
>    plugin: extend API with qemu_plugin_tb_size
>    plugins: extend insn test for new convenience functions
> 

For all the series,
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

>   include/qemu/qemu-plugin.h | 21 +++++++++++++++++++++
>   plugins/api.c              | 20 ++++++++++++++++++++
>   tests/tcg/plugins/insn.c   | 10 ++++++++++
>   3 files changed, 51 insertions(+)
> 

To accept this series, commits should be signed off [1].
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line

Thanks,
Pierrick

Re: [PATCH v2 0/3] plugins: add tb convenience functions

Posted by Luke Craig 9 months, 2 weeks ago

Hi Pierrick,

Thank you for your reply.

I have submitted a new patch series with commits signed off.

Thanks!
-Luke

On Fri, Jan 31, 2025, 2:53 PM Pierrick Bouvier <pierrick.bouvier@linaro.org>
wrote:

> Hi Luke,
>
> On 1/31/25 09:57, Luke Craig wrote:
> > This PR extends the plugin API with two functions which allow convenient
> access around tbs.
> >
> > The first, qemu_plugin_tb_size, provides a mechanism for determining the
> total size of a translation block.
> >
> > The second, qemu_plugin_tb_get_insn_by_vaddr, allows users to get a
> reference to an instruction by its virtual address rather than just its
> index.
> >
> > In response to feedback from Pierrick I have updated the implementation
> of qemu_plugin_tb_size.
> >
> > Additionally, I have added these functions to the insn.c test plugin in
> response to Alex's feedback.
> >
> > Lastly, I'll provide a reply to Alex's feeback (repeated below):
> >
> >> But the general comment is this is an example of tying the plugin API
> >> too deeply with the internals of the translator. Why does a plugin need
> >> to know what is an implementation detail?
> >
> > Finding the line between implementation detail and relevant to plugins
> is challenging, but I submitted this change because I found myself
> implementing these functions in plugins. If you'd like for me to enumerate
> examples where knowing the tb_size is relevant to analysis I'd be happy to
> submit some.
> >
> > Luke Craig (3):
> >    plugin: extend API with qemu_plugin_tb_get_insn_by_vaddr
> >    plugin: extend API with qemu_plugin_tb_size
> >    plugins: extend insn test for new convenience functions
> >
>
> For all the series,
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>
> >   include/qemu/qemu-plugin.h | 21 +++++++++++++++++++++
> >   plugins/api.c              | 20 ++++++++++++++++++++
> >   tests/tcg/plugins/insn.c   | 10 ++++++++++
> >   3 files changed, 51 insertions(+)
> >
>
> To accept this series, commits should be signed off [1].
>
> https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line
>
> Thanks,
> Pierrick
>