[PATCH 1/2] powerpc: Fix emulation of mcrf in emulate_step()

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 1/2] powerpc: Fix emulation of mcrf in emulate_step()

Anton Blanchard-3
From: Anton Blanchard <[hidden email]>

The mcrf emulation code was looking at the CR fields in the reverse
order. It also relied on reserved fields being zero which is somewhat
fragile, so fix that too.

Cc: [hidden email]
Signed-off-by: Anton Blanchard <[hidden email]>
---
 arch/powerpc/lib/sstep.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 33117f8a0882..fb84f51b1f0b 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -683,8 +683,10 @@ int analyse_instr(struct instruction_op *op, struct pt_regs *regs,
  case 19:
  switch ((instr >> 1) & 0x3ff) {
  case 0: /* mcrf */
- rd = (instr >> 21) & 0x1c;
- ra = (instr >> 16) & 0x1c;
+ rd = 7 - ((instr >> 23) & 0x7);
+ ra = 7 - ((instr >> 18) & 0x7);
+ rd *= 4;
+ ra *= 4;
  val = (regs->ccr >> ra) & 0xf;
  regs->ccr = (regs->ccr & ~(0xfUL << rd)) | (val << rd);
  goto instr_done;
--
2.11.0

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 2/2] powerpc: Fix emulation of mfocrf in emulate_step()

Anton Blanchard-3
From: Anton Blanchard <[hidden email]>

From POWER4 onwards, mfocrf() only places the specified CR field into
the destination GPR, and the rest of it is set to 0. The PowerPC AS
from version 3.0 now requires this behaviour.

The emulation code currently puts the entire CR into the destination GPR.
Fix it.

Cc: [hidden email]
Signed-off-by: Anton Blanchard <[hidden email]>
---
 arch/powerpc/lib/sstep.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index fb84f51b1f0b..ee33327686ae 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -966,6 +966,19 @@ int analyse_instr(struct instruction_op *op, struct pt_regs *regs,
 #endif
 
  case 19: /* mfcr */
+ if ((instr >> 20) & 1) {
+ imm = 0xf0000000UL;
+ for (sh = 0; sh < 8; ++sh) {
+ if (instr & (0x80000 >> sh)) {
+ regs->gpr[rd] = regs->ccr & imm;
+ break;
+ }
+ imm >>= 4;
+ }
+
+ goto instr_done;
+ }
+
  regs->gpr[rd] = regs->ccr;
  regs->gpr[rd] &= 0xffffffffUL;
  goto instr_done;
--
2.11.0

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/2] powerpc: Fix emulation of mcrf in emulate_step()

Segher Boessenkool
In reply to this post by Anton Blanchard-3
Hi Anton,

On Thu, Jun 15, 2017 at 09:46:38AM +1000, Anton Blanchard wrote:
> The mcrf emulation code was looking at the CR fields in the reverse
> order. It also relied on reserved fields being zero which is somewhat
> fragile, so fix that too.

It masked out the reserved bits.  I find the new code to be less
readable (but also more correct ;-) ).  Maybe there should be (inline)
helper function to insert/extract CR fields?


Segher


> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -683,8 +683,10 @@ int analyse_instr(struct instruction_op *op, struct pt_regs *regs,
>   case 19:
>   switch ((instr >> 1) & 0x3ff) {
>   case 0: /* mcrf */
> - rd = (instr >> 21) & 0x1c;
> - ra = (instr >> 16) & 0x1c;
> + rd = 7 - ((instr >> 23) & 0x7);
> + ra = 7 - ((instr >> 18) & 0x7);
> + rd *= 4;
> + ra *= 4;
>   val = (regs->ccr >> ra) & 0xf;
>   regs->ccr = (regs->ccr & ~(0xfUL << rd)) | (val << rd);
>   goto instr_done;
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/2] powerpc: Fix emulation of mcrf in emulate_step()

Anton Blanchard
Hi Segher,

> On Thu, Jun 15, 2017 at 09:46:38AM +1000, Anton Blanchard wrote:
> > The mcrf emulation code was looking at the CR fields in the reverse
> > order. It also relied on reserved fields being zero which is
> > somewhat fragile, so fix that too.  
>
> It masked out the reserved bits.  I find the new code to be less
> readable (but also more correct ;-) ).

Thanks for that, not sure how I missed it :) I'll respin a simpler
patch.

> Maybe there should be (inline)
> helper function to insert/extract CR fields?

That would be nice, there are quite a few places that could use it.

Anton

> Segher
>
>
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -683,8 +683,10 @@ int analyse_instr(struct instruction_op *op,
> > struct pt_regs *regs, case 19:
> >   switch ((instr >> 1) & 0x3ff) {
> >   case 0: /* mcrf */
> > - rd = (instr >> 21) & 0x1c;
> > - ra = (instr >> 16) & 0x1c;
> > + rd = 7 - ((instr >> 23) & 0x7);
> > + ra = 7 - ((instr >> 18) & 0x7);
> > + rd *= 4;
> > + ra *= 4;
> >   val = (regs->ccr >> ra) & 0xf;
> >   regs->ccr = (regs->ccr & ~(0xfUL << rd)) |
> > (val << rd); goto instr_done;  
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/2] powerpc: Fix emulation of mcrf in emulate_step()

Segher Boessenkool
On Thu, Jun 15, 2017 at 04:47:29PM +1000, Anton Blanchard wrote:
> > Maybe there should be (inline)
> > helper function to insert/extract CR fields?
>
> That would be nice, there are quite a few places that could use it.

Like patch 2/2, hint hint.


Segher
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/2] powerpc: Fix emulation of mcrf in emulate_step()

Naveen N. Rao
In reply to this post by Anton Blanchard-3
On 2017/06/15 09:46AM, Anton Blanchard wrote:
> From: Anton Blanchard <[hidden email]>
>
> The mcrf emulation code was looking at the CR fields in the reverse
> order. It also relied on reserved fields being zero which is somewhat
> fragile, so fix that too.

Yikes! Amazing catch!
Acked-by: Naveen N. Rao <[hidden email]>

Thanks,
Naveen

>
> Cc: [hidden email]
> Signed-off-by: Anton Blanchard <[hidden email]>
> ---
>  arch/powerpc/lib/sstep.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 33117f8a0882..fb84f51b1f0b 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -683,8 +683,10 @@ int analyse_instr(struct instruction_op *op, struct pt_regs *regs,
>   case 19:
>   switch ((instr >> 1) & 0x3ff) {
>   case 0: /* mcrf */
> - rd = (instr >> 21) & 0x1c;
> - ra = (instr >> 16) & 0x1c;
> + rd = 7 - ((instr >> 23) & 0x7);
> + ra = 7 - ((instr >> 18) & 0x7);
> + rd *= 4;
> + ra *= 4;
>   val = (regs->ccr >> ra) & 0xf;
>   regs->ccr = (regs->ccr & ~(0xfUL << rd)) | (val << rd);
>   goto instr_done;
> --
> 2.11.0
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 2/2] powerpc: Fix emulation of mfocrf in emulate_step()

Naveen N. Rao
In reply to this post by Anton Blanchard-3
On 2017/06/15 09:46AM, Anton Blanchard wrote:

> From: Anton Blanchard <[hidden email]>
>
> From POWER4 onwards, mfocrf() only places the specified CR field into
> the destination GPR, and the rest of it is set to 0. The PowerPC AS
> from version 3.0 now requires this behaviour.
>
> The emulation code currently puts the entire CR into the destination GPR.
> Fix it.
>
> Cc: [hidden email]
> Signed-off-by: Anton Blanchard <[hidden email]>

Acked-by: Naveen N. Rao <[hidden email]>

> ---
>  arch/powerpc/lib/sstep.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index fb84f51b1f0b..ee33327686ae 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -966,6 +966,19 @@ int analyse_instr(struct instruction_op *op, struct pt_regs *regs,
>  #endif
>
>   case 19: /* mfcr */
> + if ((instr >> 20) & 1) {
> + imm = 0xf0000000UL;
> + for (sh = 0; sh < 8; ++sh) {
> + if (instr & (0x80000 >> sh)) {
> + regs->gpr[rd] = regs->ccr & imm;
> + break;
> + }
> + imm >>= 4;
> + }
> +
> + goto instr_done;
> + }
> +
>   regs->gpr[rd] = regs->ccr;
>   regs->gpr[rd] &= 0xffffffffUL;
>   goto instr_done;
> --
> 2.11.0
>

Loading...