Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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. > > pgpSJyLieBhOv.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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 pgpmTtbKmzCPL.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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
> > ---
> > 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
> > #include
> > +#include
> >
> > #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 & 0x);
> > 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
> > */
Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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). -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc/math-emu: keep track of the instructions unimplemented by FPU
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
> ---
> 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
> #include
> +#include
>
> #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 & 0x);
> 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)
>
