On Mon, Jun 09 2014 at 5:10:30 am BST, Abel <wuyun.wu@xxxxxxxxxx> 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@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.
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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
