[PATCH v4 02/20] arm64: initial support for GICv3

[Marc Zyngier]

On Mon, Jun 09 2014 at  5:10:30 am BST, Abel <wuyun.wu at huawei.com> wrote:
> On 2014/6/5 16:44, Marc Zyngier wrote:
>
>> Hi Abel,
>> 
>> On Thu, Jun 05 2014 at  8:47:49 am BST, Abel <wuyun.wu at huawei.com> wrote:
>>> Hi Marc,
>>>
>>> On 2014/5/16 1:58, Marc Zyngier wrote:
>>>
>>>> The Generic Interrupt Controller (version 3) offers services that are
>>>> similar to GICv2, with a number of additional features:
>>>> - Affinity routing based on the CPU MPIDR (ARE)
>>>> - System register for the CPU interfaces (SRE)
>>>> - Support for more that 8 CPUs
>>>> - Locality-specific Peripheral Interrupts (LPIs)
>>>> - Interrupt Translation Services (ITS)
>>>>
>>>> This patch adds preliminary support for GICv3 with ARE and SRE,
>>>> non-secure mode only. It relies on higher exception levels to grant ARE
>>>> and SRE access.
>>>>
>>>> Support for LPI and ITS will be added at a later time.
>>>>
>>>> Cc: Thomas Gleixner <tglx at linutronix.de>
>>>> Reviewed-by: Zi Shen Lim <zlim at broadcom.com>
>>>> Reviewed-by: Christoffer Dall <christoffer.dall at linaro.org>
>>>> Acked-by: Catalin Marinas <catalin.marinas at arm.com>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>>>> ---
>>>>  arch/arm64/Kconfig                 |   1 +
>>>>  arch/arm64/kernel/head.S           |  18 +
>>>>  arch/arm64/kernel/hyp-stub.S       |   1 +
>>>>  drivers/irqchip/Kconfig            |   5 +
>>>>  drivers/irqchip/Makefile           |   1 +
>>>>  drivers/irqchip/irq-gic-v3.c | 684
>>>> +++++++++++++++++++++++++++++++++++++
>>>>  include/linux/irqchip/arm-gic-v3.h | 190 +++++++++++
>>>>  7 files changed, 900 insertions(+)
>>>>  create mode 100644 drivers/irqchip/irq-gic-v3.c
>>>>  create mode 100644 include/linux/irqchip/arm-gic-v3.h
>>>>
> [...]
>>>> +static asmlinkage void __exception_irq_entry
>>>> gic_handle_irq(struct pt_regs *regs)
>>>> +{
>>>> +     u64 irqnr;
>>>> +
>>>> +     do {
>>>> +             irqnr = gic_read_iar();
>>>> +
>>>> +             if (likely(irqnr > 15 && irqnr < 1021)) {
>>>
>>>
>>> I prefer to use gic_data.irq_nr instead of '1021'.
>>> The ranges here should be consistent with the ones in domain.map.
>> 
>> That's debatable. gic_data.irq_nr is used as a guard for the rest of the
>> kernel, so that we don't map an out-of-range interrupt number. But here,
>> we read the interrupt that has been generated by the actual HW. If such
>> thing happen, we need to know about it, handle it as a spurious
>> interrupt, and EOI it. Otherwise, we're stuck forever (with your
>> proposed change, nothing EOIs the interrupt).
>
>
> I don't think it's necessary to EOI the spurious interrupts, since EOIing
> a spurious interrupt won't cause any effect.

My choice of word was rather poor in this context. I wasn't thinking of
a spurious interrupt", as defined in the GIC architecture (where
ICC_IAR_EL1 returns 0x3ff), but rather of a valid interrupt number
(between irq_nr and 1021).

> As for the interrupts here, the ones entered into this 'if' clause, I
> think they are expected already been mapped into this irq_domain.

Mapped or not is not the problem. Think of a (broken) implementation
that would generate random IDs in the irq_nr..1021 range. You'd end up
with the deadlock situation I've outlined above.

And actually, irq_find_mapping can fail, and I should handle this.

>> 
>>>> +                     irqnr = irq_find_mapping(gic_data.domain, irqnr);
>>>> +                     handle_IRQ(irqnr, regs);
>>>> +                     continue;
>>>> +             }
>>>> +             if (irqnr < 16) {
>>>> +                     gic_write_eoir(irqnr);
>>>> +#ifdef CONFIG_SMP
>>>> +                     handle_IPI(irqnr, regs);
>>>> +#else
>>>> +                     WARN_ONCE(true, "Unexpected SGI received!\n");
>>>> +#endif
>>>> +                     continue;
>>>> +             }
>>>> +     } while (irqnr != 0x3ff);
>>>> +}
>>>> +
>>>> +static void __init gic_dist_init(void)
>>>> +{
>>>> +     unsigned int i;
>>>> +     u64 affinity;
>>>> +     void __iomem *base = gic_data.dist_base;
>>>> +
>>>> +     /* Disable the distributor */
>>>> +     writel_relaxed(0, base + GICD_CTLR);
>>>
>>>
>>> I'm not sure whether waiting for write pending here is
>>> necessary. What do you think?
>> 
>> Hmmm. That's a good point. The spec says the RWP tracks the completion
>> of writes that change (among other things) the state of an interrupt
>> group enable. But the next line will perform a wait_for_rwp anyway after
>> having disabled all SPIs. So no, I don't think we need to wait. We
>> guaranteed to get a consistent state after the call to gic_dist_config.
>
>
> The spec also says that software must ensure the interrupt is disabled before
> changing its routing/priority/group information, see chapter 4.5.5 in v18.

That's only to ensure consistency when a single interrupt is being
reconfigured, and that's what the code already does. In this particular
context, waiting for RWP looks completely superfluous.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.