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

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

[Sorav Bansal]

[I had posted to qemu-ppc list earlier, reposting to qemu-devel also
on being suggested to do so. Also adding some description about the
patch]

Hi,

I saw a minor bug in the gen_mcrxr() function in
target-ppc/translate.c. Here is the patch for your consideration. I
have verified the patch by checking the generated code using a
SAT-solver based verification tool.

thanks,
Sorav

Patch Description:
As I see it, the XER[SO], XER[OV], and XER[CA] flags are stored in the
least significant bit (bit 0) of their respective registers. They need
to be shifted left (by their respective offsets) to generate the final
XER value. The old code seemed to be assuming that  the flags are
stored in bit 2, and was shifting them right (by their respective
offsets), which seems incorrect.

>From 31f39e258cbb289c2e0a3c3adde87cde7ae02a15 Mon Sep 17 00:00:00 2001
From: Sorav Bansal <sbansal@cse.iitd.ernet.in>
Date: Tue, 10 Jun 2014 19:01:12 +0530
Subject: [PATCH] Fix to the translation of mcrxr instruction

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

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index f089014..b513998 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4147,8 +4147,8 @@ 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(dst, dst, 2);
+    tcg_gen_shli_i32(t1, t1, 1);
     tcg_gen_or_i32(dst, dst, t0);
     tcg_gen_or_i32(dst, dst, t1);
     tcg_temp_free_i32(t0);
--
1.7.9.5

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

[Tom Musta]

On 6/11/2014 3:01 AM, Sorav Bansal wrote:
> [I had posted to qemu-ppc list earlier, reposting to qemu-devel also
> on being suggested to do so. Also adding some description about the
> patch]
> 
> Hi,
> 
> I saw a minor bug in the gen_mcrxr() function in
> target-ppc/translate.c. Here is the patch for your consideration. I
> have verified the patch by checking the generated code using a
> SAT-solver based verification tool.
> 
> thanks,
> Sorav
> 
> Patch Description:
> As I see it, the XER[SO], XER[OV], and XER[CA] flags are stored in the
> least significant bit (bit 0) of their respective registers. They need
> to be shifted left (by their respective offsets) to generate the final
> XER value. The old code seemed to be assuming that  the flags are
> stored in bit 2, and was shifting them right (by their respective
> offsets), which seems incorrect.
> 
> From 31f39e258cbb289c2e0a3c3adde87cde7ae02a15 Mon Sep 17 00:00:00 2001
> From: Sorav Bansal <sbansal@cse.iitd.ernet.in>
> Date: Tue, 10 Jun 2014 19:01:12 +0530
> Subject: [PATCH] Fix to the translation of mcrxr instruction
> 
> ---
>  target-ppc/translate.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index f089014..b513998 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4147,8 +4147,8 @@ 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(dst, dst, 2);
> +    tcg_gen_shli_i32(t1, t1, 1);
>      tcg_gen_or_i32(dst, dst, t0);
>      tcg_gen_or_i32(dst, dst, t1);
>      tcg_temp_free_i32(t0);
> --
> 1.7.9.5
> 

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.

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

[Sorav Bansal]

> 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);
-- 
1.7.9.5

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

[Tom Musta]

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>

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

[Sorav Bansal]

> Your patch is missing the signoff.

Here is the patch with the signoff. thanks.

>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);
-- 
1.7.9.5

Signed-off-by: Sorav Bansal <sbansal@cse.iitd.ernet.in>

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

[Alexander Graf]

On 12.06.14 05:11, Sorav Bansal wrote:
>> Your patch is missing the signoff.
> Here is the patch with the signoff. thanks.
>
>  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

Please resend as a proper top level patch (not an inline reply) and with 
the SoB line as part of the patch description. Also please include a 
proper description of what the patch does, why it does it and how the 
old behavior was broken.

   http://wiki.qemu.org/Contribute/SubmitAPatch


Thanks a lot for your time and effort at fixing this issue :)

Alex