* [Qemu-devel] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c [not found] <CA+mWYFvkiGGexDEUpD9KXiBYFVWQAXDjVODeX=miM9udQtObDw@mail.gmail.com> @ 2014-06-11 8:01 ` Sorav Bansal 2014-06-11 11:54 ` [Qemu-devel] [Qemu-ppc] " Tom Musta 0 siblings, 1 reply; 6+ messages in thread From: Sorav Bansal @ 2014-06-11 8:01 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-ppc, Alexander Graf [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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c 2014-06-11 8:01 ` [Qemu-devel] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c Sorav Bansal @ 2014-06-11 11:54 ` Tom Musta 2014-06-11 14:23 ` Sorav Bansal 0 siblings, 1 reply; 6+ messages in thread From: Tom Musta @ 2014-06-11 11:54 UTC (permalink / raw) To: Sorav Bansal, qemu-devel; +Cc: qemu-ppc 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c 2014-06-11 11:54 ` [Qemu-devel] [Qemu-ppc] " Tom Musta @ 2014-06-11 14:23 ` Sorav Bansal 2014-06-11 18:24 ` Tom Musta 0 siblings, 1 reply; 6+ messages in thread From: Sorav Bansal @ 2014-06-11 14:23 UTC (permalink / raw) To: Tom Musta; +Cc: qemu-ppc, qemu-devel > 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c 2014-06-11 14:23 ` Sorav Bansal @ 2014-06-11 18:24 ` Tom Musta 2014-06-12 3:11 ` Sorav Bansal 0 siblings, 1 reply; 6+ messages in thread From: Tom Musta @ 2014-06-11 18:24 UTC (permalink / raw) To: Sorav Bansal; +Cc: qemu-ppc, qemu-devel, Alexander Graf 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c 2014-06-11 18:24 ` Tom Musta @ 2014-06-12 3:11 ` Sorav Bansal 2014-06-13 14:59 ` Alexander Graf 0 siblings, 1 reply; 6+ messages in thread From: Sorav Bansal @ 2014-06-12 3:11 UTC (permalink / raw) To: Tom Musta; +Cc: qemu-ppc, qemu-devel, Alexander Graf > 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> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c 2014-06-12 3:11 ` Sorav Bansal @ 2014-06-13 14:59 ` Alexander Graf 0 siblings, 0 replies; 6+ messages in thread From: Alexander Graf @ 2014-06-13 14:59 UTC (permalink / raw) To: Sorav Bansal, Tom Musta; +Cc: qemu-ppc, qemu-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-13 14:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CA+mWYFvkiGGexDEUpD9KXiBYFVWQAXDjVODeX=miM9udQtObDw@mail.gmail.com>
2014-06-11 8:01 ` [Qemu-devel] Fwd: Patch: fix to gen_mcrxr() in target-ppc/translate.c Sorav Bansal
2014-06-11 11:54 ` [Qemu-devel] [Qemu-ppc] " Tom Musta
2014-06-11 14:23 ` Sorav Bansal
2014-06-11 18:24 ` Tom Musta
2014-06-12 3:11 ` Sorav Bansal
2014-06-13 14:59 ` Alexander Graf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).