From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753434AbcAQJsF (ORCPT ); Sun, 17 Jan 2016 04:48:05 -0500 Received: from mx2.suse.de ([195.135.220.15]:53372 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753178AbcAQJro (ORCPT ); Sun, 17 Jan 2016 04:47:44 -0500 Date: Sun, 17 Jan 2016 10:47:23 +0100 From: Borislav Petkov To: Alexander Kuleshov Cc: Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , x86@kernel.org, Andy Lutomirski , Denys Vlasenko , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] x86/entry_64.S: introduce prepare_error_code macro Message-ID: <20160117094723.GB8549@pd.tnic> References: <1453016478-10760-1-git-send-email-kuleshovmail@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1453016478-10760-1-git-send-email-kuleshovmail@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jan 17, 2016 at 01:41:18PM +0600, Alexander Kuleshov wrote: > We need to put an error code to the %rsi if an exception provides > it, before the call of an exception handler. We do it in the idtentry > macro in two places. > > This patch introduces prepare_error_code macro which will check existence > of an error code and put it to %rsi from ORIG_RAX if it exists, or just > clears %esi if an error code does not exist to prevent code duplication > in the idtentry macro. > > Signed-off-by: Alexander Kuleshov > --- > > v2: indentation fixed > > arch/x86/entry/entry_64.S | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S > index 9d34d3c..e2b1e79 100644 > --- a/arch/x86/entry/entry_64.S > +++ b/arch/x86/entry/entry_64.S > @@ -722,6 +722,15 @@ apicinterrupt IRQ_WORK_VECTOR irq_work_interrupt smp_irq_work_interrupt > */ > #define CPU_TSS_IST(x) PER_CPU_VAR(cpu_tss) + (TSS_ist + ((x) - 1) * 8) > > + .macro prepare_error_code has_error_code:req > + .if \has_error_code > + movq ORIG_RAX(%rsp), %rsi /* get error code */ > + movq $-1, ORIG_RAX(%rsp) /* no syscall to restart */ > + .else > + xorl %esi, %esi /* no error code */ > + .endif > + .endm > + > .macro idtentry sym do_sym has_error_code:req paranoid=0 shift_ist=-1 > ENTRY(\sym) > /* Sanity check */ > @@ -759,12 +768,7 @@ ENTRY(\sym) > > movq %rsp, %rdi /* pt_regs pointer */ > > - .if \has_error_code > - movq ORIG_RAX(%rsp), %rsi /* get error code */ > - movq $-1, ORIG_RAX(%rsp) /* no syscall to restart */ > - .else > - xorl %esi, %esi /* no error code */ > - .endif > + prepare_error_code \has_error_code /* %rsi -> error code */ So this doesn't make it more readable to me, but less. And if it weren't for the comment to the right which says that error code goes into %rsi (yes, and not %rsi -> error code) I would have no idea what the code fragment ... movq %rsp, %rdi prepare_error_code \has_error_code ... does and I'd have to go look up the macro. Basically, this patch does the opposite of what the suggested traps.c change does. See what I mean? IOW, what the rule should be - and mind you, this is only my opinion - the code should be as readable as possible. One should be able to look at any function and say, aha, this is what it does there. Why? Well, the most frequent reason why anyone would need to look at code is when one is trying to figure out a bug and is following the code flow to refresh in one's head what's going on. Now, if the code is full if funky macros which one has to look up first to understand what they're doing and then continue on, then we clearly haven't done our job right. If there are no comments at all and the flow is not trivial, we too, haven't done everything right. So IMO, the most important strategy we should be pursuing is being able to read and understand the code as quickly as possible. And not save some lines with a funny macro name or silly traps.c helpers, or,... you name it. Ok? I hope it gets across exactly as I mean it. Thanks. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --