* [Adeos-main] [PATCH 3/3] i386: Cleanup root IRQ trampolines
@ 2007-12-26 15:40 Jan Kiszka
2007-12-26 22:11 ` Philippe Gerum
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kiszka @ 2007-12-26 15:40 UTC (permalink / raw)
To: adeos-main; +Cc: Philippe Gerum
[-- Attachment #1.1: Type: text/plain, Size: 149 bytes --]
Simplifies xirq trampoline and applies the xirq refactorings to
virq_handler too.
Merry Christmas and all the best for 2008 to everyone!
Jan
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: cleanup-root-irq-trampolines-i386.patch --]
[-- Type: text/x-patch; name="cleanup-root-irq-trampolines-i386.patch", Size: 3517 bytes --]
---
include/asm-i386/ipipe.h | 78 ++++++++++++++++++++++++-----------------------
1 file changed, 40 insertions(+), 38 deletions(-)
Index: linux-2.6.23.12-xeno/include/asm-i386/ipipe.h
===================================================================
--- linux-2.6.23.12-xeno.orig/include/asm-i386/ipipe.h
+++ linux-2.6.23.12-xeno/include/asm-i386/ipipe.h
@@ -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_ffnz
if (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); \
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Adeos-main] [PATCH 3/3] i386: Cleanup root IRQ trampolines
2007-12-26 15:40 [Adeos-main] [PATCH 3/3] i386: Cleanup root IRQ trampolines Jan Kiszka
@ 2007-12-26 22:11 ` Philippe Gerum
2007-12-27 0:19 ` Jan Kiszka
0 siblings, 1 reply; 5+ messages in thread
From: Philippe Gerum @ 2007-12-26 22:11 UTC (permalink / raw)
To: Jan Kiszka; +Cc: adeos-main
Jan Kiszka wrote:
> Simplifies xirq trampoline and applies the xirq refactorings to
> virq_handler too.
>
--- linux-2.6.23.12-xeno.orig/include/asm-i386/ipipe.h
+++ linux-2.6.23.12-xeno/include/asm-i386/ipipe.h
@@ -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.
--
Philippe.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Adeos-main] [PATCH 3/3] i386: Cleanup root IRQ trampolines
2007-12-26 22:11 ` Philippe Gerum
@ 2007-12-27 0:19 ` Jan Kiszka
2007-12-27 8:49 ` Philippe Gerum
2007-12-27 9:48 ` Philippe Gerum
0 siblings, 2 replies; 5+ messages in thread
From: Jan Kiszka @ 2007-12-27 0:19 UTC (permalink / raw)
To: rpm; +Cc: adeos-main
[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]
Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Simplifies xirq trampoline and applies the xirq refactorings to
>> virq_handler too.
>>
>
> --- linux-2.6.23.12-xeno.orig/include/asm-i386/ipipe.h
> +++ linux-2.6.23.12-xeno/include/asm-i386/ipipe.h
> @@ -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.
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 :->.
>
> "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).
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.
>
> + "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.
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?
>
> + "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.
>
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Adeos-main] [PATCH 3/3] i386: Cleanup root IRQ trampolines
2007-12-27 0:19 ` Jan Kiszka
@ 2007-12-27 8:49 ` Philippe Gerum
2007-12-27 9:48 ` Philippe Gerum
1 sibling, 0 replies; 5+ messages in thread
From: Philippe Gerum @ 2007-12-27 8:49 UTC (permalink / raw)
To: Jan Kiszka; +Cc: adeos-main
Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Simplifies xirq trampoline and applies the xirq refactorings to
>>> virq_handler too.
>>>
>> --- linux-2.6.23.12-xeno.orig/include/asm-i386/ipipe.h
>> +++ linux-2.6.23.12-xeno/include/asm-i386/ipipe.h
>> @@ -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.
>
> 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.
>
>> "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).
>
> 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.
>
Yes.
>> + "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.
>
> 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.
>> + "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.
>>
>
> Jan
>
--
Philippe.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Adeos-main] [PATCH 3/3] i386: Cleanup root IRQ trampolines
2007-12-27 0:19 ` Jan Kiszka
2007-12-27 8:49 ` Philippe Gerum
@ 2007-12-27 9:48 ` Philippe Gerum
1 sibling, 0 replies; 5+ messages in thread
From: Philippe Gerum @ 2007-12-27 9:48 UTC (permalink / raw)
To: Jan Kiszka; +Cc: adeos-main
Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Simplifies xirq trampoline and applies the xirq refactorings to
>>> virq_handler too.
>>>
>> --- linux-2.6.23.12-xeno.orig/include/asm-i386/ipipe.h
>> +++ linux-2.6.23.12-xeno/include/asm-i386/ipipe.h
>> @@ -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.
>
> 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.
--
Philippe.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-12-27 9:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-12-26 15:40 [Adeos-main] [PATCH 3/3] i386: Cleanup root IRQ trampolines Jan Kiszka
2007-12-26 22:11 ` Philippe Gerum
2007-12-27 0:19 ` Jan Kiszka
2007-12-27 8:49 ` Philippe Gerum
2007-12-27 9:48 ` Philippe Gerum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.