| Message ID | 20220418191100.270334-3-leandro.lupori@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | Port PPC64/PowerNV MMU tests to QEMU | expand |
On 4/18/22 12:10, Leandro Lupori wrote: > +static inline uint64_t sh_swap64(CPUArchState *env, uint64_t val) > +{ > + return msr_le ? val : tswap64(val); > +} > + > +static inline uint32_t sh_swap32(CPUArchState *env, uint32_t val) > +{ > + return msr_le ? val : tswap32(val); > +} That doesn't work -- tswap itself is conditional. You want return msr_le ? le32_to_cpu(x) : be32_to_cpu(x); etc. One of them will be a nop, and the other will bswap. r~
On 4/18/22 20:36, Richard Henderson wrote: > [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você > possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de > e-mail suspeito entre imediatamente em contato com o DTI. > > On 4/18/22 12:10, Leandro Lupori wrote: >> +static inline uint64_t sh_swap64(CPUArchState *env, uint64_t val) >> +{ >> + return msr_le ? val : tswap64(val); >> +} >> + >> +static inline uint32_t sh_swap32(CPUArchState *env, uint32_t val) >> +{ >> + return msr_le ? val : tswap32(val); >> +} > > That doesn't work -- tswap itself is conditional. > > You want > > return msr_le ? le32_to_cpu(x) : be32_to_cpu(x); > > etc. One of them will be a nop, and the other will bswap. > Right, I'll make this change. Thanks, Leandro > > r~
On Mon, 18 Apr 2022 at 20:19, Leandro Lupori <leandro.lupori@eldorado.org.br> wrote: > > PPC64 CPUs can change its endian dynamically, so semihosting code > must check its MSR at run time to determine if byte swapping is > needed. Arm CPUs also change endianness dynamically, so why is this change PPC-specific ? thanks -- PMM
On 4/20/22 12:42, Peter Maydell wrote: > On Mon, 18 Apr 2022 at 20:19, Leandro Lupori > <leandro.lupori@eldorado.org.br> wrote: >> >> PPC64 CPUs can change its endian dynamically, so semihosting code >> must check its MSR at run time to determine if byte swapping is >> needed. > > Arm CPUs also change endianness dynamically, so why is this > change PPC-specific ? I'm reasonably certain that we simply don't test armbe or aarch64_be semihosting. Leandro found this because qemu-system-ppc64 defaults to BE and qemu-system-aarch64 defaults to LE. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 4/20/22 12:42, Peter Maydell wrote: >> On Mon, 18 Apr 2022 at 20:19, Leandro Lupori >> <leandro.lupori@eldorado.org.br> wrote: >>> >>> PPC64 CPUs can change its endian dynamically, so semihosting code >>> must check its MSR at run time to determine if byte swapping is >>> needed. >> Arm CPUs also change endianness dynamically, so why is this >> change PPC-specific ? > > I'm reasonably certain that we simply don't test armbe or aarch64_be > semihosting. Leandro found this because qemu-system-ppc64 defaults to > BE and qemu-system-aarch64 defaults to LE. Maybe it is time to have a generic endianess variable in CPUState so we can avoid having arch specific hacks in the semihosting code. That said is endianess binary? I seem to recall on ARM the instruction stream is always in one endianess so it only really affects CPU data loads and stores. Is it the same for PPC? > > > r~
On Wed, 20 Apr 2022 at 20:52, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/20/22 12:42, Peter Maydell wrote: > > On Mon, 18 Apr 2022 at 20:19, Leandro Lupori > > <leandro.lupori@eldorado.org.br> wrote: > >> > >> PPC64 CPUs can change its endian dynamically, so semihosting code > >> must check its MSR at run time to determine if byte swapping is > >> needed. > > > > Arm CPUs also change endianness dynamically, so why is this > > change PPC-specific ? > > I'm reasonably certain that we simply don't test armbe or aarch64_be semihosting. Leandro > found this because qemu-system-ppc64 defaults to BE and qemu-system-aarch64 defaults to LE. Right, so if there's an existing bug here on arm we should fix that, and that probably means that the abstraction split between "arch-specific thing" and "non-arch-specific code" is different from "PPC just overrides the entire swap function". -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 20 Apr 2022 at 20:52, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 4/20/22 12:42, Peter Maydell wrote: >> > On Mon, 18 Apr 2022 at 20:19, Leandro Lupori >> > <leandro.lupori@eldorado.org.br> wrote: >> >> >> >> PPC64 CPUs can change its endian dynamically, so semihosting code >> >> must check its MSR at run time to determine if byte swapping is >> >> needed. >> > >> > Arm CPUs also change endianness dynamically, so why is this >> > change PPC-specific ? >> >> I'm reasonably certain that we simply don't test armbe or aarch64_be semihosting. Leandro >> found this because qemu-system-ppc64 defaults to BE and qemu-system-aarch64 defaults to LE. > > Right, so if there's an existing bug here on arm we should fix that, > and that probably means that the abstraction split between > "arch-specific thing" and "non-arch-specific code" is different > from "PPC just overrides the entire swap function". I think the helper function cpu_virtio_is_big_endian is really just a proxy for the data endianess mode of the guest. Perhaps it could be re-named and then used by the semihosting code?
On Thu, 21 Apr 2022 at 10:44, Alex Bennée <alex.bennee@linaro.org> wrote: > I think the helper function cpu_virtio_is_big_endian is really just a > proxy for the data endianess mode of the guest. Perhaps it could be > re-named and then used by the semihosting code? We specifically named and documented that as "don't use this unless you're the silly legacy virtio devices": /** * @virtio_is_big_endian: Callback to return %true if a CPU which supports * runtime configurable endianness is currently big-endian. * Non-configurable CPUs can use the default implementation of this method. * This method should not be used by any callers other than the pre-1.0 * virtio devices. */ I think you're correct that it is also the right thing for semihosting, but we should be a bit careful with the naming and commenting so we can retain the "this is the wrong thing for most situations and definitely not something to be calling in device model code" information for developers and code reviewers. -- PMM
diff --git a/include/exec/softmmu-semi.h b/include/exec/softmmu-semi.h index fbcae88f4b..723aa2f58a 100644 --- a/include/exec/softmmu-semi.h +++ b/include/exec/softmmu-semi.h @@ -12,12 +12,27 @@ #include "cpu.h" +#ifdef TARGET_PPC64 +static inline uint64_t sh_swap64(CPUArchState *env, uint64_t val) +{ + return msr_le ? val : tswap64(val); +} + +static inline uint32_t sh_swap32(CPUArchState *env, uint32_t val) +{ + return msr_le ? val : tswap32(val); +} +#else +#define sh_swap64(env, val) tswap64(val) +#define sh_swap32(env, val) tswap32(val) +#endif + static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr) { uint64_t val; cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 8, 0); - return tswap64(val); + return sh_swap64(env, val); } static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr) @@ -25,7 +40,7 @@ static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr) uint32_t val; cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 4, 0); - return tswap32(val); + return sh_swap32(env, val); } static inline uint32_t softmmu_tget8(CPUArchState *env, target_ulong addr) @@ -44,14 +59,14 @@ static inline uint32_t softmmu_tget8(CPUArchState *env, target_ulong addr) static inline void softmmu_tput64(CPUArchState *env, target_ulong addr, uint64_t val) { - val = tswap64(val); + val = sh_swap64(env, val); cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 8, 1); } static inline void softmmu_tput32(CPUArchState *env, target_ulong addr, uint32_t val) { - val = tswap32(val); + val = sh_swap32(env, val); cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 4, 1); } #define put_user_u64(arg, p) ({ softmmu_tput64(env, p, arg) ; 0; })
PPC64 CPUs can change its endian dynamically, so semihosting code must check its MSR at run time to determine if byte swapping is needed. Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> --- include/exec/softmmu-semi.h | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)