Re: [Qemu-devel] [PATCH v3 25/50] translator: add translator_ld{ub, sw, uw, l, q}

["Alex Bennée" <alex.bennee@linaro.org>]

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