[PATCH 0/2] powerpc/math-emu: two patches for the emulation of the FPU unimplemented instructions

[PATCH 0/2] powerpc/math-emu: two patches for the emulation of the FPU unimplemented instructions

[Kevin Hao]

These two patches are for the emulation of the FPU unimplemented instructions.
The first is just a bit optimization for the FPU state flush.
The second is requested by Scott.

Kevin Hao (2):
  powerpc/math-emu: move the flush FPU state function into do_mathemu
  powerpc/math-emu: keep track of the instructions unimplemented by FPU

 arch/powerpc/kernel/traps.c  |  9 --------
 arch/powerpc/math-emu/math.c | 53 +++++++++++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 25 deletions(-)

-- 
1.8.1.4

[PATCH 1/2] powerpc/math-emu: move the flush FPU state function into do_mathemu

[Kevin Hao]

By doing this we can make sure that the FPU state is only flushed to
the thread struct when it is really needed.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/powerpc/kernel/traps.c  | 9 ---------
 arch/powerpc/math-emu/math.c | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index bf33c22..58a8065 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1131,15 +1131,6 @@ void __kprobes program_check_exception(struct pt_regs *regs)
 	 * instruction or only on FP instructions, whether there is a
 	 * pattern to occurrences etc. -dgibson 31/Mar/2003
 	 */
-
-	/*
-	 * If we support a HW FPU, we need to ensure the FP state
-	 * if flushed into the thread_struct before attempting
-	 * emulation
-	 */
-#ifdef CONFIG_PPC_FPU
-	flush_fp_to_thread(current);
-#endif
 	switch (do_mathemu(regs)) {
 	case 0:
 		emulate_single_step(regs);
diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
index 3fe8e35..18ce6a7 100644
--- a/arch/powerpc/math-emu/math.c
+++ b/arch/powerpc/math-emu/math.c
@@ -420,6 +420,15 @@ do_mathemu(struct pt_regs *regs)
 		goto illegal;
 	}
 
+	/*
+	 * If we support a HW FPU, we need to ensure the FP state
+	 * if flushed into the thread_struct before attempting
+	 * emulation
+	 */
+#ifdef CONFIG_PPC_FPU
+	flush_fp_to_thread(current);
+#endif
+
 	eflag = func(op0, op1, op2, op3);
 
 	if (insn & 1) {
-- 
1.8.1.4

[PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Kevin Hao]

Some cores (such as Freescale BookE) don't implement all floating
point instructions in ISA. But some gcc versions do use these
instructions. So we would have to enable the math emulation in this
case. Add this to emulated instructions tracking statistics so that
the user has a way to know that its toolcahin emit these unimplemented
floating point instructions.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/powerpc/math-emu/math.c | 50 +++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
index 18ce6a7..9a98b6c 100644
--- a/arch/powerpc/math-emu/math.c
+++ b/arch/powerpc/math-emu/math.c
@@ -10,6 +10,7 @@
 
 #include <asm/sfp-machine.h>
 #include <math-emu/double.h>
+#include <asm/emulated_ops.h>
 
 #define FLOATFUNC(x)	extern int x(void *, void *, void *, void *)
 
@@ -222,10 +223,17 @@ do_mathemu(struct pt_regs *regs)
 	int idx = 0;
 	int (*func)(void *, void *, void *, void *);
 	int type = 0;
-	int eflag, trap;
+	int eflag, trap, ret = -ENOSYS;
+	int has_hw_fpu = 0;
 
-	if (get_user(insn, (u32 *)pc))
-		return -EFAULT;
+#ifdef CONFIG_PPC_FPU
+	has_hw_fpu = 1;
+#endif
+
+	if (get_user(insn, (u32 *)pc)) {
+		ret = -EFAULT;
+		goto out;
+	}
 
 	switch (insn >> 26) {
 	case LFS:	func = lfs;	type = D;	break;
@@ -249,7 +257,7 @@ do_mathemu(struct pt_regs *regs)
 		case STFDUX:	func = stfd;	type = XEU;	break;
 		case STFIWX:	func = stfiwx;	type = XE;	break;
 		default:
-			goto illegal;
+			goto out;
 		}
 		break;
 
@@ -267,7 +275,7 @@ do_mathemu(struct pt_regs *regs)
 		case FNMSUBS:	func = fnmsubs;	type = ABC;	break;
 		case FNMADDS:	func = fnmadds;	type = ABC;	break;
 		default:
-			goto illegal;
+			goto out;
 		}
 		break;
 
@@ -287,7 +295,7 @@ do_mathemu(struct pt_regs *regs)
 			case FNMSUB:	func = fnmsub;	type = ABC;	break;
 			case FNMADD:	func = fnmadd;	type = ABC;	break;
 			default:
-				goto illegal;
+				goto out;
 			}
 			break;
 		}
@@ -309,12 +317,12 @@ do_mathemu(struct pt_regs *regs)
 		case MFFS:	func = mffs;	type = X;	break;
 		case MTFSF:	func = mtfsf;	type = XFLB;	break;
 		default:
-			goto illegal;
+			goto out;
 		}
 		break;
 
 	default:
-		goto illegal;
+		goto out;
 	}
 
 	switch (type) {
@@ -347,7 +355,7 @@ do_mathemu(struct pt_regs *regs)
 	case DU:
 		idx = (insn >> 16) & 0x1f;
 		if (!idx)
-			goto illegal;
+			goto out;
 
 		sdisp = (insn & 0xffff);
 		op0 = (void *)&current->thread.TS_FPR((insn >> 21) & 0x1f);
@@ -375,7 +383,7 @@ do_mathemu(struct pt_regs *regs)
 			if (((insn >> 1) & 0x3ff) == STFIWX)
 				op1 = (void *)(regs->gpr[(insn >> 11) & 0x1f]);
 			else
-				goto illegal;
+				goto out;
 		} else {
 			op1 = (void *)(regs->gpr[idx] + regs->gpr[(insn >> 11) & 0x1f]);
 		}
@@ -417,7 +425,7 @@ do_mathemu(struct pt_regs *regs)
 		break;
 
 	default:
-		goto illegal;
+		goto out;
 	}
 
 	/*
@@ -425,9 +433,8 @@ do_mathemu(struct pt_regs *regs)
 	 * if flushed into the thread_struct before attempting
 	 * emulation
 	 */
-#ifdef CONFIG_PPC_FPU
-	flush_fp_to_thread(current);
-#endif
+	if (has_hw_fpu)
+		flush_fp_to_thread(current);
 
 	eflag = func(op0, op1, op2, op3);
 
@@ -437,8 +444,10 @@ do_mathemu(struct pt_regs *regs)
 	}
 
 	trap = record_exception(regs, eflag);
-	if (trap)
-		return 1;
+	if (trap) {
+		ret = 1;
+		goto out;
+	}
 
 	switch (type) {
 	case DU:
@@ -451,8 +460,11 @@ do_mathemu(struct pt_regs *regs)
 	}
 
 	regs->nip += 4;
-	return 0;
+	ret = 0;
+
+out:
+	if (has_hw_fpu && ret >= 0)
+		PPC_WARN_EMULATED(math, regs);
 
-illegal:
-	return -ENOSYS;
+	return ret;
 }
-- 
1.8.1.4

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Benjamin Herrenschmidt]

On Thu, 2013-07-11 at 20:21 +0800, Kevin Hao wrote:
> Some cores (such as Freescale BookE) don't implement all floating
> point instructions in ISA. But some gcc versions do use these
> instructions. So we would have to enable the math emulation in this
> case. Add this to emulated instructions tracking statistics so that
> the user has a way to know that its toolcahin emit these unimplemented
> floating point instructions.

That patch is gross, it makes the function even more nasty than it
already is. Besides, CONFIG_PPC_FPU doesn't mean you have a HW FPU,
you need to check the CPU feature bits.

Also the caller already does PPC_WARN_EMULATED, so this patch makes
you call it twice or am I missing something ?

Cheers,
Ben.

> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  arch/powerpc/math-emu/math.c | 50 +++++++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
> index 18ce6a7..9a98b6c 100644
> --- a/arch/powerpc/math-emu/math.c
> +++ b/arch/powerpc/math-emu/math.c
> @@ -10,6 +10,7 @@
>  
>  #include <asm/sfp-machine.h>
>  #include <math-emu/double.h>
> +#include <asm/emulated_ops.h>
>  
>  #define FLOATFUNC(x)	extern int x(void *, void *, void *, void *)
>  
> @@ -222,10 +223,17 @@ do_mathemu(struct pt_regs *regs)
>  	int idx = 0;
>  	int (*func)(void *, void *, void *, void *);
>  	int type = 0;
> -	int eflag, trap;
> +	int eflag, trap, ret = -ENOSYS;
> +	int has_hw_fpu = 0;
>  
> -	if (get_user(insn, (u32 *)pc))
> -		return -EFAULT;
> +#ifdef CONFIG_PPC_FPU
> +	has_hw_fpu = 1;
> +#endif
> +
> +	if (get_user(insn, (u32 *)pc)) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
>  
>  	switch (insn >> 26) {
>  	case LFS:	func = lfs;	type = D;	break;
> @@ -249,7 +257,7 @@ do_mathemu(struct pt_regs *regs)
>  		case STFDUX:	func = stfd;	type = XEU;	break;
>  		case STFIWX:	func = stfiwx;	type = XE;	break;
>  		default:
> -			goto illegal;
> +			goto out;
>  		}
>  		break;
>  
> @@ -267,7 +275,7 @@ do_mathemu(struct pt_regs *regs)
>  		case FNMSUBS:	func = fnmsubs;	type = ABC;	break;
>  		case FNMADDS:	func = fnmadds;	type = ABC;	break;
>  		default:
> -			goto illegal;
> +			goto out;
>  		}
>  		break;
>  
> @@ -287,7 +295,7 @@ do_mathemu(struct pt_regs *regs)
>  			case FNMSUB:	func = fnmsub;	type = ABC;	break;
>  			case FNMADD:	func = fnmadd;	type = ABC;	break;
>  			default:
> -				goto illegal;
> +				goto out;
>  			}
>  			break;
>  		}
> @@ -309,12 +317,12 @@ do_mathemu(struct pt_regs *regs)
>  		case MFFS:	func = mffs;	type = X;	break;
>  		case MTFSF:	func = mtfsf;	type = XFLB;	break;
>  		default:
> -			goto illegal;
> +			goto out;
>  		}
>  		break;
>  
>  	default:
> -		goto illegal;
> +		goto out;
>  	}
>  
>  	switch (type) {
> @@ -347,7 +355,7 @@ do_mathemu(struct pt_regs *regs)
>  	case DU:
>  		idx = (insn >> 16) & 0x1f;
>  		if (!idx)
> -			goto illegal;
> +			goto out;
>  
>  		sdisp = (insn & 0xffff);
>  		op0 = (void *)&current->thread.TS_FPR((insn >> 21) & 0x1f);
> @@ -375,7 +383,7 @@ do_mathemu(struct pt_regs *regs)
>  			if (((insn >> 1) & 0x3ff) == STFIWX)
>  				op1 = (void *)(regs->gpr[(insn >> 11) & 0x1f]);
>  			else
> -				goto illegal;
> +				goto out;
>  		} else {
>  			op1 = (void *)(regs->gpr[idx] + regs->gpr[(insn >> 11) & 0x1f]);
>  		}
> @@ -417,7 +425,7 @@ do_mathemu(struct pt_regs *regs)
>  		break;
>  
>  	default:
> -		goto illegal;
> +		goto out;
>  	}
>  
>  	/*
> @@ -425,9 +433,8 @@ do_mathemu(struct pt_regs *regs)
>  	 * if flushed into the thread_struct before attempting
>  	 * emulation
>  	 */
> -#ifdef CONFIG_PPC_FPU
> -	flush_fp_to_thread(current);
> -#endif
> +	if (has_hw_fpu)
> +		flush_fp_to_thread(current);
>  
>  	eflag = func(op0, op1, op2, op3);
>  
> @@ -437,8 +444,10 @@ do_mathemu(struct pt_regs *regs)
>  	}
>  
>  	trap = record_exception(regs, eflag);
> -	if (trap)
> -		return 1;
> +	if (trap) {
> +		ret = 1;
> +		goto out;
> +	}
>  
>  	switch (type) {
>  	case DU:
> @@ -451,8 +460,11 @@ do_mathemu(struct pt_regs *regs)
>  	}
>  
>  	regs->nip += 4;
> -	return 0;
> +	ret = 0;
> +
> +out:
> +	if (has_hw_fpu && ret >= 0)
> +		PPC_WARN_EMULATED(math, regs);
>  
> -illegal:
> -	return -ENOSYS;
> +	return ret;
>  }

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Scott Wood]

On 07/11/2013 07:45:21 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-11 at 20:21 +0800, Kevin Hao wrote:
> > Some cores (such as Freescale BookE) don't implement all floating
> > point instructions in ISA. But some gcc versions do use these
> > instructions. So we would have to enable the math emulation in this
> > case. Add this to emulated instructions tracking statistics so that
> > the user has a way to know that its toolcahin emit these =20
> unimplemented
> > floating point instructions.
>=20
> That patch is gross, it makes the function even more nasty than it
> already is. Besides, CONFIG_PPC_FPU doesn't mean you have a HW FPU,
> you need to check the CPU feature bits.
>=20
> Also the caller already does PPC_WARN_EMULATED, so this patch makes
> you call it twice or am I missing something ?

Sorry, that was my fault -- for some reason I didn't see that when I =20
grepped for PPC_WARN_EMULATED looking for math stuff, and thus =20
requested it be added.  In any case, I don't see why it should be =20
conditional on having an FPU (and indeed, the warning in the caller =20
isn't conditional).

-Scott=

Re: [PATCH 1/2] powerpc/math-emu: move the flush FPU state function into do_mathemu

[Matt Helsley]

On Thu, Jul 11, 2013 at 08:21:54PM +0800, Kevin Hao wrote:
> By doing this we can make sure that the FPU state is only flushed to
> the thread struct when it is really needed.
> 
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  arch/powerpc/kernel/traps.c  | 9 ---------
>  arch/powerpc/math-emu/math.c | 9 +++++++++
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index bf33c22..58a8065 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1131,15 +1131,6 @@ void __kprobes program_check_exception(struct pt_regs *regs)
>  	 * instruction or only on FP instructions, whether there is a
>  	 * pattern to occurrences etc. -dgibson 31/Mar/2003
>  	 */
> -
> -	/*
> -	 * If we support a HW FPU, we need to ensure the FP state
> -	 * if flushed into the thread_struct before attempting
> -	 * emulation
> -	 */
> -#ifdef CONFIG_PPC_FPU
> -	flush_fp_to_thread(current);
> -#endif
>  	switch (do_mathemu(regs)) {
>  	case 0:
>  		emulate_single_step(regs);
> diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
> index 3fe8e35..18ce6a7 100644
> --- a/arch/powerpc/math-emu/math.c
> +++ b/arch/powerpc/math-emu/math.c
> @@ -420,6 +420,15 @@ do_mathemu(struct pt_regs *regs)
>  		goto illegal;
>  	}
> 
> +	/*
> +	 * If we support a HW FPU, we need to ensure the FP state
> +	 * if flushed into the thread_struct before attempting

As long as you're moving the comment you could fix up the spelling
error: s/if/is/

Cheers,
	-Matt Helsley

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 5196 bytes --]

On Thu, Jul 11, 2013 at 10:45:21PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-11 at 20:21 +0800, Kevin Hao wrote:
> > Some cores (such as Freescale BookE) don't implement all floating
> > point instructions in ISA. But some gcc versions do use these
> > instructions. So we would have to enable the math emulation in this
> > case. Add this to emulated instructions tracking statistics so that
> > the user has a way to know that its toolcahin emit these unimplemented
> > floating point instructions.
> 
> That patch is gross, it makes the function even more nasty than it
> already is. Besides, CONFIG_PPC_FPU doesn't mean you have a HW FPU,
> you need to check the CPU feature bits.
> 
> Also the caller already does PPC_WARN_EMULATED, so this patch makes
> you call it twice or am I missing something ?

There are two invocations of do_mathemu() in the traps.c. The one in the
function program_check_exception() doesn't call the PPC_WARN_EMULATED.
This is also the one I try to fix. Of course my patch will definitely corrupt
the one in function SoftwareEmulation(). I will respin a new patch to
fix this. Sorry for my mistake.

Thanks,
Kevin

> 
> Cheers,
> Ben.
> 
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> >  arch/powerpc/math-emu/math.c | 50 +++++++++++++++++++++++++++-----------------
> >  1 file changed, 31 insertions(+), 19 deletions(-)
> > 
> > diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
> > index 18ce6a7..9a98b6c 100644
> > --- a/arch/powerpc/math-emu/math.c
> > +++ b/arch/powerpc/math-emu/math.c
> > @@ -10,6 +10,7 @@
> >  
> >  #include <asm/sfp-machine.h>
> >  #include <math-emu/double.h>
> > +#include <asm/emulated_ops.h>
> >  
> >  #define FLOATFUNC(x)	extern int x(void *, void *, void *, void *)
> >  
> > @@ -222,10 +223,17 @@ do_mathemu(struct pt_regs *regs)
> >  	int idx = 0;
> >  	int (*func)(void *, void *, void *, void *);
> >  	int type = 0;
> > -	int eflag, trap;
> > +	int eflag, trap, ret = -ENOSYS;
> > +	int has_hw_fpu = 0;
> >  
> > -	if (get_user(insn, (u32 *)pc))
> > -		return -EFAULT;
> > +#ifdef CONFIG_PPC_FPU
> > +	has_hw_fpu = 1;
> > +#endif
> > +
> > +	if (get_user(insn, (u32 *)pc)) {
> > +		ret = -EFAULT;
> > +		goto out;
> > +	}
> >  
> >  	switch (insn >> 26) {
> >  	case LFS:	func = lfs;	type = D;	break;
> > @@ -249,7 +257,7 @@ do_mathemu(struct pt_regs *regs)
> >  		case STFDUX:	func = stfd;	type = XEU;	break;
> >  		case STFIWX:	func = stfiwx;	type = XE;	break;
> >  		default:
> > -			goto illegal;
> > +			goto out;
> >  		}
> >  		break;
> >  
> > @@ -267,7 +275,7 @@ do_mathemu(struct pt_regs *regs)
> >  		case FNMSUBS:	func = fnmsubs;	type = ABC;	break;
> >  		case FNMADDS:	func = fnmadds;	type = ABC;	break;
> >  		default:
> > -			goto illegal;
> > +			goto out;
> >  		}
> >  		break;
> >  
> > @@ -287,7 +295,7 @@ do_mathemu(struct pt_regs *regs)
> >  			case FNMSUB:	func = fnmsub;	type = ABC;	break;
> >  			case FNMADD:	func = fnmadd;	type = ABC;	break;
> >  			default:
> > -				goto illegal;
> > +				goto out;
> >  			}
> >  			break;
> >  		}
> > @@ -309,12 +317,12 @@ do_mathemu(struct pt_regs *regs)
> >  		case MFFS:	func = mffs;	type = X;	break;
> >  		case MTFSF:	func = mtfsf;	type = XFLB;	break;
> >  		default:
> > -			goto illegal;
> > +			goto out;
> >  		}
> >  		break;
> >  
> >  	default:
> > -		goto illegal;
> > +		goto out;
> >  	}
> >  
> >  	switch (type) {
> > @@ -347,7 +355,7 @@ do_mathemu(struct pt_regs *regs)
> >  	case DU:
> >  		idx = (insn >> 16) & 0x1f;
> >  		if (!idx)
> > -			goto illegal;
> > +			goto out;
> >  
> >  		sdisp = (insn & 0xffff);
> >  		op0 = (void *)&current->thread.TS_FPR((insn >> 21) & 0x1f);
> > @@ -375,7 +383,7 @@ do_mathemu(struct pt_regs *regs)
> >  			if (((insn >> 1) & 0x3ff) == STFIWX)
> >  				op1 = (void *)(regs->gpr[(insn >> 11) & 0x1f]);
> >  			else
> > -				goto illegal;
> > +				goto out;
> >  		} else {
> >  			op1 = (void *)(regs->gpr[idx] + regs->gpr[(insn >> 11) & 0x1f]);
> >  		}
> > @@ -417,7 +425,7 @@ do_mathemu(struct pt_regs *regs)
> >  		break;
> >  
> >  	default:
> > -		goto illegal;
> > +		goto out;
> >  	}
> >  
> >  	/*
> > @@ -425,9 +433,8 @@ do_mathemu(struct pt_regs *regs)
> >  	 * if flushed into the thread_struct before attempting
> >  	 * emulation
> >  	 */
> > -#ifdef CONFIG_PPC_FPU
> > -	flush_fp_to_thread(current);
> > -#endif
> > +	if (has_hw_fpu)
> > +		flush_fp_to_thread(current);
> >  
> >  	eflag = func(op0, op1, op2, op3);
> >  
> > @@ -437,8 +444,10 @@ do_mathemu(struct pt_regs *regs)
> >  	}
> >  
> >  	trap = record_exception(regs, eflag);
> > -	if (trap)
> > -		return 1;
> > +	if (trap) {
> > +		ret = 1;
> > +		goto out;
> > +	}
> >  
> >  	switch (type) {
> >  	case DU:
> > @@ -451,8 +460,11 @@ do_mathemu(struct pt_regs *regs)
> >  	}
> >  
> >  	regs->nip += 4;
> > -	return 0;
> > +	ret = 0;
> > +
> > +out:
> > +	if (has_hw_fpu && ret >= 0)
> > +		PPC_WARN_EMULATED(math, regs);
> >  
> > -illegal:
> > -	return -ENOSYS;
> > +	return ret;
> >  }
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

On Thu, Jul 11, 2013 at 09:30:02AM -0500, Scott Wood wrote:
> On 07/11/2013 07:45:21 AM, Benjamin Herrenschmidt wrote:
> >On Thu, 2013-07-11 at 20:21 +0800, Kevin Hao wrote:
> >> Some cores (such as Freescale BookE) don't implement all floating
> >> point instructions in ISA. But some gcc versions do use these
> >> instructions. So we would have to enable the math emulation in this
> >> case. Add this to emulated instructions tracking statistics so that
> >> the user has a way to know that its toolcahin emit these
> >unimplemented
> >> floating point instructions.
> >
> >That patch is gross, it makes the function even more nasty than it
> >already is. Besides, CONFIG_PPC_FPU doesn't mean you have a HW FPU,
> >you need to check the CPU feature bits.
> >
> >Also the caller already does PPC_WARN_EMULATED, so this patch makes
> >you call it twice or am I missing something ?
> 
> Sorry, that was my fault -- for some reason I didn't see that when I
> grepped for PPC_WARN_EMULATED looking for math stuff, and thus
> requested it be added.  In any case, I don't see why it should be
> conditional on having an FPU (and indeed, the warning in the caller
> isn't conditional).

I thought it only made sense to warn only for the case when the core
does have a FPU but some unimplemented floating instructions are emulated.
As for the core which doesn't have a FPU at all and we explicitly enable the
math emulation it may seems a little redundant to warn in this case. But after
a second thought, this is the statistics of all the emulated instructions,
so it does seem reasonable to warn in all cases. I will remove the dependancy
on FPU.

Thanks,
Kevin

> 
> -Scott

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 10:07 +0800, Kevin Hao wrote:
> There are two invocations of do_mathemu() in the traps.c. The one in the
> function program_check_exception() doesn't call the PPC_WARN_EMULATED.
> This is also the one I try to fix. Of course my patch will definitely corrupt
> the one in function SoftwareEmulation(). I will respin a new patch to
> fix this. Sorry for my mistake.

Put the call in the caller (in program_check_exception). It keeps the code
in math-emu simpler and is consistent with what we do elsewhere.

Cheers,
Ben.

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

On Fri, Jul 12, 2013 at 01:56:03PM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2013-07-12 at 10:07 +0800, Kevin Hao wrote:
> > There are two invocations of do_mathemu() in the traps.c. The one in the
> > function program_check_exception() doesn't call the PPC_WARN_EMULATED.
> > This is also the one I try to fix. Of course my patch will definitely corrupt
> > the one in function SoftwareEmulation(). I will respin a new patch to
> > fix this. Sorry for my mistake.
> 
> Put the call in the caller (in program_check_exception). It keeps the code
> in math-emu simpler and is consistent with what we do elsewhere.

Sure. V2 coming soon.

Thanks,
Kevin

> 
> Cheers,
> Ben.
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Scott Wood]

On 07/11/2013 09:25:12 PM, Kevin Hao wrote:
> On Thu, Jul 11, 2013 at 09:30:02AM -0500, Scott Wood wrote:
> > Sorry, that was my fault -- for some reason I didn't see that when I
> > grepped for PPC_WARN_EMULATED looking for math stuff, and thus
> > requested it be added.  In any case, I don't see why it should be
> > conditional on having an FPU (and indeed, the warning in the caller
> > isn't conditional).
>=20
> I thought it only made sense to warn only for the case when the core
> does have a FPU but some unimplemented floating instructions are =20
> emulated.
> As for the core which doesn't have a FPU at all and we explicitly =20
> enable the
> math emulation it may seems a little redundant to warn in this case. =20
> But after
> a second thought, this is the statistics of all the emulated =20
> instructions,
> so it does seem reasonable to warn in all cases. I will remove the =20
> dependancy
> on FPU.

It's not redundant at all to warn when an FPU is absent.  It tells you =20
that you're being slowed down by running hard-FP code instead of =20
soft-FP code.

-Scott=

Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 15:53 -0500, Scott Wood wrote:
> 
> It's not redundant at all to warn when an FPU is absent.  It tells you  
> that you're being slowed down by running hard-FP code instead of  
> soft-FP code.

Right, just warn always.

Ben.

Re: [PATCH 1/2] powerpc/math-emu: move the flush FPU state function into do_mathemu

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

On Thu, Jul 11, 2013 at 05:34:45PM -0700, Matt Helsley wrote:
> On Thu, Jul 11, 2013 at 08:21:54PM +0800, Kevin Hao wrote:
> > By doing this we can make sure that the FPU state is only flushed to
> > the thread struct when it is really needed.
> > 
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> >  arch/powerpc/kernel/traps.c  | 9 ---------
> >  arch/powerpc/math-emu/math.c | 9 +++++++++
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index bf33c22..58a8065 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -1131,15 +1131,6 @@ void __kprobes program_check_exception(struct pt_regs *regs)
> >  	 * instruction or only on FP instructions, whether there is a
> >  	 * pattern to occurrences etc. -dgibson 31/Mar/2003
> >  	 */
> > -
> > -	/*
> > -	 * If we support a HW FPU, we need to ensure the FP state
> > -	 * if flushed into the thread_struct before attempting
> > -	 * emulation
> > -	 */
> > -#ifdef CONFIG_PPC_FPU
> > -	flush_fp_to_thread(current);
> > -#endif
> >  	switch (do_mathemu(regs)) {
> >  	case 0:
> >  		emulate_single_step(regs);
> > diff --git a/arch/powerpc/math-emu/math.c b/arch/powerpc/math-emu/math.c
> > index 3fe8e35..18ce6a7 100644
> > --- a/arch/powerpc/math-emu/math.c
> > +++ b/arch/powerpc/math-emu/math.c
> > @@ -420,6 +420,15 @@ do_mathemu(struct pt_regs *regs)
> >  		goto illegal;
> >  	}
> > 
> > +	/*
> > +	 * If we support a HW FPU, we need to ensure the FP state
> > +	 * if flushed into the thread_struct before attempting
> 
> As long as you're moving the comment you could fix up the spelling
> error: s/if/is/

Fixed. Thanks Matt.

Kevin

> 
> Cheers,
> 	-Matt Helsley
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]