| Message ID | 20140912172337.GQ12361@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/09/14 18:23, Russell King - ARM Linux wrote: > On Fri, Sep 12, 2014 at 06:19:08PM +0100, Russell King - ARM Linux wrote: >> On Fri, Sep 12, 2014 at 06:14:04PM +0100, Russell King - ARM Linux wrote: >>> On Thu, Sep 11, 2014 at 12:31:14PM +0100, Daniel Thompson wrote: >>>> This patch introduces a new default FIQ handler that is structured in a >>>> similar way to the existing ARM exception handler and result in the FIQ >>>> being handled by C code running on the SVC stack (despite this code run >>>> in the FIQ handler is subject to severe limitations with respect to >>>> locking making normal interaction with the kernel impossible). >>>> >>>> This default handler allows concepts that on x86 would be handled using >>>> NMIs to be realized on ARM. >>> >>> Okay, lastly... I sent you my version of this change, which contained >>> the changes I've detailed in the previous three emails, which are absent >>> from your version. >>> >>> However, you've taken on board the "trace" thing to svc_entry, and >>> renamed it to "call_trace". >>> >>> Now if I add this to usr_entry, "call_trace" doesn't make any sense, >>> nor does the .if/.endif placement because it's not just trace_hardirqs_off >>> which needs to be disabled there, but also ct_user_exit as well. >>> >>> I'm beginning to wonder why I tried to be nice here... it would've been >>> a lot faster for me to take your patch, make my own changes and commit >>> that instead. >>> >>> Frustrated. >> >> And another thing you're missing are the updates to arch/arm/kernel/fiq.c >> to ensure that the FIQ registers are preserved when we restore this new >> default FIQ handler. > > Right, here's my remaining delta from your patch addressing all the points > from the last five emails. If you have any disagreements with any of these > changes, then please discuss rather than choosing to ignore them. Thanks for the diff. > 107e32b0b4ef5fa4191c9fc8415ca172b886e958 > diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S > index 0c70fee9a7c9..3f6293ce0f2d 100644 > --- a/arch/arm/kernel/entry-armv.S > +++ b/arch/arm/kernel/entry-armv.S > @@ -146,7 +146,7 @@ ENDPROC(__und_invalid) > #define SPFIX(code...) > #endif > > - .macro svc_entry, stack_hole=0, call_trace=1 > + .macro svc_entry, stack_hole=0, trace=1 > UNWIND(.fnstart ) > UNWIND(.save {r0 - pc} ) > sub sp, sp, #(S_FRAME_SIZE + \stack_hole - 4) > @@ -182,11 +182,11 @@ ENDPROC(__und_invalid) > @ > stmia r7, {r2 - r6} > > + .if \trace > #ifdef CONFIG_TRACE_IRQFLAGS > - .if \call_trace > bl trace_hardirqs_off > - .endif > #endif > + .endif > .endm > > .align 5 > @@ -298,7 +298,7 @@ ENDPROC(__pabt_svc) > > .align 5 > __fiq_svc: > - svc_entry 0, 0 > + svc_entry trace=0 > mov r0, sp @ struct pt_regs *regs > bl handle_fiq_as_nmi > svc_exit_via_fiq > @@ -326,7 +326,7 @@ ENDPROC(__fiq_svc) > @ > .align 5 > __fiq_abt: > - svc_entry 0, 0 > + svc_entry trace=0 > > ARM( msr cpsr_c, #ABT_MODE | PSR_I_BIT | PSR_F_BIT ) > THUMB( mov r0, #ABT_MODE | PSR_I_BIT | PSR_F_BIT ) > @@ -353,7 +353,7 @@ __fiq_abt: > > svc_exit_via_fiq > UNWIND(.fnend ) > -ENDPROC(__fiq_svc) > +ENDPROC(__fiq_abt) > > /* > * User mode handlers > @@ -365,7 +365,7 @@ ENDPROC(__fiq_svc) > #error "sizeof(struct pt_regs) must be a multiple of 8" > #endif > > - .macro usr_entry > + .macro usr_entry, trace=1 > UNWIND(.fnstart ) > UNWIND(.cantunwind ) @ don't unwind the user space > sub sp, sp, #S_FRAME_SIZE > @@ -402,10 +402,12 @@ ENDPROC(__fiq_svc) > @ > zero_fp > > + .if \trace > #ifdef CONFIG_IRQSOFF_TRACER > bl trace_hardirqs_off > #endif > ct_user_exit save = 0 > + .endif > .endm > > .macro kuser_cmpxchg_check > @@ -736,13 +738,13 @@ ENDPROC(ret_from_exception) > > .align 5 > __fiq_usr: > - usr_entry > + usr_entry trace=0 > kuser_cmpxchg_check > mov r0, sp @ struct pt_regs *regs > bl handle_fiq_as_nmi > get_thread_info tsk > - mov why, #0 > - b ret_to_user_from_irq > + restore_user_regs fast = 0, offset = 0 > + UNWIND(.cantunwind ) > UNWIND(.fnend ) > ENDPROC(__fiq_usr) > > diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c > index 918875d96d5d..1743049c433b 100644 > --- a/arch/arm/kernel/fiq.c > +++ b/arch/arm/kernel/fiq.c > @@ -53,6 +53,7 @@ > }) > > static unsigned long no_fiq_insn; > +static struct pt_regs def_fiq_regs; > > /* Default reacquire function > * - we always relinquish FIQ control > @@ -60,8 +61,15 @@ static unsigned long no_fiq_insn; > */ > static int fiq_def_op(void *ref, int relinquish) > { > - if (!relinquish) > + if (!relinquish) { > + /* Restore default handler and registers */ > + local_fiq_disable(); > + set_fiq_regs(&dfl_fiq_regs); This variable was declared as def_fiq_regs . > set_fiq_handler(&no_fiq_insn, sizeof(no_fiq_insn)); > + local_fiq_enable(); > + > + /* FIXME: notify irq controller to standard enable FIQs */ I don't understand what we need to tell the irq controller at this point. If we do want to mask sources of FIQ from arch code then in the case of IPI_CPU_BACKTRACE we might be better off disabling it at point of generation than at the interrupt controller (to avoid the long timeout). > + } > > return 0; > } > @@ -151,5 +159,6 @@ void __init init_FIQ(int start) > { > unsigned offset = FIQ_OFFSET; > no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); > + get_fiq_regs(&dfl_fiq_regs); > fiq_start = start; > } > >
On Sun, Sep 14, 2014 at 07:36:31AM +0100, Daniel Thompson wrote: > > - if (!relinquish) > > + if (!relinquish) { > > + /* Restore default handler and registers */ > > + local_fiq_disable(); > > + set_fiq_regs(&dfl_fiq_regs); > > This variable was declared as def_fiq_regs . Yep. > > set_fiq_handler(&no_fiq_insn, sizeof(no_fiq_insn)); > > + local_fiq_enable(); > > + > > + /* FIXME: notify irq controller to standard enable FIQs */ > > I don't understand what we need to tell the irq controller at this point. > > If we do want to mask sources of FIQ from arch code then in the case of > IPI_CPU_BACKTRACE we might be better off disabling it at point of > generation than at the interrupt controller (to avoid the long timeout). The point is, when a platform decides that it wants to insert its own /fast/ interrupt handler into the vector, our default one is moved out of the way. Those fast interrupt handlers assume that they are the only user of FIQ (since only one fast interrupt handler routine can be in place at any one time.) So, it's not going to expect to receive any other FIQs other than the one(s) it's written for. So, when the platform gives up the FIQ vector, and we restore the default FIQ code, we need to make sure that the FIQ sources for that code are re-enabled. Yes, you may not like this, and you may want to turn FIQ into a sprawling mess with interrupt source demux and such like, but that's not really on. The clue is in the name. *Fast*. On certain platforms, it gets used for time critical activities, and it being low latency is an absolute must.
On 12/09/14 18:23, Russell King - ARM Linux wrote: > On Fri, Sep 12, 2014 at 06:19:08PM +0100, Russell King - ARM Linux wrote: >> On Fri, Sep 12, 2014 at 06:14:04PM +0100, Russell King - ARM Linux wrote: >>> On Thu, Sep 11, 2014 at 12:31:14PM +0100, Daniel Thompson wrote: >>>> This patch introduces a new default FIQ handler that is structured in a >>>> similar way to the existing ARM exception handler and result in the FIQ >>>> being handled by C code running on the SVC stack (despite this code run >>>> in the FIQ handler is subject to severe limitations with respect to >>>> locking making normal interaction with the kernel impossible). >>>> >>>> This default handler allows concepts that on x86 would be handled using >>>> NMIs to be realized on ARM. >>> >>> Okay, lastly... I sent you my version of this change, which contained >>> the changes I've detailed in the previous three emails, which are absent >>> from your version. >>> >>> However, you've taken on board the "trace" thing to svc_entry, and >>> renamed it to "call_trace". >>> >>> Now if I add this to usr_entry, "call_trace" doesn't make any sense, >>> nor does the .if/.endif placement because it's not just trace_hardirqs_off >>> which needs to be disabled there, but also ct_user_exit as well. >>> >>> I'm beginning to wonder why I tried to be nice here... it would've been >>> a lot faster for me to take your patch, make my own changes and commit >>> that instead. >>> >>> Frustrated. >> >> And another thing you're missing are the updates to arch/arm/kernel/fiq.c >> to ensure that the FIQ registers are preserved when we restore this new >> default FIQ handler. > > Right, here's my remaining delta from your patch addressing all the points > from the last five emails. If you have any disagreements with any of these > changes, then please discuss rather than choosing to ignore them. > > 107e32b0b4ef5fa4191c9fc8415ca172b886e958 > diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S > index 0c70fee9a7c9..3f6293ce0f2d 100644 > --- a/arch/arm/kernel/entry-armv.S > +++ b/arch/arm/kernel/entry-armv.S > <snip> > @@ -736,13 +738,13 @@ ENDPROC(ret_from_exception) > > .align 5 > __fiq_usr: > - usr_entry > + usr_entry trace=0 > kuser_cmpxchg_check > mov r0, sp @ struct pt_regs *regs > bl handle_fiq_as_nmi > get_thread_info tsk > - mov why, #0 > - b ret_to_user_from_irq > + restore_user_regs fast = 0, offset = 0 > + UNWIND(.cantunwind ) .cantunwind is already provided by the usr_entry macro (also adding .cantunwind here causes my assembler (2.24) to fail). > UNWIND(.fnend ) > ENDPROC(__fiq_usr) > > diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c > index 918875d96d5d..1743049c433b 100644 > --- a/arch/arm/kernel/fiq.c > +++ b/arch/arm/kernel/fiq.c > @@ -53,6 +53,7 @@ > }) > > static unsigned long no_fiq_insn; > +static struct pt_regs def_fiq_regs; > > /* Default reacquire function > * - we always relinquish FIQ control > @@ -60,8 +61,15 @@ static unsigned long no_fiq_insn; > */ > static int fiq_def_op(void *ref, int relinquish) > { > - if (!relinquish) > + if (!relinquish) { > + /* Restore default handler and registers */ > + local_fiq_disable(); > + set_fiq_regs(&dfl_fiq_regs); > set_fiq_handler(&no_fiq_insn, sizeof(no_fiq_insn)); > + local_fiq_enable(); > + > + /* FIXME: notify irq controller to standard enable FIQs */ > + } > > return 0; > } > @@ -151,5 +159,6 @@ void __init init_FIQ(int start) > { > unsigned offset = FIQ_OFFSET; > no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); > + get_fiq_regs(&dfl_fiq_regs); > fiq_start = start; > } > >
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S index 0c70fee9a7c9..3f6293ce0f2d 100644 --- a/arch/arm/kernel/entry-armv.S +++ b/arch/arm/kernel/entry-armv.S @@ -146,7 +146,7 @@ ENDPROC(__und_invalid) #define SPFIX(code...) #endif - .macro svc_entry, stack_hole=0, call_trace=1 + .macro svc_entry, stack_hole=0, trace=1 UNWIND(.fnstart ) UNWIND(.save {r0 - pc} ) sub sp, sp, #(S_FRAME_SIZE + \stack_hole - 4) @@ -182,11 +182,11 @@ ENDPROC(__und_invalid) @ stmia r7, {r2 - r6} + .if \trace #ifdef CONFIG_TRACE_IRQFLAGS - .if \call_trace bl trace_hardirqs_off - .endif #endif + .endif .endm .align 5 @@ -298,7 +298,7 @@ ENDPROC(__pabt_svc) .align 5 __fiq_svc: - svc_entry 0, 0 + svc_entry trace=0 mov r0, sp @ struct pt_regs *regs bl handle_fiq_as_nmi svc_exit_via_fiq @@ -326,7 +326,7 @@ ENDPROC(__fiq_svc) @ .align 5 __fiq_abt: - svc_entry 0, 0 + svc_entry trace=0 ARM( msr cpsr_c, #ABT_MODE | PSR_I_BIT | PSR_F_BIT ) THUMB( mov r0, #ABT_MODE | PSR_I_BIT | PSR_F_BIT ) @@ -353,7 +353,7 @@ __fiq_abt: svc_exit_via_fiq UNWIND(.fnend ) -ENDPROC(__fiq_svc) +ENDPROC(__fiq_abt) /* * User mode handlers @@ -365,7 +365,7 @@ ENDPROC(__fiq_svc) #error "sizeof(struct pt_regs) must be a multiple of 8" #endif - .macro usr_entry + .macro usr_entry, trace=1 UNWIND(.fnstart ) UNWIND(.cantunwind ) @ don't unwind the user space sub sp, sp, #S_FRAME_SIZE @@ -402,10 +402,12 @@ ENDPROC(__fiq_svc) @ zero_fp + .if \trace #ifdef CONFIG_IRQSOFF_TRACER bl trace_hardirqs_off #endif ct_user_exit save = 0 + .endif .endm .macro kuser_cmpxchg_check @@ -736,13 +738,13 @@ ENDPROC(ret_from_exception) .align 5 __fiq_usr: - usr_entry + usr_entry trace=0 kuser_cmpxchg_check mov r0, sp @ struct pt_regs *regs bl handle_fiq_as_nmi get_thread_info tsk - mov why, #0 - b ret_to_user_from_irq + restore_user_regs fast = 0, offset = 0 + UNWIND(.cantunwind ) UNWIND(.fnend ) ENDPROC(__fiq_usr) diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c index 918875d96d5d..1743049c433b 100644 --- a/arch/arm/kernel/fiq.c +++ b/arch/arm/kernel/fiq.c @@ -53,6 +53,7 @@ }) static unsigned long no_fiq_insn; +static struct pt_regs def_fiq_regs; /* Default reacquire function * - we always relinquish FIQ control @@ -60,8 +61,15 @@ static unsigned long no_fiq_insn; */ static int fiq_def_op(void *ref, int relinquish) { - if (!relinquish) + if (!relinquish) { + /* Restore default handler and registers */ + local_fiq_disable(); + set_fiq_regs(&dfl_fiq_regs); set_fiq_handler(&no_fiq_insn, sizeof(no_fiq_insn)); + local_fiq_enable(); + + /* FIXME: notify irq controller to standard enable FIQs */ + } return 0; } @@ -151,5 +159,6 @@ void __init init_FIQ(int start) { unsigned offset = FIQ_OFFSET; no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); + get_fiq_regs(&dfl_fiq_regs); fiq_start = start; }