| Message ID | 20230419161226.1012884-1-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] plugins: force slow path when plugins instrument memory ops | expand |
On 4/19/23 17:12, Alex Bennée wrote: > The lack of SVE memory instrumentation has been an omission in plugin > handling since it was introduced. Fortunately we can utilise the > probe_* functions to force all all memory access to follow the slow > path. We do this by checking the access type and presence of plugin > memory callbacks and if set return the TLB_MMIO flag. > > We have to jump through a few hoops in user mode to re-use the flag > but it was the desired effect: > > ./qemu-system-aarch64 -display none -serial mon:stdio \ > -M virt -cpu max -semihosting-config enable=on \ > -kernel ./tests/tcg/aarch64-softmmu/memory-sve \ > -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin > > gives (disas doesn't currently understand st1w): > > 0, 0x40001808, 0xe54342a0, ".byte 0xa0, 0x42, 0x43, 0xe5", store, 0x40213010, RAM, store, 0x40213014, RAM, store, 0x40213018, RAM > > And for user-mode: > > ./qemu-aarch64 \ > -plugin contrib/plugins/libexeclog.so,afilter=0x4007c0 \ > -d plugin \ > ./tests/tcg/aarch64-linux-user/sha512-sve > > gives: > > 1..10 > ok 1 - do_test(&tests[i]) > 0, 0x4007c0, 0xa4004b80, ".byte 0x80, 0x4b, 0x00, 0xa4", load, 0x5500800370, load, 0x5500800371, load, 0x5500800372, load, 0x5500800373, load, 0x5500800374, load, 0x5500800375, load, 0x5500800376, load, 0x5500800377, load, 0x5500800378, load, 0x5500800379, load, 0x550080037a, load, 0x550080037b, load, 0x550080037c, load, 0x550080037d, load, 0x550080037e, load, 0x550080037f, load, 0x5500800380, load, 0x5500800381, load, 0x5500800382, load, 0x5500800383, load, 0x5500800384, load, 0x5500800385, load, 0x5500800386, lo > ad, 0x5500800387, load, 0x5500800388, load, 0x5500800389, load, 0x550080038a, load, 0x550080038b, load, 0x550080038c, load, 0x550080038d, load, 0x550080038e, load, 0x550080038f, load, 0x5500800390, load, 0x5500800391, load, 0x5500800392, load, 0x5500800393, load, 0x5500800394, load, 0x5500800395, load, 0x5500800396, load, 0x5500800397, load, 0x5500800398, load, 0x5500800399, load, 0x550080039a, load, 0x550080039b, load, 0x550080039c, load, 0x550080039d, load, 0x550080039e, load, 0x550080039f, load, 0x55008003a0, load, 0x55008003a1, load, 0x55008003a2, load, 0x55008003a3, load, 0x55008003a4, load, 0x55008003a5, load, 0x55008003a6, load, 0x55008003a7, load, 0x55008003a8, load, 0x55008003a9, load, 0x55008003aa, load, 0x55008003ab, load, 0x55008003ac, load, 0x55008003ad, load, 0x55008003ae, load, 0x55008003af > > (4007c0 is the ld1b in the sha512-sve) > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Robert Henry <robhenry@microsoft.com> > Cc: Aaron Lindsay <aaron@os.amperecomputing.com> > --- > include/exec/cpu-all.h | 2 +- > include/hw/core/cpu.h | 17 +++++++++++++++++ > accel/tcg/cputlb.c | 6 +++++- > accel/tcg/user-exec.c | 6 +++++- > target/arm/tcg/sve_helper.c | 4 ---- > tests/tcg/aarch64/Makefile.target | 8 ++++++++ > 6 files changed, 36 insertions(+), 7 deletions(-) Looks good, mostly. > @@ -1530,6 +1530,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, > target_ulong tlb_addr, page_addr; > size_t elt_ofs; > int flags; > + bool not_fetch = true; > > switch (access_type) { > case MMU_DATA_LOAD: > @@ -1540,6 +1541,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, > break; > case MMU_INST_FETCH: > elt_ofs = offsetof(CPUTLBEntry, addr_code); > + not_fetch = false; > break; > default: > g_assert_not_reached(); > @@ -1578,7 +1580,9 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, > *pfull = &env_tlb(env)->d[mmu_idx].fulltlb[index]; > > /* Fold all "mmio-like" bits into TLB_MMIO. This is not RAM. */ > - if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))) { > + if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY)) > + || > + (not_fetch && cpu_plugin_mem_cbs_enabled(env_cpu(env)))) { Rather than introduce a new variable, just test access_type != MMU_INST_FETCH. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 4/19/23 17:12, Alex Bennée wrote: >> The lack of SVE memory instrumentation has been an omission in plugin >> handling since it was introduced. Fortunately we can utilise the >> probe_* functions to force all all memory access to follow the slow >> path. We do this by checking the access type and presence of plugin >> memory callbacks and if set return the TLB_MMIO flag. >> We have to jump through a few hoops in user mode to re-use the flag >> but it was the desired effect: >> ./qemu-system-aarch64 -display none -serial mon:stdio \ >> -M virt -cpu max -semihosting-config enable=on \ >> -kernel ./tests/tcg/aarch64-softmmu/memory-sve \ >> -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin >> gives (disas doesn't currently understand st1w): >> 0, 0x40001808, 0xe54342a0, ".byte 0xa0, 0x42, 0x43, 0xe5", store, >> 0x40213010, RAM, store, 0x40213014, RAM, store, 0x40213018, RAM >> And for user-mode: >> ./qemu-aarch64 \ >> -plugin contrib/plugins/libexeclog.so,afilter=0x4007c0 \ >> -d plugin \ >> ./tests/tcg/aarch64-linux-user/sha512-sve >> gives: >> 1..10 >> ok 1 - do_test(&tests[i]) >> 0, 0x4007c0, 0xa4004b80, ".byte 0x80, 0x4b, 0x00, 0xa4", load, 0x5500800370, load, 0x5500800371, load, 0x5500800372, load, 0x5500800373, load, 0x5500800374, load, 0x5500800375, load, 0x5500800376, load, 0x5500800377, load, 0x5500800378, load, 0x5500800379, load, 0x550080037a, load, 0x550080037b, load, 0x550080037c, load, 0x550080037d, load, 0x550080037e, load, 0x550080037f, load, 0x5500800380, load, 0x5500800381, load, 0x5500800382, load, 0x5500800383, load, 0x5500800384, load, 0x5500800385, load, 0x5500800386, lo >> ad, 0x5500800387, load, 0x5500800388, load, 0x5500800389, load, 0x550080038a, load, 0x550080038b, load, 0x550080038c, load, 0x550080038d, load, 0x550080038e, load, 0x550080038f, load, 0x5500800390, load, 0x5500800391, load, 0x5500800392, load, 0x5500800393, load, 0x5500800394, load, 0x5500800395, load, 0x5500800396, load, 0x5500800397, load, 0x5500800398, load, 0x5500800399, load, 0x550080039a, load, 0x550080039b, load, 0x550080039c, load, 0x550080039d, load, 0x550080039e, load, 0x550080039f, load, 0x55008003a0, load, 0x55008003a1, load, 0x55008003a2, load, 0x55008003a3, load, 0x55008003a4, load, 0x55008003a5, load, 0x55008003a6, load, 0x55008003a7, load, 0x55008003a8, load, 0x55008003a9, load, 0x55008003aa, load, 0x55008003ab, load, 0x55008003ac, load, 0x55008003ad, load, 0x55008003ae, load, 0x55008003af >> (4007c0 is the ld1b in the sha512-sve) >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Cc: Robert Henry <robhenry@microsoft.com> >> Cc: Aaron Lindsay <aaron@os.amperecomputing.com> >> --- >> include/exec/cpu-all.h | 2 +- >> include/hw/core/cpu.h | 17 +++++++++++++++++ >> accel/tcg/cputlb.c | 6 +++++- >> accel/tcg/user-exec.c | 6 +++++- >> target/arm/tcg/sve_helper.c | 4 ---- >> tests/tcg/aarch64/Makefile.target | 8 ++++++++ >> 6 files changed, 36 insertions(+), 7 deletions(-) > > Looks good, mostly. > >> @@ -1530,6 +1530,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, >> target_ulong tlb_addr, page_addr; >> size_t elt_ofs; >> int flags; >> + bool not_fetch = true; >> switch (access_type) { >> case MMU_DATA_LOAD: >> @@ -1540,6 +1541,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, >> break; >> case MMU_INST_FETCH: >> elt_ofs = offsetof(CPUTLBEntry, addr_code); >> + not_fetch = false; >> break; >> default: >> g_assert_not_reached(); >> @@ -1578,7 +1580,9 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, >> *pfull = &env_tlb(env)->d[mmu_idx].fulltlb[index]; >> /* Fold all "mmio-like" bits into TLB_MMIO. This is not >> RAM. */ >> - if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))) { >> + if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY)) >> + || >> + (not_fetch && cpu_plugin_mem_cbs_enabled(env_cpu(env)))) { > > Rather than introduce a new variable, just test access_type != > MMU_INST_FETCH. w.r.t to not instrumenting the TLB accesses how ugly would something like this be: --8<---------------cut here---------------start------------->8--- modified include/hw/core/cpu.h @@ -80,10 +80,24 @@ DECLARE_CLASS_CHECKERS(CPUClass, CPU, typedef struct ArchCPU CpuInstanceType; \ OBJECT_DECLARE_TYPE(ArchCPU, CpuClassType, CPU_MODULE_OBJ_NAME); +/** + * typedef MMUAccessType - describe the type of access for cputlb + * + * When handling the access to memory we need to know the type of + * access we are doing. Loads and store rely on read and write page + * permissions where as the instruction fetch relies on execute + * permissions. Additional bits are used for TLB access so we can + * suppress instrumentation of memory when the CPU is probing. + */ typedef enum MMUAccessType { MMU_DATA_LOAD = 0, MMU_DATA_STORE = 1, - MMU_INST_FETCH = 2 + MMU_INST_FETCH = 2, + /* MMU Mask */ + MMU_VALID_MASK = (MMU_DATA_LOAD | MMU_DATA_STORE | MMU_INST_FETCH), + /* Represents the CPU walking the page table */ + MMU_TLB_ACCESS = 0x4, + MMU_TLB_LOAD = MMU_DATA_LOAD | MMU_TLB_ACCESS } MMUAccessType; typedef struct CPUWatchpoint CPUWatchpoint; modified accel/tcg/cputlb.c @@ -1503,11 +1503,12 @@ static void notdirty_write(CPUState *cpu, vaddr mem_vaddr, unsigned size, } static int probe_access_internal(CPUArchState *env, target_ulong addr, - int fault_size, MMUAccessType access_type, + int fault_size, MMUAccessType full_access_type, int mmu_idx, bool nonfault, void **phost, CPUTLBEntryFull **pfull, uintptr_t retaddr) { + MMUAccessType access_type = full_access_type & MMU_VALID_MASK; uintptr_t index = tlb_index(env, mmu_idx, addr); CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); target_ulong tlb_addr = tlb_read_idx(entry, access_type); @@ -1546,7 +1547,9 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, /* Fold all "mmio-like" bits into TLB_MMIO. This is not RAM. */ if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY)) || - (access_type != MMU_INST_FETCH && cpu_plugin_mem_cbs_enabled(env_cpu(env)))) { + (access_type != MMU_INST_FETCH && + !(full_access_type & MMU_TLB_ACCESS) && + cpu_plugin_mem_cbs_enabled(env_cpu(env)))) { *phost = NULL; return TLB_MMIO; } --8<---------------cut here---------------end--------------->8---
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 090922e4a8..1064cb05b5 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -366,7 +366,7 @@ CPUArchState *cpu_copy(CPUArchState *env); * be signaled by probe_access_flags(). */ #define TLB_INVALID_MASK (1 << (TARGET_PAGE_BITS_MIN - 1)) -#define TLB_MMIO 0 +#define TLB_MMIO (1 << (TARGET_PAGE_BITS_MIN - 2)) #define TLB_WATCHPOINT 0 #else diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 397fd3ac68..2901b33013 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -979,6 +979,23 @@ void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint); void cpu_watchpoint_remove_all(CPUState *cpu, int mask); #endif +/** + * cpu_plugin_mem_cbs_enabled() - are plugin memory callbacks enabled? + * @cs: CPUState pointer + * + * The memory callbacks are installed if a plugin has instrumented an + * instruction for memory. This can be useful to know if you want to + * force a slow path for a series of memory accesses. + */ +static inline bool cpu_plugin_mem_cbs_enabled(const CPUState *cpu) +{ +#ifdef CONFIG_PLUGIN + return !!cpu->plugin_mem_cbs; +#else + return false; +#endif +} + /** * cpu_get_address_space: * @cpu: CPU to get address space from diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index e984a98dc4..0260587806 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1530,6 +1530,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, target_ulong tlb_addr, page_addr; size_t elt_ofs; int flags; + bool not_fetch = true; switch (access_type) { case MMU_DATA_LOAD: @@ -1540,6 +1541,7 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, break; case MMU_INST_FETCH: elt_ofs = offsetof(CPUTLBEntry, addr_code); + not_fetch = false; break; default: g_assert_not_reached(); @@ -1578,7 +1580,9 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, *pfull = &env_tlb(env)->d[mmu_idx].fulltlb[index]; /* Fold all "mmio-like" bits into TLB_MMIO. This is not RAM. */ - if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY))) { + if (unlikely(flags & ~(TLB_WATCHPOINT | TLB_NOTDIRTY)) + || + (not_fetch && cpu_plugin_mem_cbs_enabled(env_cpu(env)))) { *phost = NULL; return TLB_MMIO; } diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index a7e0c3e2f4..148d525810 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -745,6 +745,10 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr, if (guest_addr_valid_untagged(addr)) { int page_flags = page_get_flags(addr); if (page_flags & acc_flag) { + if ((acc_flag == PAGE_READ || acc_flag == PAGE_WRITE) + && cpu_plugin_mem_cbs_enabled(env_cpu(env))) { + return TLB_MMIO; + } return 0; /* success */ } maperr = !(page_flags & PAGE_VALID); @@ -767,7 +771,7 @@ int probe_access_flags(CPUArchState *env, target_ulong addr, int size, g_assert(-(addr | TARGET_PAGE_MASK) >= size); flags = probe_access_internal(env, addr, size, access_type, nonfault, ra); - *phost = flags ? NULL : g2h(env_cpu(env), addr); + *phost = (flags & TLB_INVALID_MASK) ? NULL : g2h(env_cpu(env), addr); return flags; } diff --git a/target/arm/tcg/sve_helper.c b/target/arm/tcg/sve_helper.c index ccf5e5beca..eb4ebee1e4 100644 --- a/target/arm/tcg/sve_helper.c +++ b/target/arm/tcg/sve_helper.c @@ -5688,9 +5688,6 @@ void sve_ldN_r(CPUARMState *env, uint64_t *vg, const target_ulong addr, flags = info.page[0].flags | info.page[1].flags; if (unlikely(flags != 0)) { -#ifdef CONFIG_USER_ONLY - g_assert_not_reached(); -#else /* * At least one page includes MMIO. * Any bus operation can fail with cpu_transaction_failed, @@ -5727,7 +5724,6 @@ void sve_ldN_r(CPUARMState *env, uint64_t *vg, const target_ulong addr, memcpy(&env->vfp.zregs[(rd + i) & 31], &scratch[i], reg_max); } return; -#endif } /* The entire operation is in RAM, on valid pages. */ diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target index 9e91a20b0d..35d3d2348e 100644 --- a/tests/tcg/aarch64/Makefile.target +++ b/tests/tcg/aarch64/Makefile.target @@ -81,6 +81,14 @@ sha512-vector: sha512.c TESTS += sha512-vector +ifneq ($(CROSS_CC_HAS_SVE),) +sha512-sve: CFLAGS=-O3 -march=armv8.1-a+sve +sha512-sve: sha512.c + $(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS) + +TESTS += sha512-sve +endif + ifeq ($(HOST_GDB_SUPPORTS_ARCH),y) GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
The lack of SVE memory instrumentation has been an omission in plugin handling since it was introduced. Fortunately we can utilise the probe_* functions to force all all memory access to follow the slow path. We do this by checking the access type and presence of plugin memory callbacks and if set return the TLB_MMIO flag. We have to jump through a few hoops in user mode to re-use the flag but it was the desired effect: ./qemu-system-aarch64 -display none -serial mon:stdio \ -M virt -cpu max -semihosting-config enable=on \ -kernel ./tests/tcg/aarch64-softmmu/memory-sve \ -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin gives (disas doesn't currently understand st1w): 0, 0x40001808, 0xe54342a0, ".byte 0xa0, 0x42, 0x43, 0xe5", store, 0x40213010, RAM, store, 0x40213014, RAM, store, 0x40213018, RAM And for user-mode: ./qemu-aarch64 \ -plugin contrib/plugins/libexeclog.so,afilter=0x4007c0 \ -d plugin \ ./tests/tcg/aarch64-linux-user/sha512-sve gives: 1..10 ok 1 - do_test(&tests[i]) 0, 0x4007c0, 0xa4004b80, ".byte 0x80, 0x4b, 0x00, 0xa4", load, 0x5500800370, load, 0x5500800371, load, 0x5500800372, load, 0x5500800373, load, 0x5500800374, load, 0x5500800375, load, 0x5500800376, load, 0x5500800377, load, 0x5500800378, load, 0x5500800379, load, 0x550080037a, load, 0x550080037b, load, 0x550080037c, load, 0x550080037d, load, 0x550080037e, load, 0x550080037f, load, 0x5500800380, load, 0x5500800381, load, 0x5500800382, load, 0x5500800383, load, 0x5500800384, load, 0x5500800385, load, 0x5500800386, lo ad, 0x5500800387, load, 0x5500800388, load, 0x5500800389, load, 0x550080038a, load, 0x550080038b, load, 0x550080038c, load, 0x550080038d, load, 0x550080038e, load, 0x550080038f, load, 0x5500800390, load, 0x5500800391, load, 0x5500800392, load, 0x5500800393, load, 0x5500800394, load, 0x5500800395, load, 0x5500800396, load, 0x5500800397, load, 0x5500800398, load, 0x5500800399, load, 0x550080039a, load, 0x550080039b, load, 0x550080039c, load, 0x550080039d, load, 0x550080039e, load, 0x550080039f, load, 0x55008003a0, load, 0x55008003a1, load, 0x55008003a2, load, 0x55008003a3, load, 0x55008003a4, load, 0x55008003a5, load, 0x55008003a6, load, 0x55008003a7, load, 0x55008003a8, load, 0x55008003a9, load, 0x55008003aa, load, 0x55008003ab, load, 0x55008003ac, load, 0x55008003ad, load, 0x55008003ae, load, 0x55008003af (4007c0 is the ld1b in the sha512-sve) Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Robert Henry <robhenry@microsoft.com> Cc: Aaron Lindsay <aaron@os.amperecomputing.com> --- include/exec/cpu-all.h | 2 +- include/hw/core/cpu.h | 17 +++++++++++++++++ accel/tcg/cputlb.c | 6 +++++- accel/tcg/user-exec.c | 6 +++++- target/arm/tcg/sve_helper.c | 4 ---- tests/tcg/aarch64/Makefile.target | 8 ++++++++ 6 files changed, 36 insertions(+), 7 deletions(-)