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

[wuyun.wu@huawei.com (Abel)]

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@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@linutronix.de>
>>> Reviewed-by: Zi Shen Lim <zlim@broadcom.com>
>>> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@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.
As for the interrupts here, the ones entered into this 'if' clause, I think
they are expected already been mapped into this irq_domain.

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

> 
>>> +
>>> +     gic_dist_config(base, gic_data.irq_nr, gic_dist_wait_for_rwp);
>>> +
>>> +     /* Enable distributor with ARE, Group1 */
>>> +     writel_relaxed(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1,
>>> +                    base + GICD_CTLR);
>>> +
>>> +     /*
>>> +      * Set all global interrupts to the boot CPU only. ARE must be
>>> +      * enabled.
>>> +      */
>>> +     affinity = gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id()));
>>> +     for (i = 32; i < gic_data.irq_nr; i++)
>>> +             writeq_relaxed(affinity, base + GICD_IROUTER + i * 8);
>>> +}
>>> +
> 
> [...]
> 
>>> +static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>>> +                           irq_hw_number_t hw)
>>> +{
>>> +     /* SGIs are private to the core kernel */
>>> +     if (hw < 16)
>>> +             return -EPERM;
>>> +     /* PPIs */
>>> +     if (hw < 32) {
>>> +             irq_set_percpu_devid(irq);
>>> +             irq_set_chip_and_handler(irq, &gic_chip,
>>> +                                      handle_percpu_devid_irq);
>>> +             set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
>>> +     }
>>> +     /* SPIs */
>>> +     if (hw >= 32 && hw < gic_data.irq_nr) {
>>> +             irq_set_chip_and_handler(irq, &gic_chip,
>>> +                                      handle_fasteoi_irq);
>>> +             set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>>> +     }
>>
>>
>> Use 'else' here, since the conditions are non-overlapping.
> 
> For the code as it is now, I'd agree. But I'm also looking at the ITS
> code that will follow as soon as this one is on its way to mainline, and
> I then have this:
> 
> @@ -553,6 +571,19 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>                                          handle_fasteoi_irq);
>                 set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>         }
> +       /* Nothing */
> +       if (hw >= gic_data.irq_nr && hw < 8192)
> +               return -EPERM;
> +       /* LPIs */
> +       if (hw >= 8192 && hw < GIC_ID_NR) {
> +               if (!its_chip)
> +                       return -EPERM;
> +               irq_set_chip_and_handler(irq, its_chip, handle_fasteoi_irq);
> +               set_irq_flags(irq, IRQF_VALID);
> +       }
> +       /* Off limits */
> +       if (hw >= GIC_ID_NR)
> +               return -EPERM;
>         irq_set_chip_data(irq, d->host_data);
>         return 0;
>  }
> 
> Having a bunch of else clauses looks pretty awful in this context, and
> I've decided to try and keep it readable instead. This is not on the hot
> path anyway, so it doesn't really matter if we test an extra variable.

OK, it makes sense.

Regards,
Abel.