* [PATCH v2] x86/entry_64.S: introduce prepare_error_code macro
@ 2016-01-17 7:41 Alexander Kuleshov
2016-01-17 9:47 ` Borislav Petkov
0 siblings, 1 reply; 2+ messages in thread
From: Alexander Kuleshov @ 2016-01-17 7:41 UTC (permalink / raw
To: Thomas Gleixner
Cc: Ingo Molnar, H . Peter Anvin, x86, Andy Lutomirski,
Denys Vlasenko, Borislav Petkov, linux-kernel, Alexander Kuleshov
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 <kuleshovmail@gmail.com>
---
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 */
.if \shift_ist != -1
subq $EXCEPTION_STKSZ, CPU_TSS_IST(\shift_ist)
@@ -799,13 +803,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 */
call \do_sym
jmp error_exit /* %ebx: no swapgs flag */
--
2.7.0.25.gfc10eb5
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] x86/entry_64.S: introduce prepare_error_code macro
2016-01-17 7:41 [PATCH v2] x86/entry_64.S: introduce prepare_error_code macro Alexander Kuleshov
@ 2016-01-17 9:47 ` Borislav Petkov
0 siblings, 0 replies; 2+ messages in thread
From: Borislav Petkov @ 2016-01-17 9:47 UTC (permalink / raw
To: Alexander Kuleshov
Cc: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, x86,
Andy Lutomirski, Denys Vlasenko, linux-kernel
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 <kuleshovmail@gmail.com>
> ---
>
> 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)
--
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-01-17 9:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-17 7:41 [PATCH v2] x86/entry_64.S: introduce prepare_error_code macro Alexander Kuleshov
2016-01-17 9:47 ` Borislav Petkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).