diff mbox series

[v5,23/55] translator: add translator_ld{ub,sw,uw,l,q}

Message ID 20191014104948.4291-24-alex.bennee@linaro.org (mailing list archive)
State New, archived
Headers show
Series Support for TCG plugins | expand

Commit Message

Alex Bennée Oct. 14, 2019, 10:49 a.m. UTC
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.
---
 include/exec/cpu_ldst.h   | 11 ++++++++
 include/exec/translator.h | 58 ++++++++++++++++++++++++++++++++++++++-
 include/qemu/bswap.h      |  5 ++++
 tcg/tcg.h                 |  2 ++
 4 files changed, 75 insertions(+), 1 deletion(-)

Comments

Richard Henderson Oct. 14, 2019, 4:08 p.m. UTC | #1
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~
Peter Maydell Oct. 14, 2019, 5:31 p.m. UTC | #2
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
Alex Bennée Oct. 15, 2019, 6:55 p.m. UTC | #3
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
Alex Bennée Oct. 15, 2019, 7:03 p.m. UTC | #4
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 Oct. 15, 2019, 9:34 p.m. UTC | #5
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 mbox series

Patch

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