| Message ID | 20221222215549.86872-6-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: Fix smpboot[] on big-endian hosts and remove tswap32() | expand |
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.
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); }
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(-)