| Message ID | 1416936401-5147-6-git-send-email-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel Am Dienstag, 25. November 2014, 17:26:41 schrieb Daniel Thompson: > Previous changes have introduced both a replacement default FIQ handler > and an implementation of arch_trigger_all_cpu_backtrace for ARM but > these are currently independent of each other. > > This patch plumbs together these features making it possible, on platforms > that support it, to trigger backtrace using FIQ. Does this ipi handler interfere in any way with set_fiq_handler? As far as i know there is only one FIQ handler vector so i guess there is a potential conflict. But i have not worked with IPI's so i might be completley wrong. Regards Tim > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > arch/arm/include/asm/smp.h | 3 +++ > arch/arm/kernel/smp.c | 4 +++- > arch/arm/kernel/traps.c | 3 +++ > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h > index 18f5a554134f..b076584ac0fa 100644 > --- a/arch/arm/include/asm/smp.h > +++ b/arch/arm/include/asm/smp.h > @@ -18,6 +18,8 @@ > # error "<asm/smp.h> included in non-SMP build" > #endif > > +#define SMP_IPI_FIQ_MASK 0x0100 > + > #define raw_smp_processor_id() (current_thread_info()->cpu) > > struct seq_file; > @@ -79,6 +81,7 @@ extern void arch_send_call_function_single_ipi(int cpu); > extern void arch_send_call_function_ipi_mask(const struct cpumask *mask); > extern void arch_send_wakeup_ipi_mask(const struct cpumask *mask); > > +extern void ipi_cpu_backtrace(struct pt_regs *regs); > extern int register_ipi_completion(struct completion *completion, int cpu); > > struct smp_operations { > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c > index 14c594a12bef..e923843562d9 100644 > --- a/arch/arm/kernel/smp.c > +++ b/arch/arm/kernel/smp.c > @@ -539,7 +539,7 @@ static void ipi_cpu_stop(unsigned int cpu) > cpu_relax(); > } > > -static void ipi_cpu_backtrace(struct pt_regs *regs) > +void ipi_cpu_backtrace(struct pt_regs *regs) > { > int cpu = smp_processor_id(); > > @@ -580,6 +580,8 @@ void handle_IPI(int ipinr, struct pt_regs *regs) > unsigned int cpu = smp_processor_id(); > struct pt_regs *old_regs = set_irq_regs(regs); > > + BUILD_BUG_ON(SMP_IPI_FIQ_MASK != BIT(IPI_CPU_BACKTRACE)); > + > if ((unsigned)ipinr < NR_IPI) { > trace_ipi_entry(ipi_types[ipinr]); > __inc_irq_stat(cpu, ipi_irqs[ipinr]); > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 4dc45b38e56e..9eb05be9526e 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -483,6 +483,9 @@ asmlinkage void __exception_irq_entry > handle_fiq_as_nmi(struct pt_regs *regs) #ifdef CONFIG_ARM_GIC > gic_handle_fiq_ipi(); > #endif > +#ifdef CONFIG_SMP > + ipi_cpu_backtrace(regs); > +#endif > > nmi_exit();
On Wed, Nov 26, 2014 at 01:46:52PM +0100, Tim Sander wrote: > Hi Daniel > > Am Dienstag, 25. November 2014, 17:26:41 schrieb Daniel Thompson: > > Previous changes have introduced both a replacement default FIQ handler > > and an implementation of arch_trigger_all_cpu_backtrace for ARM but > > these are currently independent of each other. > > > > This patch plumbs together these features making it possible, on platforms > > that support it, to trigger backtrace using FIQ. > Does this ipi handler interfere in any way with set_fiq_handler? > > As far as i know there is only one FIQ handler vector so i guess there is a > potential conflict. But i have not worked with IPI's so i might be completley > wrong. First, the code in arch/arm/kernel/fiq.c should work with this new FIQ code in that the new FIQ code is used as the "default" handler (as opposed to the original handler which was a total no-op.) Secondly, use of arch/arm/kernel/fiq.c in a SMP system is really not a good idea: the FIQ registers are private to each CPU in the system, and there is no infrastructure to allow fiq.c to ensure that it loads the right CPU with the register information for the provided handler. So, use of arch/arm/kernel/fiq.c and the IPI's use of FIQ /should/ be mutually exclusive.
On 26/11/14 13:12, Russell King - ARM Linux wrote: > On Wed, Nov 26, 2014 at 01:46:52PM +0100, Tim Sander wrote: >> Hi Daniel >> >> Am Dienstag, 25. November 2014, 17:26:41 schrieb Daniel Thompson: >>> Previous changes have introduced both a replacement default FIQ handler >>> and an implementation of arch_trigger_all_cpu_backtrace for ARM but >>> these are currently independent of each other. >>> >>> This patch plumbs together these features making it possible, on platforms >>> that support it, to trigger backtrace using FIQ. >> Does this ipi handler interfere in any way with set_fiq_handler? >> >> As far as i know there is only one FIQ handler vector so i guess there is a >> potential conflict. But i have not worked with IPI's so i might be completley >> wrong. > > First, the code in arch/arm/kernel/fiq.c should work with this new FIQ > code in that the new FIQ code is used as the "default" handler (as > opposed to the original handler which was a total no-op.) > > Secondly, use of arch/arm/kernel/fiq.c in a SMP system is really not a > good idea: the FIQ registers are private to each CPU in the system, and > there is no infrastructure to allow fiq.c to ensure that it loads the > right CPU with the register information for the provided handler. > > So, use of arch/arm/kernel/fiq.c and the IPI's use of FIQ /should/ be > mutually exclusive. Agree with the above. Just to add... I am currently working to get NMI features from x86 land running on top of the new default FIQ handler: arch_trigger_all_cpu_backtrace (with Russell's patch), perf, hard lockup detector, kgdb. However I don't think anything I'm doing makes it very much harder than it already is to use arch/arm/kernel/fiq.c . That said, other then setting the GIC up nicely, I am not doing anything to make it easier either. I'd like to end up somewhere where if you want the NMI features (and have a suitable device) you just use the default handler and it all just works. If you need *Fast* Interrupt reQuests, proper old school "I want to write an overclocked I2C slave in software" craziness and you can pass on the improved debug features then set_fiq_handler() is still there and still need extremely careful handling. The only thing I might have done to make your life worse is not provide the code to dynamically shunt all the debug and performance monitoring features back to group 1. All except the hard lockup detector will have logic to fall back statically. This means making it dynamic shouldn't be that hard. However since there is no code in the upstream kernel that would use the code I don't plan to go there myself. Daniel.
Hi Daniel, Russell Am Mittwoch, 26. November 2014, 16:17:06 schrieb Daniel Thompson: > On 26/11/14 13:12, Russell King - ARM Linux wrote: > > On Wed, Nov 26, 2014 at 01:46:52PM +0100, Tim Sander wrote: > >> Hi Daniel > >> > >> Am Dienstag, 25. November 2014, 17:26:41 schrieb Daniel Thompson: > >>> Previous changes have introduced both a replacement default FIQ handler > >>> and an implementation of arch_trigger_all_cpu_backtrace for ARM but > >>> these are currently independent of each other. > >>> > >>> This patch plumbs together these features making it possible, on > >>> platforms > >>> that support it, to trigger backtrace using FIQ. > >> > >> Does this ipi handler interfere in any way with set_fiq_handler? > >> > >> As far as i know there is only one FIQ handler vector so i guess there is > >> a > >> potential conflict. But i have not worked with IPI's so i might be > >> completley wrong. > > > > First, the code in arch/arm/kernel/fiq.c should work with this new FIQ > > code in that the new FIQ code is used as the "default" handler (as > > opposed to the original handler which was a total no-op.) > > > > Secondly, use of arch/arm/kernel/fiq.c in a SMP system is really not a > > good idea: the FIQ registers are private to each CPU in the system, and > > there is no infrastructure to allow fiq.c to ensure that it loads the > > right CPU with the register information for the provided handler. Well given the races in the GIC v1. i have seen in the chips on my desk initializing with for_each_possible_cpu(cpu) work_on_cpu(cpu,..) is rather easy. > > So, use of arch/arm/kernel/fiq.c and the IPI's use of FIQ /should/ be > > mutually exclusive. Yes but i digress on the assessment that this a decision between SMP and non- SMP usage or the availbility of the GIC. > Agree with the above. Just to add... > > I am currently working to get NMI features from x86 land running on top > of the new default FIQ handler: arch_trigger_all_cpu_backtrace (with > Russell's patch), perf, hard lockup detector, kgdb. > > However I don't think anything I'm doing makes it very much harder than > it already is to use arch/arm/kernel/fiq.c . That said, other then > setting the GIC up nicely, I am not doing anything to make it easier either. > > I'd like to end up somewhere where if you want the NMI features (and > have a suitable device) you just use the default handler and it all just > works. If you need *Fast* Interrupt reQuests, proper old school "I want > to write an overclocked I2C slave in software" craziness and you can > pass on the improved debug features then set_fiq_handler() is still > there and still need extremely careful handling. Well i am not against these features as they assumably improve the backtrace, but it would be nice to have a config option which switches between set_fiq_handler usage and the other conflicting usages of the fiq. > The only thing I might have done to make your life worse is not provide > the code to dynamically shunt all the debug and performance monitoring > features back to group 1. All except the hard lockup detector will have > logic to fall back statically. This means making it dynamic shouldn't be > that hard. However since there is no code in the upstream kernel that > would use the code I don't plan to go there myself. I don't think this needs to by dynamic, but from a user perspective a config option would be really nice. Tim
On Fri, Nov 28, 2014 at 10:10:04AM +0100, Tim Sander wrote: > Hi Daniel, Russell > > Am Mittwoch, 26. November 2014, 16:17:06 schrieb Daniel Thompson: > > On 26/11/14 13:12, Russell King - ARM Linux wrote: > > > On Wed, Nov 26, 2014 at 01:46:52PM +0100, Tim Sander wrote: > > >> Hi Daniel > > >> > > >> Am Dienstag, 25. November 2014, 17:26:41 schrieb Daniel Thompson: > > >>> Previous changes have introduced both a replacement default FIQ handler > > >>> and an implementation of arch_trigger_all_cpu_backtrace for ARM but > > >>> these are currently independent of each other. > > >>> > > >>> This patch plumbs together these features making it possible, on > > >>> platforms > > >>> that support it, to trigger backtrace using FIQ. > > >> > > >> Does this ipi handler interfere in any way with set_fiq_handler? > > >> > > >> As far as i know there is only one FIQ handler vector so i guess there is > > >> a > > >> potential conflict. But i have not worked with IPI's so i might be > > >> completley wrong. > > > > > > First, the code in arch/arm/kernel/fiq.c should work with this new FIQ > > > code in that the new FIQ code is used as the "default" handler (as > > > opposed to the original handler which was a total no-op.) > > > > > > Secondly, use of arch/arm/kernel/fiq.c in a SMP system is really not a > > > good idea: the FIQ registers are private to each CPU in the system, and > > > there is no infrastructure to allow fiq.c to ensure that it loads the > > > right CPU with the register information for the provided handler. > Well given the races in the GIC v1. i have seen in the chips on my desk > initializing with for_each_possible_cpu(cpu) work_on_cpu(cpu,..) is rather > easy. > > > So, use of arch/arm/kernel/fiq.c and the IPI's use of FIQ /should/ be > > > mutually exclusive. > Yes but i digress on the assessment that this a decision between SMP and non- > SMP usage or the availbility of the GIC. The two things are mutually exclusive. You can either have FIQ being used for debug purposes, where we decode the FIQ reason and call some function (which means that we will only service one FIQ at a time) or you can use it in exclusive mode (provided by fiq.c) where your handler has sole usage of the vector, and benefits from fast and immediate servicing of the event. You can't have fast and immediate servicing of the event _and_ debug usage at the same time. > Well i am not against these features as they assumably improve the backtrace, > but it would be nice to have a config option which switches between > set_fiq_handler usage and the other conflicting usages of the fiq. You have a config option already. CONFIG_FIQ.
diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h index 18f5a554134f..b076584ac0fa 100644 --- a/arch/arm/include/asm/smp.h +++ b/arch/arm/include/asm/smp.h @@ -18,6 +18,8 @@ # error "<asm/smp.h> included in non-SMP build" #endif +#define SMP_IPI_FIQ_MASK 0x0100 + #define raw_smp_processor_id() (current_thread_info()->cpu) struct seq_file; @@ -79,6 +81,7 @@ extern void arch_send_call_function_single_ipi(int cpu); extern void arch_send_call_function_ipi_mask(const struct cpumask *mask); extern void arch_send_wakeup_ipi_mask(const struct cpumask *mask); +extern void ipi_cpu_backtrace(struct pt_regs *regs); extern int register_ipi_completion(struct completion *completion, int cpu); struct smp_operations { diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 14c594a12bef..e923843562d9 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -539,7 +539,7 @@ static void ipi_cpu_stop(unsigned int cpu) cpu_relax(); } -static void ipi_cpu_backtrace(struct pt_regs *regs) +void ipi_cpu_backtrace(struct pt_regs *regs) { int cpu = smp_processor_id(); @@ -580,6 +580,8 @@ void handle_IPI(int ipinr, struct pt_regs *regs) unsigned int cpu = smp_processor_id(); struct pt_regs *old_regs = set_irq_regs(regs); + BUILD_BUG_ON(SMP_IPI_FIQ_MASK != BIT(IPI_CPU_BACKTRACE)); + if ((unsigned)ipinr < NR_IPI) { trace_ipi_entry(ipi_types[ipinr]); __inc_irq_stat(cpu, ipi_irqs[ipinr]); diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 4dc45b38e56e..9eb05be9526e 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -483,6 +483,9 @@ asmlinkage void __exception_irq_entry handle_fiq_as_nmi(struct pt_regs *regs) #ifdef CONFIG_ARM_GIC gic_handle_fiq_ipi(); #endif +#ifdef CONFIG_SMP + ipi_cpu_backtrace(regs); +#endif nmi_exit();
Previous changes have introduced both a replacement default FIQ handler and an implementation of arch_trigger_all_cpu_backtrace for ARM but these are currently independent of each other. This patch plumbs together these features making it possible, on platforms that support it, to trigger backtrace using FIQ. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> --- arch/arm/include/asm/smp.h | 3 +++ arch/arm/kernel/smp.c | 4 +++- arch/arm/kernel/traps.c | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-)