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