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

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

[Kevin Hao]

v2:
Tweak the patch 2 as suggested by Benjamin and Scott.

v1:
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: introduce function emulate_math()

 arch/powerpc/kernel/traps.c  | 88 +++++++++++++++++---------------------------
 arch/powerpc/math-emu/math.c |  9 +++++
 2 files changed, 43 insertions(+), 54 deletions(-)

-- 
1.8.1.4

[PATCH v2 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>
---
v2: No change.

 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 v2 2/2] powerpc: introduce function emulate_math()

[Kevin Hao]

There are two invocations of do_mathemu() in traps.c. And the codes
in these two places are almost the same. Introduce a locale function
to eliminate the duplication. With this change we can also make sure
that in program_check_exception() the PPC_WARN_EMULATED is invoked for
the correctly emulated math instructions.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2:
  * Don't depend on the FPU for the PPC_WARN_EMULATED.
  * Invoke the PPC_WARN_EMULATED in the caller of do_mathemu().
  * Introduce a new function to eliminate the code duplication.

 arch/powerpc/kernel/traps.c | 79 +++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 45 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 58a8065..217724a 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1052,11 +1052,41 @@ int is_valid_bugaddr(unsigned long addr)
 	return is_kernel_addr(addr);
 }
 
+#ifdef CONFIG_MATH_EMULATION
+static int emulate_math(struct pt_regs *regs)
+{
+	int ret;
+	extern int do_mathemu(struct pt_regs *regs);
+
+	ret = do_mathemu(regs);
+	if (ret >= 0)
+		PPC_WARN_EMULATED(math, regs);
+
+	switch (ret) {
+	case 0:
+		emulate_single_step(regs);
+		return 0;
+	case 1: {
+			int code = 0;
+			code = __parse_fpscr(current->thread.fpscr.val);
+			_exception(SIGFPE, regs, code, regs->nip);
+			return 0;
+		}
+	case -EFAULT:
+		_exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
+		return 0;
+	}
+
+	return -1;
+}
+#else
+static inline int emulate_math(struct pt_regs *regs) { return -1; }
+#endif
+
 void __kprobes program_check_exception(struct pt_regs *regs)
 {
 	enum ctx_state prev_state = exception_enter();
 	unsigned int reason = get_reason(regs);
-	extern int do_mathemu(struct pt_regs *regs);
 
 	/* We can now get here via a FP Unavailable exception if the core
 	 * has no FPU, in that case the reason flags will be 0 */
@@ -1122,7 +1152,6 @@ void __kprobes program_check_exception(struct pt_regs *regs)
 	if (!arch_irq_disabled_regs(regs))
 		local_irq_enable();
 
-#ifdef CONFIG_MATH_EMULATION
 	/* (reason & REASON_ILLEGAL) would be the obvious thing here,
 	 * but there seems to be a hardware bug on the 405GP (RevD)
 	 * that means ESR is sometimes set incorrectly - either to
@@ -1131,22 +1160,8 @@ 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
 	 */
-	switch (do_mathemu(regs)) {
-	case 0:
-		emulate_single_step(regs);
-		goto bail;
-	case 1: {
-			int code = 0;
-			code = __parse_fpscr(current->thread.fpscr.val);
-			_exception(SIGFPE, regs, code, regs->nip);
-			goto bail;
-		}
-	case -EFAULT:
-		_exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
+	if (!emulate_math(regs))
 		goto bail;
-	}
-	/* fall through on any other errors */
-#endif /* CONFIG_MATH_EMULATION */
 
 	/* Try to emulate it if we should. */
 	if (reason & (REASON_ILLEGAL | REASON_PRIVILEGED)) {
@@ -1425,11 +1440,6 @@ void performance_monitor_exception(struct pt_regs *regs)
 #ifdef CONFIG_8xx
 void SoftwareEmulation(struct pt_regs *regs)
 {
-	extern int do_mathemu(struct pt_regs *);
-#if defined(CONFIG_MATH_EMULATION)
-	int errcode;
-#endif
-
 	CHECK_FULL_REGS(regs);
 
 	if (!user_mode(regs)) {
@@ -1437,31 +1447,10 @@ void SoftwareEmulation(struct pt_regs *regs)
 		die("Kernel Mode Software FPU Emulation", regs, SIGFPE);
 	}
 
-#ifdef CONFIG_MATH_EMULATION
-	errcode = do_mathemu(regs);
-	if (errcode >= 0)
-		PPC_WARN_EMULATED(math, regs);
-
-	switch (errcode) {
-	case 0:
-		emulate_single_step(regs);
-		return;
-	case 1: {
-			int code = 0;
-			code = __parse_fpscr(current->thread.fpscr.val);
-			_exception(SIGFPE, regs, code, regs->nip);
-			return;
-		}
-	case -EFAULT:
-		_exception(SIGSEGV, regs, SEGV_MAPERR, regs->nip);
+	if (!emulate_math(regs))
 		return;
-	default:
-		_exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
-		return;
-	}
-#else
+
 	_exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
-#endif
 }
 #endif /* CONFIG_8xx */
 
-- 
1.8.1.4

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

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 13:36 +0800, Kevin Hao wrote:
> +       /*
> +        * 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
> +

While at it, care to send a patch that defined an empty
flush_fp_to_thread() in the header instead ?

ie, switch_to.h

#ifdef CONFIG_PPC_FPU
extern void flush_fp_to_thread(struct task-struct *);
#else
static inline void flush_fp_to_thread(struct task-struct *) { };
#endif

And get rid of the ifdef's here (and elsewhere if any) ?

Thanks !

Cheers,
Ben.

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

[Kevin Hao]

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

On Fri, Jul 12, 2013 at 06:37:13PM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2013-07-12 at 13:36 +0800, Kevin Hao wrote:
> > +       /*
> > +        * 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
> > +
> 
> While at it, care to send a patch that defined an empty
> flush_fp_to_thread() in the header instead ?
> 
> ie, switch_to.h
> 
> #ifdef CONFIG_PPC_FPU
> extern void flush_fp_to_thread(struct task-struct *);
> #else
> static inline void flush_fp_to_thread(struct task-struct *) { };
> #endif
> 
> And get rid of the ifdef's here (and elsewhere if any) ?

Sure. Matt also pointed out a typo in the comments of this patch.
So I will respin a v3 for this. But since I also have some similar
clean up patches as the change you just suggested, I will send them
as a separate patch series instead of messing the v3.

Thanks,
Kevin

> 
> Thanks !
> 
> Cheers,
> Ben.
> 
> 

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