| Message ID | 20190614171200.21078-26-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg plugin support | expand |
On 6/14/19 10:11 AM, Alex Bennée wrote: > +#define GEN_TRANSLATOR_LD(fullname, name, type, swap_fn) \ > + static inline type \ > + fullname ## _swap(CPUArchState *env, abi_ptr pc, bool do_swap) \ > + { \ > + type ret = cpu_ ## name ## _code(env, pc); \ > + \ > + if (do_swap) { \ > + ret = swap_fn(ret); \ > + } \ This feels like we should have done this at a different level. We already have lower-level functions that read from memory with the proper endianness. It seems that we don't have them for *_code, but that could be fixed. Or, indeed, bypassed, since these could be the new official interface, deprecating the *_code functions. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 6/14/19 10:11 AM, Alex Bennée wrote: >> +#define GEN_TRANSLATOR_LD(fullname, name, type, swap_fn) \ >> + static inline type \ >> + fullname ## _swap(CPUArchState *env, abi_ptr pc, bool do_swap) \ >> + { \ >> + type ret = cpu_ ## name ## _code(env, pc); \ >> + \ >> + if (do_swap) { \ >> + ret = swap_fn(ret); \ >> + } \ > > This feels like we should have done this at a different level. We already have > lower-level functions that read from memory with the proper > endianness. Yeah - this really only caters to the translator for guests which can switch their mode. > It seems that we don't have them for *_code, but that could be fixed. Or, > indeed, bypassed, since these could be the new official interface, deprecating > the *_code functions. Hmm how to properly audit that _code isn't being used elsewhere. You get lost in a maze of macros pretty quickly :-/ So you are proposing dropping the _code helpers from cpu_ldst_*_template and directly binding to the low level load/softmmu function from translator.h? Do we ever need _code access that isn't part of the translator loading instructions? > > > r~ -- Alex Bennée
On 7/30/19 5:41 AM, Alex Bennée wrote: > Do we ever need _code access that isn't part of the > translator loading instructions? We use it; I'm not sure that's the same as need. ;-) Lots of the uses that I examined should use a mechanism like arm does for recording syndrome data in the unwind slots. Quite possibly the only legitimate use is sparc, where it has an alternate address space that reads with execute permission. We could probably find a different way to accomplish that if we removed the *_code helpers. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 7/30/19 5:41 AM, Alex Bennée wrote: >> Do we ever need _code access that isn't part of the >> translator loading instructions? > > We use it; I'm not sure that's the same as need. ;-) Yeah I've run into others (e.g. alpha alpha_cpu_do_unaligned_access). So the question is do I attempt to deprecate code load in this series? > Lots of the uses that I examined should use a mechanism > like arm does for recording syndrome data in the unwind slots. Yeah - hence the semihosting fixups. ATM we only touch translator_loop guests but deprecating means having to do them all. Maybe we could poison the build someway and ensure as each legacy translation is converted to translator_loop we convert the _code use functions there. > Quite possibly the only legitimate use is sparc, where it > has an alternate address space that reads with execute permission. > We could probably find a different way to accomplish that > if we removed the *_code helpers. > > > r~ -- Alex Bennée
Richard Henderson <richard.henderson@linaro.org> writes: > On 7/30/19 5:41 AM, Alex Bennée wrote: >> Do we ever need _code access that isn't part of the >> translator loading instructions? > > We use it; I'm not sure that's the same as need. ;-) > > Lots of the uses that I examined should use a mechanism > like arm does for recording syndrome data in the unwind slots. > > Quite possibly the only legitimate use is sparc, where it > has an alternate address space that reads with execute permission. > We could probably find a different way to accomplish that > if we removed the *_code helpers. So far I've gone for deprecation, my current fixup patch looks like this: --8<---------------cut here---------------start------------->8--- fixup! translator: add translator_ld{ub,sw,uw,l,q} 4 files changed, 53 insertions(+), 7 deletions(-) include/exec/cpu_ldst.h | 11 +++++++++++ include/exec/translator.h | 42 +++++++++++++++++++++++++++++++++++------- include/qemu/bswap.h | 5 +++++ tcg/tcg.h | 2 ++ modified 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 modified include/exec/translator.h @@ -145,11 +145,39 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, void translator_loop_temp_check(DisasContextBase *db); -#define GEN_TRANSLATOR_LD(fullname, name, type, swap_fn) \ +/* + * 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 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 = cpu_ ## name ## _code(env, pc); \ + type ret; \ + DO_LOAD(type, name, shift) \ \ if (do_swap) { \ ret = swap_fn(ret); \ @@ -163,11 +191,11 @@ void translator_loop_temp_check(DisasContextBase *db); return fullname ## _swap(env, pc, false); \ } -GEN_TRANSLATOR_LD(translator_ldub, ldub, uint8_t, /* no swap needed */) -GEN_TRANSLATOR_LD(translator_ldsw, ldsw, int16_t, bswap16) -GEN_TRANSLATOR_LD(translator_lduw, lduw, uint16_t, bswap16) -GEN_TRANSLATOR_LD(translator_ldl, ldl, uint32_t, bswap32) -GEN_TRANSLATOR_LD(translator_ldq, ldq, uint64_t, bswap64) +GEN_TRANSLATOR_LD(translator_ldub, ldb, uint8_t, 1, /* no swap needed */) +GEN_TRANSLATOR_LD(translator_ldsw, lduw, int16_t, 2, bswap16) +GEN_TRANSLATOR_LD(translator_lduw, lduw, uint16_t, 2, bswap16) +GEN_TRANSLATOR_LD(translator_ldl, ldl, uint32_t, 3, bswap32) +GEN_TRANSLATOR_LD(translator_ldq, ldq, uint64_t, 4, bswap64) #undef GEN_TRANSLATOR_LD #endif /* EXEC__TRANSLATOR_H */ modified 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; modified tcg/tcg.h @@ -1404,6 +1404,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 @@ -1417,6 +1418,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 --8<---------------cut here---------------end--------------->8--- -- Alex Bennée
diff --git a/include/exec/translator.h b/include/exec/translator.h index 180c51d509..33fa709ba6 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,29 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, void translator_loop_temp_check(DisasContextBase *db); -#endif /* EXEC__TRANSLATOR_H */ +#define GEN_TRANSLATOR_LD(fullname, name, type, swap_fn) \ + static inline type \ + fullname ## _swap(CPUArchState *env, abi_ptr pc, bool do_swap) \ + { \ + type ret = cpu_ ## name ## _code(env, pc); \ + \ + 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, ldub, uint8_t, /* no swap needed */) +GEN_TRANSLATOR_LD(translator_ldsw, ldsw, int16_t, bswap16) +GEN_TRANSLATOR_LD(translator_lduw, lduw, uint16_t, bswap16) +GEN_TRANSLATOR_LD(translator_ldl, ldl, uint32_t, bswap32) +GEN_TRANSLATOR_LD(translator_ldq, ldq, uint64_t, bswap64) +#undef GEN_TRANSLATOR_LD + +#endif /* EXEC__TRANSLATOR_H */