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

login
register
mail settings
Submitter Kevin Hao
Date July 11, 2013, 12:21 p.m.
Message ID <1373545315-9219-3-git-send-email-haokexin@gmail.com>
Download mbox | patch
Permalink /patch/258438/
State Superseded
Headers show

Comments

Kevin Hao - July 11, 2013, 12:21 p.m.
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(-)
Benjamin Herrenschmidt - July 11, 2013, 12:45 p.m.
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;
>  }
Kevin Hao - July 12, 2013, 2:07 a.m.
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;
> >  }
> 
>
Kevin Hao - July 12, 2013, 2:25 a.m.
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
Benjamin Herrenschmidt - July 12, 2013, 3:56 a.m.
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.
Kevin Hao - July 12, 2013, 5:05 a.m.
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.
> 
>
Scott Wood - July 12, 2013, 8:53 p.m.
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).
> 
> 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.

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.

-Scott
Benjamin Herrenschmidt - July 12, 2013, 11:05 p.m.
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.

Patch

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