On 09/05/16 16:10, Marc Zyngier wrote:

On 09/05/16 14:13, Jon Hunter wrote:

quoted

On 09/05/16 13:23, Marc Zyngier wrote:

[snip]

quoted

quoted

This patch have the effect of making misconfigured PPIs absolutely
obvious. I still need to wrap my head around the root cause, but here's
the findings I have so far:

- kvmtool generates a DT with the wrong trigger information (edge
instead of level) for the timer.
- with this patch applied, "cyclictest -S" reliably locks up when run in
a guest (missing a timer interrupt, goodbye CPU).
- Either fixing kvmtool or reverting that patch makes it work reliably
again.

My gut feeling is that until that patch, the failing irq_set_irq_type()
wasn't affecting the kernel's view of the trigger (it was still treated
as level). With this patch, the kernel now trusts whatever is coming
from the firmware, and the misconfiguration becomes obvious. And just
grepping through the DT files for arm and arm64 sends makes me thing
"Holly effin' crap!".

I'm not saying that we shouldn't perform this change though. But it is
quite obvious that it is going to break an awful lot of existing code
and platforms. I'm also cooking a small patch for the arch timer (which
seems to be described in DT with a fairly high level of brokenness), so
that we can mop-up most of the brain damage.

Hmmm ... yes I see. I wonder if we should make the setting of the type
here dependent upon PM being enabled for an irqchip? We could check to
see if the .parent_device is populated and if so only then save the type
and otherwise just set it as we do today.

I don't really like the idea of having multiple code paths for the same thing.
This is very error prone, and likely to bitrot pretty quickly.

True. However, we really need this change for irqchips and runtime-pm.
So to confirm what are you suggesting we do? We could add a WARN around
irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how
many complaints we get :-)

quoted

We could add a WARN to the existing irq_set_irq_type() or may be just a
pr_warn() if a WARN is too verbose so people can fix up any issues.

I am also wondering if patch 4/17 "iqdomain: Fix handling of type
settings for existing mappings" could generate a lot of reports
interrupts failing due to bad firmware? I wonder if I should tone this
patch down to a warning message as well as opposed to a complete failure.

We'll see. We can always tone it down a notch, should it prove to be too noisy...
So far, I haven't seen it firing. On the other hand, I get the following stuff
on my APM board:

[    0.000000] GIC: PPI0 is either secure or misconfigured
[    0.000000] GIC: PPI13 is either secure or misconfigured
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware

Pretty awesome...

Indeed.

Jon