diff mbox

[Qemu-ppc] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c

Message ID CA+mWYFtsim=WXucQ9eNFW+Vqzhx+7k0mGqTZdNg5ELYjx=rJ5g@mail.gmail.com
State New
Headers show

Commit Message

Sorav Bansal June 11, 2014, 2:23 p.m. UTC
> Please read my comments again.  I agree that SO, OV and CA are stored in the LSB of their internal QEMU representation.  The problem is that, even with your patch, these bits are not being correctly shifted into the target four bit CR field.
>

Ah, I forgot that PPC spec starts bit numbering at the MSB. Here is
the revised patch.


From da0a962a6d14fe699ebb7cc12450c7de9553b66a Mon Sep 17 00:00:00 2001
From: Sorav Bansal <sbansal@cse.iitd.ernet.in>
Date: Wed, 11 Jun 2014 19:49:49 +0530
Subject: [PATCH] Fixed the translation of the mcrxr ppc instruction

---
 target-ppc/translate.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Tom Musta June 11, 2014, 6:24 p.m. UTC | #1
On 6/11/2014 9:23 AM, Sorav Bansal wrote:
>> Please read my comments again.  I agree that SO, OV and CA are stored in the LSB of their internal QEMU representation.  The problem is that, even with your patch, these bits are not being correctly shifted into the target four bit CR field.
>>
> 
> Ah, I forgot that PPC spec starts bit numbering at the MSB. Here is
> the revised patch.
> 
> 
> From da0a962a6d14fe699ebb7cc12450c7de9553b66a Mon Sep 17 00:00:00 2001
> From: Sorav Bansal <sbansal@cse.iitd.ernet.in>
> Date: Wed, 11 Jun 2014 19:49:49 +0530
> Subject: [PATCH] Fixed the translation of the mcrxr ppc instruction
> 
> ---
>  target-ppc/translate.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index f089014..59a92b9 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4147,8 +4147,9 @@ static void gen_mcrxr(DisasContext *ctx)
>      tcg_gen_trunc_tl_i32(t0, cpu_so);
>      tcg_gen_trunc_tl_i32(t1, cpu_ov);
>      tcg_gen_trunc_tl_i32(dst, cpu_ca);
> -    tcg_gen_shri_i32(t0, t0, 2);
> -    tcg_gen_shri_i32(t1, t1, 1);
> +    tcg_gen_shli_i32(t0, t0, 3);
> +    tcg_gen_shli_i32(t1, t1, 2);
> +    tcg_gen_shli_i32(dst, dst, 1);
>      tcg_gen_or_i32(dst, dst, t0);
>      tcg_gen_or_i32(dst, dst, t1);
>      tcg_temp_free_i32(t0);
> 

Your patch is missing the signoff.

Other than that, this does properly shift into the CR field.  My other comments about using deposit and lack of handling XER[35] still stand, but I don't believe your patch makes things worse.

Reviewed-by: Tom Musta <tommusta@gmail.com>
diff mbox

Patch

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index f089014..59a92b9 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4147,8 +4147,9 @@  static void gen_mcrxr(DisasContext *ctx)
     tcg_gen_trunc_tl_i32(t0, cpu_so);
     tcg_gen_trunc_tl_i32(t1, cpu_ov);
     tcg_gen_trunc_tl_i32(dst, cpu_ca);
-    tcg_gen_shri_i32(t0, t0, 2);
-    tcg_gen_shri_i32(t1, t1, 1);
+    tcg_gen_shli_i32(t0, t0, 3);
+    tcg_gen_shli_i32(t1, t1, 2);
+    tcg_gen_shli_i32(dst, dst, 1);
     tcg_gen_or_i32(dst, dst, t0);
     tcg_gen_or_i32(dst, dst, t1);
     tcg_temp_free_i32(t0);