mbox series

[v7,0/6] plugins: access values during a memory read/write

Message ID 20240724194708.1843704-1-pierrick.bouvier@linaro.org (mailing list archive)
Headers show
Series plugins: access values during a memory read/write | expand

Message

Pierrick Bouvier July 24, 2024, 7:47 p.m. UTC
This series allows plugins to know which value is read/written during a memory
access.

For every memory access, we know copy this value before calling mem callbacks,
and those can query it using new API function:
- qemu_plugin_mem_get_value

Mem plugin was extended to print accesses, and a new test was added to check
functionality work as expected. A bug was found where callbacks were not
called as expected.

This will open new use cases for plugins, such as tracking specific values in
memory.

Needs review:
Patch 7: tests/tcg/multiarch: add test for plugin memory access

v7
- renamed variable for adding plugins tests in Makefile
- do not run any command when plugin output should not be checked (thanks Alex)
- add LICENSE + summary for tests/tcg/multiarch/test-plugin-mem-access.c
- test for mem access is now multiarch (tested on aarch64, x86_64, i386)

v6
- fix big endian offset for plugin_gen_mem_callbacks_i32

v5
- fixed width output for mem values in mem plugin
- move plugin_mem_value to CPUNegativeOffset
- tcg/tcg-op-ldst.c: only store word size mem access (do not set upper bits)

v4
- fix prototype for stubs qemu_plugin_vcpu_mem_cb (inverted low/high parameters
  names)
- link gitlab bugs resolved (thanks @Anton Kochkov for reporting)
  https://gitlab.com/qemu-project/qemu/-/issues/1719
  https://gitlab.com/qemu-project/qemu/-/issues/2152

v3
- simplify API: return an algebraic data type for value accessed
  this can be easily extended when QEMU will support wider accesses
- fix Makefile test (use quiet-command instead of manually run the command)
- rename upper/lower to high/low
- reorder functions parameters and code to low/high instead of high/low, to
  follow current convention in QEMU codebase

v2
- fix compilation on aarch64 (missing undef in accel/tcg/atomic_template.h)

v3
- add info when printing memory accesses (insn_vaddr,mem_vaddr,mem_hwaddr)

Pierrick Bouvier (6):
  plugins: save value during memory accesses
  plugins: extend API to get latest memory value accessed
  tests/tcg: add mechanism to run specific tests with plugins
  tests/tcg: allow to check output of plugins
  tests/plugin/mem: add option to print memory accesses
  tests/tcg/multiarch: add test for plugin memory access

 accel/tcg/atomic_template.h                   |  66 ++++++-
 include/hw/core/cpu.h                         |   4 +
 include/qemu/plugin.h                         |   4 +
 include/qemu/qemu-plugin.h                    |  32 ++++
 plugins/api.c                                 |  33 ++++
 plugins/core.c                                |   6 +
 tcg/tcg-op-ldst.c                             |  66 ++++++-
 tests/plugin/mem.c                            |  69 ++++++-
 tests/tcg/multiarch/test-plugin-mem-access.c  | 175 ++++++++++++++++++
 accel/tcg/atomic_common.c.inc                 |  13 +-
 accel/tcg/ldst_common.c.inc                   |  38 ++--
 plugins/qemu-plugins.symbols                  |   1 +
 tests/tcg/Makefile.target                     |  12 +-
 tests/tcg/multiarch/Makefile.target           |   7 +
 .../tcg/multiarch/check-plugin-mem-access.sh  |  30 +++
 15 files changed, 524 insertions(+), 32 deletions(-)
 create mode 100644 tests/tcg/multiarch/test-plugin-mem-access.c
 create mode 100755 tests/tcg/multiarch/check-plugin-mem-access.sh

Comments

Alex Bennée Sept. 5, 2024, 3:21 p.m. UTC | #1
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> This series allows plugins to know which value is read/written during a memory
> access.
>
> For every memory access, we know copy this value before calling mem callbacks,
> and those can query it using new API function:
> - qemu_plugin_mem_get_value

Queued to patches 1-5 to plugins/next, thanks.

You can send the re-spun version of 6 once the review comments have been
done.
Pierrick Bouvier Sept. 7, 2024, 1:49 a.m. UTC | #2
On 9/5/24 08:21, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> This series allows plugins to know which value is read/written during a memory
>> access.
>>
>> For every memory access, we know copy this value before calling mem callbacks,
>> and those can query it using new API function:
>> - qemu_plugin_mem_get_value
> 
> Queued to patches 1-5 to plugins/next, thanks.
> 
> You can send the re-spun version of 6 once the review comments have been
> done.
> 

Thanks Alex,

right now, my try to make check-tcg are blocked with the cross 
containers who don't compile, so I'll wait for this to be resolved.
I still wonder if having a simple aarch64/x64 test is not enough, and 
covering 99.9% of the bug we could introduce in the future on this.
Alex Bennée Sept. 9, 2024, 10 a.m. UTC | #3
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 9/5/24 08:21, Alex Bennée wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>> 
>>> This series allows plugins to know which value is read/written during a memory
>>> access.
>>>
>>> For every memory access, we know copy this value before calling mem callbacks,
>>> and those can query it using new API function:
>>> - qemu_plugin_mem_get_value
>> Queued to patches 1-5 to plugins/next, thanks.
>> You can send the re-spun version of 6 once the review comments have
>> been
>> done.
>> 
>
> Thanks Alex,
>
> right now, my try to make check-tcg are blocked with the cross
> containers who don't compile, so I'll wait for this to be resolved.

Which ones?

> I still wonder if having a simple aarch64/x64 test is not enough, and
> covering 99.9% of the bug we could introduce in the future on this.

Have you measured the code coverage of the test?
Pierrick Bouvier Sept. 9, 2024, 7:04 p.m. UTC | #4
On 9/9/24 03:00, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 9/5/24 08:21, Alex Bennée wrote:
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> This series allows plugins to know which value is read/written during a memory
>>>> access.
>>>>
>>>> For every memory access, we know copy this value before calling mem callbacks,
>>>> and those can query it using new API function:
>>>> - qemu_plugin_mem_get_value
>>> Queued to patches 1-5 to plugins/next, thanks.
>>> You can send the re-spun version of 6 once the review comments have
>>> been
>>> done.
>>>
>>
>> Thanks Alex,
>>
>> right now, my try to make check-tcg are blocked with the cross
>> containers who don't compile, so I'll wait for this to be resolved.
> 
> Which ones?

docker-image-debian-mips64el-cross
docker-image-debian-mipsel-cross
(about broken packages).

I saw something mentioning this recently on the mailing list, so not 
sure what would be our solution to this (ignoring?)

> 
>> I still wonder if having a simple aarch64/x64 test is not enough, and
>> covering 99.9% of the bug we could introduce in the future on this.
> 
> Have you measured the code coverage of the test?
> 

Nope, but all the code changed is tcg-generic, so testing this on all 
arch does not bring benefit in terms of coverage.

So by focusing on the "all arch" aspect, we just test tcg implementation 
itself, instead of the plugins part.

The problems we identified so far is compilation flags specific per 
arch, and specific flags to emit words instruction. It does not seem 
related to what we really want to test here.
Alex Bennée Sept. 9, 2024, 8:21 p.m. UTC | #5
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 9/9/24 03:00, Alex Bennée wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>> 
>>> On 9/5/24 08:21, Alex Bennée wrote:
>>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>>
>>>>> This series allows plugins to know which value is read/written during a memory
>>>>> access.
>>>>>
>>>>> For every memory access, we know copy this value before calling mem callbacks,
>>>>> and those can query it using new API function:
>>>>> - qemu_plugin_mem_get_value
>>>> Queued to patches 1-5 to plugins/next, thanks.
>>>> You can send the re-spun version of 6 once the review comments have
>>>> been
>>>> done.
>>>>
>>>
>>> Thanks Alex,
>>>
>>> right now, my try to make check-tcg are blocked with the cross
>>> containers who don't compile, so I'll wait for this to be resolved.
>> Which ones?
>
> docker-image-debian-mips64el-cross
> docker-image-debian-mipsel-cross
> (about broken packages).

I have fixes for mipsel at least when I post my series.

>
> I saw something mentioning this recently on the mailing list, so not
> sure what would be our solution to this (ignoring?)
>
>> 
>>> I still wonder if having a simple aarch64/x64 test is not enough, and
>>> covering 99.9% of the bug we could introduce in the future on this.
>> Have you measured the code coverage of the test?
>> 
>
> Nope, but all the code changed is tcg-generic, so testing this on all
> arch does not bring benefit in terms of coverage.

Would that it were so simple. Quite often which bits of the generic TCG
code get exercised depends on the guest architecture using it. I'm not
saying we have to go over and above to enable fiddly architectures but we
should at least understand if the reason they fail is down to them or
core code.

> So by focusing on the "all arch" aspect, we just test tcg
> implementation itself, instead of the plugins part.
>
> The problems we identified so far is compilation flags specific per
> arch, and specific flags to emit words instruction. It does not seem
> related to what we really want to test here.

I'm also investigating why arm-softmmu seems to be seeing more accesses
than it should have from the test.
Pierrick Bouvier Sept. 9, 2024, 9:42 p.m. UTC | #6
On 9/9/24 13:21, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 9/9/24 03:00, Alex Bennée wrote:
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> On 9/5/24 08:21, Alex Bennée wrote:
>>>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>>>
>>>>>> This series allows plugins to know which value is read/written during a memory
>>>>>> access.
>>>>>>
>>>>>> For every memory access, we know copy this value before calling mem callbacks,
>>>>>> and those can query it using new API function:
>>>>>> - qemu_plugin_mem_get_value
>>>>> Queued to patches 1-5 to plugins/next, thanks.
>>>>> You can send the re-spun version of 6 once the review comments have
>>>>> been
>>>>> done.
>>>>>
>>>>
>>>> Thanks Alex,
>>>>
>>>> right now, my try to make check-tcg are blocked with the cross
>>>> containers who don't compile, so I'll wait for this to be resolved.
>>> Which ones?
>>
>> docker-image-debian-mips64el-cross
>> docker-image-debian-mipsel-cross
>> (about broken packages).
> 
> I have fixes for mipsel at least when I post my series.
> 
>>
>> I saw something mentioning this recently on the mailing list, so not
>> sure what would be our solution to this (ignoring?)
>>
>>>
>>>> I still wonder if having a simple aarch64/x64 test is not enough, and
>>>> covering 99.9% of the bug we could introduce in the future on this.
>>> Have you measured the code coverage of the test?
>>>
>>
>> Nope, but all the code changed is tcg-generic, so testing this on all
>> arch does not bring benefit in terms of coverage.
> 
> Would that it were so simple. Quite often which bits of the generic TCG
> code get exercised depends on the guest architecture using it. I'm not
> saying we have to go over and above to enable fiddly architectures but we
> should at least understand if the reason they fail is down to them or
> core code.

I understand your point, and will try to make this work on all arch.

> 
>> So by focusing on the "all arch" aspect, we just test tcg
>> implementation itself, instead of the plugins part.
>>
>> The problems we identified so far is compilation flags specific per
>> arch, and specific flags to emit words instruction. It does not seem
>> related to what we really want to test here.
> 
> I'm also investigating why arm-softmmu seems to be seeing more accesses
> than it should have from the test.
> 

Good!