[PATCH 0/2] Make ioport.c target-independent

[PATCH 0/2] Make ioport.c target-independent

[Thomas Huth]

Assuming that the code in ioport.c is not too performance critical,
we can move this to the target-independent source set after
reworking the ld*_p and st*_p helper functions a little bit.

This way, ioport.c has only to be compiled once and not multiple
times anymore (one time for each target), so this should help
to speed up the compilation process a little bit, and is a good
preparation for the single emulator binary project.

Thomas Huth (2):
  include/exec: Make ld*_p and st*_p functions available for generic
    code, too
  softmmu: Move ioport.c into the target-independent source set

 include/exec/cpu-all.h | 25 ----------------
 include/exec/tswap.h   | 66 ++++++++++++++++++++++++++++++++++++++++++
 softmmu/ioport.c       |  2 +-
 softmmu/meson.build    |  2 +-
 4 files changed, 68 insertions(+), 27 deletions(-)

-- 
2.31.1

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

[Thomas Huth]

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

[Philippe Mathieu-Daudé]

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

[Thomas Huth]

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

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

[Richard Henderson]

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~

[PATCH 2/2] softmmu: Move ioport.c into the target-independent source set

[Thomas Huth]

Now that the st*_p and ld*_p functions can be used from common code,
too, we can move ioport.c from specific_ss into softmmu_ss to avoid
that we have to compile it multiple times.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 softmmu/ioport.c    | 2 +-
 softmmu/meson.build | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index cb8adb0b93..8d9d1e5b40 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -26,10 +26,10 @@
  */
 
 #include "qemu/osdep.h"
-#include "cpu.h"
 #include "exec/ioport.h"
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
+#include "exec/tswap.h"
 #include "trace.h"
 
 typedef struct MemoryRegionPortioList {
diff --git a/softmmu/meson.build b/softmmu/meson.build
index 974732b0f3..e572af54ab 100644
--- a/softmmu/meson.build
+++ b/softmmu/meson.build
@@ -1,6 +1,5 @@
 specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: [files(
   'arch_init.c',
-  'ioport.c',
   'memory.c',
   'physmem.c',
   'watchpoint.c',
@@ -20,6 +19,7 @@ softmmu_ss.add(files(
   'dirtylimit.c',
   'dma-helpers.c',
   'globals.c',
+  'ioport.c',
   'memory_mapping.c',
   'qdev-monitor.c',
   'qtest.c',
-- 
2.31.1

Re: [PATCH 2/2] softmmu: Move ioport.c into the target-independent source set

[Richard Henderson]

On 5/17/23 00:42, Thomas Huth wrote:
> Now that the st*_p and ld*_p functions can be used from common code,
> too, we can move ioport.c from specific_ss into softmmu_ss to avoid
> that we have to compile it multiple times.
> 
> Signed-off-by: Thomas Huth<thuth@redhat.com>
> ---
>   softmmu/ioport.c    | 2 +-
>   softmmu/meson.build | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~