[PATCH 1/2] x86/asm/entry/32: Explain stub32_clone logic

[PATCH 1/2] x86/asm/entry/32: Explain stub32_clone logic

[Denys Vlasenko]

The reason for copying of %r8 to %rcx is quite non-obvious.
Add a comment which explains why it is done.

Fix indentation and trailing whitespace while at it.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---

This is a resend.

 arch/x86/ia32/ia32entry.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index 2ca052e..8e72256 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -562,9 +562,17 @@ GLOBAL(\label)
 
 	ALIGN
 GLOBAL(stub32_clone)
-	leaq sys_clone(%rip),%rax
+	leaq	sys_clone(%rip), %rax
+	/*
+	 * 32-bit clone API is clone(..., int tls_val, int *child_tidptr).
+	 * 64-bit clone API is clone(..., int *child_tidptr, int tls_val).
+	 * Native 64-bit kernel's sys_clone() implements the latter.
+	 * We need to swap args here. But since tls_val is in fact ignored
+	 * by sys_clone(), we can get away with an assignment
+	 * (arg4 = arg5) instead of a full swap:
+	 */
 	mov	%r8, %rcx
-	jmp  ia32_ptregs_common	
+	jmp	ia32_ptregs_common
 
 	ALIGN
 ia32_ptregs_common:
-- 
1.8.1.4

[PATCH 2/2] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

[Denys Vlasenko]

Really swap arguments #4 and #5 in stub32_clone instead of "optimizing"
it into a move.

Yes, tls_val is currently unused. Yes, on some CPUs XCHG is a little bit
more expensive than MOV. But a cycle or two on an expensive syscall like
clone() is way below noise floor, and this optimization is simply not worth
the obfuscation of logic.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---

This is a resend.

There was a patch by Josh Triplett
"x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit"
sent on May 11,
which does the same thing as part of a bigger cleanup.
He was supportive of this patch because of comments.
He will simply have to drop one hunk from his patch.

 arch/x86/ia32/ia32entry.S | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index 8e72256..0c302d0 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -567,11 +567,9 @@ GLOBAL(stub32_clone)
 	 * 32-bit clone API is clone(..., int tls_val, int *child_tidptr).
 	 * 64-bit clone API is clone(..., int *child_tidptr, int tls_val).
 	 * Native 64-bit kernel's sys_clone() implements the latter.
-	 * We need to swap args here. But since tls_val is in fact ignored
-	 * by sys_clone(), we can get away with an assignment
-	 * (arg4 = arg5) instead of a full swap:
+	 * We need to swap args here:
 	 */
-	mov	%r8, %rcx
+	xchg	%r8, %rcx
 	jmp	ia32_ptregs_common
 
 	ALIGN
-- 
1.8.1.4

Re: [PATCH 2/2] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

[Josh Triplett]

On Wed, Jun 03, 2015 at 03:58:50PM +0200, Denys Vlasenko wrote:
> Really swap arguments #4 and #5 in stub32_clone instead of "optimizing"
> it into a move.
> 
> Yes, tls_val is currently unused. Yes, on some CPUs XCHG is a little bit
> more expensive than MOV. But a cycle or two on an expensive syscall like
> clone() is way below noise floor, and this optimization is simply not worth
> the obfuscation of logic.
[...]
> This is a resend.
> 
> There was a patch by Josh Triplett
> "x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit"
> sent on May 11,
> which does the same thing as part of a bigger cleanup.
> He was supportive of this patch because of comments.
> He will simply have to drop one hunk from his patch.

Strictly speaking, nothing needs this until clone starts paying
attention to its tls argument, which only happens in my cleanup series
that includes this change.  So what's the purpose of driving this patch
separately?

- Josh Triplett

Re: [PATCH 2/2] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

[Denys Vlasenko]

On 06/03/2015 06:38 PM, Josh Triplett wrote:
> On Wed, Jun 03, 2015 at 03:58:50PM +0200, Denys Vlasenko wrote:
>> Really swap arguments #4 and #5 in stub32_clone instead of "optimizing"
>> it into a move.
>>
>> Yes, tls_val is currently unused. Yes, on some CPUs XCHG is a little bit
>> more expensive than MOV. But a cycle or two on an expensive syscall like
>> clone() is way below noise floor, and this optimization is simply not worth
>> the obfuscation of logic.
> [...]
>> This is a resend.
>>
>> There was a patch by Josh Triplett
>> "x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit"
>> sent on May 11,
>> which does the same thing as part of a bigger cleanup.
>> He was supportive of this patch because of comments.
>> He will simply have to drop one hunk from his patch.
> 
> Strictly speaking, nothing needs this until clone starts paying
> attention to its tls argument, which only happens in my cleanup series
> that includes this change.  So what's the purpose of driving this patch
> separately?

You wanted my comments in this patch to go in:

On 04/22/2015 07:10 PM, Josh Triplett wrote:
> I do think my two-patch HAVE_COPY_THREAD_TLS series should go in fixing
> this, but I'd like to see the final version of Denys' comment added on
> top of it (with an update to the type and name of the tls argument to
> match the changes to sys_clone).

If your patch will go in first, I'll send a patch adding only the comment.

Since for now your patch did not make it yet, I'm submitting
a patch which adds both a comment and the insn change.

-- 
vda

Re: [PATCH 2/2] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

[Josh Triplett]

On Thu, Jun 04, 2015 at 12:07:31PM +0200, Denys Vlasenko wrote:
> On 06/03/2015 06:38 PM, Josh Triplett wrote:
> > On Wed, Jun 03, 2015 at 03:58:50PM +0200, Denys Vlasenko wrote:
> >> Really swap arguments #4 and #5 in stub32_clone instead of "optimizing"
> >> it into a move.
> >>
> >> Yes, tls_val is currently unused. Yes, on some CPUs XCHG is a little bit
> >> more expensive than MOV. But a cycle or two on an expensive syscall like
> >> clone() is way below noise floor, and this optimization is simply not worth
> >> the obfuscation of logic.
> > [...]
> >> This is a resend.
> >>
> >> There was a patch by Josh Triplett
> >> "x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit"
> >> sent on May 11,
> >> which does the same thing as part of a bigger cleanup.
> >> He was supportive of this patch because of comments.
> >> He will simply have to drop one hunk from his patch.
> > 
> > Strictly speaking, nothing needs this until clone starts paying
> > attention to its tls argument, which only happens in my cleanup series
> > that includes this change.  So what's the purpose of driving this patch
> > separately?
> 
> You wanted my comments in this patch to go in:
> 
> On 04/22/2015 07:10 PM, Josh Triplett wrote:
> > I do think my two-patch HAVE_COPY_THREAD_TLS series should go in fixing
> > this, but I'd like to see the final version of Denys' comment added on
> > top of it (with an update to the type and name of the tls argument to
> > match the changes to sys_clone).
> 
> If your patch will go in first, I'll send a patch adding only the comment.
> 
> Since for now your patch did not make it yet, I'm submitting
> a patch which adds both a comment and the insn change.

Ah, I see.

My two-patch series is currently sitting in -mm; would you consider
providing a version of the patch that adds the comment for Andrew to
apply on top of those?

- Josh Triplett

Re: [PATCH 2/2] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

[Ingo Molnar]

* Josh Triplett <josh@joshtriplett.org> wrote:

> On Thu, Jun 04, 2015 at 12:07:31PM +0200, Denys Vlasenko wrote:
> > On 06/03/2015 06:38 PM, Josh Triplett wrote:
> > > On Wed, Jun 03, 2015 at 03:58:50PM +0200, Denys Vlasenko wrote:
> > >> Really swap arguments #4 and #5 in stub32_clone instead of "optimizing"
> > >> it into a move.
> > >>
> > >> Yes, tls_val is currently unused. Yes, on some CPUs XCHG is a little bit
> > >> more expensive than MOV. But a cycle or two on an expensive syscall like
> > >> clone() is way below noise floor, and this optimization is simply not worth
> > >> the obfuscation of logic.
> > > [...]
> > >> This is a resend.
> > >>
> > >> There was a patch by Josh Triplett
> > >> "x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit"
> > >> sent on May 11,
> > >> which does the same thing as part of a bigger cleanup.
> > >> He was supportive of this patch because of comments.
> > >> He will simply have to drop one hunk from his patch.
> > > 
> > > Strictly speaking, nothing needs this until clone starts paying
> > > attention to its tls argument, which only happens in my cleanup series
> > > that includes this change.  So what's the purpose of driving this patch
> > > separately?
> > 
> > You wanted my comments in this patch to go in:
> > 
> > On 04/22/2015 07:10 PM, Josh Triplett wrote:
> > > I do think my two-patch HAVE_COPY_THREAD_TLS series should go in fixing
> > > this, but I'd like to see the final version of Denys' comment added on
> > > top of it (with an update to the type and name of the tls argument to
> > > match the changes to sys_clone).
> > 
> > If your patch will go in first, I'll send a patch adding only the comment.
> > 
> > Since for now your patch did not make it yet, I'm submitting
> > a patch which adds both a comment and the insn change.
> 
> Ah, I see.
> 
> My two-patch series is currently sitting in -mm; would you consider providing a 
> version of the patch that adds the comment for Andrew to apply on top of those?

So because -mm is based on linux-next, and linux-next includes the x86 tree, 
amongst them the x86/asm topic, the dependency is the other way around.

Unrelated to these changes to the clone stub there will be conflicts anyway due to 
larger reorganization in the x86 assembly code: so I've picked up Denys's two 
patches, thus your patch set in -mm will become a bit smaller in the end, because 
it can rely on having these changes applied already.

Thanks,

	Ingo

[tip:x86/asm] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

[tip-bot for Denys Vlasenko]

Commit-ID:  7a5a9824c18f93415944c997dc6bb8eecfddd2e7
Gitweb:     http://git.kernel.org/tip/7a5a9824c18f93415944c997dc6bb8eecfddd2e7
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Wed, 3 Jun 2015 15:58:50 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 5 Jun 2015 13:41:28 +0200

x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

Really swap arguments #4 and #5 in stub32_clone instead of
"optimizing" it into a move.

Yes, tls_val is currently unused. Yes, on some CPUs XCHG is a
little bit more expensive than MOV. But a cycle or two on an
expensive syscall like clone() is way below noise floor, and
this optimization is simply not worth the obfuscation of logic.

[ There's also ongoing work on the clone() ABI by Josh Triplett
  that will depend on this change later on. ]

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1433339930-20880-2-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/ia32entry.S | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/entry/ia32entry.S b/arch/x86/entry/ia32entry.S
index d0c7b28..9558dac 100644
--- a/arch/x86/entry/ia32entry.S
+++ b/arch/x86/entry/ia32entry.S
@@ -529,14 +529,13 @@ GLOBAL(\label)
 GLOBAL(stub32_clone)
 	leaq	sys_clone(%rip), %rax
 	/*
-	 * 32-bit clone API is clone(..., int tls_val, int *child_tidptr).
-	 * 64-bit clone API is clone(..., int *child_tidptr, int tls_val).
-	 * Native 64-bit kernel's sys_clone() implements the latter.
-	 * We need to swap args here. But since tls_val is in fact ignored
-	 * by sys_clone(), we can get away with an assignment
-	 * (arg4 = arg5) instead of a full swap:
+	 * The 32-bit clone ABI is: clone(..., int tls_val, int *child_tidptr).
+	 * The 64-bit clone ABI is: clone(..., int *child_tidptr, int tls_val).
+	 *
+	 * The native 64-bit kernel's sys_clone() implements the latter,
+	 * so we need to swap arguments here before calling it:
 	 */
-	mov	%r8, %rcx
+	xchg	%r8, %rcx
 	jmp	ia32_ptregs_common
 
 	ALIGN

[tip:x86/asm] x86/asm/entry/32: Explain the stub32_clone logic

[tip-bot for Denys Vlasenko]

Commit-ID:  5cdc683b7d8b3341a3d18e0c5498bc1e4f3fb990
Gitweb:     http://git.kernel.org/tip/5cdc683b7d8b3341a3d18e0c5498bc1e4f3fb990
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Wed, 3 Jun 2015 15:58:49 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 5 Jun 2015 13:41:27 +0200

x86/asm/entry/32: Explain the stub32_clone logic

The reason for copying of %r8 to %rcx is quite non-obvious.
Add a comment which explains why it is done.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1433339930-20880-1-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/ia32entry.S | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/entry/ia32entry.S b/arch/x86/entry/ia32entry.S
index 4bb9f7b..d0c7b28 100644
--- a/arch/x86/entry/ia32entry.S
+++ b/arch/x86/entry/ia32entry.S
@@ -528,6 +528,14 @@ GLOBAL(\label)
 	ALIGN
 GLOBAL(stub32_clone)
 	leaq	sys_clone(%rip), %rax
+	/*
+	 * 32-bit clone API is clone(..., int tls_val, int *child_tidptr).
+	 * 64-bit clone API is clone(..., int *child_tidptr, int tls_val).
+	 * Native 64-bit kernel's sys_clone() implements the latter.
+	 * We need to swap args here. But since tls_val is in fact ignored
+	 * by sys_clone(), we can get away with an assignment
+	 * (arg4 = arg5) instead of a full swap:
+	 */
 	mov	%r8, %rcx
 	jmp	ia32_ptregs_common