Re: [patch V4 part 5 13/31] x86/irq: Convey vector as argument and not in ptregs

2020-05-11 Thread Lai Jiangshan
Hello

On Mon, May 11, 2020 at 10:35 PM Thomas Gleixner  wrote:
>
> Lai,
>
> Lai Jiangshan  writes:
> > On Tue, May 5, 2020 at 10:23 PM Thomas Gleixner  wrote:
> >> +SYM_CODE_START(irq_entries_start)
> >> +vector=FIRST_EXTERNAL_VECTOR
> >> +.rept (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
> >> +   UNWIND_HINT_IRET_REGS
> >> +   .byte   0x6a, vector
> >> +   jmp common_interrupt
> >> +   .align  8
> >> +vector=vector+1
> >> +.endr
> >> +SYM_CODE_END(irq_entries_start)
> >
> > Using ".byte   0x6a, vector" is somewhat ugly.
> >
> > I hope it should be " pushq $(s8_to_s64(vector))", which can also
> > help to reduce bunches of comments about ".byte   0x6a, vector".
> >
> > However, I don't know how to implement s8_to_s64() here.
>
> Neither do I.
>
> > But at least the following code works (generates the same two-byte
> > machine code as ".byte 0x6a, vector" does):
> >
> > .if vector < 128
> > pushq $(vector)
> > .else
> > pushq $(0xff00+vector)
> > .endif
>
> Only slightly less ugly and needs as much commentry as the above.

Agree.

Just FYI, I tried this later, it can work.

#define S8_TO_S64(vector) ((vector>>7)*0xff00+vector)

Thanks
Lai


Re: [patch V4 part 5 13/31] x86/irq: Convey vector as argument and not in ptregs

2020-05-11 Thread Thomas Gleixner
Lai,

Lai Jiangshan  writes:
> On Tue, May 5, 2020 at 10:23 PM Thomas Gleixner  wrote:
>> +SYM_CODE_START(irq_entries_start)
>> +vector=FIRST_EXTERNAL_VECTOR
>> +.rept (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
>> +   UNWIND_HINT_IRET_REGS
>> +   .byte   0x6a, vector
>> +   jmp common_interrupt
>> +   .align  8
>> +vector=vector+1
>> +.endr
>> +SYM_CODE_END(irq_entries_start)
>
> Using ".byte   0x6a, vector" is somewhat ugly.
>
> I hope it should be " pushq $(s8_to_s64(vector))", which can also
> help to reduce bunches of comments about ".byte   0x6a, vector".
>
> However, I don't know how to implement s8_to_s64() here.

Neither do I.

> But at least the following code works (generates the same two-byte
> machine code as ".byte 0x6a, vector" does):
>
> .if vector < 128
> pushq $(vector)
> .else
> pushq $(0xff00+vector)
> .endif

Only slightly less ugly and needs as much commentry as the above.

Thanks,

tglx


Re: [patch V4 part 5 13/31] x86/irq: Convey vector as argument and not in ptregs

2020-05-09 Thread Lai Jiangshan
On Tue, May 5, 2020 at 10:23 PM Thomas Gleixner  wrote:

> +/*
> + * ASM code to emit the common vector entry stubs where each stub is
> + * packed into 8 bytes.
> + *
> + * Note, that the 'pushq imm8' is emitted via '.byte 0x6a, vector' because
> + * GCC treats the local vector variable as unsigned int and would expand
> + * all vectors above 0x7F to a 5 byte push. The original code did an
> + * adjustment of the vector number to be in the signed byte range to avoid
> + * this. While clever it's mindboggling counterintuitive and requires the
> + * odd conversion back to a real vector number in the C entry points. Using
> + * .byte achieves the same thing and the only fixup needed in the C entry
> + * point is to mask off the bits above bit 7 because the push is sign
> + * extending.
> + */
> +   .align 8
> +SYM_CODE_START(irq_entries_start)
> +vector=FIRST_EXTERNAL_VECTOR
> +.rept (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
> +   UNWIND_HINT_IRET_REGS
> +   .byte   0x6a, vector
> +   jmp common_interrupt
> +   .align  8
> +vector=vector+1
> +.endr
> +SYM_CODE_END(irq_entries_start)

Hello, tglx

Using ".byte   0x6a, vector" is somewhat ugly.

I hope it should be " pushq $(s8_to_s64(vector))", which can also
help to reduce bunches of comments about ".byte   0x6a, vector".

However, I don't know how to implement s8_to_s64() here. But at
least the following code works (generates the same two-byte machine
code as ".byte   0x6a, vector" does):

.if vector < 128
pushq $(vector)
.else
pushq $(0xff00+vector)
.endif

Thanks,
Lai


[patch V4 part 5 13/31] x86/irq: Convey vector as argument and not in ptregs

2020-05-05 Thread Thomas Gleixner
Device interrupts which go through do_IRQ() or the spurious interrupt
handler have their separate entry code on 64 bit for no good reason.

Both 32 and 64 bit transport the vector number through ORIG_[RE]AX in
pt_regs. Further the vector number is forced to fit into an u8 and is
complemented and offset by 0x80 so it's in the signed character
range. Otherwise GAS would expand the pushq to a 5 byte instruction for any
vector > 0x7F.

Treat the vector number like an error code and hand it to the C function as
argument. This allows to get rid of the extra entry code in a later step.

Simplify the error code push magic by implementing the pushq imm8 via a
'.byte 0x6a, vector' sequence so GAS is not able to screw it up. As the
pushq imm8 is sign extending the resulting error code needs to be truncated
to 8 bits in C code.

Originally-by: Andy Lutomirski 
Signed-off-by: Thomas Gleixner 
Cc: Brian Gerst 
---
V2: Fix the pushq thinko (Brian)
---
 arch/x86/entry/calling.h  |5 +++-
 arch/x86/entry/entry_32.S |   33 +++
 arch/x86/entry/entry_64.S |   40 ++
 arch/x86/include/asm/entry_arch.h |2 -
 arch/x86/include/asm/hw_irq.h |1 
 arch/x86/include/asm/idtentry.h   |   40 ++
 arch/x86/include/asm/irq.h|2 -
 arch/x86/include/asm/traps.h  |3 +-
 arch/x86/kernel/apic/apic.c   |   31 +++--
 arch/x86/kernel/idt.c |2 -
 arch/x86/kernel/irq.c |   14 -
 11 files changed, 95 insertions(+), 78 deletions(-)

--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -339,7 +339,10 @@ For 32-bit we have the following convent
 #endif
 .endm
 
-#endif /* CONFIG_X86_64 */
+#else /* CONFIG_X86_64 */
+# undefUNWIND_HINT_IRET_REGS
+# define   UNWIND_HINT_IRET_REGS
+#endif /* !CONFIG_X86_64 */
 
 .macro STACKLEAK_ERASE
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -1215,40 +1215,15 @@ SYM_FUNC_END(entry_INT80_32)
 #endif
 .endm
 
-/*
- * Build the entry stubs with some assembler magic.
- * We pack 1 stub into every 8-byte block.
- */
-   .align 8
-SYM_CODE_START(irq_entries_start)
-vector=FIRST_EXTERNAL_VECTOR
-.rept (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
-   pushl   $(~vector+0x80) /* Note: always in signed byte 
range */
-vector=vector+1
-   jmp common_interrupt
-   .align  8
-.endr
-SYM_CODE_END(irq_entries_start)
-
 #ifdef CONFIG_X86_LOCAL_APIC
-   .align 8
-SYM_CODE_START(spurious_entries_start)
-vector=FIRST_SYSTEM_VECTOR
-.rept (NR_VECTORS - FIRST_SYSTEM_VECTOR)
-   pushl   $(~vector+0x80) /* Note: always in signed byte 
range */
-vector=vector+1
-   jmp common_spurious
-   .align  8
-.endr
-SYM_CODE_END(spurious_entries_start)
-
 SYM_CODE_START_LOCAL(common_spurious)
ASM_CLAC
-   addl$-0x80, (%esp)  /* Adjust vector into the 
[-256, -1] range */
SAVE_ALL switch_stacks=1
ENCODE_FRAME_POINTER
TRACE_IRQS_OFF
movl%esp, %eax
+   movlPT_ORIG_EAX(%esp), %edx /* get the vector from stack */
+   movl$-1, PT_ORIG_EAX(%esp)  /* no syscall to restart */
callsmp_spurious_interrupt
jmp ret_from_intr
 SYM_CODE_END(common_spurious)
@@ -1261,12 +1236,12 @@ SYM_CODE_END(common_spurious)
.p2align CONFIG_X86_L1_CACHE_SHIFT
 SYM_CODE_START_LOCAL(common_interrupt)
ASM_CLAC
-   addl$-0x80, (%esp)  /* Adjust vector into the 
[-256, -1] range */
-
SAVE_ALL switch_stacks=1
ENCODE_FRAME_POINTER
TRACE_IRQS_OFF
movl%esp, %eax
+   movlPT_ORIG_EAX(%esp), %edx /* get the vector from stack */
+   movl$-1, PT_ORIG_EAX(%esp)  /* no syscall to restart */
calldo_IRQ
jmp ret_from_intr
 SYM_CODE_END(common_interrupt)
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -359,34 +359,6 @@ SYM_CODE_START(ret_from_fork)
 SYM_CODE_END(ret_from_fork)
 .popsection
 
-/*
- * Build the entry stubs with some assembler magic.
- * We pack 1 stub into every 8-byte block.
- */
-   .align 8
-SYM_CODE_START(irq_entries_start)
-vector=FIRST_EXTERNAL_VECTOR
-.rept (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
-   UNWIND_HINT_IRET_REGS
-   pushq   $(~vector+0x80) /* Note: always in signed byte 
range */
-   jmp common_interrupt
-   .align  8
-   vector=vector+1
-.endr
-SYM_CODE_END(irq_entries_start)
-
-   .align 8
-SYM_CODE_START(spurious_entries_start)
-vector=FIRST_SYSTEM_VECTOR
-.rept (NR_VECTORS - FIRST_SYSTEM_VECTOR)
-   UNWIND_HINT_IRET_REGS
-   pushq   $(~vector+0x80) /* Note: always in signed byte 
range */