[RFC PATCH 00/12] Deal with TM on kernel entry and exit

[RFC PATCH 00/12] Deal with TM on kernel entry and exit

[Cyril Bur]

This is very much a proof of concept and if it isn't clear from the
commit names, still a work in progress.

I believe I have something that works - all the powerpc selftests
pass. I would like to get some eyes on it to a) see if I've missed
anything big and b) some opinions on if it is looking like a net
improvement.

Obviously it is still a bit rough around the edges, I'll have to
convince myself that the SPR code is correct. I don't think the
TM_KERNEL_ENTRY macro needs to check that we came from userspace, if
TM is on then we can probably assume. Maybe a check not in the
fastpath. Some of the BUG_ON()s will probably go.

Background:
Currently TM is dealt with when we need to. That is, when we switch
processes, we'll (if nessesary) reclaim the outgoing process and (if
nessesary) recheckpoint the incoming process. Same with signals, if we
need to deliver a signal, we'll ensure we've reclaimed in order to
have all the information and go from there.
I, along with some others got curious to see what it would look like if
we did the 'opposite'.
At all kernel entry points that won't simply just zoom straight to an
RFID we now check if the thread was transactional and do the reclaim.
Correspondingly do the recheckpoint quite late on exception exit. It
turns out we already had a lot of the code pathes set up on the exit
path as there were things that TM had special cased on exit already.
I wasn't sure it it would lead to more or less complexity and though
I'd have to try it to see. I feel like it was almost a win but SPRs
did add some annoying caveats.

In order to get this past Michael I'm going to prove it performs, or
rather, doesn't slow anything down - workload suggestions welcome.

Thanks,

Cyril Bur (12):
  powerpc/tm: Remove struct thread_info param from tm_reclaim_thread()
  selftests/powerpc: Fix tm.h helpers
  selftests/powerpc: Add tm-signal-drop-transaction TM test
  selftests/powerpc: Use less common thread names
  [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit
  [WIP] powerpc/tm: Remove dead code from __switch_to_tm()
  [WIP] powerpc/tm: Add TM_KERNEL_ENTRY in more delicate exception
    pathes
  [WIP] powerpc/tm: Fix *unavailable_tm exceptions
  [WIP] powerpc/tm: Tweak signal code to handle new reclaim/recheckpoint
    times
  [WIP] powerpc/tm: Correctly save/restore checkpointed sprs
  [WIP] powerpc/tm: Afterthoughts
  [WIP] selftests/powerpc: Remove incorrect tm-syscall selftest

 arch/powerpc/include/asm/exception-64s.h           |  25 ++++
 arch/powerpc/kernel/entry_64.S                     |  20 ++-
 arch/powerpc/kernel/exceptions-64s.S               |  31 ++++-
 arch/powerpc/kernel/process.c                      | 145 ++++++++++++++++++---
 arch/powerpc/kernel/ptrace.c                       |   9 +-
 arch/powerpc/kernel/signal.c                       |  11 +-
 arch/powerpc/kernel/signal_32.c                    |  16 +--
 arch/powerpc/kernel/signal_64.c                    |  41 ++++--
 arch/powerpc/kernel/traps.c                        |   3 -
 tools/testing/selftests/powerpc/tm/Makefile        |   5 +-
 .../powerpc/tm/tm-signal-drop-transaction.c        |  74 +++++++++++
 .../testing/selftests/powerpc/tm/tm-syscall-asm.S  |  28 ----
 tools/testing/selftests/powerpc/tm/tm-syscall.c    | 106 ---------------
 .../testing/selftests/powerpc/tm/tm-unavailable.c  |   4 +-
 tools/testing/selftests/powerpc/tm/tm.h            |  10 +-
 15 files changed, 319 insertions(+), 209 deletions(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-drop-transaction.c
 delete mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
 delete mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall.c

-- 
2.16.2

[RFC PATCH 01/12] powerpc/tm: Remove struct thread_info param from tm_reclaim_thread()

[Cyril Bur]

tm_reclaim_thread() doesn't use the parameter anymore, both callers have
to bother getting it as they have no need for a struct thread_info
either.

It was previously used but became unused in dc3106690b20 ("powerpc: tm:
Always use fp_state and vr_state to store live registers")

Just remove it and adjust the callers.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/kernel/process.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 1738c4127b32..77dc6d8288eb 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -850,8 +850,7 @@ static inline bool tm_enabled(struct task_struct *tsk)
 	return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
 }
 
-static void tm_reclaim_thread(struct thread_struct *thr,
-			      struct thread_info *ti, uint8_t cause)
+static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
 {
 	/*
 	 * Use the current MSR TM suspended bit to track if we have
@@ -898,7 +897,7 @@ static void tm_reclaim_thread(struct thread_struct *thr,
 void tm_reclaim_current(uint8_t cause)
 {
 	tm_enable();
-	tm_reclaim_thread(&current->thread, current_thread_info(), cause);
+	tm_reclaim_thread(&current->thread, cause);
 }
 
 static inline void tm_reclaim_task(struct task_struct *tsk)
@@ -929,7 +928,7 @@ static inline void tm_reclaim_task(struct task_struct *tsk)
 		 thr->regs->ccr, thr->regs->msr,
 		 thr->regs->trap);
 
-	tm_reclaim_thread(thr, task_thread_info(tsk), TM_CAUSE_RESCHED);
+	tm_reclaim_thread(thr, TM_CAUSE_RESCHED);
 
 	TM_DEBUG("--- tm_reclaim on pid %d complete\n",
 		 tsk->pid);
-- 
2.16.2

[RFC PATCH 02/12] selftests/powerpc: Fix tm.h helpers

[Cyril Bur]

Turns out the tcheck() helpers were subtly wrong

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 tools/testing/selftests/powerpc/tm/tm.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm.h b/tools/testing/selftests/powerpc/tm/tm.h
index df4204247d45..e187a0d3160c 100644
--- a/tools/testing/selftests/powerpc/tm/tm.h
+++ b/tools/testing/selftests/powerpc/tm/tm.h
@@ -57,11 +57,11 @@ static inline bool failure_is_nesting(void)
 	return (__builtin_get_texasru() & 0x400000);
 }
 
-static inline int tcheck(void)
+static inline uint8_t tcheck(void)
 {
-	long cr;
-	asm volatile ("tcheck 0" : "=r"(cr) : : "cr0");
-	return (cr >> 28) & 4;
+	unsigned long cr;
+	asm volatile ("tcheck 0; mfcr %0;" : "=r"(cr) : : "cr0");
+	return (cr >> 28) & 0xF;
 }
 
 static inline bool tcheck_doomed(void)
@@ -81,7 +81,7 @@ static inline bool tcheck_suspended(void)
 
 static inline bool tcheck_transactional(void)
 {
-	return tcheck() & 6;
+	return (tcheck_active()) || (tcheck_suspended());
 }
 
 #endif /* _SELFTESTS_POWERPC_TM_TM_H */
-- 
2.16.2

[RFC PATCH 03/12] selftests/powerpc: Add tm-signal-drop-transaction TM test

[Cyril Bur]

This test uses a signal to 'discard' a transaction. That is, it will
take a signal of a thread in a suspended transaction and just remove
the suspended MSR bit. Because this will send the userspace thread back
to the tebgin + 4 address, we should also set CR0 to be nice.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 tools/testing/selftests/powerpc/tm/Makefile        |  1 +
 .../powerpc/tm/tm-signal-drop-transaction.c        | 74 ++++++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-drop-transaction.c

diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index a23453943ad2..7a1e53297588 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -4,6 +4,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr tm-signal-context-chk-fpu
 
 TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv tm-signal-stack \
 	tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable tm-trap \
+	tm-signal-drop-transaction \
 	$(SIGNAL_CONTEXT_CHK_TESTS)
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-drop-transaction.c b/tools/testing/selftests/powerpc/tm/tm-signal-drop-transaction.c
new file mode 100644
index 000000000000..a8397f7e7faa
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-drop-transaction.c
@@ -0,0 +1,74 @@
+/*
+ * Copyright 2018, Cyril Bur, IBM Corp.
+ * Licensed under GPLv2.
+ *
+ * This test uses a signal handler to make a thread go from
+ * transactional state to nothing state. In practice userspace, why
+ * would userspace ever do this? In theory, they can.
+ */
+
+#include <errno.h>
+#include <inttypes.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "utils.h"
+#include "tm.h"
+
+static bool passed;
+
+static void signal_usr1(int signum, siginfo_t *info, void *uc)
+{
+	ucontext_t *ucp = uc;
+	struct pt_regs *regs = ucp->uc_mcontext.regs;
+
+	passed = true;
+
+	/* I really hope I got that right, we wan't to clear both the MSR_TS bits */
+	regs->msr &= ~(3ULL << 33);
+	/* Set CR0 to 0b0010 */
+	regs->ccr &= ~(0xDULL << 28);
+}
+
+int test_drop(void)
+{
+	struct sigaction act;
+
+	SKIP_IF(!have_htm());
+
+	act.sa_sigaction = signal_usr1;
+	sigemptyset(&act.sa_mask);
+	act.sa_flags = SA_SIGINFO;
+	if (sigaction(SIGUSR1, &act, NULL) < 0) {
+		perror("sigaction sigusr1");
+		exit(1);
+	}
+
+
+	asm __volatile__(
+		"tbegin.;"
+		"beq    1f; "
+		"tsuspend.;"
+		"1: ;"
+		: : : "memory", "cr0");
+
+	if (!passed && !tcheck_transactional()) {
+		fprintf(stderr, "Not in suspended state: 0x%1x\n", tcheck());
+		exit(1);
+	}
+
+	kill(getpid(), SIGUSR1);
+
+	/* If we reach here, we've passed.  Otherwise we've probably crashed
+	 * the kernel */
+
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	return test_harness(test_drop, "tm_signal_drop_transaction");
+}
-- 
2.16.2

[RFC PATCH 04/12] selftests/powerpc: Use less common thread names

[Cyril Bur]

"ping" and "pong" (in particular "ping") are common names. If a
selftests causes a kernel BUG_ON or any kind of backtrace the process
name is displayed. Setting a more unique name avoids confusion as to
which process caused the problem.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 tools/testing/selftests/powerpc/tm/tm-unavailable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index e6a0fad2bfd0..bcfa8add5748 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -315,7 +315,7 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
 		rc = pthread_create(&t0, attr, ping, (void *) &flags);
 		if (rc)
 			pr_err(rc, "pthread_create()");
-		rc = pthread_setname_np(t0, "ping");
+		rc = pthread_setname_np(t0, "tm-unavailable-ping");
 		if (rc)
 			pr_warn(rc, "pthread_setname_np");
 		rc = pthread_join(t0, &ret_value);
@@ -359,7 +359,7 @@ int main(int argc, char **argv)
 		pr_err(rc, "pthread_create()");
 
 	/* Name it for systemtap convenience */
-	rc = pthread_setname_np(t1, "pong");
+	rc = pthread_setname_np(t1, "tm-unavailable-pong");
 	if (rc)
 		pr_warn(rc, "pthread_create()");
 
-- 
2.16.2

[RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit

[Cyril Bur]

---
 arch/powerpc/include/asm/exception-64s.h | 25 +++++++++++++++++++++
 arch/powerpc/kernel/entry_64.S           |  5 +++++
 arch/powerpc/kernel/process.c            | 37 ++++++++++++++++++++++++++++----
 3 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index 471b2274fbeb..f904f19a9ec2 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -35,6 +35,7 @@
  * implementations as possible.
  */
 #include <asm/head-64.h>
+#include <asm/tm.h>
 
 /* PACA save area offsets (exgen, exmc, etc) */
 #define EX_R9		0
@@ -127,6 +128,26 @@
 	hrfid;								\
 	b	hrfi_flush_fallback
 
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+#define TM_KERNEL_ENTRY		                                        \
+	ld	r3,_MSR(r1);			                        \
+	/* Probably don't need to check if coming from user/kernel */	\
+	/* If TM is suspended or active then we must have come from*/	\
+	/* userspace */							\
+	andi.	r0,r3,MSR_PR;						\
+	beq	1f;							\
+	rldicl. r3,r3,(64-MSR_TS_LG),(64-2); /* SUSPENDED or ACTIVE*/   \
+	beql+	1f;                   	/* Not SUSPENDED or ACTIVE */   \
+	bl	save_nvgprs;						\
+	RECONCILE_IRQ_STATE(r10,r11);					\
+	li	r3,TM_CAUSE_MISC;					\
+	bl	tm_reclaim_current;	/* uint8 cause		   */	\
+1:
+
+#else /* CONFIG_PPC_TRANSACTIONAL_MEM */
+#define TM_KERNEL_ENTRY
+#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
+
 #ifdef CONFIG_RELOCATABLE
 #define __EXCEPTION_RELON_PROLOG_PSERIES_1(label, h)			\
 	mfspr	r11,SPRN_##h##SRR0;	/* save SRR0 */			\
@@ -675,6 +696,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
 	EXCEPTION_PROLOG_COMMON(trap, area);			\
 	/* Volatile regs are potentially clobbered here */	\
 	additions;						\
+	/* This is going to need to go somewhere else as well */\
+	/* See comment in tm_recheckpoint()		      */\
+	TM_KERNEL_ENTRY;					\
 	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
 	bl	hdlr;						\
 	b	ret
@@ -689,6 +713,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
 	EXCEPTION_PROLOG_COMMON_3(trap);			\
 	/* Volatile regs are potentially clobbered here */	\
 	additions;						\
+	TM_KERNEL_ENTRY;					\
 	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
 	bl	hdlr
 
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 2cb5109a7ea3..107c15c6f48b 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -126,6 +126,11 @@ BEGIN_FW_FTR_SECTION
 33:
 END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE && CONFIG_PPC_SPLPAR */
+	TM_KERNEL_ENTRY
+	REST_GPR(0,r1)
+	REST_4GPRS(3,r1)
+	REST_2GPRS(7,r1)
+	addi	r9,r1,STACK_FRAME_OVERHEAD
 
 	/*
 	 * A syscall should always be called with interrupts enabled
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 77dc6d8288eb..ea75da0fd506 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -951,6 +951,23 @@ void tm_recheckpoint(struct thread_struct *thread)
 	if (!(thread->regs->msr & MSR_TM))
 		return;
 
+	/*
+	 * This is 'that' comment.
+	 *
+	 * If we get where with tm suspended or active then something
+	 * has gone wrong. I've added this now as a proof of concept.
+	 *
+	 * The problem I'm seeing without it is an attempt to
+	 * recheckpoint a CPU without a previous reclaim.
+	 *
+	 * I'm probably missed an exception entry with the
+	 * TM_KERNEL_ENTRY macro. Should be easy enough to find.
+	 */
+	if (MSR_TM_ACTIVE(mfmsr()))
+		return;
+
+	tm_enable();
+
 	/* We really can't be interrupted here as the TEXASR registers can't
 	 * change and later in the trecheckpoint code, we have a userspace R1.
 	 * So let's hard disable over this region.
@@ -1009,6 +1026,13 @@ static inline void tm_recheckpoint_new_task(struct task_struct *new)
 static inline void __switch_to_tm(struct task_struct *prev,
 		struct task_struct *new)
 {
+	/*
+	 * So, with the rework none of this code should not be needed.
+	 * I've left in the reclaim for now. This *should* save us
+	 * from any mistake in the new code. Also the
+	 * enabling/disabling logic of MSR_TM really should be
+	 * refactored into a common way with MSR_{FP,VEC,VSX}
+	 */
 	if (cpu_has_feature(CPU_FTR_TM)) {
 		if (tm_enabled(prev) || tm_enabled(new))
 			tm_enable();
@@ -1016,11 +1040,14 @@ static inline void __switch_to_tm(struct task_struct *prev,
 		if (tm_enabled(prev)) {
 			prev->thread.load_tm++;
 			tm_reclaim_task(prev);
-			if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
-				prev->thread.regs->msr &= ~MSR_TM;
+			/*
+			 * The disabling logic may be confused don't
+			 * disable for now
+			 *
+			 * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
+			 *	prev->thread.regs->msr &= ~MSR_TM;
+			 */
 		}
-
-		tm_recheckpoint_new_task(new);
 	}
 }
 
@@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
 	msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
 	msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
 
+	tm_recheckpoint(&current->thread);
+
 	/* Ensure that restore_math() will restore */
 	if (msr_diff & MSR_FP)
 		current->thread.load_fp = 1;
-- 
2.16.2

Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit

[Michael Neuling]

On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:


The comment from the cover sheet should be here

> ---
>  arch/powerpc/include/asm/exception-64s.h | 25 +++++++++++++++++++++
>  arch/powerpc/kernel/entry_64.S           |  5 +++++
>  arch/powerpc/kernel/process.c            | 37 ++++++++++++++++++++++++++=
++----
>  3 files changed, 63 insertions(+), 4 deletions(-)
>=20
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/incl=
ude/asm/exception-64s.h
> index 471b2274fbeb..f904f19a9ec2 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -35,6 +35,7 @@
>   * implementations as possible.
>   */
>  #include <asm/head-64.h>
> +#include <asm/tm.h>
> =20
>  /* PACA save area offsets (exgen, exmc, etc) */
>  #define EX_R9		0
> @@ -127,6 +128,26 @@
>  	hrfid;								\
>  	b	hrfi_flush_fallback
> =20
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +#define TM_KERNEL_ENTRY		                                        \
> +	ld	r3,_MSR(r1);			                        \
> +	/* Probably don't need to check if coming from user/kernel */	\
> +	/* If TM is suspended or active then we must have come from*/	\
> +	/* userspace */							\
> +	andi.	r0,r3,MSR_PR;						\
> +	beq	1f;							\
> +	rldicl. r3,r3,(64-MSR_TS_LG),(64-2); /* SUSPENDED or ACTIVE*/   \
> +	beql+	1f;                   	/* Not SUSPENDED or ACTIVE */   \
> +	bl	save_nvgprs;						\
> +	RECONCILE_IRQ_STATE(r10,r11);					\
> +	li	r3,TM_CAUSE_MISC;					\
> +	bl	tm_reclaim_current;	/* uint8 cause		   */	\
> +1:
> +
> +#else /* CONFIG_PPC_TRANSACTIONAL_MEM */
> +#define TM_KERNEL_ENTRY
> +#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> +
>  #ifdef CONFIG_RELOCATABLE
>  #define __EXCEPTION_RELON_PROLOG_PSERIES_1(label, h)			\
>  	mfspr	r11,SPRN_##h##SRR0;	/* save SRR0 */			\
> @@ -675,6 +696,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
>  	EXCEPTION_PROLOG_COMMON(trap, area);			\
>  	/* Volatile regs are potentially clobbered here */	\
>  	additions;						\
> +	/* This is going to need to go somewhere else as well */\
> +	/* See comment in tm_recheckpoint()		      */\
> +	TM_KERNEL_ENTRY;					\
>  	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
>  	bl	hdlr;						\
>  	b	ret
> @@ -689,6 +713,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
>  	EXCEPTION_PROLOG_COMMON_3(trap);			\
>  	/* Volatile regs are potentially clobbered here */	\
>  	additions;						\
> +	TM_KERNEL_ENTRY;					\
>  	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
>  	bl	hdlr
> =20
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_6=
4.S
> index 2cb5109a7ea3..107c15c6f48b 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -126,6 +126,11 @@ BEGIN_FW_FTR_SECTION
>  33:
>  END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
>  #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE && CONFIG_PPC_SPLPAR */
> +	TM_KERNEL_ENTRY
> +	REST_GPR(0,r1)
> +	REST_4GPRS(3,r1)
> +	REST_2GPRS(7,r1)
> +	addi	r9,r1,STACK_FRAME_OVERHEAD

Why are we doing these restores here now?

> =20
>  	/*
>  	 * A syscall should always be called with interrupts enabled
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index 77dc6d8288eb..ea75da0fd506 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -951,6 +951,23 @@ void tm_recheckpoint(struct thread_struct *thread)
>  	if (!(thread->regs->msr & MSR_TM))
>  		return;
> =20
> +	/*
> +	 * This is 'that' comment.

I think I'm in the loop here but I don't actually know what this means.=20

Senior Mikey moment or Crazy Cyril comments? I'll let the peanut gallery de=
cide.

> +	 *
> +	 * If we get where with tm suspended or active then something

s/where/here/

> +	 * has gone wrong. I've added this now as a proof of concept.
> +	 *
> +	 * The problem I'm seeing without it is an attempt to
> +	 * recheckpoint a CPU without a previous reclaim.
> +	 *
> +	 * I'm probably missed an exception entry with the
> +	 * TM_KERNEL_ENTRY macro. Should be easy enough to find.
> +	 */
> +	if (MSR_TM_ACTIVE(mfmsr()))
> +		return;

I don't really get this.  Wouldn't this test apply now?

> +
> +	tm_enable();

Why did we add this?

> +
>  	/* We really can't be interrupted here as the TEXASR registers can't
>  	 * change and later in the trecheckpoint code, we have a userspace R1.
>  	 * So let's hard disable over this region.
> @@ -1009,6 +1026,13 @@ static inline void tm_recheckpoint_new_task(struct=
 task_struct *new)
>  static inline void __switch_to_tm(struct task_struct *prev,
>  		struct task_struct *new)
>  {
> +	/*
> +	 * So, with the rework none of this code should not be needed.
> +	 * I've left in the reclaim for now. This *should* save us
> +	 * from any mistake in the new code. Also the
> +	 * enabling/disabling logic of MSR_TM really should be
> +	 * refactored into a common way with MSR_{FP,VEC,VSX}
> +	 */
>  	if (cpu_has_feature(CPU_FTR_TM)) {
>  		if (tm_enabled(prev) || tm_enabled(new))
>  			tm_enable();
> @@ -1016,11 +1040,14 @@ static inline void __switch_to_tm(struct task_str=
uct *prev,
>  		if (tm_enabled(prev)) {
>  			prev->thread.load_tm++;
>  			tm_reclaim_task(prev);
> -			if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm =
=3D=3D 0)
> -				prev->thread.regs->msr &=3D ~MSR_TM;
> +			/*
> +			 * The disabling logic may be confused don't
> +			 * disable for now
> +			 *
> +			 * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm=
 =3D=3D 0)
> +			 *	prev->thread.regs->msr &=3D ~MSR_TM;
> +			 */

Why are you doing this when you just remove all this code in the next patch=
?
>  		}
> -
> -		tm_recheckpoint_new_task(new);
>  	}
>  }
> =20
> @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
>  	msr_diff =3D current->thread.ckpt_regs.msr & ~regs->msr;
>  	msr_diff &=3D MSR_FP | MSR_VEC | MSR_VSX;
> =20
> +	tm_recheckpoint(&current->thread);
> +

So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah code =
go
away in process.c after all this?

>  	/* Ensure that restore_math() will restore */
>  	if (msr_diff & MSR_FP)
>  		current->thread.load_fp =3D 1;

Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit

[Cyril Bur]

On Tue, 2018-02-20 at 13:50 +1100, Michael Neuling wrote:
> On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> 
> 
> The comment from the cover sheet should be here
> 
> > ---
> >  arch/powerpc/include/asm/exception-64s.h | 25 +++++++++++++++++++++
> >  arch/powerpc/kernel/entry_64.S           |  5 +++++
> >  arch/powerpc/kernel/process.c            | 37 ++++++++++++++++++++++++++++----
> >  3 files changed, 63 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> > index 471b2274fbeb..f904f19a9ec2 100644
> > --- a/arch/powerpc/include/asm/exception-64s.h
> > +++ b/arch/powerpc/include/asm/exception-64s.h
> > @@ -35,6 +35,7 @@
> >   * implementations as possible.
> >   */
> >  #include <asm/head-64.h>
> > +#include <asm/tm.h>
> >  
> >  /* PACA save area offsets (exgen, exmc, etc) */
> >  #define EX_R9		0
> > @@ -127,6 +128,26 @@
> >  	hrfid;								\
> >  	b	hrfi_flush_fallback
> >  
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +#define TM_KERNEL_ENTRY		                                        \
> > +	ld	r3,_MSR(r1);			                        \
> > +	/* Probably don't need to check if coming from user/kernel */	\
> > +	/* If TM is suspended or active then we must have come from*/	\
> > +	/* userspace */							\
> > +	andi.	r0,r3,MSR_PR;						\
> > +	beq	1f;							\
> > +	rldicl. r3,r3,(64-MSR_TS_LG),(64-2); /* SUSPENDED or ACTIVE*/   \
> > +	beql+	1f;                   	/* Not SUSPENDED or ACTIVE */   \
> > +	bl	save_nvgprs;						\
> > +	RECONCILE_IRQ_STATE(r10,r11);					\
> > +	li	r3,TM_CAUSE_MISC;					\
> > +	bl	tm_reclaim_current;	/* uint8 cause		   */	\
> > +1:
> > +
> > +#else /* CONFIG_PPC_TRANSACTIONAL_MEM */
> > +#define TM_KERNEL_ENTRY
> > +#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> > +
> >  #ifdef CONFIG_RELOCATABLE
> >  #define __EXCEPTION_RELON_PROLOG_PSERIES_1(label, h)			\
> >  	mfspr	r11,SPRN_##h##SRR0;	/* save SRR0 */			\
> > @@ -675,6 +696,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> >  	EXCEPTION_PROLOG_COMMON(trap, area);			\
> >  	/* Volatile regs are potentially clobbered here */	\
> >  	additions;						\
> > +	/* This is going to need to go somewhere else as well */\
> > +	/* See comment in tm_recheckpoint()		      */\
> > +	TM_KERNEL_ENTRY;					\
> >  	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
> >  	bl	hdlr;						\
> >  	b	ret
> > @@ -689,6 +713,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> >  	EXCEPTION_PROLOG_COMMON_3(trap);			\
> >  	/* Volatile regs are potentially clobbered here */	\
> >  	additions;						\
> > +	TM_KERNEL_ENTRY;					\
> >  	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
> >  	bl	hdlr
> >  
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 2cb5109a7ea3..107c15c6f48b 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -126,6 +126,11 @@ BEGIN_FW_FTR_SECTION
> >  33:
> >  END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
> >  #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE && CONFIG_PPC_SPLPAR */
> > +	TM_KERNEL_ENTRY
> > +	REST_GPR(0,r1)
> > +	REST_4GPRS(3,r1)
> > +	REST_2GPRS(7,r1)
> > +	addi	r9,r1,STACK_FRAME_OVERHEAD
> 
> Why are we doing these restores here now?

The syscall handler expects the syscall params to still be in their
respective regs.

> 
> >  
> >  	/*
> >  	 * A syscall should always be called with interrupts enabled
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 77dc6d8288eb..ea75da0fd506 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -951,6 +951,23 @@ void tm_recheckpoint(struct thread_struct *thread)
> >  	if (!(thread->regs->msr & MSR_TM))
> >  		return;
> >  
> > +	/*
> > +	 * This is 'that' comment.
> 
> I think I'm in the loop here but I don't actually know what this means. 
> 
> Senior Mikey moment or Crazy Cyril comments? I'll let the peanut gallery decide.
> 

Oh quite possibly crazy Cyril comment that will have to be...
normalised. I should actually delete this and see if that's still the
case.

> > +	 *
> > +	 * If we get where with tm suspended or active then something
> 
> s/where/here/
> 
> > +	 * has gone wrong. I've added this now as a proof of concept.
> > +	 *
> > +	 * The problem I'm seeing without it is an attempt to
> > +	 * recheckpoint a CPU without a previous reclaim.
> > +	 *
> > +	 * I'm probably missed an exception entry with the
> > +	 * TM_KERNEL_ENTRY macro. Should be easy enough to find.
> > +	 */
> > +	if (MSR_TM_ACTIVE(mfmsr()))
> > +		return;
> 
> I don't really get this.  Wouldn't this test apply now?
> 
> > +
> > +	tm_enable();
> 
> Why did we add this?
> 

Ah yes that was a cleanup I noticed along the way and clearly forgot to
finish.

At the moment there's a bunch of tm_enable()s either before calling
functions like tm_recheckpoint() or tm_reclaim_current() or inside
helpers (tm_reclaim_current() for example again). I feel like callers
shouldn't have to worry, it should be up to the function actually doing
the TM work to enable it.

> > +
> >  	/* We really can't be interrupted here as the TEXASR registers can't
> >  	 * change and later in the trecheckpoint code, we have a userspace R1.
> >  	 * So let's hard disable over this region.
> > @@ -1009,6 +1026,13 @@ static inline void tm_recheckpoint_new_task(struct task_struct *new)
> >  static inline void __switch_to_tm(struct task_struct *prev,
> >  		struct task_struct *new)
> >  {
> > +	/*
> > +	 * So, with the rework none of this code should not be needed.
> > +	 * I've left in the reclaim for now. This *should* save us
> > +	 * from any mistake in the new code. Also the
> > +	 * enabling/disabling logic of MSR_TM really should be
> > +	 * refactored into a common way with MSR_{FP,VEC,VSX}
> > +	 */
> >  	if (cpu_has_feature(CPU_FTR_TM)) {
> >  		if (tm_enabled(prev) || tm_enabled(new))
> >  			tm_enable();
> > @@ -1016,11 +1040,14 @@ static inline void __switch_to_tm(struct task_struct *prev,
> >  		if (tm_enabled(prev)) {
> >  			prev->thread.load_tm++;
> >  			tm_reclaim_task(prev);
> > -			if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > -				prev->thread.regs->msr &= ~MSR_TM;
> > +			/*
> > +			 * The disabling logic may be confused don't
> > +			 * disable for now
> > +			 *
> > +			 * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > +			 *	prev->thread.regs->msr &= ~MSR_TM;
> > +			 */
> 
> Why are you doing this when you just remove all this code in the next patch?

The next 3 or so patches will need squashing into this one before
merging.

> >  		}
> > -
> > -		tm_recheckpoint_new_task(new);
> >  	}
> >  }
> >  
> > @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
> >  	msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
> >  	msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
> >  
> > +	tm_recheckpoint(&current->thread);
> > +
> 
> So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah code go
> away in process.c after all this?
> 

I'm not sure I follow, we need to recheckpoint because we're going back
to userspace? Or would you rather calling the tm.S code directly from
the exception return path?

Yes, I hope we'll be able to have a fairly big cleanup commit of tm_
code in process.c at the end of this series.

> >  	/* Ensure that restore_math() will restore */
> >  	if (msr_diff & MSR_FP)
> >  		current->thread.load_fp = 1;

Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit

[Michael Neuling]

> > > @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
> > >  	msr_diff =3D current->thread.ckpt_regs.msr & ~regs->msr;
> > >  	msr_diff &=3D MSR_FP | MSR_VEC | MSR_VSX;
> > > =20
> > > +	tm_recheckpoint(&current->thread);
> > > +
> >=20
> > So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah c=
ode go
> > away in process.c after all this?
> >=20
>=20
> I'm not sure I follow, we need to recheckpoint because we're going back
> to userspace? Or would you rather calling the tm.S code directly from
> the exception return path?

Yeah, I was thinking the point of this series was.  We do tm_reclaim right =
on
entry and tm_recheckpoint right on exit. =20

The bits in between (ie. the tm_blah() calls process.c) would mostly go awa=
y.


> Yes, I hope we'll be able to have a fairly big cleanup commit of tm_
> code in process.c at the end of this series.

Yep, agreed.

Mikey

Re: [RFC PATCH 05/12] [WIP] powerpc/tm: Reclaim/recheckpoint on entry/exit

[Cyril Bur]

On Tue, 2018-02-20 at 16:25 +1100, Michael Neuling wrote:
> > > > @@ -1055,6 +1082,8 @@ void restore_tm_state(struct pt_regs *regs)
> > > >  	msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
> > > >  	msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
> > > >  
> > > > +	tm_recheckpoint(&current->thread);
> > > > +
> > > 
> > > So why do we do tm_recheckpoint at all? Shouldn't most of the tm_blah code go
> > > away in process.c after all this?
> > > 
> > 
> > I'm not sure I follow, we need to recheckpoint because we're going back
> > to userspace? Or would you rather calling the tm.S code directly from
> > the exception return path?
> 
> Yeah, I was thinking the point of this series was.  We do tm_reclaim right on
> entry and tm_recheckpoint right on exit.  
> 

Yeah that's the ultimate goal, considering I haven't been attacked or
offered more drugs I feel like what I've done isn't crazy. Your
feedback is great, thanks.

> The bits in between (ie. the tm_blah() calls process.c) would mostly go away.
> 
> 
> > Yes, I hope we'll be able to have a fairly big cleanup commit of tm_
> > code in process.c at the end of this series.
> 
> Yep, agreed.
> 
> Mikey

[RFC PATCH 06/12] [WIP] powerpc/tm: Remove dead code from __switch_to_tm()

[Cyril Bur]

---
 arch/powerpc/kernel/process.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index ea75da0fd506..574b05fe7d66 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1027,27 +1027,13 @@ static inline void __switch_to_tm(struct task_struct *prev,
 		struct task_struct *new)
 {
 	/*
-	 * So, with the rework none of this code should not be needed.
-	 * I've left in the reclaim for now. This *should* save us
-	 * from any mistake in the new code. Also the
-	 * enabling/disabling logic of MSR_TM really should be
+	 * The enabling/disabling logic of MSR_TM really should be
 	 * refactored into a common way with MSR_{FP,VEC,VSX}
 	 */
-	if (cpu_has_feature(CPU_FTR_TM)) {
-		if (tm_enabled(prev) || tm_enabled(new))
-			tm_enable();
-
-		if (tm_enabled(prev)) {
-			prev->thread.load_tm++;
-			tm_reclaim_task(prev);
-			/*
-			 * The disabling logic may be confused don't
-			 * disable for now
-			 *
-			 * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
-			 *	prev->thread.regs->msr &= ~MSR_TM;
-			 */
-		}
+	if (cpu_has_feature(CPU_FTR_TM) && tm_enabled(prev)) {
+		prev->thread.load_tm++;
+		if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
+			prev->thread.regs->msr &= ~MSR_TM;
 	}
 }
 
-- 
2.16.2

Re: [RFC PATCH 06/12] [WIP] powerpc/tm: Remove dead code from __switch_to_tm()

[Michael Neuling]

Not sure I understand this.. should it be merged with the last patch?

Needs a comment here.


On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> ---
>  arch/powerpc/kernel/process.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index ea75da0fd506..574b05fe7d66 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1027,27 +1027,13 @@ static inline void __switch_to_tm(struct task_str=
uct *prev,
>  		struct task_struct *new)
>  {
>  	/*
> -	 * So, with the rework none of this code should not be needed.
> -	 * I've left in the reclaim for now. This *should* save us
> -	 * from any mistake in the new code. Also the
> -	 * enabling/disabling logic of MSR_TM really should be
> +	 * The enabling/disabling logic of MSR_TM really should be
>  	 * refactored into a common way with MSR_{FP,VEC,VSX}
>  	 */
> -	if (cpu_has_feature(CPU_FTR_TM)) {
> -		if (tm_enabled(prev) || tm_enabled(new))
> -			tm_enable();
> -
> -		if (tm_enabled(prev)) {
> -			prev->thread.load_tm++;
> -			tm_reclaim_task(prev);
> -			/*
> -			 * The disabling logic may be confused don't
> -			 * disable for now
> -			 *
> -			 * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm=
 =3D=3D 0)
> -			 *	prev->thread.regs->msr &=3D ~MSR_TM;
> -			 */
> -		}
> +	if (cpu_has_feature(CPU_FTR_TM) && tm_enabled(prev)) {
> +		prev->thread.load_tm++;
> +		if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm =3D=
=3D 0)
> +			prev->thread.regs->msr &=3D ~MSR_TM;
>  	}
>  }
> =20

Re: [RFC PATCH 06/12] [WIP] powerpc/tm: Remove dead code from __switch_to_tm()

[Cyril Bur]

On Tue, 2018-02-20 at 13:52 +1100, Michael Neuling wrote:
> Not sure I understand this.. should it be merged with the last patch?
> 

Its all going to have to be one patch - I've left it split out to make
it more obvious which bits have had to mess with, this series
absolutely doesn't bisect.

> Needs a comment here.
> 
> 
> On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> > ---
> >  arch/powerpc/kernel/process.c | 24 +++++-------------------
> >  1 file changed, 5 insertions(+), 19 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index ea75da0fd506..574b05fe7d66 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1027,27 +1027,13 @@ static inline void __switch_to_tm(struct task_struct *prev,
> >  		struct task_struct *new)
> >  {
> >  	/*
> > -	 * So, with the rework none of this code should not be needed.
> > -	 * I've left in the reclaim for now. This *should* save us
> > -	 * from any mistake in the new code. Also the
> > -	 * enabling/disabling logic of MSR_TM really should be
> > +	 * The enabling/disabling logic of MSR_TM really should be
> >  	 * refactored into a common way with MSR_{FP,VEC,VSX}
> >  	 */
> > -	if (cpu_has_feature(CPU_FTR_TM)) {
> > -		if (tm_enabled(prev) || tm_enabled(new))
> > -			tm_enable();
> > -
> > -		if (tm_enabled(prev)) {
> > -			prev->thread.load_tm++;
> > -			tm_reclaim_task(prev);
> > -			/*
> > -			 * The disabling logic may be confused don't
> > -			 * disable for now
> > -			 *
> > -			 * if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > -			 *	prev->thread.regs->msr &= ~MSR_TM;
> > -			 */
> > -		}
> > +	if (cpu_has_feature(CPU_FTR_TM) && tm_enabled(prev)) {
> > +		prev->thread.load_tm++;
> > +		if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> > +			prev->thread.regs->msr &= ~MSR_TM;
> >  	}
> >  }
> >  

[RFC PATCH 07/12] [WIP] powerpc/tm: Add TM_KERNEL_ENTRY in more delicate exception pathes

[Cyril Bur]

---
 arch/powerpc/kernel/entry_64.S       | 15 ++++++++++++++-
 arch/powerpc/kernel/exceptions-64s.S | 31 ++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 107c15c6f48b..32e8d8f7e091 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -967,7 +967,20 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	bl	__check_irq_replay
 	cmpwi	cr0,r3,0
 	beq	.Lrestore_no_replay
- 
+
+	/*
+	 * We decide VERY late if we need to replay interrupts, theres
+	 * not much which can be done about that so this will have to
+	 * do
+	 */
+	TM_KERNEL_ENTRY
+	/*
+	 * This will restore r3 that TM_KERNEL_ENTRY clobbered.
+	 * Clearly not ideal! I wonder if we could change the trap
+	 * value beforehand...
+	 */
+	bl	__check_irq_replay
+
 	/*
 	 * We need to re-emit an interrupt. We do so by re-using our
 	 * existing exception frame. We first change the trap value,
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 3ac87e53b3da..c8899bf77fb0 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -504,6 +504,11 @@ EXC_COMMON_BEGIN(data_access_common)
 	li	r5,0x300
 	std	r3,_DAR(r1)
 	std	r4,_DSISR(r1)
+	/*
+	 * Can't do TM_KERNEL_ENTRY here as do_hash_page might jump to
+	 * very late in the expection exit code, well after any
+	 * possiblity of doing a recheckpoint
+	 */
 BEGIN_MMU_FTR_SECTION
 	b	do_hash_page		/* Try to handle as hpte fault */
 MMU_FTR_SECTION_ELSE
@@ -548,6 +553,11 @@ EXC_COMMON_BEGIN(instruction_access_common)
 	li	r5,0x400
 	std	r3,_DAR(r1)
 	std	r4,_DSISR(r1)
+	/*
+	 * Can't do TM_KERNEL_ENTRY here as do_hash_page might jump to
+	 * very late in the expection exit code, well after any
+	 * possiblity of doing a recheckpoint
+	 */
 BEGIN_MMU_FTR_SECTION
 	b	do_hash_page		/* Try to handle as hpte fault */
 MMU_FTR_SECTION_ELSE
@@ -761,6 +771,7 @@ EXC_COMMON_BEGIN(alignment_common)
 	std	r4,_DSISR(r1)
 	bl	save_nvgprs
 	RECONCILE_IRQ_STATE(r10, r11)
+	TM_KERNEL_ENTRY
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	alignment_exception
 	b	ret_from_except
@@ -1668,7 +1679,9 @@ do_hash_page:
 
 /* Here we have a page fault that hash_page can't handle. */
 handle_page_fault:
-11:	andis.  r0,r4,DSISR_DABRMATCH@h
+11:	TM_KERNEL_ENTRY
+	ld      r4,_DSISR(r1)
+	andis.  r0,r4,DSISR_DABRMATCH@h
 	bne-    handle_dabr_fault
 	ld	r4,_DAR(r1)
 	ld	r5,_DSISR(r1)
@@ -1685,6 +1698,10 @@ handle_page_fault:
 
 /* We have a data breakpoint exception - handle it */
 handle_dabr_fault:
+	/*
+	 * Don't need to do TM_KERNEL_ENTRY here as we'll
+	 * come from handle_page_fault: which has done it already
+	 */
 	bl	save_nvgprs
 	ld      r4,_DAR(r1)
 	ld      r5,_DSISR(r1)
@@ -1698,7 +1715,14 @@ handle_dabr_fault:
  * the PTE insertion
  */
 13:	bl	save_nvgprs
-	mr	r5,r3
+	/*
+	 * Use a non-volatile as the TM code will call, r3 is the
+	 * return value from __hash_page() so not exactly easy to get
+	 * again.
+	 */
+	mr	r31,r3
+	TM_KERNEL_ENTRY
+	mr	r5, r31
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	ld	r4,_DAR(r1)
 	bl	low_hash_fault
@@ -1713,7 +1737,8 @@ handle_dabr_fault:
  * the access, or panic if there isn't a handler.
  */
 77:	bl	save_nvgprs
-	mr	r4,r3
+	TM_KERNEL_ENTRY
+	ld	r4,_DAR(r1)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	li	r5,SIGSEGV
 	bl	bad_page_fault
-- 
2.16.2

[RFC PATCH 08/12] [WIP] powerpc/tm: Fix *unavailable_tm exceptions

[Cyril Bur]

---
 arch/powerpc/kernel/process.c | 11 ++++++++++-
 arch/powerpc/kernel/traps.c   |  3 ---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 574b05fe7d66..8a32fd062a2b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -500,11 +500,20 @@ void giveup_all(struct task_struct *tsk)
 
 	usermsr = tsk->thread.regs->msr;
 
+	/*
+	 * The *_unavailable_tm() functions might call this in a
+	 * transaction but with not FP or VEC or VSX meaning that the
+	 * if condition below will be true, this is bad since we will
+	 * have preformed a reclaim but not set the TIF flag which
+	 * must be set in order to trigger the recheckpoint.
+	 *
+	 * possibleTODO: Move setting the TIF flag into reclaim code
+	 */
+	check_if_tm_restore_required(tsk);
 	if ((usermsr & msr_all_available) == 0)
 		return;
 
 	msr_check_and_set(msr_all_available);
-	check_if_tm_restore_required(tsk);
 
 	WARN_ON((usermsr & MSR_VSX) && !((usermsr & MSR_FP) && (usermsr & MSR_VEC)));
 
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1e48d157196a..dccfcaf4f603 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1728,7 +1728,6 @@ void fp_unavailable_tm(struct pt_regs *regs)
 	 * If VMX is in use, the VRs now hold checkpointed values,
 	 * so we don't want to load the VRs from the thread_struct.
 	 */
-	tm_recheckpoint(&current->thread);
 }
 
 void altivec_unavailable_tm(struct pt_regs *regs)
@@ -1742,7 +1741,6 @@ void altivec_unavailable_tm(struct pt_regs *regs)
 		 regs->nip, regs->msr);
 	tm_reclaim_current(TM_CAUSE_FAC_UNAV);
 	current->thread.load_vec = 1;
-	tm_recheckpoint(&current->thread);
 	current->thread.used_vr = 1;
 }
 
@@ -1767,7 +1765,6 @@ void vsx_unavailable_tm(struct pt_regs *regs)
 	current->thread.load_vec = 1;
 	current->thread.load_fp = 1;
 
-	tm_recheckpoint(&current->thread);
 }
 #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
 
-- 
2.16.2

[RFC PATCH 09/12] [WIP] powerpc/tm: Tweak signal code to handle new reclaim/recheckpoint times

[Cyril Bur]

---
 arch/powerpc/kernel/process.c   | 13 ++++++++++++-
 arch/powerpc/kernel/signal.c    | 11 ++++++-----
 arch/powerpc/kernel/signal_32.c | 16 ++--------------
 arch/powerpc/kernel/signal_64.c | 41 +++++++++++++++++++++++++++++------------
 4 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 8a32fd062a2b..cd3ae80a6878 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1070,9 +1070,20 @@ void restore_tm_state(struct pt_regs *regs)
 	 * again, anything else could lead to an incorrect ckpt_msr being
 	 * saved and therefore incorrect signal contexts.
 	 */
-	clear_thread_flag(TIF_RESTORE_TM);
+
+	/*
+	 * So, on signals we're going to have cleared the TM bits from
+	 * the MSR, meaning that heading to userspace signal handler
+	 * this will be true.
+	 * I'm not convinced clearing the TIF_RESTORE_TM flag is a
+	 * good idea however, we should do it only if we actually
+	 * recheckpoint, which we'll need to do once the signal
+	 * hanlder is done and we're returning to the main thread of
+	 * execution.
+	 */
 	if (!MSR_TM_ACTIVE(regs->msr))
 		return;
+	clear_thread_flag(TIF_RESTORE_TM);
 
 	msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
 	msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index 61db86ecd318..4f0398c6ce03 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -191,16 +191,17 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
 	 *
 	 * For signals taken in non-TM or suspended mode, we use the
 	 * normal/non-checkpointed stack pointer.
+	 *
+	 * We now do reclaims on kernel entry, we should absolutely
+	 * never need to reclaim here.
+	 * TODO Update the comment above if needed.
 	 */
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	BUG_ON(tsk != current);
 
-	if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
-		tm_reclaim_current(TM_CAUSE_SIGNAL);
-		if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
-			return tsk->thread.ckpt_regs.gpr[1];
-	}
+	if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
+		return tsk->thread.ckpt_regs.gpr[1];
 #endif
 	return tsk->thread.regs->gpr[1];
 }
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index a46de0035214..a87a7c8b5d9e 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -860,21 +860,9 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 	tm_enable();
 	/* Make sure the transaction is marked as failed */
 	current->thread.tm_texasr |= TEXASR_FS;
-	/* This loads the checkpointed FP/VEC state, if used */
-	tm_recheckpoint(&current->thread);
 
-	/* This loads the speculative FP/VEC state, if used */
-	msr_check_and_set(msr & (MSR_FP | MSR_VEC));
-	if (msr & MSR_FP) {
-		load_fp_state(&current->thread.fp_state);
-		regs->msr |= (MSR_FP | current->thread.fpexc_mode);
-	}
-#ifdef CONFIG_ALTIVEC
-	if (msr & MSR_VEC) {
-		load_vr_state(&current->thread.vr_state);
-		regs->msr |= MSR_VEC;
-	}
-#endif
+	/* See comment in signal_64.c */
+	set_thread_flag(TIF_RESTORE_TM);
 
 	return 0;
 }
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 720117690822..a7751d1fcac6 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -568,21 +568,20 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 		}
 	}
 #endif
-	tm_enable();
 	/* Make sure the transaction is marked as failed */
 	tsk->thread.tm_texasr |= TEXASR_FS;
-	/* This loads the checkpointed FP/VEC state, if used */
-	tm_recheckpoint(&tsk->thread);
 
-	msr_check_and_set(msr & (MSR_FP | MSR_VEC));
-	if (msr & MSR_FP) {
-		load_fp_state(&tsk->thread.fp_state);
-		regs->msr |= (MSR_FP | tsk->thread.fpexc_mode);
-	}
-	if (msr & MSR_VEC) {
-		load_vr_state(&tsk->thread.vr_state);
-		regs->msr |= MSR_VEC;
-	}
+	/*
+	 * I believe this is only nessesary if the
+	 * clear_thread_flag(TIF_RESTORE_TM); in restore_tm_state()
+	 * stays before the if (!MSR_TM_ACTIVE(regs->msr).
+	 *
+	 * Actually no, we should follow the comment in
+	 * restore_tm_state() but this should ALSO be here if
+	 * if the signal handler does something crazy like 'generate'
+	 * a transaction.
+	 */
+	set_thread_flag(TIF_RESTORE_TM);
 
 	return err;
 }
@@ -734,6 +733,22 @@ int sys_rt_sigreturn(unsigned long r3, unsigned long r4, unsigned long r5,
 	if (MSR_TM_SUSPENDED(mfmsr()))
 		tm_reclaim_current(0);
 
+	/*
+	 * There is a universe where the signal handler did something
+	 * crazy like drop the transaction entirely. That is, the main
+	 * thread was in transactional or suspended mode and the
+	 * signal handler has put them in non transactional mode.
+	 * In that case we'll need to clear the TIF_RESTORE_TM flag.
+	 * I'll need to ponder it exactly but for now thats all I
+	 * think that needs to be done. At the moment it all works
+	 * because no signal hanlder is nuts enough to do it.
+	 *
+	 * Add... somewhere... I guess in the else block, in the
+	 * after the #endif
+	 *
+	 * clear_thread_flag(TIF_RESTORE_TM);
+	 */
+
 	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
 		goto badframe;
 	if (MSR_TM_ACTIVE(msr)) {
@@ -748,6 +763,8 @@ int sys_rt_sigreturn(unsigned long r3, unsigned long r4, unsigned long r5,
 	else
 	/* Fall through, for non-TM restore */
 #endif
+	clear_thread_flag(TIF_RESTORE_TM);
+
 	if (restore_sigcontext(current, NULL, 1, &uc->uc_mcontext))
 		goto badframe;
 
-- 
2.16.2

[RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs

[Cyril Bur]

---
 arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++++++++++--
 arch/powerpc/kernel/ptrace.c  |  9 +++----
 2 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index cd3ae80a6878..674f75c56172 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct *tsk)
 	return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
 }
 
+static inline void save_sprs(struct thread_struct *t);
+
 static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
 {
 	/*
@@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
 	if (!MSR_TM_SUSPENDED(mfmsr()))
 		return;
 
+	save_sprs(thr);
+
 	giveup_all(container_of(thr, struct task_struct, thread));
 
 	tm_reclaim(thr, cause);
@@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *thread)
 
 	__tm_recheckpoint(thread);
 
+	/*
+	 * This is a stripped down restore_sprs(), we need to do this
+	 * now as we might go straight out to userspace and currently
+	 * the checkpointed values are on the CPU.
+	 *
+	 * TODO: Improve
+	 */
+#ifdef CONFIG_ALTIVEC
+	if (cpu_has_feature(CPU_FTR_ALTIVEC))
+		mtspr(SPRN_VRSAVE, thread->vrsave);
+#endif
+#ifdef CONFIG_PPC_BOOK3S_64
+	if (cpu_has_feature(CPU_FTR_DSCR)) {
+		u64 dscr = get_paca()->dscr_default;
+		if (thread->dscr_inherit)
+			dscr = thread->dscr;
+
+		mtspr(SPRN_DSCR, dscr);
+	}
+
+	if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
+		/* The EBB regs aren't checkpointed */
+		mtspr(SPRN_FSCR, thread->fscr);
+
+		mtspr(SPRN_TAR, thread->tar);
+	}
+
+	/* I think we don't need to */
+	if (cpu_has_feature(CPU_FTR_ARCH_300))
+		mtspr(SPRN_TIDR, thread->tidr);
+#endif
 	local_irq_restore(flags);
 }
 
@@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_struct *prev,
 #endif
 
 	new_thread = &new->thread;
+	/*
+	 * Why not &prev->thread; ?
+	 * What is the difference between &prev->thread and
+	 * &current->thread ?
+	 */
 	old_thread = &current->thread;
 
 	WARN_ON(!irqs_disabled());
@@ -1237,8 +1277,16 @@ struct task_struct *__switch_to(struct task_struct *prev,
 	/*
 	 * We need to save SPRs before treclaim/trecheckpoint as these will
 	 * change a number of them.
+	 *
+	 * Because we're now reclaiming on kernel entry, we've had to
+	 * already save them. Don't do it again.
+	 * Note: To deliver a signal in the signal context, we'll have
+	 * turned off TM because we don't want the signal context to
+	 * have the transactional state of the main thread - what if
+	 * we go through switch to at that point? Can we?
 	 */
-	save_sprs(&prev->thread);
+	if (!prev->thread.regs || !MSR_TM_ACTIVE(prev->thread.regs->msr))
+		save_sprs(&prev->thread);
 
 	/* Save FPU, Altivec, VSX and SPE state */
 	giveup_all(prev);
@@ -1260,8 +1308,13 @@ struct task_struct *__switch_to(struct task_struct *prev,
 	 * for this is we manually create a stack frame for new tasks that
 	 * directly returns through ret_from_fork() or
 	 * ret_from_kernel_thread(). See copy_thread() for details.
+	 *
+	 * It isn't stricly nessesary that we avoid the restore here
+	 * because we'll simply restore again after the recheckpoint,
+	 * but we can avoid it for performance reasons.
 	 */
-	restore_sprs(old_thread, new_thread);
+	if (!new_thread->regs || !MSR_TM_ACTIVE(new_thread->regs->msr))
+		restore_sprs(old_thread, new_thread);
 
 	last = _switch(old_thread, new_thread);
 
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index ca72d7391d40..16001987ba71 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -135,12 +135,9 @@ static void flush_tmregs_to_thread(struct task_struct *tsk)
 	if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
 		return;
 
-	if (MSR_TM_SUSPENDED(mfmsr())) {
-		tm_reclaim_current(TM_CAUSE_SIGNAL);
-	} else {
-		tm_enable();
-		tm_save_sprs(&(tsk->thread));
-	}
+	BUG_ON(MSR_TM_SUSPENDED(mfmsr()));
+	tm_enable();
+	tm_save_sprs(&(tsk->thread));
 }
 #else
 static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }
-- 
2.16.2

Re: [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs

[Michael Neuling]

This needs a description of what you're trying to do.  "Correctly" doesn't
really mean anything.


On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> ---
>  arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++++++=
++++-
> -
>  arch/powerpc/kernel/ptrace.c  |  9 +++----
>  2 files changed, 58 insertions(+), 8 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index cd3ae80a6878..674f75c56172 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct *tsk=
)
>  	return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
>  }
> =20
> +static inline void save_sprs(struct thread_struct *t);
> +
>  static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
>  {
>  	/*
> @@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struct *t=
hr,
> uint8_t cause)
>  	if (!MSR_TM_SUSPENDED(mfmsr()))
>  		return;
> =20
> +	save_sprs(thr);
> +
>  	giveup_all(container_of(thr, struct task_struct, thread));
> =20
>  	tm_reclaim(thr, cause);
> @@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *thread)
> =20
>  	__tm_recheckpoint(thread);
> =20
> +	/*
> +	 * This is a stripped down restore_sprs(), we need to do this
> +	 * now as we might go straight out to userspace and currently
> +	 * the checkpointed values are on the CPU.
> +	 *
> +	 * TODO: Improve
> +	 */
> +#ifdef CONFIG_ALTIVEC
> +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> +		mtspr(SPRN_VRSAVE, thread->vrsave);
> +#endif
> +#ifdef CONFIG_PPC_BOOK3S_64
> +	if (cpu_has_feature(CPU_FTR_DSCR)) {
> +		u64 dscr =3D get_paca()->dscr_default;
> +		if (thread->dscr_inherit)
> +			dscr =3D thread->dscr;
> +
> +		mtspr(SPRN_DSCR, dscr);
> +	}
> +
> +	if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> +		/* The EBB regs aren't checkpointed */
> +		mtspr(SPRN_FSCR, thread->fscr);
> +
> +		mtspr(SPRN_TAR, thread->tar);
> +	}
> +
> +	/* I think we don't need to */
> +	if (cpu_has_feature(CPU_FTR_ARCH_300))
> +		mtspr(SPRN_TIDR, thread->tidr);
> +#endif

Why are you touching all the above hunk?

>  	local_irq_restore(flags);
>  }
> =20
> @@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_struct
> *prev,
>  #endif
> =20
>  	new_thread =3D &new->thread;
> +	/*
> +	 * Why not &prev->thread; ?
> +	 * What is the difference between &prev->thread and
> +	 * &current->thread ?
> +	 */

Why not just work it out and FIX THE CODE, rather than just rabbiting on ab=
out
it! :-P

>  	old_thread =3D &current->thread;
> =20
>  	WARN_ON(!irqs_disabled());
> @@ -1237,8 +1277,16 @@ struct task_struct *__switch_to(struct task_struct
> *prev,
>  	/*
>  	 * We need to save SPRs before treclaim/trecheckpoint as these will
>  	 * change a number of them.
> +	 *
> +	 * Because we're now reclaiming on kernel entry, we've had to
> +	 * already save them. Don't do it again.
> +	 * Note: To deliver a signal in the signal context, we'll have
> +	 * turned off TM because we don't want the signal context to
> +	 * have the transactional state of the main thread - what if
> +	 * we go through switch to at that point? Can we?
>  	 */
> -	save_sprs(&prev->thread);
> +	if (!prev->thread.regs || !MSR_TM_ACTIVE(prev->thread.regs->msr))
> +		save_sprs(&prev->thread);
> =20
>  	/* Save FPU, Altivec, VSX and SPE state */
>  	giveup_all(prev);
> @@ -1260,8 +1308,13 @@ struct task_struct *__switch_to(struct task_struct
> *prev,
>  	 * for this is we manually create a stack frame for new tasks that
>  	 * directly returns through ret_from_fork() or
>  	 * ret_from_kernel_thread(). See copy_thread() for details.
> +	 *
> +	 * It isn't stricly nessesary that we avoid the restore here
> +	 * because we'll simply restore again after the recheckpoint,
> +	 * but we can avoid it for performance reasons.
>  	 */
> -	restore_sprs(old_thread, new_thread);
> +	if (!new_thread->regs || !MSR_TM_ACTIVE(new_thread->regs->msr))
> +		restore_sprs(old_thread, new_thread);
> =20
>  	last =3D _switch(old_thread, new_thread);
> =20
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index ca72d7391d40..16001987ba71 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -135,12 +135,9 @@ static void flush_tmregs_to_thread(struct task_struc=
t
> *tsk)
>  	if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk !=3D current))
>  		return;
> =20
> -	if (MSR_TM_SUSPENDED(mfmsr())) {
> -		tm_reclaim_current(TM_CAUSE_SIGNAL);
> -	} else {
> -		tm_enable();
> -		tm_save_sprs(&(tsk->thread));
> -	}
> +	BUG_ON(MSR_TM_SUSPENDED(mfmsr()));
> +	tm_enable();
> +	tm_save_sprs(&(tsk->thread));
>  }
>  #else
>  static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }

Re: [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs

[Cyril Bur]

On Tue, 2018-02-20 at 14:00 +1100, Michael Neuling wrote:
> This needs a description of what you're trying to do.  "Correctly" doesn't
> really mean anything.
> 
> 
> On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> > ---
> >  arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++++++++++-
> > -
> >  arch/powerpc/kernel/ptrace.c  |  9 +++----
> >  2 files changed, 58 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index cd3ae80a6878..674f75c56172 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct *tsk)
> >  	return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> >  }
> >  
> > +static inline void save_sprs(struct thread_struct *t);
> > +
> >  static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
> >  {
> >  	/*
> > @@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struct *thr,
> > uint8_t cause)
> >  	if (!MSR_TM_SUSPENDED(mfmsr()))
> >  		return;
> >  
> > +	save_sprs(thr);
> > +
> >  	giveup_all(container_of(thr, struct task_struct, thread));
> >  
> >  	tm_reclaim(thr, cause);
> > @@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *thread)
> >  
> >  	__tm_recheckpoint(thread);
> >  
> > +	/*
> > +	 * This is a stripped down restore_sprs(), we need to do this
> > +	 * now as we might go straight out to userspace and currently
> > +	 * the checkpointed values are on the CPU.
> > +	 *
> > +	 * TODO: Improve
> > +	 */
> > +#ifdef CONFIG_ALTIVEC
> > +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > +		mtspr(SPRN_VRSAVE, thread->vrsave);
> > +#endif
> > +#ifdef CONFIG_PPC_BOOK3S_64
> > +	if (cpu_has_feature(CPU_FTR_DSCR)) {
> > +		u64 dscr = get_paca()->dscr_default;
> > +		if (thread->dscr_inherit)
> > +			dscr = thread->dscr;
> > +
> > +		mtspr(SPRN_DSCR, dscr);
> > +	}
> > +
> > +	if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> > +		/* The EBB regs aren't checkpointed */
> > +		mtspr(SPRN_FSCR, thread->fscr);
> > +
> > +		mtspr(SPRN_TAR, thread->tar);
> > +	}
> > +
> > +	/* I think we don't need to */
> > +	if (cpu_has_feature(CPU_FTR_ARCH_300))
> > +		mtspr(SPRN_TIDR, thread->tidr);
> > +#endif
> 
> Why are you touching all the above hunk?

I copied restore_sprs. I'm tidying that up now - we can't call
restore_sprs because we don't have a prev and next thread.

> 
> >  	local_irq_restore(flags);
> >  }
> >  
> > @@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_struct
> > *prev,
> >  #endif
> >  
> >  	new_thread = &new->thread;
> > +	/*
> > +	 * Why not &prev->thread; ?
> > +	 * What is the difference between &prev->thread and
> > +	 * &current->thread ?
> > +	 */
> 
> Why not just work it out and FIX THE CODE, rather than just rabbiting on about
> it! :-P

Agreed - I started to and then had a mini freakout that things would
end really badly if they're not the same. So I left that comment as a
reminder to investigate.

They should be the same though right?

> 
> >  	old_thread = &current->thread;
> >  
> >  	WARN_ON(!irqs_disabled());
> > @@ -1237,8 +1277,16 @@ struct task_struct *__switch_to(struct task_struct
> > *prev,
> >  	/*
> >  	 * We need to save SPRs before treclaim/trecheckpoint as these will
> >  	 * change a number of them.
> > +	 *
> > +	 * Because we're now reclaiming on kernel entry, we've had to
> > +	 * already save them. Don't do it again.
> > +	 * Note: To deliver a signal in the signal context, we'll have
> > +	 * turned off TM because we don't want the signal context to
> > +	 * have the transactional state of the main thread - what if
> > +	 * we go through switch to at that point? Can we?
> >  	 */
> > -	save_sprs(&prev->thread);
> > +	if (!prev->thread.regs || !MSR_TM_ACTIVE(prev->thread.regs->msr))
> > +		save_sprs(&prev->thread);
> >  
> >  	/* Save FPU, Altivec, VSX and SPE state */
> >  	giveup_all(prev);
> > @@ -1260,8 +1308,13 @@ struct task_struct *__switch_to(struct task_struct
> > *prev,
> >  	 * for this is we manually create a stack frame for new tasks that
> >  	 * directly returns through ret_from_fork() or
> >  	 * ret_from_kernel_thread(). See copy_thread() for details.
> > +	 *
> > +	 * It isn't stricly nessesary that we avoid the restore here
> > +	 * because we'll simply restore again after the recheckpoint,
> > +	 * but we can avoid it for performance reasons.
> >  	 */
> > -	restore_sprs(old_thread, new_thread);
> > +	if (!new_thread->regs || !MSR_TM_ACTIVE(new_thread->regs->msr))
> > +		restore_sprs(old_thread, new_thread);
> >  
> >  	last = _switch(old_thread, new_thread);
> >  
> > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > index ca72d7391d40..16001987ba71 100644
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -135,12 +135,9 @@ static void flush_tmregs_to_thread(struct task_struct
> > *tsk)
> >  	if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
> >  		return;
> >  
> > -	if (MSR_TM_SUSPENDED(mfmsr())) {
> > -		tm_reclaim_current(TM_CAUSE_SIGNAL);
> > -	} else {
> > -		tm_enable();
> > -		tm_save_sprs(&(tsk->thread));
> > -	}
> > +	BUG_ON(MSR_TM_SUSPENDED(mfmsr()));
> > +	tm_enable();
> > +	tm_save_sprs(&(tsk->thread));
> >  }
> >  #else
> >  static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }

Re: [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs

[Michael Neuling]

On Tue, 2018-02-20 at 14:59 +1100, Cyril Bur wrote:
> On Tue, 2018-02-20 at 14:00 +1100, Michael Neuling wrote:
> > This needs a description of what you're trying to do.  "Correctly" does=
n't
> > really mean anything.
> >=20
> >=20
> > On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> > > ---
> > >  arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++=
++++++++-
> > > -
> > >  arch/powerpc/kernel/ptrace.c  |  9 +++----
> > >  2 files changed, 58 insertions(+), 8 deletions(-)
> > >=20
> > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/proc=
ess.c
> > > index cd3ae80a6878..674f75c56172 100644
> > > --- a/arch/powerpc/kernel/process.c
> > > +++ b/arch/powerpc/kernel/process.c
> > > @@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct =
*tsk)
> > >  	return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> > >  }
> > > =20
> > > +static inline void save_sprs(struct thread_struct *t);
> > > +
> > >  static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cau=
se)
> > >  {
> > >  	/*
> > > @@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struc=
t *thr,
> > > uint8_t cause)
> > >  	if (!MSR_TM_SUSPENDED(mfmsr()))
> > >  		return;
> > > =20
> > > +	save_sprs(thr);
> > > +
> > >  	giveup_all(container_of(thr, struct task_struct, thread));
> > > =20
> > >  	tm_reclaim(thr, cause);
> > > @@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *threa=
d)
> > > =20
> > >  	__tm_recheckpoint(thread);
> > > =20
> > > +	/*
> > > +	 * This is a stripped down restore_sprs(), we need to do this
> > > +	 * now as we might go straight out to userspace and currently
> > > +	 * the checkpointed values are on the CPU.
> > > +	 *
> > > +	 * TODO: Improve
> > > +	 */
> > > +#ifdef CONFIG_ALTIVEC
> > > +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > > +		mtspr(SPRN_VRSAVE, thread->vrsave);
> > > +#endif
> > > +#ifdef CONFIG_PPC_BOOK3S_64
> > > +	if (cpu_has_feature(CPU_FTR_DSCR)) {
> > > +		u64 dscr =3D get_paca()->dscr_default;
> > > +		if (thread->dscr_inherit)
> > > +			dscr =3D thread->dscr;
> > > +
> > > +		mtspr(SPRN_DSCR, dscr);
> > > +	}
> > > +
> > > +	if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> > > +		/* The EBB regs aren't checkpointed */
> > > +		mtspr(SPRN_FSCR, thread->fscr);
> > > +
> > > +		mtspr(SPRN_TAR, thread->tar);
> > > +	}
> > > +
> > > +	/* I think we don't need to */
> > > +	if (cpu_has_feature(CPU_FTR_ARCH_300))
> > > +		mtspr(SPRN_TIDR, thread->tidr);
> > > +#endif
> >=20
> > Why are you touching all the above hunk?
>=20
> I copied restore_sprs. I'm tidying that up now - we can't call
> restore_sprs because we don't have a prev and next thread.

Yeah needs to be tided up... we can't have another copy of the code.. obvio=
usly.

>=20
> >=20
> > >  	local_irq_restore(flags);
> > >  }
> > > =20
> > > @@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_st=
ruct
> > > *prev,
> > >  #endif
> > > =20
> > >  	new_thread =3D &new->thread;
> > > +	/*
> > > +	 * Why not &prev->thread; ?
> > > +	 * What is the difference between &prev->thread and
> > > +	 * &current->thread ?
> > > +	 */
> >=20
> > Why not just work it out and FIX THE CODE, rather than just rabbiting o=
n about
> > it! :-P
>=20
> Agreed - I started to and then had a mini freakout that things would
> end really badly if they're not the same. So I left that comment as a
> reminder to investigate.
>=20
> They should be the same though right?

Should be if prev =3D=3D current.

Mikey

[RFC PATCH 11/12] [WIP] powerpc/tm: Afterthoughts

[Cyril Bur]

---
 arch/powerpc/kernel/process.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 674f75c56172..6ce41ee62b24 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1079,6 +1079,12 @@ static inline void __switch_to_tm(struct task_struct *prev,
 		if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
 			prev->thread.regs->msr &= ~MSR_TM;
 	}
+
+	/*
+	 * Now that we're reclaiming on kernel entry, we should never
+	 * get here still with user checkpointed state on the CPU
+	 */
+	BUG_ON(MSR_TM_ACTIVE(mfmsr()));
 }
 
 /*
@@ -1326,7 +1332,17 @@ struct task_struct *__switch_to(struct task_struct *prev,
 	}
 
 	if (current_thread_info()->task->thread.regs) {
-		restore_math(current_thread_info()->task->thread.regs);
+		/*
+		 * Calling this now has reloaded the live state, which
+		 * gets overwritten with the checkpointed state right
+		 * before the trecheckpoint. BUT the MSR still has
+		 * that the live state is on the CPU, which it isn't.
+		 *
+		 * restore_math(current_thread_info()->task->thread.regs);
+		 * Therefore:
+		 */
+		if (!MSR_TM_ACTIVE(current_thread_info()->task->thread.regs->msr))
+			restore_math(current_thread_info()->task->thread.regs);
 
 		/*
 		 * The copy-paste buffer can only store into foreign real
-- 
2.16.2

[RFC PATCH 12/12] [WIP] selftests/powerpc: Remove incorrect tm-syscall selftest

[Cyril Bur]

Currently we perform transactional memory work at late as possible.
That is we run in the kernel with the userspace checkpointed state on
the CPU untill we absolultely must remove it and store it away. Likely
a process switch, but possibly also signals or ptrace.

What this means is that if userspace does a system call in suspended
mode, it is possible that we will handle the system call and return
them without the need to to a reclaim/recheckpoint and so they can
expect to resume their transaction.

This is what tm-syscall tests for - the ability to perform a system
call in suspended state and still resume it afterwards.

TM reworks have meant that we now deal with any transactional state on
entry to the kernel, no matter the reason for entry (some expections
apply). We will categorically doom any suspended transaction that makes
a system call, making that transaction unresumeable.

This test will now always fail no matter what. I would like to note
here that this new behaviour does not break userspace at all. Hardware
Transactional Memory gives zero guarantee of forward progress and any
correct userspace has already had and will always have to implement a
non HTM fallback. Relying on this specific kernel behaviour also meant
relying on the stars aligning in the hardware such that there was no
cache overlaps and that it had a large enough footprint to handle
any system call without dooming a transaction.
---
 tools/testing/selftests/powerpc/tm/Makefile        |   4 +-
 .../testing/selftests/powerpc/tm/tm-syscall-asm.S  |  28 ------
 tools/testing/selftests/powerpc/tm/tm-syscall.c    | 106 ---------------------
 3 files changed, 1 insertion(+), 137 deletions(-)
 delete mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
 delete mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall.c

diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index 7a1e53297588..88d6edffcb24 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -2,7 +2,7 @@
 SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr tm-signal-context-chk-fpu \
 	tm-signal-context-chk-vmx tm-signal-context-chk-vsx
 
-TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv tm-signal-stack \
+TEST_GEN_PROGS := tm-resched-dscr tm-signal-msr-resv tm-signal-stack \
 	tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable tm-trap \
 	tm-signal-drop-transaction \
 	$(SIGNAL_CONTEXT_CHK_TESTS)
@@ -13,8 +13,6 @@ $(TEST_GEN_PROGS): ../harness.c ../utils.c
 
 CFLAGS += -mhtm
 
-$(OUTPUT)/tm-syscall: tm-syscall-asm.S
-$(OUTPUT)/tm-syscall: CFLAGS += -I../../../../../usr/include
 $(OUTPUT)/tm-tmspr: CFLAGS += -pthread
 $(OUTPUT)/tm-vmx-unavail: CFLAGS += -pthread -m64
 $(OUTPUT)/tm-resched-dscr: ../pmu/lib.o
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
deleted file mode 100644
index bd1ca25febe4..000000000000
--- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
+++ /dev/null
@@ -1,28 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#include <ppc-asm.h>
-#include <asm/unistd.h>
-
-	.text
-FUNC_START(getppid_tm_active)
-	tbegin.
-	beq 1f
-	li	r0, __NR_getppid
-	sc
-	tend.
-	blr
-1:
-	li	r3, -1
-	blr
-
-FUNC_START(getppid_tm_suspended)
-	tbegin.
-	beq 1f
-	li	r0, __NR_getppid
-	tsuspend.
-	sc
-	tresume.
-	tend.
-	blr
-1:
-	li	r3, -1
-	blr
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c
deleted file mode 100644
index 454b965a2db3..000000000000
--- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
+++ /dev/null
@@ -1,106 +0,0 @@
-/*
- * Copyright 2015, Sam Bobroff, IBM Corp.
- * Licensed under GPLv2.
- *
- * Test the kernel's system call code to ensure that a system call
- * made from within an active HTM transaction is aborted with the
- * correct failure code.
- * Conversely, ensure that a system call made from within a
- * suspended transaction can succeed.
- */
-
-#include <stdio.h>
-#include <unistd.h>
-#include <sys/syscall.h>
-#include <asm/tm.h>
-#include <sys/time.h>
-#include <stdlib.h>
-
-#include "utils.h"
-#include "tm.h"
-
-extern int getppid_tm_active(void);
-extern int getppid_tm_suspended(void);
-
-unsigned retries = 0;
-
-#define TEST_DURATION 10 /* seconds */
-#define TM_RETRIES 100
-
-pid_t getppid_tm(bool suspend)
-{
-	int i;
-	pid_t pid;
-
-	for (i = 0; i < TM_RETRIES; i++) {
-		if (suspend)
-			pid = getppid_tm_suspended();
-		else
-			pid = getppid_tm_active();
-
-		if (pid >= 0)
-			return pid;
-
-		if (failure_is_persistent()) {
-			if (failure_is_syscall())
-				return -1;
-
-			printf("Unexpected persistent transaction failure.\n");
-			printf("TEXASR 0x%016lx, TFIAR 0x%016lx.\n",
-			       __builtin_get_texasr(), __builtin_get_tfiar());
-			exit(-1);
-		}
-
-		retries++;
-	}
-
-	printf("Exceeded limit of %d temporary transaction failures.\n", TM_RETRIES);
-	printf("TEXASR 0x%016lx, TFIAR 0x%016lx.\n",
-	       __builtin_get_texasr(), __builtin_get_tfiar());
-
-	exit(-1);
-}
-
-int tm_syscall(void)
-{
-	unsigned count = 0;
-	struct timeval end, now;
-
-	SKIP_IF(!have_htm_nosc());
-
-	setbuf(stdout, NULL);
-
-	printf("Testing transactional syscalls for %d seconds...\n", TEST_DURATION);
-
-	gettimeofday(&end, NULL);
-	now.tv_sec = TEST_DURATION;
-	now.tv_usec = 0;
-	timeradd(&end, &now, &end);
-
-	for (count = 0; timercmp(&now, &end, <); count++) {
-		/*
-		 * Test a syscall within a suspended transaction and verify
-		 * that it succeeds.
-		 */
-		FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */
-
-		/*
-		 * Test a syscall within an active transaction and verify that
-		 * it fails with the correct failure code.
-		 */
-		FAIL_IF(getppid_tm(false) != -1);  /* Should fail... */
-		FAIL_IF(!failure_is_persistent()); /* ...persistently... */
-		FAIL_IF(!failure_is_syscall());    /* ...with code syscall. */
-		gettimeofday(&now, 0);
-	}
-
-	printf("%d active and suspended transactions behaved correctly.\n", count);
-	printf("(There were %d transaction retries.)\n", retries);
-
-	return 0;
-}
-
-int main(void)
-{
-	return test_harness(tm_syscall, "tm_syscall");
-}
-- 
2.16.2

Re: [RFC PATCH 12/12] [WIP] selftests/powerpc: Remove incorrect tm-syscall selftest

[Michael Neuling]

> --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
> +++ /dev/null
> @@ -1,106 +0,0 @@
> -/*
> - * Copyright 2015, Sam Bobroff, IBM Corp.
> - * Licensed under GPLv2.
> - *
> - * Test the kernel's system call code to ensure that a system call
> - * made from within an active HTM transaction is aborted with the
> - * correct failure code.

The above is still true

> - * Conversely, ensure that a system call made from within a
> - * suspended transaction can succeed.

This is true anymore....

So can we just modify the test to remove the second part?

Mikey

Re: [RFC PATCH 12/12] [WIP] selftests/powerpc: Remove incorrect tm-syscall selftest

[Cyril Bur]

On Tue, 2018-02-20 at 14:04 +1100, Michael Neuling wrote:
> > --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
> > +++ /dev/null
> > @@ -1,106 +0,0 @@
> > -/*
> > - * Copyright 2015, Sam Bobroff, IBM Corp.
> > - * Licensed under GPLv2.
> > - *
> > - * Test the kernel's system call code to ensure that a system call
> > - * made from within an active HTM transaction is aborted with the
> > - * correct failure code.
> 
> The above is still true
> 
> > - * Conversely, ensure that a system call made from within a
> > - * suspended transaction can succeed.
> 
> This is true anymore....
> 
> So can we just modify the test to remove the second part?
> 

Oh true I overlooked that

Thanks

> Mikey

Re: [RFC,00/12] Deal with TM on kernel entry and exit

[Breno Leitao]

Hi Cyril,

On 02/19/2018 09:22 PM, Cyril Bur wrote:
> This is very much a proof of concept and if it isn't clear from the
> commit names, still a work in progress.

> I believe I have something that works - all the powerpc selftests
> pass. I would like to get some eyes on it to a) see if I've missed
> anything big and b) some opinions on if it is looking like a net
> improvement.

I started to look at this patchset. The patchset apply cleanly on top of the
current kernel and I started to run some tests.

It seems there is a different behavior when there is a trap (as a illegal
instruction or a 'trap' instruction) inside the transaction.

In this case, the signal handler does not seem to be called, and and the task
segfaults. On current upstream, the signal handler is called and the program
can continue.

4.17 pristine
-------------
	$ ./illegal
	Failure

4.17 plus your patches
----------------------
	$ ./illegal
	[1]    2504 segmentation fault  ./illegal


Here is a minimal example that is able to recreate this behaviour:

	#include <stdio.h>
	#include <signal.h>
	
	int htm(){
		asm goto ("tbegin.  		\n\t"
			  "beq %l[failure]	\n\t"
			  "li 3, 3		\n\t"
			  "trap 		\n\t"
			  "tend. 		\n\t"
			  : : : : failure);
	
		return 0;
	failure:
		printf("Failure\n");
		return 1;
	}
	
	void signal_handler(int signo, siginfo_t *si, void *data) {
		// Do nothing
	}
	
	int main(){
		struct sigaction sa;
	        sa.sa_flags = SA_SIGINFO;
	        sa.sa_sigaction = signal_handler;
	
	        sigaction(SIGTRAP, &sa, NULL);
	        sigaction(SIGILL,  &sa, NULL);
	
		return htm();
	}