[PATCH v2 0/9] misc: Reduce 'exec/tswap.h' inclusions

[PATCH v2 0/9] misc: Reduce 'exec/tswap.h' inclusions

[Philippe Mathieu-Daudé]

Include "exec/tswap.h" where it is needed and remove it from
"exec/cpu-all.h" which is a header included very often.

Philippe Mathieu-Daudé (9):
  target/xtensa: Remove tswap() calls in semihosting simcall() helper
  target/mips: Remove tswap() calls in semihosting uhi_fstat_cb()
  accel/tcg: Include missing 'exec/tswap.h' header in translator.c
  hw/arm: Include missing 'exec/tswap.h' header
  hw/ppc: Include missing 'exec/tswap.h' header
  hw/mips: Include missing 'exec/tswap.h' header
  hw/sh4/r2d: Include missing 'exec/tswap.h' header
  hw/xtensa: Include missing 'exec/tswap.h' header
  exec/cpu-all: Do not include 'exec/tswap.h' header

 hw/xtensa/bootparam.h              |  1 +
 include/exec/cpu-all.h             |  1 -
 accel/tcg/translator.c             |  1 +
 hw/arm/boot.c                      |  1 +
 hw/arm/npcm7xx.c                   |  1 +
 hw/mips/fuloong2e.c                |  1 +
 hw/mips/malta.c                    |  1 +
 hw/ppc/sam460ex.c                  |  1 +
 hw/ppc/spapr.c                     |  1 +
 hw/ppc/virtex_ml507.c              |  1 +
 hw/sh4/r2d.c                       |  1 +
 hw/xtensa/xtfpga.c                 |  1 +
 target/mips/tcg/sysemu/mips-semi.c | 43 +++++++++++++++++++++---------
 target/xtensa/xtensa-semi.c        | 15 ++++-------
 14 files changed, 46 insertions(+), 24 deletions(-)

-- 
2.45.2

[PATCH v2 1/9] target/xtensa: Remove tswap() calls in semihosting simcall() helper

[Philippe Mathieu-Daudé]

In preparation of heterogeneous emulation where cores with
different endianness can run concurrently, we need to remove
the tswap() calls -- which use a fixed per-binary endianness.

Get the endianness of the CPU accessed using the libisa
xtensa_isa_is_big_endian() call and replace the tswap() calls
by bswap() ones when necessary.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/xtensa/xtensa-semi.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
index fa21b7e11fc..2ded8e5634e 100644
--- a/target/xtensa/xtensa-semi.c
+++ b/target/xtensa/xtensa-semi.c
@@ -30,6 +30,7 @@
 #include "chardev/char-fe.h"
 #include "exec/helper-proto.h"
 #include "semihosting/semihost.h"
+#include "semihosting/uaccess.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
 
@@ -323,15 +324,12 @@ void HELPER(simcall)(CPUXtensaState *env)
             uint32_t fd = regs[3];
             uint32_t rq = regs[4];
             uint32_t target_tv = regs[5];
-            uint32_t target_tvv[2];
 
             struct timeval tv = {0};
 
             if (target_tv) {
-                cpu_memory_rw_debug(cs, target_tv,
-                        (uint8_t *)target_tvv, sizeof(target_tvv), 0);
-                tv.tv_sec = (int32_t)tswap32(target_tvv[0]);
-                tv.tv_usec = (int32_t)tswap32(target_tvv[1]);
+                get_user_u32(tv.tv_sec, target_tv);
+                get_user_u32(tv.tv_sec, target_tv + 4);
             }
             if (fd < 3 && sim_console) {
                 if ((fd == 1 || fd == 2) && rq == SELECT_ONE_WRITE) {
@@ -387,11 +385,8 @@ void HELPER(simcall)(CPUXtensaState *env)
                 const char *str = semihosting_get_arg(i);
                 int str_size = strlen(str) + 1;
 
-                argptr = tswap32(regs[3] + str_offset);
-
-                cpu_memory_rw_debug(cs,
-                                    regs[3] + i * sizeof(uint32_t),
-                                    (uint8_t *)&argptr, sizeof(argptr), 1);
+                put_user_u32(regs[3] + str_offset,
+                             regs[3] + i * sizeof(uint32_t));
                 cpu_memory_rw_debug(cs,
                                     regs[3] + str_offset,
                                     (uint8_t *)str, str_size, 1);
-- 
2.45.2

[PATCH v2 2/9] target/mips: Remove tswap() calls in semihosting uhi_fstat_cb()

[Philippe Mathieu-Daudé]

In preparation of heterogeneous emulation where cores with
different endianness can run concurrently, we need to remove
the tswap() calls -- which use a fixed per-binary endianness.

Get the endianness of the UHI CPU accessed using
mips_env_is_bigendian() and replace the tswap() calls
by bswap() ones when necessary.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/mips/tcg/sysemu/mips-semi.c | 43 +++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/target/mips/tcg/sysemu/mips-semi.c b/target/mips/tcg/sysemu/mips-semi.c
index 5ba06e95734..df0c3256d9e 100644
--- a/target/mips/tcg/sysemu/mips-semi.c
+++ b/target/mips/tcg/sysemu/mips-semi.c
@@ -168,6 +168,7 @@ static void uhi_fstat_cb(CPUState *cs, uint64_t ret, int err)
 
     if (!err) {
         CPUMIPSState *env = cpu_env(cs);
+        bool swap_needed = HOST_BIG_ENDIAN != mips_env_is_bigendian(env);
         target_ulong addr = env->active_tc.gpr[5];
         UHIStat *dst = lock_user(VERIFY_WRITE, addr, sizeof(UHIStat), 1);
         struct gdb_stat s;
@@ -179,19 +180,35 @@ static void uhi_fstat_cb(CPUState *cs, uint64_t ret, int err)
         memcpy(&s, dst, sizeof(struct gdb_stat));
         memset(dst, 0, sizeof(UHIStat));
 
-        dst->uhi_st_dev = tswap16(be32_to_cpu(s.gdb_st_dev));
-        dst->uhi_st_ino = tswap16(be32_to_cpu(s.gdb_st_ino));
-        dst->uhi_st_mode = tswap32(be32_to_cpu(s.gdb_st_mode));
-        dst->uhi_st_nlink = tswap16(be32_to_cpu(s.gdb_st_nlink));
-        dst->uhi_st_uid = tswap16(be32_to_cpu(s.gdb_st_uid));
-        dst->uhi_st_gid = tswap16(be32_to_cpu(s.gdb_st_gid));
-        dst->uhi_st_rdev = tswap16(be32_to_cpu(s.gdb_st_rdev));
-        dst->uhi_st_size = tswap64(be64_to_cpu(s.gdb_st_size));
-        dst->uhi_st_atime = tswap64(be32_to_cpu(s.gdb_st_atime));
-        dst->uhi_st_mtime = tswap64(be32_to_cpu(s.gdb_st_mtime));
-        dst->uhi_st_ctime = tswap64(be32_to_cpu(s.gdb_st_ctime));
-        dst->uhi_st_blksize = tswap64(be64_to_cpu(s.gdb_st_blksize));
-        dst->uhi_st_blocks = tswap64(be64_to_cpu(s.gdb_st_blocks));
+        dst->uhi_st_dev = be32_to_cpu(s.gdb_st_dev);
+        dst->uhi_st_ino = be32_to_cpu(s.gdb_st_ino);
+        dst->uhi_st_mode = be32_to_cpu(s.gdb_st_mode);
+        dst->uhi_st_nlink = be32_to_cpu(s.gdb_st_nlink);
+        dst->uhi_st_uid = be32_to_cpu(s.gdb_st_uid);
+        dst->uhi_st_gid = be32_to_cpu(s.gdb_st_gid);
+        dst->uhi_st_rdev = be32_to_cpu(s.gdb_st_rdev);
+        dst->uhi_st_size = be64_to_cpu(s.gdb_st_size);
+        dst->uhi_st_atime = be32_to_cpu(s.gdb_st_atime);
+        dst->uhi_st_mtime = be32_to_cpu(s.gdb_st_mtime);
+        dst->uhi_st_ctime = be32_to_cpu(s.gdb_st_ctime);
+        dst->uhi_st_blksize = be64_to_cpu(s.gdb_st_blksize);
+        dst->uhi_st_blocks = be64_to_cpu(s.gdb_st_blocks);
+
+        if (swap_needed) {
+            dst->uhi_st_dev = bswap16(dst->uhi_st_dev);
+            dst->uhi_st_ino = bswap16(dst->uhi_st_ino);
+            dst->uhi_st_mode = bswap32(dst->uhi_st_mode);
+            dst->uhi_st_nlink = bswap16(dst->uhi_st_nlink);
+            dst->uhi_st_uid = bswap16(dst->uhi_st_uid);
+            dst->uhi_st_gid = bswap16(dst->uhi_st_gid);
+            dst->uhi_st_rdev = bswap16(dst->uhi_st_rdev);
+            dst->uhi_st_size = bswap64(dst->uhi_st_size);
+            dst->uhi_st_atime = bswap64(dst->uhi_st_atime);
+            dst->uhi_st_mtime = bswap64(dst->uhi_st_mtime);
+            dst->uhi_st_ctime = bswap64(dst->uhi_st_ctime);
+            dst->uhi_st_blksize = bswap64(dst->uhi_st_blksize);
+            dst->uhi_st_blocks = bswap64(dst->uhi_st_blocks);
+        }
 
         unlock_user(dst, addr, sizeof(UHIStat));
     }
-- 
2.45.2

[PATCH v2 3/9] accel/tcg: Include missing 'exec/tswap.h' header in translator.c

[Philippe Mathieu-Daudé]

translator.c indirectly gets "exec/tswap.h" declarations via
"exec/cpu-all.h". Include it directly to be able to remove the
former from the latter, otherwise we get:

  accel/tcg/translator.c:433:15: error: call to undeclared function 'tswap16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    433 |         tgt = tswap16(raw);
        |               ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/translator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index cbad00a5172..ff5dabc9014 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -15,6 +15,7 @@
 #include "exec/cpu_ldst.h"
 #include "exec/plugin-gen.h"
 #include "exec/cpu_ldst.h"
+#include "exec/tswap.h"
 #include "tcg/tcg-op-common.h"
 #include "internal-target.h"
 #include "disas/disas.h"
-- 
2.45.2

[PATCH v2 4/9] hw/arm: Include missing 'exec/tswap.h' header

[Philippe Mathieu-Daudé]

Some files indirectly get "exec/tswap.h" declarations via
"exec/cpu-all.h". Include it directly to be able to remove
the former from the latter, otherwise we get:

  hw/arm/boot.c:175:19: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    175 |         code[i] = tswap32(insn);
        |                   ^
  hw/arm/npcm7xx.c:326:26: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    326 |         board_setup[i] = tswap32(board_setup[i]);
        |                          ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/boot.c    | 1 +
 hw/arm/npcm7xx.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 5301d8d318c..30ba98d52fb 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -26,6 +26,7 @@
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/units.h"
+#include "exec/tswap.h"
 
 /* Kernel boot protocol is specified in the kernel docs
  * Documentation/arm/Booting and Documentation/arm64/booting.txt
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index af04c4b7ec4..3a5ef26f689 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -28,6 +28,7 @@
 #include "qemu/units.h"
 #include "sysemu/sysemu.h"
 #include "target/arm/cpu-qom.h"
+#include "exec/tswap.h"
 
 /*
  * This covers the whole MMIO space. We'll use this to catch any MMIO accesses
-- 
2.45.2

[PATCH v2 5/9] hw/ppc: Include missing 'exec/tswap.h' header

[Philippe Mathieu-Daudé]

Some files indirectly get "exec/tswap.h" declarations via
"exec/cpu-all.h". Include it directly to be able to remove
the former from the latter, otherwise we get:

  hw/ppc/virtex_ml507.c:123:19: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    123 |     env->gpr[6] = tswap32(EPAPR_MAGIC);
        |                   ^
  hw/ppc/sam460ex.c:238:23: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    238 |         env->gpr[6] = tswap32(EPAPR_MAGIC);
        |                       ^
  hw/ppc/spapr.c:1617:13: error: call to undeclared function 'tswap64'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   1617 |             DIRTY_HPTE(HPTE(spapr->htab, i));
        |             ^
  hw/ppc/spapr.c:1406:55: note: expanded from macro 'DIRTY_HPTE'
   1406 | #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
        |                                                       ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/sam460ex.c     | 1 +
 hw/ppc/spapr.c        | 1 +
 hw/ppc/virtex_ml507.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 78e2a46e753..3bbab263ae8 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -22,6 +22,7 @@
 #include "sysemu/device_tree.h"
 #include "sysemu/block-backend.h"
 #include "exec/page-protection.h"
+#include "exec/tswap.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "exec/memory.h"
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0d4efaa0c09..02136b3295a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -90,6 +90,7 @@
 #include "hw/ppc/spapr_numa.h"
 
 #include <libfdt.h>
+#include "exec/tswap.h"
 
 /* SLOF memory layout:
  *
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index f378e5c4a90..0e9a2469599 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -26,6 +26,7 @@
 #include "qemu/datadir.h"
 #include "qemu/units.h"
 #include "exec/page-protection.h"
+#include "exec/tswap.h"
 #include "cpu.h"
 #include "hw/sysbus.h"
 #include "hw/char/serial-mm.h"
-- 
2.45.2

[PATCH v2 6/9] hw/mips: Include missing 'exec/tswap.h' header

[Philippe Mathieu-Daudé]

Some files indirectly get "exec/tswap.h" declarations via
"exec/cpu-all.h". Include it directly to be able to remove
the former from the latter, otherwise we get:

  hw/mips/malta.c:674:22: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    674 |                      tswap32((1 << 31) /* ConfigEn */
        |                      ^
  hw/mips/fuloong2e.c:89:23: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     89 |     prom_buf[index] = tswap32(ENVP_VADDR + table_addr);
        |                       ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/fuloong2e.c | 1 +
 hw/mips/malta.c     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 7fd8296ccb6..904c10b90e3 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -40,6 +40,7 @@
 #include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
 #include "qemu/error-report.h"
+#include "exec/tswap.h"
 
 #define ENVP_PADDR              0x2000
 #define ENVP_VADDR              cpu_mips_phys_to_kseg0(NULL, ENVP_PADDR)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 198da5ba3d4..834636dae59 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -28,6 +28,7 @@
 #include "qemu/datadir.h"
 #include "qemu/cutils.h"
 #include "qemu/guest-random.h"
+#include "exec/tswap.h"
 #include "hw/clock.h"
 #include "hw/southbridge/piix.h"
 #include "hw/isa/superio.h"
-- 
2.45.2

[PATCH v2 7/9] hw/sh4/r2d: Include missing 'exec/tswap.h' header

[Philippe Mathieu-Daudé]

r2d.c indirectly get "exec/tswap.h" declarations via
"exec/cpu-all.h". Include it directly to be able to
remove the former from the latter, otherwise we get:

  hw/sh4/r2d.c:357:35: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    357 |         boot_params.loader_type = tswap32(1);
        |                                   ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sh4/r2d.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 7eecd79fcc1..e6cc156c238 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -43,6 +43,7 @@
 #include "hw/loader.h"
 #include "hw/usb.h"
 #include "hw/block/flash.h"
+#include "exec/tswap.h"
 
 #define FLASH_BASE 0x00000000
 #define FLASH_SIZE (16 * MiB)
-- 
2.45.2

[PATCH v2 8/9] hw/xtensa: Include missing 'exec/tswap.h' header

[Philippe Mathieu-Daudé]

Some files indirectly get "exec/tswap.h" declarations via
"exec/cpu-all.h". Include it directly to be able to remove
the former from the latter, otherwise we get:

  hw/xtensa/bootparam.h:40:16: error: call to undeclared function 'tswap16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     40 |         .tag = tswap16(tag),
        |                ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/xtensa/bootparam.h | 1 +
 hw/xtensa/xtfpga.c    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/xtensa/bootparam.h b/hw/xtensa/bootparam.h
index f57ff850bcb..4418c78d5bb 100644
--- a/hw/xtensa/bootparam.h
+++ b/hw/xtensa/bootparam.h
@@ -2,6 +2,7 @@
 #define HW_XTENSA_BOOTPARAM_H
 
 #include "exec/cpu-common.h"
+#include "exec/tswap.h"
 
 #define BP_TAG_COMMAND_LINE     0x1001  /* command line (0-terminated string)*/
 #define BP_TAG_INITRD           0x1002  /* ramdisk addr and size (bp_meminfo) */
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 398e6256e1d..2e264c61988 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -35,6 +35,7 @@
 #include "hw/qdev-properties.h"
 #include "elf.h"
 #include "exec/memory.h"
+#include "exec/tswap.h"
 #include "hw/char/serial-mm.h"
 #include "net/net.h"
 #include "hw/sysbus.h"
-- 
2.45.2

[PATCH v2 9/9] exec/cpu-all: Do not include 'exec/tswap.h' header

[Philippe Mathieu-Daudé]

Nothing in "exec/cpu-all.h" requires "exec/tswap.h" declarations.
Remove it to reduce headers preprocessing and poisonned target
definitions polution.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/cpu-all.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 45e66769380..b45defdbf43 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -22,7 +22,6 @@
 #include "exec/page-protection.h"
 #include "exec/cpu-common.h"
 #include "exec/memory.h"
-#include "exec/tswap.h"
 #include "hw/core/cpu.h"
 
 /* some important defines:
-- 
2.45.2

Re: [PATCH v2 1/9] target/xtensa: Remove tswap() calls in semihosting simcall() helper

[Philippe Mathieu-Daudé]

On 12/12/24 00:03, Philippe Mathieu-Daudé wrote:
> In preparation of heterogeneous emulation where cores with
> different endianness can run concurrently, we need to remove
> the tswap() calls -- which use a fixed per-binary endianness.
> 
> Get the endianness of the CPU accessed using the libisa
> xtensa_isa_is_big_endian() call and replace the tswap() calls
> by bswap() ones when necessary.

Instead read here:

   In preparation of heterogeneous emulation where cores with
   different endianness can run concurrently, replace the pair
   of cpu_memory_rw_debug() + tswap() calls by put/get_user_u32()
   ones, which still do the same under the hood, but simplify the
   code maintenance (having less sites to do endianness code
   conversion).

> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/xtensa/xtensa-semi.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
> index fa21b7e11fc..2ded8e5634e 100644
> --- a/target/xtensa/xtensa-semi.c
> +++ b/target/xtensa/xtensa-semi.c
> @@ -30,6 +30,7 @@
>   #include "chardev/char-fe.h"
>   #include "exec/helper-proto.h"
>   #include "semihosting/semihost.h"
> +#include "semihosting/uaccess.h"
>   #include "qapi/error.h"
>   #include "qemu/log.h"
>   
> @@ -323,15 +324,12 @@ void HELPER(simcall)(CPUXtensaState *env)
>               uint32_t fd = regs[3];
>               uint32_t rq = regs[4];
>               uint32_t target_tv = regs[5];
> -            uint32_t target_tvv[2];
>   
>               struct timeval tv = {0};
>   
>               if (target_tv) {
> -                cpu_memory_rw_debug(cs, target_tv,
> -                        (uint8_t *)target_tvv, sizeof(target_tvv), 0);
> -                tv.tv_sec = (int32_t)tswap32(target_tvv[0]);
> -                tv.tv_usec = (int32_t)tswap32(target_tvv[1]);
> +                get_user_u32(tv.tv_sec, target_tv);
> +                get_user_u32(tv.tv_sec, target_tv + 4);
>               }
>               if (fd < 3 && sim_console) {
>                   if ((fd == 1 || fd == 2) && rq == SELECT_ONE_WRITE) {
> @@ -387,11 +385,8 @@ void HELPER(simcall)(CPUXtensaState *env)
>                   const char *str = semihosting_get_arg(i);
>                   int str_size = strlen(str) + 1;
>   
> -                argptr = tswap32(regs[3] + str_offset);
> -
> -                cpu_memory_rw_debug(cs,
> -                                    regs[3] + i * sizeof(uint32_t),
> -                                    (uint8_t *)&argptr, sizeof(argptr), 1);
> +                put_user_u32(regs[3] + str_offset,
> +                             regs[3] + i * sizeof(uint32_t));
>                   cpu_memory_rw_debug(cs,
>                                       regs[3] + str_offset,
>                                       (uint8_t *)str, str_size, 1);

Re: [PATCH v2 1/9] target/xtensa: Remove tswap() calls in semihosting simcall() helper

[Richard Henderson]

On 12/11/24 17:08, Philippe Mathieu-Daudé wrote:
> On 12/12/24 00:03, Philippe Mathieu-Daudé wrote:
>> In preparation of heterogeneous emulation where cores with
>> different endianness can run concurrently, we need to remove
>> the tswap() calls -- which use a fixed per-binary endianness.
>>
>> Get the endianness of the CPU accessed using the libisa
>> xtensa_isa_is_big_endian() call and replace the tswap() calls
>> by bswap() ones when necessary.
> 
> Instead read here:
> 
>    In preparation of heterogeneous emulation where cores with
>    different endianness can run concurrently, replace the pair
>    of cpu_memory_rw_debug() + tswap() calls by put/get_user_u32()
>    ones, which still do the same under the hood, but simplify the
>    code maintenance (having less sites to do endianness code
>    conversion).
> 
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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


r~

Re: [PATCH v2 3/9] accel/tcg: Include missing 'exec/tswap.h' header in translator.c

[Richard Henderson]

On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
> translator.c indirectly gets "exec/tswap.h" declarations via
> "exec/cpu-all.h". Include it directly to be able to remove the
> former from the latter, otherwise we get:
> 
>    accel/tcg/translator.c:433:15: error: call to undeclared function 'tswap16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      433 |         tgt = tswap16(raw);
>          |               ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/tcg/translator.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

Re: [PATCH v2 4/9] hw/arm: Include missing 'exec/tswap.h' header

[Richard Henderson]

On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
> Some files indirectly get "exec/tswap.h" declarations via
> "exec/cpu-all.h". Include it directly to be able to remove
> the former from the latter, otherwise we get:
> 
>    hw/arm/boot.c:175:19: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      175 |         code[i] = tswap32(insn);
>          |                   ^
>    hw/arm/npcm7xx.c:326:26: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      326 |         board_setup[i] = tswap32(board_setup[i]);
>          |                          ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/boot.c    | 1 +
>   hw/arm/npcm7xx.c | 1 +
>   2 files changed, 2 insertions(+)
> 

These could all be cpu_to_le32, since TARGET_BIG_ENDIAN is always false for qemu-system-arm.


r~

Re: [PATCH v2 5/9] hw/ppc: Include missing 'exec/tswap.h' header

[Richard Henderson]

On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
> Some files indirectly get "exec/tswap.h" declarations via
> "exec/cpu-all.h". Include it directly to be able to remove
> the former from the latter, otherwise we get:
> 
>    hw/ppc/virtex_ml507.c:123:19: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      123 |     env->gpr[6] = tswap32(EPAPR_MAGIC);
>          |                   ^
>    hw/ppc/sam460ex.c:238:23: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      238 |         env->gpr[6] = tswap32(EPAPR_MAGIC);
>          |                       ^
>    hw/ppc/spapr.c:1617:13: error: call to undeclared function 'tswap64'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>     1617 |             DIRTY_HPTE(HPTE(spapr->htab, i));
>          |             ^
>    hw/ppc/spapr.c:1406:55: note: expanded from macro 'DIRTY_HPTE'
>     1406 | #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>          |                                                       ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I suspect that, like Arm, all of these could be big-endian.
AFAIK all ppc reset into BE; the OS has to flip the bit to get into LE mode.
But I am less sure of this than I was for Arm.

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


r~

Re: [PATCH v2 6/9] hw/mips: Include missing 'exec/tswap.h' header

[Richard Henderson]

On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
> Some files indirectly get "exec/tswap.h" declarations via
> "exec/cpu-all.h". Include it directly to be able to remove
> the former from the latter, otherwise we get:
> 
>    hw/mips/malta.c:674:22: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      674 |                      tswap32((1 << 31) /* ConfigEn */
>          |                      ^
>    hw/mips/fuloong2e.c:89:23: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>       89 |     prom_buf[index] = tswap32(ENVP_VADDR + table_addr);
>          |                       ^
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/mips/fuloong2e.c | 1 +
>   hw/mips/malta.c     | 1 +
>   2 files changed, 2 insertions(+)

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

r~

Re: [PATCH v2 7/9] hw/sh4/r2d: Include missing 'exec/tswap.h' header

[Richard Henderson]

On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
> r2d.c indirectly get "exec/tswap.h" declarations via
> "exec/cpu-all.h". Include it directly to be able to
> remove the former from the latter, otherwise we get:
> 
>    hw/sh4/r2d.c:357:35: error: call to undeclared function 'tswap32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>      357 |         boot_params.loader_type = tswap32(1);
>          |                                   ^
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/sh4/r2d.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

Re: [PATCH v2 8/9] hw/xtensa: Include missing 'exec/tswap.h' header

[Richard Henderson]

On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
> Some files indirectly get "exec/tswap.h" declarations via
> "exec/cpu-all.h". Include it directly to be able to remove
> the former from the latter, otherwise we get:
> 
>    hw/xtensa/bootparam.h:40:16: error: call to undeclared function 'tswap16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>       40 |         .tag = tswap16(tag),
>          |                ^
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/xtensa/bootparam.h | 1 +
>   hw/xtensa/xtfpga.c    | 1 +
>   2 files changed, 2 insertions(+)

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

r~

Re: [PATCH v2 9/9] exec/cpu-all: Do not include 'exec/tswap.h' header

[Richard Henderson]

On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
> Nothing in "exec/cpu-all.h" requires "exec/tswap.h" declarations.
> Remove it to reduce headers preprocessing and poisonned target
> definitions polution.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/cpu-all.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 45e66769380..b45defdbf43 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -22,7 +22,6 @@
>   #include "exec/page-protection.h"
>   #include "exec/cpu-common.h"
>   #include "exec/memory.h"
> -#include "exec/tswap.h"
>   #include "hw/core/cpu.h"
>   
>   /* some important defines:

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

\o/

r~

Re: [PATCH v2 4/9] hw/arm: Include missing 'exec/tswap.h' header

[Philippe Mathieu-Daudé]

On 12/12/24 00:53, Richard Henderson wrote:
> On 12/11/24 17:03, Philippe Mathieu-Daudé wrote:
>> Some files indirectly get "exec/tswap.h" declarations via
>> "exec/cpu-all.h". Include it directly to be able to remove
>> the former from the latter, otherwise we get:
>>
>>    hw/arm/boot.c:175:19: error: call to undeclared function 'tswap32'; 
>> ISO C99 and later do not support implicit function declarations [- 
>> Wimplicit-function-declaration]
>>      175 |         code[i] = tswap32(insn);
>>          |                   ^
>>    hw/arm/npcm7xx.c:326:26: error: call to undeclared function 
>> 'tswap32'; ISO C99 and later do not support implicit function 
>> declarations [-Wimplicit-function-declaration]
>>      326 |         board_setup[i] = tswap32(board_setup[i]);
>>          |                          ^
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/boot.c    | 1 +
>>   hw/arm/npcm7xx.c | 1 +
>>   2 files changed, 2 insertions(+)
>>
> 
> These could all be cpu_to_le32, since TARGET_BIG_ENDIAN is always false 
> for qemu-system-arm.

I agree, but last time I did that Peter insisted for tswap():
https://lore.kernel.org/qemu-devel/CAFEAcA8Exn0VMzM1h048q4Nm7toxkpaOv4B-ZE4FEXKgHoqy7A@mail.gmail.com/

Peter, was my wording in that previous series not clear (in that case
I can try to clarify) or was it OK but you reject the possibility of
using cpu_to_le32() to remove tswap() calls?

(Here my goal is to have a single binary, so I start removing target-
specific endianness).

Thanks,

Phil.