| Message ID | 1373545315-9219-3-git-send-email-haokexin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 *)¤t->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; > }
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 *)¤t->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; > > } > >
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
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.
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. > >
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
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.
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 *)¤t->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; }
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(-)