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

[Abel]

On 2014/6/9 16:41, Marc Zyngier wrote:

> 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).


Understood.


>> 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.


Yes, makes sense. This case should be properly handled.
And I still have two questions.
a) Does this scenario really exist or necessary? I mean, part of interrupts
   mapped while others which belongs to the same interrupt controller are not.
b) Why 1021?


>>>
>>>>> +                     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.

It's reasonable to think GIC having been enabled by boot loader or something else
in the boot procedure. And here not waiting for RWP may cause hardware implementation
defined behavior, that is configuring an enabled interrupt. It's beyond the scope of
the GIC architecture, and vendors also don't want to see this happen.

Thanks,
Abel.