[3/3] hw/arm: Replace tswap32() calls by target agnostic stl_endian_p()

Commit Message

Philippe Mathieu-Daudé Sept. 30, 2024, 10:12 p.m. UTC

Replace the target-specific tswap32() calls by stl_endian_p()
which does the same but takes the endianness as argument, thus
is target-agnostic.
Get the vCPU endianness calling arm_cpu_code_is_big_endian().

Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
---
 hw/arm/boot.c        | 8 +++++---
 hw/arm/exynos4210.c  | 7 +++----
 hw/arm/npcm7xx.c     | 6 ++++--
 hw/arm/xilinx_zynq.c | 5 +++--
 4 files changed, 15 insertions(+), 11 deletions(-)

Comments

Peter Maydell Oct. 1, 2024, 9:35 a.m. UTC | #1

On Mon, 30 Sept 2024 at 23:12, Philippe Mathieu-Daudé <[e-mail]> wrote:
>
> Replace the target-specific tswap32() calls by stl_endian_p()
> which does the same but takes the endianness as argument, thus
> is target-agnostic.
> Get the vCPU endianness calling arm_cpu_code_is_big_endian().
>
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
>  hw/arm/boot.c        | 8 +++++---
>  hw/arm/exynos4210.c  | 7 +++----
>  hw/arm/npcm7xx.c     | 6 ++++--
>  hw/arm/xilinx_zynq.c | 5 +++--
>  4 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 6efd21f9c2..6e8dc00e6d 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -137,6 +137,7 @@ void arm_write_bootloader(const char *name,
>                            const uint32_t *fixupcontext)
>  {
>      AddressSpace *as = arm_boot_address_space(cpu, info);
> +    bool be = arm_cpu_code_is_big_endian(&cpu->env);
>      /* Fix up the specified bootloader fragment and write it into
>       * guest memory using rom_add_blob_fixed(). fixupcontext is
>       * an array giving the values to write in for the fixup types
> @@ -173,7 +174,7 @@ void arm_write_bootloader(const char *name,
>          default:
>              abort();
>          }
> -        code[i] = tswap32(insn);
> +        stl_endian_p(be, &code[i], insn);
>      }

This is a behaviour change. For Arm, TARGET_BIG_ENDIAN
is always false, so tswap32() is "swap to/from little endian".
But arm_cpu_code_is_big_endian() looks at the state of
the guest vCPU (specifically, its SCTLR.B bit) and so
may swap to either big or little endian.

These functions are also called before the CPU is
reset for the first time, and before do_cpu_reset()
has maybe set SCTLR_B based on the ELF file. So we
can't guarantee SCTLR.B to be set correctly here where
we're trying to use it.

Maybe we do get this wrong for the old ARMv6-and-earlier
BE32 model where SCTLR.B might be non-zero -- they're
such a niche case that I don't suppose gets tested often.
(ARMv7-and-up is BE8 and instructions are always little
endian even when data is big endian.) But we should
separate out bug fixes from refactorings.

thanks
-- PMM

Patch

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 6efd21f9c2..6e8dc00e6d 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -137,6 +137,7 @@  void arm_write_bootloader(const char *name,
                           const uint32_t *fixupcontext)
 {
     AddressSpace *as = arm_boot_address_space(cpu, info);
+    bool be = arm_cpu_code_is_big_endian(&cpu->env);
     /* Fix up the specified bootloader fragment and write it into
      * guest memory using rom_add_blob_fixed(). fixupcontext is
      * an array giving the values to write in for the fixup types
@@ -173,7 +174,7 @@  void arm_write_bootloader(const char *name,
         default:
             abort();
         }
-        code[i] = tswap32(insn);
+        stl_endian_p(be, &code[i], insn);
     }
 
     assert((len * sizeof(uint32_t)) < BOOTLOADER_MAX_SIZE);
@@ -205,6 +206,7 @@  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
                                             hwaddr mvbar_addr)
 {
     AddressSpace *as = arm_boot_address_space(cpu, info);
+    bool be = arm_cpu_code_is_big_endian(&cpu->env);
     int n;
     uint32_t mvbar_blob[] = {
         /* mvbar_addr: secure monitor vectors
@@ -243,13 +245,13 @@  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
           || (info->board_setup_addr + sizeof(board_setup_blob) <= mvbar_addr));
 
     for (n = 0; n < ARRAY_SIZE(mvbar_blob); n++) {
-        mvbar_blob[n] = tswap32(mvbar_blob[n]);
+        stl_endian_p(be, &mvbar_blob[n], mvbar_blob[n]);
     }
     rom_add_blob_fixed_as("board-setup-mvbar", mvbar_blob, sizeof(mvbar_blob),
                           mvbar_addr, as);
 
     for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
-        board_setup_blob[n] = tswap32(board_setup_blob[n]);
+        stl_endian_p(be, &board_setup_blob[n], board_setup_blob[n]);
     }
     rom_add_blob_fixed_as("board-setup", board_setup_blob,
                           sizeof(board_setup_blob), info->board_setup_addr, as);
diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index e3f1de2631..78e3fae3c1 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -23,7 +23,6 @@ 
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "exec/tswap.h"
 #include "cpu.h"
 #include "hw/cpu/a9mpcore.h"
 #include "hw/irq.h"
@@ -473,7 +472,7 @@  static const MemoryRegionOps exynos4210_chipid_and_omr_ops = {
 void exynos4210_write_secondary(ARMCPU *cpu,
         const struct arm_boot_info *info)
 {
-    int n;
+    bool be = arm_cpu_code_is_big_endian(&cpu->env);
     uint32_t smpboot[] = {
         0xe59f3034, /* ldr r3, External gic_cpu_if */
         0xe59f2034, /* ldr r2, Internal gic_cpu_if */
@@ -496,8 +495,8 @@  void exynos4210_write_secondary(ARMCPU *cpu,
     };
     smpboot[ARRAY_SIZE(smpboot) - 1] = info->smp_bootreg_addr;
     smpboot[ARRAY_SIZE(smpboot) - 2] = info->gic_cpu_if_addr;
-    for (n = 0; n < ARRAY_SIZE(smpboot); n++) {
-        smpboot[n] = tswap32(smpboot[n]);
+    for (int n = 0; n < ARRAY_SIZE(smpboot); n++) {
+        stl_endian_p(be, &smpboot[n], smpboot[n]);
     }
     rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
                        info->smp_loader_start);
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index cb7791301b..6afdbf1598 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -309,6 +309,7 @@  static const struct {
 static void npcm7xx_write_board_setup(ARMCPU *cpu,
                                       const struct arm_boot_info *info)
 {
+    bool be = arm_cpu_code_is_big_endian(&cpu->env);
     uint32_t board_setup[] = {
         0xe59f0010,     /* ldr r0, clk_base_addr */
         0xe59f1010,     /* ldr r1, pllcon1_value */
@@ -323,7 +324,7 @@  static void npcm7xx_write_board_setup(ARMCPU *cpu,
     int i;
 
     for (i = 0; i < ARRAY_SIZE(board_setup); i++) {
-        board_setup[i] = tswap32(board_setup[i]);
+        stl_endian_p(be, &board_setup[i], board_setup[i]);
     }
     rom_add_blob_fixed("board-setup", board_setup, sizeof(board_setup),
                        info->board_setup_addr);
@@ -332,6 +333,7 @@  static void npcm7xx_write_board_setup(ARMCPU *cpu,
 static void npcm7xx_write_secondary_boot(ARMCPU *cpu,
                                          const struct arm_boot_info *info)
 {
+    bool be = arm_cpu_code_is_big_endian(&cpu->env);
     /*
      * The default smpboot stub halts the secondary CPU with a 'wfi'
      * instruction, but the arch/arm/mach-npcm/platsmp.c in the Linux kernel
@@ -353,7 +355,7 @@  static void npcm7xx_write_secondary_boot(ARMCPU *cpu,
     int i;
 
     for (i = 0; i < ARRAY_SIZE(smpboot); i++) {
-        smpboot[i] = tswap32(smpboot[i]);
+        stl_endian_p(be, &smpboot[i], smpboot[i]);
     }
 
     rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 37c234f5ab..0d6e246543 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -36,9 +36,9 @@ 
 #include "hw/qdev-clock.h"
 #include "sysemu/reset.h"
 #include "qom/object.h"
-#include "exec/tswap.h"
 #include "target/arm/cpu-qom.h"
 #include "qapi/visitor.h"
+#include "cpu.h"
 
 #define TYPE_ZYNQ_MACHINE MACHINE_TYPE_NAME("xilinx-zynq-a9")
 OBJECT_DECLARE_SIMPLE_TYPE(ZynqMachineState, ZYNQ_MACHINE)
@@ -97,6 +97,7 @@  struct ZynqMachineState {
 static void zynq_write_board_setup(ARMCPU *cpu,
                                    const struct arm_boot_info *info)
 {
+    bool be = arm_cpu_code_is_big_endian(&cpu->env);
     int n;
     uint32_t board_setup_blob[] = {
         0xe3a004f8, /* mov r0, #0xf8000000 */
@@ -106,7 +107,7 @@  static void zynq_write_board_setup(ARMCPU *cpu,
         0xe12fff1e, /* bx lr */
     };
     for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
-        board_setup_blob[n] = tswap32(board_setup_blob[n]);
+        stl_endian_p(be, &board_setup_blob[n], board_setup_blob[n]);
     }
     rom_add_blob_fixed("board-setup", board_setup_blob,
                        sizeof(board_setup_blob), BOARD_SETUP_ADDR);