[PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86

[PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86

[Chris Metcalf]

This change is a prerequisite change for TASK_ISOLATION but also
stands on its own for readability and maintainability.  The existing
arm64 do_notify_resume() is called in a loop from assembly on
the slow path; this change moves the loop into C code as well.
For the x86 version see commit c5c46f59e4e7 ("x86/entry: Add new,
comprehensible entry and exit handlers written in C").

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 arch/arm64/kernel/entry.S  |  6 +++---
 arch/arm64/kernel/signal.c | 32 ++++++++++++++++++++++----------
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 7ed3d75f6304..04eff4c4ac6e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -630,9 +630,8 @@ work_pending:
 	mov	x0, sp				// 'regs'
 	tst	x2, #PSR_MODE_MASK		// user mode regs?
 	b.ne	no_work_pending			// returning to kernel
-	enable_irq				// enable interrupts for do_notify_resume()
-	bl	do_notify_resume
-	b	ret_to_user
+	bl	prepare_exit_to_usermode
+	b	no_user_work_pending
 work_resched:
 	bl	schedule
 
@@ -644,6 +643,7 @@ ret_to_user:
 	ldr	x1, [tsk, #TI_FLAGS]
 	and	x2, x1, #_TIF_WORK_MASK
 	cbnz	x2, work_pending
+no_user_work_pending:
 	enable_step_tsk x1, x2
 no_work_pending:
 	kernel_exit 0
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e18c48cb6db1..fde59c1139a9 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -399,18 +399,30 @@ static void do_signal(struct pt_regs *regs)
 	restore_saved_sigmask();
 }
 
-asmlinkage void do_notify_resume(struct pt_regs *regs,
-				 unsigned int thread_flags)
+asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs,
+					 unsigned int thread_flags)
 {
-	if (thread_flags & _TIF_SIGPENDING)
-		do_signal(regs);
+	do {
+		local_irq_enable();
 
-	if (thread_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
-		tracehook_notify_resume(regs);
-	}
+		if (thread_flags & _TIF_NEED_RESCHED)
+			schedule();
+
+		if (thread_flags & _TIF_SIGPENDING)
+			do_signal(regs);
+
+		if (thread_flags & _TIF_NOTIFY_RESUME) {
+			clear_thread_flag(TIF_NOTIFY_RESUME);
+			tracehook_notify_resume(regs);
+		}
+
+		if (thread_flags & _TIF_FOREIGN_FPSTATE)
+			fpsimd_restore_current_state();
+
+		local_irq_disable();
 
-	if (thread_flags & _TIF_FOREIGN_FPSTATE)
-		fpsimd_restore_current_state();
+		thread_flags = READ_ONCE(current_thread_info()->flags) &
+			_TIF_WORK_MASK;
 
+	} while (thread_flags);
 }
-- 
2.1.2

[PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86

[Mark Rutland]

Hi,

On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
> This change is a prerequisite change for TASK_ISOLATION but also
> stands on its own for readability and maintainability. 

I have also been looking into converting the userspace return path from
assembly to C [1], for the latter two reasons. Based on that, I have a
couple of comments.

> The existing arm64 do_notify_resume() is called in a loop from
> assembly on the slow path; this change moves the loop into C code as
> well.  For the x86 version see commit c5c46f59e4e7 ("x86/entry: Add
> new, comprehensible entry and exit handlers written in C").
>
> Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
> ---
>  arch/arm64/kernel/entry.S  |  6 +++---
>  arch/arm64/kernel/signal.c | 32 ++++++++++++++++++++++----------
>  2 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 7ed3d75f6304..04eff4c4ac6e 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -630,9 +630,8 @@ work_pending:
>  	mov	x0, sp				// 'regs'
>  	tst	x2, #PSR_MODE_MASK		// user mode regs?
>  	b.ne	no_work_pending			// returning to kernel
> -	enable_irq				// enable interrupts for do_notify_resume()
> -	bl	do_notify_resume
> -	b	ret_to_user
> +	bl	prepare_exit_to_usermode
> +	b	no_user_work_pending
>  work_resched:
>  	bl	schedule
>  
> @@ -644,6 +643,7 @@ ret_to_user:
>  	ldr	x1, [tsk, #TI_FLAGS]
>  	and	x2, x1, #_TIF_WORK_MASK
>  	cbnz	x2, work_pending
> +no_user_work_pending:
>  	enable_step_tsk x1, x2
>  no_work_pending:
>  	kernel_exit 0

It seems unfortunate to leave behind portions of the entry.S
_TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall,
and the majority of work_pending and ret_to_user).

I think it would be nicer if we could handle all of that in one place
(or at least all in C).

> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index e18c48cb6db1..fde59c1139a9 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -399,18 +399,30 @@ static void do_signal(struct pt_regs *regs)
>  	restore_saved_sigmask();
>  }
>  
> -asmlinkage void do_notify_resume(struct pt_regs *regs,
> -				 unsigned int thread_flags)
> +asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs,
> +					 unsigned int thread_flags)
>  {
> -	if (thread_flags & _TIF_SIGPENDING)
> -		do_signal(regs);
> +	do {
> +		local_irq_enable();
>  
> -	if (thread_flags & _TIF_NOTIFY_RESUME) {
> -		clear_thread_flag(TIF_NOTIFY_RESUME);
> -		tracehook_notify_resume(regs);
> -	}
> +		if (thread_flags & _TIF_NEED_RESCHED)
> +			schedule();

Previously, had we called schedule(), we'd reload the thread info flags
and start that state machine again, whereas now we'll handle all the
cached flags before reloading.

Are we sure nothing is relying on the prior behaviour?

> +
> +		if (thread_flags & _TIF_SIGPENDING)
> +			do_signal(regs);
> +
> +		if (thread_flags & _TIF_NOTIFY_RESUME) {
> +			clear_thread_flag(TIF_NOTIFY_RESUME);
> +			tracehook_notify_resume(regs);
> +		}
> +
> +		if (thread_flags & _TIF_FOREIGN_FPSTATE)
> +			fpsimd_restore_current_state();
> +
> +		local_irq_disable();
>  
> -	if (thread_flags & _TIF_FOREIGN_FPSTATE)
> -		fpsimd_restore_current_state();
> +		thread_flags = READ_ONCE(current_thread_info()->flags) &
> +			_TIF_WORK_MASK;
>  
> +	} while (thread_flags);
>  }

Other than that, this looks good to me.

Thanks,
Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm

[PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86

[Chris Metcalf]

On 01/04/2016 03:33 PM, Mark Rutland wrote:
> Hi,
>
> On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
>> This change is a prerequisite change for TASK_ISOLATION but also
>> stands on its own for readability and maintainability.
> I have also been looking into converting the userspace return path from
> assembly to C [1], for the latter two reasons. Based on that, I have a
> couple of comments.

Thanks!

> It seems unfortunate to leave behind portions of the entry.S
> _TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall,
> and the majority of work_pending and ret_to_user).
>
> I think it would be nicer if we could handle all of that in one place
> (or at least all in C).

Yes, in principle I agree with this, and I think your deasm tree looks
like an excellent idea.

For this patch series I wanted to focus more on what was necessary
for the various platforms to implement task isolation, and less on
additional cleanups of the platforms in question.  I think my changes
don't make the TIF state machine any less clear, nor do they make
it harder for an eventual further migration to C code along the lines
of what you've done, so it seems plausible to me to commit them
upstream independently of your work.

>> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> index e18c48cb6db1..fde59c1139a9 100644
>> --- a/arch/arm64/kernel/signal.c
>> +++ b/arch/arm64/kernel/signal.c
>> @@ -399,18 +399,30 @@ static void do_signal(struct pt_regs *regs)
>>   	restore_saved_sigmask();
>>   }
>>   
>> -asmlinkage void do_notify_resume(struct pt_regs *regs,
>> -				 unsigned int thread_flags)
>> +asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs,
>> +					 unsigned int thread_flags)
>>   {
>> -	if (thread_flags & _TIF_SIGPENDING)
>> -		do_signal(regs);
>> +	do {
>> +		local_irq_enable();
>>   
>> -	if (thread_flags & _TIF_NOTIFY_RESUME) {
>> -		clear_thread_flag(TIF_NOTIFY_RESUME);
>> -		tracehook_notify_resume(regs);
>> -	}
>> +		if (thread_flags & _TIF_NEED_RESCHED)
>> +			schedule();
> Previously, had we called schedule(), we'd reload the thread info flags
> and start that state machine again, whereas now we'll handle all the
> cached flags before reloading.
>
> Are we sure nothing is relying on the prior behaviour?

Good eye, and I probably should have called that out in the commit
message.  My best guess is that there should be nothing that depends
on the old semantics.  Other platforms (certainly x86 and tile, anyway)
already have the semantics that you run out the old state machine on
return from schedule(), so regardless, it's probably appropriate for
arm to follow that same convention.

>> +
>> +		if (thread_flags & _TIF_SIGPENDING)
>> +			do_signal(regs);
>> +
>> +		if (thread_flags & _TIF_NOTIFY_RESUME) {
>> +			clear_thread_flag(TIF_NOTIFY_RESUME);
>> +			tracehook_notify_resume(regs);
>> +		}
>> +
>> +		if (thread_flags & _TIF_FOREIGN_FPSTATE)
>> +			fpsimd_restore_current_state();
>> +
>> +		local_irq_disable();
>>   
>> -	if (thread_flags & _TIF_FOREIGN_FPSTATE)
>> -		fpsimd_restore_current_state();
>> +		thread_flags = READ_ONCE(current_thread_info()->flags) &
>> +			_TIF_WORK_MASK;
>>   
>> +	} while (thread_flags);
>>   }
> Other than that, this looks good to me.
>
> Thanks,
> Mark.
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm

Thanks again for the review - shall I add your Reviewed-by (or Acked-by?)
to this patch?

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

[PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86

[Mark Rutland]

On Mon, Jan 04, 2016 at 04:01:05PM -0500, Chris Metcalf wrote:
> On 01/04/2016 03:33 PM, Mark Rutland wrote:
> >Hi,
> >
> >On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
> >>This change is a prerequisite change for TASK_ISOLATION but also
> >>stands on its own for readability and maintainability.
> >I have also been looking into converting the userspace return path from
> >assembly to C [1], for the latter two reasons. Based on that, I have a
> >couple of comments.
> 
> Thanks!
> 
> >It seems unfortunate to leave behind portions of the entry.S
> >_TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall,
> >and the majority of work_pending and ret_to_user).
> >
> >I think it would be nicer if we could handle all of that in one place
> >(or at least all in C).
> 
> Yes, in principle I agree with this, and I think your deasm tree looks
> like an excellent idea.
> 
> For this patch series I wanted to focus more on what was necessary
> for the various platforms to implement task isolation, and less on
> additional cleanups of the platforms in question.  I think my changes
> don't make the TIF state machine any less clear, nor do they make
> it harder for an eventual further migration to C code along the lines
> of what you've done, so it seems plausible to me to commit them
> upstream independently of your work.

I appreciate that you don't want to rewrite all the code.

However, I think it's easier to factor out a small amount of additional
code now and evlove that as a whole than it will be to evolve part of it
and try to put it back together later.

I have a patch which I will reply with momentarily.

Thanks,
Mark.

[PATCH 1/2] arm64: entry: remove pointless SPSR mode check

[Mark Rutland]

In work_pending  we may skip work if the stacked SPSR value represents
anything other than an EL0 context. We then immediately invoke the
kernel_exit 0 macro as part of ret_to_user, assuming a return to EL0.
This is somewhat confusing.

We use work_pending as part of the ret_to_user/ret_fast_syscall state
machine. We only use ret_fast_syscall in the return from an SVC issued
from EL0. We use ret_to_user for return from EL0 exception handlers and
also for return from ret_from_fork in the case the task was not a kernel
thread (i.e. it is a user task).

Thus in all cases the stacked SPSR value must represent an EL0 context,
and the check is redundant. This patch removes it, along with the now
unused no_work_pending label.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/entry.S | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 7ed3d75..6b30ab1 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -626,10 +626,7 @@ ret_fast_syscall_trace:
 work_pending:
 	tbnz	x1, #TIF_NEED_RESCHED, work_resched
 	/* TIF_SIGPENDING, TIF_NOTIFY_RESUME or TIF_FOREIGN_FPSTATE case */
-	ldr	x2, [sp, #S_PSTATE]
 	mov	x0, sp				// 'regs'
-	tst	x2, #PSR_MODE_MASK		// user mode regs?
-	b.ne	no_work_pending			// returning to kernel
 	enable_irq				// enable interrupts for do_notify_resume()
 	bl	do_notify_resume
 	b	ret_to_user
@@ -645,7 +642,6 @@ ret_to_user:
 	and	x2, x1, #_TIF_WORK_MASK
 	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
-no_work_pending:
 	kernel_exit 0
 ENDPROC(ret_to_user)
 
-- 
1.9.1

[PATCH 1/2] arm64: entry: remove pointless SPSR mode check

[Catalin Marinas]

On Tue, Jan 05, 2016 at 05:33:34PM +0000, Mark Rutland wrote:
> In work_pending  we may skip work if the stacked SPSR value represents
> anything other than an EL0 context. We then immediately invoke the
> kernel_exit 0 macro as part of ret_to_user, assuming a return to EL0.
> This is somewhat confusing.
> 
> We use work_pending as part of the ret_to_user/ret_fast_syscall state
> machine. We only use ret_fast_syscall in the return from an SVC issued
> from EL0. We use ret_to_user for return from EL0 exception handlers and
> also for return from ret_from_fork in the case the task was not a kernel
> thread (i.e. it is a user task).
> 
> Thus in all cases the stacked SPSR value must represent an EL0 context,
> and the check is redundant. This patch removes it, along with the now
> unused no_work_pending label.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Will Deacon <will.deacon@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 2/2] arm64: factor work_pending state machine to C

[Mark Rutland]

Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
state machine that can be difficult to reason about due to duplicated
code and a large number of branch targets.

This patch factors the common logic out into the existing
do_notify_resume function, converting the code to C in the process,
making the code more legible.

This patch tries to mirror the existing behaviour as closely as possible
while using the usual C control flow primitives. There should be no
functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/entry.S  | 24 +++---------------------
 arch/arm64/kernel/signal.c | 36 ++++++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 6b30ab1..41f5dfc 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -612,35 +612,17 @@ ret_fast_syscall:
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK
 	cbnz	x2, ret_fast_syscall_trace
-	and	x2, x1, #_TIF_WORK_MASK
-	cbnz	x2, work_pending
-	enable_step_tsk x1, x2
-	kernel_exit 0
+	b	ret_to_user
 ret_fast_syscall_trace:
 	enable_irq				// enable interrupts
 	b	__sys_trace_return_skipped	// we already saved x0
 
 /*
- * Ok, we need to do extra processing, enter the slow path.
- */
-work_pending:
-	tbnz	x1, #TIF_NEED_RESCHED, work_resched
-	/* TIF_SIGPENDING, TIF_NOTIFY_RESUME or TIF_FOREIGN_FPSTATE case */
-	mov	x0, sp				// 'regs'
-	enable_irq				// enable interrupts for do_notify_resume()
-	bl	do_notify_resume
-	b	ret_to_user
-work_resched:
-	bl	schedule
-
-/*
  * "slow" syscall return path.
  */
 ret_to_user:
-	disable_irq				// disable interrupts
-	ldr	x1, [tsk, #TI_FLAGS]
-	and	x2, x1, #_TIF_WORK_MASK
-	cbnz	x2, work_pending
+	bl	do_notify_resume
+	ldr	x1, [tsk, #TI_FLAGS]		// re-check for single-step
 	enable_step_tsk x1, x2
 	kernel_exit 0
 ENDPROC(ret_to_user)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e18c48c..3a6c60b 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -399,18 +399,34 @@ static void do_signal(struct pt_regs *regs)
 	restore_saved_sigmask();
 }
 
-asmlinkage void do_notify_resume(struct pt_regs *regs,
-				 unsigned int thread_flags)
+asmlinkage void do_notify_resume(void)
 {
-	if (thread_flags & _TIF_SIGPENDING)
-		do_signal(regs);
+	struct pt_regs *regs = task_pt_regs(current);
+	unsigned long thread_flags;
 
-	if (thread_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
-		tracehook_notify_resume(regs);
-	}
+	for (;;) {
+		local_irq_disable();
+
+		thread_flags = READ_ONCE(current_thread_info()->flags);
+		if (!(thread_flags & _TIF_WORK_MASK))
+			break;
+
+		if (thread_flags & _TIF_NEED_RESCHED) {
+			schedule();
+			continue;
+		}
 
-	if (thread_flags & _TIF_FOREIGN_FPSTATE)
-		fpsimd_restore_current_state();
+		local_irq_enable();
 
+		if (thread_flags & _TIF_SIGPENDING)
+			do_signal(regs);
+
+		if (thread_flags & _TIF_NOTIFY_RESUME) {
+			clear_thread_flag(TIF_NOTIFY_RESUME);
+			tracehook_notify_resume(regs);
+		}
+
+		if (thread_flags & _TIF_FOREIGN_FPSTATE)
+			fpsimd_restore_current_state();
+	}
 }
-- 
1.9.1

[PATCH 2/2] arm64: factor work_pending state machine to C

[Chris Metcalf]

On 01/05/2016 12:33 PM, Mark Rutland wrote:
> Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
> state machine that can be difficult to reason about due to duplicated
> code and a large number of branch targets.
>
> This patch factors the common logic out into the existing
> do_notify_resume function, converting the code to C in the process,
> making the code more legible.
>
> This patch tries to mirror the existing behaviour as closely as possible
> while using the usual C control flow primitives. There should be no
> functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland<mark.rutland@arm.com>
> Cc: Catalin Marinas<catalin.marinas@arm.com>
> Cc: Chris Metcalf<cmetcalf@ezchip.com>
> Cc: Will Deacon<will.deacon@arm.com>
> ---
>   arch/arm64/kernel/entry.S  | 24 +++---------------------
>   arch/arm64/kernel/signal.c | 36 ++++++++++++++++++++++++++----------
>   2 files changed, 29 insertions(+), 31 deletions(-)

This looks good, and also makes the task isolation change drop in
very cleanly (relatively speaking).  Since do_notify_resume() is
called unconditionally now, we don't have to worry about fussing
with the bit numbering for the TIF_xxx flags in asm/threadinfo.h, so
that whole part of the patch can be dropped, and the actual
change to do_notify_resume() becomes:

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 3a6c60beadca..00d0ec3a8e60 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -25,6 +25,7 @@
  #include <linux/uaccess.h>
  #include <linux/tracehook.h>
  #include <linux/ratelimit.h>
+#include <linux/isolation.h>
  
  #include <asm/debug-monitors.h>
  #include <asm/elf.h>
@@ -408,7 +409,8 @@ asmlinkage void do_notify_resume(void)
                 local_irq_disable();
  
                 thread_flags = READ_ONCE(current_thread_info()->flags);
-               if (!(thread_flags & _TIF_WORK_MASK))
+               if (!(thread_flags & _TIF_WORK_MASK) &&
+                   task_isolation_ready())
                         break;
  
                 if (thread_flags & _TIF_NEED_RESCHED) {
@@ -428,5 +430,7 @@ asmlinkage void do_notify_resume(void)
  
                 if (thread_flags & _TIF_FOREIGN_FPSTATE)
                         fpsimd_restore_current_state();
+
+               task_isolation_enter();
         }
  }


For the moment I just added your two commits into my task-isolation
tree and pushed it up, but if your changes make it into 4.5 and the
task-isolation series doesn't, I will remove them and rebase on 4.5-rc1
once that's released.  I've similarly staged the arch/tile enablement
changes to go into 4.5 so I can drop them from the task-isolation tree
as well at that point.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

[PATCH 2/2] arm64: factor work_pending state machine to C

[Catalin Marinas]

On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
> Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
> state machine that can be difficult to reason about due to duplicated
> code and a large number of branch targets.
> 
> This patch factors the common logic out into the existing
> do_notify_resume function, converting the code to C in the process,
> making the code more legible.
> 
> This patch tries to mirror the existing behaviour as closely as possible
> while using the usual C control flow primitives. There should be no
> functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Will Deacon <will.deacon@arm.com>

This is definitely cleaner. The only downside is slightly more expensive
ret_fast_syscall. I guess it's not noticeable (though we could do some
quick benchmark like getpid in a loop). Anyway, I'm fine with the patch:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 2/2] arm64: factor work_pending state machine to C

[Mark Rutland]

On Wed, Jan 06, 2016 at 12:30:11PM +0000, Catalin Marinas wrote:
> On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
> > Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
> > state machine that can be difficult to reason about due to duplicated
> > code and a large number of branch targets.
> > 
> > This patch factors the common logic out into the existing
> > do_notify_resume function, converting the code to C in the process,
> > making the code more legible.
> > 
> > This patch tries to mirror the existing behaviour as closely as possible
> > while using the usual C control flow primitives. There should be no
> > functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Chris Metcalf <cmetcalf@ezchip.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> 
> This is definitely cleaner. The only downside is slightly more expensive
> ret_fast_syscall. I guess it's not noticeable (though we could do some
> quick benchmark like getpid in a loop). Anyway, I'm fine with the patch:
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Cheers!

While any additional overhead hasn't been noticeable, I'll try to get
some numbers out as part of the larger deasm testing/benchmarking.

Thanks,
Mark.

[PATCH 2/2] arm64: factor work_pending state machine to C

[Mark Rutland]

On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
> Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
> state machine that can be difficult to reason about due to duplicated
> code and a large number of branch targets.
> 
> This patch factors the common logic out into the existing
> do_notify_resume function, converting the code to C in the process,
> making the code more legible.
> 
> This patch tries to mirror the existing behaviour as closely as possible
> while using the usual C control flow primitives. There should be no
> functional change as a result of this patch.

I realised there is a problem with this for kernel built with
TRACE_IRQFLAGS, as local_irq_{enable,disable}() will verify that the IRQ
state is as expected.

In ret_fast_syscall we disable irqs behind the back of the tracer, so
when we get into do_notify_resume we'll get a splat.

In the non-syscall cases we do not disable interrupts first, so we can't
balance things in do_notify_resume.

We can either add a trace_hardirqs_off call to ret_fast_syscall, or we
can use raw_local_irq_{disable,enable}. The latter would match the
current behaviour (and is a nicer diff). Once the syscall path is moved
to C it would be possible to use the non-raw variants all-over.

Catalin, are you happy with using the raw accessors in do_notify_resume,
or would you prefer using trace_hardirqs_off?

Thanks,
Mark.

> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/entry.S  | 24 +++---------------------
>  arch/arm64/kernel/signal.c | 36 ++++++++++++++++++++++++++----------
>  2 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 6b30ab1..41f5dfc 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -612,35 +612,17 @@ ret_fast_syscall:
>  	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
>  	and	x2, x1, #_TIF_SYSCALL_WORK
>  	cbnz	x2, ret_fast_syscall_trace
> -	and	x2, x1, #_TIF_WORK_MASK
> -	cbnz	x2, work_pending
> -	enable_step_tsk x1, x2
> -	kernel_exit 0
> +	b	ret_to_user
>  ret_fast_syscall_trace:
>  	enable_irq				// enable interrupts
>  	b	__sys_trace_return_skipped	// we already saved x0
>  
>  /*
> - * Ok, we need to do extra processing, enter the slow path.
> - */
> -work_pending:
> -	tbnz	x1, #TIF_NEED_RESCHED, work_resched
> -	/* TIF_SIGPENDING, TIF_NOTIFY_RESUME or TIF_FOREIGN_FPSTATE case */
> -	mov	x0, sp				// 'regs'
> -	enable_irq				// enable interrupts for do_notify_resume()
> -	bl	do_notify_resume
> -	b	ret_to_user
> -work_resched:
> -	bl	schedule
> -
> -/*
>   * "slow" syscall return path.
>   */
>  ret_to_user:
> -	disable_irq				// disable interrupts
> -	ldr	x1, [tsk, #TI_FLAGS]
> -	and	x2, x1, #_TIF_WORK_MASK
> -	cbnz	x2, work_pending
> +	bl	do_notify_resume
> +	ldr	x1, [tsk, #TI_FLAGS]		// re-check for single-step
>  	enable_step_tsk x1, x2
>  	kernel_exit 0
>  ENDPROC(ret_to_user)
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index e18c48c..3a6c60b 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -399,18 +399,34 @@ static void do_signal(struct pt_regs *regs)
>  	restore_saved_sigmask();
>  }
>  
> -asmlinkage void do_notify_resume(struct pt_regs *regs,
> -				 unsigned int thread_flags)
> +asmlinkage void do_notify_resume(void)
>  {
> -	if (thread_flags & _TIF_SIGPENDING)
> -		do_signal(regs);
> +	struct pt_regs *regs = task_pt_regs(current);
> +	unsigned long thread_flags;
>  
> -	if (thread_flags & _TIF_NOTIFY_RESUME) {
> -		clear_thread_flag(TIF_NOTIFY_RESUME);
> -		tracehook_notify_resume(regs);
> -	}
> +	for (;;) {
> +		local_irq_disable();

This should be raw_local_irq_disable()...

> +
> +		thread_flags = READ_ONCE(current_thread_info()->flags);
> +		if (!(thread_flags & _TIF_WORK_MASK))
> +			break;
> +
> +		if (thread_flags & _TIF_NEED_RESCHED) {
> +			schedule();
> +			continue;
> +		}
>  
> -	if (thread_flags & _TIF_FOREIGN_FPSTATE)
> -		fpsimd_restore_current_state();
> +		local_irq_enable();

... likewise, raw_local_irq_enable() here.

>  
> +		if (thread_flags & _TIF_SIGPENDING)
> +			do_signal(regs);
> +
> +		if (thread_flags & _TIF_NOTIFY_RESUME) {
> +			clear_thread_flag(TIF_NOTIFY_RESUME);
> +			tracehook_notify_resume(regs);
> +		}
> +
> +		if (thread_flags & _TIF_FOREIGN_FPSTATE)
> +			fpsimd_restore_current_state();
> +	}
>  }
> -- 
> 1.9.1
> 

[PATCH 2/2] arm64: factor work_pending state machine to C

[Catalin Marinas]

On Wed, Jan 06, 2016 at 01:43:14PM +0000, Mark Rutland wrote:
> On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
> > Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
> > state machine that can be difficult to reason about due to duplicated
> > code and a large number of branch targets.
> > 
> > This patch factors the common logic out into the existing
> > do_notify_resume function, converting the code to C in the process,
> > making the code more legible.
> > 
> > This patch tries to mirror the existing behaviour as closely as possible
> > while using the usual C control flow primitives. There should be no
> > functional change as a result of this patch.
> 
> I realised there is a problem with this for kernel built with
> TRACE_IRQFLAGS, as local_irq_{enable,disable}() will verify that the IRQ
> state is as expected.
> 
> In ret_fast_syscall we disable irqs behind the back of the tracer, so
> when we get into do_notify_resume we'll get a splat.
> 
> In the non-syscall cases we do not disable interrupts first, so we can't
> balance things in do_notify_resume.
> 
> We can either add a trace_hardirqs_off call to ret_fast_syscall, or we
> can use raw_local_irq_{disable,enable}. The latter would match the
> current behaviour (and is a nicer diff). Once the syscall path is moved
> to C it would be possible to use the non-raw variants all-over.
> 
> Catalin, are you happy with using the raw accessors in do_notify_resume,
> or would you prefer using trace_hardirqs_off?

I would prefer the explicit trace_hardirqs_off annotation, even though
it is a few more lines.

-- 
Catalin

[PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86

[Andy Lutomirski]

On Mon, Jan 4, 2016 at 12:33 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
>> This change is a prerequisite change for TASK_ISOLATION but also
>> stands on its own for readability and maintainability.
>
> I have also been looking into converting the userspace return path from
> assembly to C [1], for the latter two reasons. Based on that, I have a
> couple of comments.
>

>
> [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm

Neat!

In case you want to compare notes, I have a branch with the entire
syscall path on x86 in C except for cleanly separated asm fast path
optimizations:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=x86/entry_compat

Even in Linus' tree, the x86 32-bit syscalls are in C.

[PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86

[Mark Rutland]

On Mon, Jan 04, 2016 at 02:31:42PM -0800, Andy Lutomirski wrote:
> On Mon, Jan 4, 2016 at 12:33 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi,
> >
> > On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
> >> This change is a prerequisite change for TASK_ISOLATION but also
> >> stands on its own for readability and maintainability.
> >
> > I have also been looking into converting the userspace return path from
> > assembly to C [1], for the latter two reasons. Based on that, I have a
> > couple of comments.
> >
> 
> >
> > [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm
> 
> Neat!
> 
> In case you want to compare notes, I have a branch with the entire
> syscall path on x86 in C except for cleanly separated asm fast path
> optimizations:
> 
> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=x86/entry_compat

It was in fact your x86 effort that inspired me to look at this!

Thanks for the pointer, I'm almost certainly going to steal an idea or
two.

Currently it looks like arm64's conversion will be less painful than
that for x86 as the entry assembly is smaller and relatively uniform.
It looks like all but the register save/restore is possible in C.

That said, I have yet to stress/validate everything with tracing, irq
debugging, and so on, so my confidence may be misplaced.

Thanks,
Mark.

[PATCH v9 09/13] arch/arm64: enable task isolation functionality

[Chris Metcalf]

We need to call task_isolation_enter() from prepare_exit_to_usermode(),
so that we can both ensure we do it last before returning to
userspace, and we also are able to re-run signal handling, etc.,
if something occurs while task_isolation_enter() has interrupts
enabled.  To do this we add _TIF_NOHZ to the _TIF_WORK_MASK if
we have CONFIG_TASK_ISOLATION enabled, which brings us into
prepare_exit_to_usermode() on all return to userspace.  But we
don't put _TIF_NOHZ in the flags that we use to loop back and
recheck, since we don't need to loop back only because the flag
is set.  Instead we unconditionally call task_isolation_enter()
at the end of the loop if any other work is done.

To make the assembly code continue to be as optimized as before,
we renumber the _TIF flags so that both _TIF_WORK_MASK and
_TIF_SYSCALL_WORK still have contiguous runs of bits in the
immediate operand for the "and" instruction, as required by the
ARM64 ISA.  Since TIF_NOHZ is in both masks, it must be the
middle bit in the contiguous run that starts with the
_TIF_WORK_MASK bits and ends with the _TIF_SYSCALL_WORK bits.

We tweak syscall_trace_enter() slightly to carry the "flags"
value from current_thread_info()->flags for each of the tests,
rather than doing a volatile read from memory for each one.  This
avoids a small overhead for each test, and in particular avoids
that overhead for TIF_NOHZ when TASK_ISOLATION is not enabled.

We instrument the smp_cross_call() routine so that it checks for
isolated tasks and generates a suitable warning if we are about
to disturb one of them in strict or debug mode.

Finally, add an explicit check for STRICT mode in do_mem_abort()
to handle the case of page faults.

Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
 arch/arm64/include/asm/thread_info.h | 18 ++++++++++++------
 arch/arm64/kernel/ptrace.c           | 12 +++++++++---
 arch/arm64/kernel/signal.c           |  7 +++++--
 arch/arm64/kernel/smp.c              |  2 ++
 arch/arm64/mm/fault.c                |  4 ++++
 5 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 90c7ff233735..94a98e9e29ef 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -103,11 +103,11 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NEED_RESCHED	1
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
-#define TIF_NOHZ		7
-#define TIF_SYSCALL_TRACE	8
-#define TIF_SYSCALL_AUDIT	9
-#define TIF_SYSCALL_TRACEPOINT	10
-#define TIF_SECCOMP		11
+#define TIF_NOHZ		4
+#define TIF_SYSCALL_TRACE	5
+#define TIF_SYSCALL_AUDIT	6
+#define TIF_SYSCALL_TRACEPOINT	7
+#define TIF_SECCOMP		8
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_FREEZE		19
 #define TIF_RESTORE_SIGMASK	20
@@ -125,9 +125,15 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 #define _TIF_32BIT		(1 << TIF_32BIT)
 
-#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
+#define _TIF_WORK_LOOP_MASK	(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
 				 _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE)
 
+#ifdef CONFIG_TASK_ISOLATION
+# define _TIF_WORK_MASK		(_TIF_WORK_LOOP_MASK | _TIF_NOHZ)
+#else
+# define _TIF_WORK_MASK		_TIF_WORK_LOOP_MASK
+#endif
+
 #define _TIF_SYSCALL_WORK	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
 				 _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
 				 _TIF_NOHZ)
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 1971f491bb90..69ed3ba81650 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -37,6 +37,7 @@
 #include <linux/regset.h>
 #include <linux/tracehook.h>
 #include <linux/elf.h>
+#include <linux/isolation.h>
 
 #include <asm/compat.h>
 #include <asm/debug-monitors.h>
@@ -1240,14 +1241,19 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs)
 {
-	/* Do the secure computing check first; failures should be fast. */
+	unsigned long work = ACCESS_ONCE(current_thread_info()->flags);
+
+	if ((work & _TIF_NOHZ) && task_isolation_check_syscall(regs->syscallno))
+		return -1;
+
+	/* Do the secure computing check early; failures should be fast. */
 	if (secure_computing() == -1)
 		return -1;
 
-	if (test_thread_flag(TIF_SYSCALL_TRACE))
+	if (work & _TIF_SYSCALL_TRACE)
 		tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
 
-	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
+	if (work & _TIF_SYSCALL_TRACEPOINT)
 		trace_sys_enter(regs, regs->syscallno);
 
 	audit_syscall_entry(regs->syscallno, regs->orig_x0, regs->regs[1],
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index fde59c1139a9..641c828653c7 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -25,6 +25,7 @@
 #include <linux/uaccess.h>
 #include <linux/tracehook.h>
 #include <linux/ratelimit.h>
+#include <linux/isolation.h>
 
 #include <asm/debug-monitors.h>
 #include <asm/elf.h>
@@ -419,10 +420,12 @@ asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs,
 		if (thread_flags & _TIF_FOREIGN_FPSTATE)
 			fpsimd_restore_current_state();
 
+		task_isolation_enter();
+
 		local_irq_disable();
 
 		thread_flags = READ_ONCE(current_thread_info()->flags) &
-			_TIF_WORK_MASK;
+			_TIF_WORK_LOOP_MASK;
 
-	} while (thread_flags);
+	} while (thread_flags || !task_isolation_ready());
 }
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index b1adc51b2c2e..dcb3282d04a2 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -37,6 +37,7 @@
 #include <linux/completion.h>
 #include <linux/of.h>
 #include <linux/irq_work.h>
+#include <linux/isolation.h>
 
 #include <asm/alternative.h>
 #include <asm/atomic.h>
@@ -632,6 +633,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
 static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
 {
 	trace_ipi_raise(target, ipi_types[ipinr]);
+	task_isolation_debug_cpumask(target);
 	__smp_cross_call(target, ipinr);
 }
 
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 92ddac1e8ca2..fbc78035b2af 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -29,6 +29,7 @@
 #include <linux/sched.h>
 #include <linux/highmem.h>
 #include <linux/perf_event.h>
+#include <linux/isolation.h>
 
 #include <asm/cpufeature.h>
 #include <asm/exception.h>
@@ -466,6 +467,9 @@ asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
 	const struct fault_info *inf = fault_info + (esr & 63);
 	struct siginfo info;
 
+	if (user_mode(regs))
+		task_isolation_check_exception("%s at %#lx", inf->name, addr);
+
 	if (!inf->fn(addr, esr, regs))
 		return;
 
-- 
2.1.2