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

[Naveen N. Rao]

On 2017/06/15 09:46AM, Anton Blanchard wrote:
> From: Anton Blanchard 
> 
> 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 

Thanks,
Naveen

> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Anton Blanchard 
> ---
>  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
> 

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

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

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

[Segher Boessenkool]

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;

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

[Anton Blanchard]

From: Anton Blanchard 

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: sta...@vger.kernel.org
Signed-off-by: Anton Blanchard 
---
 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