[PATCH 0/6] hw/arm: Fix smpboot[] on big-endian hosts and remove tswap32()

[PATCH 0/6] hw/arm: Fix smpboot[] on big-endian hosts and remove tswap32()

[Philippe Mathieu-Daudé]

ARM CPUs fetch instructions in little-endian.

smpboot[] encoded instructions are written in little-endian.

We call tswap32() on the array. tswap32 function swap a 32-bit
value if the target endianness doesn't match the host one.
Otherwise it is a NOP.

* On a little-endian host, the array is stored as it. tswap32()
  is a NOP, and the vCPU fetches the instructions as it, in
  little-endian.

* On a big-endian host, the array is stored as it. tswap32()
  swap the instructions to little-endian, and the vCPU fetches
  the instructions as it, in little-endian.

Using tswap() on system emulation is a bit odd: while the target
particularities might change the system emulation, the host ones
(such its endianness) shouldn't interfere.

We can simplify by using const_le32() to always store the
instructions in the array in little-endian, removing the need
for the dubious tswap().

Two boards which weren't swapping (aspeed and raspi) are fixed.

Tested running ARM avocado tests on x86_64 and s390x.

Philippe Mathieu-Daudé (6):
  hw/arm/aspeed: Fix smpboot[] on big-endian hosts
  hw/arm/raspi: Fix smpboot[] on big-endian hosts
  hw/arm/exynos4210: Remove tswap32() calls and constify smpboot[]
  hw/arm/npcm7xx: Remove tswap32() calls and constify smpboot[]
  hw/arm/xilinx_zynq: Remove tswap32() calls and constify smpboot[]
  hw/arm/boot: Remove tswap32() calls and constify board_setup_blob[]

 hw/arm/aspeed.c      | 28 ++++++++++++------------
 hw/arm/boot.c        | 52 +++++++++++++++++++-------------------------
 hw/arm/exynos4210.c  | 48 ++++++++++++++++++----------------------
 hw/arm/npcm7xx.c     | 49 +++++++++++++++++------------------------
 hw/arm/raspi.c       | 46 +++++++++++++++++++--------------------
 hw/arm/xilinx_zynq.c | 27 ++++++++++-------------
 6 files changed, 112 insertions(+), 138 deletions(-)

-- 
2.38.1

[PATCH 1/6] hw/arm/aspeed: Fix smpboot[] on big-endian hosts

[Philippe Mathieu-Daudé]

ARM CPUs fetch instructions in little-endian.

smpboot[] encoded instructions are written in little-endian.
This is fine on little-endian host, but on big-endian ones
the smpboot[] array ends swapped. Use the const_le32()
macro so the instructions are always in little-endian in the
smpboot[] array.

Fixes: 9bb6d14081 ("aspeed: Add boot stub for smp booting")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/aspeed.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 55f114ef72..adff9a0d73 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -194,22 +194,22 @@ static void aspeed_write_smpboot(ARMCPU *cpu,
          * r1 = AST_SMP_MBOX_FIELD_ENTRY
          * r0 = AST_SMP_MBOX_FIELD_GOSIGN
          */
-        0xee100fb0,  /* mrc     p15, 0, r0, c0, c0, 5 */
-        0xe21000ff,  /* ands    r0, r0, #255          */
-        0xe59f201c,  /* ldr     r2, [pc, #28]         */
-        0xe1822000,  /* orr     r2, r2, r0            */
+        const_le32(0xee100fb0),     /* mrc     p15, 0, r0, c0, c0, 5 */
+        const_le32(0xe21000ff),     /* ands    r0, r0, #255          */
+        const_le32(0xe59f201c),     /* ldr     r2, [pc, #28]         */
+        const_le32(0xe1822000),     /* orr     r2, r2, r0            */
 
-        0xe59f1018,  /* ldr     r1, [pc, #24]         */
-        0xe59f0018,  /* ldr     r0, [pc, #24]         */
+        const_le32(0xe59f1018),     /* ldr     r1, [pc, #24]         */
+        const_le32(0xe59f0018),     /* ldr     r0, [pc, #24]         */
 
-        0xe320f002,  /* wfe                           */
-        0xe5904000,  /* ldr     r4, [r0]              */
-        0xe1520004,  /* cmp     r2, r4                */
-        0x1afffffb,  /* bne     <wfe>                 */
-        0xe591f000,  /* ldr     pc, [r1]              */
-        AST_SMP_MBOX_GOSIGN,
-        AST_SMP_MBOX_FIELD_ENTRY,
-        AST_SMP_MBOX_FIELD_GOSIGN,
+        const_le32(0xe320f002),     /* wfe                           */
+        const_le32(0xe5904000),     /* ldr     r4, [r0]              */
+        const_le32(0xe1520004),     /* cmp     r2, r4                */
+        const_le32(0x1afffffb),     /* bne     <wfe>                 */
+        const_le32(0xe591f000),     /* ldr     pc, [r1]              */
+        const_le32(AST_SMP_MBOX_GOSIGN),
+        const_le32(AST_SMP_MBOX_FIELD_ENTRY),
+        const_le32(AST_SMP_MBOX_FIELD_GOSIGN)
     };
 
     rom_add_blob_fixed("aspeed.smpboot", poll_mailbox_ready,
-- 
2.38.1

Re: [PATCH 1/6] hw/arm/aspeed: Fix smpboot[] on big-endian hosts

[Cédric Le Goater]

On 12/22/22 22:55, Philippe Mathieu-Daudé wrote:
> ARM CPUs fetch instructions in little-endian.
> 
> smpboot[] encoded instructions are written in little-endian.
> This is fine on little-endian host, but on big-endian ones
> the smpboot[] array ends swapped. Use the const_le32()
> macro so the instructions are always in little-endian in the
> smpboot[] array.
> 
> Fixes: 9bb6d14081 ("aspeed: Add boot stub for smp booting")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


>   hw/arm/aspeed.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 55f114ef72..adff9a0d73 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -194,22 +194,22 @@ static void aspeed_write_smpboot(ARMCPU *cpu,
>            * r1 = AST_SMP_MBOX_FIELD_ENTRY
>            * r0 = AST_SMP_MBOX_FIELD_GOSIGN
>            */
> -        0xee100fb0,  /* mrc     p15, 0, r0, c0, c0, 5 */
> -        0xe21000ff,  /* ands    r0, r0, #255          */
> -        0xe59f201c,  /* ldr     r2, [pc, #28]         */
> -        0xe1822000,  /* orr     r2, r2, r0            */
> +        const_le32(0xee100fb0),     /* mrc     p15, 0, r0, c0, c0, 5 */
> +        const_le32(0xe21000ff),     /* ands    r0, r0, #255          */
> +        const_le32(0xe59f201c),     /* ldr     r2, [pc, #28]         */
> +        const_le32(0xe1822000),     /* orr     r2, r2, r0            */
>   
> -        0xe59f1018,  /* ldr     r1, [pc, #24]         */
> -        0xe59f0018,  /* ldr     r0, [pc, #24]         */
> +        const_le32(0xe59f1018),     /* ldr     r1, [pc, #24]         */
> +        const_le32(0xe59f0018),     /* ldr     r0, [pc, #24]         */
>   
> -        0xe320f002,  /* wfe                           */
> -        0xe5904000,  /* ldr     r4, [r0]              */
> -        0xe1520004,  /* cmp     r2, r4                */
> -        0x1afffffb,  /* bne     <wfe>                 */
> -        0xe591f000,  /* ldr     pc, [r1]              */
> -        AST_SMP_MBOX_GOSIGN,
> -        AST_SMP_MBOX_FIELD_ENTRY,
> -        AST_SMP_MBOX_FIELD_GOSIGN,
> +        const_le32(0xe320f002),     /* wfe                           */
> +        const_le32(0xe5904000),     /* ldr     r4, [r0]              */
> +        const_le32(0xe1520004),     /* cmp     r2, r4                */
> +        const_le32(0x1afffffb),     /* bne     <wfe>                 */
> +        const_le32(0xe591f000),     /* ldr     pc, [r1]              */
> +        const_le32(AST_SMP_MBOX_GOSIGN),
> +        const_le32(AST_SMP_MBOX_FIELD_ENTRY),
> +        const_le32(AST_SMP_MBOX_FIELD_GOSIGN)
>       };
>   
>       rom_add_blob_fixed("aspeed.smpboot", poll_mailbox_ready,

Re: [PATCH 1/6] hw/arm/aspeed: Fix smpboot[] on big-endian hosts

[Peter Maydell]

On Thu, 22 Dec 2022 at 21:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> ARM CPUs fetch instructions in little-endian.
>
> smpboot[] encoded instructions are written in little-endian.
> This is fine on little-endian host, but on big-endian ones
> the smpboot[] array ends swapped. Use the const_le32()
> macro so the instructions are always in little-endian in the
> smpboot[] array.
>
> Fixes: 9bb6d14081 ("aspeed: Add boot stub for smp booting")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/aspeed.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 55f114ef72..adff9a0d73 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -194,22 +194,22 @@ static void aspeed_write_smpboot(ARMCPU *cpu,
>           * r1 = AST_SMP_MBOX_FIELD_ENTRY
>           * r0 = AST_SMP_MBOX_FIELD_GOSIGN
>           */
> -        0xee100fb0,  /* mrc     p15, 0, r0, c0, c0, 5 */
> -        0xe21000ff,  /* ands    r0, r0, #255          */
> -        0xe59f201c,  /* ldr     r2, [pc, #28]         */
> -        0xe1822000,  /* orr     r2, r2, r0            */
> +        const_le32(0xee100fb0),     /* mrc     p15, 0, r0, c0, c0, 5 */
> +        const_le32(0xe21000ff),     /* ands    r0, r0, #255          */
> +        const_le32(0xe59f201c),     /* ldr     r2, [pc, #28]         */
> +        const_le32(0xe1822000),     /* orr     r2, r2, r0            */
>
> -        0xe59f1018,  /* ldr     r1, [pc, #24]         */
> -        0xe59f0018,  /* ldr     r0, [pc, #24]         */
> +        const_le32(0xe59f1018),     /* ldr     r1, [pc, #24]         */
> +        const_le32(0xe59f0018),     /* ldr     r0, [pc, #24]         */
>
> -        0xe320f002,  /* wfe                           */
> -        0xe5904000,  /* ldr     r4, [r0]              */
> -        0xe1520004,  /* cmp     r2, r4                */
> -        0x1afffffb,  /* bne     <wfe>                 */
> -        0xe591f000,  /* ldr     pc, [r1]              */
> -        AST_SMP_MBOX_GOSIGN,
> -        AST_SMP_MBOX_FIELD_ENTRY,
> -        AST_SMP_MBOX_FIELD_GOSIGN,
> +        const_le32(0xe320f002),     /* wfe                           */
> +        const_le32(0xe5904000),     /* ldr     r4, [r0]              */
> +        const_le32(0xe1520004),     /* cmp     r2, r4                */
> +        const_le32(0x1afffffb),     /* bne     <wfe>                 */
> +        const_le32(0xe591f000),     /* ldr     pc, [r1]              */
> +        const_le32(AST_SMP_MBOX_GOSIGN),
> +        const_le32(AST_SMP_MBOX_FIELD_ENTRY),
> +        const_le32(AST_SMP_MBOX_FIELD_GOSIGN)
>      };
>
>      rom_add_blob_fixed("aspeed.smpboot", poll_mailbox_ready,

Can we use the write_bootloader() function, which handles the
endianness question correctly and is how other boards do the
"put a little lump of code into the guest" job ?

thanks
-- PMM

Re: [PATCH 1/6] hw/arm/aspeed: Fix smpboot[] on big-endian hosts

[Cédric Le Goater]

On 1/3/23 18:33, Peter Maydell wrote:
> On Thu, 22 Dec 2022 at 21:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> ARM CPUs fetch instructions in little-endian.
>>
>> smpboot[] encoded instructions are written in little-endian.
>> This is fine on little-endian host, but on big-endian ones
>> the smpboot[] array ends swapped. Use the const_le32()
>> macro so the instructions are always in little-endian in the
>> smpboot[] array.
>>
>> Fixes: 9bb6d14081 ("aspeed: Add boot stub for smp booting")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/aspeed.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 55f114ef72..adff9a0d73 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -194,22 +194,22 @@ static void aspeed_write_smpboot(ARMCPU *cpu,
>>            * r1 = AST_SMP_MBOX_FIELD_ENTRY
>>            * r0 = AST_SMP_MBOX_FIELD_GOSIGN
>>            */
>> -        0xee100fb0,  /* mrc     p15, 0, r0, c0, c0, 5 */
>> -        0xe21000ff,  /* ands    r0, r0, #255          */
>> -        0xe59f201c,  /* ldr     r2, [pc, #28]         */
>> -        0xe1822000,  /* orr     r2, r2, r0            */
>> +        const_le32(0xee100fb0),     /* mrc     p15, 0, r0, c0, c0, 5 */
>> +        const_le32(0xe21000ff),     /* ands    r0, r0, #255          */
>> +        const_le32(0xe59f201c),     /* ldr     r2, [pc, #28]         */
>> +        const_le32(0xe1822000),     /* orr     r2, r2, r0            */
>>
>> -        0xe59f1018,  /* ldr     r1, [pc, #24]         */
>> -        0xe59f0018,  /* ldr     r0, [pc, #24]         */
>> +        const_le32(0xe59f1018),     /* ldr     r1, [pc, #24]         */
>> +        const_le32(0xe59f0018),     /* ldr     r0, [pc, #24]         */
>>
>> -        0xe320f002,  /* wfe                           */
>> -        0xe5904000,  /* ldr     r4, [r0]              */
>> -        0xe1520004,  /* cmp     r2, r4                */
>> -        0x1afffffb,  /* bne     <wfe>                 */
>> -        0xe591f000,  /* ldr     pc, [r1]              */
>> -        AST_SMP_MBOX_GOSIGN,
>> -        AST_SMP_MBOX_FIELD_ENTRY,
>> -        AST_SMP_MBOX_FIELD_GOSIGN,
>> +        const_le32(0xe320f002),     /* wfe                           */
>> +        const_le32(0xe5904000),     /* ldr     r4, [r0]              */
>> +        const_le32(0xe1520004),     /* cmp     r2, r4                */
>> +        const_le32(0x1afffffb),     /* bne     <wfe>                 */
>> +        const_le32(0xe591f000),     /* ldr     pc, [r1]              */
>> +        const_le32(AST_SMP_MBOX_GOSIGN),
>> +        const_le32(AST_SMP_MBOX_FIELD_ENTRY),
>> +        const_le32(AST_SMP_MBOX_FIELD_GOSIGN)
>>       };
>>
>>       rom_add_blob_fixed("aspeed.smpboot", poll_mailbox_ready,
> 
> Can we use the write_bootloader() function, which handles the
> endianness question correctly and is how other boards do the
> "put a little lump of code into the guest" job ?

Yes. See below.

May be we could change write_bootloader a little to handle an empty
fixupcontext.

Thanks,

C.

 From 671d43faa7e14b896855403feb0afd777350cb0a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Wed, 4 Jan 2023 09:30:28 +0100
Subject: [PATCH] hw/arm/boot: Export write_bootloader for Aspeed machines
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

AST2600 Aspeed machines have an home made boot loader for secondaries.
Instead, use the internal ARM boot loader.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
  include/hw/arm/boot.h | 24 ++++++++++++++++++++++++
  hw/arm/aspeed.c       | 42 ++++++++++++++++++++++--------------------
  hw/arm/boot.c         | 34 +++++++---------------------------
  3 files changed, 53 insertions(+), 47 deletions(-)

diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index f18cc3064f..23edd0d31b 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -183,4 +183,28 @@ void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
                                              const struct arm_boot_info *info,
                                              hwaddr mvbar_addr);
  
+typedef enum {
+    FIXUP_NONE = 0,     /* do nothing */
+    FIXUP_TERMINATOR,   /* end of insns */
+    FIXUP_BOARDID,      /* overwrite with board ID number */
+    FIXUP_BOARD_SETUP,  /* overwrite with board specific setup code address */
+    FIXUP_ARGPTR_LO,    /* overwrite with pointer to kernel args */
+    FIXUP_ARGPTR_HI,    /* overwrite with pointer to kernel args (high half) */
+    FIXUP_ENTRYPOINT_LO, /* overwrite with kernel entry point */
+    FIXUP_ENTRYPOINT_HI, /* overwrite with kernel entry point (high half) */
+    FIXUP_GIC_CPU_IF,   /* overwrite with GIC CPU interface address */
+    FIXUP_BOOTREG,      /* overwrite with boot register address */
+    FIXUP_DSB,          /* overwrite with correct DSB insn for cpu */
+    FIXUP_MAX,
+} FixupType;
+
+typedef struct ARMInsnFixup {
+    uint32_t insn;
+    FixupType fixup;
+} ARMInsnFixup;
+
+void arm_write_bootloader(const char *name, hwaddr addr,
+                          const ARMInsnFixup *insns, uint32_t *fixupcontext,
+                          AddressSpace *as);
+
  #endif /* HW_ARM_BOOT_H */
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 9c60575cb8..311c0091ca 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -201,33 +201,35 @@ struct AspeedMachineState {
  static void aspeed_write_smpboot(ARMCPU *cpu,
                                   const struct arm_boot_info *info)
  {
-    static const uint32_t poll_mailbox_ready[] = {
+    AddressSpace *as = arm_boot_address_space(cpu, info);
+    static const ARMInsnFixup poll_mailbox_ready[] = {
          /*
           * r2 = per-cpu go sign value
           * r1 = AST_SMP_MBOX_FIELD_ENTRY
           * r0 = AST_SMP_MBOX_FIELD_GOSIGN
           */
-        0xee100fb0,  /* mrc     p15, 0, r0, c0, c0, 5 */
-        0xe21000ff,  /* ands    r0, r0, #255          */
-        0xe59f201c,  /* ldr     r2, [pc, #28]         */
-        0xe1822000,  /* orr     r2, r2, r0            */
-
-        0xe59f1018,  /* ldr     r1, [pc, #24]         */
-        0xe59f0018,  /* ldr     r0, [pc, #24]         */
-
-        0xe320f002,  /* wfe                           */
-        0xe5904000,  /* ldr     r4, [r0]              */
-        0xe1520004,  /* cmp     r2, r4                */
-        0x1afffffb,  /* bne     <wfe>                 */
-        0xe591f000,  /* ldr     pc, [r1]              */
-        AST_SMP_MBOX_GOSIGN,
-        AST_SMP_MBOX_FIELD_ENTRY,
-        AST_SMP_MBOX_FIELD_GOSIGN,
+        { 0xee100fb0 },  /* mrc     p15, 0, r0, c0, c0, 5 */
+        { 0xe21000ff },  /* ands    r0, r0, #255          */
+        { 0xe59f201c },  /* ldr     r2, [pc, #28]         */
+        { 0xe1822000 },  /* orr     r2, r2, r0            */
+
+        { 0xe59f1018 },  /* ldr     r1, [pc, #24]         */
+        { 0xe59f0018 },  /* ldr     r0, [pc, #24]         */
+
+        { 0xe320f002 },  /* wfe                           */
+        { 0xe5904000 },  /* ldr     r4, [r0]              */
+        { 0xe1520004 },  /* cmp     r2, r4                */
+        { 0x1afffffb },  /* bne     <wfe>                 */
+        { 0xe591f000 },  /* ldr     pc, [r1]              */
+        { AST_SMP_MBOX_GOSIGN },
+        { AST_SMP_MBOX_FIELD_ENTRY },
+        { AST_SMP_MBOX_FIELD_GOSIGN },
+        { 0, FIXUP_TERMINATOR }
      };
+    uint32_t fixupcontext[FIXUP_MAX] = { 0 };
  
-    rom_add_blob_fixed("aspeed.smpboot", poll_mailbox_ready,
-                       sizeof(poll_mailbox_ready),
-                       info->smp_loader_start);
+    arm_write_bootloader("aspeed.smpboot", info->smp_loader_start,
+                         poll_mailbox_ready, fixupcontext, as);
  }
  
  static void aspeed_reset_secondary(ARMCPU *cpu,
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 3d7d11f782..ed6fd7c77f 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -59,26 +59,6 @@ AddressSpace *arm_boot_address_space(ARMCPU *cpu,
      return cpu_get_address_space(cs, asidx);
  }
  
-typedef enum {
-    FIXUP_NONE = 0,     /* do nothing */
-    FIXUP_TERMINATOR,   /* end of insns */
-    FIXUP_BOARDID,      /* overwrite with board ID number */
-    FIXUP_BOARD_SETUP,  /* overwrite with board specific setup code address */
-    FIXUP_ARGPTR_LO,    /* overwrite with pointer to kernel args */
-    FIXUP_ARGPTR_HI,    /* overwrite with pointer to kernel args (high half) */
-    FIXUP_ENTRYPOINT_LO, /* overwrite with kernel entry point */
-    FIXUP_ENTRYPOINT_HI, /* overwrite with kernel entry point (high half) */
-    FIXUP_GIC_CPU_IF,   /* overwrite with GIC CPU interface address */
-    FIXUP_BOOTREG,      /* overwrite with boot register address */
-    FIXUP_DSB,          /* overwrite with correct DSB insn for cpu */
-    FIXUP_MAX,
-} FixupType;
-
-typedef struct ARMInsnFixup {
-    uint32_t insn;
-    FixupType fixup;
-} ARMInsnFixup;
-
  static const ARMInsnFixup bootloader_aarch64[] = {
      { 0x580000c0 }, /* ldr x0, arg ; Load the lower 32-bits of DTB */
      { 0xaa1f03e1 }, /* mov x1, xzr */
@@ -149,9 +129,9 @@ static const ARMInsnFixup smpboot[] = {
      { 0, FIXUP_TERMINATOR }
  };
  
-static void write_bootloader(const char *name, hwaddr addr,
-                             const ARMInsnFixup *insns, uint32_t *fixupcontext,
-                             AddressSpace *as)
+void arm_write_bootloader(const char *name, hwaddr addr,
+                          const ARMInsnFixup *insns, uint32_t *fixupcontext,
+                          AddressSpace *as)
  {
      /* Fix up the specified bootloader fragment and write it into
       * guest memory using rom_add_blob_fixed(). fixupcontext is
@@ -213,8 +193,8 @@ static void default_write_secondary(ARMCPU *cpu,
          fixupcontext[FIXUP_DSB] = CP15_DSB_INSN;
      }
  
-    write_bootloader("smpboot", info->smp_loader_start,
-                     smpboot, fixupcontext, as);
+    arm_write_bootloader("smpboot", info->smp_loader_start,
+                         smpboot, fixupcontext, as);
  }
  
  void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
@@ -1173,8 +1153,8 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
          fixupcontext[FIXUP_ENTRYPOINT_LO] = entry;
          fixupcontext[FIXUP_ENTRYPOINT_HI] = entry >> 32;
  
-        write_bootloader("bootloader", info->loader_start,
-                         primary_loader, fixupcontext, as);
+        arm_write_bootloader("bootloader", info->loader_start,
+                             primary_loader, fixupcontext, as);
  
          if (info->write_board_setup) {
              info->write_board_setup(cpu, info);
-- 
2.38.1

Re: [PATCH 1/6] hw/arm/aspeed: Fix smpboot[] on big-endian hosts

[Philippe Mathieu-Daudé]

Hi Cédric,

On 4/1/23 09:43, Cédric Le Goater wrote:
> On 1/3/23 18:33, Peter Maydell wrote:

>> Can we use the write_bootloader() function, which handles the
>> endianness question correctly and is how other boards do the
>> "put a little lump of code into the guest" job ?
> 
> Yes. See below.
> 
> May be we could change write_bootloader a little to handle an empty
> fixupcontext.
> 
> Thanks,
> 
> C.
> 
>  From 671d43faa7e14b896855403feb0afd777350cb0a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
> Date: Wed, 4 Jan 2023 09:30:28 +0100
> Subject: [PATCH] hw/arm/boot: Export write_bootloader for Aspeed machines
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> AST2600 Aspeed machines have an home made boot loader for secondaries.
> Instead, use the internal ARM boot loader.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/arm/boot.h | 24 ++++++++++++++++++++++++
>   hw/arm/aspeed.c       | 42 ++++++++++++++++++++++--------------------
>   hw/arm/boot.c         | 34 +++++++---------------------------
>   3 files changed, 53 insertions(+), 47 deletions(-)
[...]

I'm getting:

Applying: hw/arm/boot: Export write_bootloader for Aspeed machines
error: patch failed: include/hw/arm/boot.h:183
error: include/hw/arm/boot.h: patch does not apply
error: patch failed: hw/arm/aspeed.c:201
error: hw/arm/aspeed.c: patch does not apply
error: patch failed: hw/arm/boot.c:59
error: hw/arm/boot.c: patch does not apply
Patch failed at 0001 hw/arm/boot: Export write_bootloader for Aspeed 
machines
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

What is your base commit? Can you post a normal patch?

Thanks,

Phil.

Re: [PATCH 1/6] hw/arm/aspeed: Fix smpboot[] on big-endian hosts

[Cédric Le Goater]

On 1/4/23 23:35, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 4/1/23 09:43, Cédric Le Goater wrote:
>> On 1/3/23 18:33, Peter Maydell wrote:
> 
>>> Can we use the write_bootloader() function, which handles the
>>> endianness question correctly and is how other boards do the
>>> "put a little lump of code into the guest" job ?
>>
>> Yes. See below.
>>
>> May be we could change write_bootloader a little to handle an empty
>> fixupcontext.
>>
>> Thanks,
>>
>> C.
>>
>>  From 671d43faa7e14b896855403feb0afd777350cb0a Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
>> Date: Wed, 4 Jan 2023 09:30:28 +0100
>> Subject: [PATCH] hw/arm/boot: Export write_bootloader for Aspeed machines
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> AST2600 Aspeed machines have an home made boot loader for secondaries.
>> Instead, use the internal ARM boot loader.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/arm/boot.h | 24 ++++++++++++++++++++++++
>>   hw/arm/aspeed.c       | 42 ++++++++++++++++++++++--------------------
>>   hw/arm/boot.c         | 34 +++++++---------------------------
>>   3 files changed, 53 insertions(+), 47 deletions(-)
> [...]
> 
> I'm getting:
> 
> Applying: hw/arm/boot: Export write_bootloader for Aspeed machines
> error: patch failed: include/hw/arm/boot.h:183
> error: include/hw/arm/boot.h: patch does not apply
> error: patch failed: hw/arm/aspeed.c:201
> error: hw/arm/aspeed.c: patch does not apply
> error: patch failed: hw/arm/boot.c:59
> error: hw/arm/boot.c: patch does not apply
> Patch failed at 0001 hw/arm/boot: Export write_bootloader for Aspeed machines
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> What is your base commit? 

It applies on 222059a0fc ("Merge tag 'pull-ppc-20221221' of
https://gitlab.com/danielhb/qemu into staging")

> Can you post a normal patch?

Sure.

C.

[PATCH 2/6] hw/arm/raspi: Fix smpboot[] on big-endian hosts

[Philippe Mathieu-Daudé]

ARM CPUs fetch instructions in little-endian.

smpboot[] encoded instructions are written in little-endian.
This is fine on little-endian host, but on big-endian ones
the smpboot[] array ends swapped. Use the const_le32()
macro so the instructions are always in little-endian in the
smpboot[] array.

Fixes: 1df7d1f930 ("raspi: add raspberry pi 2 machine")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/raspi.c | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 92d068d1f9..72572a45c2 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -125,18 +125,18 @@ static const char *board_type(uint32_t board_rev)
 static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     static const uint32_t smpboot[] = {
-        0xe1a0e00f, /*    mov     lr, pc */
-        0xe3a0fe00 + (BOARDSETUP_ADDR >> 4), /* mov pc, BOARDSETUP_ADDR */
-        0xee100fb0, /*    mrc     p15, 0, r0, c0, c0, 5;get core ID */
-        0xe7e10050, /*    ubfx    r0, r0, #0, #2       ;extract LSB */
-        0xe59f5014, /*    ldr     r5, =0x400000CC      ;load mbox base */
-        0xe320f001, /* 1: yield */
-        0xe7953200, /*    ldr     r3, [r5, r0, lsl #4] ;read mbox for our core*/
-        0xe3530000, /*    cmp     r3, #0               ;spin while zero */
-        0x0afffffb, /*    beq     1b */
-        0xe7853200, /*    str     r3, [r5, r0, lsl #4] ;clear mbox */
-        0xe12fff13, /*    bx      r3                   ;jump to target */
-        0x400000cc, /* (constant: mailbox 3 read/clear base) */
+        const_le32(0xe1a0e00f), /*    mov   lr, pc */
+        const_le32(0xe3a0fe00 + (BOARDSETUP_ADDR >> 4)), /* mov pc, BOARDSETUP_ADDR */
+        const_le32(0xee100fb0), /*    mrc   p15, 0, r0, c0, c0, 5;get core ID */
+        const_le32(0xe7e10050), /*    ubfx  r0, r0, #0, #2       ;extract LSB */
+        const_le32(0xe59f5014), /*    ldr   r5, =0x400000CC      ;load mbox base */
+        const_le32(0xe320f001), /* 1: yield */
+        const_le32(0xe7953200), /*    ldr   r3, [r5, r0, lsl #4] ;read mbox for our core*/
+        const_le32(0xe3530000), /*    cmp   r3, #0               ;spin while zero */
+        const_le32(0x0afffffb), /*    beq   1b */
+        const_le32(0xe7853200), /*    str   r3, [r5, r0, lsl #4] ;clear mbox */
+        const_le32(0xe12fff13), /*    bx    r3                   ;jump to target */
+        const_le32(0x400000cc), /* (constant: mailbox 3 read/clear base) */
     };
 
     /* check that we don't overrun board setup vectors */
@@ -162,17 +162,17 @@ static void write_smpboot64(ARMCPU *cpu, const struct arm_boot_info *info)
      * a rom blob, so that the reset for ROM contents zeroes them for us.
      */
     static const uint32_t smpboot[] = {
-        0xd2801b05, /*        mov     x5, 0xd8 */
-        0xd53800a6, /*        mrs     x6, mpidr_el1 */
-        0x924004c6, /*        and     x6, x6, #0x3 */
-        0xd503205f, /* spin:  wfe */
-        0xf86678a4, /*        ldr     x4, [x5,x6,lsl #3] */
-        0xb4ffffc4, /*        cbz     x4, spin */
-        0xd2800000, /*        mov     x0, #0x0 */
-        0xd2800001, /*        mov     x1, #0x0 */
-        0xd2800002, /*        mov     x2, #0x0 */
-        0xd2800003, /*        mov     x3, #0x0 */
-        0xd61f0080, /*        br      x4 */
+        const_le32(0xd2801b05), /*        mov     x5, 0xd8 */
+        const_le32(0xd53800a6), /*        mrs     x6, mpidr_el1 */
+        const_le32(0x924004c6), /*        and     x6, x6, #0x3 */
+        const_le32(0xd503205f), /* spin:  wfe */
+        const_le32(0xf86678a4), /*        ldr     x4, [x5,x6,lsl #3] */
+        const_le32(0xb4ffffc4), /*        cbz     x4, spin */
+        const_le32(0xd2800000), /*        mov     x0, #0x0 */
+        const_le32(0xd2800001), /*        mov     x1, #0x0 */
+        const_le32(0xd2800002), /*        mov     x2, #0x0 */
+        const_le32(0xd2800003), /*        mov     x3, #0x0 */
+        const_le32(0xd61f0080), /*        br      x4 */
     };
 
     static const uint64_t spintables[] = {
-- 
2.38.1

[PATCH 3/6] hw/arm/exynos4210: Remove tswap32() calls and constify smpboot[]

[Philippe Mathieu-Daudé]

ARM CPUs fetch instructions in little-endian.

smpboot[] encoded instructions are written in little-endian.

We call tswap32() on the array. tswap32 function swap a 32-bit
value if the target endianness doesn't match the host one.
Otherwise it is a NOP.

* On a little-endian host, the array is stored as it. tswap32()
  is a NOP, and the vCPU fetches the instructions as it, in
  little-endian.

* On a big-endian host, the array is stored as it. tswap32()
  swap the instructions to little-endian, and the vCPU fetches
  the instructions as it, in little-endian.

Using tswap() on system emulation is a bit odd: while the target
particularities might change the system emulation, the host ones
(such its endianness) shouldn't interfere.

We can simplify by using const_le32() to always store the
instructions in the array in little-endian, removing the need
for the dubious tswap().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/exynos4210.c | 48 ++++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 8dafa2215b..89ee83456d 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -468,35 +468,29 @@ static const MemoryRegionOps exynos4210_chipid_and_omr_ops = {
     }
 };
 
-void exynos4210_write_secondary(ARMCPU *cpu,
-        const struct arm_boot_info *info)
+void exynos4210_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
 {
-    int n;
-    uint32_t smpboot[] = {
-        0xe59f3034, /* ldr r3, External gic_cpu_if */
-        0xe59f2034, /* ldr r2, Internal gic_cpu_if */
-        0xe59f0034, /* ldr r0, startaddr */
-        0xe3a01001, /* mov r1, #1 */
-        0xe5821000, /* str r1, [r2] */
-        0xe5831000, /* str r1, [r3] */
-        0xe3a010ff, /* mov r1, #0xff */
-        0xe5821004, /* str r1, [r2, #4] */
-        0xe5831004, /* str r1, [r3, #4] */
-        0xf57ff04f, /* dsb */
-        0xe320f003, /* wfi */
-        0xe5901000, /* ldr     r1, [r0] */
-        0xe1110001, /* tst     r1, r1 */
-        0x0afffffb, /* beq     <wfi> */
-        0xe12fff11, /* bx      r1 */
-        EXYNOS4210_EXT_GIC_CPU_BASE_ADDR,
-        0,          /* gic_cpu_if: base address of Internal GIC CPU interface */
-        0           /* bootreg: Boot register address is held here */
+    const uint32_t smpboot[] = {
+        const_le32(0xe59f3034),     /* ldr r3, External gic_cpu_if */
+        const_le32(0xe59f2034),     /* ldr r2, Internal gic_cpu_if */
+        const_le32(0xe59f0034),     /* ldr r0, startaddr */
+        const_le32(0xe3a01001),     /* mov r1, #1 */
+        const_le32(0xe5821000),     /* str r1, [r2] */
+        const_le32(0xe5831000),     /* str r1, [r3] */
+        const_le32(0xe3a010ff),     /* mov r1, #0xff */
+        const_le32(0xe5821004),     /* str r1, [r2, #4] */
+        const_le32(0xe5831004),     /* str r1, [r3, #4] */
+        const_le32(0xf57ff04f),     /* dsb */
+        const_le32(0xe320f003),     /* wfi */
+        const_le32(0xe5901000),     /* ldr     r1, [r0] */
+        const_le32(0xe1110001),     /* tst     r1, r1 */
+        const_le32(0x0afffffb),     /* beq     <wfi> */
+        const_le32(0xe12fff11),     /* bx      r1 */
+        const_le32(EXYNOS4210_EXT_GIC_CPU_BASE_ADDR),
+        cpu_to_le32(info->gic_cpu_if_addr), /* base address of Internal GIC CPU interface */
+        cpu_to_le32(info->smp_bootreg_addr) /* Boot register address is held here */
     };
-    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]);
-    }
+
     rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
                        info->smp_loader_start);
 }
-- 
2.38.1

[PATCH 4/6] hw/arm/npcm7xx: Remove tswap32() calls and constify smpboot[]

[Philippe Mathieu-Daudé]

ARM CPUs fetch instructions in little-endian.

smpboot[] encoded instructions are written in little-endian.

We call tswap32() on the array. tswap32 function swap a 32-bit
value if the target endianness doesn't match the host one.
Otherwise it is a NOP.

* On a little-endian host, the array is stored as it. tswap32()
  is a NOP, and the vCPU fetches the instructions as it, in
  little-endian.

* On a big-endian host, the array is stored as it. tswap32()
  swap the instructions to little-endian, and the vCPU fetches
  the instructions as it, in little-endian.

Using tswap() on system emulation is a bit odd: while the target
particularities might change the system emulation, the host ones
(such its endianness) shouldn't interfere.

We can simplify by using const_le32() to always store the
instructions in the array in little-endian, removing the need
for the dubious tswap().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Note there is still a tswap() call in npcm7xx_init_fuses()
---
 hw/arm/npcm7xx.c | 49 ++++++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index d85cc02765..2976192731 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -291,22 +291,18 @@ static const struct {
 static void npcm7xx_write_board_setup(ARMCPU *cpu,
                                       const struct arm_boot_info *info)
 {
-    uint32_t board_setup[] = {
-        0xe59f0010,     /* ldr r0, clk_base_addr */
-        0xe59f1010,     /* ldr r1, pllcon1_value */
-        0xe5801010,     /* str r1, [r0, #16] */
-        0xe59f100c,     /* ldr r1, clksel_value */
-        0xe5801004,     /* str r1, [r0, #4] */
-        0xe12fff1e,     /* bx lr */
-        NPCM7XX_CLK_BA,
-        NPCM7XX_PLLCON1_FIXUP_VAL,
-        NPCM7XX_CLKSEL_FIXUP_VAL,
+    static const uint32_t board_setup[] = {
+        const_le32(0xe59f0010),     /* ldr r0, clk_base_addr */
+        const_le32(0xe59f1010),     /* ldr r1, pllcon1_value */
+        const_le32(0xe5801010),     /* str r1, [r0, #16] */
+        const_le32(0xe59f100c),     /* ldr r1, clksel_value */
+        const_le32(0xe5801004),     /* str r1, [r0, #4] */
+        const_le32(0xe12fff1e),     /* bx lr */
+        const_le32(NPCM7XX_CLK_BA),
+        const_le32(NPCM7XX_PLLCON1_FIXUP_VAL),
+        const_le32(NPCM7XX_CLKSEL_FIXUP_VAL),
     };
-    int i;
 
-    for (i = 0; i < ARRAY_SIZE(board_setup); i++) {
-        board_setup[i] = tswap32(board_setup[i]);
-    }
     rom_add_blob_fixed("board-setup", board_setup, sizeof(board_setup),
                        info->board_setup_addr);
 }
@@ -321,22 +317,17 @@ static void npcm7xx_write_secondary_boot(ARMCPU *cpu,
      * we need to provide our own smpboot stub that can not use 'wfi', it has
      * to spin the secondary CPU until the first CPU writes to the SCRPAD reg.
      */
-    uint32_t smpboot[] = {
-        0xe59f2018,     /* ldr r2, bootreg_addr */
-        0xe3a00000,     /* mov r0, #0 */
-        0xe5820000,     /* str r0, [r2] */
-        0xe320f002,     /* wfe */
-        0xe5921000,     /* ldr r1, [r2] */
-        0xe1110001,     /* tst r1, r1 */
-        0x0afffffb,     /* beq <wfe> */
-        0xe12fff11,     /* bx r1 */
-        NPCM7XX_SMP_BOOTREG_ADDR,
+    static const uint32_t smpboot[] = {
+        const_le32(0xe59f2018),             /* ldr r2, bootreg_addr */
+        const_le32(0xe3a00000),             /* mov r0, #0 */
+        const_le32(0xe5820000),             /* str r0, [r2] */
+        const_le32(0xe320f002),             /* wfe */
+        const_le32(0xe5921000),             /* ldr r1, [r2] */
+        const_le32(0xe1110001),             /* tst r1, r1 */
+        const_le32(0x0afffffb),             /* beq <wfe> */
+        const_le32(0xe12fff11),             /* bx r1 */
+        const_le32(NPCM7XX_SMP_BOOTREG_ADDR),
     };
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(smpboot); i++) {
-        smpboot[i] = tswap32(smpboot[i]);
-    }
 
     rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
                        NPCM7XX_SMP_LOADER_START);
-- 
2.38.1

[PATCH 5/6] hw/arm/xilinx_zynq: Remove tswap32() calls and constify smpboot[]

[Philippe Mathieu-Daudé]

ARM CPUs fetch instructions in little-endian.

smpboot[] encoded instructions are written in little-endian.

We call tswap32() on the array. tswap32 function swap a 32-bit
value if the target endianness doesn't match the host one.
Otherwise it is a NOP.

* On a little-endian host, the array is stored as it. tswap32()
  is a NOP, and the vCPU fetches the instructions as it, in
  little-endian.

* On a big-endian host, the array is stored as it. tswap32()
  swap the instructions to little-endian, and the vCPU fetches
  the instructions as it, in little-endian.

Using tswap() on system emulation is a bit odd: while the target
particularities might change the system emulation, the host ones
(such its endianness) shouldn't interfere.

We can simplify by using const_le32() to always store the
instructions in the array in little-endian, removing the need
for the dubious tswap().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/xilinx_zynq.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3190cc0b8d..4316143b71 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -71,6 +71,11 @@ static const int dma_irqs[8] = {
 
 #define ZYNQ_SDHCI_CAPABILITIES 0x69ec0080  /* Datasheet: UG585 (v1.12.1) */
 
+struct ZynqMachineState {
+    MachineState parent;
+    Clock *ps_clk;
+};
+
 #define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
                         extract32((x), 12,  4) << 16)
 
@@ -79,29 +84,21 @@ static const int dma_irqs[8] = {
  */
 
 #define SLCR_WRITE(addr, val) \
-    0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16)), /* movw r1 ... */ \
-    0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
-    0xe5801000 + (addr)
-
-struct ZynqMachineState {
-    MachineState parent;
-    Clock *ps_clk;
-};
+    cpu_to_le32(0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16))), /* movw r1 ... */ \
+    cpu_to_le32(0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16))), /* movt r1 ... */ \
+    const_le32(0xe5801000 + (addr))
 
 static void zynq_write_board_setup(ARMCPU *cpu,
                                    const struct arm_boot_info *info)
 {
-    int n;
-    uint32_t board_setup_blob[] = {
-        0xe3a004f8, /* mov r0, #0xf8000000 */
+    const uint32_t board_setup_blob[] = {
+        const_le32(0xe3a004f8),         /* mov r0, #0xf8000000 */
         SLCR_WRITE(SLCR_UNLOCK_OFFSET, SLCR_XILINX_UNLOCK_KEY),
         SLCR_WRITE(SLCR_ARM_PLL_OFFSET, 0x00014008),
         SLCR_WRITE(SLCR_LOCK_OFFSET, SLCR_XILINX_LOCK_KEY),
-        0xe12fff1e, /* bx lr */
+        const_le32(0xe12fff1e)          /* bx lr */
     };
-    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
-        board_setup_blob[n] = tswap32(board_setup_blob[n]);
-    }
+
     rom_add_blob_fixed("board-setup", board_setup_blob,
                        sizeof(board_setup_blob), BOARD_SETUP_ADDR);
 }
-- 
2.38.1

Re: [PATCH 5/6] hw/arm/xilinx_zynq: Remove tswap32() calls and constify smpboot[]

[Edgar E. Iglesias]

On Thu, Dec 22, 2022 at 10:55:48PM +0100, Philippe Mathieu-Daudé wrote:
> ARM CPUs fetch instructions in little-endian.
> 
> smpboot[] encoded instructions are written in little-endian.
> 
> We call tswap32() on the array. tswap32 function swap a 32-bit
> value if the target endianness doesn't match the host one.
> Otherwise it is a NOP.
> 
> * On a little-endian host, the array is stored as it. tswap32()
>   is a NOP, and the vCPU fetches the instructions as it, in
>   little-endian.
> 
> * On a big-endian host, the array is stored as it. tswap32()
>   swap the instructions to little-endian, and the vCPU fetches
>   the instructions as it, in little-endian.
> 
> Using tswap() on system emulation is a bit odd: while the target
> particularities might change the system emulation, the host ones
> (such its endianness) shouldn't interfere.
> 
> We can simplify by using const_le32() to always store the
> instructions in the array in little-endian, removing the need
> for the dubious tswap().


Hi Philippe,


> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/xilinx_zynq.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 3190cc0b8d..4316143b71 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -71,6 +71,11 @@ static const int dma_irqs[8] = {
>  
>  #define ZYNQ_SDHCI_CAPABILITIES 0x69ec0080  /* Datasheet: UG585 (v1.12.1) */
>  
> +struct ZynqMachineState {
> +    MachineState parent;
> +    Clock *ps_clk;
> +};
> +
>  #define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
>                          extract32((x), 12,  4) << 16)
>  
> @@ -79,29 +84,21 @@ static const int dma_irqs[8] = {
>   */
>  
>  #define SLCR_WRITE(addr, val) \
> -    0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16)), /* movw r1 ... */ \
> -    0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
> -    0xe5801000 + (addr)
> -
> -struct ZynqMachineState {
> -    MachineState parent;
> -    Clock *ps_clk;
> -};
> +    cpu_to_le32(0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16))), /* movw r1 ... */ \
> +    cpu_to_le32(0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16))), /* movt r1 ... */ \

Looks like the callers all pass in constants, perhaps const_le32 should be used everywhere or am I missing something?


> +    const_le32(0xe5801000 + (addr))
>  
>  static void zynq_write_board_setup(ARMCPU *cpu,
>                                     const struct arm_boot_info *info)
>  {
> -    int n;
> -    uint32_t board_setup_blob[] = {
> -        0xe3a004f8, /* mov r0, #0xf8000000 */
> +    const uint32_t board_setup_blob[] = {
> +        const_le32(0xe3a004f8),         /* mov r0, #0xf8000000 */
>          SLCR_WRITE(SLCR_UNLOCK_OFFSET, SLCR_XILINX_UNLOCK_KEY),
>          SLCR_WRITE(SLCR_ARM_PLL_OFFSET, 0x00014008),
>          SLCR_WRITE(SLCR_LOCK_OFFSET, SLCR_XILINX_LOCK_KEY),
> -        0xe12fff1e, /* bx lr */
> +        const_le32(0xe12fff1e)          /* bx lr */
>      };
> -    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
> -        board_setup_blob[n] = tswap32(board_setup_blob[n]);
> -    }
> +
>      rom_add_blob_fixed("board-setup", board_setup_blob,
>                         sizeof(board_setup_blob), BOARD_SETUP_ADDR);
>  }
> -- 
> 2.38.1
> 

Re: [PATCH 5/6] hw/arm/xilinx_zynq: Remove tswap32() calls and constify smpboot[]

[Philippe Mathieu-Daudé]

On 23/12/22 04:54, Edgar E. Iglesias wrote:
> On Thu, Dec 22, 2022 at 10:55:48PM +0100, Philippe Mathieu-Daudé wrote:
>> ARM CPUs fetch instructions in little-endian.
>>
>> smpboot[] encoded instructions are written in little-endian.
>>
>> We call tswap32() on the array. tswap32 function swap a 32-bit
>> value if the target endianness doesn't match the host one.
>> Otherwise it is a NOP.
>>
>> * On a little-endian host, the array is stored as it. tswap32()
>>    is a NOP, and the vCPU fetches the instructions as it, in
>>    little-endian.
>>
>> * On a big-endian host, the array is stored as it. tswap32()
>>    swap the instructions to little-endian, and the vCPU fetches
>>    the instructions as it, in little-endian.
>>
>> Using tswap() on system emulation is a bit odd: while the target
>> particularities might change the system emulation, the host ones
>> (such its endianness) shouldn't interfere.
>>
>> We can simplify by using const_le32() to always store the
>> instructions in the array in little-endian, removing the need
>> for the dubious tswap().
> 
> 
> Hi Philippe,
> 
> 
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/xilinx_zynq.c | 27 ++++++++++++---------------
>>   1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> index 3190cc0b8d..4316143b71 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -71,6 +71,11 @@ static const int dma_irqs[8] = {
>>   
>>   #define ZYNQ_SDHCI_CAPABILITIES 0x69ec0080  /* Datasheet: UG585 (v1.12.1) */
>>   
>> +struct ZynqMachineState {
>> +    MachineState parent;
>> +    Clock *ps_clk;
>> +};
>> +
>>   #define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
>>                           extract32((x), 12,  4) << 16)
>>   
>> @@ -79,29 +84,21 @@ static const int dma_irqs[8] = {
>>    */
>>   
>>   #define SLCR_WRITE(addr, val) \
>> -    0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16)), /* movw r1 ... */ \
>> -    0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
>> -    0xe5801000 + (addr)
>> -
>> -struct ZynqMachineState {
>> -    MachineState parent;
>> -    Clock *ps_clk;
>> -};
>> +    cpu_to_le32(0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16))), /* movw r1 ... */ \
>> +    cpu_to_le32(0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16))), /* movt r1 ... */ \
> 
> Looks like the callers all pass in constants, perhaps const_le32 should be used everywhere or am I missing something?

extract32() is a function. I agree we can rewrite this macro to remove
it, I was simply lazy ;) I'll do for v2 so the array will be const.

> 
> 
>> +    const_le32(0xe5801000 + (addr))

Re: [PATCH 5/6] hw/arm/xilinx_zynq: Remove tswap32() calls and constify smpboot[]

[Philippe Mathieu-Daudé]

On 23/12/22 11:01, Philippe Mathieu-Daudé wrote:
> On 23/12/22 04:54, Edgar E. Iglesias wrote:
>> On Thu, Dec 22, 2022 at 10:55:48PM +0100, Philippe Mathieu-Daudé wrote:
>>> ARM CPUs fetch instructions in little-endian.
>>>
>>> smpboot[] encoded instructions are written in little-endian.
>>>
>>> We call tswap32() on the array. tswap32 function swap a 32-bit
>>> value if the target endianness doesn't match the host one.
>>> Otherwise it is a NOP.
>>>
>>> * On a little-endian host, the array is stored as it. tswap32()
>>>    is a NOP, and the vCPU fetches the instructions as it, in
>>>    little-endian.
>>>
>>> * On a big-endian host, the array is stored as it. tswap32()
>>>    swap the instructions to little-endian, and the vCPU fetches
>>>    the instructions as it, in little-endian.
>>>
>>> Using tswap() on system emulation is a bit odd: while the target
>>> particularities might change the system emulation, the host ones
>>> (such its endianness) shouldn't interfere.
>>>
>>> We can simplify by using const_le32() to always store the
>>> instructions in the array in little-endian, removing the need
>>> for the dubious tswap().
>>
>>
>> Hi Philippe,
>>
>>
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/arm/xilinx_zynq.c | 27 ++++++++++++---------------
>>>   1 file changed, 12 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>>> index 3190cc0b8d..4316143b71 100644
>>> --- a/hw/arm/xilinx_zynq.c
>>> +++ b/hw/arm/xilinx_zynq.c
>>> @@ -71,6 +71,11 @@ static const int dma_irqs[8] = {
>>>   #define ZYNQ_SDHCI_CAPABILITIES 0x69ec0080  /* Datasheet: UG585 
>>> (v1.12.1) */
>>> +struct ZynqMachineState {
>>> +    MachineState parent;
>>> +    Clock *ps_clk;
>>> +};
>>> +
>>>   #define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
>>>                           extract32((x), 12,  4) << 16)
>>> @@ -79,29 +84,21 @@ static const int dma_irqs[8] = {
>>>    */
>>>   #define SLCR_WRITE(addr, val) \
>>> -    0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16)), /* movw r1 
>>> ... */ \
>>> -    0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 
>>> ... */ \
>>> -    0xe5801000 + (addr)
>>> -
>>> -struct ZynqMachineState {
>>> -    MachineState parent;
>>> -    Clock *ps_clk;
>>> -};
>>> +    cpu_to_le32(0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16))), 
>>> /* movw r1 ... */ \
>>> +    cpu_to_le32(0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16))), 
>>> /* movt r1 ... */ \
>>
>> Looks like the callers all pass in constants, perhaps const_le32 
>> should be used everywhere or am I missing something?
> 
> extract32() is a function. I agree we can rewrite this macro to remove
> it, I was simply lazy ;) I'll do for v2 so the array will be const.

Well it is already runtime const, I meant 'static const' so it becomes
build-time const.

[PATCH 6/6] hw/arm/boot: Remove tswap32() calls and constify board_setup_blob[]

[Philippe Mathieu-Daudé]

ARM CPUs fetch instructions in little-endian.

board_setup_blob[] encoded instructions are written in little-endian.

We call tswap32() on the array. tswap32 function swap a 32-bit
value if the target endianness doesn't match the host one.
Otherwise it is a NOP.

* On a little-endian host, the array is stored as it. tswap32()
  is a NOP, and the vCPU fetches the instructions as it, in
  little-endian.

* On a big-endian host, the array is stored as it. tswap32()
  swap the instructions to little-endian, and the vCPU fetches
  the instructions as it, in little-endian.

Using tswap() on system emulation is a bit odd: while the target
particularities might change the system emulation, the host ones
(such its endianness) shouldn't interfere.

We can simplify by using const_le32() to always store the
instructions in the array in little-endian, removing the need
for the dubious tswap().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/boot.c | 52 ++++++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 3d7d11f782..22a100f19b 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -189,7 +189,7 @@ static void write_bootloader(const char *name, hwaddr addr,
         default:
             abort();
         }
-        code[i] = tswap32(insn);
+        code[i] = cpu_to_le32(insn);
     }
 
     assert((len * sizeof(uint32_t)) < BOOTLOADER_MAX_SIZE);
@@ -222,34 +222,33 @@ void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
                                             hwaddr mvbar_addr)
 {
     AddressSpace *as = arm_boot_address_space(cpu, info);
-    int n;
-    uint32_t mvbar_blob[] = {
+    static const uint32_t mvbar_blob[] = {
         /* mvbar_addr: secure monitor vectors
          * Default unimplemented and unused vectors to spin. Makes it
          * easier to debug (as opposed to the CPU running away).
          */
-        0xeafffffe, /* (spin) */
-        0xeafffffe, /* (spin) */
-        0xe1b0f00e, /* movs pc, lr ;SMC exception return */
-        0xeafffffe, /* (spin) */
-        0xeafffffe, /* (spin) */
-        0xeafffffe, /* (spin) */
-        0xeafffffe, /* (spin) */
-        0xeafffffe, /* (spin) */
+        const_le32(0xeafffffe), /* (spin) */
+        const_le32(0xeafffffe), /* (spin) */
+        const_le32(0xe1b0f00e), /* movs pc, lr ;SMC exception return */
+        const_le32(0xeafffffe), /* (spin) */
+        const_le32(0xeafffffe), /* (spin) */
+        const_le32(0xeafffffe), /* (spin) */
+        const_le32(0xeafffffe), /* (spin) */
+        const_le32(0xeafffffe)  /* (spin) */
     };
-    uint32_t board_setup_blob[] = {
+    const uint32_t board_setup_blob[] = {
         /* board setup addr */
-        0xee110f51, /* mrc     p15, 0, r0, c1, c1, 2  ;read NSACR */
-        0xe3800b03, /* orr     r0, #0xc00             ;set CP11, CP10 */
-        0xee010f51, /* mcr     p15, 0, r0, c1, c1, 2  ;write NSACR */
-        0xe3a00e00 + (mvbar_addr >> 4), /* mov r0, #mvbar_addr */
-        0xee0c0f30, /* mcr     p15, 0, r0, c12, c0, 1 ;set MVBAR */
-        0xee110f11, /* mrc     p15, 0, r0, c1 , c1, 0 ;read SCR */
-        0xe3800031, /* orr     r0, #0x31              ;enable AW, FW, NS */
-        0xee010f11, /* mcr     p15, 0, r0, c1, c1, 0  ;write SCR */
-        0xe1a0100e, /* mov     r1, lr                 ;save LR across SMC */
-        0xe1600070, /* smc     #0                     ;call monitor to flush SCR */
-        0xe1a0f001, /* mov     pc, r1                 ;return */
+        const_le32(0xee110f51), /* mrc  p15, 0, r0, c1, c1, 2  ;read NSACR */
+        const_le32(0xe3800b03), /* orr  r0, #0xc00             ;set CP11, CP10 */
+        const_le32(0xee010f51), /* mcr  p15, 0, r0, c1, c1, 2  ;write NSACR */
+        const_le32(0xe3a00e00 + (mvbar_addr >> 4)), /* mov r0, #mvbar_addr */
+        const_le32(0xee0c0f30), /* mcr  p15, 0, r0, c12, c0, 1 ;set MVBAR */
+        const_le32(0xee110f11), /* mrc  p15, 0, r0, c1 , c1, 0 ;read SCR */
+        const_le32(0xe3800031), /* orr  r0, #0x31              ;enable AW, FW, NS */
+        const_le32(0xee010f11), /* mcr  p15, 0, r0, c1, c1, 0  ;write SCR */
+        const_le32(0xe1a0100e), /* mov  r1, lr                 ;save LR across SMC */
+        const_le32(0xe1600070), /* smc  #0                     ;call monitor to flush SCR */
+        const_le32(0xe1a0f001)  /* mov  pc, r1                 ;return */
     };
 
     /* check that mvbar_addr is correctly aligned and relocatable (using MOV) */
@@ -259,15 +258,8 @@ void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
     assert((mvbar_addr + sizeof(mvbar_blob) <= info->board_setup_addr)
           || (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]);
-    }
     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]);
-    }
     rom_add_blob_fixed_as("board-setup", board_setup_blob,
                           sizeof(board_setup_blob), info->board_setup_addr, as);
 }
-- 
2.38.1

Re: [PATCH 0/6] hw/arm: Fix smpboot[] on big-endian hosts and remove tswap32()

[Philippe Mathieu-Daudé]

On 22/12/22 22:55, Philippe Mathieu-Daudé wrote:
> ARM CPUs fetch instructions in little-endian.
> 
> smpboot[] encoded instructions are written in little-endian.
> 
> We call tswap32() on the array. tswap32 function swap a 32-bit
> value if the target endianness doesn't match the host one.
> Otherwise it is a NOP.
> 
> * On a little-endian host, the array is stored as it. tswap32()
>    is a NOP, and the vCPU fetches the instructions as it, in
>    little-endian.
> 
> * On a big-endian host, the array is stored as it. tswap32()
>    swap the instructions to little-endian, and the vCPU fetches
>    the instructions as it, in little-endian.
> 
> Using tswap() on system emulation is a bit odd: while the target
> particularities might change the system emulation, the host ones
> (such its endianness) shouldn't interfere.
> 
> We can simplify by using const_le32() to always store the
> instructions in the array in little-endian, removing the need
> for the dubious tswap().
> 
> Two boards which weren't swapping (aspeed and raspi) are fixed.
> 
> Tested running ARM avocado tests on x86_64 and s390x.
> 
> Philippe Mathieu-Daudé (6):
>    hw/arm/aspeed: Fix smpboot[] on big-endian hosts
>    hw/arm/raspi: Fix smpboot[] on big-endian hosts
>    hw/arm/exynos4210: Remove tswap32() calls and constify smpboot[]
>    hw/arm/npcm7xx: Remove tswap32() calls and constify smpboot[]
>    hw/arm/xilinx_zynq: Remove tswap32() calls and constify smpboot[]
>    hw/arm/boot: Remove tswap32() calls and constify board_setup_blob[]
> 
>   hw/arm/aspeed.c      | 28 ++++++++++++------------
>   hw/arm/boot.c        | 52 +++++++++++++++++++-------------------------
>   hw/arm/exynos4210.c  | 48 ++++++++++++++++++----------------------
>   hw/arm/npcm7xx.c     | 49 +++++++++++++++++------------------------
>   hw/arm/raspi.c       | 46 +++++++++++++++++++--------------------
>   hw/arm/xilinx_zynq.c | 27 ++++++++++-------------
>   6 files changed, 112 insertions(+), 138 deletions(-)

I forgot to mention, checkpatch warns for 11 lines over 80chars, and
errors for 2 over 90chars. Since these are the assembler comments,
I choose to keep it that way for readability.

Re: [PATCH 0/6] hw/arm: Fix smpboot[] on big-endian hosts and remove tswap32()

[Richard Henderson]

On 12/22/22 13:55, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (6):
>    hw/arm/aspeed: Fix smpboot[] on big-endian hosts
>    hw/arm/raspi: Fix smpboot[] on big-endian hosts
>    hw/arm/exynos4210: Remove tswap32() calls and constify smpboot[]
>    hw/arm/npcm7xx: Remove tswap32() calls and constify smpboot[]
>    hw/arm/xilinx_zynq: Remove tswap32() calls and constify smpboot[]
>    hw/arm/boot: Remove tswap32() calls and constify board_setup_blob[]

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

r~

Re: [PATCH 0/6] hw/arm: Fix smpboot[] on big-endian hosts and remove tswap32()

[Peter Maydell]

On Thu, 22 Dec 2022 at 21:55, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> ARM CPUs fetch instructions in little-endian.
>
> smpboot[] encoded instructions are written in little-endian.
>
> We call tswap32() on the array. tswap32 function swap a 32-bit
> value if the target endianness doesn't match the host one.
> Otherwise it is a NOP.
>
> * On a little-endian host, the array is stored as it. tswap32()
>   is a NOP, and the vCPU fetches the instructions as it, in
>   little-endian.
>
> * On a big-endian host, the array is stored as it. tswap32()
>   swap the instructions to little-endian, and the vCPU fetches
>   the instructions as it, in little-endian.
>
> Using tswap() on system emulation is a bit odd: while the target
> particularities might change the system emulation, the host ones
> (such its endianness) shouldn't interfere.
>
> We can simplify by using const_le32() to always store the
> instructions in the array in little-endian, removing the need
> for the dubious tswap().

The tswap() in boot.c is not dubious at all. We start
with a 32-bit value in host order (i.e. a C constant),
and we want a value in guest order so we can write it
into memory as a byte array. The correct function for that
task is tswap()...

-- PMM

Re: [PATCH 0/6] hw/arm: Fix smpboot[] on big-endian hosts and remove tswap32()

[Philippe Mathieu-Daudé]

On 3/1/23 18:43, Peter Maydell wrote:
> On Thu, 22 Dec 2022 at 21:55, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> ARM CPUs fetch instructions in little-endian.
>>
>> smpboot[] encoded instructions are written in little-endian.
>>
>> We call tswap32() on the array. tswap32 function swap a 32-bit
>> value if the target endianness doesn't match the host one.
>> Otherwise it is a NOP.
>>
>> * On a little-endian host, the array is stored as it. tswap32()
>>    is a NOP, and the vCPU fetches the instructions as it, in
>>    little-endian.
>>
>> * On a big-endian host, the array is stored as it. tswap32()
>>    swap the instructions to little-endian, and the vCPU fetches
>>    the instructions as it, in little-endian.
>>
>> Using tswap() on system emulation is a bit odd: while the target
>> particularities might change the system emulation, the host ones
>> (such its endianness) shouldn't interfere.
>>
>> We can simplify by using const_le32() to always store the
>> instructions in the array in little-endian, removing the need
>> for the dubious tswap().
> 
> The tswap() in boot.c is not dubious at all. We start
> with a 32-bit value in host order (i.e. a C constant),
> and we want a value in guest order so we can write it
> into memory as a byte array. The correct function for that
> task is tswap()...

Maybe 'dubious' is a strong word inappropriate here. What I meant
is tswap() forces extra reasoning "oh, on what endianness will I
run this, what will happens then, is tswap() a NOP?". When using
the const_le32() macro we knows the 32-bit values are already in
little-endian order in the host memory, regardless of its
endianness. This is convenient with ARM guests which load their
instructions in this endianness, not need to tswap() at all.

I'll try to reword the commit descriptions in some clearer way.

Thanks,

Phil.