Re: [RFC PATCH v3 07/18] xen/arm: ITS: implement hw_irq_controller for LPIs

[Julien Grall <julien.grall@citrix.com>]

On 02/07/15 13:59, Ian Campbell wrote:
> On Thu, 2015-07-02 at 18:14 +0530, Vijay Kilari wrote:
>> On Thu, Jul 2, 2015 at 6:05 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
>>> On Thu, 2015-07-02 at 17:51 +0530, Vijay Kilari wrote:
>>>> On Mon, Jun 29, 2015 at 5:29 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
>>>>> On Tue, 2015-06-23 at 15:32 +0100, Julien Grall wrote:
>>>>> [...]
>>>>>>> +{
>>>>>>> +    struct its_collection *col;
>>>>>>> +    struct its_device *its_dev = get_irq_device(desc);
>>>>>>> +    u8 *cfg;
>>>>>>> +    u32 virq = irq_to_virq(desc);
>>>>>>> +
>>>>>>> +    ASSERT(virq < its_dev->nr_lpis);
>>>>>>> +
>>>>>>> +    cfg = gic_rdists->prop_page + desc->irq - NR_GIC_LPI;
>>>>>>> +    if ( enable )
>>>>>>> +        *cfg |= LPI_PROP_ENABLED;
>>>>>>> +    else
>>>>>>> +        *cfg &= ~LPI_PROP_ENABLED;
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Make the above write visible to the redistributors.
>>>>>>> +     * And yes, we're flushing exactly: One. Single. Byte.
>>>>>>> +     * Humpf...
>>>>>>> +     */
>>>>>>> +    if ( gic_rdists->flags & RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING )
>>>>>>> +        clean_and_invalidate_dcache_va_range(cfg, sizeof(*cfg));
>>>>>>> +    else
>>>>>>> +        dsb(ishst);
>>>>>>> +
>>>>>>> +    /* Get collection id for this event id */
>>>>>>> +    col = &its_dev->its->collections[virq % num_online_cpus()];
>>>>>>
>>>>>> This is fragile, you are assuming that num_online_cpus() will never
>>>>>> change. Why don't you store the collection in every irq_desc?
>>>>>
>>>>> The original Linux code upon which this is based doesn't seem to need to
>>>>> lookup the collection here, why is flushing needed for us but not Linux?
>>>>
>>>> We are writing to lpi property table. Even linux code flushes it.
>>>
>>> Sorry I was referring to the collection look up and inv, not the cache
>>> flush, i.e. this bit:
>>>
>>> +    /* Get collection id for this event id */
>>> +    col = &its_dev->its->collections[virq % num_online_cpus()];
>>> +    its_send_inv(its_dev, col, virq);
>>>
>>> Linux doesn't seem to do that INV there.
>>
>> Linux does INV.
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-v3-its.c?id=refs/tags/v4.1
>>
>> line 555
> 
> So it does, not sure how I missed that when I first looked.
> 
> Linux's approach of saving collection in the its_dev seems preferable to
> looking it up like this here though.

That would be a better idea. We have too many place in this series where
the way to retrieve the collection for an interrupt.

Hence, the irq_desc should have a pointer to its_dev (as suggested in
another patch).

struct device would even be better, but I can live with its_dev for now.

Regards,

-- 
Julien Grall