by Daniel Thompson

[permalink] [raw]

Hi Russell, Thomas

I have some replys below, but i just post my most important question up here,
which is my current takeaway from this discussion:
Would patches be accepted which -as Daniel Thompson pointed out- dynamically
switch the FIQ IPI's off when set_fiq_handler is called (given that the FIQ IPI
patches are to be merged proper beforhand).

Am Montag, 1. Dezember 2014, 15:02:40 schrieb Russell King - ARM Linux:
> On Mon, Dec 01, 2014 at 02:54:10PM +0100, Tim Sander wrote:
> > Hi Russell
> >
> > Am Montag, 1. Dezember 2014, 10:38:32 schrieb Russell King - ARM Linux:
> > > That whole paragraph doesn't make much sense to me.
> > >
> > > Look, in my mind it is very simple. If you are using CONFIG_FIQ on a
> > > SMP platform, your life will be very difficult. The FIQ code enabled
> > > by that symbol is not designed to be used on SMP systems, *period*.
> >
> > Well the only extra thing you had to do is set up the FIQ registers on
> > every cpu, but i would not call that very difficult. Other than that i am
> > not aware of any problems that are not also present on a uniprocessor
> > system. So i have a hard time following your reasoning why SMP is
> > different from UP in regard to the CONFIG_FIQ.
>
> One of the things which FIQ handlers can do is they have their own private
> registers which they can modify on each invocation of the FIQ handler -
> for example, as a software DMA pointer.
>
> Each CPU has its own private set of FIQ registers, so merely copying the
> registers to each CPU will only set their initial state: updates by one
> CPU to the register set will not be seen by a different CPU.
>
> > > If you decide to enable CONFIG_FIQ, and you use that code on a SMP
> > > platform, I'm going to say right now so it's totally clear: if you
> > > encounter a problem, I don't want to know about it. The code is not
> > > designed for use on that situation.
> >
> > Even with using the FIQ on a Linux SMP system you have not heard from me
> > before, as i knew that this is not your problem (and that is not to say
> > that there where none!). The only interface Linux has been making
> > available is set_fiq_handler. So it was clear that the FIQ is its own
> > domain otherwise untouched by the kernel.
>
> Correct, because FIQs have very little use in Linux. They have been used
> in the past to implement:
> - software DMA to floppy disk controllers (see arch/arm/lib/floppydma.S)
> - audio DMA (arch/arm/mach-imx/ssi-fiq.S)
> - s2c24xx SPI DMA (drivers/spi/spi-s3c24xx-fiq.S)
> - Keyboard (yes, how that qualifies for FIQ I don't know
> (arch/arm/mach-omap1/ams-delta-fiq-handler.S)
>
> The first three do exactly what I describe above, and none of these users
> are SMP platforms. Hence, the FIQ code which we currently have does exactly
> what we need it to for the platforms we have.
>
> Now, you're talking about using this in a SMP context - that's a totally
> new use for this code which - as I have said several times now - is not
> really something that this code is intended to support.
Yes but as i said the only additional problem is the seperate registers for
each core. Given the quirks the current GIC version 1 this is really a minor
problem:
https://lkml.org/lkml/2014/7/15/550

> > And i really don't get it, that neither ARM nor the kernel community
> > sees fast interrupts as a worthwhile usecase. Unfortunatly the interrupt
> > latencies with Linux are at least a order of magnitude higher than the
> > pure hardware even with longer pipelines can deliver.
>
> First point: fast interrupts won't be fast if you load them up with all
> the interrupt demux and locking that normal interrupts have; if you
> start doing that, then you end up having to make /all/ IRQ-safe locks
> in the kernel not only disable normal interrupts, but also disable the
> FIQs as well.
I just want to have an CPU context where IRQ's are not switched off by Linux.
It would be nice to use Linux infrastructure like printk but thats just not
that important. And no, i don't wan't to use some IRQ demuxing, Thats why
i would be nice to disable the FIQ IPI's dynamically if other uses are set.

> At that point, FIQs are no longer "fast" - they will be subject to
> exactly the same latencies as normal interrupts.
Well the main difference i am after, is to have one interrupt which is not
masked in any way and which is as fast as the hardware can get (which on a
cortex a9 is depending on implementation between 500ns to a couple of ?s).

> Second point: we have embraced FIQs where it is appropriate to do so,
> but within the restrictions that FIQs present - that is, to keep them
> fast, we have to avoid the problem in the first point above, which
> means normal C code called from FIQs /can't/ take any kernel lock what
> so ever without potentially causing a deadlock.
Yes i am aware of that. I think thats one of the main reasons why the FIQ has
been mainly unused by Linux.

> Even if you think you can (why would UP have locks if it's not SMP)
> debugging facilities such as the lock validator will bite you if you
> try taking a lock in FIQ context which was already taken in the parent
> context.
Well no Linux context in FIQ at all. Thats why i was using a daisy chained
normal interrupt to hand of the normal stuff in linux context.

> Third point: FIQs are not available on a lot of ARM platforms. Hardware
> which routes interrupts to FIQs is very limited, normally it's only a
> few interrupts which appear there. Moreover, with more modern platforms
> where the kernel runs in the non-secure side, FIQs are /totally/ and
> /completely/ unavailable there - FIQs are only available for the secure
> monitor to use.
I am fully aware that ARM started to mix up FIQ and SecureMode, confusing even
some silicon vendors, which sadly have the FIQ missing. But aside from that i
know that i.mx6, xilinx zynq, altera soc all have a FIQ available. The only
one i know missing the FIQ is the Sitara which had the FIQ documented in first
revisions of the spec (but has not anymore). So from my totally empirical
unscientific view 3 of 4 cpus have FIQ functionality.

Or do you mean that the platform is capable of delivering FIQ but have no
CONFIG_FIQ set. In that case there is indeed only a small fraction which has
this config option in use.

> > > No. With a single zImage kernel, you could very well have SMP and FIQ
> > > both enabled, but have a non-SMP platform using FIQ, but also support
> > > SMP platforms as well. Your change prevents that happening.
> >
> > Ah, well i have to get used to this "new" devicetree thingy, where one
> > size
> > fits all...
>
> No, you're conflating different things there. It doesn't have much
> to do with DT vs non-DT, because this same problem existed before DT
> came along, since there were platforms which could be both UP and SMP.
D'accord.

> > Still if you boot a single process system which has FIQ available and has
> > a
> > GIC with such a kernel, then you also reprogramm the IPI's as FIQs. But i
> > guess thats not a problem as Linux does not self IPI the kernel as other
> > os'es do?
>
> I'm really sorry, but your above paragraph doesn't make much sense to me.
> "single process system" - if there's only one process, there's no point
> having a scheduler (it has nothing to schedule) and so I guess you're
> not talking about Linux there.
>
> Or do you mean "single processor system" (in other words, uniprocessor or
> UP). In that case, the kernel doesn't use IPIs, because, by definition,
> there's no other processors for it to signal to.
I am sorry, mark that to beeing a non native english speaker, i indeed meant a
single processor system as with a single process i would definetly not bother
to run linux at all.

Best regards
Tim

2014-12-08 16:01:06

by Daniel Thompson

[permalink] [raw]

Subject: Re: [PATCH 3.18-rc4 v12 0/5] arm: Implement arch_trigger_all_cpu_backtrace

On 28/11/14 16:16, Daniel Thompson wrote:
> Hi Thomas, Hi Jason: Patches 1 to 3 are for you.
>
> This patchset modifies the GIC driver to allow it, on supported
> platforms, to route IPI interrupts to FIQ and uses this feature to
> implement arch_trigger_all_cpu_backtrace for arm.

Are the irq parts of this patchset looking OK now?

Daniel.

>
> On platforms not capable of supporting FIQ the signal to generate a
> backtrace we fall back to using IRQ for propagation instead (relying on
> a timeout to avoid wedging the CPU requesting the backtrace if other
> CPUs are not responsive).
>
> It has been tested on two systems capable of supporting grouping
> (Freescale i.MX6 and STiH416) and two that do not (vexpress-a9 and
> Qualcomm Snapdragon 600).
>
> v12:
>
> * Squash first two patches into a single one and re-describe
> (Thomas Gleixner).
>
> * Improve description of "irqchip: gic: Make gic_raise_softirq FIQ-safe"
> (Thomas Gleixner).
>
> v11:
>
> * Optimized gic_raise_softirq() by replacing a register read with
> a memory read (Jason Cooper).
>
> v10:
>
> * Add a further patch to optimize away some of the locking on systems
> where CONFIG_BL_SWITCHER is not set (Marc Zyngier). Compiles OK with
> exynos_defconfig (which is the only defconfig to set this option).
>
> * Whitespace fixes in patch 4. That patch previously used spaces for
> alignment of new constants but the rest of the file used tabs.
>
> v9:
>
> * Improved documentation and structure of initial patch (now initial
> two patches) to make gic_raise_softirq() safe to call from FIQ
> (Thomas Gleixner).
>
> * Avoid masking interrupts during gic_raise_softirq(). The use of the
> read lock makes this redundant (because we can safely re-enter the
> function).
>
> v8:
>
> * Fixed build on arm64 causes by a spurious include file in irq-gic.c.
>
> v7-2 (accidentally released twice with same number):
>
> * Fixed boot regression on vexpress-a9 (reported by Russell King).
>
> * Rebased on v3.18-rc3; removed one patch from set that is already
> included in mainline.
>
> * Dropped arm64/fiq.h patch from the set (still useful but not related
> to issuing backtraces).
>
> v7:
>
> * Re-arranged code within the patch series to fix a regression
> introduced midway through the series and corrected by a later patch
> (testing by Olof's autobuilder). Tested offending patch in isolation
> using defconfig identified by the autobuilder.
>
> v6:
>
> * Renamed svc_entry's call_trace argument to just trace (example code
> from Russell King).
>
> * Fixed mismatched ENDPROC() in __fiq_abt (example code from Russell
> King).
>
> * Modified usr_entry to optional avoid calling into the trace code and
> used this in FIQ entry from usr path. Modified corresponding exit code
> to avoid calling into trace code and the scheduler (example code from
> Russell King).
>
> * Ensured the default FIQ register state is restored when the default
> FIQ handler is reinstalled (example code from Russell King).
>
> * Renamed no_fiq_insn to dfl_fiq_insn to reflect the effect of adopting
> a default FIQ handler.
>
> * Re-instated fiq_safe_migration_lock and associated logic in
> gic_raise_softirq(). gic_raise_softirq() is called by wake_up_klogd()
> in the console unlock logic.
>
> v5:
>
> * Rebased on 3.17-rc4.
>
> * Removed a spurious line from the final "glue it together" patch
> that broke the build.
>
> v4:
>
> * Replaced push/pop with stmfd/ldmfd respectively (review of Nicolas
> Pitre).
>
> * Really fix bad pt_regs pointer generation in __fiq_abt.
>
> * Remove fiq_safe_migration_lock and associated logic in
> gic_raise_softirq() (review of Russell King)
>
> * Restructured to introduce the default FIQ handler first, before the
> new features (review of Russell King).
>
> v3:
>
> * Removed redundant header guards from arch/arm64/include/asm/fiq.h
> (review of Catalin Marinas).
>
> * Moved svc_exit_via_fiq macro to entry-header.S (review of Nicolas
> Pitre).
>
> v2:
>
> * Restructured to sit nicely on a similar FYI patchset from Russell
> King. It now effectively replaces the work in progress final patch
> with something much more complete.
>
> * Implemented (and tested) a Thumb-2 implementation of svc_exit_via_fiq
> (review of Nicolas Pitre)
>
> * Dropped the GIC group 0 workaround patch. The issue of FIQ interrupts
> being acknowledged by the IRQ handler does still exist but should be
> harmless because the IRQ handler will still wind up calling
> ipi_cpu_backtrace().
>
> * Removed any dependency on CONFIG_FIQ; all cpu backtrace effectively
> becomes a platform feature (although the use of non-maskable
> interrupts to implement it is best effort rather than guaranteed).
>
> * Better comments highlighting usage of RAZ/WI registers (and parts of
> registers) in the GIC code.
>
> Changes *before* v1:
>
> * This patchset is a hugely cut-down successor to "[PATCH v11 00/19]
> arm: KGDB NMI/FIQ support". Thanks to Thomas Gleixner for suggesting
> the new structure. For historic details see:
> https://lkml.org/lkml/2014/9/2/227
>
> * Fix bug in __fiq_abt (no longer passes a bad struct pt_regs value).
> In fixing this we also remove the useless indirection previously
> found in the fiq_handler macro.
>
> * Make default fiq handler "always on" by migrating from fiq.c to
> traps.c and replace do_unexp_fiq with the new handler (review
> of Russell King).
>
> * Add arm64 version of fiq.h (review of Russell King)
>
> * Removed conditional branching and code from irq-gic.c, this is
> replaced by much simpler code that relies on the GIC specification's
> heavy use of read-as-zero/write-ignored (review of Russell King)
>
>
> Daniel Thompson (5):
> irqchip: gic: Optimize locking in gic_raise_softirq
> irqchip: gic: Make gic_raise_softirq FIQ-safe
> irqchip: gic: Introduce plumbing for IPI FIQ
> ARM: add basic support for on-demand backtrace of other CPUs
> arm: smp: Handle ipi_cpu_backtrace() using FIQ (if available)
>
> arch/arm/include/asm/irq.h | 5 +
> arch/arm/include/asm/smp.h | 3 +
> arch/arm/kernel/smp.c | 64 +++++++++++++
> arch/arm/kernel/traps.c | 8 +-
> drivers/irqchip/irq-gic.c | 203 +++++++++++++++++++++++++++++++++++++---
> include/linux/irqchip/arm-gic.h | 8 ++
> 6 files changed, 275 insertions(+), 16 deletions(-)
>
> --
> 1.9.3
>