Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301055 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode()  model from x86 Mark Rutland <mark.rutland@arm.com> - 2016-01-04 21:40 +0100
    Re: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode()  model from x86 Chris Metcalf <cmetcalf@ezchip.com> - 2016-01-04 22:10 +0100
      Re: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode()  model from x86 Mark Rutland <mark.rutland@arm.com> - 2016-01-05 18:30 +0100
        [PATCH 2/2] arm64: factor work_pending state machine to C Mark Rutland <mark.rutland@arm.com> - 2016-01-05 18:40 +0100
          Re: [PATCH 2/2] arm64: factor work_pending state machine to C Chris Metcalf <cmetcalf@ezchip.com> - 2016-01-05 20:00 +0100
          Re: [PATCH 2/2] arm64: factor work_pending state machine to C Catalin Marinas <catalin.marinas@arm.com> - 2016-01-06 13:40 +0100
            Re: [PATCH 2/2] arm64: factor work_pending state machine to C Mark Rutland <mark.rutland@arm.com> - 2016-01-06 13:50 +0100
          Re: [PATCH 2/2] arm64: factor work_pending state machine to C Mark Rutland <mark.rutland@arm.com> - 2016-01-06 14:50 +0100
            Re: [PATCH 2/2] arm64: factor work_pending state machine to C Catalin Marinas <catalin.marinas@arm.com> - 2016-01-06 15:20 +0100
        [PATCH 1/2] arm64: entry: remove pointless SPSR mode check Mark Rutland <mark.rutland@arm.com> - 2016-01-05 18:40 +0100
          Re: [PATCH 1/2] arm64: entry: remove pointless SPSR mode check Catalin Marinas <catalin.marinas@arm.com> - 2016-01-06 13:20 +0100
    Re: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode()  model from x86 Andy Lutomirski <luto@amacapital.net> - 2016-01-04 23:40 +0100
      Re: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode()  model from x86 Mark Rutland <mark.rutland@arm.com> - 2016-01-05 19:10 +0100

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1301095

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301725

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301880 — Re: [PATCH 2/2] arm64: factor work_pending state machine to C

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302723 — Re: [PATCH 2/2] arm64: factor work_pending state machine to C

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>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302731 — Re: [PATCH 2/2] arm64: factor work_pending state machine to C

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302771 — Re: [PATCH 2/2] arm64: factor work_pending state machine to C

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302800 — Re: [PATCH 2/2] arm64: factor work_pending state machine to C

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302713 — Re: [PATCH 1/2] arm64: entry: remove pointless SPSR mode check

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>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301165

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301798

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web