[RFC PATCH 00/11] New TM Model

[RFC PATCH 00/11] New TM Model

[Breno Leitao]

This patchset for the hardware transactional memory (TM) subsystem aims to
avoid spending a lot of time on TM suspended mode in kernel space. It basically
changes where the reclaim/recheckpoint will be executed.

Once a CPU enters in transactional state it uses a footprint area to track
down the load/stores performed in transaction so it can be verified later
to decide if a conflict happened due to some change done in that state. If
a transaction is active in userspace and there is an exception that takes
the CPU to the kernel space the CPU moves the transaction to suspended
state but does not discard the footprint area.

POWER9 has a known problem[1][2] and does not have enough room in
footprint area for several transactions to be suspended at the same time
on concurrent CPUs leading to CPU stalls.

This patchset aims to reclaim the checkpointed footprint as soon as the
kernel is invoked, in the beginning of the exception handlers, thus freeing
room to other CPUs enter in suspended mode, avoiding too many CPUs in suspended
state that can cause the CPUs to stall. The same mechanism is done on kernel
exit, doing a recheckpoint as late as possible (which will reload the
checkpointed state into CPU's room) at the exception return.

The way to achieve this goal is creating a macro (TM_KERNEL_ENTRY) which
will check if userspace was in an active transaction just after getting
into kernel and reclaiming if that's the case. Thus all exception handlers
will call this macro as soon as possible.

All exceptions should reclaim (if necessary) at this stage and only
recheckpoint if the task is tagged as TIF_RESTORE_TM (i.e. was in
transactional state before being interrupted), which will be done at
ret_from_except_lite().

Ideally all reclaims will happen at the exception entrance, however during
the recheckpoint process another exception can hit the CPU which might
cause the current thread to be rescheduled, thus there is another reclaim
point to be considered at __switch_to().

Hence, by allowing the CPU to be in suspended state for only a brief period
it's possible to cope with the TM hardware limitations like the current
problem on the new POWER9.

This patchset was tested in different scenarios using different test
suites, as the kernel selftests and htm-torture[3], in the following
configuration:

 * POWER8/pseries LE and BE
 * POWER8/powernv LE
 * POWER9/powernv LE hosting KVM guests running TM tests

This patchset is based on initial work done by Cyril Bur:
    https://patchwork.ozlabs.org/cover/875341/

Regards,
Breno

[1] Documentation/powerpc/transactional_memory.txt
[2] commit 4bb3c7a0208fc13ca70598efd109901a7cd45ae7
[3] https://github.com/leitao/htm_torture/

Breno Leitao (11):
  powerpc/tm: Reclaim transaction on kernel entry
  powerpc/tm: Reclaim on unavailable exception
  powerpc/tm: Recheckpoint when exiting from kernel
  powerpc/tm: Always set TIF_RESTORE_TM on reclaim
  powerpc/tm: Function that updates the failure code
  powerpc/tm: Refactor the __switch_to_tm code
  powerpc/tm: Do not recheckpoint at sigreturn
  powerpc/tm: Do not reclaim on ptrace
  powerpc/tm: Do not restore default DSCR
  powerpc/tm: Set failure summary
  selftests/powerpc: Adapt the test

 arch/powerpc/include/asm/exception-64s.h      |  46 +++++
 arch/powerpc/kernel/entry_64.S                |  10 +
 arch/powerpc/kernel/exceptions-64s.S          |  15 +-
 arch/powerpc/kernel/process.c                 | 185 +++++++++---------
 arch/powerpc/kernel/ptrace.c                  |  10 +-
 arch/powerpc/kernel/signal_32.c               |  25 +--
 arch/powerpc/kernel/signal_64.c               |  17 +-
 arch/powerpc/kernel/tm.S                      |   4 -
 arch/powerpc/kernel/traps.c                   |  16 +-
 .../testing/selftests/powerpc/tm/tm-syscall.c |   6 -
 10 files changed, 178 insertions(+), 156 deletions(-)

-- 
2.19.0

[RFC PATCH 01/11] powerpc/tm: Reclaim transaction on kernel entry

[Breno Leitao]

This patch creates a macro that will be invoked on all entrance to the
kernel, so, in kernel space the transaction will be completely reclaimed
and not suspended anymore.

This patchset checks if we are coming from PR, if not, skip. This is useful
when there is a irq_replay() being called after recheckpoint, when the IRQ
is re-enable. In this case, we do not want to re-reclaim and
re-recheckpoint, thus, if not coming from PR, skip it completely.

This macro does not care about TM SPR also, it will only be saved and
restore in the context switch code now on.

This macro will return 0 or 1 in r3 register, to specify if a reclaim was
executed or not.

This patchset is based on initial work done by Cyril:
https://patchwork.ozlabs.org/cover/875341/

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/include/asm/exception-64s.h | 46 ++++++++++++++++++++++++
 arch/powerpc/kernel/entry_64.S           | 10 ++++++
 arch/powerpc/kernel/exceptions-64s.S     | 12 +++++--
 3 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index a86feddddad0..db90b6d7826e 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -36,6 +36,7 @@
  */
 #include <asm/head-64.h>
 #include <asm/feature-fixups.h>
+#include <asm/tm.h>
 
 /* PACA save area offsets (exgen, exmc, etc) */
 #define EX_R9		0
@@ -686,10 +687,54 @@ BEGIN_FTR_SECTION				\
 	beql	ppc64_runlatch_on_trampoline;	\
 END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
 
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+
+/*
+ * This macro will reclaim a transaction if called when coming from userspace
+ * (MSR.PR = 1) and if the transaction state is active or suspended.
+ *
+ * Since we don't want to reclaim when coming from kernel, for instance after
+ * a trechkpt. or a IRQ replay, the live MSR is not useful and instead of it the
+ * MSR from thread stack is used to check the MSR.PR bit.
+ * This macro has one argument which is the cause that will be used by treclaim.
+ * and returns in r3 '1' if the reclaim happens or '0' if reclaim didn't
+ * happen, which is useful to know what registers were clobbered.
+ *
+ * NOTE: If addition registers are clobbered here, make sure the callee
+ * function restores them before proceeding.
+ */
+#define TM_KERNEL_ENTRY(cause)						\
+	ld      r3, _MSR(r1);						\
+	andi.   r0, r3, MSR_PR;	/* Coming from userspace? */		\
+	beq     1f;		/* Skip reclaim if MSR.PR != 1 */	\
+	rldicl. r0, r3, (64-MSR_TM_LG), 63; /* Is TM enabled? */	\
+	beq     1f;		/* Skip reclaim if TM is off */		\
+	rldicl. r0, r3, (64-MSR_TS_LG), 62;	/* Is active */		\
+	beq     1f;		/* Skip reclaim if neither */		\
+	/*								\
+	 * If there is a transaction active or suspended, save the	\
+	 * non-volatile GPRs if they are not already saved.		\
+	 */								\
+	bl      save_nvgprs;						\
+	/*								\
+	 * Soft disable the IRQs, otherwise it might cause a CPU hang.	\
+	 */								\
+	RECONCILE_IRQ_STATE(r10, r11);					\
+	li      r3, cause;						\
+	bl      tm_reclaim_current;					\
+	li      r3, 1;		/* Reclaim happened */			\
+	b       2f;							\
+1:	li      r3, 0;		/* Reclaim didn't happen */		\
+2:
+#else
+#define TM_KERNEL_ENTRY(cause)
+#endif
+
 #define EXCEPTION_COMMON(area, trap, label, hdlr, ret, additions) \
 	EXCEPTION_PROLOG_COMMON(trap, area);			\
 	/* Volatile regs are potentially clobbered here */	\
 	additions;						\
+	TM_KERNEL_ENTRY(TM_CAUSE_MISC);				\
 	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
 	bl	hdlr;						\
 	b	ret
@@ -704,6 +749,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
 	EXCEPTION_PROLOG_COMMON_3(trap);			\
 	/* Volatile regs are potentially clobbered here */	\
 	additions;						\
+	TM_KERNEL_ENTRY(TM_CAUSE_MISC);				\
 	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 2206912ea4f0..c38677b7442c 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -131,6 +131,16 @@ BEGIN_FW_FTR_SECTION
 END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE && CONFIG_PPC_SPLPAR */
 
+#if CONFIG_PPC_TRANSACTIONAL_MEM
+	TM_KERNEL_ENTRY(TM_CAUSE_SYSCALL)
+	cmpdi	r3, 0x1
+	bne	44f
+	/* Restore from r4 to r12 */
+	REST_8GPRS(4,r1)
+44:	/* treclaim was not called, just restore r3 and r0 */
+	REST_GPR(3, r1)
+	REST_GPR(0, r1)
+#endif
 	/*
 	 * A syscall should always be called with interrupts enabled
 	 * so we just unconditionally hard-enable here. When some kind
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index ea04dfb8c092..78aba71a4b2d 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -805,6 +805,7 @@ EXC_COMMON_BEGIN(alignment_common)
 	std	r3,_DAR(r1)
 	std	r4,_DSISR(r1)
 	bl	save_nvgprs
+	TM_KERNEL_ENTRY(TM_CAUSE_ALIGNMENT)
 	RECONCILE_IRQ_STATE(r10, r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	alignment_exception
@@ -839,6 +840,8 @@ EXC_COMMON_BEGIN(program_check_common)
 	b 3f				/* Jump into the macro !!	*/
 1:	EXCEPTION_PROLOG_COMMON(0x700, PACA_EXGEN)
 	bl	save_nvgprs
+	ld      r3, _MSR(r1)
+	TM_KERNEL_ENTRY(TM_CAUSE_FAC_UNAV)
 	RECONCILE_IRQ_STATE(r10, r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	program_check_exception
@@ -1738,7 +1741,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(TM_CAUSE_TLBI)
+	ld      r4,_DSISR(r1)
+	andis.  r0,r4,DSISR_DABRMATCH@h
 	bne-    handle_dabr_fault
 	ld	r4,_DAR(r1)
 	ld	r5,_DSISR(r1)
@@ -1769,6 +1774,8 @@ handle_dabr_fault:
  */
 13:	bl	save_nvgprs
 	mr	r5,r3
+	TM_KERNEL_ENTRY(TM_CAUSE_TLBI)
+	REST_GPR(3,r1)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	ld	r4,_DAR(r1)
 	bl	low_hash_fault
@@ -1783,7 +1790,8 @@ handle_dabr_fault:
  * the access, or panic if there isn't a handler.
  */
 77:	bl	save_nvgprs
-	mr	r4,r3
+	TM_KERNEL_ENTRY(TM_CAUSE_TLBI)
+	ld      r4,_DAR(r1)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	li	r5,SIGSEGV
 	bl	bad_page_fault
-- 
2.19.0

[RFC PATCH 02/11] powerpc/tm: Reclaim on unavailable exception

[Breno Leitao]

If there is a FP/VEC/Altivec touch inside a transaction and the facility is
disabled, then a facility unavailable exception is raised and ends up
calling {fp,vec,vsx}_unavailable_tm, which was reclaiming and recheckpointing.

This is not required anymore, since the checkpointed state was reclaimed in
the trap entrace itself, and it will be recheckpointed by restore_tm_state
later.

With this new patchset, we only reclaim at exception entrance (except at
syscall level if the transaction is suspended), and always recheckpoint on
restore_tm_state.

Adding a WARN_ON() warning if we hit the _unavailable_tm() in suspended
mode, i.e, the reclaim was not executed somehow in the trap entrance.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/exceptions-64s.S |  3 +++
 arch/powerpc/kernel/traps.c          | 16 ++++------------
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 78aba71a4b2d..4108f3944bdd 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -874,6 +874,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_TM)
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 2:	/* User process was in a transaction */
 	bl	save_nvgprs
+	TM_KERNEL_ENTRY(TM_CAUSE_FAC_UNAV)
 	RECONCILE_IRQ_STATE(r10, r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	fp_unavailable_tm
@@ -1216,6 +1217,7 @@ BEGIN_FTR_SECTION
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 2:	/* User process was in a transaction */
 	bl	save_nvgprs
+	TM_KERNEL_ENTRY(TM_CAUSE_FAC_UNAV)
 	RECONCILE_IRQ_STATE(r10, r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	altivec_unavailable_tm
@@ -1252,6 +1254,7 @@ BEGIN_FTR_SECTION
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 2:	/* User process was in a transaction */
 	bl	save_nvgprs
+	TM_KERNEL_ENTRY(TM_CAUSE_FAC_UNAV)
 	RECONCILE_IRQ_STATE(r10, r11)
 	addi	r3,r1,STACK_FRAME_OVERHEAD
 	bl	vsx_unavailable_tm
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index c85adb858271..b973fdb72826 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1749,19 +1749,12 @@ void fp_unavailable_tm(struct pt_regs *regs)
          * transaction, and probably retry but now with FP enabled.  So the
          * checkpointed FP registers need to be loaded.
 	 */
-	tm_reclaim_current(TM_CAUSE_FAC_UNAV);
+	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
 	/* Reclaim didn't save out any FPRs to transact_fprs. */
 
 	/* Enable FP for the task: */
 	current->thread.load_fp = 1;
 
-	/* This loads and recheckpoints the FP registers from
-	 * thread.fpr[].  They will remain in registers after the
-	 * checkpoint so we don't need to reload them after.
-	 * 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)
@@ -1773,10 +1766,10 @@ void altivec_unavailable_tm(struct pt_regs *regs)
 	TM_DEBUG("Vector Unavailable trap whilst transactional at 0x%lx,"
 		 "MSR=%lx\n",
 		 regs->nip, regs->msr);
-	tm_reclaim_current(TM_CAUSE_FAC_UNAV);
+	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
 	current->thread.load_vec = 1;
-	tm_recheckpoint(&current->thread);
 	current->thread.used_vr = 1;
+
 }
 
 void vsx_unavailable_tm(struct pt_regs *regs)
@@ -1795,12 +1788,11 @@ void vsx_unavailable_tm(struct pt_regs *regs)
 	current->thread.used_vsr = 1;
 
 	/* This reclaims FP and/or VR regs if they're already enabled */
-	tm_reclaim_current(TM_CAUSE_FAC_UNAV);
+	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
 
 	current->thread.load_vec = 1;
 	current->thread.load_fp = 1;
 
-	tm_recheckpoint(&current->thread);
 }
 #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
 
-- 
2.19.0

[RFC PATCH 03/11] powerpc/tm: Recheckpoint when exiting from kernel

[Breno Leitao]

This is the only place we are going to recheckpoint now. Now the task
needs to have TIF_RESTORE_TM flag set, which will get into
restore_tm_state() and execute the recheckpoint if MSR shows that the
transaction was active.

Every time a task is required to recheckpoint, or just have the TM SPRs
restore, the TIF_RESTORE_TM flag should be set and the task MSR should
properly be in a transactional state, which will be checked by
restore_tm_state().

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/process.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 913c5725cdb2..f22f82ce174c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1069,6 +1069,10 @@ void restore_tm_state(struct pt_regs *regs)
 	if (!MSR_TM_ACTIVE(regs->msr))
 		return;
 
+	tm_enable();
+	/* The only place we recheckpoint */
+	tm_recheckpoint(&current->thread);
+
 	msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
 	msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
 
-- 
2.19.0

[RFC PATCH 04/11] powerpc/tm: Always set TIF_RESTORE_TM on reclaim

[Breno Leitao]

If the task data was reclaimed (through TM_KERNEL_ENTRY), then it needs to
be recheckpointed later, once exiting to userspace. The recheckpoint is
done by restore_tm_state() function, which is called on our way to
userspace if the task has the TIF_RESTORE_TM flag set.

This patch makes sure the task has TIF_RESTORE_TM tag set every time there
is a reclaim, so, a recheckpoint will be executed later.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/process.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f22f82ce174c..54fddf03b97a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -891,6 +891,9 @@ static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause)
 
 	tm_reclaim(thr, cause);
 
+	/* Tag it so restore_tm_state will pay attention to this task */
+	set_thread_flag(TIF_RESTORE_TM);
+
 	/*
 	 * If we are in a transaction and FP is off then we can't have
 	 * used FP inside that transaction. Hence the checkpointed
-- 
2.19.0

[RFC PATCH 05/11] powerpc/tm: Function that updates the failure code

[Breno Leitao]

Now the transaction reclaims happens very earlier in the trap handler, and
it is impossible to know precisely, at that early time, what should be set
as the failure cause for some specific cases, as, if the task will be
rescheduled, thus, the transaction abort case should be updated from
TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.

This patch creates a function that will update TEXASR special purpose
register in the task thread and set the failure code which will be
moved to the live register afterward.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/process.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 54fddf03b97a..fe063c0142e3 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -85,6 +85,7 @@ extern unsigned long _get_SP(void);
  * other paths that we should never reach with suspend disabled.
  */
 bool tm_suspend_disabled __ro_after_init = false;
+static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause);
 
 static void check_if_tm_restore_required(struct task_struct *tsk)
 {
@@ -988,6 +989,14 @@ void tm_recheckpoint(struct thread_struct *thread)
 	local_irq_restore(flags);
 }
 
+/* Change thread->tm.texasr failure code */
+static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause)
+{
+	/* Clear the cause first */
+	task->thread.tm_texasr &= ~TEXASR_FC;
+	task->thread.tm_texasr |= (unsigned long) cause << 56;
+}
+
 static inline void tm_recheckpoint_new_task(struct task_struct *new)
 {
 	if (!cpu_has_feature(CPU_FTR_TM))
-- 
2.19.0

[RFC PATCH 06/11] powerpc/tm: Refactor the __switch_to_tm code

[Breno Leitao]

__switch_to_tm is the function that switches between two tasks which might
have TM enabled. This function is clearly split in two parts, the task that
is leaving the CPU, known as 'prev' and the task that is being scheduled,
known as new.

It starts checking if the previous task had TM enable, if so, it increases
the load_tm (this is the only place we increment load_tm). It also saves
the TM SPRs here.

If the previous task was scheduled out with a transaction active, the
failure cause needs to be updated, since it might contain the failure cause
that caused the exception, as TM_CAUSE_MISC. In this case, since there was
a context switch, overwrite the failure cause.

If the previous task has overflowed load_tm, disable TM, putting the
facility save/restore lazy mechanism at lazy mode.

Regarding the new task, when loading it, it basically restore the SPRs, and
TIF_RESTORE_TM (already set by tm_reclaim_current if the transaction was
active) would invoke the recheckpoint process later in restore_tm_state()
if recheckpoint is somehow required.

On top of that, both tm_reclaim_task() and tm_recheckpoint_new_task()
functions are not used anymore, removing them.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/process.c | 163 +++++++++++++++-------------------
 1 file changed, 74 insertions(+), 89 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index fe063c0142e3..5cace1b744b1 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -921,48 +921,6 @@ void tm_reclaim_current(uint8_t cause)
 	tm_reclaim_thread(&current->thread, cause);
 }
 
-static inline void tm_reclaim_task(struct task_struct *tsk)
-{
-	/* We have to work out if we're switching from/to a task that's in the
-	 * middle of a transaction.
-	 *
-	 * In switching we need to maintain a 2nd register state as
-	 * oldtask->thread.ckpt_regs.  We tm_reclaim(oldproc); this saves the
-	 * checkpointed (tbegin) state in ckpt_regs, ckfp_state and
-	 * ckvr_state
-	 *
-	 * We also context switch (save) TFHAR/TEXASR/TFIAR in here.
-	 */
-	struct thread_struct *thr = &tsk->thread;
-
-	if (!thr->regs)
-		return;
-
-	if (!MSR_TM_ACTIVE(thr->regs->msr))
-		goto out_and_saveregs;
-
-	WARN_ON(tm_suspend_disabled);
-
-	TM_DEBUG("--- tm_reclaim on pid %d (NIP=%lx, "
-		 "ccr=%lx, msr=%lx, trap=%lx)\n",
-		 tsk->pid, thr->regs->nip,
-		 thr->regs->ccr, thr->regs->msr,
-		 thr->regs->trap);
-
-	tm_reclaim_thread(thr, TM_CAUSE_RESCHED);
-
-	TM_DEBUG("--- tm_reclaim on pid %d complete\n",
-		 tsk->pid);
-
-out_and_saveregs:
-	/* Always save the regs here, even if a transaction's not active.
-	 * This context-switches a thread's TM info SPRs.  We do it here to
-	 * be consistent with the restore path (in recheckpoint) which
-	 * cannot happen later in _switch().
-	 */
-	tm_save_sprs(thr);
-}
-
 extern void __tm_recheckpoint(struct thread_struct *thread);
 
 void tm_recheckpoint(struct thread_struct *thread)
@@ -997,59 +955,87 @@ static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause)
 	task->thread.tm_texasr |= (unsigned long) cause << 56;
 }
 
-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)
 {
 	if (!cpu_has_feature(CPU_FTR_TM))
 		return;
 
-	/* Recheckpoint the registers of the thread we're about to switch to.
-	 *
-	 * If the task was using FP, we non-lazily reload both the original and
-	 * the speculative FP register states.  This is because the kernel
-	 * doesn't see if/when a TM rollback occurs, so if we take an FP
-	 * unavailable later, we are unable to determine which set of FP regs
-	 * need to be restored.
-	 */
-	if (!tm_enabled(new))
-		return;
-
-	if (!MSR_TM_ACTIVE(new->thread.regs->msr)){
-		tm_restore_sprs(&new->thread);
-		return;
-	}
-	/* Recheckpoint to restore original checkpointed register state. */
-	TM_DEBUG("*** tm_recheckpoint of pid %d (new->msr 0x%lx)\n",
-		 new->pid, new->thread.regs->msr);
-
-	tm_recheckpoint(&new->thread);
-
-	/*
-	 * The checkpointed state has been restored but the live state has
-	 * not, ensure all the math functionality is turned off to trigger
-	 * restore_math() to reload.
-	 */
-	new->thread.regs->msr &= ~(MSR_FP | MSR_VEC | MSR_VSX);
+	/* The task leaving the CPU was using TM, let's handle it */
+	if (tm_enabled(prev)) {
+		/*
+		 * Load_tm is incremented only when the task is scheduled out
+		 */
+		prev->thread.load_tm++;
 
-	TM_DEBUG("*** tm_recheckpoint of pid %d complete "
-		 "(kernel msr 0x%lx)\n",
-		 new->pid, mfmsr());
-}
+		/*
+		 * If TM is enabled for the thread, it needs to, at least,
+		 * save the SPRs
+		 */
+		tm_enable();
+		tm_save_sprs(&prev->thread);
 
-static inline void __switch_to_tm(struct task_struct *prev,
-		struct task_struct *new)
-{
-	if (cpu_has_feature(CPU_FTR_TM)) {
-		if (tm_enabled(prev) || tm_enabled(new))
-			tm_enable();
+		/*
+		 * If we got here with an active transaction, then, it was
+		 * aborted by TM_KERNEL_ENTRY and the fix the failure case
+		 * needs to be fixed, so, indepedently how we arrived here, the
+		 * new TM abort case will be TM_CAUSE_RESCHED now.
+		 */
+		if (MSR_TM_ACTIVE(prev->thread.regs->msr)) {
+			/*
+			 * If there was an IRQ during trecheckpoint, it will
+			 * cause an IRQ to be replayed. This replayed IRQ can
+			 * invoke SCHEDULE_USER, thus, we arrive here with a TM
+			 * active transaction.
+			 * I.e, the task was leaving kernelspace to userspace,
+			 * already trecheckpointed, but there was a IRQ during
+			 * the trecheckpoint process (soft irq disabled), and
+			 * on the IRQ replay, the process was de-scheduled, so,
+			 * SCHEDULE_USER was called and here we are.
+			 *
+			 */
+			if (MSR_TM_ACTIVE(mfmsr())) {
+				/*
+				 * This is the only other case other than
+				 * TM_KERNEL_ENTRY that does a TM reclaim
+				 */
+				tm_reclaim_current(TM_CAUSE_RESCHED);
+			}
 
-		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)
+			/*
+			 * If rescheduled with TM active, update the
+			 * failure cause
+			 */
+			tm_fix_failure_cause(prev, TM_CAUSE_RESCHED);
+		} else {
+			/*
+			 * TM enabled but not transactional. Just disable TM
+			 * if load_tm overflows. This should be the only place
+			 * that disables the TM and reenables the laziness
+			 * save/restore
+			 */
+			if (prev->thread.load_tm == 0)
 				prev->thread.regs->msr &= ~MSR_TM;
 		}
+	}
 
-		tm_recheckpoint_new_task(new);
+	/*
+	 * It is a *bug* if we arrived so late with a transaction active
+	 * (more precisely suspended)
+	 */
+	if (WARN_ON(MSR_TM_ACTIVE(mfmsr()))) {
+		/* Recovery path. 0x99 shouldn't be exported to UAPI */
+		tm_reclaim_current(0x99);
+	}
+
+	/*
+	 * If the next task has TM enabled, restore the SPRs. Do not need to
+	 * care about recheckpoint at this time. It will be done later if
+	 * TIF_RESTORE_TM was set when the task was scheduled out
+	 */
+	if (tm_enabled(new)) {
+		tm_enable();
+		tm_restore_sprs(&new->thread);
 	}
 }
 
@@ -1101,7 +1087,6 @@ void restore_tm_state(struct pt_regs *regs)
 }
 
 #else
-#define tm_recheckpoint_new_task(new)
 #define __switch_to_tm(prev, new)
 #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
 
@@ -1588,9 +1573,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	/*
 	 * Flush TM state out so we can copy it.  __switch_to_tm() does this
 	 * flush but it removes the checkpointed state from the current CPU and
-	 * transitions the CPU out of TM mode.  Hence we need to call
-	 * tm_recheckpoint_new_task() (on the same task) to restore the
-	 * checkpointed state back and the TM mode.
+	 * transitions the CPU out of TM mode.  Hence we need to make sure
+	 * TIF_RESTORE_TM is set so restore_tm_state is called to restore the
+	 * checkpointed state and back to TM mode.
 	 *
 	 * Can't pass dst because it isn't ready. Doesn't matter, passing
 	 * dst is only important for __switch_to()
-- 
2.19.0

[RFC PATCH 07/11] powerpc/tm: Do not recheckpoint at sigreturn

[Breno Leitao]

Do not recheckpoint at signal code return. Just make sure TIF_RESTORE_TM is
set, which will restore on the exit to userspace by restore_tm_state.

All the FP and VEC lazy restore was already done by tm_reclaim_current(),
where it checked if FP/VEC was set, and filled out the ckfp and ckvr
registers area to the expected value.

The current FP/VEC restoration is not necessary, since the transaction will
be aborted and the checkpointed values will be restore.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/signal_32.c | 23 +++--------------------
 arch/powerpc/kernel/signal_64.c | 15 ++-------------
 2 files changed, 5 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index e6474a45cef5..4a1b17409bf3 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -850,28 +850,11 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 		return 1;
 	/* Pull in the MSR TM bits from the user context */
 	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
-	/* Now, recheckpoint.  This loads up all of the checkpointed (older)
-	 * registers, including FP and V[S]Rs.  After recheckpointing, the
-	 * transactional versions should be loaded.
-	 */
-	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
+	/* Make sure restore_tm_state will be called */
+	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 83d51bf586c7..32402aa23a5e 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -569,21 +569,10 @@ 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;
-	}
+	/* Guarantee that restore_tm_state() will be called */
+	set_thread_flag(TIF_RESTORE_TM);
 
 	return err;
 }
-- 
2.19.0

[RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace

[Breno Leitao]

Make sure that we are not suspended on ptrace and that the registers were
already reclaimed.

Since the data was already reclaimed, there is nothing to be done here
except to restore the SPRs.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/ptrace.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9667666eb18e..cf6ee9154b11 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -136,12 +136,10 @@ 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));
-	}
+	WARN_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.19.0

[RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR

[Breno Leitao]

In the previous TM code, trecheckpoint was being executed in the middle of
an exception, thus, DSCR was being restored to default kernel DSCR value
after trecheckpoint was done.

With this current patchset, trecheckpoint is executed just before getting
to userspace, at ret_from_except_lite, for example. Thus, we do not need to
set default kernel DSCR value anymore, as we are leaving kernel space.  It
is OK to keep the checkpointed DSCR value into the live SPR, mainly because
the transaction is doomed and it will fail soon (after RFID), so,
continuing with the pre-checkpointed DSCR value is what seems correct.

That said, we must set the DSCR value that will be used in userspace now.
Current trecheckpoint() function sets it to the pre-checkpointed value
prior to lines being changed in this patch, so, removing these lines would
keep the pre-checkpointed values.

Important to say that we do not need to do the same thing with tm_reclaim,
since it already set the DSCR to the default value, after TRECLAIM is
called, in the following lines:

        /* Load CPU's default DSCR */
        ld      r0, PACA_DSCR_DEFAULT(r13)
        mtspr   SPRN_DSCR, r0

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/tm.S | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 6bffbc5affe7..5427eda69846 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -493,10 +493,6 @@ restore_gprs:
 	mtlr	r0
 	ld	r2, STK_GOT(r1)
 
-	/* Load CPU's default DSCR */
-	ld	r0, PACA_DSCR_DEFAULT(r13)
-	mtspr	SPRN_DSCR, r0
-
 	blr
 
 	/* ****************************************************************** */
-- 
2.19.0

[RFC PATCH 10/11] powerpc/tm: Set failure summary

[Breno Leitao]

Since the transaction will be doomed with treckpt., the TEXASR[FS]
should be set, to reflect that the transaction is a failure. This patch
ensures it before recheckpointing, and remove changes from other places
that were calling recheckpoint.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/process.c   | 6 ++++++
 arch/powerpc/kernel/signal_32.c | 2 --
 arch/powerpc/kernel/signal_64.c | 2 --
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5cace1b744b1..77725b2e4dc1 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -937,6 +937,12 @@ void tm_recheckpoint(struct thread_struct *thread)
 	local_irq_save(flags);
 	hard_irq_disable();
 
+	/*
+	 * Make sure the failure summary is set, since the transaction will be
+	 * doomed.
+	 */
+	thread->tm_texasr |= TEXASR_FS;
+
 	/* The TM SPRs are restored here, so that TEXASR.FS can be set
 	 * before the trecheckpoint and no explosion occurs.
 	 */
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 4a1b17409bf3..96956d50538e 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -851,8 +851,6 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 	/* Pull in the MSR TM bits from the user context */
 	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
 
-	/* Make sure the transaction is marked as failed */
-	current->thread.tm_texasr |= TEXASR_FS;
 	/* Make sure restore_tm_state will be called */
 	set_thread_flag(TIF_RESTORE_TM);
 
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 32402aa23a5e..c84501711b14 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -569,8 +569,6 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 		}
 	}
 #endif
-	/* Make sure the transaction is marked as failed */
-	tsk->thread.tm_texasr |= TEXASR_FS;
 	/* Guarantee that restore_tm_state() will be called */
 	set_thread_flag(TIF_RESTORE_TM);
 
-- 
2.19.0

[RFC PATCH 11/11] selftests/powerpc: Adapt the test

[Breno Leitao]

The Documentation/powerpc/transactional_memory.txt says:

 "Syscalls made from within a suspended transaction are performed as normal
  and the transaction is not explicitly doomed by the kernel.  However,
  what the kernel does to perform the syscall may result in the transaction
  being doomed by the hardware."

With this new TM mechanism, the syscall will continue to be executed if the
syscall happens on a suspended syscall, but, the syscall will *fail* if the
transaction is still active during the syscall invocation.

On the syscall path, if the transaction is active and not suspended, it
will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the
transaction, thus, dooming the transaction on userspace return, with
failure code TM_CAUSE_SYSCALL.

This new model will break part of this test, but I understand that that the
documentation above didn't guarantee that the syscall would succeed, and it
will never succeed anymore now on.

In fact, glibc is calling 'tabort' before every syscalls, thus, any syscall
called through glibc from inside a transaction will be doomed anyhow.

This patch updates the test case to not assume that a syscall inside a
active transaction will succeed, because it will not anymore.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/tm/tm-syscall.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c
index 454b965a2db3..1439a87eba3a 100644
--- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
+++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c
@@ -78,12 +78,6 @@ int tm_syscall(void)
 	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.
-- 
2.19.0

Re: [RFC PATCH 00/11] New TM Model

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> This patchset for the hardware transactional memory (TM) subsystem aims t=
o
> avoid spending a lot of time on TM suspended mode in kernel space. It
> basically
> changes where the reclaim/recheckpoint will be executed.
>=20
> Once a CPU enters in transactional state it uses a footprint area to trac=
k
> down the load/stores performed in transaction so it can be verified later
> to decide if a conflict happened due to some change done in that state. I=
f
> a transaction is active in userspace and there is an exception that takes
> the CPU to the kernel space the CPU moves the transaction to suspended
> state but does not discard the footprint area.

In this description, you should differente between memory and register
(GPR/VSX/SPR) footprints.

In suspend, the CPU can disregard the memory footprint at any point, but it=
 has
to keep the register footprint. =20

In the above paragraph you are talking about register footprint but not mem=
ory
footprint.=20

>=20
> POWER9 has a known problem[1][2] and does not have enough room in
> footprint area for several transactions to be suspended at the same time
> on concurrent CPUs leading to CPU stalls.
>=20
> This patchset aims to reclaim the checkpointed footprint as soon as the
> kernel is invoked, in the beginning of the exception handlers, thus freei=
ng
> room to other CPUs enter in suspended mode, avoiding too many CPUs in
> suspended
> state that can cause the CPUs to stall. The same mechanism is done on ker=
nel
> exit, doing a recheckpoint as late as possible (which will reload the
> checkpointed state into CPU's room) at the exception return.

OK, but we are still potentially in suspend in userspace, so that doesn't h=
elp
us on the lockup issue.

We need fake suspend in userspace to prevent lockups.

> The way to achieve this goal is creating a macro (TM_KERNEL_ENTRY) which
> will check if userspace was in an active transaction just after getting
> into kernel and reclaiming if that's the case. Thus all exception handler=
s
> will call this macro as soon as possible.
>=20
> All exceptions should reclaim (if necessary) at this stage and only
> recheckpoint if the task is tagged as TIF_RESTORE_TM (i.e. was in
> transactional state before being interrupted), which will be done at
> ret_from_except_lite().
>=20
> Ideally all reclaims will happen at the exception entrance, however durin=
g
> the recheckpoint process another exception can hit the CPU which might
> cause the current thread to be rescheduled, thus there is another reclaim
> point to be considered at __switch_to().

Can we do the recheckpoint() later so that it's when we have interrupts off=
 and
can't be rescheduled?

> Hence, by allowing the CPU to be in suspended state for only a brief peri=
od
> it's possible to cope with the TM hardware limitations like the current
> problem on the new POWER9.

As mentioned, since we're still running userspace with real suspend, we sti=
ll
have an issue.

> This patchset was tested in different scenarios using different test
> suites, as the kernel selftests and htm-torture[3], in the following
> configuration:
>=20
>  * POWER8/pseries LE and BE
>  * POWER8/powernv LE
>  * POWER9/powernv LE hosting KVM guests running TM tests
>=20
> This patchset is based on initial work done by Cyril Bur:
>     https://patchwork.ozlabs.org/cover/875341/

Adding Cyril to CC.

Mikey

Re: [RFC PATCH 05/11] powerpc/tm: Function that updates the failure code

[Michael Neuling]

This series is not bisectable because this patch fails with:

arch/powerpc/kernel/process.c:993:13: error: =E2=80=98tm_fix_failure_cause=
=E2=80=99 defined but not used [-Werror=3Dunused-function]
 static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause)
             ^
cc1: all warnings being treated as errors
scripts/Makefile.build:305: recipe for target 'arch/powerpc/kernel/process.=
o' failed

Mikey

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Now the transaction reclaims happens very earlier in the trap handler, an=
d
> it is impossible to know precisely, at that early time, what should be se=
t
> as the failure cause for some specific cases, as, if the task will be
> rescheduled, thus, the transaction abort case should be updated from
> TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.
>=20
> This patch creates a function that will update TEXASR special purpose
> register in the task thread and set the failure code which will be
> moved to the live register afterward.
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/process.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index 54fddf03b97a..fe063c0142e3 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -85,6 +85,7 @@ extern unsigned long _get_SP(void);
>   * other paths that we should never reach with suspend disabled.
>   */
>  bool tm_suspend_disabled __ro_after_init =3D false;
> +static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause=
);
> =20
>  static void check_if_tm_restore_required(struct task_struct *tsk)
>  {
> @@ -988,6 +989,14 @@ void tm_recheckpoint(struct thread_struct *thread)
>  	local_irq_restore(flags);
>  }
> =20
> +/* Change thread->tm.texasr failure code */
> +static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause=
)
> +{
> +	/* Clear the cause first */
> +	task->thread.tm_texasr &=3D ~TEXASR_FC;
> +	task->thread.tm_texasr |=3D (unsigned long) cause << 56;
> +}
> +
>  static inline void tm_recheckpoint_new_task(struct task_struct *new)
>  {
>  	if (!cpu_has_feature(CPU_FTR_TM))

Re: [RFC PATCH 05/11] powerpc/tm: Function that updates the failure code

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Now the transaction reclaims happens very earlier in the trap handler, an=
d
> it is impossible to know precisely, at that early time, what should be se=
t
> as the failure cause for some specific cases, as, if the task will be
> rescheduled, thus, the transaction abort case should be updated from
> TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.

Please add comments to where this is used (in EXCEPTION_COMMON macro I thin=
k)
that say this might happen.

>=20
> This patch creates a function that will update TEXASR special purpose
> register in the task thread and set the failure code which will be
> moved to the live register afterward.
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/process.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index 54fddf03b97a..fe063c0142e3 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -85,6 +85,7 @@ extern unsigned long _get_SP(void);
>   * other paths that we should never reach with suspend disabled.
>   */
>  bool tm_suspend_disabled __ro_after_init =3D false;
> +static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause=
);
> =20
>  static void check_if_tm_restore_required(struct task_struct *tsk)
>  {
> @@ -988,6 +989,14 @@ void tm_recheckpoint(struct thread_struct *thread)
>  	local_irq_restore(flags);
>  }
> =20
> +/* Change thread->tm.texasr failure code */
> +static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause=
)
> +{
> +	/* Clear the cause first */
> +	task->thread.tm_texasr &=3D ~TEXASR_FC;
> +	task->thread.tm_texasr |=3D (unsigned long) cause << 56;
> +}
> +
>  static inline void tm_recheckpoint_new_task(struct task_struct *new)
>  {
>  	if (!cpu_has_feature(CPU_FTR_TM))

Re: [RFC PATCH 01/11] powerpc/tm: Reclaim transaction on kernel entry

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> This patch creates a macro that will be invoked on all entrance to the
> kernel, so, in kernel space the transaction will be completely reclaimed
> and not suspended anymore.

There are still some calls to tm_reclaim_current() in process.c. Should the=
se
probably go now, right?

Mikey

> This patchset checks if we are coming from PR, if not, skip. This is usef=
ul
> when there is a irq_replay() being called after recheckpoint, when the IR=
Q
> is re-enable. In this case, we do not want to re-reclaim and
> re-recheckpoint, thus, if not coming from PR, skip it completely.
>=20
> This macro does not care about TM SPR also, it will only be saved and
> restore in the context switch code now on.
>=20
> This macro will return 0 or 1 in r3 register, to specify if a reclaim was
> executed or not.
>=20
> This patchset is based on initial work done by Cyril:
> https://patchwork.ozlabs.org/cover/875341/
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/include/asm/exception-64s.h | 46 ++++++++++++++++++++++++
>  arch/powerpc/kernel/entry_64.S           | 10 ++++++
>  arch/powerpc/kernel/exceptions-64s.S     | 12 +++++--
>  3 files changed, 66 insertions(+), 2 deletions(-)
>=20
> diff --git a/arch/powerpc/include/asm/exception-64s.h
> b/arch/powerpc/include/asm/exception-64s.h
> index a86feddddad0..db90b6d7826e 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -36,6 +36,7 @@
>   */
>  #include <asm/head-64.h>
>  #include <asm/feature-fixups.h>
> +#include <asm/tm.h>
> =20
>  /* PACA save area offsets (exgen, exmc, etc) */
>  #define EX_R9		0
> @@ -686,10 +687,54 @@ BEGIN_FTR_SECTION				\
>  	beql	ppc64_runlatch_on_trampoline;	\
>  END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> =20
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +
> +/*
> + * This macro will reclaim a transaction if called when coming from user=
space
> + * (MSR.PR =3D 1) and if the transaction state is active or suspended.
> + *
> + * Since we don't want to reclaim when coming from kernel, for instance =
after
> + * a trechkpt. or a IRQ replay, the live MSR is not useful and instead o=
f it
> the
> + * MSR from thread stack is used to check the MSR.PR bit.
> + * This macro has one argument which is the cause that will be used by
> treclaim.
> + * and returns in r3 '1' if the reclaim happens or '0' if reclaim didn't
> + * happen, which is useful to know what registers were clobbered.
> + *
> + * NOTE: If addition registers are clobbered here, make sure the callee
> + * function restores them before proceeding.
> + */
> +#define TM_KERNEL_ENTRY(cause)					=09
> \
> +	ld      r3, _MSR(r1);						\
> +	andi.   r0, r3, MSR_PR;	/* Coming from userspace? */		\
> +	beq     1f;		/* Skip reclaim if MSR.PR !=3D 1 */	\
> +	rldicl. r0, r3, (64-MSR_TM_LG), 63; /* Is TM enabled? */	\
> +	beq     1f;		/* Skip reclaim if TM is off */		\
> +	rldicl. r0, r3, (64-MSR_TS_LG), 62;	/* Is active */		\
> +	beq     1f;		/* Skip reclaim if neither */		\
> +	/*								\
> +	 * If there is a transaction active or suspended, save the	\
> +	 * non-volatile GPRs if they are not already saved.		\
> +	 */								\
> +	bl      save_nvgprs;						\
> +	/*								\
> +	 * Soft disable the IRQs, otherwise it might cause a CPU hang.	\
> +	 */								\
> +	RECONCILE_IRQ_STATE(r10, r11);					\
> +	li      r3, cause;						\
> +	bl      tm_reclaim_current;					\
> +	li      r3, 1;		/* Reclaim happened */			\
> +	b       2f;							\
> +1:	li      r3, 0;		/* Reclaim didn't happen */		\
> +2:
> +#else
> +#define TM_KERNEL_ENTRY(cause)
> +#endif
> +
>  #define EXCEPTION_COMMON(area, trap, label, hdlr, ret, additions) \
>  	EXCEPTION_PROLOG_COMMON(trap, area);			\
>  	/* Volatile regs are potentially clobbered here */	\
>  	additions;						\
> +	TM_KERNEL_ENTRY(TM_CAUSE_MISC);				\
>  	addi	r3,r1,STACK_FRAME_OVERHEAD;			\
>  	bl	hdlr;						\
>  	b	ret
> @@ -704,6 +749,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
>  	EXCEPTION_PROLOG_COMMON_3(trap);			\
>  	/* Volatile regs are potentially clobbered here */	\
>  	additions;						\
> +	TM_KERNEL_ENTRY(TM_CAUSE_MISC);				\
>  	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 2206912ea4f0..c38677b7442c 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -131,6 +131,16 @@ BEGIN_FW_FTR_SECTION
>  END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
>  #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE && CONFIG_PPC_SPLPAR */
> =20
> +#if CONFIG_PPC_TRANSACTIONAL_MEM
> +	TM_KERNEL_ENTRY(TM_CAUSE_SYSCALL)
> +	cmpdi	r3, 0x1
> +	bne	44f
> +	/* Restore from r4 to r12 */
> +	REST_8GPRS(4,r1)
> +44:	/* treclaim was not called, just restore r3 and r0 */
> +	REST_GPR(3, r1)
> +	REST_GPR(0, r1)
> +#endif
>  	/*
>  	 * A syscall should always be called with interrupts enabled
>  	 * so we just unconditionally hard-enable here. When some kind
> diff --git a/arch/powerpc/kernel/exceptions-64s.S
> b/arch/powerpc/kernel/exceptions-64s.S
> index ea04dfb8c092..78aba71a4b2d 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -805,6 +805,7 @@ EXC_COMMON_BEGIN(alignment_common)
>  	std	r3,_DAR(r1)
>  	std	r4,_DSISR(r1)
>  	bl	save_nvgprs
> +	TM_KERNEL_ENTRY(TM_CAUSE_ALIGNMENT)
>  	RECONCILE_IRQ_STATE(r10, r11)
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	bl	alignment_exception
> @@ -839,6 +840,8 @@ EXC_COMMON_BEGIN(program_check_common)
>  	b 3f				/* Jump into the macro !!	*/
>  1:	EXCEPTION_PROLOG_COMMON(0x700, PACA_EXGEN)
>  	bl	save_nvgprs
> +	ld      r3, _MSR(r1)
> +	TM_KERNEL_ENTRY(TM_CAUSE_FAC_UNAV)
>  	RECONCILE_IRQ_STATE(r10, r11)
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	bl	program_check_exception
> @@ -1738,7 +1741,9 @@ do_hash_page:
> =20
>  /* 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(TM_CAUSE_TLBI)
> +	ld      r4,_DSISR(r1)
> +	andis.  r0,r4,DSISR_DABRMATCH@h
>  	bne-    handle_dabr_fault
>  	ld	r4,_DAR(r1)
>  	ld	r5,_DSISR(r1)
> @@ -1769,6 +1774,8 @@ handle_dabr_fault:
>   */
>  13:	bl	save_nvgprs
>  	mr	r5,r3
> +	TM_KERNEL_ENTRY(TM_CAUSE_TLBI)
> +	REST_GPR(3,r1)
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	ld	r4,_DAR(r1)
>  	bl	low_hash_fault
> @@ -1783,7 +1790,8 @@ handle_dabr_fault:
>   * the access, or panic if there isn't a handler.
>   */
>  77:	bl	save_nvgprs
> -	mr	r4,r3
> +	TM_KERNEL_ENTRY(TM_CAUSE_TLBI)
> +	ld      r4,_DAR(r1)
>  	addi	r3,r1,STACK_FRAME_OVERHEAD
>  	li	r5,SIGSEGV
>  	bl	bad_page_fault

Re: [RFC PATCH 05/11] powerpc/tm: Function that updates the failure code

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Now the transaction reclaims happens very earlier in the trap handler, an=
d
> it is impossible to know precisely, at that early time, what should be se=
t
> as the failure cause for some specific cases, as, if the task will be
> rescheduled, thus, the transaction abort case should be updated from
> TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.
>=20
> This patch creates a function that will update TEXASR special purpose
> register in the task thread and set the failure code which will be
> moved to the live register afterward.
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/process.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index 54fddf03b97a..fe063c0142e3 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -85,6 +85,7 @@ extern unsigned long _get_SP(void);
>   * other paths that we should never reach with suspend disabled.
>   */
>  bool tm_suspend_disabled __ro_after_init =3D false;
> +static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause=
);
> =20
>  static void check_if_tm_restore_required(struct task_struct *tsk)
>  {
> @@ -988,6 +989,14 @@ void tm_recheckpoint(struct thread_struct *thread)
>  	local_irq_restore(flags);
>  }
> =20
> +/* Change thread->tm.texasr failure code */
> +static void tm_fix_failure_cause(struct task_struct *task, uint8_t cause=
)

I would just call this tm_change_failure_cause() and drop the comment above=
.

> +{
> +	/* Clear the cause first */
> +	task->thread.tm_texasr &=3D ~TEXASR_FC;
> +	task->thread.tm_texasr |=3D (unsigned long) cause << 56;

56 =3D=3D TEXASR_FC_LG;


> +}
> +
>  static inline void tm_recheckpoint_new_task(struct task_struct *new)
>  {
>  	if (!cpu_has_feature(CPU_FTR_TM))

Re: [RFC PATCH 06/11] powerpc/tm: Refactor the __switch_to_tm code

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> __switch_to_tm is the function that switches between two tasks which migh=
t
> have TM enabled. This function is clearly split in two parts, the task th=
at
> is leaving the CPU, known as 'prev' and the task that is being scheduled,
> known as new.
>=20
> It starts checking if the previous task had TM enable, if so, it increase=
s
> the load_tm (this is the only place we increment load_tm). It also saves
> the TM SPRs here.
>=20
> If the previous task was scheduled out with a transaction active, the
> failure cause needs to be updated, since it might contain the failure cau=
se
> that caused the exception, as TM_CAUSE_MISC. In this case, since there wa=
s
> a context switch, overwrite the failure cause.
>=20
> If the previous task has overflowed load_tm, disable TM, putting the
> facility save/restore lazy mechanism at lazy mode.
>=20
> Regarding the new task, when loading it, it basically restore the SPRs, a=
nd
> TIF_RESTORE_TM (already set by tm_reclaim_current if the transaction was
> active) would invoke the recheckpoint process later in restore_tm_state()
> if recheckpoint is somehow required.

This paragraph is a little awkwardly worded.  Can you rewrite?

> On top of that, both tm_reclaim_task() and tm_recheckpoint_new_task()
> functions are not used anymore, removing them.

What about tm_reclaim_current().  This is being used in places like signals
which I would have thought we could avoid with this series

>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/process.c | 163 +++++++++++++++-------------------
>  1 file changed, 74 insertions(+), 89 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index fe063c0142e3..5cace1b744b1 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -921,48 +921,6 @@ void tm_reclaim_current(uint8_t cause)
>  	tm_reclaim_thread(&current->thread, cause);
>  }
> =20
> -static inline void tm_reclaim_task(struct task_struct *tsk)
> -{
> -	/* We have to work out if we're switching from/to a task that's in the
> -	 * middle of a transaction.
> -	 *
> -	 * In switching we need to maintain a 2nd register state as
> -	 * oldtask->thread.ckpt_regs.  We tm_reclaim(oldproc); this saves the
> -	 * checkpointed (tbegin) state in ckpt_regs, ckfp_state and
> -	 * ckvr_state
> -	 *
> -	 * We also context switch (save) TFHAR/TEXASR/TFIAR in here.
> -	 */
> -	struct thread_struct *thr =3D &tsk->thread;
> -
> -	if (!thr->regs)
> -		return;
> -
> -	if (!MSR_TM_ACTIVE(thr->regs->msr))
> -		goto out_and_saveregs;
> -
> -	WARN_ON(tm_suspend_disabled);
> -
> -	TM_DEBUG("--- tm_reclaim on pid %d (NIP=3D%lx, "
> -		 "ccr=3D%lx, msr=3D%lx, trap=3D%lx)\n",
> -		 tsk->pid, thr->regs->nip,
> -		 thr->regs->ccr, thr->regs->msr,
> -		 thr->regs->trap);
> -
> -	tm_reclaim_thread(thr, TM_CAUSE_RESCHED);
> -
> -	TM_DEBUG("--- tm_reclaim on pid %d complete\n",
> -		 tsk->pid);
> -
> -out_and_saveregs:
> -	/* Always save the regs here, even if a transaction's not active.
> -	 * This context-switches a thread's TM info SPRs.  We do it here to
> -	 * be consistent with the restore path (in recheckpoint) which
> -	 * cannot happen later in _switch().
> -	 */
> -	tm_save_sprs(thr);
> -}
> -
>  extern void __tm_recheckpoint(struct thread_struct *thread);
> =20
>  void tm_recheckpoint(struct thread_struct *thread)
> @@ -997,59 +955,87 @@ static void tm_fix_failure_cause(struct task_struct
> *task, uint8_t cause)
>  	task->thread.tm_texasr |=3D (unsigned long) cause << 56;
>  }
> =20
> -static inline void tm_recheckpoint_new_task(struct task_struct *new)
> +static inline void __switch_to_tm(struct task_struct *prev,

Can we just drop the __ ?

> +		struct task_struct *new)
>  {
>  	if (!cpu_has_feature(CPU_FTR_TM))
>  		return;
> =20
> -	/* Recheckpoint the registers of the thread we're about to switch to.
> -	 *
> -	 * If the task was using FP, we non-lazily reload both the original and
> -	 * the speculative FP register states.  This is because the kernel
> -	 * doesn't see if/when a TM rollback occurs, so if we take an FP
> -	 * unavailable later, we are unable to determine which set of FP regs
> -	 * need to be restored.
> -	 */
> -	if (!tm_enabled(new))
> -		return;
> -
> -	if (!MSR_TM_ACTIVE(new->thread.regs->msr)){
> -		tm_restore_sprs(&new->thread);
> -		return;
> -	}
> -	/* Recheckpoint to restore original checkpointed register state. */
> -	TM_DEBUG("*** tm_recheckpoint of pid %d (new->msr 0x%lx)\n",
> -		 new->pid, new->thread.regs->msr);
> -
> -	tm_recheckpoint(&new->thread);
> -
> -	/*
> -	 * The checkpointed state has been restored but the live state has
> -	 * not, ensure all the math functionality is turned off to trigger
> -	 * restore_math() to reload.
> -	 */
> -	new->thread.regs->msr &=3D ~(MSR_FP | MSR_VEC | MSR_VSX);
> +	/* The task leaving the CPU was using TM, let's handle it */
> +	if (tm_enabled(prev)) {
> +		/*
> +		 * Load_tm is incremented only when the task is scheduled out
> +		 */
> +		prev->thread.load_tm++;
> =20
> -	TM_DEBUG("*** tm_recheckpoint of pid %d complete "
> -		 "(kernel msr 0x%lx)\n",
> -		 new->pid, mfmsr());
> -}
> +		/*
> +		 * If TM is enabled for the thread, it needs to, at least,
> +		 * save the SPRs
> +		 */
> +		tm_enable();
> +		tm_save_sprs(&prev->thread);
> =20
> -static inline void __switch_to_tm(struct task_struct *prev,
> -		struct task_struct *new)
> -{
> -	if (cpu_has_feature(CPU_FTR_TM)) {
> -		if (tm_enabled(prev) || tm_enabled(new))
> -			tm_enable();
> +		/*
> +		 * If we got here with an active transaction, then, it was
> +		 * aborted by TM_KERNEL_ENTRY and the fix the failure case
> +		 * needs to be fixed, so, indepedently how we arrived here, the
> +		 * new TM abort case will be TM_CAUSE_RESCHED now.

What does "fix the failure case needs to be fixed" mean?

also s/indepedently/independently/

> +		 */
> +		if (MSR_TM_ACTIVE(prev->thread.regs->msr)) {
> +			/*
> +			 * If there was an IRQ during trecheckpoint, it will
> +			 * cause an IRQ to be replayed. This replayed IRQ can
> +			 * invoke SCHEDULE_USER, thus, we arrive here with a TM
> +			 * active transaction.

I don't think this can happen. trecheckpoint (and treclaim) are called with=
 IRQs
hard off (since they change r1).

I think something else is going on here. I think this code and comment need=
s to
go but I assume it's here because you are seeing something.

> +			 * I.e, the task was leaving kernelspace to userspace,
> +			 * already trecheckpointed, but there was a IRQ during
> +			 * the trecheckpoint process (soft irq disabled), and
> +			 * on the IRQ replay, the process was de-scheduled, so,
> +			 * SCHEDULE_USER was called and here we are.
> +			 *
> +			 */
> +			if (MSR_TM_ACTIVE(mfmsr())) {
> +				/*
> +				 * This is the only other case other than
> +				 * TM_KERNEL_ENTRY that does a TM reclaim
> +				 */
> +				tm_reclaim_current(TM_CAUSE_RESCHED);
> +			}
> =20
> -		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)
> +			/*
> +			 * If rescheduled with TM active, update the
> +			 * failure cause
> +			 */
> +			tm_fix_failure_cause(prev, TM_CAUSE_RESCHED);
> +		} else {
> +			/*
> +			 * TM enabled but not transactional. Just disable TM
> +			 * if load_tm overflows. This should be the only place
> +			 * that disables the TM and reenables the laziness
> +			 * save/restore
> +			 */
> +			if (prev->thread.load_tm =3D=3D 0)
>  				prev->thread.regs->msr &=3D ~MSR_TM; 	=09
> }
> +	}
> =20
> -		tm_recheckpoint_new_task(new);
> +	/*
> +	 * It is a *bug* if we arrived so late with a transaction active
> +	 * (more precisely suspended)
> +	 */
> +	if (WARN_ON(MSR_TM_ACTIVE(mfmsr()))) {
> +		/* Recovery path. 0x99 shouldn't be exported to UAPI */
> +		tm_reclaim_current(0x99);
> +	}
> +
> +	/*
> +	 * If the next task has TM enabled, restore the SPRs. Do not need to
> +	 * care about recheckpoint at this time. It will be done later if
> +	 * TIF_RESTORE_TM was set when the task was scheduled out
> +	 */
> +	if (tm_enabled(new)) {
> +		tm_enable();
> +		tm_restore_sprs(&new->thread);
>  	}
>  }
> =20
> @@ -1101,7 +1087,6 @@ void restore_tm_state(struct pt_regs *regs)
>  }
> =20
>  #else
> -#define tm_recheckpoint_new_task(new)
>  #define __switch_to_tm(prev, new)
>  #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> =20
> @@ -1588,9 +1573,9 @@ int arch_dup_task_struct(struct task_struct *dst, s=
truct
> task_struct *src)
>  	/*
>  	 * Flush TM state out so we can copy it.  __switch_to_tm() does this
>  	 * flush but it removes the checkpointed state from the current CPU and
> -	 * transitions the CPU out of TM mode.  Hence we need to call
> -	 * tm_recheckpoint_new_task() (on the same task) to restore the
> -	 * checkpointed state back and the TM mode.
> +	 * transitions the CPU out of TM mode.  Hence we need to make sure
> +	 * TIF_RESTORE_TM is set so restore_tm_state is called to restore the
> +	 * checkpointed state and back to TM mode.
>  	 *
>  	 * Can't pass dst because it isn't ready. Doesn't matter, passing
>  	 * dst is only important for __switch_to()

Re: [RFC PATCH 07/11] powerpc/tm: Do not recheckpoint at sigreturn

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Do not recheckpoint at signal code return. Just make sure TIF_RESTORE_TM =
is
> set, which will restore on the exit to userspace by restore_tm_state.

Cool, but what about the same for reclaim? Why not avoid treclaim since it'=
s
done on entry?

Mikey

>=20
> All the FP and VEC lazy restore was already done by tm_reclaim_current(),
> where it checked if FP/VEC was set, and filled out the ckfp and ckvr
> registers area to the expected value.
>=20
> The current FP/VEC restoration is not necessary, since the transaction wi=
ll
> be aborted and the checkpointed values will be restore.
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/signal_32.c | 23 +++--------------------
>  arch/powerpc/kernel/signal_64.c | 15 ++-------------
>  2 files changed, 5 insertions(+), 33 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal=
_32.c
> index e6474a45cef5..4a1b17409bf3 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -850,28 +850,11 @@ static long restore_tm_user_regs(struct pt_regs *re=
gs,
>  		return 1;
>  	/* Pull in the MSR TM bits from the user context */
>  	regs->msr =3D (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
> -	/* Now, recheckpoint.  This loads up all of the checkpointed (older)
> -	 * registers, including FP and V[S]Rs.  After recheckpointing, the
> -	 * transactional versions should be loaded.
> -	 */
> -	tm_enable();
> +
>  	/* Make sure the transaction is marked as failed */
>  	current->thread.tm_texasr |=3D 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 |=3D (MSR_FP | current->thread.fpexc_mode);
> -	}
> -#ifdef CONFIG_ALTIVEC
> -	if (msr & MSR_VEC) {
> -		load_vr_state(&current->thread.vr_state);
> -		regs->msr |=3D MSR_VEC;
> -	}
> -#endif
> +	/* Make sure restore_tm_state will be called */
> +	set_thread_flag(TIF_RESTORE_TM);
> =20
>  	return 0;
>  }
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal=
_64.c
> index 83d51bf586c7..32402aa23a5e 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -569,21 +569,10 @@ static long restore_tm_sigcontexts(struct task_stru=
ct
> *tsk,
>  		}
>  	}
>  #endif
> -	tm_enable();
>  	/* Make sure the transaction is marked as failed */
>  	tsk->thread.tm_texasr |=3D 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 |=3D (MSR_FP | tsk->thread.fpexc_mode);
> -	}
> -	if (msr & MSR_VEC) {
> -		load_vr_state(&tsk->thread.vr_state);
> -		regs->msr |=3D MSR_VEC;
> -	}
> +	/* Guarantee that restore_tm_state() will be called */
> +	set_thread_flag(TIF_RESTORE_TM);
> =20
>  	return err;
>  }

Re: [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Make sure that we are not suspended on ptrace and that the registers were
> already reclaimed.
>=20
> Since the data was already reclaimed, there is nothing to be done here
> except to restore the SPRs.
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/ptrace.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 9667666eb18e..cf6ee9154b11 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -136,12 +136,10 @@ static void flush_tmregs_to_thread(struct task_stru=
ct
> *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));
> -	}
> +	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
> +
> +	tm_enable();
> +	tm_save_sprs(&(tsk->thread));

Do we need to check if TM was enabled in the task before saving the TM SPRs=
?

What happens if TM was lazily off and hence we had someone else's TM SPRs i=
n the
CPU currently?  Wouldn't this flush the wrong values to the task_struct?

I think we need to check the processes MSR before doing this.

Mikey

>  }
>  #else
>  static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }

Re: [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> In the previous TM code, trecheckpoint was being executed in the middle o=
f
> an exception, thus, DSCR was being restored to default kernel DSCR value
> after trecheckpoint was done.
>=20
> With this current patchset, trecheckpoint is executed just before getting
> to userspace, at ret_from_except_lite, for example. Thus, we do not need =
to
> set default kernel DSCR value anymore, as we are leaving kernel space.  I=
t
> is OK to keep the checkpointed DSCR value into the live SPR, mainly becau=
se
> the transaction is doomed and it will fail soon (after RFID),=20

What if we are going back to a suspended transaction?  It will remain live =
until
userspace does a tresume

> so,
> continuing with the pre-checkpointed DSCR value is what seems correct.

Reading this description suggests this patch isn't really needed. Right?

Mikey

> That said, we must set the DSCR value that will be used in userspace now.
> Current trecheckpoint() function sets it to the pre-checkpointed value
> prior to lines being changed in this patch, so, removing these lines woul=
d
> keep the pre-checkpointed values.
>=20
> Important to say that we do not need to do the same thing with tm_reclaim=
,
> since it already set the DSCR to the default value, after TRECLAIM is
> called, in the following lines:
>=20
>         /* Load CPU's default DSCR */
>         ld      r0, PACA_DSCR_DEFAULT(r13)
>         mtspr   SPRN_DSCR, r0
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/tm.S | 4 ----
>  1 file changed, 4 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
> index 6bffbc5affe7..5427eda69846 100644
> --- a/arch/powerpc/kernel/tm.S
> +++ b/arch/powerpc/kernel/tm.S
> @@ -493,10 +493,6 @@ restore_gprs:
>  	mtlr	r0
>  	ld	r2, STK_GOT(r1)
> =20
> -	/* Load CPU's default DSCR */
> -	ld	r0, PACA_DSCR_DEFAULT(r13)
> -	mtspr	SPRN_DSCR, r0
> -
>  	blr
> =20
>  	/* ****************************************************************** *=
/

Re: [RFC PATCH 10/11] powerpc/tm: Set failure summary

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Since the transaction will be doomed with treckpt., the TEXASR[FS]
> should be set, to reflect that the transaction is a failure. This patch
> ensures it before recheckpointing, and remove changes from other places
> that were calling recheckpoint.

TEXASR[FS] should be set by the reclaim. I don't know why you'd need to set=
 this
explicitly in process.c. The only case is when the user supplies a bad sign=
al
context, but we should check that in the signals code, not process.c

Hence I think this patch is wrong.

Also, according to the architecture, TEXASR[FS] HAS TO BE SET on trecheckpo=
int
otherwise you'll get a TM Bad Thing. You should say that rather than sugges=
ting
it's because the transaction is doomed. It's illegal to not do it. That's w=
hy we
have this check in arch/powerpc/kernel/tm.S.


	/* Do final sanity check on TEXASR to make sure FS is set.  Do this
	 * here before we load up the userspace r1 so any bugs we hit will get
	 * a call chain */
	mfspr	r5, SPRN_TEXASR
	srdi	r5, r5, 16
	li	r6, (TEXASR_FS)@h
	and	r6, r6, r5
1:	tdeqi	r6, 0
	EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,0


Mikey

> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/process.c   | 6 ++++++
>  arch/powerpc/kernel/signal_32.c | 2 --
>  arch/powerpc/kernel/signal_64.c | 2 --
>  3 files changed, 6 insertions(+), 4 deletions(-)
>=20
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.=
c
> index 5cace1b744b1..77725b2e4dc1 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -937,6 +937,12 @@ void tm_recheckpoint(struct thread_struct *thread)
>  	local_irq_save(flags);
>  	hard_irq_disable();
> =20
> +	/*
> +	 * Make sure the failure summary is set, since the transaction will be
> +	 * doomed.
> +	 */
> +	thread->tm_texasr |=3D TEXASR_FS;
> +
>  	/* The TM SPRs are restored here, so that TEXASR.FS can be set
>  	 * before the trecheckpoint and no explosion occurs.
>  	 */
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal=
_32.c
> index 4a1b17409bf3..96956d50538e 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -851,8 +851,6 @@ static long restore_tm_user_regs(struct pt_regs *regs=
,
>  	/* Pull in the MSR TM bits from the user context */
>  	regs->msr =3D (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
> =20
> -	/* Make sure the transaction is marked as failed */
> -	current->thread.tm_texasr |=3D TEXASR_FS;
>  	/* Make sure restore_tm_state will be called */
>  	set_thread_flag(TIF_RESTORE_TM);
> =20
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal=
_64.c
> index 32402aa23a5e..c84501711b14 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -569,8 +569,6 @@ static long restore_tm_sigcontexts(struct task_struct
> *tsk,
>  		}
>  	}
>  #endif
> -	/* Make sure the transaction is marked as failed */
> -	tsk->thread.tm_texasr |=3D TEXASR_FS;
>  	/* Guarantee that restore_tm_state() will be called */
>  	set_thread_flag(TIF_RESTORE_TM);
> =20

Re: [RFC PATCH 11/11] selftests/powerpc: Adapt the test

[Michael Neuling]

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> The Documentation/powerpc/transactional_memory.txt says:
>=20
>  "Syscalls made from within a suspended transaction are performed as norm=
al
>   and the transaction is not explicitly doomed by the kernel.  However,
>   what the kernel does to perform the syscall may result in the transacti=
on
>   being doomed by the hardware."
>=20
> With this new TM mechanism, the syscall will continue to be executed if t=
he
> syscall happens on a suspended syscall, but, the syscall will *fail* if t=
he
> transaction is still active during the syscall invocation.

Not sure I get this. This doesn't seem any different to before.

An active (not suspended) transaction *will* result in the syscall failing =
and
the transaction being doomed. =20

A syscall in a suspended transaction should succeed and the transaction.

You might need to clean up the language. I try to use:

   Active =3D=3D transactional but not suspended (ie MSR[TS] =3D T)
   Suspended =3D=3D suspended (ie MSR [TS] =3D S)
   Doomed =3D=3D transaction to be rolled back at next opportinity (ie tche=
ck returns doomed)

(note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since i=
t's
MSR[TS] =3D=3D S or T).

> On the syscall path, if the transaction is active and not suspended, it
> will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the
> transaction, thus, dooming the transaction on userspace return, with
> failure code TM_CAUSE_SYSCALL.

But the test below is on a suspend transaction?

> This new model will break part of this test, but I understand that that t=
he
> documentation above didn't guarantee that the syscall would succeed, and =
it
> will never succeed anymore now on.

The syscall should pass in suspend (modulo the normal syscall checks). The
transaction may fail as a result.

> In fact, glibc is calling 'tabort' before every syscalls, thus, any sysca=
ll
> called through glibc from inside a transaction will be doomed anyhow.
>=20
> This patch updates the test case to not assume that a syscall inside a
> active transaction will succeed, because it will not anymore.
>=20
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  tools/testing/selftests/powerpc/tm/tm-syscall.c | 6 ------
>  1 file changed, 6 deletions(-)
>=20
> diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c
> b/tools/testing/selftests/powerpc/tm/tm-syscall.c
> index 454b965a2db3..1439a87eba3a 100644
> --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
> +++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c
> @@ -78,12 +78,6 @@ int tm_syscall(void)
>  	timeradd(&end, &now, &end);
> =20
>  	for (count =3D 0; timercmp(&now, &end, <); count++) {
> -		/*
> -		 * Test a syscall within a suspended transaction and verify
> -		 * that it succeeds.
> -		 */
> -		FAIL_IF(getppid_tm(true) =3D=3D -1); /* Should succeed. */
> -
>  		/*
>  		 * Test a syscall within an active transaction and verify that
>  		 * it fails with the correct failure code.

Re: [RFC PATCH 00/11] New TM Model

[Breno Leitao]

Hi Mikey,

First of all, thanks for you detailed review. I really appreciate your
comments here.

On 09/17/2018 02:25 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> This patchset for the hardware transactional memory (TM) subsystem aims to
>> avoid spending a lot of time on TM suspended mode in kernel space. It
>> basically
>> changes where the reclaim/recheckpoint will be executed.
>>
>> Once a CPU enters in transactional state it uses a footprint area to track
>> down the load/stores performed in transaction so it can be verified later
>> to decide if a conflict happened due to some change done in that state. If
>> a transaction is active in userspace and there is an exception that takes
>> the CPU to the kernel space the CPU moves the transaction to suspended
>> state but does not discard the footprint area.
> 
> In this description, you should differente between memory and register
> (GPR/VSX/SPR) footprints.

Right, reading the ISA, I understand that footprint is a term for memory only
and it represents the modified memory that was stored during a transactional
state (that after tbegin). For registers, the ISA talks about checkpointed
registers, which is the register state *before* a transaction starts.

I.e, for register it is the previous state, and for memory, it is the
current/live state.

That said, if the transactional is aborted, the memory footprint is discarded
and the checkpointed registers replaces the live registers.

> In suspend, the CPU can disregard the memory footprint at any point, but it has> to keep the register footprint.
Yes!

Anyway, I was just trying to describe how the hardware works, it is not
related to the kernel at the paragraph above, but I will make sure I will
re-write it better.

>> This patchset aims to reclaim the checkpointed footprint as soon as the
>> kernel is invoked, in the beginning of the exception handlers, thus freeing
>> room to other CPUs enter in suspended mode, avoiding too many CPUs in
>> suspended
>> state that can cause the CPUs to stall. The same mechanism is done on kernel
>> exit, doing a recheckpoint as late as possible (which will reload the
>> checkpointed state into CPU's room) at the exception return.
> 
> OK, but we are still potentially in suspend in userspace, so that doesn't help
> us on the lockup issue.
> 
> We need fake suspend in userspace to prevent lockups.

Correct. I will make sure I document it. This patchset is the very first step
to start creating a work around for the hardware limitations.

>> The way to achieve this goal is creating a macro (TM_KERNEL_ENTRY) which
>> will check if userspace was in an active transaction just after getting
>> into kernel and reclaiming if that's the case. Thus all exception handlers
>> will call this macro as soon as possible.
>>
>> All exceptions should reclaim (if necessary) at this stage and only
>> recheckpoint if the task is tagged as TIF_RESTORE_TM (i.e. was in
>> transactional state before being interrupted), which will be done at
>> ret_from_except_lite().
>>
>> Ideally all reclaims will happen at the exception entrance, however during
>> the recheckpoint process another exception can hit the CPU which might
>> cause the current thread to be rescheduled, thus there is another reclaim
>> point to be considered at __switch_to().
> 
> Can we do the recheckpoint() later so that it's when we have interrupts off and
> can't be rescheduled?

Yes!  After thinking on it for a long time, this is definitely what should be
done. I will send a v2 with this change (and others being discussed here)

Thank you,
Breno

Re: [RFC PATCH 00/11] New TM Model

[Breno Leitao]

Hi Mikey,

First of all, thanks for you detailed review. I really appreciate your
comments here.

On 09/17/2018 02:25 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> This patchset for the hardware transactional memory (TM) subsystem aims to
>> avoid spending a lot of time on TM suspended mode in kernel space. It
>> basically
>> changes where the reclaim/recheckpoint will be executed.
>>
>> Once a CPU enters in transactional state it uses a footprint area to track
>> down the load/stores performed in transaction so it can be verified later
>> to decide if a conflict happened due to some change done in that state. If
>> a transaction is active in userspace and there is an exception that takes
>> the CPU to the kernel space the CPU moves the transaction to suspended
>> state but does not discard the footprint area.
> 
> In this description, you should differente between memory and register
> (GPR/VSX/SPR) footprints.

Right, reading the ISA, I understand that footprint is a term for memory only
and it represents the modified memory that was stored during a transactional
state (that after tbegin). For registers, the ISA talks about checkpointed
registers, which is the register state *before* a transaction starts.

I.e, for register it is the previous state, and for memory, it is the
current/live state.

That said, if the transactional is aborted, the memory footprint is discarded
and the checkpointed registers replaces the live registers.

> In suspend, the CPU can disregard the memory footprint at any point, but it has> to keep the register footprint.
Yes!

Anyway, I was just trying to describe how the hardware works, it is not
related to the kernel at the paragraph above, but I will make sure I will
re-write it better.

>> This patchset aims to reclaim the checkpointed footprint as soon as the
>> kernel is invoked, in the beginning of the exception handlers, thus freeing
>> room to other CPUs enter in suspended mode, avoiding too many CPUs in
>> suspended
>> state that can cause the CPUs to stall. The same mechanism is done on kernel
>> exit, doing a recheckpoint as late as possible (which will reload the
>> checkpointed state into CPU's room) at the exception return.
> 
> OK, but we are still potentially in suspend in userspace, so that doesn't help
> us on the lockup issue.
> 
> We need fake suspend in userspace to prevent lockups.

Correct. I will make sure I document it. This patchset is the very first step
to start creating a work around for the hardware limitations.

>> The way to achieve this goal is creating a macro (TM_KERNEL_ENTRY) which
>> will check if userspace was in an active transaction just after getting
>> into kernel and reclaiming if that's the case. Thus all exception handlers
>> will call this macro as soon as possible.
>>
>> All exceptions should reclaim (if necessary) at this stage and only
>> recheckpoint if the task is tagged as TIF_RESTORE_TM (i.e. was in
>> transactional state before being interrupted), which will be done at
>> ret_from_except_lite().
>>
>> Ideally all reclaims will happen at the exception entrance, however during
>> the recheckpoint process another exception can hit the CPU which might
>> cause the current thread to be rescheduled, thus there is another reclaim
>> point to be considered at __switch_to().
> 
> Can we do the recheckpoint() later so that it's when we have interrupts off and
> can't be rescheduled?

Yes!  After thinking on it for a long time, this is definitely what should be
done. I will send a v2 with this change (and others being discussed here)

Thank you,
Breno

Re: [RFC PATCH 01/11] powerpc/tm: Reclaim transaction on kernel entry

[Breno Leitao]

Hi Mikey,

On 09/17/2018 10:31 PM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> This patch creates a macro that will be invoked on all entrance to the
>> kernel, so, in kernel space the transaction will be completely reclaimed
>> and not suspended anymore.
> 
> There are still some calls to tm_reclaim_current() in process.c. Should these
> probably go now, right?

Yes, we shouldn't not call tm_reclaim_current() from anywhere else other than
TM_KERNEL_ENTRY anymore.

That said, I think we can still have some references for tm_reclaim_current()
in some specific checkpoints. Something as:

        if (WARN_ON(MSR_TM_SUSPENDED(mfmsr())))
                tm_reclaim_current(0);

I initially wrote something as BUG_ON(MSR_TM_SUSPENDED(mfmsr()) but
scripts/checkpatch.pl suggested me writing a WARN_ON() and create a recovery
pass, which seems fair.

Anyway, if you think it is not a good strategy, I can get rid of them at v2.

Thank you!

Re: [RFC PATCH 01/11] powerpc/tm: Reclaim transaction on kernel entry

[Breno Leitao]

Hi Mikey,

On 09/17/2018 10:31 PM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> This patch creates a macro that will be invoked on all entrance to the
>> kernel, so, in kernel space the transaction will be completely reclaimed
>> and not suspended anymore.
> 
> There are still some calls to tm_reclaim_current() in process.c. Should these
> probably go now, right?

Yes, we shouldn't not call tm_reclaim_current() from anywhere else other than
TM_KERNEL_ENTRY anymore.

That said, I think we can still have some references for tm_reclaim_current()
in some specific checkpoints. Something as:

        if (WARN_ON(MSR_TM_SUSPENDED(mfmsr())))
                tm_reclaim_current(0);

I initially wrote something as BUG_ON(MSR_TM_SUSPENDED(mfmsr()) but
scripts/checkpatch.pl suggested me writing a WARN_ON() and create a recovery
pass, which seems fair.

Anyway, if you think it is not a good strategy, I can get rid of them at v2.

Thank you!

Re: [RFC PATCH 06/11] powerpc/tm: Refactor the __switch_to_tm code

[Breno Leitao]

hi Mikey

On 09/18/2018 01:04 AM, Michael Neuling wrote:
>> On top of that, both tm_reclaim_task() and tm_recheckpoint_new_task()
>> functions are not used anymore, removing them.
> 
> What about tm_reclaim_current().  This is being used in places like signals
> which I would have thought we could avoid with this series

In fact tm_reclaim_current() is the default function to reclaim. It is the
function that is being called by TM_KERNEL_ENTRY, other than that, it should
never be called on the sane path.

>> +		 * If we got here with an active transaction, then, it was
>> +		 * aborted by TM_KERNEL_ENTRY and the fix the failure case
>> +		 * needs to be fixed, so, indepedently how we arrived here, the
>> +		 * new TM abort case will be TM_CAUSE_RESCHED now.
> 
> What does "fix the failure case needs to be fixed" mean?
> 
> also s/indepedently/independently/

In fact, I rewrote this paragraph. I try to say that the "TEXASR Failure
code" needs to be updated/fixed, but it became that messy and crazy
paragraph. :-(

> 
>> +		 */
>> +		if (MSR_TM_ACTIVE(prev->thread.regs->msr)) {
>> +			/*
>> +			 * If there was an IRQ during trecheckpoint, it will
>> +			 * cause an IRQ to be replayed. This replayed IRQ can
>> +			 * invoke SCHEDULE_USER, thus, we arrive here with a TM
>> +			 * active transaction.
> 
> I don't think this can happen. trecheckpoint (and treclaim) are called with IRQs
> hard off (since they change r1).

There will be an IRQ check and replay later. An IRQ being replayed can cause
a process to be reschedule and arrive here after a trecheckpoint.

> I think something else is going on here. I think this code and comment needs to
> go but I assume it's here because you are seeing something.

Right, and it was my major concern about this whole review.

The tm_recheckpoint() was being called with IRQ hard off, as you said, but
the IRQ is being re-enabled later, after "trecheckpoint", when it calls
local_irq_restore().

Since the IRQ was re-enabled, there is a mechanism to check for IRQs that
were pending and execute them (after recheckpoint - hence with MSR[TS]
active). Some of these pending IRQ might cause a task reschedule, bringing us
to __switch_to with MSR[TS] active.

I also checked that there were cases where a pending IRQ was waiting to be
replayed even before  tm_recheckpoint() is called. I suspect we were reaching
tm_recheckpoint soft-disabled.

On the v2 patchset, I am following your suggestion, which is calling
restore_tm_state() much later (at the beginning of fast_exception_return),
after the _TIF_USER_WORK_MASK section, and after the IRQ replay section also.
So, after this point, there is no way to rollback the exit to userspace after
trecheckpoint. Therefore, if there is a recheckpoint, a rfid will always come
directly.

In order to do so, I need to remove _TIF_RESTORE_TM as part of
_TIF_USER_WORK_MASK and handle _TIF_RESTORE_TM individually later.

This seems to solve this problem, and I can remove this reclaim in the middle
of __switch_to_tm().

Thank you

Re: [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR

[Breno Leitao]

Hi Mikey,

On 09/18/2018 02:41 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> In the previous TM code, trecheckpoint was being executed in the middle of
>> an exception, thus, DSCR was being restored to default kernel DSCR value
>> after trecheckpoint was done.
>>
>> With this current patchset, trecheckpoint is executed just before getting
>> to userspace, at ret_from_except_lite, for example. Thus, we do not need to
>> set default kernel DSCR value anymore, as we are leaving kernel space.  It
>> is OK to keep the checkpointed DSCR value into the live SPR, mainly because
>> the transaction is doomed and it will fail soon (after RFID), 
> 
> What if we are going back to a suspended transaction?  It will remain live until
> userspace does a tresume

Hmm, I understand that once we get in kernel space, and call
treclaim/trecheckpoint, the transaction will be doomed and it will abort and
rollback when we leave kernel space. I.e., if we can treclaim/trecheckpointn
in kernel space, the transaction will *always* abort just after RFID, so, a
possible tresume will never be executed. Is my understanding wrong?

> 
>> so,
>> continuing with the pre-checkpointed DSCR value is what seems correct.
> 
> Reading this description suggests this patch isn't really needed. Right?

Maybe the description is not clear, but I understand this patch is required,
otherwise we will leave userspace with a default DSCR value.

By the way, do you know if there is a change in DSCR inside a transaction,
will it be reverted if the transaction is aborted?

Thank you

Re: [RFC PATCH 10/11] powerpc/tm: Set failure summary

[Breno Leitao]

Hi Mikey,

On 09/18/2018 02:50 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> Since the transaction will be doomed with treckpt., the TEXASR[FS]
>> should be set, to reflect that the transaction is a failure. This patch
>> ensures it before recheckpointing, and remove changes from other places
>> that were calling recheckpoint.
> 
> TEXASR[FS] should be set by the reclaim. 

Do you mean that the CPU should set TEXASR[FS] when treclaim is called, or,
that the tm_reclaim?

Looking at the ISA, I didn't see TEXASR[FS] being set automatically when a
reclaim happens, although, I see it needs TEXASR[FS] to be set when executing
a trecheckpoint, otherwise it will cause a TM Bad Thing.

That is why I am forcing TEXASR[FS]=1 to doom the transaction so we can
recheckpoint it, but it seems I understood this wrongly.

> I don't know why you'd need to set this
> explicitly in process.c. The only case is when the user supplies a bad signal
> context, but we should check that in the signals code, not process.c
> 
> Hence I think this patch is wrong.
> 
> Also, according to the architecture, TEXASR[FS] HAS TO BE SET on trecheckpoint
> otherwise you'll get a TM Bad Thing. You should say that rather than suggesting
> it's because the transaction is doomed. It's ilqlegal to not do it. That's why we
> have this check in arch/powerpc/kernel/tm.S.

When you say "HAS TO BE SET", do you mean that the hardware will set it and
we shouldn't care about this flag? Thus, if I am hitting EMIT_BUG_ENTRY, it
means my TEXASR was messed somehow?

Thank you

Re: [RFC PATCH 11/11] selftests/powerpc: Adapt the test

[Breno Leitao]

Hi Mikey,

On 09/18/2018 03:36 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> The Documentation/powerpc/transactional_memory.txt says:
>>
>>  "Syscalls made from within a suspended transaction are performed as normal
>>   and the transaction is not explicitly doomed by the kernel.  However,
>>   what the kernel does to perform the syscall may result in the transaction
>>   being doomed by the hardware."
>>
>> With this new TM mechanism, the syscall will continue to be executed if the
>> syscall happens on a suspended syscall, but, the syscall will *fail* if the
>> transaction is still active during the syscall invocation.
> 
> Not sure I get this. This doesn't seem any different to before.
> 
> An active (not suspended) transaction *will* result in the syscall failing and
> the transaction being doomed.  
> 
> A syscall in a suspended transaction should succeed and the transaction.

I understand that a transaction will always be doomed if there is a
reclaim/checkpoint, thus, if the you make a system call inside a suspended
transaction, it will reclaim and recheckpoint, thus, dooming the transaction,
and failing it on the next RFID.

So, the syscall executed in suspended mode may succeed, because it will not
be skipped (as in active mode), but the transaction will *always* fail,
because there was a reclaim and recheckpoint.

> You might need to clean up the language. I try to use:
> 
>    Active == transactional but not suspended (ie MSR[TS] = T)
>    Suspended == suspended (ie MSR [TS] = S)
>    Doomed == transaction to be rolled back at next opportinity (ie tcheck returns doomed)
> 
> (note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since it's
> MSR[TS] == S or T).

Ok, I agree with this language as well. I might want to improve the code to
follow the same language all across the code.

>> On the syscall path, if the transaction is active and not suspended, it
>> will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the
>> transaction, thus, dooming the transaction on userspace return, with
>> failure code TM_CAUSE_SYSCALL.
> 
> But the test below is on a suspend transaction?

Sorry, I meant "suspended transaction" above instead of "transaction is
active and not suspended".

> 
>> This new model will break part of this test, but I understand that that the
>> documentation above didn't guarantee that the syscall would succeed, and it
>> will never succeed anymore now on.
> 
> The syscall should pass in suspend (modulo the normal syscall checks). The
> transaction may fail as a result.

Perfect, and if the transaction fail, the CPU will rollback the changes and
restore the checkpoint registers (replacing the r3 that contains the pid
value), thus, it will be like "getpid" system call didn't execute.

For this test specifically, it assumes the syscall didn't execute if the
transaction failed. Take a look:

	FUNC_START(getppid_tm_suspended)
		tbegin.
		beq 1f
		li      r0, __NR_getppid
		tsuspend.
		sc
		tresume.
		tend.
		blr
	1:
		li      r3, -1
		blr

So, the tests thinks the syscall failed because the transaction aborted.

Anyway, I can try to improve this test other than removing this test, but I
am not sure how.

Thank you

Re: [RFC PATCH 05/11] powerpc/tm: Function that updates the failure code

[Breno Leitao]

Hi Mikey,

On 09/17/2018 10:29 PM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> Now the transaction reclaims happens very earlier in the trap handler, and
>> it is impossible to know precisely, at that early time, what should be set
>> as the failure cause for some specific cases, as, if the task will be
>> rescheduled, thus, the transaction abort case should be updated from
>> TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.
> 
> Please add comments to where this is used (in EXCEPTION_COMMON macro I think)
> that say this might happen.

Is it OK if I comment it in TM_KERNEL_ENTRY macro, since the failure cause
could be updated independently of the exception being execute, so, every call
to TM_KERNEL_ENTRY can have the cause overwritten.

I.e. it does not matter if  the exception is a systemcall or a page fault,
the failure cause will be updated if there is a process reschedule after the
exception/syscall is handled.

Thank you

Re: [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace

[Breno Leitao]

Hi Mikey,

On 09/18/2018 02:36 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> Make sure that we are not suspended on ptrace and that the registers were
>> already reclaimed.
>>
>> Since the data was already reclaimed, there is nothing to be done here
>> except to restore the SPRs.
>>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>>  arch/powerpc/kernel/ptrace.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>> index 9667666eb18e..cf6ee9154b11 100644
>> --- a/arch/powerpc/kernel/ptrace.c
>> +++ b/arch/powerpc/kernel/ptrace.c
>> @@ -136,12 +136,10 @@ 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));
>> -	}
>> +	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
>> +
>> +	tm_enable();
>> +	tm_save_sprs(&(tsk->thread));
> 
> Do we need to check if TM was enabled in the task before saving the TM SPRs?
> 
> What happens if TM was lazily off and hence we had someone else's TM SPRs in the
> CPU currently?  Wouldn't this flush the wrong values to the task_struct?
> 
> I think we need to check the processes MSR before doing this.

Yes, it is a *very* good point, and I think we are vulnerable even before
this patch (in the current kernel). Take a look above, we are calling
tm_save_sprs() if MSR is not TM suspended independently if TM is lazily off.

It shouldn't be hard to create a test case for it. I can try to call
ptrace(PTRACE_GETVRREGS) on a task that sleeps until TM is lazily disabled,
compare if the TM SPR changed in this mean time. (while doing HTM in parallel
to keep HTM SPR changing). Let's see if I can read others task TM SPRs.

Thank you.

Re: [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR

[Michael Neuling]

On Thu, 2018-09-27 at 17:51 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/18/2018 02:41 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > In the previous TM code, trecheckpoint was being executed in the middle of
> > > an exception, thus, DSCR was being restored to default kernel DSCR value
> > > after trecheckpoint was done.
> > > 
> > > With this current patchset, trecheckpoint is executed just before getting
> > > to userspace, at ret_from_except_lite, for example. Thus, we do not need
> > > to
> > > set default kernel DSCR value anymore, as we are leaving kernel space.  It
> > > is OK to keep the checkpointed DSCR value into the live SPR, mainly
> > > because
> > > the transaction is doomed and it will fail soon (after RFID), 
> > 
> > What if we are going back to a suspended transaction?  It will remain live
> > until
> > userspace does a tresume
> 
> Hmm, I understand that once we get in kernel space, and call
> treclaim/trecheckpoint, the transaction will be doomed and it will abort and
> rollback when we leave kernel space. I.e., if we can treclaim/trecheckpointn
> in kernel space, the transaction will *always* abort just after RFID, so, a
> possible tresume will never be executed. Is my understanding wrong?

Your understanding is wrong. We don't roll back until MSR[TS] = T.

There are two cases for the RFID.

1) if you RFID back to userspace that is transactional (ie MSR[TS] = T), then it
will immediately rollback as soon as the RFID happens since we are going
directly to T.

2) If you RFID back to userspace that is suspended (ie MSR[TS] = S), then it
won't roll back until userspace does a tresume. It doesn't rollback until we go
MSR[TS] = S -> T).

> > 
> > > so,
> > > continuing with the pre-checkpointed DSCR value is what seems correct.
> > 
> > Reading this description suggests this patch isn't really needed. Right?
> 
> Maybe the description is not clear, but I understand this patch is required,
> otherwise we will leave userspace with a default DSCR value.
> 
> By the way, do you know if there is a change in DSCR inside a transaction,
> will it be reverted if the transaction is aborted?

Yes it will be reverted. We even have a selftest for it in
tools/testing/selftests/powerpc/tm/tm-resched-dscr.c

There are a bunch of checkpointed SPRs. From the arch:
   Checkpointed registers: The set of registers that are
   saved to the “checkpoint area” when a transaction is
   initiated, and restored upon transaction failure, is a
   subset of the architected register state, consisting of
   the General Purpose Registers, Floating-Point Regis-
   ters, Vector Registers, Vector-Scalar Registers, and the
   following Special Registers and fields: CR fields other
   than CR0, XER, LR, CTR, FPSCR, AMR, PPR,
   VRSAVE, VSCR, DSCR, and TAR.

Mikey

Re: [RFC PATCH 10/11] powerpc/tm: Set failure summary

[Michael Neuling]

On Thu, 2018-09-27 at 17:52 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/18/2018 02:50 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Since the transaction will be doomed with treckpt., the TEXASR[FS]
> > > should be set, to reflect that the transaction is a failure. This patch
> > > ensures it before recheckpointing, and remove changes from other places
> > > that were calling recheckpoint.
> > 
> > TEXASR[FS] should be set by the reclaim. 
> 
> Do you mean that the CPU should set TEXASR[FS] when treclaim is called, or,
> that the tm_reclaim?
> 
> Looking at the ISA, I didn't see TEXASR[FS] being set automatically when a
> reclaim happens, 
treclaim in ISA talks about "TMRecordFailure(cause)" and that macro sets
TEXASR[FS]=1. 

So yes treclaim always sets TEXASR[FS]=1.

> although, I see it needs TEXASR[FS] to be set when executing a 
> trecheckpoint, otherwise it will cause a TM Bad Thing.

Yep

> That is why I am forcing TEXASR[FS]=1 to doom the transaction so we can
> recheckpoint it, but it seems I understood this wrongly.

You shouldn't need to.  We do a bug_on() just before the trecheckpoint to make
sure it's set, but you shouldn't need to set it (other than the signals case).

> > I don't know why you'd need to set this
> > explicitly in process.c. The only case is when the user supplies a bad
> > signal
> > context, but we should check that in the signals code, not process.c
> > 
> > Hence I think this patch is wrong.
> > 
> > Also, according to the architecture, TEXASR[FS] HAS TO BE SET on
> > trecheckpoint
> > otherwise you'll get a TM Bad Thing. You should say that rather than
> > suggesting
> > it's because the transaction is doomed. It's ilqlegal to not do it. That's
> > why we
> > have this check in arch/powerpc/kernel/tm.S.
> 
> When you say "HAS TO BE SET", do you mean that the hardware will set it and
> we shouldn't care about this flag? Thus, if I am hitting EMIT_BUG_ENTRY, it
> means my TEXASR was messed somehow?

I'm just saying what you said before about the tm bad thing.  We have to make
sure it's set before we do the trecheckpoint otherwise we end up in the TM bad
thing. The treclaim should handle this for us (but again the signals case need
to be checked).

Mikey

Re: [RFC PATCH 11/11] selftests/powerpc: Adapt the test

[Michael Neuling]

On Thu, 2018-09-27 at 17:57 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/18/2018 03:36 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > The Documentation/powerpc/transactional_memory.txt says:
> > > 
> > >  "Syscalls made from within a suspended transaction are performed as
> > > normal
> > >   and the transaction is not explicitly doomed by the kernel.  However,
> > >   what the kernel does to perform the syscall may result in the
> > > transaction
> > >   being doomed by the hardware."
> > > 
> > > With this new TM mechanism, the syscall will continue to be executed if
> > > the
> > > syscall happens on a suspended syscall, but, the syscall will *fail* if
> > > the
> > > transaction is still active during the syscall invocation.
> > 
> > Not sure I get this. This doesn't seem any different to before.
> > 
> > An active (not suspended) transaction *will* result in the syscall failing
> > and
> > the transaction being doomed.  
> > 
> > A syscall in a suspended transaction should succeed and the transaction.
> 
> I understand that a transaction will always be doomed if there is a
> reclaim/checkpoint, thus, if the you make a system call inside a suspended
> transaction, it will reclaim and recheckpoint, thus, dooming the transaction,
> and failing it on the next RFID.

So currently a syscall won't explicitly treclaim/trecheckpoint so it won't
necessarily be doomed.

With your new patches it will be.

> So, the syscall executed in suspended mode may succeed, because it will not
> be skipped (as in active mode), but the transaction will *always* fail,
> because there was a reclaim and recheckpoint.
> 
> > You might need to clean up the language. I try to use:
> > 
> >    Active == transactional but not suspended (ie MSR[TS] = T)
> >    Suspended == suspended (ie MSR [TS] = S)
> >    Doomed == transaction to be rolled back at next opportinity (ie tcheck
> > returns doomed)
> > 
> > (note: the kernel MSR_TM_ACTIVE() macro is not consistent with this since
> > it's
> > MSR[TS] == S or T).
> 
> Ok, I agree with this language as well. I might want to improve the code to
> follow the same language all across the code.
> 
> > > On the syscall path, if the transaction is active and not suspended, it
> > > will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the
> > > transaction, thus, dooming the transaction on userspace return, with
> > > failure code TM_CAUSE_SYSCALL.
> > 
> > But the test below is on a suspend transaction?
> 
> Sorry, I meant "suspended transaction" above instead of "transaction is
> active and not suspended".
> 
> > 
> > > This new model will break part of this test, but I understand that that
> > > the
> > > documentation above didn't guarantee that the syscall would succeed, and
> > > it
> > > will never succeed anymore now on.
> > 
> > The syscall should pass in suspend (modulo the normal syscall checks). The
> > transaction may fail as a result.
> 
> Perfect, and if the transaction fail, the CPU will rollback the changes and
> restore the checkpoint registers (replacing the r3 that contains the pid
> value), thus, it will be like "getpid" system call didn't execute.

No.  If we are suspended, then we go back right after the sc. We don't get
rolled back till the tresume.

Mikey

> For this test specifically, it assumes the syscall didn't execute if the
> transaction failed. Take a look:
> 
> 	FUNC_START(getppid_tm_suspended)
> 		tbegin.
> 		beq 1f
> 		li      r0, __NR_getppid
> 		tsuspend.
> 		sc
> 		tresume.
> 		tend.
> 		blr
> 	1:
> 		li      r3, -1
> 		blr
> 
> So, the tests thinks the syscall failed because the transaction aborted.
> 
> Anyway, I can try to improve this test other than removing this test, but I
> am not sure how.
> 
> Thank you
> 

Re: [RFC PATCH 05/11] powerpc/tm: Function that updates the failure code

[Michael Neuling]

On Thu, 2018-09-27 at 17:58 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/17/2018 10:29 PM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Now the transaction reclaims happens very earlier in the trap handler, and
> > > it is impossible to know precisely, at that early time, what should be set
> > > as the failure cause for some specific cases, as, if the task will be
> > > rescheduled, thus, the transaction abort case should be updated from
> > > TM_CAUSE_MISC to TM_CAUSE_RESCHED, for example.
> > 
> > Please add comments to where this is used (in EXCEPTION_COMMON macro I
> > think)
> > that say this might happen.
> 
> Is it OK if I comment it in TM_KERNEL_ENTRY macro, since the failure cause
> could be updated independently of the exception being execute, so, every call
> to TM_KERNEL_ENTRY can have the cause overwritten.

Sure.


> I.e. it does not matter if  the exception is a systemcall or a page fault,
> the failure cause will be updated if there is a process reschedule after the
> exception/syscall is handled.
> 
> Thank you
> 

Re: [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace

[Michael Neuling]

On Thu, 2018-09-27 at 18:03 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/18/2018 02:36 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Make sure that we are not suspended on ptrace and that the registers were
> > > already reclaimed.
> > > 
> > > Since the data was already reclaimed, there is nothing to be done here
> > > except to restore the SPRs.
> > > 
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > >  arch/powerpc/kernel/ptrace.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > > index 9667666eb18e..cf6ee9154b11 100644
> > > --- a/arch/powerpc/kernel/ptrace.c
> > > +++ b/arch/powerpc/kernel/ptrace.c
> > > @@ -136,12 +136,10 @@ 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));
> > > -	}
> > > +	WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
> > > +
> > > +	tm_enable();
> > > +	tm_save_sprs(&(tsk->thread));
> > 
> > Do we need to check if TM was enabled in the task before saving the TM SPRs?
> > 
> > What happens if TM was lazily off and hence we had someone else's TM SPRs in
> > the
> > CPU currently?  Wouldn't this flush the wrong values to the task_struct?
> > 
> > I think we need to check the processes MSR before doing this.
> 
> Yes, it is a *very* good point, and I think we are vulnerable even before
> this patch (in the current kernel). Take a look above, we are calling
> tm_save_sprs() if MSR is not TM suspended independently if TM is lazily off.

I think you're right, we might already have an issue.  There are some paths in
here that don't check the userspace msr or any of the lazy tm enable. :(

Mikey


> It shouldn't be hard to create a test case for it. I can try to call
> ptrace(PTRACE_GETVRREGS) on a task that sleeps until TM is lazily disabled,
> compare if the TM SPR changed in this mean time. (while doing HTM in parallel
> to keep HTM SPR changing). Let's see if I can read others task TM SPRs.
> 
> Thank you.
> 

Re: [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace

[Breno Leitao]

Hi Mikey,

On 09/28/2018 02:36 AM, Michael Neuling wrote:
>>>> +	WARN_ON(MSR_TM_SUSPENDED(mfmsr())); + +	tm_enable(); + 
>>>> tm_save_sprs(&(tsk->thread));
>>> 
>>> Do we need to check if TM was enabled in the task before saving the
>>> TM SPRs?
>>> 
>>> What happens if TM was lazily off and hence we had someone else's TM 
>>> SPRs in the CPU currently?  Wouldn't this flush the wrong values to 
>>> the task_struct?
>>> 
>>> I think we need to check the processes MSR before doing this.
>> 
>> Yes, it is a *very* good point, and I think we are vulnerable even 
>> before this patch (in the current kernel). Take a look above, we are 
>> calling tm_save_sprs() if MSR is not TM suspended independently if TM
>> is lazily off.
> 
> I think you're right, we might already have an issue.  There are some 
> paths in here that don't check the userspace msr or any of the lazy tm 
> enable. :(

I was able to create a test case that reproduces this bug cleanly.

The testcase basically sleeps for N cycles, and then segfaults.

If N is high enough to have load_tm overflowed, then you see a corrupted
TEXASR value in the core dump file. If load_tm != 0 during the coredump, you
see the expected TEXASR value.

I wrote a small bash that check for both cases.

  $ git clone https://github.com/leitao/texasr_corrupt.git
  $ make check

Anyway, I will propose a fix for this problem soon, since this whole patchset
may delay to get ready. Is it OK?

Thank you

Re: [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace

[Michael Neuling]

On Sun, 2018-09-30 at 20:51 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/28/2018 02:36 AM, Michael Neuling wrote:
> > > > > +	WARN_ON(MSR_TM_SUSPENDED(mfmsr())); + +	tm_enable(); + 
> > > > > tm_save_sprs(&(tsk->thread));
> > > > 
> > > > Do we need to check if TM was enabled in the task before saving the
> > > > TM SPRs?
> > > > 
> > > > What happens if TM was lazily off and hence we had someone else's TM 
> > > > SPRs in the CPU currently?  Wouldn't this flush the wrong values to 
> > > > the task_struct?
> > > > 
> > > > I think we need to check the processes MSR before doing this.
> > > 
> > > Yes, it is a *very* good point, and I think we are vulnerable even 
> > > before this patch (in the current kernel). Take a look above, we are 
> > > calling tm_save_sprs() if MSR is not TM suspended independently if TM
> > > is lazily off.
> > 
> > I think you're right, we might already have an issue.  There are some 
> > paths in here that don't check the userspace msr or any of the lazy tm 
> > enable. :(
> 
> I was able to create a test case that reproduces this bug cleanly.
> 
> The testcase basically sleeps for N cycles, and then segfaults.
> 
> If N is high enough to have load_tm overflowed, then you see a corrupted
> TEXASR value in the core dump file. If load_tm != 0 during the coredump, you
> see the expected TEXASR value.
> 
> I wrote a small bash that check for both cases.
> 
>   $ git clone https://github.com/leitao/texasr_corrupt.git
>   $ make check
> 
> Anyway, I will propose a fix for this problem soon, since this whole patchset
> may delay to get ready. Is it OK?

Yeah, best to get a fix for this one out soon.

Mikey

Re: [RFC PATCH 11/11] selftests/powerpc: Adapt the test

[Breno Leitao]

Hi Mikey,

On 09/28/2018 02:25 AM, Michael Neuling wrote:
>> Perfect, and if the transaction fail, the CPU will rollback the changes and
>> restore the checkpoint registers (replacing the r3 that contains the pid
>> value), thus, it will be like "getpid" system call didn't execute.
> 
> No.  If we are suspended, then we go back right after the sc. We don't get
> rolled back till the tresume.

Yes, but the test code (below) just run tresume after 'sc'. i.e, the syscall
will execute, but there is a tresume just after the syscall, which will cause
the transaction to rollback and jump to "1:" label, which will replace r3.
with -1.

So, the difference now (with this patchset) is that we are calling
treclaim/trecheckpoint in kernel space, which will doom the transaction. It
was not being done before, thus, the test was passing and it is not anymore.

Anyway,  the other way to check for it is calling 'blr' just after 'sc' (
before 'tresume.'), and then tresume after checking if pid == getpid().

If you prefer this method, I can implement it.

>> For this test specifically, it assumes the syscall didn't execute if the
>> transaction failed. Take a look:
>>
>> 	FUNC_START(getppid_tm_suspended)
>> 		tbegin.
>> 		beq 1f
>> 		li      r0, __NR_getppid
>> 		tsuspend.
>> 		sc
>> 		tresume.
>> 		tend.
>> 		blr
>> 	1:
>> 		li      r3, -1
>> 		blr
>>

Thank you!