On Thu, Jul 4, 2019 at 1:03 PM Peter Zijlstra <pet...@infradead.org> wrote: > > There's a bunch of duplication in idtentry, namely the > .Lfrom_usermode_switch_stack is a paranoid=0 copy of the normal flow. > > Make this explicit by creating a idtentry_part helper macro. > > Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org> > --- > arch/x86/entry/entry_64.S | 100 > +++++++++++++++++++++------------------------- > 1 file changed, 47 insertions(+), 53 deletions(-) > > --- a/arch/x86/entry/entry_64.S > +++ b/arch/x86/entry/entry_64.S > @@ -865,6 +865,51 @@ apicinterrupt IRQ_WORK_VECTOR > irq_work > */ > #define CPU_TSS_IST(x) PER_CPU_VAR(cpu_tss_rw) + (TSS_ist + (x) * 8) > > +.macro idtentry_part do_sym, has_error_code:req, paranoid:req, shift_ist=-1, > ist_offset=0 > + > + .if \paranoid > + call paranoid_entry > + /* returned flag: ebx=0: need swapgs on exit, ebx=1: don't need it */ > + .else > + call error_entry > + .endif > + UNWIND_HINT_REGS > + > + .if \paranoid > + .if \shift_ist != -1 > + TRACE_IRQS_OFF_DEBUG /* reload IDT in case of > recursion */ > + .else > + TRACE_IRQS_OFF > + .endif > + .endif > + > + 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 > + > + .if \shift_ist != -1 > + subq $\ist_offset, CPU_TSS_IST(\shift_ist) > + .endif > + > + call \do_sym > + > + .if \shift_ist != -1 > + addq $\ist_offset, CPU_TSS_IST(\shift_ist) > + .endif > + > + .if \paranoid > + jmp paranoid_exit > + .else > + jmp error_exit > + .endif > + > +.endm > + > /** > * idtentry - Generate an IDT entry stub > * @sym: Name of the generated entry point > @@ -935,46 +980,7 @@ ENTRY(\sym) > .Lfrom_usermode_no_gap_\@: > .endif > > - .if \paranoid > - call paranoid_entry > - .else > - call error_entry > - .endif > - UNWIND_HINT_REGS > - /* returned flag: ebx=0: need swapgs on exit, ebx=1: don't need it */ > - > - .if \paranoid > - .if \shift_ist != -1 > - TRACE_IRQS_OFF_DEBUG /* reload IDT in case of > recursion */ > - .else > - TRACE_IRQS_OFF > - .endif > - .endif > - > - 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 > - > - .if \shift_ist != -1 > - subq $\ist_offset, CPU_TSS_IST(\shift_ist) > - .endif > - > - call \do_sym > - > - .if \shift_ist != -1 > - addq $\ist_offset, CPU_TSS_IST(\shift_ist) > - .endif > - > - .if \paranoid > - jmp paranoid_exit > - .else > - jmp error_exit > - .endif > + idtentry_part \do_sym, \has_error_code, \paranoid, \shift_ist, > \ist_offset > > .if \paranoid == 1 > /* > @@ -983,21 +989,9 @@ ENTRY(\sym) > * run in real process context if user_mode(regs). > */ > .Lfrom_usermode_switch_stack_\@: > - call error_entry > - > - 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 */ > + idtentry_part \do_sym, \has_error_code, 0
Nice! You are adding an extra UNWIND_HINT_REGS that wasn't here before, but I think that's fine. However, can you pleace make it paranoid=0 instead of just 0? You could go all the way verbose and say do_sym=\do_sym, etc, but that seems like overkill. Other than that nitpick, Acked-by: Andy Lutomirski <l...@kernel.org> --Andy
