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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Adeos-main mailing list [email protected] https://mail.gna.org/listinfo/adeos-main
