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