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

@@ -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 */

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

@@ -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
 

@@ -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

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

@@ -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

@@ -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");
+}

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

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

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

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

@@ -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,

@@ -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

@@ -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);

@@ -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()");
 

@@ -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

@@ -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)));
 

@@ -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 */
 

@@ -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);
 

@@ -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,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

@@ -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

@@ -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");
-}