[Qemu-devel] [PATCH] tcg/tcg-op.c: Fix ld/st of 64 bit values on 32-bit bigendian hosts

[Qemu-devel] [PATCH] tcg/tcg-op.c: Fix ld/st of 64 bit values on 32-bit bigendian hosts

[Peter Maydell]

Commit 951c6300f7 out-of-lined the 32-bit-host versions of
tcg_gen_{ld,st}_i64, but in the process it inadvertently changed
an #ifdef HOST_WORDS_BIGENDIAN to #ifdef TCG_TARGET_WORDS_BIGENDIAN.
Since the latter doesn't get defined anywhere this meant we always
took the "LE host" codepath, and stored the two halves of the value
in the wrong order on BE hosts. This typically breaks any 64-bit
guest on a 32-bit BE host completely, and will have possibly more
subtle effects even for 32-bit guests.

Switch the ifdef back to HOST_WORDS_BIGENDIAN.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I checked that with this fix we no longer fail catastrophically
on the first insn of the x86-64 BIOS image, but since I don't have
a convenient way to display the graphical screen from the PPC
box I have access to I didn't check that it continued to do
sensible things thereafter. Andreas, could you confirm this fixes
the breakage you're seeing?

 tcg/tcg-op.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index f7a2767..2b6be75 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -808,7 +808,7 @@ void tcg_gen_ld_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
 {
     /* Since arg2 and ret have different types,
        they cannot be the same temporary */
-#ifdef TCG_TARGET_WORDS_BIGENDIAN
+#ifdef HOST_WORDS_BIGENDIAN
     tcg_gen_ld_i32(TCGV_HIGH(ret), arg2, offset);
     tcg_gen_ld_i32(TCGV_LOW(ret), arg2, offset + 4);
 #else
@@ -819,7 +819,7 @@ void tcg_gen_ld_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
 
 void tcg_gen_st_i64(TCGv_i64 arg1, TCGv_ptr arg2, tcg_target_long offset)
 {
-#ifdef TCG_TARGET_WORDS_BIGENDIAN
+#ifdef HOST_WORDS_BIGENDIAN
     tcg_gen_st_i32(TCGV_HIGH(arg1), arg2, offset);
     tcg_gen_st_i32(TCGV_LOW(arg1), arg2, offset + 4);
 #else
-- 
1.9.1

Re: [Qemu-devel] [PATCH] tcg/tcg-op.c: Fix ld/st of 64 bit values on 32-bit bigendian hosts

[Andreas Färber]

Am 08.04.2015 um 21:57 schrieb Peter Maydell:
> Commit 951c6300f7 out-of-lined the 32-bit-host versions of
> tcg_gen_{ld,st}_i64, but in the process it inadvertently changed
> an #ifdef HOST_WORDS_BIGENDIAN to #ifdef TCG_TARGET_WORDS_BIGENDIAN.
> Since the latter doesn't get defined anywhere this meant we always
> took the "LE host" codepath, and stored the two halves of the value
> in the wrong order on BE hosts. This typically breaks any 64-bit
> guest on a 32-bit BE host completely, and will have possibly more
> subtle effects even for 32-bit guests.
> 
> Switch the ifdef back to HOST_WORDS_BIGENDIAN.

Doh, not the first time we've screwed up such an ifdef...

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I checked that with this fix we no longer fail catastrophically
> on the first insn of the x86-64 BIOS image, but since I don't have
> a convenient way to display the graphical screen from the PPC
> box I have access to I didn't check that it continued to do
> sensible things thereafter. Andreas, could you confirm this fixes
> the breakage you're seeing?

It fixes the observed make check breakage on ppc. i586 still works, too.

Tested-by: Andreas Färber <afaerber@suse.de>

I'll check VNC tomorrow. I'm assuming this is for-2.3 material.

Thanks a lot,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] tcg/tcg-op.c: Fix ld/st of 64 bit values on 32-bit bigendian hosts

[Richard Henderson]

On 04/08/2015 12:57 PM, Peter Maydell wrote:
> Switch the ifdef back to HOST_WORDS_BIGENDIAN.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Doh.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] tcg/tcg-op.c: Fix ld/st of 64 bit values on 32-bit bigendian hosts

[Peter Maydell]

On 9 April 2015 at 02:20, Richard Henderson <rth@twiddle.net> wrote:
> On 04/08/2015 12:57 PM, Peter Maydell wrote:
>> Switch the ifdef back to HOST_WORDS_BIGENDIAN.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Doh.
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Applied to master, thanks.

-- PMM