Re: [PATCH v14 00/15] selftests/powerpc: Add ptrace tests for ppc registers

From: Simon Guo <wei.guo.simon@gmail.com>
To: Cyril Bur <cyrilbur@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org,
	Michael Ellerman <mpe@ellerman.id.au>,
	Suraj Jitindar Singh <sjitindarsingh@gmail.com>,
	Anshuman Khandual <khandual@linux.vnet.ibm.com>,
	Rashmica Gupta <rashmicy@gmail.com>
Subject: Re: [PATCH v14 00/15] selftests/powerpc: Add ptrace tests for ppc registers
Date: Tue, 13 Sep 2016 01:01:05 +0800	[thread overview]
Message-ID: <20160912170105.GA17588@simonLocalRHEL7.x64> (raw)
In-Reply-To: <1473745750.15800.1.camel@gmail.com>

Hi Cyril,
On Tue, Sep 13, 2016 at 03:49:10PM +1000, Cyril Bur wrote:
> Thanks for putting the effort in to get these merged! I have a few
> remarks that apply to more than one patch which I'll say here.
> 
> I'm not sure #defining the TM instructions as .long for the selftests
> is useful. Compilers these days know about the instructions 'tbegin.'
> 'tsuspend.' and the like, I would question anyone using a compiler old
> enough not to know about these...
I agree. But let me check with original author Anshuman firstly.

> 
> There are a few assembly fpu register load functions that could be
> consolidated into those in math/ and even some in tm/
Will rework that.

> 
> Doing while(ptr); to wait for another thread should be 
> 
> while(ptr)
>     asm volatile("" : : : "memory");
> 
> Documentation/volatile-considered-harmful.txt for reasons why.
> Even knowing this I did it your way without thinking in a selftest I
> wrote doing similar things and it turns out that it didn't work [the
> way we both expect it would].
You are right.

> 
> Having said all that, I'm aware that these are selftests and this
> series could be nicer but I won't lose any sleep if they were merged
> almost as is. Thanks for your work!
> 
> Finally, they didn't compile for me, I did a git rebase --exec with my
> build scripts and:
> 
> selftests/powerpc: Add ptrace tests for EBB
> 	[snip]
> 	*** No rule to make target 'ptrace.S', needed by 'ptrace-ebb'.
> (that appears fixed by subsequent patch)
> 
> selftests/powerpc: Add ptrace tests for GPR/FPR registers
> 	Seems to have failed horribly and those problems continue...
> 
> I applied these to powerpc-next at:
> commit c6935931c1894ff857616ff8549b61236a19148f
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Sun Sep 4 14:31:46 2016 -0700
> 
>     Linux 4.8-rc5
> 
> Should I have based on something else?
I didn't reproduce the latter error and I also applied on c69359.
My build script is only one line:
make -C tools/testing/selftests TARGETS=powerpc 1>/dev/null

Did I miss anything with your build script?
Anyway I need to fix that.

Thanks for the sharing. Most are good comments and I will rework that.

BR,
- Simon