| Message ID | 20240918210712.2336854-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 18 Sept 2024 at 22:08, Alex Bennée <alex.bennee@linaro.org> wrote: > > The following changes since commit 2b81c046252fbfb375ad30632362fc16e6e22bd5: > > Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2024-09-17 14:02:18 +0100) > > are available in the Git repository at: > > https://gitlab.com/stsquad/qemu.git tags/pull-tcg-plugin-memory-180924-2 > > for you to fetch changes up to a33f4871e0a0f4bf1cb037ab29fae7df7f2fc658: > > contrib/plugins: avoid hanging program (2024-09-18 21:02:36 +0100) > > ---------------------------------------------------------------- > TCG plugin memory instrumentation updates > > - deprecate plugins on 32 bit hosts > - deprecate plugins with TCI > - extend memory API to save value > - add check-tcg tests to exercise new memory API > - fix timer deadlock with non-changing timer > - add basic block vector plugin to contrib > - add cflow plugin to contrib > - extend syscall plugin to dump write memory > - validate ips plugin arguments meet minimum slice value > > ---------------------------------------------------------------- Fails to build on macos: https://gitlab.com/qemu-project/qemu/-/jobs/7865151156 ../tests/tcg/plugins/mem.c:12:10: fatal error: 'endian.h' file not found endian.h is a Linuxism. While I'm looking at the code, this caught my eye: case QEMU_PLUGIN_MEM_VALUE_U64: { uint64_t *p = (uint64_t *) &ri->data[offset]; uint64_t val = be ? htobe64(value.data.u64) : htole64(value.data.u64); if (is_store) { *p = val; } else if (*p != val) { unseen_data = true; } break; } Casting a random byte pointer to uint64_t* like that and dereferencing it isn't valid -- it can fault if it's not aligned correctly. I suspect the plugin needs to define versions of at least some of the functionality in qemu's include/qemu/bswap.h. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 18 Sept 2024 at 22:08, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> The following changes since commit 2b81c046252fbfb375ad30632362fc16e6e22bd5: >> >> Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2024-09-17 14:02:18 +0100) >> >> are available in the Git repository at: >> >> https://gitlab.com/stsquad/qemu.git tags/pull-tcg-plugin-memory-180924-2 >> >> for you to fetch changes up to a33f4871e0a0f4bf1cb037ab29fae7df7f2fc658: >> >> contrib/plugins: avoid hanging program (2024-09-18 21:02:36 +0100) >> >> ---------------------------------------------------------------- >> TCG plugin memory instrumentation updates >> >> - deprecate plugins on 32 bit hosts >> - deprecate plugins with TCI >> - extend memory API to save value >> - add check-tcg tests to exercise new memory API >> - fix timer deadlock with non-changing timer >> - add basic block vector plugin to contrib >> - add cflow plugin to contrib >> - extend syscall plugin to dump write memory >> - validate ips plugin arguments meet minimum slice value >> >> ---------------------------------------------------------------- > > Fails to build on macos: > https://gitlab.com/qemu-project/qemu/-/jobs/7865151156 > > ../tests/tcg/plugins/mem.c:12:10: fatal error: 'endian.h' file not found > > endian.h is a Linuxism. Doh - I'd written it off the failure as waiting for the MacOS bump and didn't see the actual error. I'll see what we can do. > > While I'm looking at the code, this caught my eye: > > case QEMU_PLUGIN_MEM_VALUE_U64: > { > uint64_t *p = (uint64_t *) &ri->data[offset]; > uint64_t val = be ? htobe64(value.data.u64) : htole64(value.data.u64); > if (is_store) { > *p = val; > } else if (*p != val) { > unseen_data = true; > } > break; > } > > Casting a random byte pointer to uint64_t* like that > and dereferencing it isn't valid -- it can fault if > it's not aligned correctly. Hmm in the normal case of x86 hosts we will never hit this. I guess we could do a memcpy step and then the byteswap? > I suspect the plugin needs to define versions of at least some > of the functionality in qemu's include/qemu/bswap.h. Could it be included directly without bringing in the rest of QEMU's include deps? > > thanks > -- PMM
On Thu, 19 Sept 2024 at 14:11, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > While I'm looking at the code, this caught my eye: > > > > case QEMU_PLUGIN_MEM_VALUE_U64: > > { > > uint64_t *p = (uint64_t *) &ri->data[offset]; > > uint64_t val = be ? htobe64(value.data.u64) : htole64(value.data.u64); > > if (is_store) { > > *p = val; > > } else if (*p != val) { > > unseen_data = true; > > } > > break; > > } > > > > Casting a random byte pointer to uint64_t* like that > > and dereferencing it isn't valid -- it can fault if > > it's not aligned correctly. > > Hmm in the normal case of x86 hosts we will never hit this. Not necessarily -- some x86 SIMD insns enforce alignment. > I guess we > could do a memcpy step and then the byteswap? That's what bswap.h does, yes. > Could it be included directly without bringing in the rest of QEMU's > include deps? It's technically quite close to standalone I think, but I think it would be a bad idea to directly include it because once you put QEMU's include/ on the plugin compile include path then that's a slippery slope to the plugins not actually being standalone code any more. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 19 Sept 2024 at 14:11, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> > While I'm looking at the code, this caught my eye: >> > >> > case QEMU_PLUGIN_MEM_VALUE_U64: >> > { >> > uint64_t *p = (uint64_t *) &ri->data[offset]; >> > uint64_t val = be ? htobe64(value.data.u64) : htole64(value.data.u64); >> > if (is_store) { >> > *p = val; >> > } else if (*p != val) { >> > unseen_data = true; >> > } >> > break; >> > } >> > >> > Casting a random byte pointer to uint64_t* like that >> > and dereferencing it isn't valid -- it can fault if >> > it's not aligned correctly. >> >> Hmm in the normal case of x86 hosts we will never hit this. > > Not necessarily -- some x86 SIMD insns enforce alignment. > >> I guess we >> could do a memcpy step and then the byteswap? > > That's what bswap.h does, yes. > >> Could it be included directly without bringing in the rest of QEMU's >> include deps? > > It's technically quite close to standalone I think, > but I think it would be a bad idea to directly include > it because once you put QEMU's include/ on the plugin > compile include path then that's a slippery slope to > the plugins not actually being standalone code any more. In this case tests/tcg/plugins are built for testing the implementation. We could let it slide to save on just copy and pasting the whole file: --8<---------------cut here---------------start------------->8--- modified tests/tcg/plugins/mem.c @@ -9,10 +9,23 @@ #include <stdlib.h> #include <string.h> #include <unistd.h> -#include <endian.h> #include <stdio.h> #include <glib.h> +/* + * plugins should not include anything from QEMU aside from the + * API header. However the bswap.h header is sufficiently self + * contained that we include it here to avoid code duplication. + */ +#define HOST_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__) +#define HOST_LONG_BITS (__SIZEOF_POINTER__ * 8) +#ifndef glue +#define xglue(x, y) x ## y +#define glue(x, y) xglue(x, y) +#define stringify(s) tostring(s) +#define tostring(s) #s +#endif +#include <bswap.h> #include <qemu-plugin.h> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; @@ -154,33 +167,45 @@ static void update_region_info(uint64_t region, uint64_t offset, case QEMU_PLUGIN_MEM_VALUE_U16: { uint16_t *p = (uint16_t *) &ri->data[offset]; - uint16_t val = be ? htobe16(value.data.u16) : htole16(value.data.u16); if (is_store) { - *p = val; - } else if (*p != val) { - unseen_data = true; + if (be) { + stw_be_p(p, value.data.u16); + } else { + stw_le_p(p, value.data.u16); + } + } else { + uint16_t val = be ? lduw_be_p(p) : lduw_le_p(p); + unseen_data = val != value.data.u16; } break; } case QEMU_PLUGIN_MEM_VALUE_U32: { uint32_t *p = (uint32_t *) &ri->data[offset]; - uint32_t val = be ? htobe32(value.data.u32) : htole32(value.data.u32); if (is_store) { - *p = val; - } else if (*p != val) { - unseen_data = true; + if (be) { + stl_be_p(p, value.data.u32); + } else { + stl_le_p(p, value.data.u32); + } + } else { + uint32_t val = be ? ldl_be_p(p) : ldl_le_p(p); + unseen_data = val != value.data.u32; } break; } case QEMU_PLUGIN_MEM_VALUE_U64: { uint64_t *p = (uint64_t *) &ri->data[offset]; - uint64_t val = be ? htobe64(value.data.u64) : htole64(value.data.u64); if (is_store) { - *p = val; - } else if (*p != val) { - unseen_data = true; + if (be) { + stq_be_p(p, value.data.u64); + } else { + stq_le_p(p, value.data.u64); + } + } else { + uint64_t val = be ? ldq_be_p(p) : ldq_le_p(p); + unseen_data = val != value.data.u64; } break; --8<---------------cut here---------------end--------------->8---
The following changes since commit 2b81c046252fbfb375ad30632362fc16e6e22bd5: Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2024-09-17 14:02:18 +0100) are available in the Git repository at: https://gitlab.com/stsquad/qemu.git tags/pull-tcg-plugin-memory-180924-2 for you to fetch changes up to a33f4871e0a0f4bf1cb037ab29fae7df7f2fc658: contrib/plugins: avoid hanging program (2024-09-18 21:02:36 +0100) ---------------------------------------------------------------- TCG plugin memory instrumentation updates - deprecate plugins on 32 bit hosts - deprecate plugins with TCI - extend memory API to save value - add check-tcg tests to exercise new memory API - fix timer deadlock with non-changing timer - add basic block vector plugin to contrib - add cflow plugin to contrib - extend syscall plugin to dump write memory - validate ips plugin arguments meet minimum slice value ---------------------------------------------------------------- Akihiko Odaki (1): contrib/plugins: Add a plugin to generate basic block vectors Alex Bennée (9): deprecation: don't enable TCG plugins by default on 32 bit hosts deprecation: don't enable TCG plugins by default with TCI contrib/plugins: control flow plugin tests/tcg: clean up output of memory system test tests/tcg: only read/write 64 bit words on 64 bit systems tests/tcg: ensure s390x-softmmu output redirected tests/tcg: add a system test to check memory instrumentation util/timer: avoid deadlock when shutting down contrib/plugins: avoid hanging program 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/tcg/plugins/mem: add option to print memory accesses tests/tcg/multiarch: add test for plugin memory access Rowan Hart (2): plugins: add plugin API to read guest memory plugins: add option to dump write argument to syscall plugin docs/about/deprecated.rst | 19 + docs/about/emulation.rst | 44 ++- configure | 32 +- accel/tcg/atomic_template.h | 66 +++- include/hw/core/cpu.h | 4 + include/qemu/plugin.h | 4 + include/qemu/qemu-plugin.h | 64 +++- contrib/plugins/bbv.c | 158 +++++++++ contrib/plugins/cflow.c | 388 +++++++++++++++++++++ contrib/plugins/ips.c | 6 + plugins/api.c | 53 +++ plugins/core.c | 6 + tcg/tcg-op-ldst.c | 66 +++- tests/tcg/multiarch/system/memory.c | 123 ++++--- tests/tcg/multiarch/test-plugin-mem-access.c | 177 ++++++++++ tests/tcg/plugins/mem.c | 250 ++++++++++++- tests/tcg/plugins/syscall.c | 117 +++++++ util/qemu-timer.c | 14 +- accel/tcg/atomic_common.c.inc | 13 +- accel/tcg/ldst_common.c.inc | 38 +- contrib/plugins/Makefile | 2 + plugins/qemu-plugins.symbols | 2 + tests/tcg/Makefile.target | 12 +- tests/tcg/alpha/Makefile.softmmu-target | 2 +- tests/tcg/alpha/Makefile.target | 3 + tests/tcg/multiarch/Makefile.target | 11 + tests/tcg/multiarch/check-plugin-output.sh | 36 ++ tests/tcg/multiarch/system/Makefile.softmmu-target | 6 + .../tcg/multiarch/system/validate-memory-counts.py | 130 +++++++ tests/tcg/ppc64/Makefile.target | 5 + tests/tcg/s390x/Makefile.softmmu-target | 8 +- 31 files changed, 1776 insertions(+), 83 deletions(-) create mode 100644 contrib/plugins/bbv.c create mode 100644 contrib/plugins/cflow.c create mode 100644 tests/tcg/multiarch/test-plugin-mem-access.c create mode 100755 tests/tcg/multiarch/check-plugin-output.sh create mode 100755 tests/tcg/multiarch/system/validate-memory-counts.py