From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2B299CA9EB9 for ; Wed, 23 Oct 2019 23:16:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 03ED6205F4 for ; Wed, 23 Oct 2019 23:16:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2436900AbfJWXQQ (ORCPT ); Wed, 23 Oct 2019 19:16:16 -0400 Received: from Galois.linutronix.de ([193.142.43.55]:51296 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728773AbfJWXQP (ORCPT ); Wed, 23 Oct 2019 19:16:15 -0400 Received: from p5b06da22.dip0.t-ipconnect.de ([91.6.218.34] helo=nanos) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iNPrM-0002cM-M8; Thu, 24 Oct 2019 01:16:08 +0200 Date: Thu, 24 Oct 2019 01:16:07 +0200 (CEST) From: Thomas Gleixner To: Andy Lutomirski cc: LKML , X86 ML , Peter Zijlstra , Will Deacon , Paolo Bonzini , kvm list , linux-arch , Mike Rapoport , Josh Poimboeuf , Miroslav Benes Subject: Re: [patch V2 08/17] x86/entry: Move syscall irq tracing to C code In-Reply-To: Message-ID: References: <20191023122705.198339581@linutronix.de> <20191023123118.386844979@linutronix.de> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 23 Oct 2019, Andy Lutomirski wrote: > On Wed, Oct 23, 2019 at 5:31 AM Thomas Gleixner wrote: > > > > Interrupt state tracing can be safely done in C code. The few stack > > operations in assembly do not need to be covered. > > > > Remove the now pointless indirection via .Lsyscall_32_done and jump to > > swapgs_restore_regs_and_return_to_usermode directly. > > This doesn't look right. > > > #define SYSCALL_EXIT_WORK_FLAGS \ > > @@ -279,6 +282,9 @@ static void syscall_slow_exit_work(struc > > { > > struct thread_info *ti; > > > > + /* User to kernel transition disabled interrupts. */ > > + trace_hardirqs_off(); > > + > > So you just traced IRQs off, but... > > > enter_from_user_mode(); > > local_irq_enable(); > > Now they're on and traced on again? Yes, because that's what actually happens. usermode syscall <- Disables interrupts, but tracing thinks they are on entry_SYSCALL_64 .... call do_syscall_64 trace_hardirqs_off() <- So before calling anything else, we have to tell the tracer that interrupts are on, which we did so far in the ASM code between entry_SYSCALL_64 and 'call do_syscall_64'. I'm merily lifting this to C-code. enter_from_user_mode() local_irq_enable() > I also don't see how your patch handles the fastpath case. Hmm? All syscalls return through: syscall_return_slowpath() local_irq_disable() prepare_exit_to_usermode() user_enter_irqoff() mds_user_clear_cpu_buffers() trace_hardirqs_on() What am I missing? > How about the attached patch instead? ^^^^^^ Groan. > > user_enter_irqoff(); > > + /* > + * The actual return to usermode will almost certainly turn IRQs on. > + * Trace it here to simplify the asm code. Why would we return to user from a syscall or interrupt with interrupts traced as disabled? Also the existing ASM is inconsistent vs. that: ENTRY(entry_SYSENTER_32) TRACE_IRQS_ON ENTRY(entry_INT80_32) TRACE_IRQS_IRET ENTRY(entry_SYSCALL_64) TRACE_IRQS_IRET ENTRY(ret_from_fork) TRACE_IRQS_ON GLOBAL(retint_user) TRACE_IRQS_IRETQ ENTRY(entry_SYSCALL_compat) TRACE_IRQS_ON ENTRY(entry_INT80_compat) TRACE_IRQS_ON > + */ > + if (likely(regs->flags & X86_EFLAGS_IF)) > + trace_hardirqs_on(); My variant does this unconditionally and after mds_user_clear_cpu_buffers(). > mds_user_clear_cpu_buffers(); > } And your ASM changes keep still all the TRACE_IRQS_OFF invocations in the various syscall entry pathes, which is what I removed and put as the first thing into the C functions. Confused. Thanks, tglx