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@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>> Reviewed-by: Zi Shen Lim <zlim@xxxxxxxxxxxx>
>>> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>>> Acked-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>> ---
>>> 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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
