| Message ID | 20191014104948.4291-24-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Support for TCG plugins | expand |
On 10/14/19 3:49 AM, Alex Bennée wrote: > From: "Emilio G. Cota" <cota@braap.org> > > We don't bother with replicating the fast path (tlb_hit) of the old > cpu_ldst helpers as it has no measurable effect on performance. This > probably indicates we should consider flattening the whole set of > helpers but that is out of scope for this change. > > Suggested-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> > [AJB: directly plumb into softmmu/user helpers] > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v4 > - don't use the cpu_ldst helpers, plumb directly into the lower > level > - mark the CODE_ACCESS/SOFTMMU_CODE_ACCESS as deprecated > v5 > - expand commit message w.r.t. fast path. > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, 14 Oct 2019 at 12:38, Alex Bennée <alex.bennee@linaro.org> wrote: > > From: "Emilio G. Cota" <cota@braap.org> > > We don't bother with replicating the fast path (tlb_hit) of the old > cpu_ldst helpers as it has no measurable effect on performance. This > probably indicates we should consider flattening the whole set of > helpers but that is out of scope for this change. > > Suggested-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Emilio G. Cota <cota@braap.org> > [AJB: directly plumb into softmmu/user helpers] > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > diff --git a/tcg/tcg.h b/tcg/tcg.h > index a38659ea5b..302533b463 100644 > --- a/tcg/tcg.h > +++ b/tcg/tcg.h > @@ -1317,6 +1317,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, > # define helper_ret_stl_mmu helper_be_stl_mmu > # define helper_ret_stq_mmu helper_be_stq_mmu > # define helper_ret_ldw_cmmu helper_be_ldw_cmmu > +# define helper_ret_lduw_cmmu helper_be_ldw_cmmu > # define helper_ret_ldl_cmmu helper_be_ldl_cmmu > # define helper_ret_ldq_cmmu helper_be_ldq_cmmu > #else > @@ -1330,6 +1331,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, > # define helper_ret_stl_mmu helper_le_stl_mmu > # define helper_ret_stq_mmu helper_le_stq_mmu > # define helper_ret_ldw_cmmu helper_le_ldw_cmmu > +# define helper_ret_lduw_cmmu helper_le_ldw_cmmu > # define helper_ret_ldl_cmmu helper_le_ldl_cmmu > # define helper_ret_ldq_cmmu helper_le_ldq_cmmu > #endif This looks odd. Why is it ok to define a 'lduw' helper as the 'ldw' cmmu helper ? One ought to be sign extending and the other not... thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 14 Oct 2019 at 12:38, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> From: "Emilio G. Cota" <cota@braap.org> >> >> We don't bother with replicating the fast path (tlb_hit) of the old >> cpu_ldst helpers as it has no measurable effect on performance. This >> probably indicates we should consider flattening the whole set of >> helpers but that is out of scope for this change. >> >> Suggested-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Emilio G. Cota <cota@braap.org> >> [AJB: directly plumb into softmmu/user helpers] >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> diff --git a/tcg/tcg.h b/tcg/tcg.h >> index a38659ea5b..302533b463 100644 >> --- a/tcg/tcg.h >> +++ b/tcg/tcg.h >> @@ -1317,6 +1317,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, >> # define helper_ret_stl_mmu helper_be_stl_mmu >> # define helper_ret_stq_mmu helper_be_stq_mmu >> # define helper_ret_ldw_cmmu helper_be_ldw_cmmu >> +# define helper_ret_lduw_cmmu helper_be_ldw_cmmu >> # define helper_ret_ldl_cmmu helper_be_ldl_cmmu >> # define helper_ret_ldq_cmmu helper_be_ldq_cmmu >> #else >> @@ -1330,6 +1331,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, >> # define helper_ret_stl_mmu helper_le_stl_mmu >> # define helper_ret_stq_mmu helper_le_stq_mmu >> # define helper_ret_ldw_cmmu helper_le_ldw_cmmu >> +# define helper_ret_lduw_cmmu helper_le_ldw_cmmu >> # define helper_ret_ldl_cmmu helper_le_ldl_cmmu >> # define helper_ret_ldq_cmmu helper_le_ldq_cmmu >> #endif > > This looks odd. Why is it ok to define a 'lduw' helper > as the 'ldw' cmmu helper ? One ought to be sign > extending and the other not... This was attempting to make things line up between the softmmu helpers and the user-mode ld*_p helpers that we need to expand to. I'm not sure a sign extending loader even makes sense for code load anyway. > > thanks > -- PMM -- Alex Bennée
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 14 Oct 2019 at 12:38, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> From: "Emilio G. Cota" <cota@braap.org> >> >> We don't bother with replicating the fast path (tlb_hit) of the old >> cpu_ldst helpers as it has no measurable effect on performance. This >> probably indicates we should consider flattening the whole set of >> helpers but that is out of scope for this change. >> >> Suggested-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Emilio G. Cota <cota@braap.org> >> [AJB: directly plumb into softmmu/user helpers] >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> diff --git a/tcg/tcg.h b/tcg/tcg.h >> index a38659ea5b..302533b463 100644 >> --- a/tcg/tcg.h >> +++ b/tcg/tcg.h >> @@ -1317,6 +1317,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, >> # define helper_ret_stl_mmu helper_be_stl_mmu >> # define helper_ret_stq_mmu helper_be_stq_mmu >> # define helper_ret_ldw_cmmu helper_be_ldw_cmmu >> +# define helper_ret_lduw_cmmu helper_be_ldw_cmmu >> # define helper_ret_ldl_cmmu helper_be_ldl_cmmu >> # define helper_ret_ldq_cmmu helper_be_ldq_cmmu >> #else >> @@ -1330,6 +1331,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, >> # define helper_ret_stl_mmu helper_le_stl_mmu >> # define helper_ret_stq_mmu helper_le_stq_mmu >> # define helper_ret_ldw_cmmu helper_le_ldw_cmmu >> +# define helper_ret_lduw_cmmu helper_le_ldw_cmmu >> # define helper_ret_ldl_cmmu helper_le_ldl_cmmu >> # define helper_ret_ldq_cmmu helper_le_ldq_cmmu >> #endif > > This looks odd. Why is it ok to define a 'lduw' helper > as the 'ldw' cmmu helper ? One ought to be sign > extending and the other not... This is the alternative: 3 files changed, 9 insertions(+), 17 deletions(-) include/exec/translator.h | 19 +++++++++---------- include/qemu/bswap.h | 5 ----- tcg/tcg.h | 2 -- modified include/exec/translator.h @@ -158,26 +158,26 @@ void translator_loop_temp_check(DisasContextBase *db); #ifdef CONFIG_USER_ONLY -#define DO_LOAD(type, name, shift) \ +#define DO_LOAD(type, name, uname, shift) \ set_helper_retaddr(1); \ - ret = name ## _p(g2h(pc)); \ + ret = uname ## _p(g2h(pc)); \ clear_helper_retaddr(); #else -#define DO_LOAD(type, name, shift) \ +#define DO_LOAD(type, name, uname, shift) \ int mmu_idx = cpu_mmu_index(env, true); \ TCGMemOpIdx oi = make_memop_idx(shift, mmu_idx); \ ret = helper_ret_ ## name ## _cmmu(env, pc, oi, 0); #endif -#define GEN_TRANSLATOR_LD(fullname, name, type, shift, swap_fn) \ +#define GEN_TRANSLATOR_LD(fullname, name, uname, type, shift, swap_fn) \ static inline type \ fullname ## _swap(CPUArchState *env, abi_ptr pc, bool do_swap) \ { \ type ret; \ - DO_LOAD(type, name, shift) \ + DO_LOAD(type, name, uname, shift) \ \ if (do_swap) { \ ret = swap_fn(ret); \ @@ -191,11 +191,10 @@ void translator_loop_temp_check(DisasContextBase *db); return fullname ## _swap(env, pc, false); \ } -GEN_TRANSLATOR_LD(translator_ldub, ldb, uint8_t, 0, /* no swap needed */) -GEN_TRANSLATOR_LD(translator_ldsw, lduw, int16_t, 1, bswap16) -GEN_TRANSLATOR_LD(translator_lduw, lduw, uint16_t, 1, bswap16) -GEN_TRANSLATOR_LD(translator_ldl, ldl, uint32_t, 2, bswap32) -GEN_TRANSLATOR_LD(translator_ldq, ldq, uint64_t, 3, bswap64) +GEN_TRANSLATOR_LD(translator_ldub, ldb, ldub, uint8_t, 0, /* no swap needed */) +GEN_TRANSLATOR_LD(translator_ldw, ldw, lduw, uint16_t, 1, bswap16) +GEN_TRANSLATOR_LD(translator_ldl, ldl, ldl, uint32_t, 2, bswap32) +GEN_TRANSLATOR_LD(translator_ldq, ldq, ldl, uint64_t, 3, bswap64) #undef GEN_TRANSLATOR_LD #endif /* EXEC__TRANSLATOR_H */ modified include/qemu/bswap.h @@ -306,11 +306,6 @@ static inline int ldub_p(const void *ptr) return *(uint8_t *)ptr; } -static inline int ldb_p(const void *ptr) -{ - return ldub_p(ptr); -} - static inline int ldsb_p(const void *ptr) { return *(int8_t *)ptr; modified tcg/tcg.h @@ -1317,7 +1317,6 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, # define helper_ret_stl_mmu helper_be_stl_mmu # define helper_ret_stq_mmu helper_be_stq_mmu # define helper_ret_ldw_cmmu helper_be_ldw_cmmu -# define helper_ret_lduw_cmmu helper_be_ldw_cmmu # define helper_ret_ldl_cmmu helper_be_ldl_cmmu # define helper_ret_ldq_cmmu helper_be_ldq_cmmu #else @@ -1331,7 +1330,6 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, # define helper_ret_stl_mmu helper_le_stl_mmu # define helper_ret_stq_mmu helper_le_stq_mmu # define helper_ret_ldw_cmmu helper_le_ldw_cmmu -# define helper_ret_lduw_cmmu helper_le_ldw_cmmu # define helper_ret_ldl_cmmu helper_le_ldl_cmmu # define helper_ret_ldq_cmmu helper_le_ldq_cmmu #endif -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Mon, 14 Oct 2019 at 12:38, Alex Bennée <alex.bennee@linaro.org> wrote: >>> >>> From: "Emilio G. Cota" <cota@braap.org> >>> >>> We don't bother with replicating the fast path (tlb_hit) of the old >>> cpu_ldst helpers as it has no measurable effect on performance. This >>> probably indicates we should consider flattening the whole set of >>> helpers but that is out of scope for this change. >>> >>> Suggested-by: Richard Henderson <richard.henderson@linaro.org> >>> Signed-off-by: Emilio G. Cota <cota@braap.org> >>> [AJB: directly plumb into softmmu/user helpers] >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> >>> diff --git a/tcg/tcg.h b/tcg/tcg.h >>> index a38659ea5b..302533b463 100644 >>> --- a/tcg/tcg.h >>> +++ b/tcg/tcg.h >>> @@ -1317,6 +1317,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, >>> # define helper_ret_stl_mmu helper_be_stl_mmu >>> # define helper_ret_stq_mmu helper_be_stq_mmu >>> # define helper_ret_ldw_cmmu helper_be_ldw_cmmu >>> +# define helper_ret_lduw_cmmu helper_be_ldw_cmmu >>> # define helper_ret_ldl_cmmu helper_be_ldl_cmmu >>> # define helper_ret_ldq_cmmu helper_be_ldq_cmmu >>> #else >>> @@ -1330,6 +1331,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, >>> # define helper_ret_stl_mmu helper_le_stl_mmu >>> # define helper_ret_stq_mmu helper_le_stq_mmu >>> # define helper_ret_ldw_cmmu helper_le_ldw_cmmu >>> +# define helper_ret_lduw_cmmu helper_le_ldw_cmmu >>> # define helper_ret_ldl_cmmu helper_le_ldl_cmmu >>> # define helper_ret_ldq_cmmu helper_le_ldq_cmmu >>> #endif >> >> This looks odd. Why is it ok to define a 'lduw' helper >> as the 'ldw' cmmu helper ? One ought to be sign >> extending and the other not... > > This was attempting to make things line up between the softmmu helpers > and the user-mode ld*_p helpers that we need to expand to. I'm not sure > a sign extending loader even makes sense for code load anyway. That last bit is not true as sign extending helpers are used for loading sign-extended immediate values. > >> >> thanks >> -- PMM -- Alex Bennée
diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h index 9151fdb042..fd499f7e2f 100644 --- a/include/exec/cpu_ldst.h +++ b/include/exec/cpu_ldst.h @@ -129,6 +129,11 @@ static inline void clear_helper_retaddr(void) #include "exec/cpu_ldst_useronly_template.h" #undef MEMSUFFIX +/* + * Code access is deprecated in favour of translator_ld* functions + * (see translator.h). However there are still users that need to + * converted so for now these stay. + */ #define MEMSUFFIX _code #define CODE_ACCESS #define DATA_SIZE 1 @@ -427,6 +432,12 @@ static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx, #undef CPU_MMU_INDEX #undef MEMSUFFIX +/* + * Code access is deprecated in favour of translator_ld* functions + * (see translator.h). However there are still users that need to + * converted so for now these stay. + */ + #define CPU_MMU_INDEX (cpu_mmu_index(env, true)) #define MEMSUFFIX _code #define SOFTMMU_CODE_ACCESS diff --git a/include/exec/translator.h b/include/exec/translator.h index 180c51d509..7a9dc1b937 100644 --- a/include/exec/translator.h +++ b/include/exec/translator.h @@ -19,7 +19,10 @@ */ +#include "qemu/bswap.h" #include "exec/exec-all.h" +#include "exec/cpu_ldst.h" +#include "exec/plugin-gen.h" #include "tcg/tcg.h" @@ -142,4 +145,57 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, void translator_loop_temp_check(DisasContextBase *db); -#endif /* EXEC__TRANSLATOR_H */ +/* + * Translator Load Functions + * + * These are intended to replace the old cpu_ld*_code functions and + * are mandatory for front-ends that have been migrated to the common + * translator_loop. These functions are only intended to be called + * from the translation stage and should not be called from helper + * functions. Those functions should be converted to encode the + * relevant information at translation time. + */ + +#ifdef CONFIG_USER_ONLY + +#define DO_LOAD(type, name, shift) \ + set_helper_retaddr(1); \ + ret = name ## _p(g2h(pc)); \ + clear_helper_retaddr(); + +#else + +#define DO_LOAD(type, name, shift) \ + int mmu_idx = cpu_mmu_index(env, true); \ + TCGMemOpIdx oi = make_memop_idx(shift, mmu_idx); \ + ret = helper_ret_ ## name ## _cmmu(env, pc, oi, 0); + +#endif + +#define GEN_TRANSLATOR_LD(fullname, name, type, shift, swap_fn) \ + static inline type \ + fullname ## _swap(CPUArchState *env, abi_ptr pc, bool do_swap) \ + { \ + type ret; \ + DO_LOAD(type, name, shift) \ + \ + if (do_swap) { \ + ret = swap_fn(ret); \ + } \ + plugin_insn_append(&ret, sizeof(ret)); \ + return ret; \ + } \ + \ + static inline type fullname(CPUArchState *env, abi_ptr pc) \ + { \ + return fullname ## _swap(env, pc, false); \ + } + +GEN_TRANSLATOR_LD(translator_ldub, ldb, uint8_t, 0, /* no swap needed */) +GEN_TRANSLATOR_LD(translator_ldsw, lduw, int16_t, 1, bswap16) +GEN_TRANSLATOR_LD(translator_lduw, lduw, uint16_t, 1, bswap16) +GEN_TRANSLATOR_LD(translator_ldl, ldl, uint32_t, 2, bswap32) +GEN_TRANSLATOR_LD(translator_ldq, ldq, uint64_t, 3, bswap64) +#undef GEN_TRANSLATOR_LD + +#endif /* EXEC__TRANSLATOR_H */ diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h index 2a9f3fe783..4f70727874 100644 --- a/include/qemu/bswap.h +++ b/include/qemu/bswap.h @@ -306,6 +306,11 @@ static inline int ldub_p(const void *ptr) return *(uint8_t *)ptr; } +static inline int ldb_p(const void *ptr) +{ + return ldub_p(ptr); +} + static inline int ldsb_p(const void *ptr) { return *(int8_t *)ptr; diff --git a/tcg/tcg.h b/tcg/tcg.h index a38659ea5b..302533b463 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -1317,6 +1317,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, # define helper_ret_stl_mmu helper_be_stl_mmu # define helper_ret_stq_mmu helper_be_stq_mmu # define helper_ret_ldw_cmmu helper_be_ldw_cmmu +# define helper_ret_lduw_cmmu helper_be_ldw_cmmu # define helper_ret_ldl_cmmu helper_be_ldl_cmmu # define helper_ret_ldq_cmmu helper_be_ldq_cmmu #else @@ -1330,6 +1331,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr, # define helper_ret_stl_mmu helper_le_stl_mmu # define helper_ret_stq_mmu helper_le_stq_mmu # define helper_ret_ldw_cmmu helper_le_ldw_cmmu +# define helper_ret_lduw_cmmu helper_le_ldw_cmmu # define helper_ret_ldl_cmmu helper_le_ldl_cmmu # define helper_ret_ldq_cmmu helper_le_ldq_cmmu #endif