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

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

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

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

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

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

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

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;

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]

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()

[Naveen N. Rao]

On 2017/06/15 09:46AM, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> 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 <naveen.n.rao@linux.vnet.ibm.com>

Thanks,
Naveen

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>  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 2/2] powerpc: Fix emulation of mfocrf in emulate_step()

[Naveen N. Rao]

On 2017/06/15 09:46AM, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Anton Blanchard <anton@samba.org>

Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

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

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

[Michael Ellerman]

On Wed, 2017-06-14 at 23:46:38 UTC, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/87c4b83e0fe234a1f0eed131ab6fa2

cheers

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

[Michael Ellerman]

On Wed, 2017-06-14 at 23:46:39 UTC, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> >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: stable@vger.kernel.org
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/64e756c55aa46fc18fd53e8f3598b7

cheers