1. Patchew
  2. QEMU
  3. [PATCH 0/2] Make ioport.c target-independent
  4. View patch

[PATCH 1/2] include/exec: Make ld*_p and st*_p functions available for generic code, too

Thomas Huth posted 2 patches 2 years, 8 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
[PATCH 1/2] include/exec: Make ld*_p and st*_p functions available for generic code, too
Posted by Thomas Huth 2 years, 8 months ago 
This will allow to move more code into the target independent source set.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/exec/cpu-all.h | 25 ----------------
 include/exec/tswap.h   | 66 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 25 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index ad824fee52..0daa4c06e5 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -55,31 +55,6 @@
 #define bswaptls(s) bswap64s(s)
 #endif
 
-/* Target-endianness CPU memory access functions. These fit into the
- * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
- */
-#if TARGET_BIG_ENDIAN
-#define lduw_p(p) lduw_be_p(p)
-#define ldsw_p(p) ldsw_be_p(p)
-#define ldl_p(p) ldl_be_p(p)
-#define ldq_p(p) ldq_be_p(p)
-#define stw_p(p, v) stw_be_p(p, v)
-#define stl_p(p, v) stl_be_p(p, v)
-#define stq_p(p, v) stq_be_p(p, v)
-#define ldn_p(p, sz) ldn_be_p(p, sz)
-#define stn_p(p, sz, v) stn_be_p(p, sz, v)
-#else
-#define lduw_p(p) lduw_le_p(p)
-#define ldsw_p(p) ldsw_le_p(p)
-#define ldl_p(p) ldl_le_p(p)
-#define ldq_p(p) ldq_le_p(p)
-#define stw_p(p, v) stw_le_p(p, v)
-#define stl_p(p, v) stl_le_p(p, v)
-#define stq_p(p, v) stq_le_p(p, v)
-#define ldn_p(p, sz) ldn_le_p(p, sz)
-#define stn_p(p, sz, v) stn_le_p(p, sz, v)
-#endif
-
 /* MMU memory access macros */
 
 #if defined(CONFIG_USER_ONLY)
diff --git a/include/exec/tswap.h b/include/exec/tswap.h
index 68944a880b..2774820bbe 100644
--- a/include/exec/tswap.h
+++ b/include/exec/tswap.h
@@ -69,4 +69,70 @@ static inline void tswap64s(uint64_t *s)
     }
 }
 
+/*
+ * Target-endianness CPU memory access functions. These fit into the
+ * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
+ */
+
+static inline int lduw_p(const void *ptr)
+{
+    return (uint16_t)tswap16(lduw_he_p(ptr));
+}
+
+static inline int ldsw_p(const void *ptr)
+{
+    return (int16_t)tswap16(lduw_he_p(ptr));
+}
+
+static inline int ldl_p(const void *ptr)
+{
+    return tswap32(ldl_he_p(ptr));
+}
+
+static inline uint64_t ldq_p(const void *ptr)
+{
+    return tswap64(ldq_he_p(ptr));
+}
+
+static inline uint64_t ldn_p(const void *ptr, int sz)
+{
+    if (target_needs_bswap()) {
+#if HOST_BIG_ENDIAN
+        return ldn_le_p(ptr, sz);
+#else
+        return ldn_be_p(ptr, sz);
+#endif
+    } else {
+        return ldn_he_p(ptr, sz);
+    }
+}
+
+static inline void stw_p(void *ptr, uint16_t v)
+{
+    stw_he_p(ptr, tswap16(v));
+}
+
+static inline void stl_p(void *ptr, uint32_t v)
+{
+    stl_he_p(ptr, tswap32(v));
+}
+
+static inline void stq_p(void *ptr, uint64_t v)
+{
+    stq_he_p(ptr, tswap64(v));
+}
+
+static inline void stn_p(void *ptr, int sz, uint64_t v)
+{
+    if (target_needs_bswap()) {
+#if HOST_BIG_ENDIAN
+        stn_le_p(ptr, sz, v);
+#else
+        stn_be_p(ptr, sz, v);
+#endif
+    } else {
+        stn_he_p(ptr, sz, v);
+    }
+}
+
 #endif  /* TSWAP_H */
-- 
2.31.1
Re: [PATCH 1/2] include/exec: Make ld*_p and st*_p functions available for generic code, too
Posted by Richard Henderson 2 years, 8 months ago 
On 5/17/23 00:42, Thomas Huth wrote:
> +static inline int ldl_p(const void *ptr)
> +{
> +    return tswap32(ldl_he_p(ptr));

Not an ideal formulation for some hosts, e.g. Power < 3.0, because there is no separate 
bswap instruction.  Power 2.07 only has bswapping-load/store.  Keeping the bswap adjacent 
to the memory operation helps the compiler.


> +static inline uint64_t ldn_p(const void *ptr, int sz)
> +{
> +    if (target_needs_bswap()) {
> +#if HOST_BIG_ENDIAN
> +        return ldn_le_p(ptr, sz);
> +#else
> +        return ldn_be_p(ptr, sz);
> +#endif
> +    } else {
> +        return ldn_he_p(ptr, sz);
> +    }

Better to avoid #if for if.  And even better to merge to one test:

     if (HOST_BIG_ENDIAN ^ target_needs_bswap()) {
         return ldn_le_p(ptr, sz);
     } else {
         return ldn_be_p(ptr, sz);
     }


r~
Re: [PATCH 1/2] include/exec: Make ld*_p and st*_p functions available for generic code, too
Posted by Philippe Mathieu-Daudé 2 years, 8 months ago 
Hi Thomas,

On 17/5/23 09:42, Thomas Huth wrote:
> This will allow to move more code into the target independent source set.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   include/exec/cpu-all.h | 25 ----------------
>   include/exec/tswap.h   | 66 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 66 insertions(+), 25 deletions(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index ad824fee52..0daa4c06e5 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -55,31 +55,6 @@
>   #define bswaptls(s) bswap64s(s)
>   #endif
>   
> -/* Target-endianness CPU memory access functions. These fit into the
> - * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
> - */
> -#if TARGET_BIG_ENDIAN
> -#define lduw_p(p) lduw_be_p(p)
> -#define ldsw_p(p) ldsw_be_p(p)
> -#define ldl_p(p) ldl_be_p(p)
> -#define ldq_p(p) ldq_be_p(p)
> -#define stw_p(p, v) stw_be_p(p, v)
> -#define stl_p(p, v) stl_be_p(p, v)
> -#define stq_p(p, v) stq_be_p(p, v)
> -#define ldn_p(p, sz) ldn_be_p(p, sz)
> -#define stn_p(p, sz, v) stn_be_p(p, sz, v)
> -#else
> -#define lduw_p(p) lduw_le_p(p)
> -#define ldsw_p(p) ldsw_le_p(p)
> -#define ldl_p(p) ldl_le_p(p)
> -#define ldq_p(p) ldq_le_p(p)
> -#define stw_p(p, v) stw_le_p(p, v)
> -#define stl_p(p, v) stl_le_p(p, v)
> -#define stq_p(p, v) stq_le_p(p, v)
> -#define ldn_p(p, sz) ldn_le_p(p, sz)
> -#define stn_p(p, sz, v) stn_le_p(p, sz, v)
> -#endif
> -
>   /* MMU memory access macros */
>   
>   #if defined(CONFIG_USER_ONLY)
> diff --git a/include/exec/tswap.h b/include/exec/tswap.h
> index 68944a880b..2774820bbe 100644
> --- a/include/exec/tswap.h
> +++ b/include/exec/tswap.h
> @@ -69,4 +69,70 @@ static inline void tswap64s(uint64_t *s)
>       }
>   }
>   
> +/*
> + * Target-endianness CPU memory access functions. These fit into the
> + * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
> + */
> +
> +static inline int lduw_p(const void *ptr)
> +{
> +    return (uint16_t)tswap16(lduw_he_p(ptr));
> +}
> +
> +static inline int ldsw_p(const void *ptr)
> +{
> +    return (int16_t)tswap16(lduw_he_p(ptr));
> +}
> +
> +static inline int ldl_p(const void *ptr)
> +{
> +    return tswap32(ldl_he_p(ptr));
> +}
> +
> +static inline uint64_t ldq_p(const void *ptr)
> +{
> +    return tswap64(ldq_he_p(ptr));
> +}

Hmm I am a bit confused, I was working on removing the tswapXX API
from softmmu [*] (restricting it locally to gdbstub). Now I realize
commit 24be3369ad ("include/exec: Provide the tswap() functions for 
target independent code, too") exposes it furthermore. I thought the
ld/st API was clearer and enough for all our uses, but maybe I am
wrong and we need this API.

[*] 
https://lore.kernel.org/qemu-devel/20221213125218.39868-1-philmd@linaro.org/
Re: [PATCH 1/2] include/exec: Make ld*_p and st*_p functions available for generic code, too
Posted by Thomas Huth 2 years, 8 months ago 
On 17/05/2023 12.18, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 17/5/23 09:42, Thomas Huth wrote:
>> This will allow to move more code into the target independent source set.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/exec/cpu-all.h | 25 ----------------
>>   include/exec/tswap.h   | 66 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 66 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index ad824fee52..0daa4c06e5 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -55,31 +55,6 @@
>>   #define bswaptls(s) bswap64s(s)
>>   #endif
>> -/* Target-endianness CPU memory access functions. These fit into the
>> - * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
>> - */
>> -#if TARGET_BIG_ENDIAN
>> -#define lduw_p(p) lduw_be_p(p)
>> -#define ldsw_p(p) ldsw_be_p(p)
>> -#define ldl_p(p) ldl_be_p(p)
>> -#define ldq_p(p) ldq_be_p(p)
>> -#define stw_p(p, v) stw_be_p(p, v)
>> -#define stl_p(p, v) stl_be_p(p, v)
>> -#define stq_p(p, v) stq_be_p(p, v)
>> -#define ldn_p(p, sz) ldn_be_p(p, sz)
>> -#define stn_p(p, sz, v) stn_be_p(p, sz, v)
>> -#else
>> -#define lduw_p(p) lduw_le_p(p)
>> -#define ldsw_p(p) ldsw_le_p(p)
>> -#define ldl_p(p) ldl_le_p(p)
>> -#define ldq_p(p) ldq_le_p(p)
>> -#define stw_p(p, v) stw_le_p(p, v)
>> -#define stl_p(p, v) stl_le_p(p, v)
>> -#define stq_p(p, v) stq_le_p(p, v)
>> -#define ldn_p(p, sz) ldn_le_p(p, sz)
>> -#define stn_p(p, sz, v) stn_le_p(p, sz, v)
>> -#endif
>> -
>>   /* MMU memory access macros */
>>   #if defined(CONFIG_USER_ONLY)
>> diff --git a/include/exec/tswap.h b/include/exec/tswap.h
>> index 68944a880b..2774820bbe 100644
>> --- a/include/exec/tswap.h
>> +++ b/include/exec/tswap.h
>> @@ -69,4 +69,70 @@ static inline void tswap64s(uint64_t *s)
>>       }
>>   }
>> +/*
>> + * Target-endianness CPU memory access functions. These fit into the
>> + * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
>> + */
>> +
>> +static inline int lduw_p(const void *ptr)
>> +{
>> +    return (uint16_t)tswap16(lduw_he_p(ptr));
>> +}
>> +
>> +static inline int ldsw_p(const void *ptr)
>> +{
>> +    return (int16_t)tswap16(lduw_he_p(ptr));
>> +}
>> +
>> +static inline int ldl_p(const void *ptr)
>> +{
>> +    return tswap32(ldl_he_p(ptr));
>> +}
>> +
>> +static inline uint64_t ldq_p(const void *ptr)
>> +{
>> +    return tswap64(ldq_he_p(ptr));
>> +}
> 
> Hmm I am a bit confused, I was working on removing the tswapXX API
> from softmmu [*] (restricting it locally to gdbstub). Now I realize
> commit 24be3369ad ("include/exec: Provide the tswap() functions for target 
> independent code, too") exposes it furthermore. I thought the
> ld/st API was clearer and enough for all our uses, but maybe I am
> wrong and we need this API.

I think we need both. In some cases, you want to read/write values in a 
certain endianess, and in some cases you have some generic code that just 
wants to write some values in the endianess of the target (where the generic 
code does not know the endianess of the target by default, e.g. since it is 
one time linked with a big endian target and one time with a little endian 
target). In the latter case, we need these functions here from tswap.h.

  Thomas

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.