Re: [PATCH v4 2/4] x86: simplify _TIF_SYSCALL_EMU handling

[Thomas Gleixner]

On Thu, 23 May 2019, Sudeep Holla wrote:

$Subject: Please use the proper prefix and start the sentence with an upper
case letter.

  x86/entry: Simplify _TIF_SYSCALL_EMU handling

> The usage of emulated/_TIF_SYSCALL_EMU flags in syscall_trace_enter
> seems to be bit overcomplicated than required. Let's simplify it.

s/seems to be bit overcomplicated/is more complicated/

 Either you are sure that it is overengineered, then say so. If not, then
 you should not touch the code at all.

s/Let's simplify it.//

 'Let's do X.' is a popular, but technically useless phrase.

> Cc: Andy Lutomirski 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: Borislav Petkov 
> Acked-by: Oleg Nesterov 
> Signed-off-by: Sudeep Holla 

This is a nice simplification indeed! With the changelog fixed:

 Reviewed-by: Thomas Gleixner 

Re: [PATCH v4 2/4] x86: simplify _TIF_SYSCALL_EMU handling

[Catalin Marinas]

On Thu, May 23, 2019 at 10:06:16AM +0100, Sudeep Holla wrote:
> The usage of emulated/_TIF_SYSCALL_EMU flags in syscall_trace_enter
> seems to be bit overcomplicated than required. Let's simplify it.
> 
> Cc: Andy Lutomirski 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: Borislav Petkov 
> Acked-by: Oleg Nesterov 
> Signed-off-by: Sudeep Holla 
> ---
>  arch/x86/entry/common.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index a986b3c8294c..0a61705d62ec 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -72,23 +72,18 @@ static long syscall_trace_enter(struct pt_regs *regs)
>  
>   struct thread_info *ti = current_thread_info();
>   unsigned long ret = 0;
> - bool emulated = false;
>   u32 work;
>  
>   if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
>   BUG_ON(regs != task_pt_regs(current));
>  
> - work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
> + work = READ_ONCE(ti->flags);
>  
> - if (unlikely(work & _TIF_SYSCALL_EMU))
> - emulated = true;
> -
> - if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
> - tracehook_report_syscall_entry(regs))
> - return -1L;
> -
> - if (emulated)
> - return -1L;
> + if (work & (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_EMU)) {
> + ret = tracehook_report_syscall_entry(regs);
> + if (ret || (work & _TIF_SYSCALL_EMU))
> + return -1L;
> + }

Andy (or the other x86 folk), could I please get an ack on this patch? I
plan to queue this series through the arm64 tree (though if you want to
merge it separately, it looks like an independent clean-up with no
dependencies on the other patches).

Thanks.

-- 
Catalin

[PATCH v4 2/4] x86: simplify _TIF_SYSCALL_EMU handling

[Sudeep Holla]

The usage of emulated/_TIF_SYSCALL_EMU flags in syscall_trace_enter
seems to be bit overcomplicated than required. Let's simplify it.

Cc: Andy Lutomirski 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: Borislav Petkov 
Acked-by: Oleg Nesterov 
Signed-off-by: Sudeep Holla 
---
 arch/x86/entry/common.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index a986b3c8294c..0a61705d62ec 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -72,23 +72,18 @@ static long syscall_trace_enter(struct pt_regs *regs)
 
struct thread_info *ti = current_thread_info();
unsigned long ret = 0;
-   bool emulated = false;
u32 work;
 
if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
BUG_ON(regs != task_pt_regs(current));
 
-   work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
+   work = READ_ONCE(ti->flags);
 
-   if (unlikely(work & _TIF_SYSCALL_EMU))
-   emulated = true;
-
-   if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
-   tracehook_report_syscall_entry(regs))
-   return -1L;
-
-   if (emulated)
-   return -1L;
+   if (work & (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_EMU)) {
+   ret = tracehook_report_syscall_entry(regs);
+   if (ret || (work & _TIF_SYSCALL_EMU))
+   return -1L;
+   }
 
 #ifdef CONFIG_SECCOMP
/*
-- 
2.17.1