[PATCH] powerpc/64: system call implement the bulk of the logic in C fix

[PATCH] powerpc/64: system call implement the bulk of the logic in C fix

[Nicholas Piggin]

This incremental patch fixes several soft-mask debug and unsafe
smp_processor_id messages due to tracing and false positives in
"unreconciled" code.

It also fixes a bug with syscall tracing functions that set registers
(e.g., PTRACE_SETREG) not setting GPRs properly.

There was a bug reported with the TM selftests, I haven't been able
to reproduce that one.

I can squash this into the main patch and resend the series if it
helps but the incremental helps to see the bug fixes.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/cputime.h | 39 +++++++++++++++++-------------
 arch/powerpc/kernel/syscall_64.c   | 26 ++++++++++++++------
 2 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h
index c43614cffaac..6639a6847cc0 100644
--- a/arch/powerpc/include/asm/cputime.h
+++ b/arch/powerpc/include/asm/cputime.h
@@ -44,6 +44,28 @@ static inline unsigned long cputime_to_usecs(const cputime_t ct)
 #ifdef CONFIG_PPC64
 #define get_accounting(tsk)	(&get_paca()->accounting)
 static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
+
+/*
+ * account_cpu_user_entry/exit runs "unreconciled", so can't trace,
+ * can't use use get_paca()
+ */
+static notrace inline void account_cpu_user_entry(void)
+{
+	unsigned long tb = mftb();
+	struct cpu_accounting_data *acct = &local_paca->accounting;
+
+	acct->utime += (tb - acct->starttime_user);
+	acct->starttime = tb;
+}
+static notrace inline void account_cpu_user_exit(void)
+{
+	unsigned long tb = mftb();
+	struct cpu_accounting_data *acct = &local_paca->accounting;
+
+	acct->stime += (tb - acct->starttime);
+	acct->starttime_user = tb;
+}
+
 #else
 #define get_accounting(tsk)	(&task_thread_info(tsk)->accounting)
 /*
@@ -60,23 +82,6 @@ static inline void arch_vtime_task_switch(struct task_struct *prev)
 }
 #endif
 
-static inline void account_cpu_user_entry(void)
-{
-	unsigned long tb = mftb();
-	struct cpu_accounting_data *acct = get_accounting(current);
-
-	acct->utime += (tb - acct->starttime_user);
-	acct->starttime = tb;
-}
-static inline void account_cpu_user_exit(void)
-{
-	unsigned long tb = mftb();
-	struct cpu_accounting_data *acct = get_accounting(current);
-
-	acct->stime += (tb - acct->starttime);
-	acct->starttime_user = tb;
-}
-
 #endif /* __KERNEL__ */
 #else /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
 static inline void account_cpu_user_entry(void)
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 529393a1ff1e..cfe458adde07 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -19,7 +19,8 @@ extern void __noreturn tabort_syscall(void);
 
 typedef long (*syscall_fn)(long, long, long, long, long, long);
 
-long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
+/* Has to run notrace because it is entered "unreconciled" */
+notrace long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 			   unsigned long r0, struct pt_regs *regs)
 {
 	unsigned long ti_flags;
@@ -36,7 +37,7 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 #ifdef CONFIG_PPC_SPLPAR
 	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) &&
 	    firmware_has_feature(FW_FEATURE_SPLPAR)) {
-		struct lppaca *lp = get_lppaca();
+		struct lppaca *lp = local_paca->lppaca_ptr;
 
 		if (unlikely(local_paca->dtl_ridx != be64_to_cpu(lp->dtl_idx)))
 			accumulate_stolen_time();
@@ -71,13 +72,22 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
 		 * We use the return value of do_syscall_trace_enter() as the
 		 * syscall number. If the syscall was rejected for any reason
 		 * do_syscall_trace_enter() returns an invalid syscall number
-		 * and the test below against NR_syscalls will fail.
+		 * and the test against NR_syscalls will fail and the return
+		 * value to be used is in regs->gpr[3].
 		 */
 		r0 = do_syscall_trace_enter(regs);
-	}
-
-	if (unlikely(r0 >= NR_syscalls))
+		if (unlikely(r0 >= NR_syscalls))
+			return regs->gpr[3];
+		r3 = regs->gpr[3];
+		r4 = regs->gpr[4];
+		r5 = regs->gpr[5];
+		r6 = regs->gpr[6];
+		r7 = regs->gpr[7];
+		r8 = regs->gpr[8];
+
+	} else if (unlikely(r0 >= NR_syscalls)) {
 		return -ENOSYS;
+	}
 
 	/* May be faster to do array_index_nospec? */
 	barrier_nospec();
@@ -139,8 +149,10 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 		regs->gpr[3] = r3;
 	}
 
-	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE))
+	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
 		do_syscall_trace_leave(regs);
+		ret |= _TIF_RESTOREALL;
+	}
 
 again:
 	local_irq_disable();
-- 
2.23.0

Re: [PATCH] powerpc/64: system call implement the bulk of the logic in C fix

[Michal Suchánek]

On Tue, Jan 28, 2020 at 12:17:12AM +1000, Nicholas Piggin wrote:
> This incremental patch fixes several soft-mask debug and unsafe
> smp_processor_id messages due to tracing and false positives in
> "unreconciled" code.
> 
> It also fixes a bug with syscall tracing functions that set registers
> (e.g., PTRACE_SETREG) not setting GPRs properly.
> 
> There was a bug reported with the TM selftests, I haven't been able
> to reproduce that one.
> 
> I can squash this into the main patch and resend the series if it
> helps but the incremental helps to see the bug fixes.

There are some whitespace differences between this and the series I have
applied locally. What does it apply to?

Is there some revision of the patchset I missed?

Thanks

Michal
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/cputime.h | 39 +++++++++++++++++-------------
>  arch/powerpc/kernel/syscall_64.c   | 26 ++++++++++++++------
>  2 files changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h
> index c43614cffaac..6639a6847cc0 100644
> --- a/arch/powerpc/include/asm/cputime.h
> +++ b/arch/powerpc/include/asm/cputime.h
> @@ -44,6 +44,28 @@ static inline unsigned long cputime_to_usecs(const cputime_t ct)
>  #ifdef CONFIG_PPC64
>  #define get_accounting(tsk)	(&get_paca()->accounting)
>  static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
> +
> +/*
> + * account_cpu_user_entry/exit runs "unreconciled", so can't trace,
> + * can't use use get_paca()
> + */
> +static notrace inline void account_cpu_user_entry(void)
> +{
> +	unsigned long tb = mftb();
> +	struct cpu_accounting_data *acct = &local_paca->accounting;
> +
> +	acct->utime += (tb - acct->starttime_user);
> +	acct->starttime = tb;
> +}
> +static notrace inline void account_cpu_user_exit(void)
> +{
> +	unsigned long tb = mftb();
> +	struct cpu_accounting_data *acct = &local_paca->accounting;
> +
> +	acct->stime += (tb - acct->starttime);
> +	acct->starttime_user = tb;
> +}
> +
>  #else
>  #define get_accounting(tsk)	(&task_thread_info(tsk)->accounting)
>  /*
> @@ -60,23 +82,6 @@ static inline void arch_vtime_task_switch(struct task_struct *prev)
>  }
>  #endif
>  
> -static inline void account_cpu_user_entry(void)
> -{
> -	unsigned long tb = mftb();
> -	struct cpu_accounting_data *acct = get_accounting(current);
> -
> -	acct->utime += (tb - acct->starttime_user);
> -	acct->starttime = tb;
> -}
> -static inline void account_cpu_user_exit(void)
> -{
> -	unsigned long tb = mftb();
> -	struct cpu_accounting_data *acct = get_accounting(current);
> -
> -	acct->stime += (tb - acct->starttime);
> -	acct->starttime_user = tb;
> -}
> -
>  #endif /* __KERNEL__ */
>  #else /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
>  static inline void account_cpu_user_entry(void)
> diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
> index 529393a1ff1e..cfe458adde07 100644
> --- a/arch/powerpc/kernel/syscall_64.c
> +++ b/arch/powerpc/kernel/syscall_64.c
> @@ -19,7 +19,8 @@ extern void __noreturn tabort_syscall(void);
>  
>  typedef long (*syscall_fn)(long, long, long, long, long, long);
>  
> -long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
> +/* Has to run notrace because it is entered "unreconciled" */
> +notrace long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
>  			   unsigned long r0, struct pt_regs *regs)
>  {
>  	unsigned long ti_flags;
> @@ -36,7 +37,7 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
>  #ifdef CONFIG_PPC_SPLPAR
>  	if (IS_ENABLED(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) &&
>  	    firmware_has_feature(FW_FEATURE_SPLPAR)) {
> -		struct lppaca *lp = get_lppaca();
> +		struct lppaca *lp = local_paca->lppaca_ptr;
>  
>  		if (unlikely(local_paca->dtl_ridx != be64_to_cpu(lp->dtl_idx)))
>  			accumulate_stolen_time();
> @@ -71,13 +72,22 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
>  		 * We use the return value of do_syscall_trace_enter() as the
>  		 * syscall number. If the syscall was rejected for any reason
>  		 * do_syscall_trace_enter() returns an invalid syscall number
> -		 * and the test below against NR_syscalls will fail.
> +		 * and the test against NR_syscalls will fail and the return
> +		 * value to be used is in regs->gpr[3].
>  		 */
>  		r0 = do_syscall_trace_enter(regs);
> -	}
> -
> -	if (unlikely(r0 >= NR_syscalls))
> +		if (unlikely(r0 >= NR_syscalls))
> +			return regs->gpr[3];
> +		r3 = regs->gpr[3];
> +		r4 = regs->gpr[4];
> +		r5 = regs->gpr[5];
> +		r6 = regs->gpr[6];
> +		r7 = regs->gpr[7];
> +		r8 = regs->gpr[8];
> +
> +	} else if (unlikely(r0 >= NR_syscalls)) {
>  		return -ENOSYS;
> +	}
>  
>  	/* May be faster to do array_index_nospec? */
>  	barrier_nospec();
> @@ -139,8 +149,10 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>  		regs->gpr[3] = r3;
>  	}
>  
> -	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE))
> +	if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
>  		do_syscall_trace_leave(regs);
> +		ret |= _TIF_RESTOREALL;
> +	}
>  
>  again:
>  	local_irq_disable();
> -- 
> 2.23.0
> 

Re: [PATCH] powerpc/64: system call implement the bulk of the logic in C fix

[Nicholas Piggin]

Michal Suchánek's on January 28, 2020 4:08 am:
> On Tue, Jan 28, 2020 at 12:17:12AM +1000, Nicholas Piggin wrote:
>> This incremental patch fixes several soft-mask debug and unsafe
>> smp_processor_id messages due to tracing and false positives in
>> "unreconciled" code.
>> 
>> It also fixes a bug with syscall tracing functions that set registers
>> (e.g., PTRACE_SETREG) not setting GPRs properly.
>> 
>> There was a bug reported with the TM selftests, I haven't been able
>> to reproduce that one.
>> 
>> I can squash this into the main patch and resend the series if it
>> helps but the incremental helps to see the bug fixes.
> 
> There are some whitespace differences between this and the series I have
> applied locally. What does it apply to?
> 
> Is there some revision of the patchset I missed?

No I may have just missed some of your whitespace cleanups, or maybe I got
some that Michael made which you don't have in his next-test branch.

Thanks,
Nick

Re: [PATCH] powerpc/64: system call implement the bulk of the logic in C fix

[Michal Suchánek]

On Tue, Jan 28, 2020 at 10:41:02AM +1000, Nicholas Piggin wrote:
> Michal Suchánek's on January 28, 2020 4:08 am:
> > On Tue, Jan 28, 2020 at 12:17:12AM +1000, Nicholas Piggin wrote:
> >> This incremental patch fixes several soft-mask debug and unsafe
> >> smp_processor_id messages due to tracing and false positives in
> >> "unreconciled" code.
> >> 
> >> It also fixes a bug with syscall tracing functions that set registers
> >> (e.g., PTRACE_SETREG) not setting GPRs properly.
> >> 
> >> There was a bug reported with the TM selftests, I haven't been able
> >> to reproduce that one.
> >> 
> >> I can squash this into the main patch and resend the series if it
> >> helps but the incremental helps to see the bug fixes.
> > 
> > There are some whitespace differences between this and the series I have
> > applied locally. What does it apply to?
> > 
> > Is there some revision of the patchset I missed?
> 
> No I may have just missed some of your whitespace cleanups, or maybe I got
> some that Michael made which you don't have in his next-test branch.

Looks like the latter. I will pick patches from next-test.

Thanks

Michal