Re: [PATCH V10 02/28] powerpc, process: Add the function flush_tmregs_to_thread

On Wed, 02 Mar 2016 09:59:06 +0530
Anshuman Khandual [off-list ref] wrote:

On 03/02/2016 05:45 AM, Cyril Bur wrote:

quoted

On Tue, 16 Feb 2016 14:29:32 +0530
Anshuman Khandual [off-list ref] wrote:
  

quoted

This patch creates a function flush_tmregs_to_thread which
will then be used by subsequent patches in this series. The
function checks for self tracing ptrace interface attempts
while in the TM context and logs appropriate warning message.
 

Hi Anshuman,

You'll have to bare with me, my ptrace knowledge is non existent so you might
have to walk me though some aspects.

I have been playing with FPU/VMX and VSX saving so I thought I'd take a look.  

Sure.

quoted

quoted

Signed-off-by: Anshuman Khandual <redacted>
---
 arch/powerpc/include/asm/switch_to.h |  8 ++++++++
 arch/powerpc/kernel/process.c        | 20 ++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
index 5b268b6..7b297bf 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h

@@ -70,6 +70,14 @@ static inline void disable_kernel_spe(void)
 }
 #endif
 
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+extern void flush_tmregs_to_thread(struct task_struct *);
+#else
+static inline void flush_tmregs_to_thread(struct task_struct *t)
+{
+}
+#endif
+
 static inline void clear_task_ebb(struct task_struct *t)
 {
 #ifdef CONFIG_PPC_BOOK3S_64

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index dccc87e..2c4fa7f 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c

@@ -918,6 +918,26 @@ static inline void restore_sprs(struct thread_struct *old_thread,
 #endif
 }
   

quoted

+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+void flush_tmregs_to_thread(struct task_struct *tsk)
+{
+	/*
+	 * Process self tracing is not yet supported through
+	 * ptrace interface. Ptrace generic code should have
+	 * prevented this from happening in the first place.
+	 * Warn once here with the message, if some how it
+	 * is attempted.
+	 */
+	WARN_ONCE(tsk == current,
+		"Not expecting ptrace on self: TM regs may be incorrect\n");
+
+	/*
+	 * If task is not current, it should have been flushed
+	 * already to it's thread_struct during __switch_to().
+	 */  

I totally agree except this highlights something that I notice in subsequent
patches, and existing code. All the *_{get,set}() functions call
flush_*_to_thread() when, as per your comment (and my understanding of task
switching) there really shouldn't be a need to do that. My only thought is that
this could be a relic of uniprocessor days when it would have been necessary but
Anton recently stripped that out. Are you able to shed some light here?  

Its been sometime I had looked into this aspect of the series. I remember
Michael Neuling and myself discussed about this and settled on a single
WARN_ON here as nothing else was required to be done in the function. It
may be possible that all the flush_*_to_thread() functions used else where
are because of uniprocessor concerns. I dont understand completely our
context save/restore paths including the lazy ones. I believed that these
flush_*_to_thread() routines just made sure task struct has the latest
values of the thread context in case of some complicated save/restore
paths might not have done the complete save at that point in time.

Well as you note in the comment though, it should be done since we've gone
through __switch_to()...

If you think that all these flush_*_to_thread() functions used through
out POWER ptrace need review to see whether they are required or not
anymore I would suggest we should do it as a separate patch after this
series and I am willing to work with you on that.

I THINK your patches are correct and we're just performing needless
flush_*_to_thread() calls now in which case its fine and we can review laster,
my concern is that I've been wrong before so having flush_tmregs_to_thread() do
nothing worries me. I wonder if Mr Neuling or Mr Ellerman have anything to say
on the subject...

quoted

The reason I ask is that if the flush_*_to_thread() calls ARE actually
important then I worry that this function is inadequate...  

I guess we went through that and finally settled on WARN_ON once but dont
remember the exact context now. Will look into all previous discussions
on this and get back.

Thanks,

Cyril