On 2014/6/10 18:43, Marc Zyngier wrote:
> 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.
Yes, sure.
>
>> b) Why 1021?
>
> The maximum SPI ID is 1020.
Oooh.. SPI ends at 1019, and 1020 is for special purpose.
>
>>
>>>>>
>>>>>>> + 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... ;-)
>
OK, then let's keep it what it was. :)
Thanks,
Abel.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
