i386: Cleanup root IRQ trampolines
PATCH 3/3 i386: Cleanup root IRQ trampolines
Simplifies xirq trampoline and applies the xirq refactorings to virq_handler too.
Merry Christmas and all the best for 2008 to everyone!
Jan
--- include/asm-i386/ipipe.h | 78 ++++++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 38 deletions(-)
include/asm-i386/ipipe.h +40 -38
@@ -141,11 +141,7 @@ __ipipe_call_root_xirq_handler(unsigned"pushl %%ds\n\t" "pushl %%eax\n\t" "pushl %%ebp\n\t" -"pushl %%edi\n\t" -"pushl %%esi\n\t" -"pushl %%edx\n\t" -"pushl %%ecx\n\t" -"pushl %%ebx\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t" "movl %[regs],%%eax # always pass tick regs\n\t" "call *%[handler]\n\t"
@@ -158,33 +154,41 @@ __ipipe_call_root_xirq_handler(unsigned : "eax"); } -#define __ipipe_call_root_virq_handler(ipd,irq) \ - __asm__ __volatile__ ("pushfl\n\t" \ - "pushl %%cs\n\t" \ - "pushl $__virq_end\n\t" \ - "pushl $-1\n\t" \ - "pushl %%fs\n\t" \ - "pushl %%es\n\t" \ - "pushl %%ds\n\t" \ - "pushl %%eax\n\t" \ - "pushl %%ebp\n\t" \ - "pushl %%edi\n\t" \ - "pushl %%esi\n\t" \ - "pushl %%edx\n\t" \ - "pushl %%ecx\n\t" \ - "pushl %%ebx\n\t" \ - "pushl %2\n\t" \ - "pushl %%eax\n\t" \ - "call *%1\n\t" \ - "addl $8,%%esp\n\t" \ - : /* no output */ \ - : "a" (irq), "m" ((ipd)->irqs[irq].handler), "r" ((ipd)->irqs[irq].cookie)) - -#define __ipipe_finalize_root_virq_handler() \ - __asm__ __volatile__ ("jmp ret_from_intr\n\t" \ - "__virq_end: cli\n" \ - : /* no output */ \ - : /* no input */) +void irq_enter (void ); +void irq_exit (void ); + +static inline void +__ipipe_call_root_virq_handler (unsigned irq, +void (*handler)(unsigned irq,void *cookie), +void *cookie) +{ +irq_enter (); + __asm__ __volatile__ ( +"pushfl\n\t" +"pushl %%cs\n\t" +"pushl $virq_end\n\t" +"pushl %%eax # dummy value\n\t" +"pushl %%fs\n\t" +"pushl %%es\n\t" +"pushl %%ds\n\t" +"pushl %%eax\n\t" +"pushl %%ebp\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t" + +"pushl %[cookie]\n\t" +"pushl %[irq]\n\t" +"call *%[handler]\n\t" +"addl $8,%%esp\n\t" \ + :/* no output */ \ + : [irq]"rm" (irq), [handler]"rm" (handler), + [cookie]"rm" (cookie)); +irq_exit (); + __asm__ __volatile__ ( +"jmp ret_from_intr # Linux IRQ epilogue\n\t" +"virq_end: cli\n\t" + :/* no output */ + :/* no input */ ); +}static inline unsigned long __ipipe_ffnz (unsigned long ul) {
@@ -205,12 +209,10 @@ static inline unsigned long __ipipe_ffnzif (likely (!ipipe_virtual_irq_p (irq))) \__ipipe_call_root_xirq_handler ( \ irq, (ipd)->irqs[irq].handler); \ -else { \ -irq_enter (); \ -__ipipe_call_root_virq_handler (ipd,irq); \ -irq_exit (); \ -__ipipe_finalize_root_virq_handler (); \ - } \ +else \ +__ipipe_call_root_virq_handler ( \ + irq, (ipd)->irqs[irq].handler, \ + (ipd)->irqs[irq].cookie); \ }else { \__clear_bit (IPIPE_SYNC_FLAG , &ipipe_cpudom_var (ipd, status)); \ ipd->irqs[irq].handler (irq, ipd->irqs[irq].cookie); \
Quoted 3 lines · Jan Kiszka
Simplifies xirq trampoline and applies the xirq refactorings to
virq_handlertoo.
include/asm-i386/ipipe.h +1 -5
@@ -141,11 +141,7 @@ __ipipe_call_root_xirq_handler(unsigned"pushl %%ds\n\t" "pushl %%eax\n\t" "pushl %%ebp\n\t" -"pushl %%edi\n\t" -"pushl %%esi\n\t" -"pushl %%edx\n\t" -"pushl %%ecx\n\t" -"pushl %%ebx\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t"
This won't work. We actually want to save our regs, so that they are restored when going back to xirq_end through the iret path. If we don't save them, the registers will remain terminally clobbered.
"movl %[regs],%%eax # always pass tick regs\n\t"
"call *%[handler]\n\t"
@@ -158,33 +154,41 @@ __ipipe_call_root_xirq_handler(unsigned
: "eax");
}
[snip]
+static inline void
+__ipipe_call_root_virq_handler(unsigned irq,
+ void (*handler)(unsigned irq, void *cookie),
+ void *cookie)
+{
+ irq_enter();
+ __asm__ __volatile__ (
+ "pushfl\n\t"
+ "pushl %%cs\n\t"
+ "pushl $virq_end\n\t"
+ "pushl %%eax # dummy value\n\t"
We do want -1 to be pushed here, not any dummy value, so that the signal handling code is not going to try restarting some interrupted syscall when running the Linux epilogue for us. This is consistent with having negated irq numbers pushed onto interrupt frames (see handle_signal and friends).
+ "pushl %%fs\n\t"
+ "pushl %%es\n\t"
+ "pushl %%ds\n\t"
+ "pushl %%eax\n\t"
+ "pushl %%ebp\n\t"
+ "subl $5*4,%%esp # rest can remain unsaved\n\t"
+
Same problem as above.
+ "pushl %[cookie]\n\t"
+ "pushl %[irq]\n\t"
+ "call *%[handler]\n\t"
We need to conform both to regparm(0) and regparm(3) when passing args to the interrupt handler (i.e we don't know the convention used, especially by assembly code grabbing an IRQ), and regparm(3) is missing now. C-written ones are likely to follow regparm(3) as the rest of the kernel unless they explicitly fiddle with compiler pragmas. We could get rid of regparm(0) in x86 code, requiring all handlers to conform to regparm(3), but not the other way around.
+ "addl $8,%%esp\n\t" \
+ : /* no output */ \
+ : [irq] "rm" (irq), [handler] "rm" (handler),
+ [cookie] "rm" (cookie));
+ irq_exit();
+ __asm__ __volatile__ (
+ "jmp ret_from_intr # Linux IRQ epilogue\n\t"
+ "virq_end: cli\n\t"
+ : /* no output */
+ : /* no input */);
+}
This said, the fix regarding CPU accounting is right, and passing the tick regs from the trampoline looks definitely saner than tweaking timer_interrupt() for the same purpose, so I'll merge that part.
Signature · 1 line
Philippe.
Quoted 18 lines · Philippe Gerum
Quoted 3 lines · Jan Kiszka
Simplifies xirq trampoline and applies the xirq refactorings to
virq_handlertoo.include/asm-i386/ipipe.h +1
-5@@ -141,11 +141,7 @@ __ipipe_call_root_xirq_handler(unsigned"pushl %%ds\n\t" "pushl %%eax\n\t" "pushl %%ebp\n\t" -"pushl %%edi\n\t" -"pushl %%esi\n\t" -"pushl %%edx\n\t" -"pushl %%ecx\n\t" -"pushl %%ebx\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t"
Quoted 3 lines
This won't work. We actually want to save our regs, so that they are restored when going back to
xirq_endthrough the iret path. If we don't save them, the registers will remain terminally clobbered.
General purpose registers aren't stable across functions calls. But it is a valid question what the compiler assumes due to the function inlining here. So declaring those five clobbered should be safe enough - but if you prefer to add a few extra cycles for safety, keep it as explicit as it was :->.
Quoted 21 lines
include/asm-i386/ipipe.h +11
-0"movl %[regs],%%eax # always pass tick regs\n\t" "call *%[handler]\n\t" @@ -158,33 +154,41 @@ __ipipe_call_root_xirq_handler(unsigned : "eax"); } +static inline void +__ipipe_call_root_virq_handler (unsigned irq, +void (*handler)(unsigned irq,void *cookie), +void *cookie) +{ +irq_enter (); + __asm__ __volatile__ ( +"pushfl\n\t" +"pushl %%cs\n\t" +"pushl $virq_end\n\t" + "pushl %%eax # dummy value\n\t"
Quoted 5 lines
We do want -1 to be pushed here, not any dummy value, so that the signal handling code is not going to try restarting some interrupted syscall when running the Linux epilogue for us. This is consistent with having negated irq numbers pushed onto interrupt frames (see
handle_signaland friends).
Ah, ok, missed that path. Then we need the right irq number on the stack of call_root_xirq_handler as well, for both archs.
Quoted 15 lines
include/asm-i386/ipipe.h +7
-0+"pushl %%fs\n\t" +"pushl %%es\n\t" +"pushl %%ds\n\t" +"pushl %%eax\n\t" +"pushl %%ebp\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t" +Same problem as above.
include/asm-i386/ipipe.h +3
-0+"pushl %[cookie]\n\t" +"pushl %[irq]\n\t" +"call *%[handler]\n\t"
Quoted 7 lines
We need to conform both to regparm(0) and regparm(3) when passing args to the interrupt handler (i.e we don't know the convention used, especially by assembly code grabbing an IRQ), and regparm(3) is missing now. C-written ones are likely to follow regparm(3) as the rest of the kernel unless they explicitly fiddle with compiler pragmas. We could get rid of regparm(0) in x86 code, requiring all handlers to conform to regparm(3), but not the other way around.
Previous code wasn't regparm(3)-safe (cookie should have been pinned to edx then), so I don't see any new problem with this variant - unless we were just very lucky before. Are you sure we need both here?
Quoted 13 lines
include/asm-i386/ipipe.h +11
-0+"addl $8,%%esp\n\t" \ + : /* no output */ \ + : [irq] "rm" (irq), [handler] "rm" (handler), + [cookie] "rm" (cookie)); +irq_exit (); + __asm__ __volatile__ ( +"jmp ret_from_intr # Linux IRQ epilogue\n\t" +"virq_end: cli\n\t" + :/* no output */ + :/* no input */ ); +}
Quoted 4 lines
This said, the fix regarding CPU accounting is right, and passing the tick regs from the trampoline looks definitely saner than tweaking
timer_interrupt()for the same purpose, so I'll merge that part.
Jan
Quoted 18 lines · Jan Kiszka
Quoted 17 lines · Philippe Gerum
Quoted 3 lines · Jan Kiszka
Simplifies xirq trampoline and applies the xirq refactorings to
virq_handlertoo.include/asm-i386/ipipe.h +1
-5@@ -141,11 +141,7 @@ __ipipe_call_root_xirq_handler(unsigned"pushl %%ds\n\t" "pushl %%eax\n\t" "pushl %%ebp\n\t" -"pushl %%edi\n\t" -"pushl %%esi\n\t" -"pushl %%edx\n\t" -"pushl %%ecx\n\t" -"pushl %%ebx\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t"
Quoted 9 lines
Quoted 3 lines
This won't work. We actually want to save our regs, so that they are restored when going back to
xirq_endthrough the iret path. If we don't save them, the registers will remain terminally clobbered.General purpose registers aren't stable across functions calls. But it is a valid question what the compiler assumes due to the function inlining here. So declaring those five clobbered should be safe enough - but if you prefer to add a few extra cycles for safety, keep it as explicit as it was :->.
Problem is that the compiler cannot assume anything right about what's going to be trashed by jumping to ret_from_intr, so caller-save action will not be enough.
Quoted 21 lines
Quoted 21 lines
include/asm-i386/ipipe.h +11
-0"movl %[regs],%%eax # always pass tick regs\n\t" "call *%[handler]\n\t" @@ -158,33 +154,41 @@ __ipipe_call_root_xirq_handler(unsigned : "eax"); } +static inline void +__ipipe_call_root_virq_handler (unsigned irq, +void (*handler)(unsigned irq,void *cookie), +void *cookie) +{ +irq_enter (); + __asm__ __volatile__ ( +"pushfl\n\t" +"pushl %%cs\n\t" +"pushl $virq_end\n\t" + "pushl %%eax # dummy value\n\t"
Quoted 9 lines
Quoted 5 lines
We do want -1 to be pushed here, not any dummy value, so that the signal handling code is not going to try restarting some interrupted syscall when running the Linux epilogue for us. This is consistent with having negated irq numbers pushed onto interrupt frames (see
handle_signaland friends).Ah, ok, missed that path. Then we need the right irq number on the stack of
call_root_xirq_handleras well, for both archs.
Yes.
Quoted 14 lines
Quoted 14 lines
include/asm-i386/ipipe.h +7
-0+"pushl %%fs\n\t" +"pushl %%es\n\t" +"pushl %%ds\n\t" +"pushl %%eax\n\t" +"pushl %%ebp\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t" +Same problem as above.
include/asm-i386/ipipe.h +3
-0+"pushl %[cookie]\n\t" +"pushl %[irq]\n\t" +"call *%[handler]\n\t"
Quoted 12 lines
Quoted 7 lines
We need to conform both to regparm(0) and regparm(3) when passing args to the interrupt handler (i.e we don't know the convention used, especially by assembly code grabbing an IRQ), and regparm(3) is missing now. C-written ones are likely to follow regparm(3) as the rest of the kernel unless they explicitly fiddle with compiler pragmas. We could get rid of regparm(0) in x86 code, requiring all handlers to conform to regparm(3), but not the other way around.
Previous code wasn't regparm(3)-safe (cookie should have been pinned to edx then), so I don't see any new problem with this variant - unless we were just very lucky before. Are you sure we need both here?
Actually, cookie was unused in the virq case (the trampoline code should have been updated after this second parameter was added), but %eax did contain the irq number which was enough to have things working, even if incomplete for regparm(3). Proper fix should actually load %eax and %edx.
Quoted 19 lines
Quoted 12 lines
include/asm-i386/ipipe.h +11
-0+"addl $8,%%esp\n\t" \ + : /* no output */ \ + : [irq] "rm" (irq), [handler] "rm" (handler), + [cookie] "rm" (cookie)); +irq_exit (); + __asm__ __volatile__ ( +"jmp ret_from_intr # Linux IRQ epilogue\n\t" +"virq_end: cli\n\t" + :/* no output */ + :/* no input */ ); +}Quoted 4 lines
This said, the fix regarding CPU accounting is right, and passing the tick regs from the trampoline looks definitely saner than tweaking
timer_interrupt()for the same purpose, so I'll merge that part.Jan
Signature · 1 line
Philippe.
Quoted 18 lines · Jan Kiszka
Quoted 17 lines · Philippe Gerum
Quoted 3 lines · Jan Kiszka
Simplifies xirq trampoline and applies the xirq refactorings to
virq_handlertoo.include/asm-i386/ipipe.h +1
-5@@ -141,11 +141,7 @@ __ipipe_call_root_xirq_handler(unsigned"pushl %%ds\n\t" "pushl %%eax\n\t" "pushl %%ebp\n\t" -"pushl %%edi\n\t" -"pushl %%esi\n\t" -"pushl %%edx\n\t" -"pushl %%ecx\n\t" -"pushl %%ebx\n\t" +"subl $5*4,%%esp # rest can remain unsaved\n\t"
Quoted 10 lines
Quoted 3 lines
This won't work. We actually want to save our regs, so that they are restored when going back to
xirq_endthrough the iret path. If we don't save them, the registers will remain terminally clobbered.General purpose registers aren't stable across functions calls. But it is a valid question what the compiler assumes due to the function inlining here. So declaring those five clobbered should be safe enough - but if you prefer to add a few extra cycles for safety, keep it as explicit as it was :->.
Declaring clobbers would duplicate what entry.S already does when restoring args from the interrupt frame.
Signature · 1 line
Philippe.