Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751215AbbFMGav (ORCPT ); Sat, 13 Jun 2015 02:30:51 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:34661 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750765AbbFMGam (ORCPT ); Sat, 13 Jun 2015 02:30:42 -0400 Date: Sat, 13 Jun 2015 08:30:36 +0200 From: Ingo Molnar To: "H. Peter Anvin" Cc: Denys Vlasenko , Linus Torvalds , Steven Rostedt , Borislav Petkov , Andy Lutomirski , Oleg Nesterov , Frederic Weisbecker , Alexei Starovoitov , Will Drewry , Kees Cook , "x86@kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] x86/asm/entry/32: Slightly better handling of syscall errors in auditing Message-ID: <20150613063036.GA12612@gmail.com> References: <1434023278-21808-1-git-send-email-dvlasenk@redhat.com> <82D8676C-3E8C-4603-BAE3-5E48EBC09233@zytor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <82D8676C-3E8C-4603-BAE3-5E48EBC09233@zytor.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2763 Lines: 66 * H. Peter Anvin wrote: > I think you misunderstand partial register stalls. They happen (on some > microarchitectures) when you write part of a register and then use the whole > register. Yes, there's no partial register stall in this or later code handling these values. > > "setbe %al" insn has a register merge stall: it needs to combine previous %eax > > value with new value for the lowest byte. Subsequent "movzbl %al,%edi" in turn > > depends on its completion. > > > > This patch replaces "setbe %al + movzbl %al,%edi" pair of insns with "xor > > %edi,%edi" before the comparison, and conditional "inc %edi". So here's the code in wider context: > cmpl $-MAX_ERRNO, %eax /* is it an error ? */ > jbe 1f > movslq %eax, %rsi /* if error sign extend to 64 bits */ > 1: setbe %al /* 1 if error, 0 if not */ > movzbl %al, %edi /* zero-extend that into %edi */ What happens here is that at the point the SETBE executes it needs to know the previous 32-bit value of EAX. But the previous JBE needs to know it already (it needs the CF and ZF result of the CMPL comparison), so there's no real additional dependency. (The MOVSLQ of EAX will likewise already have the full value of EAX, because the already JBE needs it.) Furthermore, the following SETBE sets an entirely new value for the 8-bit AL. The 'entirely new value' will be handled by modern uarchs with register renaming (and marking that it's a rename for the low byte of EAX), giving the new value a separate, independent path to compute and use - and that renamed register value will be moved into EDI (zero-extended). The CPU might eventually have to merge the previous value of EAX with the new value for AL, but there's no dependency on it in this piece of code. If there was a dependency on the full value then _that_ would create a partial register stall. And as it happens, there's no such subsequent dependency, because we call a C function right away: call __audit_syscall_exit and RAX is a freely available register used as the return code. It's being overwritten early in the __audit_syscall_exit() function's execution by zeroing: 28d4: 19 c0 sbb %eax,%eax which will fully overwrite the previous partial value without extra dependencies. So the real motivation of the patch is to simplify the setting of EDI to 0 or 1 by using a branch we already execute. Thanks, Ingo -- 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/