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

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
> 

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))

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.