On Tue, Jun 10 2014 at 4:57:03 am BST, Abel <wuyun.wu@xxxxxxxxxx> wrote:
> On 2014/6/9 16:41, Marc Zyngier wrote:
>
>> On Mon, Jun 09 2014 at 5:10:30 am BST, Abel <wuyun.wu@xxxxxxxxxx> wrote:
>>> 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.
Not that I know of. But consider this cheap defensive programming. It is
actually cheaper to check against a constant boundary than do a load and
check against a variable. Safer, faster.
> b) Why 1021?
The maximum SPI ID is 1020.
>
>>>>
>>>>>> + 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.
Well, I guess that since this is a one-off thing, it doesn't really add
an extra synchronization. As long as it gives you peace of mind... ;-)
Thanks,
M.
--
Jazz is not dead. It just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
